| Message ID | 20221026201527.24063-15-akihiko.odaki@daynix.com |
|---|---|
| State | New |
| Headers | show |
| Series | pci: Abort if pci_add_capability fails | expand |
On Thu, 27 Oct 2022 05:15:25 +0900 Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > The code generating errors in pci_add_capability has a comment which > says: > > Verify that capabilities don't overlap. Note: device assignment > > depends on this check to verify that the device is not broken. > > Should never trigger for emulated devices, but it's helpful for > > debugging these. > > Indeed vfio has some code that passes capability offsets and sizes from > a physical device, but it explicitly pays attention so that the > capabilities never overlap. Therefore, in pci_add_capability(), we can > always assert that capabilities never overlap, and that is what happens > when omitting errp. > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > hw/vfio/pci-quirks.c | 15 +++------------ > hw/vfio/pci.c | 14 +++++--------- > 2 files changed, 8 insertions(+), 21 deletions(-) > > diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c > index f0147a050a..e94fd273ea 100644 > --- a/hw/vfio/pci-quirks.c > +++ b/hw/vfio/pci-quirks.c > @@ -1530,7 +1530,7 @@ const PropertyInfo qdev_prop_nv_gpudirect_clique = { > static int vfio_add_nv_gpudirect_cap(VFIOPCIDevice *vdev, Error **errp) > { > PCIDevice *pdev = &vdev->pdev; > - int ret, pos = 0xC8; > + int pos = 0xC8; > > if (vdev->nv_gpudirect_clique == 0xFF) { > return 0; > @@ -1547,11 +1547,7 @@ static int vfio_add_nv_gpudirect_cap(VFIOPCIDevice *vdev, Error **errp) > return -EINVAL; > } > > - ret = pci_add_capability(pdev, PCI_CAP_ID_VNDR, pos, 8, errp); > - if (ret < 0) { > - error_prepend(errp, "Failed to add NVIDIA GPUDirect cap: "); > - return ret; > - } > + pci_add_capability(pdev, PCI_CAP_ID_VNDR, pos, 8); > > memset(vdev->emulated_config_bits + pos, 0xFF, 8); > pos += PCI_CAP_FLAGS; > @@ -1718,12 +1714,7 @@ static int vfio_add_vmd_shadow_cap(VFIOPCIDevice *vdev, Error **errp) > return -EFAULT; > } > > - ret = pci_add_capability(&vdev->pdev, PCI_CAP_ID_VNDR, pos, > - VMD_SHADOW_CAP_LEN, errp); > - if (ret < 0) { > - error_prepend(errp, "Failed to add VMD MEMBAR Shadow cap: "); > - return ret; > - } > + pci_add_capability(&vdev->pdev, PCI_CAP_ID_VNDR, pos, VMD_SHADOW_CAP_LEN); For these, I guess we're assuming that since they're added first via vfio_add_virt_caps() that we cannot create an overlap? > > memset(vdev->emulated_config_bits + pos, 0xFF, VMD_SHADOW_CAP_LEN); > pos += PCI_CAP_FLAGS; > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index 939dcc3d4a..2b653d01e3 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -1826,7 +1826,7 @@ static void vfio_add_emulated_long(VFIOPCIDevice *vdev, int pos, > vfio_set_long_bits(vdev->emulated_config_bits + pos, mask, mask); > } > > -static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size, > +static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, uint8_t pos, uint8_t size, > Error **errp) > { > uint16_t flags; > @@ -1943,11 +1943,7 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size, > 1, PCI_EXP_FLAGS_VERS); > } > > - pos = pci_add_capability(&vdev->pdev, PCI_CAP_ID_EXP, pos, size, > - errp); > - if (pos < 0) { > - return pos; > - } > + pos = pci_add_capability(&vdev->pdev, PCI_CAP_ID_EXP, pos, size); > > vdev->pdev.exp.exp_cap = pos; > > @@ -2045,14 +2041,14 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos, Error **errp) > case PCI_CAP_ID_PM: > vfio_check_pm_reset(vdev, pos); > vdev->pm_cap = pos; > - ret = pci_add_capability(pdev, cap_id, pos, size, errp); > + pci_add_capability(pdev, cap_id, pos, size); > break; > case PCI_CAP_ID_AF: > vfio_check_af_flr(vdev, pos); > - ret = pci_add_capability(pdev, cap_id, pos, size, errp); > + pci_add_capability(pdev, cap_id, pos, size); > break; > default: > - ret = pci_add_capability(pdev, cap_id, pos, size, errp); > + pci_add_capability(pdev, cap_id, pos, size); > break; > } And for these we've calculated a size to make sure that we don't bump into the next capability, but how do you account for the MSI and MSI-X cases above this chunk where vfio's overlap prevention can't be used? Thanks, Alex
diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c index f0147a050a..e94fd273ea 100644 --- a/hw/vfio/pci-quirks.c +++ b/hw/vfio/pci-quirks.c @@ -1530,7 +1530,7 @@ const PropertyInfo qdev_prop_nv_gpudirect_clique = { static int vfio_add_nv_gpudirect_cap(VFIOPCIDevice *vdev, Error **errp) { PCIDevice *pdev = &vdev->pdev; - int ret, pos = 0xC8; + int pos = 0xC8; if (vdev->nv_gpudirect_clique == 0xFF) { return 0; @@ -1547,11 +1547,7 @@ static int vfio_add_nv_gpudirect_cap(VFIOPCIDevice *vdev, Error **errp) return -EINVAL; } - ret = pci_add_capability(pdev, PCI_CAP_ID_VNDR, pos, 8, errp); - if (ret < 0) { - error_prepend(errp, "Failed to add NVIDIA GPUDirect cap: "); - return ret; - } + pci_add_capability(pdev, PCI_CAP_ID_VNDR, pos, 8); memset(vdev->emulated_config_bits + pos, 0xFF, 8); pos += PCI_CAP_FLAGS; @@ -1718,12 +1714,7 @@ static int vfio_add_vmd_shadow_cap(VFIOPCIDevice *vdev, Error **errp) return -EFAULT; } - ret = pci_add_capability(&vdev->pdev, PCI_CAP_ID_VNDR, pos, - VMD_SHADOW_CAP_LEN, errp); - if (ret < 0) { - error_prepend(errp, "Failed to add VMD MEMBAR Shadow cap: "); - return ret; - } + pci_add_capability(&vdev->pdev, PCI_CAP_ID_VNDR, pos, VMD_SHADOW_CAP_LEN); memset(vdev->emulated_config_bits + pos, 0xFF, VMD_SHADOW_CAP_LEN); pos += PCI_CAP_FLAGS; diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 939dcc3d4a..2b653d01e3 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -1826,7 +1826,7 @@ static void vfio_add_emulated_long(VFIOPCIDevice *vdev, int pos, vfio_set_long_bits(vdev->emulated_config_bits + pos, mask, mask); } -static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size, +static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, uint8_t pos, uint8_t size, Error **errp) { uint16_t flags; @@ -1943,11 +1943,7 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size, 1, PCI_EXP_FLAGS_VERS); } - pos = pci_add_capability(&vdev->pdev, PCI_CAP_ID_EXP, pos, size, - errp); - if (pos < 0) { - return pos; - } + pos = pci_add_capability(&vdev->pdev, PCI_CAP_ID_EXP, pos, size); vdev->pdev.exp.exp_cap = pos; @@ -2045,14 +2041,14 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos, Error **errp) case PCI_CAP_ID_PM: vfio_check_pm_reset(vdev, pos); vdev->pm_cap = pos; - ret = pci_add_capability(pdev, cap_id, pos, size, errp); + pci_add_capability(pdev, cap_id, pos, size); break; case PCI_CAP_ID_AF: vfio_check_af_flr(vdev, pos); - ret = pci_add_capability(pdev, cap_id, pos, size, errp); + pci_add_capability(pdev, cap_id, pos, size); break; default: - ret = pci_add_capability(pdev, cap_id, pos, size, errp); + pci_add_capability(pdev, cap_id, pos, size); break; }