From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:55480) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S9ohX-0001bG-FV for qemu-devel@nongnu.org; Mon, 19 Mar 2012 22:21:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S9ohV-0003tY-EI for qemu-devel@nongnu.org; Mon, 19 Mar 2012 22:21:31 -0400 Received: from usrksweb02.endace.com ([174.143.168.194]:39789) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S9ohV-0003qR-80 for qemu-devel@nongnu.org; Mon, 19 Mar 2012 22:21:29 -0400 Message-ID: <4F67E981.3070709@endace.com> Date: Tue, 20 Mar 2012 15:20:49 +1300 From: Alexey Korolev MIME-Version: 1.0 References: <1331613556.26071.43.camel@nzhmlwks0057.ad.endace.com> <1331613919.26071.49.camel@nzhmlwks0057.ad.endace.com> <20120314004837.GB2252@morn.localdomain> <4F61621A.40805@endace.com> <20120316005544.GA4834@morn.localdomain> In-Reply-To: <20120316005544.GA4834@morn.localdomain> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/6] Redesign of pciinit.c (take 2) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin O'Connor Cc: sfd@endace.com, seabios@seabios.org, qemu-devel@nongnu.org On 16/03/12 13:55, Kevin O'Connor wrote: > On Thu, Mar 15, 2012 at 04:29:30PM +1300, Alexey Korolev wrote: >> 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 { >>> - u32 addr; >>> - u32 size; >>> - int is64; >>> - } bars[PCI_NUM_REGIONS]; > [...] >> Yes I see what you mean here. > Thanks - I do find this patch much easier to understand. I do have > some comments on the patch (see below). > >>> 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) > On patch 3/4, neither patch should introduce code that isn't actually > used nor leave code that isn't used still in. So, for example, if the > arrays are still used after patch 3 then it's okay to leave them to > patch 4, but if they are no longer used at all they should be removed > within patch 3. > >> 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? > patch 1/6 - I'd prefer to have a list.h with struct > hlist_head/hlist_node and container_of before extending to other > parts of seabios. That said, I'm okay with what you have for > pciinit - we can introduce list.h afterwards. Then, it should be a separate patch. It's is better to do this afterwards. > patch 3/4 - was confusing to me as it added new code in one patch and > removed the replaced code in the second patch. > > patch 5 - looked okay to me. > > Thanks for looking at this - I know it's time consuming. Given the > churn in this area I want to make sure there is a good understanding > before any big changes. > > comments on the patch: > [...] >> +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; > Minor - whitespace. > > [...] >> +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; > Minor - I prefer to use C99 inline variable declarations though it > isn't a requirement. > >> + min_size = (type == PCI_REGION_TYPE_IO) ? (1<> + (1<> + size = (size > min_size) ? size : min_size; > My only real comment: Why the min_size change? Is that a fix of some > sort or is it related to the move to lists? This is a good question. The min_size changes are to keep the exactly the same behaviour as the original implementation, when each PCI MEM bar is aligned to 4KB (1< [...] >> >> - >> /**************************************************************** >> * Main setup code > Minor - whitespace change. > > -Kevin