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
