qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v5 02/17] pci: Allow to omit errp for pci_add_capability


From: Philippe Mathieu-Daudé
Subject: Re: [PATCH v5 02/17] pci: Allow to omit errp for pci_add_capability
Date: Sat, 29 Oct 2022 00:35:36 +0200
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.4.0

On 28/10/22 14:26, 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

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. Therefore, we can always assert that
capabilities never overlap when pci_add_capability is called, resolving
these inconsistencies.

Such an implementation of pci_add_capability will not have errp
parameter. However, there are so many callers of pci_add_capability
that it does not make sense to amend all of them at once to match
with the new signature. Instead, this change will allow callers of
pci_add_capability to omit errp as the first step.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
  hw/pci/pci.c         |  8 ++++----
  include/hw/pci/pci.h | 13 ++++++++++---
  2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index b54b6ef88f..51fd106f16 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -2,6 +2,7 @@
  #define QEMU_PCI_H
#include "exec/memory.h"
+#include "qapi/error.h"
  #include "sysemu/dma.h"
/* PCI includes legacy ISA access. */
@@ -390,9 +391,15 @@ void pci_register_vga(PCIDevice *pci_dev, MemoryRegion 
*mem,
  void pci_unregister_vga(PCIDevice *pci_dev);
  pcibus_t pci_get_bar_addr(PCIDevice *pci_dev, int region_num);
-int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
-                       uint8_t offset, uint8_t size,
-                       Error **errp);
+int pci_add_capability_legacy(PCIDevice *pdev, uint8_t cap_id,
+                              uint8_t offset, uint8_t size,
+                              Error **errp);
+
+#define PCI_ADD_CAPABILITY_VA(pdev, cap_id, offset, size, errp, ...) \
+    pci_add_capability_legacy(pdev, cap_id, offset, size, errp)
+
+#define pci_add_capability(...) \
+    PCI_ADD_CAPABILITY_VA(__VA_ARGS__, &error_abort)
Do we really need PCI_ADD_CAPABILITY_VA?

  #define pci_add_capability(...) \
       pci_add_capability_legacy(__VA_ARGS__, &error_abort)




reply via email to

[Prev in Thread] Current Thread [Next in Thread]