[SeaBIOS] [PATCH V2 2/2] fw/pci: map memory and IO regions for multiple pci root buses

[Marcel Apfelbaum]

On 02/16/2015 06:36 PM, Kevin O'Connor wrote:
> On Mon, Feb 16, 2015 at 06:14:57PM +0200, Marcel Apfelbaum wrote:
>> On 02/16/2015 06:07 PM, Kevin O'Connor wrote:
>>> On Mon, Feb 16, 2015 at 05:43:31PM +0200, Marcel Apfelbaum wrote:
>>>> On 02/16/2015 04:55 PM, Kevin O'Connor wrote:
>>>>> On Mon, Feb 16, 2015 at 11:31:03AM +0200, Marcel Apfelbaum wrote:
>>>>>> For resource sizing and mapping purposes treat devices on extra root
>>>>>> buses as if they are on the default root bus (bus 0).
>>>>>
>>>> Hi Kevin,
>>>> Thank you for the review.
>>>>
>>>>> Remind me - did you need the resources for each extra root bus to be
>>>>> continuous (ie, not interspersed with resources allocated to devices
>>>>> on the root bus)?  If I read your patch correctly, it wont segregate
>>>>> resources on the extra buses.
>>>>
>>>> I did need them continuous for prev implementation.
>>>> I modified the QEMU code to be generic allowing mem/IO
>>>> regions not to be continuous (per PCI root bus).
>>>> The ACPI and the Operating systems do not require this,
>>>> so the correct way is to achieve better resources utilization.
>>>
>>> Okay, thanks.
>>>
>>> BTW, do you need the res_on_default_bus flag?  Would the below work
>>> (totally untested)?
>> I tried that already :)
>> It will not work if you have a pci-bridge behind a pci-bridge (and so on...)
>
> Okay - I'm missing something then.  The only place bus->bus_dev==NULL
> is for root busses (busses[0] or extra root busses), and the only
> place bus->res_on_default_bus is set is when bus->bus_dev==NULL.  So,
> why can't bus->bus_dev==NULL be tested instead of
> bus->res_on_default_bus?
You are right!
As I previously said, I tried it by myself and failed.
I might have missed some things up when I tested it.

The prev patch also works OK and I prefer it because it is simple.
I'll send a new version shortly.

Thanks,
Marcel

>
> If it can be done, the patch below is likely better (totally untested).
>
> -Kevin
>
>
> --- a/src/fw/pciinit.c
> +++ b/src/fw/pciinit.c
> @@ -601,7 +601,7 @@ static void pci_region_migrate_64bit_entries(struct pci_region *from,
>   }
>
>   static struct pci_region_entry *
> -pci_region_create_entry(struct pci_bus *bus, struct pci_device *dev,
> +pci_region_create_entry(struct pci_bus *busses, struct pci_device *dev,
>                           int bar, u64 size, u64 align, int type, int is64)
>   {
>       struct pci_region_entry *entry = malloc_tmp(sizeof(*entry));
> @@ -617,6 +617,10 @@ pci_region_create_entry(struct pci_bus *bus, struct pci_device *dev,
>       entry->is64 = is64;
>       entry->type = type;
>       // Insert into list in sorted order.
> +    struct pci_bus *bus = &busses[pci_bdf_to_bus(dev->bdf)];
> +    if (!bus->bus_dev)
> +        // Resources for all root busses go in busses[0]
> +        bus = &busses[0];
>       struct hlist_node **pprev;
>       struct pci_region_entry *pos;
>       hlist_for_each_entry_pprev(pos, pprev, &bus->r[type].list, node) {
> @@ -667,7 +671,6 @@ static int pci_bios_check_devices(struct pci_bus *busses)
>           if (pci->class == PCI_CLASS_BRIDGE_PCI)
>               busses[pci->secondary_bus].bus_dev = pci;
>
> -        struct pci_bus *bus = &busses[pci_bdf_to_bus(pci->bdf)];
>           int i;
>           for (i = 0; i < PCI_NUM_REGIONS; i++) {
>               if ((pci->class == PCI_CLASS_BRIDGE_PCI) &&
> @@ -682,7 +685,7 @@ static int pci_bios_check_devices(struct pci_bus *busses)
>               if (type != PCI_REGION_TYPE_IO && size < PCI_DEVICE_MEM_MIN)
>                   size = PCI_DEVICE_MEM_MIN;
>               struct pci_region_entry *entry = pci_region_create_entry(
> -                bus, pci, i, size, size, type, is64);
> +                busses, pci, i, size, size, type, is64);
>               if (!entry)
>                   return -1;
>
> @@ -697,7 +700,6 @@ static int pci_bios_check_devices(struct pci_bus *busses)
>           struct pci_bus *s = &busses[secondary_bus];
>           if (!s->bus_dev)
>               continue;
> -        struct pci_bus *parent = &busses[pci_bdf_to_bus(s->bus_dev->bdf)];
>           int type;
>           int hotplug_support = pci_bus_hotplug_support(s);
>           for (type = 0; type < PCI_REGION_TYPE_COUNT; type++) {
> @@ -715,7 +717,7 @@ static int pci_bios_check_devices(struct pci_bus *busses)
>                                               s->bus_dev, type);
>               // entry->bar is -1 if the entry represents a bridge region
>               struct pci_region_entry *entry = pci_region_create_entry(
> -                parent, s->bus_dev, -1, size, align, type, is64);
> +                busses, s->bus_dev, -1, size, align, type, is64);
>               if (!entry)
>                   return -1;
>               dprintf(1, "PCI: secondary bus %d size %08llx type %s\n",
>