Re: [PATCH v3 5/9] net/tap: implement interfaces for local migration

[Peter Xu <peterx@redhat.com>]

On Tue, Sep 09, 2025 at 10:44:22AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 08.09.25 23:01, Peter Xu wrote:
> > On Mon, Sep 08, 2025 at 07:48:09PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > On 08.09.25 18:42, Peter Xu wrote:
> > > > On Fri, Sep 05, 2025 at 04:50:35PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > > > +static const VMStateDescription vmstate_tap = {
> > > > > +    .name = "virtio-net-device",
> > > > > +    .post_load = tap_post_load,
> > > > > +    .fields = (const VMStateField[]) {
> > > > > +        VMSTATE_FD(fd, TAPState),
> > > > > +        VMSTATE_BOOL(using_vnet_hdr, TAPState),
> > > > > +        VMSTATE_BOOL(has_ufo, TAPState),
> > > > > +        VMSTATE_BOOL(has_uso, TAPState),
> > > > > +        VMSTATE_BOOL(enabled, TAPState),
> > > > > +        VMSTATE_UINT32(host_vnet_hdr_len, TAPState),
> > > > > +        VMSTATE_END_OF_LIST()
> > > > > +    }
> > > > > +};
> > > > > +
> > > > > +int tap_save(NetClientState *nc, QEMUFile *f)
> > > > > +{
> > > > > +    TAPState *s = DO_UPCAST(TAPState, nc, nc);
> > > > > +
> > > > > +    return vmstate_save_state(f, &vmstate_tap, s, 0);
> > > > > +}
> > > > > +
> > > > > +int tap_load(NetClientState *nc, QEMUFile *f)
> > > > > +{
> > > > > +    TAPState *s = DO_UPCAST(TAPState, nc, nc);
> > > > > +
> > > > > +    return vmstate_load_state(f, &vmstate_tap, s, 0);
> > > > > +}
> > > > 
> > > > Instead of hard-coding vmstate_save_state() / vmstate_load_state(), could
> > > > we make tap's VMSD to be a subsection of virtio-net's?
> > > > 
> > > > Multifd already doesn't support qemufile, but only iochannels (which is the
> > > > internal impl of qemufiles).  We might at some point start to concurrently
> > > > load devices with multifd, then anything with qemufile will be a no-go and
> > > > need to be serialized as legacy code in the main channel, or rewritten.
> > > > 
> > > > IMHO it'll be great if we can avoid adding new codes operating on
> > > > qemufiles, and also avoid adding any new custom VMSD fields' put()/get() if
> > > > ever possible.
> > > > 
> > > 
> > > Subsections are loaded after fields.
> > > 
> > > And virtio-net already has fields
> > > 
> > >          VMSTATE_WITH_TMP(VirtIONet, struct VirtIONetMigTmp,
> > >                           vmstate_virtio_net_has_vnet),
> > > 
> > > and
> > > 
> > >          VMSTATE_WITH_TMP(VirtIONet, struct VirtIONetMigTmp,
> > >                           vmstate_virtio_net_has_ufo),
> > 
> > Side note: I'm actually a bit confused on why it needs to use
> > VMSTATE_WITH_TMP(), or say, the get()/put() directly.
> > 
> > Taking example of vmstate_virtio_net_has_ufo, afaiu, the only point here is
> > that virtio_net_ufo_post_load() would check peer UFO support.  I wonder if
> > that should work too to check that in virtio_net_post_load_device(), and
> > fail there if anything is wrong..  then would has_ufo be able to be
> > migrated as a VMSTATE_U8 field?

[1]

> > 
> > > 
> > > Which do check on virtio-net level some parameters, which should come from local migration of TAP.
> > > 
> > > That's why I made TAP a field, and put it before these two ones. This way these two checks works.
> > > 
> > > 
> > > Still, from your comment I understand that hard-coding save/load is worse problem. So I can just
> > > skip checking in vmstate_virtio_net_has_vnet / vmstate_virtio_net_has_ufo with enabled "local-tap"
> > > (or "fd-passing") capability (or better migration parameter). This way TAP may be a subsection.
> > 
> > That'll be nice, thanks.  Or would a VMSTATE_STRUCT() for TAP to work (so
> > that it can also be put before the two _TMPs, but avoid raw get()/put())?
> > 
> 
> I considered using VMSTATE_STRUCT, but it would mean, I should access TAPState directly from virtio-net
> code. That's not possible now, as TAPState is a static type in tap.c, and think it's better to keep
> it static. So, in this case, subsection should be better, if I understand correctly.

Ah OK.  Then I wonder if this is a good chance too to move the peer feature
checks above [1] directly over to virtio-net's post_load as a pre-requisite
change, which should also be after the subsections.  Then the hope is it'll
also help removing some _TMP users.

-- 
Peter Xu