On Thu, 27 Oct 2022 15:36:49 +0900
Akihiko Odaki <akihiko.od...@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.od...@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?
> + 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()?
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?
Shouldn't this also jump to reporting the error rather than silently
dropping a capability? 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 +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;
