Re: [PATCH] pci: Abort if pci_add_capability fails

[Markus Armbruster <armbru@redhat.com>]

Alex, got a question for you below.

小田喜陽彦 <akihiko.odaki@daynix.com> writes:

> From: Akihiko Odaki <akihiko.odaki@daynix.com>
>
> pci_add_capability appears most PCI devices. The 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
>
> pci_add_capability fails if the new capability overlaps with an existing
> one. It happens only if the device implementation is wrong so
> pci_add_capability can just abort instead of returning to the caller in
> the case, fixing inconsistencies and removing extra code.

It already abort()s when passed a zero @offset and there is not enough
config space left.

> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>

[...]

> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 2f450f6a72..5831dfc742 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2513,14 +2513,11 @@ static void pci_del_option_rom(PCIDevice *pdev)
>  }
>  
>  /*
> - * On success, pci_add_capability() returns a positive value
> - * that the offset of the pci capability.
> - * On failure, it sets an error and returns a negative error
> - * code.
> + * pci_add_capability() returns a positive value that the offset of the pci
> + * capability.

Suggest "returns the (positive) offset of the PCI capability".

>   */
> -int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
> -                       uint8_t offset, uint8_t size,
> -                       Error **errp)
> +uint8_t pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
> +                           uint8_t offset, uint8_t size)
>  {
>      uint8_t *config;
>      int i, overlapping_cap;
> @@ -2537,13 +2534,12 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
       if (!offset) {
           offset = pci_find_space(pdev, size);
           /* out of PCI config space is programming error */
           assert(offset);
       } else {
           /* 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. */

The comment makes me suspect that device assignment of a broken device
could trigger the error.  It goes back to

commit c9abe111209abca1b910e35c6ca9888aced5f183
Author: Jan Kiszka <jan.kiszka@siemens.com>
Date:   Wed Aug 24 14:29:30 2011 +0200

    pci: Error on PCI capability collisions
    
    Nothing good can happen when we overlap capabilities. This may happen
    when plugging in assigned devices or when devices models contain bugs.
    Detect the overlap and report it.
    
    Based on qemu-kvm commit by Alex Williamson.
    
    Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
    Acked-by: Don Dutile <ddutile@redhat.com>
    Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

If this is still correct, then your patch is a regression: QEMU is no
longer able to gracefully handle assignment of a broken device.  Does
this matter?  Alex, maybe?

Even if we must avoid this regression, you still make a compelling
argument for *virtual* devices, where pci_add_capability() failure is
always a programming error.  Simplest solution: change them all to pass
&error_abort.

Alternatively, add a wrapper that does, then call that.  Not sure that's
worth the bother.

>          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 "
> +                error_setg(&error_abort, "%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);

error.h advises:

 * Please don't error_setg(&error_fatal, ...), use error_report() and
 * exit(), because that's more obvious.
 * Likewise, don't error_setg(&error_abort, ...), use assert().

Thus

               assert(!overlapping_cap);

or even

               assert(!pci_find_capability_at_offset(pdev, i);

> -                return -EINVAL;
>              }
>          }
>      }

[...]