Re: [PATCH v8 01/17] hw/vfio/pci: Ensure MSI and MSI-X do not overlap

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 1/11/22 14:57, Akihiko Odaki 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/pci/pci.c         | 34 ++++++++++++++++++++++------------
>   hw/vfio/pci.c        | 15 ++++++++++++++-
>   include/hw/pci/pci.h |  3 +++
>   3 files changed, 39 insertions(+), 13 deletions(-)

>   /*
>    * On success, pci_add_capability() returns a positive value
>    * that the offset of the pci capability.
> @@ -2523,7 +2542,6 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
>                          Error **errp)
>   {
>       uint8_t *config;
> -    int i, overlapping_cap;
>   
>       if (!offset) {
>           offset = pci_find_space(pdev, size);
> @@ -2534,17 +2552,9 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
>            * 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. */
> -        for (i = offset; i < offset + size; i++) {
> -            overlapping_cap = pci_find_capability_at_offset(pdev, i);
> -            if (overlapping_cap) {
> -                error_setg(errp, "%s:%02x:%02x.%x "
> -                           "Attempt to add PCI capability %x at offset "
> -                           "%x overlaps existing capability %x at offset %x",
> -                           pci_root_bus_path(pdev), pci_dev_bus_num(pdev),
> -                           PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn),
> -                           cap_id, offset, overlapping_cap, i);
> -                return -EINVAL;
> -            }
> +        pci_check_capability_overlap(pdev, cap_id, offset, size, errp);
> +        if (errp) {

            if (!pci_check_capability_overlap(...)) {

> +            return -EINVAL;
>           }
>       }