Re: [PATCH v26 03/17] vfio: Add save and load functions for VFIO PCI devices

[Alex Williamson]

On Wed, 21 Oct 2020 17:30:04 +0800
Zenghui Yu <yuzenghui@huawei.com> wrote:

> On 2020/9/25 6:49, Alex Williamson wrote:
> >> +    } else if (interrupt_type == VFIO_INT_MSIX) {
> >> +        uint16_t offset;
> >> +
> >> +        msix_load(pdev, f);
> >> +        offset = pci_default_read_config(pdev,
> >> +                                       pdev->msix_cap + PCI_MSIX_FLAGS + 
> >> 1, 2);
> >> +        /* load enable bit and maskall bit */
> >> +        vfio_pci_write_config(pdev, pdev->msix_cap + PCI_MSIX_FLAGS + 1,
> >> +                              offset, 2);  
> 
> It isn't clear that what purpose this load operation serves.  The config
> space has already been restored and we'll see that MSI-X _was_ and _is_
> enabled (or disabled).  vfio_msix_enable() will therefore not be invoked
> and no vectors would actually be enabled...  Not sure if I had missed
> something.

Yeah, afaict your interpretation is correct.  I think the intention was
to mimic userspace preforming a write to set the enable bit, but
re-writing it doesn't change the vconfig value, so the effect is not
the same.  I think this probably never worked.

> >> +    }
> >> +    return 0;  
> > 
> > It seems this could be simplified down to:
> > 
> > if (msi_enabled(pdev)) {
> >      vfio_msi_enable(vdev);
> > } else if (msix_enabled(pdev)) {
> >      msix_load(pdev, f);
> >      vfio_msix_enable(vdev);
> > }  
> 
> And it seems that this has fixed something :-)

Yep, no dependency on the value changing, simply set the state to that
indicated in vconfig.  Thanks,

Alex