qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Kevin O'Connor
Subject: Re: [Qemu-devel] [PATCH 3/4] Switch from array based resource allocation to list
Date: Wed, 11 Apr 2012 23:15:51 -0400
User-agent: Mutt/1.5.21 (2010-09-15)

On Thu, Apr 12, 2012 at 02:44:54PM +1200, Alexey Korolev wrote:
> 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.

Yes.  It's your patches tweaked slightly.

> 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.

Okay - will do.

> [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;

This was me playing with some of Gerd's patches.  I think it would
require additional changes before committing.  In particular, the
patch misses the ROM bar on bridges - I think maybe it should be
changed to keep the same loop constraints but add a "if (bar >
PCI_BRIDGE_NUM_REGIONS && bar < PCI_ROM_SLOT) continue;".

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

This was also me playing with one of Gerd's patches.  It just makes
the bar read/write code 64bit aware.  It doesn't actually program
them.  The logic to do real 64bit allocations would require list
merging.  Is this something you have looked at?

-Kevin



reply via email to

[Prev in Thread] Current Thread [Next in Thread]