[Qemu-devel] Re: [PATCH 16/20] pci: kill goto in pci_update_mappings()

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Nov 12, 2009 at 02:58:44PM +0900, Isaku Yamahata wrote:
> This patch kills nasty goto in pci_update_mappings().
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  hw/pci.c |   54 ++++++++++++++++++++++++++++--------------------------
>  1 files changed, 28 insertions(+), 26 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index cae3d53..2eff7fe 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -756,35 +756,37 @@ static void pci_update_mappings(PCIDevice *d)
>                      new_addr = pci_get_long(d->config + pci_bar(d, i));
>                  }
>                  /* the ROM slot has a specific enable bit */
> -                if (i == PCI_ROM_SLOT && !(new_addr & PCI_ROM_ADDRESS_ENABLE))
> -                    goto no_mem_map;
> -                new_addr = new_addr & ~(r->size - 1);
> -                last_addr = new_addr + r->size - 1;
> -                /* NOTE: we do not support wrapping */
> -                /* XXX: as we cannot support really dynamic
> -                   mappings, we handle specific values as invalid
> -                   mappings. */
> -                if (last_addr <= new_addr || new_addr == 0 ||

By the way, any idea why do we need new_addr == 0
and last_addr == PCI_BAR_UNMAPPED?
What would it take to fix these?

> -                    last_addr == PCI_BAR_UNMAPPED ||
> -
> -                    /* Now pcibus_t is 64bit.
> -                     * Check if 32 bit BAR wrap around explicitly.
> -                     * Without this, PC ide doesn't work well.
> -                     * TODO: remove this work around.
> -                     */
> -                    (!(r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) &&
> -                     last_addr >= UINT32_MAX) ||
> -
> -                    /*
> -                     * OS is allowed to set BAR beyond its addressable
> -                     * bits. For example, 32 bit OS can set 64bit bar
> -                     * to >4G. Check it.
> -                     */
> -                    last_addr >= TARGET_PHYS_ADDR_MAX) {
> +                if (i == PCI_ROM_SLOT &&
> +                    !(new_addr & PCI_ROM_ADDRESS_ENABLE)) {
>                      new_addr = PCI_BAR_UNMAPPED;
> +                } else {
> +                    new_addr = new_addr & ~(r->size - 1);
> +                    last_addr = new_addr + r->size - 1;
> +                    /* NOTE: we do not support wrapping */
> +                    /* XXX: as we cannot support really dynamic
> +                       mappings, we handle specific values as invalid
> +                       mappings. */
> +                    if (last_addr <= new_addr || new_addr == 0 ||
> +                        last_addr == PCI_BAR_UNMAPPED ||
> +
> +                        /* Now pcibus_t is 64bit.
> +                         * Check if 32 bit BAR wrap around explicitly.
> +                         * Without this, PC ide doesn't work well.
> +                         * TODO: remove this work around.
> +                         */
> +                        (!(r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) &&
> +                         last_addr >= UINT32_MAX) ||
> +
> +                        /*
> +                         * OS is allowed to set BAR beyond its addressable
> +                         * bits. For example, 32 bit OS can set 64bit bar
> +                         * to >4G. Check it.
> +                         */
> +                        last_addr >= TARGET_PHYS_ADDR_MAX) {
> +                        new_addr = PCI_BAR_UNMAPPED;
> +                    }
>                  }
>              } else {
> -            no_mem_map:
>                  new_addr = PCI_BAR_UNMAPPED;
>              }
>          }

Nesting is too deep in pci_update_mappings already.
And this makes it worse. I split out the math into
a subfunction, and it looks better IMO:

--->

From: Michael S. Tsirkin <mst@redhat.com>
Subject: pci: split up up pci_update mappings

Split bar address math into a separate function.
In particular, this gets rid of an ugly forward goto
into scope that we have there.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/hw/pci.c b/hw/pci.c
index 380d43c..847d31e 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -720,14 +720,77 @@ static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
     }
 }
 
+static pcibus_t pci_bar_address(PCIDevice *d,
+				int reg, uint8_t type, pcibus_t size)
+{
+    pcibus_t new_addr, last_addr;
+    int bar = pci_bar(d, reg);
+    uint16_t cmd = pci_get_word(d->config + PCI_COMMAND);
+
+    if (type & PCI_BASE_ADDRESS_SPACE_IO) {
+        if (!(cmd & PCI_COMMAND_IO)) {
+            return PCI_BAR_UNMAPPED;
+        }
+        new_addr = pci_get_long(d->config + bar) & ~(size - 1);
+        last_addr = new_addr + size - 1;
+        /* NOTE: we have only 64K ioports on PC */
+        if (last_addr <= new_addr || new_addr == 0 || last_addr > UINT16_MAX) {
+            return PCI_BAR_UNMAPPED;
+        }
+        return new_addr;
+    }
+
+    if (!(cmd & PCI_COMMAND_MEMORY)) {
+        return PCI_BAR_UNMAPPED;
+    }
+    if (type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
+        new_addr = pci_get_quad(d->config + bar);
+    } else {
+        new_addr = pci_get_long(d->config + bar);
+    }
+    /* the ROM slot has a specific enable bit */
+    if (reg == PCI_ROM_SLOT && !(new_addr & PCI_ROM_ADDRESS_ENABLE)) {
+        return PCI_BAR_UNMAPPED;
+    }
+    new_addr &= ~(size - 1);
+    last_addr = new_addr + size - 1;
+    /* NOTE: we do not support wrapping */
+    /* XXX: as we cannot support really dynamic
+       mappings, we handle specific values as invalid
+       mappings. */
+    if (last_addr <= new_addr || new_addr == 0 ||
+        last_addr == PCI_BAR_UNMAPPED) {
+        return PCI_BAR_UNMAPPED;
+    }
+
+    /* Now pcibus_t is 64bit.
+     * Check if 32 bit BAR wraps around explicitly.
+     * Without this, PC ide doesn't work well.
+     * TODO: remove this work around.
+     */
+    if  (!(type & PCI_BASE_ADDRESS_MEM_TYPE_64) && last_addr >= UINT32_MAX) {
+        return PCI_BAR_UNMAPPED;
+    }
+
+    /*
+     * OS is allowed to set BAR beyond its addressable
+     * bits. For example, 32 bit OS can set 64bit bar
+     * to >4G. Check it. TODO: we might need to support
+     * it in the future for e.g. PAE.
+     */
+    if (last_addr >= TARGET_PHYS_ADDR_MAX) {
+        return PCI_BAR_UNMAPPED;
+    }
+
+    return new_addr;
+}
+
 static void pci_update_mappings(PCIDevice *d)
 {
     PCIIORegion *r;
-    int cmd, i;
-    pcibus_t last_addr, new_addr;
-    pcibus_t filtered_size;
+    int i;
+    pcibus_t new_addr, filtered_size;
 
-    cmd = pci_get_word(d->config + PCI_COMMAND);
     for(i = 0; i < PCI_NUM_REGIONS; i++) {
         r = &d->io_regions[i];
 
@@ -735,59 +798,7 @@ static void pci_update_mappings(PCIDevice *d)
         if (!r->size)
             continue;
 
-        if (r->type & PCI_BASE_ADDRESS_SPACE_IO) {
-            if (cmd & PCI_COMMAND_IO) {
-                new_addr = pci_get_long(d->config + pci_bar(d, i));
-                new_addr = new_addr & ~(r->size - 1);
-                last_addr = new_addr + r->size - 1;
-                /* NOTE: we have only 64K ioports on PC */
-                if (last_addr <= new_addr || new_addr == 0 ||
-                    last_addr >= 0x10000) {
-                    new_addr = PCI_BAR_UNMAPPED;
-                }
-            } else {
-                new_addr = PCI_BAR_UNMAPPED;
-            }
-        } else {
-            if (cmd & PCI_COMMAND_MEMORY) {
-                if (r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
-                    new_addr = pci_get_quad(d->config + pci_bar(d, i));
-                } else {
-                    new_addr = pci_get_long(d->config + pci_bar(d, i));
-                }
-                /* the ROM slot has a specific enable bit */
-                if (i == PCI_ROM_SLOT && !(new_addr & PCI_ROM_ADDRESS_ENABLE))
-                    goto no_mem_map;
-                new_addr = new_addr & ~(r->size - 1);
-                last_addr = new_addr + r->size - 1;
-                /* NOTE: we do not support wrapping */
-                /* XXX: as we cannot support really dynamic
-                   mappings, we handle specific values as invalid
-                   mappings. */
-                if (last_addr <= new_addr || new_addr == 0 ||
-                    last_addr == PCI_BAR_UNMAPPED ||
-
-                    /* Now pcibus_t is 64bit.
-                     * Check if 32 bit BAR wrap around explicitly.
-                     * Without this, PC ide doesn't work well.
-                     * TODO: remove this work around.
-                     */
-                    (!(r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) &&
-                     last_addr >= UINT32_MAX) ||
-
-                    /*
-                     * OS is allowed to set BAR beyond its addressable
-                     * bits. For example, 32 bit OS can set 64bit bar
-                     * to >4G. Check it.
-                     */
-                    last_addr >= TARGET_PHYS_ADDR_MAX) {
-                    new_addr = PCI_BAR_UNMAPPED;
-                }
-            } else {
-            no_mem_map:
-                new_addr = PCI_BAR_UNMAPPED;
-            }
-        }
+        new_addr = pci_bar_address(d, i, r->type, r->size);
 
         /* bridge filtering */
         filtered_size = r->size;