Re: [Qemu-devel] [PATCH 3/4] Switch from array based resource allocation to list

[Alexey Korolev]

On 04/04/12 15:31, Kevin O'Connor wrote:
> Agreed - the only thing it does is force a minimum size for memory bars as 
> you pointed out in a previous email. As above, I did play with
> this a little more on Sunday. I also added in two patches from Gerd's series 
> and made alignment handling more explicit. I'm including the
> series here if you're interested. Again, I think this should wait until after 
> the 1.7.0 release. -Kevin 
Hi Kevin,

Sorry for late response, was quite busy.
I've reviewed and tried your patches.

[Patches 1-4] are almost the same as I proposed in this series.
The noticeable differences are:
1) instead of sorting entries at mapping stage your patches sort entries at 
allocation stage. (no difference in behavior or readability so
any approach is fine for me)
2) instead of storing pointer to bus resource which the bridge device 
represents (this_bus), it is obtained from pci structure and array of
"busses".
  -     entry->this_bus->r[entry->type].base = entry->base;
 +    struct pci_bus *child_bus = &busses[entry->dev->secondary_bus];
 +    child_bus->r[entry->type].base = addr;
Since  "entry->this_bus" member is removed the information about whether the 
resource is bridge region or PCI BAR is encoded inside
"entry->bar".
Value "-1" - means this is a bridge region, the positive values mean it is a 
BAR.
IMHO 2) makes code less readable, at least a comment explaining the meaning of 
negative value of PCI BAR is required.
I've found just cosmetic difference to my patches so please don't forget to add 
my sign-off-by to them.

[Patch 5] Track region alignment explicitly. Looks very good and safe. Should 
reduce address range wastes.

[Patch 6] Small patch to account resources of PCI bridges.
Looks fine.
May be instead of
+            num_regions = 2;
I would add
#define PCI_BRIDGE_NUM_REGIONS 2
.....
+            num_regions = PCI_BRIDGE_NUM_REGIONS;

[Patch 7] 64bit aware code. Patch is incomplete. It is not working.

After 1.7.0 is fine.