SerenityOS
Kernel+USB: Fix USB on 64-bit platforms
This fixes the problem of USB not working on 64-bit Systems. As per @b14ckcat's idea in discord, the bookkeeping data inside of the descriptors has been moved into a struct that is allocated when the descriptor itself is created.
Is ptr64 only used on x64?
If so, why is it needed in comparison to just a void* or FlatPtr
Is ptr64 only used on x64? If so, why is it needed in comparison to just a
void*orFlatPtr
Currently, yes, however one could conceivably use this on the AArch64 port eventually.
The reason they're not FlatPtr nor void is because we need to actually dereference them. FlatPtr is typdef'd to void* for the platform it's becoming compiled to right?
I might be missing something, but if we're storing 32-bit pointers on x86 and 64-bit pointers on x64...why aren't we just storing e.g. TransferDescriptor*?
Or if we explicitly do need u32/u64, that's what FlatPtr is:
using FlatPtr = Conditional<sizeof(void*) == 8, u64, u32>;
So just the getters inside UHCIDescriptorTypes would need to do some reinterpret_casting.
FlatPtr is typdef'd to void* for the platform it's becoming compiled to right
FlatPtr is essentially ptrdiff_t. It's an integer. I wonder if VirtualAddress or PhysicalAddress solve the need here?
FlatPtr is typdef'd to void* for the platform it's becoming compiled to right
FlatPtr is essentially
ptrdiff_t. It's an integer. I wonder if VirtualAddress or PhysicalAddress solve the need here?
Same difference. Is there any point to littering the Kernel with reinterpret_cast<> when we can have an object that supports 64-bit pointer to object types that be automagically dereferenced? Perhaps I'm missing something here.
I suppose these could be raw pointers, but I'm of the assumption that we should stay away from using them for complex types as much as possible, if at all possible.
Okay, so interestingly enough, it looks like this code is the only consumer of the Ptr32<> type, and was added specifically for the UHCI Controller: https://github.com/SerenityOS/serenity/commit/7ba3c229312cddd1c3eea3e5cd493aced5cdca02
Should we perhaps remove Ptr32<> completely in this PR, then move to using one of the other types and just eat the reinterpret_cast<>'s?
I don't have strong opinion, but Ptr32 / Ptr64 feels like an overly generic solution to a very specific and unique problem.
At a cursory glance, all of the reinterpret_casting could happen in the getters and setters only, e.g. next_td() and set_next_td(), which doesn't feel like littering the kernel to me.
From what I can tell, it seems like Ptr32 was intended to be used with physical addresses so as to ensure that you don't try to assign a pointer value outside of the 4GB address range, since the hardware can't handle it and we will end up with a similar silent truncation problem that caused the original x64 crash. I think that is useful functionality to keep around where it is currently used, but for the virtual addresses within the bookkeeping struct, I think it would be ok to use regular raw pointers, since we don't care about bounding these to any particular address range.
Also correct me if I'm wrong, but VirtualAddress & PhysicalAddress are more analogous to void* or u8*, right? So those wouldn't really provide any advantage over a pointer to the actual types themselves
I don't have strong opinion, but
Ptr32/Ptr64feels like an overly generic solution to a very specific and unique problem.At a cursory glance, all of the
reinterpret_casting could happen in the getters and setters only, e.g.next_td()andset_next_td(), which doesn't feel like littering the kernel to me.
From what I can tell, it seems like Ptr32 was intended to be used with physical addresses so as to ensure that you don't try to assign a pointer value outside of the 4GB address range, since the hardware can't handle it and we will end up with a similar silent truncation problem that caused the original x64 crash. I think that is useful functionality to keep around where it is currently used, but for the virtual addresses within the bookkeeping struct, I think it would be ok to use regular raw pointers, since we don't care about bounding these to any particular address range.
Also correct me if I'm wrong, but VirtualAddress & PhysicalAddress are more analogous to void* or u8*, right? So those wouldn't really provide any advantage over a pointer to the actual types themselves
I think the solution here is going to be just making these raw pointers to the types themselves. Considering we're basically guaranteed that these won't be NULL as we error out during pool creation via UHCIDescriptorPool::ErrorOr<NonnullOwnPtr<UHCIDescriptorPool<T>>> try_create(StringView name), it's safe enough just having it non-wrapped in any special class.
I'll drop the Ptr64<> commit and make all of the pointer to objects raw pointers.