Re: [RFC PATCH v2 1/9] igb: Add x-vf-migration property and DVSEC extended capability

["Cédric Le Goater" <clg@redhat.com>]

On 9/3/26 21:57, Alex Williamson wrote:
> On Wed,  2 Sep 2026 21:20:46 +0200
> Cédric Le Goater <clg@redhat.com> wrote:
> 
>> diff --git a/hw/net/igb_migration.c b/hw/net/igb_migration.c
>> new file mode 100644
>> index 000000000000..4dfebd82344c
>> --- /dev/null
>> +++ b/hw/net/igb_migration.c
>> @@ -0,0 +1,106 @@
>> +/*
>> + * QEMU Intel 82576 SR/IOV VF Migration Support
>> + *
>> + * Copyright (c) 2026 Red Hat, Inc.
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "hw/pci/pci_device.h"
>> +#include "hw/pci/pcie.h"
>> +#include "igb_common.h"
>> +#include "igb_migration.h"
>> +
>> +static void igbvf_mig_update_status(IgbVfState *s, uint8_t err)
>> +{
>> +    IgbVfMigState *ms = &s->mig;
>> +    PCIDevice *dev = PCI_DEVICE(s);
>> +    uint32_t status;
>> +
>> +    status = ms->mig_state & IGB_MIG_STATUS_STATE_MASK;
>> +
>> +    if (err) {
>> +        status = IGB_MIG_STATE_ERROR | IGB_MIG_STATUS_ERR(err);
>> +    }
>> +
>> +    pci_set_long(dev->config + IGB_MIG_DVSEC_OFFSET + IGB_MIG_STATUS, status);
>> +}
>> +
>> +
>> +bool igbvf_add_migration_dvsec(PCIDevice *dev, Error **errp)
>> +{
>> +    uint16_t offset = IGB_MIG_DVSEC_OFFSET;
>> +    uint32_t caps;
>> +
>> +    pcie_add_capability(dev, PCI_EXT_CAP_ID_DVSEC, 1, offset,
>> +                        IGB_MIG_DVSEC_SIZE);
>> +
>> +    /* DVSEC header 1: length[31:20] | rev[19:16] | vendor_id[15:0] */
>> +    pci_set_long(dev->config + offset + 0x4,
>> +                 (IGB_MIG_DVSEC_SIZE << 20) |
>> +                 (IGB_MIG_DVSEC_VER << 16) |
>> +                 PCI_VENDOR_ID_INTEL);
> 
> Let's not co-opt an Intel DVSEC ID, we should use a vendor ID that we
> have a claim to, the RedHat/Qumranet one, I'd guess.  We may need to
> add DVSEC IDs to the spreadsheet for whoever is tracking Device IDs.
> Gerd?
> 
>> +
>> +    /* DVSEC header 2: DVSEC ID */
>> +    pci_set_word(dev->config + offset + 0x8, IGB_MIG_DVSEC_ID);
>> +
>> +    /* CAPS: features (state migration only) */
>> +    caps = IGB_MIG_CAP_F_STATE;
>> +    pci_set_long(dev->config + offset + IGB_MIG_CAPS, caps);
>> +
>> +    /* STATUS: initial state is RUNNING */
>> +    pci_set_long(dev->config + offset + IGB_MIG_STATUS,
>> +                 IGB_MIG_STATE_RUNNING);
>> +
>> +    /* BUF_ADDR_LO and BUF_ADDR_HI are writable */
>> +    memset(dev->wmask + offset + IGB_MIG_BUF_ADDR_LO, 0xff, 4);
>> +    memset(dev->wmask + offset + IGB_MIG_BUF_ADDR_HI, 0xff, 4);
>> +
>> +    return true;
>> +}
> ...
>> diff --git a/hw/net/igbvf.c b/hw/net/igbvf.c
>> index 9a165c7063ee..30dfdb574ac7 100644
>> --- a/hw/net/igbvf.c
>> +++ b/hw/net/igbvf.c
>> @@ -38,27 +38,21 @@
>>    */
>>   
>>   #include "qemu/osdep.h"
>> +#include "qemu/range.h"
>>   #include "hw/core/hw-error.h"
>>   #include "hw/net/mii.h"
>>   #include "hw/pci/pci_device.h"
>>   #include "hw/pci/pcie.h"
>> +#include "hw/pci/pcie_sriov.h"
>>   #include "hw/pci/msix.h"
>>   #include "net/eth.h"
>>   #include "net/net.h"
>>   #include "igb_common.h"
>>   #include "igb_core.h"
>> +#include "igb_migration.h"
>>   #include "trace.h"
>>   #include "qapi/error.h"
>>   
>> -OBJECT_DECLARE_SIMPLE_TYPE(IgbVfState, IGBVF)
>> -
>> -struct IgbVfState {
>> -    PCIDevice parent_obj;
>> -
>> -    MemoryRegion mmio;
>> -    MemoryRegion msix;
>> -};
>> -
>>   static hwaddr vf_to_pf_addr(hwaddr addr, uint16_t vfn, bool write)
>>   {
>>       switch (addr) {
>> @@ -199,10 +193,35 @@ static hwaddr vf_to_pf_addr(hwaddr addr, uint16_t vfn, bool write)
>>       return HWADDR_MAX;
>>   }
>>   
>> +static bool igbvf_addr_in_dvsec(uint32_t addr, int len)
>> +{
>> +    return ranges_overlap(addr, len,
>> +                          IGB_MIG_DVSEC_OFFSET, IGB_MIG_DVSEC_SIZE);
>> +}
>> +
>> +static uint32_t igbvf_read_config(PCIDevice *dev, uint32_t addr, int size)
>> +{
>> +    IgbVfState *s = IGBVF(dev);
>> +
>> +    if (s->migration_enabled && igbvf_addr_in_dvsec(addr, size)) {
>> +        return igbvf_mig_config_read(s, addr, size);
>> +    }
>> +
>> +    return pci_default_read_config(dev, addr, size);
>> +}
>> +
>>   static void igbvf_write_config(PCIDevice *dev, uint32_t addr, uint32_t val,
>>       int len)
>>   {
>> +    IgbVfState *s = IGBVF(dev);
>> +
>>       trace_igbvf_write_config(addr, val, len);
>> +
>> +    if (s->migration_enabled && igbvf_addr_in_dvsec(addr, len)) {
>> +        igbvf_mig_config_write(s, addr, val, len);
>> +        return;
>> +    }
>> +
>>       pci_default_write_config(dev, addr, val, len);
>>       if (object_property_get_bool(OBJECT(pcie_sriov_get_pf(dev)),
>>                                    "x-pcie-flr-init", &error_abort)) {
>> @@ -282,13 +301,27 @@ static void igbvf_pci_realize(PCIDevice *dev, Error **errp)
>>       }
>>   
>>       pcie_ari_init(dev, 0x150);
>> +
>> +    if (object_property_get_bool(OBJECT(pcie_sriov_get_pf(dev)),
>> +                                 "x-vf-migration", &error_abort)) {
>> +        s->vfn = pcie_sriov_vf_number(dev);
>> +        s->migration_enabled = true;
>> +        if (!igbvf_add_migration_dvsec(dev, errp)) {
>> +            return;
>> +        }
> 
> The migration blocker noted later in the docs should be added here.
> Trivial to add, avoids the internal migration state being reset by the
> L0 VM being migrated, doesn't seem worth extending this driver's VMState
> while the feature is experimental.
> 
>> +    }
>>   }
>>   
>>   static void igbvf_qdev_reset_hold(Object *obj, ResetType type)
>>   {
>>       PCIDevice *vf = PCI_DEVICE(obj);
>> +    IgbVfState *s = IGBVF(vf);
>>   
>>       igb_vf_reset(pcie_sriov_get_pf(vf), pcie_sriov_vf_number(vf));
>> +
>> +    if (s->migration_enabled) {
>> +        igbvf_mig_state_reset(s);
>> +    }
> 
> Hmm, I think reset is more complicated that this.  This seems to define
> that any device reset will reset the migration state.  The vfio
> migration protocol only defines that a VFIO_DEVICE_RESET returns the
> device to running.
> 
> Consider the case of an FLR triggered by the L2 guest.  L1 QEMU passes
> through the config space write, that lands in vfio-pci core config
> space handling in the L1 variant driver, which turns into a
> pci_reset_function() in the L1 kernel and I think lands here in the L0
> QEMU.  Therefore, it seems like the L2 guest can corrupt the migration
> state.
> 
> As above, the migration state is defined to be reset via the RESET
> ioctl, which also turns into a pci_reset_function() in the L1 kernel.
> So L0 QEMU can't tell the difference here.
> 
> I think that means that the variant driver itself needs to own this
> part of the protocol, performing the migration state housekeeping on
> RESET ioctl, while both allow the state to persist on other resets.  In
> that sense QEMU cannot emulate this DVSEC as normal config space, it's
> a persistent control plane that lives in the VMM and happens to be
> accessed through config space.  Thanks,


Hi Alex,
  
You are right. The key principle: The DVSEC is a persistent control
plane living in the VMM, not device state. Device reset (VFIO ioctl
and L2 FLR) resets the device, not the mailbox. The variant driver
owns the reset, not the emulated device in L0 QEMU.

I missed two things : 1. transferring the DVSEC hiding (as it was done
for the mig BAR) and 2. reset of migration state.

Looking closer at it, the changes are small.

In QEMU:

The cold boot initialization sets all fields to defaults (zero). The
only change from today is that the initial state is ERROR, which means
migration isn't operational until the driver binds. The variant driver
activates the control plane by cycling the machine state to RUNNING.

The VF functional registers (queues, DMA rings, interrupts ... ) are
reset normally by igb_core_vf_reset(), and the migration control plane
(IgbVfMigState) persists in memory. It survives pci_do_device_reset()
and the variant driver sets the fields as needed through the normal
DVSEC command flow.

BUF_ADDR could be restored from the IgbVfMigState cache values since
it's a RW reg, but even that isn't a strong requirement.

So igbvf_mig_state_reset() is no longer needed in the reset path.
That's all for QEMU.

Driver :

Variant driver owns all the cleanup after reset: closes migration fds,
disables dirty tracking, reads and cycles the DVSEC state back to
RUNNING.

The first path, VFIO_DEVICE_RESET, needs a custom ioctl handler to run
the cleanup.

Second path, L2 FLR. To hide the DVSEC range from the L2 guest, the
driver implements custom VFIO config space reads and writes. The
config write handler can detect the FLR and implement the same cleanup
logic.

That's for v3.

Thanks,

C.