| Message ID | 20221031123319.21532-2-akihiko.odaki@daynix.com |
|---|---|
| State | New |
| Headers | show |
| Series | pci: Abort if pci_add_capability fails | expand |
On Mon, 31 Oct 2022 21:33:03 +0900 Akihiko Odaki <akihiko.odaki@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.odaki@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;
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; + } + /* 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;
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.odaki@daynix.com> --- hw/vfio/pci.c | 61 ++++++++++++++++++++++++++++++++++++++++++--------- hw/vfio/pci.h | 3 +++ 2 files changed, 54 insertions(+), 10 deletions(-)