Re: [Qemu-devel] [PATCH 2/6] Redesign of pciinit.c (take 2)
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
Re: [Qemu-devel] [PATCH 2/6] Redesign of pciinit.c (take 2)
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.
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?
[...]
>
> -
> /
> * Main setup code
Minor - whitespace change.
-Kevin
Re: [Qemu-devel] [PATCH 2/6] Redesign of pciinit.c (take 2)
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(e
Re: [Qemu-devel] [PATCH 2/6] Redesign of pciinit.c (take 2)
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. The code is being changed -
it's not new code being added and old code being deleted - the patches
need to reflect that.
(If it's a pain to move the struct out of struct pci_device at the
start, then add your new fields into struct pci_device.bars and use a
patch to move the whole thing out of pci.h at the end.)
-Kevin
