Re: [Qemu-devel] [PATCH 2/6] Redesign of pciinit.c (take 2)

[Alexey Korolev <alexey.korolev@endace.com>]

On 14/03/12 13:48, Kevin O'Connor wrote:
> On Tue, Mar 13, 2012 at 05:45:19PM +1300, Alexey Korolev wrote:
>> Added pci_region_entry structure and list operations to pciinit.c
>> List is filled with entries during pci_check_devices.
>> List is used just for printing space allocation if we were using lists. 
>> Next step will resource allocation using mapping functions.
> [...]
>> +struct pci_bus;
>> +struct pci_region_entry {
>> +    struct pci_device *dev;
>> +    int bar;
>> +    u32 base;
>> +    u32 size;
>> +    int is64bit;
>> +    enum pci_region_type type;
>> +    struct pci_bus *this_bus;
>> +    struct pci_bus *parent_bus;
>> +    struct pci_region_entry *next;
>> +    struct pci_region_entry **pprev;
>> +};
> It's fine to introduce a new struct, but a patch that does this should
> have something like the following in the same patch:
>
> --- a/src/pci.h
> +++ b/src/pci.h
> @@ -51,11 +51,6 @@ struct pci_device {
>      u8 prog_if, revision;
>      u8 header_type;
>      u8 secondary_bus;
> -    struct {
> -        u32 addr;
> -        u32 size;
> -        int is64;
> -    } bars[PCI_NUM_REGIONS];
>  
>      // Local information on device.
>      int have_driver;
>
> And it should compile and work fine after applying just that one
> patch.  That is, you're not introducing a new struct, you're moving
> the contents from one struct to another. 
Yes I see what you mean here.
Basically I kept pci_device->bars and pci_region_entry altogether because they are for different things.
The pci_region_entry describes bridge regions in addition to bars and contains information to build topology.

In your proposal for patches splitting the pci_device->bars are removed and pci_region_entry data
is used to program pci bars. This is reasonable so I've made the changes.  See patch below in this message.

Of course further patches [3-6] won't apply on top of this, so the series should be reposted.

>  The code is being changed -
> it's not new code being added and old code being deleted - the patches
> need to reflect that.
Because of structural changes it is not possible to completely avoid this scenario where
new code is added and old deleted.
In this patch series I tried my best to make migration as obvious and safe as possible.
So the existing approach (with your suggestions) for pciinit.c redesign is this:

1. Introduce list operations
2. Introduce pci_region_entry structure and add code which fills this new structure.
We don't modify resource addresses calculations, but we use pci_region_entry data to do resource assignment.
3. Modify resource addresses calculations to be based on linked lists of region entries.
4. Remove code which fills the arrays, remove use of arrays for mapping.
(note 3&4 could be combined altogether but it will be harder to read then)

Could you please have a look at the other parts in this series and let me know if you are happy about this approach,
so I won't have to redo patchwork too many times?


---
 src/pci.h     |    5 --
 src/pciinit.c |  122 ++++++++++++++++++++++++++++++++++++++++++---------------
 2 files changed, 90 insertions(+), 37 deletions(-)

diff --git a/src/pci.h b/src/pci.h
index a2a5a4c..5598100 100644
--- a/src/pci.h
+++ b/src/pci.h
@@ -51,11 +51,6 @@ struct pci_device {
     u8 prog_if, revision;
     u8 header_type;
     u8 secondary_bus;
-    struct {
-        u32 addr;
-        u32 size;
-        int is64;
-    } bars[PCI_NUM_REGIONS];
 
     // Local information on device.
     int have_driver;
diff --git a/src/pciinit.c b/src/pciinit.c
index 9f3fdd4..f75f393 100644
--- a/src/pciinit.c
+++ b/src/pciinit.c
@@ -31,6 +31,20 @@ static const char *region_type_name[] = {
     [ PCI_REGION_TYPE_PREFMEM ] = "prefmem",
 };
 
+struct pci_bus;
+struct pci_region_entry {
+    struct pci_device *dev;
+    int bar;
+    u32 base;
+    u32 size;
+    int is64bit;
+    enum pci_region_type type;
+    struct pci_bus *this_bus;
+    struct pci_bus *parent_bus;
+    struct pci_region_entry *next;
+    struct pci_region_entry **pprev;
+};
+
 struct pci_bus {
     struct {
         /* pci region stats */
@@ -41,6 +55,7 @@ struct pci_bus {
         /* pci region assignments */
         u32 bases[32 - PCI_MEM_INDEX_SHIFT];
         u32 base;
+        struct pci_region_entry *list;
     } r[PCI_REGION_TYPE_COUNT];
     struct pci_device *bus_dev;
 };
@@ -352,6 +367,31 @@ pci_bios_get_bar(struct pci_device *pci, int bar, u32 *val, u32 *size)
     *size = (~(*val & mask)) + 1;
 }
 
+/****************************************************************
+ * Build topology and calculate size of entries
+ ****************************************************************/
+
+struct pci_region_entry *
+pci_region_create_entry(struct pci_bus *bus, struct pci_device *dev,
+                       u32 size, int type, int is64bit)
+{
+    struct pci_region_entry *entry= malloc_tmp(sizeof(*entry));
+    if (!entry) {
+            warn_noalloc();
+            return NULL;
+    }
+    memset(entry, 0, sizeof(*entry));
+
+    entry->dev = dev;
+    entry->type = type;
+    entry->is64bit = is64bit;
+    entry->size = size;
+
+    list_add_head(&bus->r[type].list, entry);
+    entry->parent_bus = bus;
+    return entry;
+}
+
 static void pci_bios_bus_reserve(struct pci_bus *bus, int type, u32 size)
 {
     u32 index;
@@ -364,9 +404,10 @@ static void pci_bios_bus_reserve(struct pci_bus *bus, int type, u32 size)
         bus->r[type].max = size;
 }
 
-static void pci_bios_check_devices(struct pci_bus *busses)
+static int pci_bios_check_devices(struct pci_bus *busses)
 {
     dprintf(1, "PCI: check devices\n");
+    struct pci_region_entry *entry;
 
     // Calculate resources needed for regular (non-bus) devices.
     struct pci_device *pci;
@@ -378,19 +419,27 @@ static void pci_bios_check_devices(struct pci_bus *busses)
         struct pci_bus *bus = &busses[pci_bdf_to_bus(pci->bdf)];
         int i;
         for (i = 0; i < PCI_NUM_REGIONS; i++) {
-            u32 val, size;
+            u32 val, size, min_size;
+            int type, is64bit;
             pci_bios_get_bar(pci, i, &val, &size);
             if (val == 0)
                 continue;
-
-            pci_bios_bus_reserve(bus, pci_addr_to_type(val), size);
-            pci->bars[i].addr = val;
-            pci->bars[i].size = size;
-            pci->bars[i].is64 = (!(val & PCI_BASE_ADDRESS_SPACE_IO) &&
+            type = pci_addr_to_type(val);
+            min_size = (type == PCI_REGION_TYPE_IO) ? (1<<PCI_IO_INDEX_SHIFT):
+                     (1<<PCI_MEM_INDEX_SHIFT);
+            size = (size > min_size) ? size : min_size;
+            is64bit = (!(val & PCI_BASE_ADDRESS_SPACE_IO) &&
                                  (val & PCI_BASE_ADDRESS_MEM_TYPE_MASK)
                                  == PCI_BASE_ADDRESS_MEM_TYPE_64);
 
-            if (pci->bars[i].is64)
+            pci_bios_bus_reserve(bus, pci_addr_to_type(val), size);
+
+            entry = pci_region_create_entry(bus, pci, size, type, is64bit);
+            if (!entry)
+                return -1;
+            entry->bar = i;
+
+            if (is64bit)
                 i++;
         }
     }
@@ -411,6 +460,11 @@ static void pci_bios_check_devices(struct pci_bus *busses)
                 s->r[type].size = limit;
             s->r[type].size = pci_size_roundup(s->r[type].size);
             pci_bios_bus_reserve(parent, type, s->r[type].size);
+            entry = pci_region_create_entry(parent, s->bus_dev,
+                                            s->r[type].size, type, 0);
+            if (!entry)
+                return -1;
+            entry->this_bus = s;
         }
         dprintf(1, "PCI: secondary bus %d sizes: io %x, mem %x, prefmem %x\n",
                 secondary_bus,
@@ -418,6 +472,7 @@ static void pci_bios_check_devices(struct pci_bus *busses)
                 s->r[PCI_REGION_TYPE_MEM].size,
                 s->r[PCI_REGION_TYPE_PREFMEM].size);
     }
+    return 0;
 }
 
 #define ROOT_BASE(top, sum, max) ALIGN_DOWN((top)-(sum),(max) ?: 1)
@@ -526,34 +581,35 @@ static void pci_bios_map_devices(struct pci_bus *busses)
     }
 
     // Map regions on each device.
-    struct pci_device *pci;
-    foreachpci(pci) {
-        if (pci->class == PCI_CLASS_BRIDGE_PCI)
-            continue;
-        u16 bdf = pci->bdf;
-        dprintf(1, "PCI: map device bdf=%02x:%02x.%x\n"
-                , pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf), pci_bdf_to_fn(bdf));
-        struct pci_bus *bus = &busses[pci_bdf_to_bus(bdf)];
-        int i;
-        for (i = 0; i < PCI_NUM_REGIONS; i++) {
-            if (pci->bars[i].addr == 0)
-                continue;
-
-            int type = pci_addr_to_type(pci->bars[i].addr);
-            u32 addr = pci_bios_bus_get_addr(bus, type, pci->bars[i].size);
-            dprintf(1, "  bar %d, addr %x, size %x [%s]\n",
-                    i, addr, pci->bars[i].size, region_type_name[type]);
-            pci_set_io_region_addr(pci, i, addr);
-
-            if (pci->bars[i].is64) {
-                i++;
-                pci_set_io_region_addr(pci, i, 0);
+    int bus;
+    struct pci_region_entry *entry, *next;
+    for (bus = 0; bus<=MaxPCIBus; bus++) {
+        int type;
+        for (type = 0; type < PCI_REGION_TYPE_COUNT; type++) {
+            list_foreach_entry_safe(busses[bus].r[type].list,
+                                      next, entry) {
+                if (!entry->this_bus) {
+                    entry->base = pci_bios_bus_get_addr(&busses[bus],
+                                                      entry->type, entry->size);
+                    pci_set_io_region_addr(entry->dev, entry->bar, entry->base);
+                    if (entry->is64bit)
+                        pci_set_io_region_addr(entry->dev, entry->bar, 0);
+
+                    dprintf(1, "PCI: map device bdf=%02x:%02x.%x \tbar %d"
+                                "\tsize\t0x%08x\tbase 0x%x type %s\n",
+                               pci_bdf_to_bus(entry->dev->bdf),
+                               pci_bdf_to_dev(entry->dev->bdf),
+                               pci_bdf_to_fn(entry->dev->bdf),
+                               entry->bar, entry->size, entry->base,
+                               region_type_name[entry->type]);
+                }
+                list_del(entry);
+                free(entry);
             }
         }
     }
 }
 
-
 /****************************************************************
  * Main setup code
  ****************************************************************/
@@ -588,7 +644,9 @@ pci_setup(void)
         return;
     }
     memset(busses, 0, sizeof(*busses) * (MaxPCIBus + 1));
-    pci_bios_check_devices(busses);
+    if (pci_bios_check_devices(busses))
+        return;
+
     if (pci_bios_init_root_regions(&busses[0], start, end) != 0) {
         panic("PCI: out of address space\n");
     }
-- 
1.7.5.4