On Mon, 31 Oct 2022 21:33:03 +0900 Akihiko Odaki <akihiko.od...@daynix.com> wrote:
> pci_add_capability() checks whether capabilities overlap, and notifies > its caller so that it can properly handle the case. However, in the > most cases, the capabilities actually never overlap, and the interface > incurred extra error handling code, which is often incorrect or > suboptimal. For such cases, pci_add_capability() can simply abort the > execution if the capabilities actually overlap since it should be a > programming error. > > This change handles the other cases: hw/vfio/pci depends on the check to > decide MSI and MSI-X capabilities overlap with another. As they are > quite an exceptional and hw/vfio/pci knows much about PCI capabilities, > adding code specific to the cases to hw/vfio/pci still results in less > code than having error handling code everywhere in total. > > Signed-off-by: Akihiko Odaki <akihiko.od...@daynix.com> > --- > hw/vfio/pci.c | 61 ++++++++++++++++++++++++++++++++++++++++++--------- > hw/vfio/pci.h | 3 +++ > 2 files changed, 54 insertions(+), 10 deletions(-) > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index 939dcc3d4a..c7e3ef95a7 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_MSI); > + 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); > > - msi_64bit = !!(ctrl & PCI_MSI_FLAGS_64BIT); > - msi_maskbit = !!(ctrl & PCI_MSI_FLAGS_MASKBIT); > - entries = 1 << ((ctrl & PCI_MSI_FLAGS_QMASK) >> 1); > + 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; > + } > +} > + > +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,22 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, > uint8_t pos, Error **errp) > } > } > > + if (cap_id != PCI_CAP_ID_MSI && > + range_covers_byte(vdev->msi_pos, vdev->msi_cap_size, pos)) { > + error_setg(errp, > + "A capability overlaps with MSI (%" PRIu8 " in [%" PRIu8 ", %" > PRIu8 "))", > + pos, vdev->msi_pos, vdev->msi_pos + vdev->msi_cap_size); > + return -EINVAL; > + } > + > + if (cap_id != PCI_CAP_ID_MSIX && vdev->msix && > + range_covers_byte(vdev->msix->pos, MSIX_CAP_LENGTH, pos)) { > + error_setg(errp, > + "A capability overlaps with MSI-X (%" PRIu8 " in [%" PRIu8 ", %" > PRIu8 "))", > + pos, vdev->msix->pos, vdev->msix->pos + MSIX_CAP_LENGTH); > + return -EINVAL; > + } I provided an example of how the existing vfio_msi[x]_setup() can be trivially extended to perform the necessary size checking in place of pci_add_capability() without special cases and additional error paths as done here. Please adopt the approach I suggested. Thanks, Alex > + > /* Scale down size, esp in case virt caps were added above */ > size = MIN(size, vfio_std_cap_max_size(pdev, pos)); > > @@ -3037,6 +3072,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;