qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Akihiko Odaki <akihiko.odaki@daynix.com>
Cc: Alex Williamson <alex.williamson@redhat.com>,
	qemu-devel@nongnu.org, qemu-block@nongnu.org,
	qemu-arm@nongnu.org, "Michael S . Tsirkin" <mst@redhat.com>,
	Marcel Apfelbaum <marcel.apfelbaum@gmail.com>,
	Gerd Hoffmann <kraxel@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Richard Henderson <richard.henderson@linaro.org>,
	Eduardo Habkost <eduardo@habkost.net>,
	John Snow <jsnow@redhat.com>,
	Dmitry Fleytman <dmitry.fleytman@gmail.com>,
	Jason Wang <jasowang@redhat.com>, Stefan Weil <sw@weilnetz.de>,
	Keith Busch <kbusch@kernel.org>, Klaus Jensen <its@irrelevant.dk>,
	Peter Maydell <peter.maydell@linaro.org>,
	Andrey Smirnov <andrew.smirnov@gmail.com>,
	Paul Burton <paulburton@kernel.org>,
	Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>,
	Yan Vugenfirer <yan@daynix.com>,
	Yuri Benditovich <yuri.benditovich@daynix.com>,
	Akihiko Odaki <akihiko.odaki@daynix.com>
Subject: [PATCH v3 01/16] pci: Allow to omit errp for pci_add_capability
Date: Thu, 27 Oct 2022 05:15:12 +0900	[thread overview]
Message-ID: <20221026201527.24063-2-akihiko.odaki@daynix.com> (raw)
In-Reply-To: <20221026201527.24063-1-akihiko.odaki@daynix.com>

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/hw/pci/pci.c b/hw/pci/pci.c
index 2f450f6a72..8ee2171011 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2513,14 +2513,14 @@ static void pci_del_option_rom(PCIDevice *pdev)
 }
 
 /*
- * On success, pci_add_capability() returns a positive value
+ * On success, pci_add_capability_legacy() returns a positive value
  * that the offset of the pci capability.
  * On failure, it sets an error and returns a negative error
  * code.
  */
-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)
 {
     uint8_t *config;
     int i, overlapping_cap;
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)
 
 void pci_del_capability(PCIDevice *pci_dev, uint8_t cap_id, uint8_t cap_size);
 
-- 
2.37.3



  reply	other threads:[~2022-10-26 20:19 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-10-26 20:15 [PATCH v3 00/16] pci: Abort if pci_add_capability fails Akihiko Odaki
2022-10-26 20:15 ` Akihiko Odaki [this message]
2022-10-26 20:15 ` [PATCH v3 02/16] hw/i386/amd_iommu: Omit errp for pci_add_capability Akihiko Odaki
2022-10-27  5:43   ` Markus Armbruster
2022-10-26 20:15 ` [PATCH v3 03/16] ahci: " Akihiko Odaki
2022-10-26 20:15 ` [PATCH v3 04/16] e1000e: " Akihiko Odaki
2022-10-26 20:15 ` [PATCH v3 05/16] eepro100: " Akihiko Odaki
2022-10-26 20:15 ` [PATCH v3 06/16] hw/nvme: " Akihiko Odaki
2022-10-26 20:15 ` [PATCH v3 07/16] msi: " Akihiko Odaki
2022-10-26 20:15 ` [PATCH v3 08/16] hw/pci/pci_bridge: " Akihiko Odaki
2022-10-26 20:15 ` [PATCH v3 09/16] pcie: " Akihiko Odaki
2022-10-26 20:15 ` [PATCH v3 10/16] pci/shpc: " Akihiko Odaki
2022-10-26 20:15 ` [PATCH v3 11/16] msix: " Akihiko Odaki
2022-10-26 20:15 ` [PATCH v3 12/16] pci/slotid: " Akihiko Odaki
2022-10-26 20:15 ` [PATCH v3 13/16] hw/pci-bridge/pcie_pci_bridge: " Akihiko Odaki
2022-10-26 20:15 ` [PATCH v3 14/16] hw/vfio/pci: " Akihiko Odaki
2022-10-26 22:10   ` Alex Williamson
2022-10-26 20:15 ` [PATCH v3 15/16] virtio-pci: " Akihiko Odaki
2022-10-26 20:15 ` [PATCH v3 16/16] pci: Remove legacy errp from pci_add_capability Akihiko Odaki

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20221026201527.24063-2-akihiko.odaki@daynix.com \
    --to=akihiko.odaki@daynix.com \
    --cc=aleksandar.rikalo@syrmia.com \
    --cc=alex.williamson@redhat.com \
    --cc=andrew.smirnov@gmail.com \
    --cc=dmitry.fleytman@gmail.com \
    --cc=eduardo@habkost.net \
    --cc=its@irrelevant.dk \
    --cc=jasowang@redhat.com \
    --cc=jsnow@redhat.com \
    --cc=kbusch@kernel.org \
    --cc=kraxel@redhat.com \
    --cc=marcel.apfelbaum@gmail.com \
    --cc=mst@redhat.com \
    --cc=paulburton@kernel.org \
    --cc=pbonzini@redhat.com \
    --cc=peter.maydell@linaro.org \
    --cc=qemu-arm@nongnu.org \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=richard.henderson@linaro.org \
    --cc=sw@weilnetz.de \
    --cc=yan@daynix.com \
    --cc=yuri.benditovich@daynix.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).