Re: [PATCH] hw/pci: migration: Skip config space check for vendor specific capability during restore/load

[Alex Williamson]

On Tue, 30 Jan 2024 23:32:26 +0530
Vinayak Kale <vkale@nvidia.com> wrote:

> Missed adding Michael, Marcel, Alex and Avihai earlier, apologies.
> 
> Regards,
> Vinayak
> 
> On 30/01/24 3:26 pm, Vinayak Kale wrote:
> > In case of migration, during restore operation, qemu checks the config 
> > space of the pci device with the config space
> > in the migration stream captured during save operation. In case of config 
> > space data mismatch, restore operation is failed.
> > 
> > config space check is done in function get_pci_config_device(). By default 
> > VSC (vendor-specific-capability) in config space is checked.
> > 
> > Ideally qemu should not check VSC during restore/load. This patch skips the 
> > check by not setting pdev->cmask[] for VSC offsets in pci_add_capability().
> > If cmask[] is not set for an offset, then qemu skips config space check for 
> > that offset.
> > 
> > Signed-off-by: Vinayak Kale <vkale@nvidia.com>
> > ---
> >   hw/pci/pci.c | 7 +++++--
> >   1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 76080af580..32429109df 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -2485,8 +2485,11 @@ int pci_add_capability(PCIDevice *pdev, uint8_t 
> > cap_id,
> >       memset(pdev->used + offset, 0xFF, QEMU_ALIGN_UP(size, 4));
> >       /* Make capability read-only by default */
> >       memset(pdev->wmask + offset, 0, size);
> > -    /* Check capability by default */
> > -    memset(pdev->cmask + offset, 0xFF, size);
> > +
> > +    if (cap_id != PCI_CAP_ID_VNDR) {
> > +        /* Check non-vendor specific capability by default */
> > +        memset(pdev->cmask + offset, 0xFF, size);
> > +    }
> >       return offset;
> >   }
> >     
> 

If there is a possibility that the data within the vendor specific cap
can be consumed by the driver or diagnostic tools, then it's part of
the device ABI and should be consistent across migration.  A mismatch
can certainly cause a migration failure, but why shouldn't it?

This might be arguably ok (with more details) for a specific device,
but I don't think it can be the default given the arbitrary data
vendors can expose here.  Also, if this one, why not also the vendor
specific extended capability?  Thanks,

Alex