On Wed, 21 Oct 2020 17:30:04 +0800
Zenghui Yu <yuzeng...@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
