Re: [PATCH v4 01/17] hw/vfio/pci: Ensure MSI and MSI-X do not overlap

[Akihiko Odaki]

Hi,

Thanks for keeping reviewing. I have just sent v5 so please check it out.

On 2022/10/28 4:31, Alex Williamson wrote:
> On Thu, 27 Oct 2022 15:36:49 +0900
> Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> > vfio_add_std_cap() is designed to ensure that capabilities do not
> > overlap, but it failed to do so for MSI and MSI-X capabilities.
> >
> > Ensure MSI and MSI-X capabilities do not overlap with others by omitting
> > other overlapping capabilities.
> >
> > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > ---
> >   hw/vfio/pci.c | 55 +++++++++++++++++++++++++++++++++++++++++----------
> >   hw/vfio/pci.h |  3 +++
> >   2 files changed, 48 insertions(+), 10 deletions(-)
> >
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index 939dcc3d4a..8a4995cd68 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -1278,23 +1278,42 @@ static void vfio_disable_interrupts(VFIOPCIDevice *vdev)
> >       }
> >   }
> >   
> > -static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
> > +static void vfio_msi_early_setup(VFIOPCIDevice *vdev, Error **errp)
> >   {
> >       uint16_t ctrl;
> > -    bool msi_64bit, msi_maskbit;
> > -    int ret, entries;
> > -    Error *err = NULL;
> > +    uint8_t pos;
> > +
> > +    pos = pci_find_capability(&vdev->pdev, PCI_CAP_ID_MSIX);
>
>
> PCI_CAP_ID_MSIX???  Is this tested with MSI?

Oops, I think I have failed it to test a device with MSI. I confirmed 
this version does not work, and the new version I have just sent works.

> > +    if (!pos) {
> > +        return;
> > +    }
> >   
> >       if (pread(vdev->vbasedev.fd, &ctrl, sizeof(ctrl),
> >                 vdev->config_offset + pos + PCI_CAP_FLAGS) != sizeof(ctrl)) {
> >           error_setg_errno(errp, errno, "failed reading MSI PCI_CAP_FLAGS");
> > -        return -errno;
> > +        return;
> >       }
> > -    ctrl = le16_to_cpu(ctrl);
> > +    vdev->msi_pos = pos;
> > +    vdev->msi_ctrl = le16_to_cpu(ctrl);
> > +
> > +    vdev->msi_cap_size = 0xa;
> > +    if ((vdev->msi_ctrl & PCI_MSI_FLAGS_MASKBIT)) {
> > +        vdev->msi_cap_size += 0xa;
> > +    }
> > +    if ((vdev->msi_ctrl & PCI_MSI_FLAGS_64BIT)) {
> > +        vdev->msi_cap_size += 0x4;
> > +    }
> > +}
> >   
> > -    msi_64bit = !!(ctrl & PCI_MSI_FLAGS_64BIT);
> > -    msi_maskbit = !!(ctrl & PCI_MSI_FLAGS_MASKBIT);
> > -    entries = 1 << ((ctrl & PCI_MSI_FLAGS_QMASK) >> 1);
> > +static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
> > +{
> > +    bool msi_64bit, msi_maskbit;
> > +    int ret, entries;
> > +    Error *err = NULL;
> > +
> > +    msi_64bit = !!(vdev->msi_ctrl & PCI_MSI_FLAGS_64BIT);
> > +    msi_maskbit = !!(vdev->msi_ctrl & PCI_MSI_FLAGS_MASKBIT);
> > +    entries = 1 << ((vdev->msi_ctrl & PCI_MSI_FLAGS_QMASK) >> 1);
> >   
> >       trace_vfio_msi_setup(vdev->vbasedev.name, pos);
> >   
> > @@ -1306,7 +1325,6 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
> >           error_propagate_prepend(errp, err, "msi_init failed: ");
> >           return ret;
> >       }
> > -    vdev->msi_cap_size = 0xa + (msi_maskbit ? 0xa : 0) + (msi_64bit ? 0x4 : 0);
> >   
> >       return 0;
> >   }
> > @@ -1524,6 +1542,7 @@ static void vfio_msix_early_setup(VFIOPCIDevice *vdev, 
> > Error **errp)
> >       pba = le32_to_cpu(pba);
> >   
> >       msix = g_malloc0(sizeof(*msix));
> > +    msix->pos = pos;
> >       msix->table_bar = table & PCI_MSIX_FLAGS_BIRMASK;
> >       msix->table_offset = table & ~PCI_MSIX_FLAGS_BIRMASK;
> >       msix->pba_bar = pba & PCI_MSIX_FLAGS_BIRMASK;
> > @@ -2025,6 +2044,16 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t 
> > pos, Error **errp)
> >           }
> >       }
> >   
> > +    if (cap_id != PCI_CAP_ID_MSI &&
> > +        pos >= vdev->msi_pos && pos < vdev->msi_pos + vdev->msi_cap_size) {
> > +        return 0;
> > +    }
> > +
> > +    if (cap_id != PCI_CAP_ID_MSIX && vdev->msix &&
> > +        pos >= vdev->msix->pos && pos < vdev->msix->pos + MSIX_CAP_LENGTH) {
> > +        return 0;
> > +    }
> > +
>
> These only test a specific kind of overlap, why not use
> ranges_overlap()?

Done so in v5.

> We also need to sanity test vdev->msi_pos, or are you just letting
> msi_pos = 0, msi_cap_size = 0 fall through since we cannot overlap?

Yes, It is expected that msi_cap_size will be 0 if the device does not 
have MSI capability and nothing will overlap in the case.

> Shouldn't this also jump to reporting the error rather than silently
> dropping a capability?  Thanks,

I added warnings in v5.

Regards,
Akihiko Odaki

> Alex
>
> >       /* Scale down size, esp in case virt caps were added above */
> >       size = MIN(size, vfio_std_cap_max_size(pdev, pos));
> >   
> > @@ -3037,6 +3066,12 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
> >   
> >       vfio_bars_prepare(vdev);
> >   
> > +    vfio_msi_early_setup(vdev, &err);
> > +    if (err) {
> > +        error_propagate(errp, err);
> > +        goto error;
> > +    }
> > +
> >       vfio_msix_early_setup(vdev, &err);
> >       if (err) {
> >           error_propagate(errp, err);
> > diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> > index 7c236a52f4..9ae0278058 100644
> > --- a/hw/vfio/pci.h
> > +++ b/hw/vfio/pci.h
> > @@ -107,6 +107,7 @@ enum {
> >   
> >   /* Cache of MSI-X setup */
> >   typedef struct VFIOMSIXInfo {
> > +    uint8_t pos;
> >       uint8_t table_bar;
> >       uint8_t pba_bar;
> >       uint16_t entries;
> > @@ -128,6 +129,8 @@ struct VFIOPCIDevice {
> >       unsigned int rom_size;
> >       off_t rom_offset; /* Offset of ROM region within device fd */
> >       void *rom;
> > +    uint8_t msi_pos;
> > +    uint16_t msi_ctrl;
> >       int msi_cap_size;
> >       VFIOMSIVector *msi_vectors;
> >       VFIOMSIXInfo *msix;