Re: [PATCH v6 00/17] pci: Abort if pci_add_capability fails

[Michael S. Tsirkin]

On Mon, Oct 31, 2022 at 09:33:02PM +0900, Akihiko Odaki wrote:
> pci_add_capability appears most PCI devices. Its error handling required
> lots of code, and led to inconsistent behaviors such as:
> - passing error_abort
> - passing error_fatal
> - asserting the returned value
> - propagating the error to the caller
> - skipping the rest of the function
> - just ignoring

Thanks for the patchset! I don't think I'll be merging it for
this merge window, the benefit seems small and chance of regressions
high.
I will tag this but pls remind me after the freeze to help make sure I
do not lose it.


> The code generating errors in pci_add_capability had 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 and the only exception are MSI and MSI-X
> capabilities. Therefore, we can add code specific to the case, and
> always assert that capabilities never overlap in the other cases,
> resolving these inconsistencies.
> 
> v6:
> - Error in case of MSI/MSI-X capability overlap (Alex Williamson)
> 
> v5:
> - Fix capability ID specification in vfio_msi_early_setup (Alex Williamson)
> - Use range_covers_byte() (Alex Williamson)
> - warn_report() in case of MSI/MSI-X capability overlap (Alex Williamson)
> 
> v4:
> - Fix typos in messages (Markus Armbruster)
> - hw/vfio/pci: Ensure MSI and MSI-X do not overlap (Alex Williamson)
> 
> v3:
> - Correct patch split between virtio-pci and pci (Markus Armbruster)
> - Add messages for individual patches (Markus Armbruster)
> - Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Akihiko Odaki (17):
>   hw/vfio/pci: Ensure MSI and MSI-X do not overlap
>   pci: Allow to omit errp for pci_add_capability
>   hw/i386/amd_iommu: Omit errp for pci_add_capability
>   ahci: Omit errp for pci_add_capability
>   e1000e: Omit errp for pci_add_capability
>   eepro100: Omit errp for pci_add_capability
>   hw/nvme: Omit errp for pci_add_capability
>   msi: Omit errp for pci_add_capability
>   hw/pci/pci_bridge: Omit errp for pci_add_capability
>   pcie: Omit errp for pci_add_capability
>   pci/shpc: Omit errp for pci_add_capability
>   msix: Omit errp for pci_add_capability
>   pci/slotid: Omit errp for pci_add_capability
>   hw/pci-bridge/pcie_pci_bridge: Omit errp for pci_add_capability
>   hw/vfio/pci: Omit errp for pci_add_capability
>   virtio-pci: Omit errp for pci_add_capability
>   pci: Remove legacy errp from pci_add_capability
> 
>  docs/pcie_sriov.txt                |  4 +-
>  hw/display/bochs-display.c         |  4 +-
>  hw/i386/amd_iommu.c                | 21 ++-------
>  hw/ide/ich.c                       |  8 +---
>  hw/net/e1000e.c                    | 22 ++-------
>  hw/net/eepro100.c                  |  7 +--
>  hw/nvme/ctrl.c                     | 14 +-----
>  hw/pci-bridge/cxl_downstream.c     |  9 +---
>  hw/pci-bridge/cxl_upstream.c       |  8 +---
>  hw/pci-bridge/i82801b11.c          | 14 +-----
>  hw/pci-bridge/pci_bridge_dev.c     |  2 +-
>  hw/pci-bridge/pcie_pci_bridge.c    | 19 ++------
>  hw/pci-bridge/pcie_root_port.c     | 16 +------
>  hw/pci-bridge/xio3130_downstream.c | 15 ++----
>  hw/pci-bridge/xio3130_upstream.c   | 15 ++----
>  hw/pci-host/designware.c           |  3 +-
>  hw/pci-host/xilinx-pcie.c          |  4 +-
>  hw/pci/msi.c                       |  9 +---
>  hw/pci/msix.c                      |  8 +---
>  hw/pci/pci.c                       | 29 +++---------
>  hw/pci/pci_bridge.c                | 21 +++------
>  hw/pci/pcie.c                      | 52 ++++++---------------
>  hw/pci/shpc.c                      | 23 +++------
>  hw/pci/slotid_cap.c                |  8 +---
>  hw/usb/hcd-xhci-pci.c              |  3 +-
>  hw/vfio/pci-quirks.c               | 15 ++----
>  hw/vfio/pci.c                      | 75 ++++++++++++++++++++++--------
>  hw/vfio/pci.h                      |  3 ++
>  hw/virtio/virtio-pci.c             | 12 ++---
>  include/hw/pci/pci.h               |  5 +-
>  include/hw/pci/pci_bridge.h        |  5 +-
>  include/hw/pci/pcie.h              | 11 ++---
>  include/hw/pci/shpc.h              |  3 +-
>  include/hw/virtio/virtio-pci.h     |  2 +-
>  34 files changed, 153 insertions(+), 316 deletions(-)
> 
> -- 
> 2.38.1