[edk2,v5,2/6] OvmfPkg/PciHostBridgeLib: Init PCI aperture to 0

Message ID 1519887444-75510-3-git-send-email-heyi.guo@linaro.org
State New
Headers show
Series
  • Add translation support to generic PciHostBridge
Related show

Commit Message

gary guo March 1, 2018, 6:57 a.m.
Use ZeroMem to initialize all fields in temporary
PCI_ROOT_BRIDGE_APERTURE variables to zero. This is not mandatory but
is helpful for future extension: when we add new fields to
PCI_ROOT_BRIDGE_APERTURE and the default value of these fields can
safely be zero, this code will not suffer from an additional
change.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Heyi Guo <heyi.guo@linaro.org>


Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Julien Grall <julien.grall@linaro.org>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 4 ++++
 OvmfPkg/Library/PciHostBridgeLib/XenSupport.c       | 5 +++++
 2 files changed, 9 insertions(+)

-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Laszlo Ersek March 1, 2018, 10:17 a.m. | #1
Hello Heyi,

On 03/01/18 07:57, Heyi Guo wrote:
> Use ZeroMem to initialize all fields in temporary

> PCI_ROOT_BRIDGE_APERTURE variables to zero. This is not mandatory but

> is helpful for future extension: when we add new fields to

> PCI_ROOT_BRIDGE_APERTURE and the default value of these fields can

> safely be zero, this code will not suffer from an additional

> change.

>

> Contributed-under: TianoCore Contribution Agreement 1.1

> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>

>

> Cc: Jordan Justen <jordan.l.justen@intel.com>

> Cc: Anthony Perard <anthony.perard@citrix.com>

> Cc: Julien Grall <julien.grall@linaro.org>

> Cc: Ruiyu Ni <ruiyu.ni@intel.com>

> Cc: Laszlo Ersek <lersek@redhat.com>

> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 4 ++++

>  OvmfPkg/Library/PciHostBridgeLib/XenSupport.c       | 5 +++++

>  2 files changed, 9 insertions(+)

>

> diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c

> index ff837035caff..4a650a4c6df9 100644

> --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c

> +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c

> @@ -217,6 +217,10 @@ PciHostBridgeGetRootBridges (

>    PCI_ROOT_BRIDGE_APERTURE Mem;

>    PCI_ROOT_BRIDGE_APERTURE MemAbove4G;

>

> +  ZeroMem (&Io, sizeof (Io));

> +  ZeroMem (&Mem, sizeof (Mem));

> +  ZeroMem (&MemAbove4G, sizeof (MemAbove4G));

> +

>    if (PcdGetBool (PcdPciDisableBusEnumeration)) {

>      return ScanForRootBridges (Count);

>    }


This is OK. (Although for a trivial perf improvement, you could move the
ZeroMem() calls after the PcdGetBool() / return. Not necessary, up to
you.)

However:

> diff --git a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c

> index 31c63ae19e0a..aaf101dfcb0e 100644

> --- a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c

> +++ b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c

> @@ -193,6 +193,11 @@ ScanForRootBridges (

>

>    *NumberOfRootBridges = 0;

>    RootBridges = NULL;

> +  ZeroMem (&Io, sizeof (Io));

> +  ZeroMem (&Mem, sizeof (Mem));

> +  ZeroMem (&MemAbove4G, sizeof (MemAbove4G));

> +  ZeroMem (&PMem, sizeof (PMem));

> +  ZeroMem (&PMemAbove4G, sizeof (PMemAbove4G));

>

>    //

>    // After scanning all the PCI devices on the PCI root bridge's primary bus,

>


these ZeroMem() calls are not in the correct place. Please move them
into the "PrimaryBus" loop just underneath. That loop works like this:

For each primary bus:

  (1) set all of the aperture variables to "nonexistent":

    Io.Base = Mem.Base = MemAbove4G.Base = PMem.Base = PMemAbove4G.Base = MAX_UINT64;
    Io.Limit = Mem.Limit = MemAbove4G.Limit = PMem.Limit = PMemAbove4G.Limit = 0;

  (2) accumulate the BARs of the devices on the bus into the aperture
      variables

  (3) call InitRootBridge() with the aperture variables


That is, the ZeroMem() calls that you are adding have to be part of step
(1). So, please replace the assignments

    Io.Base = Mem.Base = MemAbove4G.Base = PMem.Base = PMemAbove4G.Base = MAX_UINT64;
    Io.Limit = Mem.Limit = MemAbove4G.Limit = PMem.Limit = PMemAbove4G.Limit = 0;

with

    ZeroMem (&Io, sizeof (Io));
    ZeroMem (&Mem, sizeof (Mem));
    ZeroMem (&MemAbove4G, sizeof (MemAbove4G));
    ZeroMem (&PMem, sizeof (PMem));
    ZeroMem (&PMemAbove4G, sizeof (PMemAbove4G));
    Io.Base = Mem.Base = MemAbove4G.Base = PMem.Base = PMemAbove4G.Base = MAX_UINT64;

Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek March 1, 2018, 10:20 a.m. | #2
On 03/01/18 07:57, Heyi Guo wrote:
> Use ZeroMem to initialize all fields in temporary

> PCI_ROOT_BRIDGE_APERTURE variables to zero. This is not mandatory but

> is helpful for future extension: when we add new fields to

> PCI_ROOT_BRIDGE_APERTURE and the default value of these fields can

> safely be zero, this code will not suffer from an additional

> change.

> 

> Contributed-under: TianoCore Contribution Agreement 1.1

> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>

> 

> Cc: Jordan Justen <jordan.l.justen@intel.com>

> Cc: Anthony Perard <anthony.perard@citrix.com>

> Cc: Julien Grall <julien.grall@linaro.org>

> Cc: Ruiyu Ni <ruiyu.ni@intel.com>

> Cc: Laszlo Ersek <lersek@redhat.com>

> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 4 ++++

>  OvmfPkg/Library/PciHostBridgeLib/XenSupport.c       | 5 +++++

>  2 files changed, 9 insertions(+)


I also suggest a different subject line:

OvmfPkg/PciHostBridgeLib: clear PCI_ROOT_BRIDGE_APERTURE vars for (re)init

(74 chars)

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
gary guo March 1, 2018, 10:25 a.m. | #3
Thanks; I got some trouble in making the subject short and clear :)

Regards,
Heyi

On Thu, Mar 01, 2018 at 11:20:22AM +0100, Laszlo Ersek wrote:
> On 03/01/18 07:57, Heyi Guo wrote:

> > Use ZeroMem to initialize all fields in temporary

> > PCI_ROOT_BRIDGE_APERTURE variables to zero. This is not mandatory but

> > is helpful for future extension: when we add new fields to

> > PCI_ROOT_BRIDGE_APERTURE and the default value of these fields can

> > safely be zero, this code will not suffer from an additional

> > change.

> > 

> > Contributed-under: TianoCore Contribution Agreement 1.1

> > Signed-off-by: Heyi Guo <heyi.guo@linaro.org>

> > 

> > Cc: Jordan Justen <jordan.l.justen@intel.com>

> > Cc: Anthony Perard <anthony.perard@citrix.com>

> > Cc: Julien Grall <julien.grall@linaro.org>

> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>

> > Cc: Laszlo Ersek <lersek@redhat.com>

> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> > ---

> >  OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 4 ++++

> >  OvmfPkg/Library/PciHostBridgeLib/XenSupport.c       | 5 +++++

> >  2 files changed, 9 insertions(+)

> 

> I also suggest a different subject line:

> 

> OvmfPkg/PciHostBridgeLib: clear PCI_ROOT_BRIDGE_APERTURE vars for (re)init

> 

> (74 chars)

> 

> Thanks

> Laszlo

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
gary guo March 1, 2018, 10:48 a.m. | #4
On Thu, Mar 01, 2018 at 11:17:30AM +0100, Laszlo Ersek wrote:
> Hello Heyi,

> 

> On 03/01/18 07:57, Heyi Guo wrote:

> > Use ZeroMem to initialize all fields in temporary

> > PCI_ROOT_BRIDGE_APERTURE variables to zero. This is not mandatory but

> > is helpful for future extension: when we add new fields to

> > PCI_ROOT_BRIDGE_APERTURE and the default value of these fields can

> > safely be zero, this code will not suffer from an additional

> > change.

> >

> > Contributed-under: TianoCore Contribution Agreement 1.1

> > Signed-off-by: Heyi Guo <heyi.guo@linaro.org>

> >

> > Cc: Jordan Justen <jordan.l.justen@intel.com>

> > Cc: Anthony Perard <anthony.perard@citrix.com>

> > Cc: Julien Grall <julien.grall@linaro.org>

> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>

> > Cc: Laszlo Ersek <lersek@redhat.com>

> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> > ---

> >  OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 4 ++++

> >  OvmfPkg/Library/PciHostBridgeLib/XenSupport.c       | 5 +++++

> >  2 files changed, 9 insertions(+)

> >

> > diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c

> > index ff837035caff..4a650a4c6df9 100644

> > --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c

> > +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c

> > @@ -217,6 +217,10 @@ PciHostBridgeGetRootBridges (

> >    PCI_ROOT_BRIDGE_APERTURE Mem;

> >    PCI_ROOT_BRIDGE_APERTURE MemAbove4G;

> >

> > +  ZeroMem (&Io, sizeof (Io));

> > +  ZeroMem (&Mem, sizeof (Mem));

> > +  ZeroMem (&MemAbove4G, sizeof (MemAbove4G));

> > +

> >    if (PcdGetBool (PcdPciDisableBusEnumeration)) {

> >      return ScanForRootBridges (Count);

> >    }

> 

> This is OK. (Although for a trivial perf improvement, you could move the

> ZeroMem() calls after the PcdGetBool() / return. Not necessary, up to

> you.)

> 

> However:

> 

> > diff --git a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c

> > index 31c63ae19e0a..aaf101dfcb0e 100644

> > --- a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c

> > +++ b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c

> > @@ -193,6 +193,11 @@ ScanForRootBridges (

> >

> >    *NumberOfRootBridges = 0;

> >    RootBridges = NULL;

> > +  ZeroMem (&Io, sizeof (Io));

> > +  ZeroMem (&Mem, sizeof (Mem));

> > +  ZeroMem (&MemAbove4G, sizeof (MemAbove4G));

> > +  ZeroMem (&PMem, sizeof (PMem));

> > +  ZeroMem (&PMemAbove4G, sizeof (PMemAbove4G));

> >

> >    //

> >    // After scanning all the PCI devices on the PCI root bridge's primary bus,

> >

> 

> these ZeroMem() calls are not in the correct place. Please move them

> into the "PrimaryBus" loop just underneath. That loop works like this:

> 

> For each primary bus:

> 

>   (1) set all of the aperture variables to "nonexistent":

> 

>     Io.Base = Mem.Base = MemAbove4G.Base = PMem.Base = PMemAbove4G.Base = MAX_UINT64;

>     Io.Limit = Mem.Limit = MemAbove4G.Limit = PMem.Limit = PMemAbove4G.Limit = 0;

> 

>   (2) accumulate the BARs of the devices on the bus into the aperture

>       variables

> 

>   (3) call InitRootBridge() with the aperture variables

> 

> 

> That is, the ZeroMem() calls that you are adding have to be part of step

> (1). So, please replace the assignments

> 

>     Io.Base = Mem.Base = MemAbove4G.Base = PMem.Base = PMemAbove4G.Base = MAX_UINT64;

>     Io.Limit = Mem.Limit = MemAbove4G.Limit = PMem.Limit = PMemAbove4G.Limit = 0;

> 

> with

> 

>     ZeroMem (&Io, sizeof (Io));

>     ZeroMem (&Mem, sizeof (Mem));

>     ZeroMem (&MemAbove4G, sizeof (MemAbove4G));

>     ZeroMem (&PMem, sizeof (PMem));

>     ZeroMem (&PMemAbove4G, sizeof (PMemAbove4G));

>     Io.Base = Mem.Base = MemAbove4G.Base = PMem.Base = PMemAbove4G.Base = MAX_UINT64;


Will it cause functional issue?

My idea of making the change is like this:

1. ZeroMem() is used to initialize all fields of APERTURE to 0; it can make it
   in the current place of the patch;

2. In the loop, some fields may be changed by the end of each iteration, and it
   is the responsibility of the existing code to re-initialize the changed fields
   to expected values explicitly. It seems not necessary to re-initialize the other
   fields which will not be changed.

However, your advice may be better that merges the initialization code together.
I can make the change in the next version of patch.

Thanks,
Heyi

> 

> Thanks!

> Laszlo

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ni, Ruiyu March 1, 2018, 12:03 p.m. | #5
On 3/1/2018 6:20 PM, Laszlo Ersek wrote:
> On 03/01/18 07:57, Heyi Guo wrote:

>> Use ZeroMem to initialize all fields in temporary

>> PCI_ROOT_BRIDGE_APERTURE variables to zero. This is not mandatory but

>> is helpful for future extension: when we add new fields to

>> PCI_ROOT_BRIDGE_APERTURE and the default value of these fields can

>> safely be zero, this code will not suffer from an additional

>> change.

>>

>> Contributed-under: TianoCore Contribution Agreement 1.1

>> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>

>>

>> Cc: Jordan Justen <jordan.l.justen@intel.com>

>> Cc: Anthony Perard <anthony.perard@citrix.com>

>> Cc: Julien Grall <julien.grall@linaro.org>

>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>

>> Cc: Laszlo Ersek <lersek@redhat.com>

>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> ---

>>   OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 4 ++++

>>   OvmfPkg/Library/PciHostBridgeLib/XenSupport.c       | 5 +++++

>>   2 files changed, 9 insertions(+)

> 

> I also suggest a different subject line:

> 

> OvmfPkg/PciHostBridgeLib: clear PCI_ROOT_BRIDGE_APERTURE vars for (re)init

> 

> (74 chars)


I sometimes tries very hard to make the subject line be <= 70 chars.
74 is acceptable?

> 

> Thanks

> Laszlo

> 



-- 
Thanks,
Ray
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek March 2, 2018, 10:08 a.m. | #6
On 03/01/18 13:03, Ni, Ruiyu wrote:
> On 3/1/2018 6:20 PM, Laszlo Ersek wrote:
>> On 03/01/18 07:57, Heyi Guo wrote:
>>> Use ZeroMem to initialize all fields in temporary
>>> PCI_ROOT_BRIDGE_APERTURE variables to zero. This is not mandatory but
>>> is helpful for future extension: when we add new fields to
>>> PCI_ROOT_BRIDGE_APERTURE and the default value of these fields can
>>> safely be zero, this code will not suffer from an additional
>>> change.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
>>>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> Cc: Anthony Perard <anthony.perard@citrix.com>
>>> Cc: Julien Grall <julien.grall@linaro.org>
>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>   OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 4 ++++
>>>   OvmfPkg/Library/PciHostBridgeLib/XenSupport.c       | 5 +++++
>>>   2 files changed, 9 insertions(+)
>>
>> I also suggest a different subject line:
>>
>> OvmfPkg/PciHostBridgeLib: clear PCI_ROOT_BRIDGE_APERTURE vars for
>> (re)init
>>
>> (74 chars)
> 
> I sometimes tries very hard to make the subject line be <= 70 chars.
> 74 is acceptable?

To my knowledge, the Linux kernel development guidelines suggest
wrapping the commit message body at 74 characters, and IIRC the same
limit applies to the subject line. I tend to follow these ideas for edk2
development too.

I think anything under 74 chars (for the subject) is an unreasonable
expectation for edk2. First, we start with a prefix of the form

  XxxPkg/Module: ...

Sometimes this prefix is incredibly long alread :/ So what I do (and I
guess most others do as well) is that I first write an "honest" subject
line (which frequently reaches 90-100 chars), and then work on
compressing it down to 74 characters. Sometimes it becomes a real
struggle, with strange abbreviations etc. I might make an exception and
go up to 75-76, and hope that nobody notices :) So, in that range,
limiting ourselves to 70 chars would be catastrophic.

Thanks!
Laszlo
Laszlo Ersek March 2, 2018, 10:19 a.m. | #7
On 03/01/18 11:48, Guo Heyi wrote:
> On Thu, Mar 01, 2018 at 11:17:30AM +0100, Laszlo Ersek wrote:

>> On 03/01/18 07:57, Heyi Guo wrote:


>>> diff --git a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c

>>> index 31c63ae19e0a..aaf101dfcb0e 100644

>>> --- a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c

>>> +++ b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c

>>> @@ -193,6 +193,11 @@ ScanForRootBridges (

>>>

>>>    *NumberOfRootBridges = 0;

>>>    RootBridges = NULL;

>>> +  ZeroMem (&Io, sizeof (Io));

>>> +  ZeroMem (&Mem, sizeof (Mem));

>>> +  ZeroMem (&MemAbove4G, sizeof (MemAbove4G));

>>> +  ZeroMem (&PMem, sizeof (PMem));

>>> +  ZeroMem (&PMemAbove4G, sizeof (PMemAbove4G));

>>>

>>>    //

>>>    // After scanning all the PCI devices on the PCI root bridge's primary bus,

>>>

>>

>> these ZeroMem() calls are not in the correct place. Please move them

>> into the "PrimaryBus" loop just underneath. That loop works like

>> this:

>>

>> For each primary bus:

>>

>>   (1) set all of the aperture variables to "nonexistent":

>>

>>     Io.Base = Mem.Base = MemAbove4G.Base = PMem.Base = PMemAbove4G.Base = MAX_UINT64;

>>     Io.Limit = Mem.Limit = MemAbove4G.Limit = PMem.Limit = PMemAbove4G.Limit = 0;

>>

>>   (2) accumulate the BARs of the devices on the bus into the aperture

>>       variables

>>

>>   (3) call InitRootBridge() with the aperture variables

>>

>>

>> That is, the ZeroMem() calls that you are adding have to be part of

>> step (1). So, please replace the assignments

>>

>>     Io.Base = Mem.Base = MemAbove4G.Base = PMem.Base = PMemAbove4G.Base = MAX_UINT64;

>>     Io.Limit = Mem.Limit = MemAbove4G.Limit = PMem.Limit = PMemAbove4G.Limit = 0;

>>

>> with

>>

>>     ZeroMem (&Io, sizeof (Io));

>>     ZeroMem (&Mem, sizeof (Mem));

>>     ZeroMem (&MemAbove4G, sizeof (MemAbove4G));

>>     ZeroMem (&PMem, sizeof (PMem));

>>     ZeroMem (&PMemAbove4G, sizeof (PMemAbove4G));

>>     Io.Base = Mem.Base = MemAbove4G.Base = PMem.Base = PMemAbove4G.Base = MAX_UINT64;

>

> Will it cause functional issue?

>

> My idea of making the change is like this:

>

> 1. ZeroMem() is used to initialize all fields of APERTURE to 0; it can

>    make it in the current place of the patch;

>

> 2. In the loop, some fields may be changed by the end of each

>    iteration, and it is the responsibility of the existing code to

>    re-initialize the changed fields to expected values explicitly. It

>    seems not necessary to re-initialize the other fields which will

>    not be changed.

>

> However, your advice may be better that merges the initialization code

> together. I can make the change in the next version of patch.


Yes, if it's not a big problem for you, please implement my request.
Going forward I wouldn't like to depend on such intricate details as
described in your point (2). Namely, in any other C project, I would
suggest that we write:

  for (PrimaryBus = 0; PrimaryBus <= PCI_MAX_BUS; PrimaryBus = SubBus + 1) {
    PCI_ROOT_BRIDGE_APERTURE Io = { .Base = MAX_UINT64 },
      Mem = Io,
      MemAbove4G = Io,
      PMem = Io,
      PMemAbove4G = Io;
    /* ... */
  }

In other words, I would:
- move the definition of the structs into the loop (sort of accepted,
  but not really liked in edk2),
- use real C initialization (forbidden in edk2),
- use designated initializers for the first object, which clears the
  unlisted fields (C99, forbidden in edk2),
- initialize the rest of the structs from the first struct where I used
  the designated initializer explicitly.

Moving the ZeroMem() into the loop is the closest approximation of this,
for edk2.

Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
gary guo March 5, 2018, 8:23 a.m. | #8
On Fri, Mar 02, 2018 at 11:19:31AM +0100, Laszlo Ersek wrote:
> On 03/01/18 11:48, Guo Heyi wrote:

> > On Thu, Mar 01, 2018 at 11:17:30AM +0100, Laszlo Ersek wrote:

> >> On 03/01/18 07:57, Heyi Guo wrote:

> 

> >>> diff --git a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c

> >>> index 31c63ae19e0a..aaf101dfcb0e 100644

> >>> --- a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c

> >>> +++ b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c

> >>> @@ -193,6 +193,11 @@ ScanForRootBridges (

> >>>

> >>>    *NumberOfRootBridges = 0;

> >>>    RootBridges = NULL;

> >>> +  ZeroMem (&Io, sizeof (Io));

> >>> +  ZeroMem (&Mem, sizeof (Mem));

> >>> +  ZeroMem (&MemAbove4G, sizeof (MemAbove4G));

> >>> +  ZeroMem (&PMem, sizeof (PMem));

> >>> +  ZeroMem (&PMemAbove4G, sizeof (PMemAbove4G));

> >>>

> >>>    //

> >>>    // After scanning all the PCI devices on the PCI root bridge's primary bus,

> >>>

> >>

> >> these ZeroMem() calls are not in the correct place. Please move them

> >> into the "PrimaryBus" loop just underneath. That loop works like

> >> this:

> >>

> >> For each primary bus:

> >>

> >>   (1) set all of the aperture variables to "nonexistent":

> >>

> >>     Io.Base = Mem.Base = MemAbove4G.Base = PMem.Base = PMemAbove4G.Base = MAX_UINT64;

> >>     Io.Limit = Mem.Limit = MemAbove4G.Limit = PMem.Limit = PMemAbove4G.Limit = 0;

> >>

> >>   (2) accumulate the BARs of the devices on the bus into the aperture

> >>       variables

> >>

> >>   (3) call InitRootBridge() with the aperture variables

> >>

> >>

> >> That is, the ZeroMem() calls that you are adding have to be part of

> >> step (1). So, please replace the assignments

> >>

> >>     Io.Base = Mem.Base = MemAbove4G.Base = PMem.Base = PMemAbove4G.Base = MAX_UINT64;

> >>     Io.Limit = Mem.Limit = MemAbove4G.Limit = PMem.Limit = PMemAbove4G.Limit = 0;

> >>

> >> with

> >>

> >>     ZeroMem (&Io, sizeof (Io));

> >>     ZeroMem (&Mem, sizeof (Mem));

> >>     ZeroMem (&MemAbove4G, sizeof (MemAbove4G));

> >>     ZeroMem (&PMem, sizeof (PMem));

> >>     ZeroMem (&PMemAbove4G, sizeof (PMemAbove4G));

> >>     Io.Base = Mem.Base = MemAbove4G.Base = PMem.Base = PMemAbove4G.Base = MAX_UINT64;

> >

> > Will it cause functional issue?

> >

> > My idea of making the change is like this:

> >

> > 1. ZeroMem() is used to initialize all fields of APERTURE to 0; it can

> >    make it in the current place of the patch;

> >

> > 2. In the loop, some fields may be changed by the end of each

> >    iteration, and it is the responsibility of the existing code to

> >    re-initialize the changed fields to expected values explicitly. It

> >    seems not necessary to re-initialize the other fields which will

> >    not be changed.

> >

> > However, your advice may be better that merges the initialization code

> > together. I can make the change in the next version of patch.

> 

> Yes, if it's not a big problem for you, please implement my request.

> Going forward I wouldn't like to depend on such intricate details as

> described in your point (2). Namely, in any other C project, I would

> suggest that we write:

> 

>   for (PrimaryBus = 0; PrimaryBus <= PCI_MAX_BUS; PrimaryBus = SubBus + 1) {

>     PCI_ROOT_BRIDGE_APERTURE Io = { .Base = MAX_UINT64 },

>       Mem = Io,

>       MemAbove4G = Io,

>       PMem = Io,

>       PMemAbove4G = Io;

>     /* ... */

>   }

> 

> In other words, I would:

> - move the definition of the structs into the loop (sort of accepted,

>   but not really liked in edk2),

> - use real C initialization (forbidden in edk2),

> - use designated initializers for the first object, which clears the

>   unlisted fields (C99, forbidden in edk2),

> - initialize the rest of the structs from the first struct where I used

>   the designated initializer explicitly.

> 

> Moving the ZeroMem() into the loop is the closest approximation of this,

> for edk2.


OK, I can do that in the next version of patch.

Thanks,

Heyi

> 

> Thanks!

> Laszlo

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
index ff837035caff..4a650a4c6df9 100644
--- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
+++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
@@ -217,6 +217,10 @@  PciHostBridgeGetRootBridges (
   PCI_ROOT_BRIDGE_APERTURE Mem;
   PCI_ROOT_BRIDGE_APERTURE MemAbove4G;
 
+  ZeroMem (&Io, sizeof (Io));
+  ZeroMem (&Mem, sizeof (Mem));
+  ZeroMem (&MemAbove4G, sizeof (MemAbove4G));
+
   if (PcdGetBool (PcdPciDisableBusEnumeration)) {
     return ScanForRootBridges (Count);
   }
diff --git a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
index 31c63ae19e0a..aaf101dfcb0e 100644
--- a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
+++ b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
@@ -193,6 +193,11 @@  ScanForRootBridges (
 
   *NumberOfRootBridges = 0;
   RootBridges = NULL;
+  ZeroMem (&Io, sizeof (Io));
+  ZeroMem (&Mem, sizeof (Mem));
+  ZeroMem (&MemAbove4G, sizeof (MemAbove4G));
+  ZeroMem (&PMem, sizeof (PMem));
+  ZeroMem (&PMemAbove4G, sizeof (PMemAbove4G));
 
   //
   // After scanning all the PCI devices on the PCI root bridge's primary bus,