| Message ID | 1519887444-75510-3-git-send-email-heyi.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Add translation support to generic PciHostBridge | expand |
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
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
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
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
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
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
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
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
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,
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