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

[Peter Xu <peterx@redhat.com>]

On Wed, Apr 22, 2026 at 09:21:55AM +0000, Jamin Lin wrote:
> Hi Peter, Cédric and Philippe
> 
> > Subject: Re: [PATCH v3 06/17] hw/usb/hcd-ehci: Change descriptor addresses to
> > 64-bit
> > 
> > On Mon, Apr 20, 2026 at 05:56:11AM +0000, Jamin Lin wrote:
> > > Hi Peter, Philippe, Cédric
> > 
> > Hi, Jamin,
> > 
> > [...]
> > 
> > > > > > @@ -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.
> > > >
> > >
> > > I don't understand this: Could you please describe it in more detail?
> > 
> > See hw/core/machine.c and entries added into hw_compat_*[] arrays.  IIUC,
> > if this is an important device and if we want to guarantee bidirectional
> > migration (I'll explain later), then we should stick with machine 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.
> > > >
> > >
> > > Sorry, I am not familiar with the design of QEMU migration, and I don't know
> > how to test it.
> > > I added these hook functions based on Philippe's suggestion and review.
> > > Could you tell me how to test migration to ensure both version 3 and version
> > 2 are workable?
> > 
> > Since this patch already modified VMSDs, I believe migration test should have
> > been carried out at the very minimum..  Logically if vmsd versioning is used,
> > we should also have tested forward migrations, because that's only for that.
> > 
> > What I'm talking about is I think backward migration will fail.
> > 
> > Forward migration describes the case when a VM hosted by an old QEMU
> > binary (e.g. QEMU v10.2.0) is migrated to a new QEMU (e.g., after this patch
> > applied on top of master).
> > 
> > Backward migration describes the case when a VM hosted by a new QEMU
> > binary (e.g., after this patch applied on top of master) is migrated back to an
> > old QEMU binary (e.g. QEMU v10.2.0).
> > 
> > For different devices, we should allow different level of support on migration.
> > Any device that may be important to either upstream QEMU or downstreams
> > should better allow bi-directional migration.  In this case we need to use
> > machine compat properties I mentioned.  I'm not yet sure where EHCI
> > controller falls e.g. v.s. XHCI.. when not sure, we can also just stick with
> > machine compat properties to be on the safe side.
> > 
> > For more info, you can also refer to:
> > 
> > https://www.qemu.org/docs/master/devel/migration/compatibility.html
> > 
> > Thanks,
> > 
> > --
> > Peter Xu
> Thanks for your support and the detailed explanation.
> Following Cédric’s comments, I now understand how to properly test migration. With the changes below, both forward and backward migration tests pass.
> Could you please review these changes, or would you prefer that I resend them as v4?

Per what I read on Phil's reply on the cover letter, he prefers you to
resend anyway.  Better do it.  Comments inline.

> 
> Thanks again for your guidance and support.
> 
> Jamin
> 
> diff --git a/hw/usb/hcd-ehci.h b/hw/usb/hcd-ehci.h
> index 310183bbc4..bd165c793c 100644
> --- a/hw/usb/hcd-ehci.h
> +++ b/hw/usb/hcd-ehci.h
> @@ -264,6 +264,7 @@ struct EHCIState {
>      uint32_t maxframes;
>      bool caps_64bit_addr;
>      uint32_t ctrldssegment_default;
> +    bool migrate_fetch_addr_64bit;
>  
>      /*
>       *  EHCI spec version 1.0 Section 2.3
> @@ -321,7 +322,9 @@ struct EHCIState {
>      DEFINE_PROP_UINT32("maxframes", _state, ehci.maxframes, 128), \
>      DEFINE_PROP_BOOL("caps-64bit-addr", _state, ehci.caps_64bit_addr, false), \
>      DEFINE_PROP_UINT32("ctrldssegment-default", _state, \
> -                       ehci.ctrldssegment_default, 0)
> +                       ehci.ctrldssegment_default, 0), \
> +    DEFINE_PROP_BOOL("x-migrate-fetch-addr-64bit", _state, \
> +                     ehci.migrate_fetch_addr_64bit, true)

The diff looks a bit weird; this chunk is not present in hw/usb/hcd-ehci.h
in master branch, but maybe this series touched it.. in that case it's
fine.

>  
>  extern const VMStateDescription vmstate_ehci;
>  
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 0aa77a57e9..e77400c7ab 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -41,6 +41,8 @@
>  GlobalProperty hw_compat_10_2[] = {
>      { "scsi-block", "migrate-pr", "off" },
>      { "isa-cirrus-vga", "global-vmstate", "true" },
> +    { "sysbus-ehci-usb", "x-migrate-fetch-addr-64bit", "off" },
> +    { "pci-ehci-usb", "x-migrate-fetch-addr-64bit", "off" },

I'm still looking at master branch, vmstate_ehci is referenced in both
"ehci-sysbus" (vmstate_ehci_sysbus) and "ehci" (vmstate_ehci_pci).  Here
you used different names, I'm not sure if it's relevant to your other
patches, please double check.

Also if there're two drivers involved, IIUC we should need to add two bool
properties.. I only saw one above.  Again, not sure if it's relevant to
what your prior patch did, but please double check.

>  };
>  const size_t hw_compat_10_2_len = G_N_ELEMENTS(hw_compat_10_2);
>  
> diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
> index 983b1731f9..87528ddc02 100644
> --- a/hw/usb/hcd-ehci.c
> +++ b/hw/usb/hcd-ehci.c
> @@ -2469,6 +2469,9 @@ static int usb_ehci_pre_save(void *opaque)
>      ehci->last_run_ns -= (ehci->frindex - new_frindex) * UFRAME_TIMER_NS;
>      ehci->frindex = new_frindex;
>  
> +    ehci->a_fetch_addr_pre_v3 = ehci->a_fetch_addr;
> +    ehci->p_fetch_addr_pre_v3 = ehci->p_fetch_addr;

This looks fine if you always prepare the 32bit versions of the addr, but
it can also be put under a "if (!ehci_fetch_addr_64_needed(...))".  Not a
big deal.

> +
>      return 0;
>  }
>  
> @@ -2489,10 +2492,8 @@ 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;
> -    }
> +    s->a_fetch_addr = s->a_fetch_addr_pre_v3;
> +    s->p_fetch_addr = s->p_fetch_addr_pre_v3;

This one looks risky, it will also run on new machine types, I think a "if
(!ehci_fetch_addr_64_needed(...))" is required here (unlike the pre_save),
otherwise I suspect you may overwrite the 64bits with 32bits after
migration.  It may explode when high 32bits have something nonzero?

>  
>      return 0;
>  }
> @@ -2523,14 +2524,29 @@ 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)
> +static bool ehci_fetch_addr_64_needed(void *opaque)
>  {
> -    return version_id < 3;
> +    EHCIState *s = opaque;
> +
> +    return s->migrate_fetch_addr_64bit;
>  }

Yes this is the traditional way.

>  
> +static const VMStateDescription vmstate_ehci_fetch_addr_64 = {
> +    .name = "ehci-core/fetch-addr64",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = ehci_fetch_addr_64_needed,
> +    .fields = (const VMStateField[]) {
> +        /* mmio registers */
> +        VMSTATE_UINT64(a_fetch_addr, EHCIState),
> +        VMSTATE_UINT64(p_fetch_addr, EHCIState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  const VMStateDescription vmstate_ehci = {
>      .name        = "ehci-core",
> -    .version_id  = 3,
> +    .version_id  = 2,
>      .minimum_version_id  = 1,
>      .pre_save    = usb_ehci_pre_save,
>      .post_load   = usb_ehci_post_load,
> @@ -2559,14 +2575,14 @@ const VMStateDescription vmstate_ehci = {
>          /* schedule state */
>          VMSTATE_UINT32(astate, EHCIState),
>          VMSTATE_UINT32(pstate, 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),
> +        VMSTATE_UINT32(a_fetch_addr_pre_v3, EHCIState),
> +        VMSTATE_UINT32(p_fetch_addr_pre_v3, EHCIState),
>          VMSTATE_END_OF_LIST()
> -    }
> +    },
> +    .subsections = (const VMStateDescription * const []) {
> +        &vmstate_ehci_fetch_addr_64,

IMHO you don't need to make it a subsection, you can use
VMSTATE_UINT64_TEST().

Similarly you can also use VMSTATE_UINT32_TEST() to only migrate 32bit
versions in old machine types.

With those you can keep them together and add a comment, may look better
than subsections where you split them apart, it'll be harder to follow in
the future.

Thanks,

> +        NULL,
> +    },
>  };
>  
>  void usb_ehci_realize(EHCIState *s, DeviceState *dev, Error **errp)

-- 
Peter Xu