| Message ID | 20231211071204.30156-3-tomoyuki.hirose@igel.co.jp |
|---|---|
| State | New |
| Headers | show |
| Series | support unaligned access for some xHCI registers | expand |
On Mon, 11 Dec 2023 at 07:13, Tomoyuki HIROSE <tomoyuki.hirose@igel.co.jp> wrote: > > According to xHCI spec rev 1.2, unaligned access to xHCI Host > Controller Capability Registers is not prohibited. In Addition, the > limit of access size is also unspecified. Actually, some real devices > allow unaligned access and 8-byte access to these registers. > This commit makes it possible to unaligned access and 8-byte access > to Host Controller Capability Registers. We should definitely look at fixing the unaligned access stuff, but the linked bug report is not trying to do an unaligned access -- it wants to do a 2-byte read from offset 2, which is aligned. The capability registers in the xHCI spec are also all at offsets and sizes that mean that a natural read of them is not unaligned. thanks -- PMM
Thanks for comment. On Mon, Dec 11, 2023 at 10:57 PM Peter Maydell <peter.maydell@linaro.org> wrote: > We should definitely look at fixing the unaligned access > stuff, but the linked bug report is not trying to do an > unaligned access -- it wants to do a 2-byte read from offset 2, > which is aligned. The capability registers in the xHCI spec > are also all at offsets and sizes that mean that a natural > read of them is not unaligned. Shouldn't I link this bug report? Or is it not appropriate to allow unaligned access? thanks, Tomoyuki HIROSE
On Tue, 12 Dec 2023 at 01:43, Tomoyuki Hirose <tomoyuki.hirose@igel.co.jp> wrote: > > Thanks for comment. > > On Mon, Dec 11, 2023 at 10:57 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > We should definitely look at fixing the unaligned access > > stuff, but the linked bug report is not trying to do an > > unaligned access -- it wants to do a 2-byte read from offset 2, > > which is aligned. The capability registers in the xHCI spec > > are also all at offsets and sizes that mean that a natural > > read of them is not unaligned. > > Shouldn't I link this bug report? > Or is it not appropriate to allow unaligned access? The bug report is definitely relevant. But depending on how tricky the unaligned access handling turns out to be to get right, we might be able to fix the bug by permitting aligned-but-not-4-bytes accesses. (I'm a bit surprised that doesn't work already, in fact: we use it in other devices.) thanks -- PMM
On Tue, Dec 12, 2023 at 7:26 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Tue, 12 Dec 2023 at 01:43, Tomoyuki Hirose > <tomoyuki.hirose@igel.co.jp> wrote: > > > > Thanks for comment. > > > > On Mon, Dec 11, 2023 at 10:57 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > > We should definitely look at fixing the unaligned access > > > stuff, but the linked bug report is not trying to do an > > > unaligned access -- it wants to do a 2-byte read from offset 2, > > > which is aligned. The capability registers in the xHCI spec > > > are also all at offsets and sizes that mean that a natural > > > read of them is not unaligned. > > > > Shouldn't I link this bug report? > > Or is it not appropriate to allow unaligned access? > > The bug report is definitely relevant. But depending > on how tricky the unaligned access handling turns out to > be to get right, we might be able to fix the bug by > permitting aligned-but-not-4-bytes accesses. (I'm > a bit surprised that doesn't work already, in fact: > we use it in other devices.) > > thanks > -- PMM Thank you for answering my question. The unaligned access handling of my patch is not so tricky. If the access is unaligned, just correct the access size and address and read the value as before. Also, it is allowed by the specifications, and byte access was possible even on real devices. Regards, Tomoyuki HIROSE
On Mon, Dec 11, 2023 at 01:57:42PM +0000, Peter Maydell wrote: > On Mon, 11 Dec 2023 at 07:13, Tomoyuki HIROSE > <tomoyuki.hirose@igel.co.jp> wrote: > > > > According to xHCI spec rev 1.2, unaligned access to xHCI Host > > Controller Capability Registers is not prohibited. In Addition, the > > limit of access size is also unspecified. Actually, some real devices > > allow unaligned access and 8-byte access to these registers. > > This commit makes it possible to unaligned access and 8-byte access > > to Host Controller Capability Registers. > > We should definitely look at fixing the unaligned access > stuff, but the linked bug report is not trying to do an > unaligned access -- it wants to do a 2-byte read from offset 2, > which is aligned. IIUC the issue is the xHCI xhci_cap_ops defines impl.min_access_size=4. Then it'll be enlarged to 4 bytes read on offset 0x2. IOW, I think it'll also work if xhci_cap_ops can support impl.min_access_size=2, however I don't know whether that can be more than that register, so that the memory patch is still a more generic approach. Thanks,
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index 4b60114207..41abeb9ac5 100644 --- a/hw/usb/hcd-xhci.c +++ b/hw/usb/hcd-xhci.c @@ -3181,9 +3181,11 @@ static const MemoryRegionOps xhci_cap_ops = { .read = xhci_cap_read, .write = xhci_cap_write, .valid.min_access_size = 1, - .valid.max_access_size = 4, + .valid.max_access_size = 8, + .valid.unaligned = true, .impl.min_access_size = 4, .impl.max_access_size = 4, + .impl.unaligned = false, .endianness = DEVICE_LITTLE_ENDIAN, };
According to xHCI spec rev 1.2, unaligned access to xHCI Host Controller Capability Registers is not prohibited. In Addition, the limit of access size is also unspecified. Actually, some real devices allow unaligned access and 8-byte access to these registers. This commit makes it possible to unaligned access and 8-byte access to Host Controller Capability Registers. Resolves: https://gitlab.com/qemu-project/qemu/-/issues/143 Signed-off-by: Tomoyuki HIROSE <tomoyuki.hirose@igel.co.jp> --- hw/usb/hcd-xhci.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)