Re: [PATCH v3 14/16] hw/vfio/pci: Omit errp for pci_add_capability

[Alex Williamson]

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