diff mbox series

[v2,02/17] hw/i386/amd_iommu: Omit errp for pci_add_capability

Message ID 20221022044053.81650-3-akihiko.odaki@daynix.com
State New
Headers show
Series pci: Abort if pci_add_capability fails | expand

Commit Message

Akihiko Odaki Oct. 22, 2022, 4:40 a.m. UTC 
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 hw/i386/amd_iommu.c | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

Comments

Markus Armbruster Oct. 25, 2022, 12:23 p.m. UTC | #1 
Akihiko Odaki <akihiko.odaki@daynix.com> writes:

> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  hw/i386/amd_iommu.c | 21 ++++-----------------
>  1 file changed, 4 insertions(+), 17 deletions(-)
>
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 725f69095b..8a88cbea0a 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -1539,7 +1539,6 @@ static void amdvi_sysbus_reset(DeviceState *dev)
>  
>  static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
>  {
> -    int ret = 0;
>      AMDVIState *s = AMD_IOMMU_DEVICE(dev);
>      MachineState *ms = MACHINE(qdev_get_machine());
>      PCMachineState *pcms = PC_MACHINE(ms);
       X86MachineState *x86ms = X86_MACHINE(ms);
       PCIBus *bus = pcms->bus;

       s->iotlb = g_hash_table_new_full(amdvi_uint64_hash,
                                        amdvi_uint64_equal, g_free, g_free);

> @@ -1553,23 +1552,11 @@ static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
>      if (!qdev_realize(DEVICE(&s->pci), &bus->qbus, errp)) {
>          return;
>      }
> -    ret = pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC, 0,
> -                                         AMDVI_CAPAB_SIZE, errp);
> -    if (ret < 0) {
> -        return;
> -    }
> -    s->capab_offset = ret;
> +    s->capab_offset = pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC, 0,
> +                                         AMDVI_CAPAB_SIZE);
>  
> -    ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_MSI, 0,
> -                             AMDVI_CAPAB_REG_SIZE, errp);
> -    if (ret < 0) {
> -        return;
> -    }
> -    ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_HT, 0,
> -                             AMDVI_CAPAB_REG_SIZE, errp);
> -    if (ret < 0) {
> -        return;
> -    }
> +    pci_add_capability(&s->pci.dev, PCI_CAP_ID_MSI, 0, AMDVI_CAPAB_REG_SIZE);
> +    pci_add_capability(&s->pci.dev, PCI_CAP_ID_HT, 0, AMDVI_CAPAB_REG_SIZE);
>  
>      /* Pseudo address space under root PCI bus. */
>      x86ms->ioapic_as = amdvi_host_dma_iommu(bus, s, AMDVI_IOAPIC_SB_DEVID);

Your patch replaces error handling by abort().  The commit message
should explain why this is okay.

It is, because these are programming errors, and aborting on programming
errors is appropriate.

Moreover, the error handling is incorrect: it leaks s->iotlb.  To be clear:
replacing it would be okay even if it cleaned up properly.

The other patches also need to explain.  Yes, this repetitive, but
anyone looking at one of these commits later will be grateful.  Yes,
they could find an explanation in PATCH 01.  If they find it in git
history.  Each commit should make sense on its own whenever practical.

The explanation will be a bit more involved for device assignment (PATCH
15, maybe more), where we need to point out that the caller guards
against these errors.
diff mbox series

Patch

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 725f69095b..8a88cbea0a 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1539,7 +1539,6 @@  static void amdvi_sysbus_reset(DeviceState *dev)
 
 static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
 {
-    int ret = 0;
     AMDVIState *s = AMD_IOMMU_DEVICE(dev);
     MachineState *ms = MACHINE(qdev_get_machine());
     PCMachineState *pcms = PC_MACHINE(ms);
@@ -1553,23 +1552,11 @@  static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
     if (!qdev_realize(DEVICE(&s->pci), &bus->qbus, errp)) {
         return;
     }
-    ret = pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC, 0,
-                                         AMDVI_CAPAB_SIZE, errp);
-    if (ret < 0) {
-        return;
-    }
-    s->capab_offset = ret;
+    s->capab_offset = pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC, 0,
+                                         AMDVI_CAPAB_SIZE);
 
-    ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_MSI, 0,
-                             AMDVI_CAPAB_REG_SIZE, errp);
-    if (ret < 0) {
-        return;
-    }
-    ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_HT, 0,
-                             AMDVI_CAPAB_REG_SIZE, errp);
-    if (ret < 0) {
-        return;
-    }
+    pci_add_capability(&s->pci.dev, PCI_CAP_ID_MSI, 0, AMDVI_CAPAB_REG_SIZE);
+    pci_add_capability(&s->pci.dev, PCI_CAP_ID_HT, 0, AMDVI_CAPAB_REG_SIZE);
 
     /* Pseudo address space under root PCI bus. */
     x86ms->ioapic_as = amdvi_host_dma_iommu(bus, s, AMDVI_IOAPIC_SB_DEVID);