Hi Cédric, Philippe > Subject: RE: [PATCH v6 00/11] hw/usb/ehci: Add 64-bit descriptor addressing > support > > + Philippe Mathieu-Daudé <[email protected]> > > > Subject: RE: [PATCH v6 00/11] hw/usb/ehci: Add 64-bit descriptor > > addressing support > > > > Hi Cédric, Philippe > > > > > Subject: Re: [PATCH v6 00/11] hw/usb/ehci: Add 64-bit descriptor > > > addressing support > > > > > > On 7/9/26 23:09, Philippe Mathieu-Daudé wrote: > > > > On 12/5/26 16:47, Cédric Le Goater wrote: > > > >> + Gerd, Marc-André, > > > >> > > > >> May be they can provide some more feedback. > > > >> > > > >> On 5/11/26 08:13, Cédric Le Goater wrote: > > > >>> On 5/4/26 07:27, Cédric Le Goater wrote: > > > >>>> On 5/4/26 04:53, Jamin Lin wrote: > > > >>>>> EHCI supports 64-bit addressing through the CTRLDSSEGMENT > > > >>>>> register, which provides the upper 32 bits of descriptor > > > >>>>> addresses when the controller advertises 64-bit capability. > > > >>>>> > > > >>>>> Currently QEMU EHCI model only partially supports this > > > >>>>> functionality and descriptor addresses are effectively treated > > > >>>>> as 32-bit. This becomes problematic on systems where system > > > >>>>> memory is located above the 4GB boundary. > > > >>>>> > > > >>>>> The Linux EHCI driver enables 64-bit addressing if the > > > >>>>> controller advertises the capability. During initialization it > > > >>>>> programs the segment register to zero: > > > >>>>> > > > >>>>> > > > >>>>> https://github.com/torvalds/linux/blob/master/drivers/usb/host > > > >>>>> / > > > >>>>> ehci-hcd.c#L600 > > > >>>>> > > > >>>>> The driver also notes that descriptor structures allocated > > > >>>>> from the DMA pool use segment zero semantics. Descriptor > > > >>>>> memory is allocated using the DMA API and platforms may > > > >>>>> configure a 64-bit DMA mask, allowing descriptor memory to be > placed above 4GB. > > > >>>>> > > > >>>>> On AST2700 platforms, system DRAM is mapped at 0x400000000. > As > > a > > > >>>>> result, descriptor addresses constructed directly from the > > > >>>>> EHCI registers do not match the actual system addresses used > > > >>>>> by the controller when accessing queue heads (QH) and queue > > > >>>>> element transfer descriptors (qTD). > > > >>>>> > > > >>>>> This patch series implements full 64-bit descriptor addressing > > > >>>>> support in the EHCI emulation. Descriptor address handling is > > > >>>>> updated to use 64-bit values and the descriptor structures > > > >>>>> (QH, qTD, iTD and siTD) are extended to support the upper > > > >>>>> address bits provided by the segment register. > > > >>>>> > > > >>>>> Add a ctrldssegment-default property so platforms can provide > > > >>>>> a descriptor address offset when constructing descriptor addresses. > > > >>>>> This allows systems where DRAM resides above 4GB to access > > > >>>>> EHCI descriptors correctly. > > > >>>>> > > > >>>>> The AST2700 machine uses this property to account for its DRAM > > > >>>>> mapping at 0x400000000 and enables 64-bit EHCI DMA addressing. > > > >>>>> > > > >>>>> Test Result: > > > >>>>> 1. EHCI 32bits with ast2600-evb machine Command line: > > > >>>>> ./build/qemu-system-arm \ > > > >>>>> -machine ast2600-evb \ > > > >>>>> -m 1G \ > > > >>>>> -drive file=image-bmc,if=mtd,format=raw \ > > > >>>>> -nographic \ > > > >>>>> -device usb-kbd,bus=usb-bus.1,id=mykbd \ > > > >>>>> -drive id=usbdisk,if=none,file=image0.ext4,format=raw \ > > > >>>>> -device > > > >>>>> usb-storage,bus=usb-bus.1,id=mystorage,drive=usbdisk > > > >>>>> -snapshot \ > > > >>>>> -nographic > > > >>>>> Result: > > > >>>>> unable to initialize usb specBus 001 Device 001: ID 1d6b:0002 > > > >>>>> Linux 6.18.3-v00.08.01-g172b7e27a30d ehci_hcd EHCI Host > > > >>>>> Controller Bus 001 Device 002: ID 0627:0001 QEMU QEMU USB > > > >>>>> Keyboard Bus 001 Device 003: ID 46f4:0001 QEMU QEMU USB > > > >>>>> HARDDRIVE Bus 002 > > > Device > > > >>>>> 001: ID 1d6b:0001 Linux 6.18.3-v00.08.01- g172b7e27a30d > > > >>>>> uhci_hcd Generic UHCI Host Controller > > > >>>>> > > > >>>>> 2. EHCI 64bits with ast2700a2-evb machine Command line: > > > >>>>> ./build/qemu-system-aarch64 -M ast2700a2-evb -nographic\ > > > >>>>> -bios ast27x0_bootrom.bin \ > > > >>>>> -drive file=image-bmc,format=raw,if=mtd \ > > > >>>>> -snapshot \ > > > >>>>> -device usb-kbd,bus=usb-bus.3,id=mykbd \ > > > >>>>> -drive id=usbdisk,if=none,file=image0.ext4,format=raw \ > > > >>>>> -device usb-storage,bus=usb-bus.3,id=mystorage,drive=usbdisk > > > >>>>> Result: > > > >>>>> root@ast2700-default:~# lsusb > > > >>>>> unable to initialize usb specBus 001 Device 001: ID 1d6b:0001 > > > >>>>> Linux 6.18.3-v00.08.01-g172b7e27a30d uhci_hcd Generic UHCI > > > >>>>> Host Controller Bus 002 Device 001: ID 1d6b:0002 Linux > > > >>>>> 6.18.3-v00.08.01- g172b7e27a30d ehci_hcd EHCI Host Controller > > > >>>>> Bus > > > >>>>> 002 Device 002: ID 0627:0001 QEMU QEMU USB Keyboard Bus 002 > > > Device > > > >>>>> 003: ID 46f4:0001 QEMU QEMU USB HARDDRIVE > > > >>>>> v1 > > > >>>>> 1. Fix checkpatch coding style issues > > > >>>>> 2. Implement 64-bit addressing for QH/qTD/iTD/siTD > > > >>>>> descriptors > > > >>>>> 3. Add descriptor address offset property > > > >>>>> 4. Enable 64-bit EHCI DMA addressing on AST2700 > > > >>>>> 5. Configure descriptor address offset for AST2700 > > > >>>>> > > > >>>>> v2 > > > >>>>> 1. Remove unused EHCIfstn structure and dead code > > > >>>>> 2. Replace fprintf(stderr, ...) with > > > >>>>> qemu_log_mask(LOG_GUEST_ERROR) > > > >>>>> 3. Replace DPRINTF debug logs with trace events > > > >>>>> 4. Add functional tests for USB EHCI on AST2600 and AST2700 > > > >>>>> A1/A2 > > > >>>>> 5. Fix review issue > > > >>>>> > > > >>>>> v3: > > > >>>>> 1. Add Migration version test function > > > >>>>> 2. Add EHCI 64-bit buffer pointer fields description in > > > >>>>> commit log > > > >>>>> > > > >>>>> v4: > > > >>>>> 1. Reorder patches in the series > > > >>>>> 2. Fix EHCI migration issues > > > >>>>> 3. Introduce a common properties macro for both sysbus and > > > >>>>> PCI > > > >>>>> 4. Drop the descriptor address offset property > > > >>>>> 5. Add ctrldssegment-default property > > > >>>>> 6. Address review comments > > > >>>>> > > > >>>>> v5: > > > >>>>> 1. Add 11.0 machine compatibility properties > > > >>>>> > > > >>>>> v6: > > > >>>>> 1. Update reviewer suggested improvements. > > > >>>>> Jamin Lin (11): > > > >>>>> tests/functional/arm/test_aspeed_ast2600_sdk: Add USB EHCI > > > >>>>> test for > > > >>>>> AST2600 SDK > > > >>>>> hw/usb/hcd-ehci: Change descriptor addresses to 64-bit with > > > >>>>> migration > > > >>>>> compatibility > > > >>>>> hw/usb/hcd-ehci: Add property to advertise 64-bit > > > >>>>> addressing > > > >>>>> capability > > > >>>>> hw/usb/hcd-ehci: Implement 64-bit QH descriptor addressing > > > >>>>> hw/usb/hcd-ehci: Implement 64-bit qTD descriptor addressing > > > >>>>> hw/usb/hcd-ehci: Implement 64-bit iTD descriptor addressing > > > >>>>> hw/usb/hcd-ehci: Implement 64-bit siTD descriptor > > > >>>>> addressing > > > >>>>> hw/usb/hcd-ehci: Add ctrldssegment-default property > > > >>>>> hw/arm/aspeed_ast27x0: Set EHCI ctrldssegment-default > > > >>>>> hw/arm/aspeed_ast27x0: Enable 64-bit EHCI DMA addressing > > > >>>>> tests/functional/aarch64/test_aspeed_ast2700: Add USB EHCI > > > >>>>> test for > > > >>>>> AST2700 A1/A2 > > > >>>>> > > > > > > >>>>> hw/usb/hcd-ehci.h | 2 > > > 9 +++- > > > > > > >>>>> hw/arm/aspeed_ast27x0.c | 5 > > > + > > > >>>>> hw/core/machine.c | > > > 5 +- > > > >>>>> hw/usb/hcd-ehci.c | > > 162 > > > ++++++++++++ > > > >>>>> +----- > > > > > > >>>>> hw/usb/trace-events | 26 > > > +-- > > > >>>>> .../aarch64/test_aspeed_ast2700a1.py | 7 + > > > >>>>> .../aarch64/test_aspeed_ast2700a2.py | 7 + > > > >>>>> .../functional/arm/test_aspeed_ast2600_sdk.py | 7 + > > > >>>>> 8 files changed, 185 insertions(+), 63 deletions(-) > > > >>>>> > > > >>>> > > > >>>> Applied to > > > >>>> > > > >>>> https://github.com/legoater/qemu aspeed-next > > > >>> > > > >>> Jamin, > > > >>> > > > >>> I dropped the series from aspeed-next and kept : > > > >>> > > > >>> tests/functional/arm/test_aspeed_ast2600_sdk: Add USB EHCI > > > >>> test for AST2600 SDK > > > >>> > > > >>> The USB subsystem is orphan but it is still used in the virt world. > > > >>> This series is modifying the EHCI internals in such way that > > > >>> migration is impacted and the CTRLDSSEGMENT (Control Data > > > >>> Structure Segment > > > >>> Register) implementation is incomplete AFAICT. From EHCI specs : > > > >>> > > > >>> This register allows the host software to locate all control > > > >>> data > > > >>> structures within the *same 4 Gigabyte memory segment*. > > > >>> > > > >>> Linux driver seems buggy too. > > > >>> > > > >>> Overall, the risk of regression is too high given the time I can > > > >>> dedicate to this topic. > > > >>> > > > >>> Thanks for all the good work you done there, specially for migration. > > > >>> If someone can Ack the series and step forward to become USB > > > >>> maintainer, then I will reconsider. > > > > > > > > I'm not an USB expert / maintainer but FWIW this series looked > > > > good enough to me when I looked at it. Maybe consider to merge > > > > after the > > > > 11.1 release? > > > > > > Yes. I would like to too but ... > > > > > > The change in size of the EHCIqh and EHCIitd descriptors is a bit > > > worrying since these are used for guest DMA, see ehci_flush_qh(), or > > > compare, see ehci_verify_qh(). > > > > > > This should be fine for Linux, which has support since day one. > > > Other older guest OSes may not allocate the extra space. This is a > > > 20+ year old spec so the risk is probably low but I don't have the > > > expertise to assess > > the impact tbh. > > > > > > Anyhow if we want the model to be correct, we should not write > > > beyond spec-defined descriptor boundaries when in 32-bit mode. > > > > > > More work ... > > > > > > Thanks, > > > > > > C. > > > > Thanks for pointing this out. > > > > I agree that even though the QEMU internal EHCIqh, EHCIqtd, and > > EHCIitd structures include the extended high buffer pointer fields, we > > must not access those fields in guest memory when 64-bit addressing > > capability is not advertised. > > > > I have updated the patch accordingly. > > > > The descriptor DMA size is now selected based on caps_64bit_addr: > > > > When 64-bit addressing is supported and advertised, the complete > > descriptor, including the high buffer pointer fields, is transferred. > > In 32-bit mode, DMA reads and writes stop at the beginning of > > bufptr_hi, so QEMU does not access memory beyond the descriptor layout > > defined for 32-bit addressing. > > > > I also clear the local descriptor structures before partial DMA reads. > > Therefore, the high buffer pointer fields remain zero in 32-bit mode > > rather than containing stale or uninitialized values. > > > > This has been applied to: > > > > QH fetching, verification, and writeback qTD fetching and verification > > iTD fetching and writeback > > > > For example: > > /* > > * EHCIqh / EHCIqtd / EHCIitd are sized to always include the extended > > * high buffer pointer fields from EHCI 1.0 Appendix B. When 64-bit > > * addressing capability is not advertised to the guest, the > > descriptors > > * in guest memory only have the classic 32-bit layout, so DMA > > transfers > > * must not read or write past that boundary. > > */ > > #define EHCI_QH_DWORDS_32 (offsetof(EHCIqh, bufptr_hi) / > > sizeof(uint32_t)) > > #define EHCI_QTD_DWORDS_32 (offsetof(EHCIqtd, bufptr_hi) / > > sizeof(uint32_t)) #define EHCI_ITD_DWORDS_32 (offsetof(EHCIitd, > > bufptr_hi) / sizeof(uint32_t)) > > > > #define EHCI_QH_DWORDS_32 \ > > (offsetof(EHCIqh, bufptr_hi) / sizeof(uint32_t)) > > > > static uint32_t ehci_qh_dwords(const EHCIState *s) { > > return s->caps_64bit_addr ? > > (sizeof(EHCIqh) >> 2) : EHCI_QH_DWORDS_32; } > > > > ehci_flush_qh() now uses this selected size, so in 32-bit mode it will > > not write the extended high buffer pointer fields back to guest memory. > > > > Similarly, ehci_state_fetchqtd() only reads qtd.bufptr_hi when > > caps_64bit_addr is enabled: > > > > memset(qtd.bufptr_hi, 0, sizeof(qtd.bufptr_hi)); > > > > if (... || > > (q->ehci->caps_64bit_addr && > > get_dwords(q->ehci, addr + offsetof(EHCIqtd, bufptr_hi), > > qtd.bufptr_hi, ARRAY_SIZE(qtd.bufptr_hi)) < 0)) > > { return 0; > > } > > > > This should preserve compatibility with guests that allocate only the > > original 32-bit EHCI descriptor size, while still allowing the model > > to support the extended 64-bit descriptor layout. > > If you think this approach looks OK, I'll include it in the next revision > > (v7). > > > > Thanks, > > Jamin
I sent v7 here, https://patchwork.kernel.org/project/qemu-devel/cover/[email protected]/ Thanks, Jamin
