Re: [PATCH v3 06/17] hw/usb/hcd-ehci: Change descriptor addresses to 64-bit

[Peter Xu <peterx@redhat.com>]

On Fri, Apr 17, 2026 at 09:01:34AM +0200, Cédric Le Goater wrote:
> + Peter, Fabiano,
> 
> On 4/16/26 03:49, Jamin Lin wrote:
> > Change internal EHCI descriptor addresses from uint32_t to uint64_t.
> > 
> > The following fields are updated:
> > - EHCIPacket::qtdaddr
> > - EHCIQueue::{qhaddr, qtdaddr}
> > - EHCIState::{a_fetch_addr, p_fetch_addr}
> > 
> > Update get_dwords() and put_dwords() to take 64-bit addresses and
> > propagate the type change through the descriptor traversal paths.
> > 
> > Adjust NLPTR_GET() to operate on 64-bit values:
> > 
> >      #define NLPTR_GET(x) ((x) & ~0x1fULL)
> > 
> > so that link pointer masking works correctly when descriptor
> > addresses exceed 32-bit space. The previous mask (0xffffffe0)
> > implicitly truncated addresses to 32 bits.
> > 
> > This patch does not change the on-wire descriptor layout yet.
> > It only removes the internal 32-bit address limit and prepares
> > for later patches that will add full 64-bit QH/qTD/iTD/siTD support.
> > 
> > Update the EHCI trace-events prototypes for QH, qTD, iTD, and siTD to
> > use uint64_t for the address argument and print it with PRIx64. This
> > ensures full 64-bit addresses are shown in trace output and improves
> > debugging of queue heads and transfer descriptors.
> > 
> > Migration compatibility:
> > 
> > The fetch address fields in EHCIState are extended from 32-bit to
> > 64-bit, requiring a VMState version bump (v2 -> v3).
> > 
> > Backward compatibility is preserved by keeping the legacy 32-bit
> > fields (a_fetch_addr_pre_v3, p_fetch_addr_pre_v3) for pre-v3
> > migration streams via VMSTATE_UINT32_TEST(), and introducing new
> > 64-bit fields for v3+.
> > 
> > In post_load, pre-v3 state is promoted to the 64-bit fields.
> 
> Since ehci is supported by downstream, I'd prefer that the
> migration maintainers also verify the implementation.

Thanks, I'll only look at the migration bits.

[...]

> > @@ -2441,6 +2445,11 @@ static int usb_ehci_post_load(void *opaque, int version_id)
> >           }
> >       }
> > +    if (version_id < 3) {
> > +        s->a_fetch_addr = s->a_fetch_addr_pre_v3;
> > +        s->p_fetch_addr = s->p_fetch_addr_pre_v3;
> > +    }
> > +
> >       return 0;
> >   }
> > @@ -2470,9 +2479,14 @@ static void usb_ehci_vm_state_change(void *opaque, bool running, RunState state)
> >       }
> >   }
> > +static bool ehci_core_is_before_version_3(void *opaque, int version_id)
> > +{
> > +    return version_id < 3;
> > +}
> > +
> >   const VMStateDescription vmstate_ehci = {
> >       .name        = "ehci-core",
> > -    .version_id  = 2,
> > +    .version_id  = 3,
> >       .minimum_version_id  = 1,
> >       .pre_save    = usb_ehci_pre_save,
> >       .post_load   = usb_ehci_post_load,
> > @@ -2501,8 +2515,12 @@ const VMStateDescription vmstate_ehci = {
> >           /* schedule state */
> >           VMSTATE_UINT32(astate, EHCIState),
> >           VMSTATE_UINT32(pstate, EHCIState),
> > -        VMSTATE_UINT32(a_fetch_addr, EHCIState),
> > -        VMSTATE_UINT32(p_fetch_addr, EHCIState),
> > +        VMSTATE_UINT32_TEST(a_fetch_addr_pre_v3, EHCIState,
> > +                            ehci_core_is_before_version_3),
> > +        VMSTATE_UINT32_TEST(p_fetch_addr_pre_v3, EHCIState,
> > +                            ehci_core_is_before_version_3),
> > +        VMSTATE_UINT64_V(a_fetch_addr, EHCIState, 3),
> > +        VMSTATE_UINT64_V(p_fetch_addr, EHCIState, 3),

TL;DR: I feel like we still need machine type compat properties.

Details:

When a v2 stream arrives, two _TEST()s will do the loading, then
post_load() extend it to 64bits, looks fine.

When a v3 stream arrives, two _TEST()s got skipped then latter two take
effect.  post_load() skips.  Looks fine.

When migrating to another QEMU, due to the fact saving vmstates always take
vmsd's version declared (3), I don't see how it can migrate back to a v2
stream; it didn't know about v3.

Jiamin, have you tested migrating from a new QEMU binary back to another
old one?  For upstream and serious devices, we need to guarantee
bi-directional migrations, back and forth.

Thanks,

> >           VMSTATE_END_OF_LIST()
> >       }
> >   };

-- 
Peter Xu