Re: [edk2] [PATCH v2] SecurityPkg: Fix assert when setting key from eMMC/SD/USB
Yes, it is being taking care of.
On Mon, Jul 16, 2018 at 8:50 AM, Yao, Jiewen wrote:
> Laszlo already filed one - https://bugzilla.tianocore.
> org/show_bug.cgi?id=1008
>
> I suggest we add to UefiLib instead of fixing all individual driver.
>
> Thank you
> Yao Jiewen
>
>
> > -Original Message-
> > From: Zhang, Chao B
> > Sent: Monday, July 16, 2018 11:10 PM
> > To: rba...@gmail.com; edk2-devel@lists.01.org
> > Cc: Yao, Jiewen ; Laszlo Ersek >;
> > Vladimir Olovyannikov
> > Subject: RE: [PATCH v2] SecurityPkg: Fix assert when setting key from
> > eMMC/SD/USB
> >
> > Hi Bacik:
> >Tks for the fix. Would you please file another report in Bugzilla for
> RamDisk
> > & Tls Configuration driver? They have same issue as SecureBootConfig
> driver
> >
> > -Original Message-
> > From: rba...@gmail.com [mailto:rba...@gmail.com]
> > Sent: Wednesday, July 11, 2018 6:51 AM
> > To: edk2-devel@lists.01.org
> > Cc: Zhang, Chao B ; Yao, Jiewen
> > ; Laszlo Ersek ; Vladimir
> > Olovyannikov
> > Subject: [PATCH v2] SecurityPkg: Fix assert when setting key from
> > eMMC/SD/USB
> >
> > From: Roman Bacik
> >
> > When secure boot is enabled, if one loads keys from a FAT formatted
> > eMMC/SD/USB when trying to provision PK/KEK/DB keys via the menu, an
> > assert in StrLen() occurs.
> > This is because the filename starts on odd address, which is not a uint16
> > aligned boundary: https://bugzilla.tianocore.org/show_bug.cgi?id=1003
> >
> > Cc: Chao Zhang
> > Cc: Jiewen Yao
> > Cc: Laszlo Ersek
> > Cc: Vladimir Olovyannikov
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Roman Bacik
> > ---
> >
> > SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/
> SecureBootConfigFil
> > eExplorer.c | 13 +++--
> > 1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git
> > a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfig
> > FileExplorer.c
> > b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfig
> > FileExplorer.c
> > index 1b6f88804275..19b13a5569a6 100644
> > ---
> > a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfig
> > FileExplorer.c
> > +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootCo
> > +++ nfigFileExplorer.c
> > @@ -123,6 +123,8 @@ OpenFileByDevicePath(
> >EFI_FILE_PROTOCOL *Handle1;
> >EFI_FILE_PROTOCOL *Handle2;
> >EFI_HANDLE DeviceHandle;
> > + CHAR16 *PathName;
> > + UINTN PathLength;
> >
> >if ((FilePath == NULL || FileHandle == NULL)) {
> > return EFI_INVALID_PARAMETER;
> > @@ -173,6 +175,11 @@ OpenFileByDevicePath(
> > //
> > Handle2 = Handle1;
> > Handle1 = NULL;
> > +PathLength = DevicePathNodeLength(*FilePath) -
> > sizeof(EFI_DEVICE_PATH_PROTOCOL);
> > +PathName = AllocateCopyPool(PathLength,
> > ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName);
> > +if (PathName == NULL) {
> > + return EFI_OUT_OF_RESOURCES;
> > +}
> >
> > //
> > // Try to test opening an existing file @@ -180,7 +187,7 @@
> > OpenFileByDevicePath(
> > Status = Handle2->Open (
> >Handle2,
> >,
> > -
> > ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
> > + PathName,
> >OpenMode &~EFI_FILE_MODE_CREATE,
> >0
> > );
> > @@ -192,7 +199,7 @@ OpenFileByDevicePath(
> >Status = Handle2->Open (
> > Handle2,
> > ,
> > -
> > ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
> > +PathName,
> > OpenMode,
> > Attributes
> > );
> > @@ -202,6 +209,8 @@ OpenFileByDevicePath(
> > //
> > Handle2->Close (Handle2);
> >
> > +FreePool (PathName);
> > +
> > if (EFI_ERROR(Status)) {
> >return (Status);
> > }
> > --
> > 2.17.1
>
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2] SecurityPkg: Fix assert when setting key from eMMC/SD/USB
Hi Laszlo,
Thank you very much for your review and help. I would prefer the option 2b.
Thanks,
Roman
On Wed, Jul 11, 2018 at 5:05 AM, Laszlo Ersek wrote:
> Hi Roman,
>
> On 07/11/18 00:51, rba...@gmail.com wrote:
> > From: Roman Bacik
> >
> > When secure boot is enabled, if one loads keys from a FAT formatted
> > eMMC/SD/USB when trying to provision PK/KEK/DB keys via the menu,
> > an assert in StrLen() occurs.
> > This is because the filename starts on odd address, which is not a uint16
> > aligned boundary: https://bugzilla.tianocore.org/show_bug.cgi?id=1003
> >
> > Cc: Chao Zhang
> > Cc: Jiewen Yao
> > Cc: Laszlo Ersek
> > Cc: Vladimir Olovyannikov
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Roman Bacik
> > ---
> >
> > SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
> | 13 +++--
> > 1 file changed, 11 insertions(+), 2 deletions(-)
>
> Thank you for sending a well-formed patch.
>
> I notice that you sent this email from , which is not
> the same as the Signed-off-by line. I realize you posted from
> for technical reasons, and it should be no problem.
>
> However, I *think* in such cases we usually request the following:
>
> - Using your broadcom.com email address, please respond to this patch
> (not my present email, but your original git posting), keeping full
> context, and just repeat your Signed-off-by line (referencing the
> broadcom address).
>
> I'm CC'ing Jordan and Ard for confirmation -- I believe this is what
> we've done in the past, in cases when submitters had to post their work
> from private addresses due to company email issues.
>
> Technical comments below:
>
> > diff --git
> > a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/
> SecureBootConfigFileExplorer.c
> > index 1b6f88804275..19b13a5569a6 100644
> > --- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/
> SecureBootConfigFileExplorer.c
> > +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/
> SecureBootConfigFileExplorer.c
> > @@ -123,6 +123,8 @@ OpenFileByDevicePath(
> >EFI_FILE_PROTOCOL *Handle1;
> >EFI_FILE_PROTOCOL *Handle2;
> >EFI_HANDLE DeviceHandle;
> > + CHAR16 *PathName;
> > + UINTN PathLength;
> >
> >if ((FilePath == NULL || FileHandle == NULL)) {
> > return EFI_INVALID_PARAMETER;
> > @@ -173,6 +175,11 @@ OpenFileByDevicePath(
> > //
> > Handle2 = Handle1;
> > Handle1 = NULL;
> > +PathLength = DevicePathNodeLength(*FilePath) -
> sizeof(EFI_DEVICE_PATH_PROTOCOL);
> > +PathName = AllocateCopyPool(PathLength, ((FILEPATH_DEVICE_PATH*)*
> FilePath)->PathName);
>
> (1) On both lines above, space characters are missing after:
> DevicePathNodeLength, sizeof, and AllocateCopyPool. (Edk2 coding style.)
> I think we can fix this up for you when we push the patch. (I'm willing
> to help with that, but we need SecurityPkg maintainer review first.)
>
>
> > +if (PathName == NULL) {
> > + return EFI_OUT_OF_RESOURCES;
> > +}
>
> (2) I have now reviewed the original state of the function more
> carefully, and, while the above "return" branch introduces a leak
> *path*, it does not introduce a leak that doesn't already exist!
>
> In fact, the original function has multiple issues:
>
> - If the OpenVolume() call fails, "FileHandle" is set to NULL. That's
> useless; the intent is obviously to set (*FileHandle) to NULL.
>
> - At the top of the "while" loop body, "Handle1" stands for an open
> EFI_FILE_PROTOCOL. If the device path type check at the top of the loop
> body returns EFI_INVALID_PARAMETER, then it (a) performs the same
> useless assignment to "FileHandle" as described above, and (b) fails to
> close "Handle1". This is why I say that the above EFI_OUT_OF_RESOURCES
> branch introduces no new leak, just a new path to the existent leak.
>
> - The OpenFileByDevicePath() function is duplicated in the following
> modules: "NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c", and
> "MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c". With the
> implication that the alignment issue you found affects all three drivers!
>
>
> Roman, I realize this could be more than what you signed up for; so
> please pick one:
>
> (2a) you could submit a patch series:
>
&
Re: [edk2] [PATCH v2] SecurityPkg: Fix assert when setting key from eMMC/SD/USB
Signed-off-by: Roman Bacik
On Tue, Jul 10, 2018 at 3:51 PM, wrote:
> From: Roman Bacik
>
> When secure boot is enabled, if one loads keys from a FAT formatted
> eMMC/SD/USB when trying to provision PK/KEK/DB keys via the menu,
> an assert in StrLen() occurs.
> This is because the filename starts on odd address, which is not a uint16
> aligned boundary: https://bugzilla.tianocore.org/show_bug.cgi?id=1003
>
> Cc: Chao Zhang
> Cc: Jiewen Yao
> Cc: Laszlo Ersek
> Cc: Vladimir Olovyannikov
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Roman Bacik
> ---
>
> SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
> | 13 +++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/
> SecureBootConfigFileExplorer.c
> index 1b6f88804275..19b13a5569a6 100644
> --- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/
> SecureBootConfigFileExplorer.c
> +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/
> SecureBootConfigFileExplorer.c
> @@ -123,6 +123,8 @@ OpenFileByDevicePath(
>EFI_FILE_PROTOCOL *Handle1;
>EFI_FILE_PROTOCOL *Handle2;
>EFI_HANDLE DeviceHandle;
> + CHAR16 *PathName;
> + UINTN PathLength;
>
>if ((FilePath == NULL || FileHandle == NULL)) {
> return EFI_INVALID_PARAMETER;
> @@ -173,6 +175,11 @@ OpenFileByDevicePath(
> //
> Handle2 = Handle1;
> Handle1 = NULL;
> +PathLength = DevicePathNodeLength(*FilePath) -
> sizeof(EFI_DEVICE_PATH_PROTOCOL);
> +PathName = AllocateCopyPool(PathLength, ((FILEPATH_DEVICE_PATH*)*
> FilePath)->PathName);
> +if (PathName == NULL) {
> + return EFI_OUT_OF_RESOURCES;
> +}
>
> //
> // Try to test opening an existing file
> @@ -180,7 +187,7 @@ OpenFileByDevicePath(
> Status = Handle2->Open (
>Handle2,
>,
> - ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
> + PathName,
>OpenMode &~EFI_FILE_MODE_CREATE,
>0
> );
> @@ -192,7 +199,7 @@ OpenFileByDevicePath(
>Status = Handle2->Open (
> Handle2,
> ,
> -((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
> +PathName,
> OpenMode,
> Attributes
> );
> @@ -202,6 +209,8 @@ OpenFileByDevicePath(
> //
> Handle2->Close (Handle2);
>
> +FreePool (PathName);
> +
> if (EFI_ERROR(Status)) {
>return (Status);
> }
> --
> 2.17.1
>
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v1] SecurityPkg: Fix assert when setting key from FAT formatted eMMC/SD/USB
Laszlo, Thank you very much for your comments. I will address them and post another patch. Regards, Roman On Mon, Jul 9, 2018 at 5:24 PM, Laszlo Ersek wrote: > On 07/10/18 02:02, Laszlo Ersek wrote: > > On 07/10/18 00:11, Roman Bacik wrote: > > >> +PathName = AllocateZeroPool (PathLength); > >> +CopyMem (PathName, ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName, > >> PathLength); > > > > (3) I think it's not necessary to zero-fill the buffer, we're going to > > overwrite it right after the allocation. > > > > There's a convenience function for that: AllocateCopyPool(). > > > > (4) The number of bytes is not correct IMO. "PathLength" stands for the > > number of bytes in the entire device path node (FILEPATH_DEVICE_PATH), > > including Header and PathName. So, for getting the number of bytes in > > just PathName, we should subtract the size of Header. > > > > Presently, we over-read the source buffer; it's not causing problems > > because PathName is NUL-terminated. > > Sorry, I was unclear; I meant there were no *observable* problems. The > over-read of the source buffer results in garbage at the end of the > target buffer, which are later not consumed due to the terminating NUL > appearing earlier. Still, we should not over-read the source buffer. > > Thanks > Laszlo > ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH v1] SecurityPkg: Fix assert when setting key from FAT formatted eMMC/SD/USB
When secure boot is enabled, if one loads keys from a FAT formatted
eMMC/SD/USB
when trying to provision PK/KEK/DB keys via the menu, an assert in StrLen()
occurs.
This is because the filename starts on odd address, which is not a uint16
aligned
boundary: https://bugzilla.tianocore.org/show_bug.cgi?id=1003
Cc: Chao Zhang
Cc: Jiewen Yao
Cc: Laszlo Ersek
Cc: Vladimir Olovyannikov
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Roman Bacik
---
.../SecureBootConfigFileExplorer.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git
a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
index 1b6f88804275..d5338406957c 100644
---
a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
+++
b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
@@ -123,6 +123,8 @@ OpenFileByDevicePath(
EFI_FILE_PROTOCOL *Handle1;
EFI_FILE_PROTOCOL *Handle2;
EFI_HANDLE DeviceHandle;
+ CHAR16 *PathName;
+ UINT16 PathLength;
if ((FilePath == NULL || FileHandle == NULL)) {
return EFI_INVALID_PARAMETER;
@@ -173,6 +175,10 @@ OpenFileByDevicePath(
//
Handle2 = Handle1;
Handle1 = NULL;
+PathLength = ((FILEPATH_DEVICE_PATH*)*FilePath)->Header.Length[0] |
+ ((FILEPATH_DEVICE_PATH*)*FilePath)->Header.Length[1] << 8;
+PathName = AllocateZeroPool (PathLength);
+CopyMem (PathName, ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
PathLength);
//
// Try to test opening an existing file
@@ -180,7 +186,7 @@ OpenFileByDevicePath(
Status = Handle2->Open (
Handle2,
,
- ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
+ PathName,
OpenMode &~EFI_FILE_MODE_CREATE,
0
);
@@ -192,7 +198,7 @@ OpenFileByDevicePath(
Status = Handle2->Open (
Handle2,
,
-((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
+PathName,
OpenMode,
Attributes
);
@@ -202,6 +208,8 @@ OpenFileByDevicePath(
//
Handle2->Close (Handle2);
+FreePool (PathName);
+
if (EFI_ERROR(Status)) {
return (Status);
}
--
2.17.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] Enable using device address when programming BARs
Hi Laszlo, Ard, After seeing the commits you have pointed out your comments are starting to be more clear. We will try to follow your suggestions and will let you know if it works for us. Thank you very much for your explanation, Roman > -Original Message- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Monday, May 14, 2018 11:05 AM > To: Roman Bacik > Cc: Ard Biesheuvel; Ruiyu Ni; edk2-devel@lists.01.org; Vladimir > Olovyannikov > Subject: Re: [edk2] [PATCH] Enable using device address when programming > BARs > > Hi Roman, > > On 05/14/18 19:35, Ard Biesheuvel wrote: > > On 14 May 2018 at 19:28, Roman Bacik <roman.ba...@broadcom.com> > wrote: > >> Ard, > >> > >> Thank you very much for your comment. > >> > >>> -Original Message- > >>> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] > >>> Sent: Sunday, May 13, 2018 3:25 AM > >>> To: Roman Bacik > >>> Cc: edk2-devel@lists.01.org; Ruiyu Ni; Vladimir Olovyannikov > >>> Subject: Re: [edk2] [PATCH] Enable using device address when > >>> programming BARs > >>> > >>> On 9 May 2018 at 22:17, Roman Bacik <roman.ba...@broadcom.com> > >>> wrote: > >>>> I will upload v2 with the corrected subject - add package name > >>>> MdeModulePkg/Bus . > >>>> > >>> > >>> I don't think this is the correct approach. Please use the address > >>> translation support that has been added recently to PciHostBridgeDxe > >>> and PciHostBridgeLib. > >>> > >> > >> Would you like to see this change: > >> > >> Address = Base + Node->Offset; > >> + if (UseDeviceAddress) > >> + Address = TO_DEVICE_ADDRESS(Address, -Base); > >> > >> Instead of: > >> > >> - Address = Base + Node->Offset; > >> + Address = UseDeviceAddress? Node->Offset: Base + Node->Offset; > >> > > > > No. > > > > Programming BARs should always involve device addresses, never CPU > > addresses. If you wire up the MMIO translation support correctly, the > > existing code will already do what you want. > > To clarify a bit, please look at the following commits: > > 1 5bb1866e5383 MdeModulePkg/PciHostBridgeLib.h: add address > Translation > 2 74d0a339b832 MdeModulePkg/PciHostBridgeDxe: Add support for > address translation > 3 c03860d05233 MdeModulePkg/PciBus: convert host address to device > address > 4 dc080d3b61e5 MdeModulePkg/PciBus: return CPU address for > GetBarAttributes > > in particular, as Ard's first response suggests, the new field introduced > in > commit 5bb1866e5383. The PciHostBridgeLib instance that your platform > plugs into MdeModulePkg/PciHostBridgeDxe should populate this field > correctly. Then PciHostBridgeDxe and PciBusDxe will do the right thing for > you. > > Thanks > Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] Enable using device address when programming BARs
> -Original Message- > From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] > Sent: Monday, May 14, 2018 10:35 AM > To: Roman Bacik > Cc: edk2-devel@lists.01.org; Ruiyu Ni; Vladimir Olovyannikov > Subject: Re: [edk2] [PATCH] Enable using device address when programming > BARs > > On 14 May 2018 at 19:28, Roman Bacik <roman.ba...@broadcom.com> wrote: > > Ard, > > > > Thank you very much for your comment. > > > >> -Original Message- > >> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] > >> Sent: Sunday, May 13, 2018 3:25 AM > >> To: Roman Bacik > >> Cc: edk2-devel@lists.01.org; Ruiyu Ni; Vladimir Olovyannikov > >> Subject: Re: [edk2] [PATCH] Enable using device address when > >> programming BARs > >> > >> On 9 May 2018 at 22:17, Roman Bacik <roman.ba...@broadcom.com> > wrote: > >> > I will upload v2 with the corrected subject - add package name > >> > MdeModulePkg/Bus . > >> > > >> > >> I don't think this is the correct approach. Please use the address > >> translation support that has been added recently to PciHostBridgeDxe > >> and PciHostBridgeLib. > >> > > > > Would you like to see this change: > > > > Address = Base + Node->Offset; > > + if (UseDeviceAddress) > > + Address = TO_DEVICE_ADDRESS(Address, -Base); > > > > Instead of: > > > > - Address = Base + Node->Offset; > > + Address = UseDeviceAddress? Node->Offset: Base + Node->Offset; > > > > No. > > Programming BARs should always involve device addresses, never CPU > addresses. If you wire up the MMIO translation support correctly, the > existing code will already do what you want. > Our code has base address 0x6000 and offset 0x200. We need Address=0x200 to be passed to PciIo->Pci.Write() and Address=0x6200 to be stored in Node->PciDev->PciBar[Node->Bar].BaseAddress. The code as is does not seem to do that unless PciIo->Pci.Write() itself changes the content of the Address from 0x200 to 0x6200 for us. So if we modify MMIO and the existing code gets the correct Address 0x200 being passed to PciIo->Pci.Write() then it means Address = 0x200 will be stored into Node->PciDev->PciBar[Node->Bar].BaseAddress, which would be wrong in our case. It appears that regardless how we change MMIO translation the existing code would not work for us as is. Or I may be missing something. > >> > > >> > > >> > *From:* Roman Bacik [mailto:roman.ba...@broadcom.com] > >> > *Sent:* Thursday, May 3, 2018 3:55 PM > >> > *To:* edk2-devel@lists.01.org > >> > *Cc:* Ruiyu Ni; Vladimir Olovyannikov > >> > *Subject:* [edk2] [PATCH] Enable using device address when > >> > programming BARs > >> > > >> > > >> > > >> > Some SoCs require to use device address when BARs are programmed: > >> > https://bugzilla.tianocore.org/show_bug.cgi?id=948 > >> > > >> > Cc: Ruiyu Ni <ruiyu...@intel.com> > >> > Cc: Vladimir Olovyannikov <vladimir.olovyanni...@broadcom.com> > >> > Contributed-under: TianoCore Contribution Agreement 1.1 > >> > Signed-off-by: Roman Bacik <roman.ba...@broadcom.com> > >> > --- > >> > MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf| 1 + > >> > MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 8 +--- > >> > MdeModulePkg/MdeModulePkg.dec | 3 +++ > >> > MdeModulePkg/MdeModulePkg.dsc | 1 + > >> > 4 files changed, 10 insertions(+), 3 deletions(-) > >> > > >> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf > >> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf > >> > index 97608bfcf245..1368e5068574 100644 > >> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf > >> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf > >> > @@ -110,6 +110,7 @@ > >> >gEfiMdeModulePkgTokenSpaceGuid.PcdAriSupport ## > >> CONSUMES > >> >gEfiMdeModulePkgTokenSpaceGuid.PcdMrIovSupport## > >> CONSUMES > >> >gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration > ## > >> > SOMETIMES_CONSUMES > >> > + gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress## > >> CONSUMES > >> > > >> > [UserExtensions.TianoCore."ExtraFiles"] > >> >PciBusDxeE
Re: [edk2] [PATCH] Enable using device address when programming BARs
Ard, Thank you very much for your comment. > -Original Message- > From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] > Sent: Sunday, May 13, 2018 3:25 AM > To: Roman Bacik > Cc: edk2-devel@lists.01.org; Ruiyu Ni; Vladimir Olovyannikov > Subject: Re: [edk2] [PATCH] Enable using device address when programming > BARs > > On 9 May 2018 at 22:17, Roman Bacik <roman.ba...@broadcom.com> wrote: > > I will upload v2 with the corrected subject - add package name > > MdeModulePkg/Bus . > > > > I don't think this is the correct approach. Please use the address > translation > support that has been added recently to PciHostBridgeDxe and > PciHostBridgeLib. > Would you like to see this change: Address = Base + Node->Offset; + if (UseDeviceAddress) + Address = TO_DEVICE_ADDRESS(Address, -Base); Instead of: - Address = Base + Node->Offset; + Address = UseDeviceAddress? Node->Offset: Base + Node->Offset; > > > > > > *From:* Roman Bacik [mailto:roman.ba...@broadcom.com] > > *Sent:* Thursday, May 3, 2018 3:55 PM > > *To:* edk2-devel@lists.01.org > > *Cc:* Ruiyu Ni; Vladimir Olovyannikov > > *Subject:* [edk2] [PATCH] Enable using device address when programming > > BARs > > > > > > > > Some SoCs require to use device address when BARs are programmed: > > https://bugzilla.tianocore.org/show_bug.cgi?id=948 > > > > Cc: Ruiyu Ni <ruiyu...@intel.com> > > Cc: Vladimir Olovyannikov <vladimir.olovyanni...@broadcom.com> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Roman Bacik <roman.ba...@broadcom.com> > > --- > > MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf| 1 + > > MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 8 +--- > > MdeModulePkg/MdeModulePkg.dec | 3 +++ > > MdeModulePkg/MdeModulePkg.dsc | 1 + > > 4 files changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf > > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf > > index 97608bfcf245..1368e5068574 100644 > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf > > @@ -110,6 +110,7 @@ > >gEfiMdeModulePkgTokenSpaceGuid.PcdAriSupport ## > CONSUMES > >gEfiMdeModulePkgTokenSpaceGuid.PcdMrIovSupport## > CONSUMES > >gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration## > > SOMETIMES_CONSUMES > > + gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress## > CONSUMES > > > > [UserExtensions.TianoCore."ExtraFiles"] > >PciBusDxeExtra.uni > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > > index 2f713fcee95e..a23bd1e258ef 100644 > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > > @@ -1269,6 +1269,7 @@ ProgramBar ( > >EFI_PCI_IO_PROTOCOL *PciIo; > >UINT64 Address; > >UINT32 Address32; > > + BOOLEAN UseDeviceAddress; > > > >ASSERT (Node->Bar < PCI_MAX_BAR); > > > > @@ -1282,8 +1283,9 @@ ProgramBar ( > > > >Address = 0; > >PciIo = &(Node->PciDev->PciIo); > > + UseDeviceAddress = FeaturePcdGet (PcdUseDeviceAddress); > > > > - Address = Base + Node->Offset; > > + Address = UseDeviceAddress? Node->Offset: Base + Node->Offset; > > > >// > >// Indicate pci bus driver has allocated @@ -1308,7 +1310,7 @@ > > ProgramBar ( > > > > ); > > > > -Node->PciDev->PciBar[Node->Bar].BaseAddress = Address; > > +Node->PciDev->PciBar[Node->Bar].BaseAddress = UseDeviceAddress? > > + Base + > > Address: Address; > > > > break; > > > > @@ -1335,7 +1337,7 @@ ProgramBar ( > > > > ); > > > > -Node->PciDev->PciBar[Node->Bar].BaseAddress = Address; > > +Node->PciDev->PciBar[Node->Bar].BaseAddress = UseDeviceAddress? > > + Base + > > Address: Address; > > > > break; > > > > diff --git a/MdeModulePkg/MdeModulePkg.dec > > b/MdeModulePkg/MdeModulePkg.dec index cc397185f7b9..58425ee0d57f > > 100644 > > --- a/MdeModulePkg/MdeModulePkg.dec > > +++ b/MdeModulePkg/MdeModulePkg.dec > > @@ -1005,6 +100
Re: [edk2] [PATCH v2] MdeModulePkg/Bus: Enable to use device address when programming BARs
Ray, Sadly our SoC requires offset instead of base+offset to be used when programming BARs. Moreover base+offset must be kept for the drivers to use the correct address. This applies to all PCI devices in the system. We would really appreciate if such requirement can be supported. Thanks, Roman On Wed, May 9, 2018 at 8:02 PM, Ni, Ruiyu <ruiyu...@intel.com> wrote: > > Roman, > > Can you point to me the spec content which states the “Offset” instead of > “Base + Offset” should be written to the BAR? > > Does the policy apply to all PCI devices in a system, or certain PCI devices > in a system? > > > > Thanks/Ray > > > > From: Roman Bacik <roman.ba...@broadcom.com> > Sent: Thursday, May 10, 2018 4:25 AM > To: edk2-devel@lists.01.org > Cc: Ni, Ruiyu <ruiyu...@intel.com>; Vladimir Olovyannikov > <vladimir.olovyanni...@broadcom.com> > Subject: [edk2] [PATCH v2] MdeModulePkg/Bus: Enable to use device address > when programming BARs > > > > Some SoCs require to use device address when BARs are programmed: > https://bugzilla.tianocore.org/show_bug.cgi?id=948 > > Cc: Ruiyu Ni <ruiyu...@intel.com> > Cc: Vladimir Olovyannikov <vladimir.olovyanni...@broadcom.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Roman Bacik <roman.ba...@broadcom.com> > --- > MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf| 1 + > MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 8 +--- > MdeModulePkg/MdeModulePkg.dec | 3 +++ > MdeModulePkg/MdeModulePkg.dsc | 1 + > 4 files changed, 10 insertions(+), 3 deletions(-) > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf > index 97608bfcf245..1368e5068574 100644 > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf > @@ -110,6 +110,7 @@ >gEfiMdeModulePkgTokenSpaceGuid.PcdAriSupport ## CONSUMES >gEfiMdeModulePkgTokenSpaceGuid.PcdMrIovSupport## CONSUMES >gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration## > SOMETIMES_CONSUMES > + gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress## CONSUMES > > [UserExtensions.TianoCore."ExtraFiles"] >PciBusDxeExtra.uni > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > index 2f713fcee95e..a23bd1e258ef 100644 > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > @@ -1269,6 +1269,7 @@ ProgramBar ( >EFI_PCI_IO_PROTOCOL *PciIo; >UINT64 Address; >UINT32 Address32; > + BOOLEAN UseDeviceAddress; > >ASSERT (Node->Bar < PCI_MAX_BAR); > > @@ -1282,8 +1283,9 @@ ProgramBar ( > >Address = 0; >PciIo = &(Node->PciDev->PciIo); > + UseDeviceAddress = FeaturePcdGet (PcdUseDeviceAddress); > > - Address = Base + Node->Offset; > + Address = UseDeviceAddress? Node->Offset: Base + Node->Offset; > >// >// Indicate pci bus driver has allocated > @@ -1308,7 +1310,7 @@ ProgramBar ( > > ); > > -Node->PciDev->PciBar[Node->Bar].BaseAddress = Address; > +Node->PciDev->PciBar[Node->Bar].BaseAddress = UseDeviceAddress? Base + > Address: Address; > > break; > > @@ -1335,7 +1337,7 @@ ProgramBar ( > > ); > > -Node->PciDev->PciBar[Node->Bar].BaseAddress = Address; > +Node->PciDev->PciBar[Node->Bar].BaseAddress = UseDeviceAddress? Base + > Address: Address; > > break; > > diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec > index cc397185f7b9..58425ee0d57f 100644 > --- a/MdeModulePkg/MdeModulePkg.dec > +++ b/MdeModulePkg/MdeModulePkg.dec > @@ -1005,6 +1005,9 @@ ># @Prompt Enable UEFI Stack Guard. >gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|FALSE|BOOLEAN|0x30001055 > > + ## Indicates whether the device address should be used for BAR programming > + gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress|FALSE|BOOLEAN|0x30001056 > + > [PcdsFixedAtBuild, PcdsPatchableInModule] >## Dynamic type PCD can be registered callback function for Pcd setting > action. ># PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum number of > callback function > diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc > index ec24a50c7d0a..39b397cb13d9 100644 > --- a/MdeModulePkg/
[edk2] [PATCH v2] MdeModulePkg/Bus: Enable to use device address when programming BARs
Some SoCs require to use device address when BARs are programmed: https://bugzilla.tianocore.org/show_bug.cgi?id=948 Cc: Ruiyu Ni <ruiyu...@intel.com> Cc: Vladimir Olovyannikov <vladimir.olovyanni...@broadcom.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Roman Bacik <roman.ba...@broadcom.com> --- MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf| 1 + MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 8 +--- MdeModulePkg/MdeModulePkg.dec | 3 +++ MdeModulePkg/MdeModulePkg.dsc | 1 + 4 files changed, 10 insertions(+), 3 deletions(-) diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf index 97608bfcf245..1368e5068574 100644 --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf @@ -110,6 +110,7 @@ gEfiMdeModulePkgTokenSpaceGuid.PcdAriSupport ## CONSUMES gEfiMdeModulePkgTokenSpaceGuid.PcdMrIovSupport## CONSUMES gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration## SOMETIMES_CONSUMES + gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress## CONSUMES [UserExtensions.TianoCore."ExtraFiles"] PciBusDxeExtra.uni diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c index 2f713fcee95e..a23bd1e258ef 100644 --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c @@ -1269,6 +1269,7 @@ ProgramBar ( EFI_PCI_IO_PROTOCOL *PciIo; UINT64 Address; UINT32 Address32; + BOOLEAN UseDeviceAddress; ASSERT (Node->Bar < PCI_MAX_BAR); @@ -1282,8 +1283,9 @@ ProgramBar ( Address = 0; PciIo = &(Node->PciDev->PciIo); + UseDeviceAddress = FeaturePcdGet (PcdUseDeviceAddress); - Address = Base + Node->Offset; + Address = UseDeviceAddress? Node->Offset: Base + Node->Offset; // // Indicate pci bus driver has allocated @@ -1308,7 +1310,7 @@ ProgramBar ( ); -Node->PciDev->PciBar[Node->Bar].BaseAddress = Address; +Node->PciDev->PciBar[Node->Bar].BaseAddress = UseDeviceAddress? Base + Address: Address; break; @@ -1335,7 +1337,7 @@ ProgramBar ( ); -Node->PciDev->PciBar[Node->Bar].BaseAddress = Address; +Node->PciDev->PciBar[Node->Bar].BaseAddress = UseDeviceAddress? Base + Address: Address; break; diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec index cc397185f7b9..58425ee0d57f 100644 --- a/MdeModulePkg/MdeModulePkg.dec +++ b/MdeModulePkg/MdeModulePkg.dec @@ -1005,6 +1005,9 @@ # @Prompt Enable UEFI Stack Guard. gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|FALSE|BOOLEAN|0x30001055 + ## Indicates whether the device address should be used for BAR programming + gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress|FALSE| BOOLEAN|0x30001056 + [PcdsFixedAtBuild, PcdsPatchableInModule] ## Dynamic type PCD can be registered callback function for Pcd setting action. # PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum number of callback function diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc index ec24a50c7d0a..39b397cb13d9 100644 --- a/MdeModulePkg/MdeModulePkg.dsc +++ b/MdeModulePkg/MdeModulePkg.dsc @@ -200,6 +200,7 @@ gEfiMdeModulePkgTokenSpaceGuid.PcdMaxSizeNonPopulateCapsule|0x0 gEfiMdeModulePkgTokenSpaceGuid.PcdMaxSizePopulateCapsule|0x0 gEfiMdeModulePkgTokenSpaceGuid.PcdMaxPeiPerformanceLogEntries|28 + gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress|FALSE [PcdsFixedAtBuild.IPF] gEfiMdePkgTokenSpaceGuid.PcdIoBlockBaseAddressForIpf|0x0c00 -- 1.9.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] Enable using device address when programming BARs
I will upload v2 with the corrected subject - add package name MdeModulePkg/Bus . *From:* Roman Bacik [mailto:roman.ba...@broadcom.com] *Sent:* Thursday, May 3, 2018 3:55 PM *To:* edk2-devel@lists.01.org *Cc:* Ruiyu Ni; Vladimir Olovyannikov *Subject:* [edk2] [PATCH] Enable using device address when programming BARs Some SoCs require to use device address when BARs are programmed: https://bugzilla.tianocore.org/show_bug.cgi?id=948 Cc: Ruiyu Ni <ruiyu...@intel.com> Cc: Vladimir Olovyannikov <vladimir.olovyanni...@broadcom.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Roman Bacik <roman.ba...@broadcom.com> --- MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf| 1 + MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 8 +--- MdeModulePkg/MdeModulePkg.dec | 3 +++ MdeModulePkg/MdeModulePkg.dsc | 1 + 4 files changed, 10 insertions(+), 3 deletions(-) diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf index 97608bfcf245..1368e5068574 100644 --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf @@ -110,6 +110,7 @@ gEfiMdeModulePkgTokenSpaceGuid.PcdAriSupport ## CONSUMES gEfiMdeModulePkgTokenSpaceGuid.PcdMrIovSupport## CONSUMES gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration## SOMETIMES_CONSUMES + gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress## CONSUMES [UserExtensions.TianoCore."ExtraFiles"] PciBusDxeExtra.uni diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c index 2f713fcee95e..a23bd1e258ef 100644 --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c @@ -1269,6 +1269,7 @@ ProgramBar ( EFI_PCI_IO_PROTOCOL *PciIo; UINT64 Address; UINT32 Address32; + BOOLEAN UseDeviceAddress; ASSERT (Node->Bar < PCI_MAX_BAR); @@ -1282,8 +1283,9 @@ ProgramBar ( Address = 0; PciIo = &(Node->PciDev->PciIo); + UseDeviceAddress = FeaturePcdGet (PcdUseDeviceAddress); - Address = Base + Node->Offset; + Address = UseDeviceAddress? Node->Offset: Base + Node->Offset; // // Indicate pci bus driver has allocated @@ -1308,7 +1310,7 @@ ProgramBar ( ); -Node->PciDev->PciBar[Node->Bar].BaseAddress = Address; +Node->PciDev->PciBar[Node->Bar].BaseAddress = UseDeviceAddress? Base + Address: Address; break; @@ -1335,7 +1337,7 @@ ProgramBar ( ); -Node->PciDev->PciBar[Node->Bar].BaseAddress = Address; +Node->PciDev->PciBar[Node->Bar].BaseAddress = UseDeviceAddress? Base + Address: Address; break; diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec index cc397185f7b9..58425ee0d57f 100644 --- a/MdeModulePkg/MdeModulePkg.dec +++ b/MdeModulePkg/MdeModulePkg.dec @@ -1005,6 +1005,9 @@ # @Prompt Enable UEFI Stack Guard. gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|FALSE|BOOLEAN|0x30001055 + ## Indicates whether the device address should be used for BAR programming + gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress|FALSE|BOOLEAN|0x30001056 + [PcdsFixedAtBuild, PcdsPatchableInModule] ## Dynamic type PCD can be registered callback function for Pcd setting action. # PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum number of callback function diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc index ec24a50c7d0a..39b397cb13d9 100644 --- a/MdeModulePkg/MdeModulePkg.dsc +++ b/MdeModulePkg/MdeModulePkg.dsc @@ -200,6 +200,7 @@ gEfiMdeModulePkgTokenSpaceGuid.PcdMaxSizeNonPopulateCapsule|0x0 gEfiMdeModulePkgTokenSpaceGuid.PcdMaxSizePopulateCapsule|0x0 gEfiMdeModulePkgTokenSpaceGuid.PcdMaxPeiPerformanceLogEntries|28 + gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress|FALSE [PcdsFixedAtBuild.IPF] gEfiMdePkgTokenSpaceGuid.PcdIoBlockBaseAddressForIpf|0x0c00 -- 1.9.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH] Enable using device address when programming BARs
Some SoCs require to use device address when BARs are programmed: https://bugzilla.tianocore.org/show_bug.cgi?id=948 Cc: Ruiyu Ni <ruiyu...@intel.com> Cc: Vladimir Olovyannikov <vladimir.olovyanni...@broadcom.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Roman Bacik <roman.ba...@broadcom.com> --- MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf| 1 + MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 8 +--- MdeModulePkg/MdeModulePkg.dec | 3 +++ MdeModulePkg/MdeModulePkg.dsc | 1 + 4 files changed, 10 insertions(+), 3 deletions(-) diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf index 97608bfcf245..1368e5068574 100644 --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf @@ -110,6 +110,7 @@ gEfiMdeModulePkgTokenSpaceGuid.PcdAriSupport ## CONSUMES gEfiMdeModulePkgTokenSpaceGuid.PcdMrIovSupport## CONSUMES gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration## SOMETIMES_CONSUMES + gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress## CONSUMES [UserExtensions.TianoCore."ExtraFiles"] PciBusDxeExtra.uni diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c index 2f713fcee95e..a23bd1e258ef 100644 --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c @@ -1269,6 +1269,7 @@ ProgramBar ( EFI_PCI_IO_PROTOCOL *PciIo; UINT64 Address; UINT32 Address32; + BOOLEAN UseDeviceAddress; ASSERT (Node->Bar < PCI_MAX_BAR); @@ -1282,8 +1283,9 @@ ProgramBar ( Address = 0; PciIo = &(Node->PciDev->PciIo); + UseDeviceAddress = FeaturePcdGet (PcdUseDeviceAddress); - Address = Base + Node->Offset; + Address = UseDeviceAddress? Node->Offset: Base + Node->Offset; // // Indicate pci bus driver has allocated @@ -1308,7 +1310,7 @@ ProgramBar ( ); -Node->PciDev->PciBar[Node->Bar].BaseAddress = Address; +Node->PciDev->PciBar[Node->Bar].BaseAddress = UseDeviceAddress? Base + Address: Address; break; @@ -1335,7 +1337,7 @@ ProgramBar ( ); -Node->PciDev->PciBar[Node->Bar].BaseAddress = Address; +Node->PciDev->PciBar[Node->Bar].BaseAddress = UseDeviceAddress? Base + Address: Address; break; diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec index cc397185f7b9..58425ee0d57f 100644 --- a/MdeModulePkg/MdeModulePkg.dec +++ b/MdeModulePkg/MdeModulePkg.dec @@ -1005,6 +1005,9 @@ # @Prompt Enable UEFI Stack Guard. gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|FALSE|BOOLEAN|0x30001055 + ## Indicates whether the device address should be used for BAR programming + gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress|FALSE|BOOLEAN|0x30001056 + [PcdsFixedAtBuild, PcdsPatchableInModule] ## Dynamic type PCD can be registered callback function for Pcd setting action. # PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum number of callback function diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc index ec24a50c7d0a..39b397cb13d9 100644 --- a/MdeModulePkg/MdeModulePkg.dsc +++ b/MdeModulePkg/MdeModulePkg.dsc @@ -200,6 +200,7 @@ gEfiMdeModulePkgTokenSpaceGuid.PcdMaxSizeNonPopulateCapsule|0x0 gEfiMdeModulePkgTokenSpaceGuid.PcdMaxSizePopulateCapsule|0x0 gEfiMdeModulePkgTokenSpaceGuid.PcdMaxPeiPerformanceLogEntries|28 + gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress|FALSE [PcdsFixedAtBuild.IPF] gEfiMdePkgTokenSpaceGuid.PcdIoBlockBaseAddressForIpf|0x0c00 -- 1.9.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2] MdeModulePkg/Bus: Enable ascending resource list
Hi Ard, Thank you very much for your suggestion, we will have a look. Regards, Roman > -Original Message- > From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] > Sent: Thursday, May 3, 2018 12:36 PM > To: Roman Bacik > Cc: Laszlo Ersek; Ruiyu Ni; edk2-devel@lists.01.org; Vladimir Olovyannikov > Subject: Re: [edk2] [PATCH v2] MdeModulePkg/Bus: Enable ascending > resource list > > On 3 May 2018 at 21:01, Roman Bacik <roman.ba...@broadcom.com> wrote: > > Laszlo, > > > > You are right, the ascending order is not necessary and the resources > > can stay in descending order. Thank you very much for your > > clarification. It is the second part of the patch, which is necessary: > > > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > > index 2f713fcee95e..6a6a7e73d343 100644 > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > > @@ -1300,6 +1300,8 @@ ProgramBar ( > >case PciBarTypeMem32: > >case PciBarTypePMem32: > > > > +Node->PciDev->PciBar[Node->Bar].BaseAddress = Address; > > +Address %= Base; > > PciIo->Pci.Write ( > > PciIo, > > EfiPciIoWidthUint32, @@ -1308,13 +1310,15 @@ > > ProgramBar ( > > > > ); > > > > -Node->PciDev->PciBar[Node->Bar].BaseAddress = Address; > > +//Node->PciDev->PciBar[Node->Bar].BaseAddress = Address; > > > > break; > > > >case PciBarTypeMem64: > >case PciBarTypePMem64: > > > > +Node->PciDev->PciBar[Node->Bar].BaseAddress = Address; > > +Address %= Base;//^M > > Address32 = (UINT32) (Address & 0x); > > > > PciIo->Pci.Write ( > > @@ -1335,7 +1339,7 @@ ProgramBar ( > > > > ); > > > > -Node->PciDev->PciBar[Node->Bar].BaseAddress = Address; > > +//Node->PciDev->PciBar[Node->Bar].BaseAddress = Address; > > > > break; > > > > In particular, the BARs must be programmed with a bus address of 0, > > not the memory address of 0x6000. I will submit another patch > enabling this. > > We only recently added support to the generic PbiHostBridgeDxe for > translation between the PCI and CPU views of the MMIO regions. You need > to update your PciHostBridgeLib implementation, and likely also you > CpuIo2Dxe driver. > > > > > I > > would like to withdraw the current patch. > > I appreciate your time and apologise for my misunderstandings, > > > > Roman > > > > > >> -Original Message- > >> From: Roman Bacik [mailto:roman.ba...@broadcom.com] > >> Sent: Thursday, May 3, 2018 9:46 AM > >> To: 'Laszlo Ersek' > >> Cc: 'edk2-devel@lists.01.org'; 'Ruiyu Ni'; Vladimir Olovyannikov > >> Subject: RE: [edk2] [PATCH v2] MdeModulePkg/Bus: Enable ascending > >> resource list > >> > >> Laszlo, > >> > >> After changing gcd allocate type to EfiGcdAllocateAnySearchBottomUp > >> we are getting a correct placement. However when using the original > >> descending order, we are getting a crash later unless we change the > >> resource order to ascending. That is why we would like to enable > >> ascending order. This is the crash we are getting without the patch: > >> > >> PciBus: Resource Map for Bridge [00|00|00] > >> Type = PMem32; Base = 0x6000; Length = 0x10; > >> Alignment > >> = > >> 0xF > >>Base = 0x6000; Length = 0x2; Alignment = 0x1; > >> Owner = > >> PCI [01|00|03:18]; Type = PMem64 > >>Base = 0x6002; Length = 0x2; Alignment = 0x1; > >> Owner = > >> PCI [01|00|02:18]; Type = PMem64 > >>Base = 0x6004; Length = 0x2; Alignment = 0x1; > >> Owner = > >> PCI [01|00|01:18]; Type = PMem64 > >>Base = 0x6006; Length = 0x2; Alignment = 0x1; > >> Owner = > >> PCI [01|00|00:18]; Type = PMem64 > >>Base = 0x6008; Length = 0x1; Alignment = 0x; > >> Owner = > >> PCI [01|00|03:10]; Type = PMem64 > >>Base = 0x6009; Length = 0x1; Alignment = 0x; > >> Owner = > >> PCI [01|00|02:10]; Type = PMem64 > >>
Re: [edk2] [PATCH v2] MdeModulePkg/Bus: Enable ascending resource list
Laszlo, You are right, the ascending order is not necessary and the resources can stay in descending order. Thank you very much for your clarification. It is the second part of the patch, which is necessary: diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c index 2f713fcee95e..6a6a7e73d343 100644 --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c @@ -1300,6 +1300,8 @@ ProgramBar ( case PciBarTypeMem32: case PciBarTypePMem32: +Node->PciDev->PciBar[Node->Bar].BaseAddress = Address; +Address %= Base; PciIo->Pci.Write ( PciIo, EfiPciIoWidthUint32, @@ -1308,13 +1310,15 @@ ProgramBar ( ); -Node->PciDev->PciBar[Node->Bar].BaseAddress = Address; +//Node->PciDev->PciBar[Node->Bar].BaseAddress = Address; break; case PciBarTypeMem64: case PciBarTypePMem64: +Node->PciDev->PciBar[Node->Bar].BaseAddress = Address; +Address %= Base;//^M Address32 = (UINT32) (Address & 0x); PciIo->Pci.Write ( @@ -1335,7 +1339,7 @@ ProgramBar ( ); -Node->PciDev->PciBar[Node->Bar].BaseAddress = Address; +//Node->PciDev->PciBar[Node->Bar].BaseAddress = Address; break; In particular, the BARs must be programmed with a bus address of 0, not the memory address of 0x6000. I will submit another patch enabling this. I would like to withdraw the current patch. I appreciate your time and apologise for my misunderstandings, Roman > -Original Message- > From: Roman Bacik [mailto:roman.ba...@broadcom.com] > Sent: Thursday, May 3, 2018 9:46 AM > To: 'Laszlo Ersek' > Cc: 'edk2-devel@lists.01.org'; 'Ruiyu Ni'; Vladimir Olovyannikov > Subject: RE: [edk2] [PATCH v2] MdeModulePkg/Bus: Enable ascending > resource list > > Laszlo, > > After changing gcd allocate type to EfiGcdAllocateAnySearchBottomUp we > are getting a correct placement. However when using the original > descending > order, we are getting a crash later unless we change the resource order to > ascending. That is why we would like to enable ascending order. This is > the > crash we are getting without the patch: > > PciBus: Resource Map for Bridge [00|00|00] > Type = PMem32; Base = 0x6000; Length = 0x10; Alignment > = > 0xF >Base = 0x6000; Length = 0x2; Alignment = 0x1; > Owner = > PCI [01|00|03:18]; Type = PMem64 >Base = 0x6002; Length = 0x2; Alignment = 0x1; > Owner = > PCI [01|00|02:18]; Type = PMem64 >Base = 0x6004; Length = 0x2; Alignment = 0x1; > Owner = > PCI [01|00|01:18]; Type = PMem64 >Base = 0x6006; Length = 0x2; Alignment = 0x1; > Owner = > PCI [01|00|00:18]; Type = PMem64 >Base = 0x6008; Length = 0x1; Alignment = 0x; > Owner = > PCI [01|00|03:10]; Type = PMem64 >Base = 0x6009; Length = 0x1; Alignment = 0x; > Owner = > PCI [01|00|02:10]; Type = PMem64 >Base = 0x600A; Length = 0x1; Alignment = 0x; > Owner = > PCI [01|00|01:10]; Type = PMem64 >Base = 0x600B; Length = 0x1; Alignment = 0x; > Owner = > PCI [01|00|00:10]; Type = PMem64 >Base = 0x600C; Length = 0x1000;Alignment = 0xFFF; > Owner = > PCI [01|00|03:20]; Type = PMem64 >Base = 0x600C1000; Length = 0x1000;Alignment = 0xFFF; > Owner = > PCI [01|00|02:20]; Type = PMem64 >Base = 0x600C2000; Length = 0x1000;Alignment = 0xFFF; > Owner = > PCI [01|00|01:20]; Type = PMem64 >Base = 0x600C3000; Length = 0x1000;Alignment = 0xFFF; > Owner = > PCI [01|00|00:20]; Type = PMem64 > > ... > > SError Exception at 0xFDDF6338 > PC 0xFDDF6338 (0xFDDF4000+0x2338) [ 0] CpuIo2Dxe.dll PC > 0xFDDF62F4 (0xFDDF4000+0x22F4) [ 0] CpuIo2Dxe.dll PC > 0xFDDF5634 (0xFDDF4000+0x1634) [ 0] CpuIo2Dxe.dll PC > 0xF87EAFAC (0xF87E7000+0x3FAC) [ 1] PciHostBridge.dll PC > 0xF81E926C (0xF81DA000+0xF26C) [ 2] PciBusDxe.dll PC > 0xF8765F8C (0xF8757000+0xEF8C) [ 3] cxundi.dll PC > 0xF875C96C (0xF8757000+0x596C) [ 3] cxundi.dll PC > 0xF875B68C (0xF8757000+0x468C) [ 3] cxundi.dll PC > 0xF8762480 (0xF8757000+0xB480) [ 3] cxundi.dll PC > 0xFE66191C (0xFE653000+0xE91C) [ 4] DxeCore.dll PC > 0xFE660CE8 (0xFE653000+0xDCE8) [ 4] DxeCore.dll PC > 0xFE661038 (0xFE653000+0xE038) [ 4] DxeCore.dll PC > 0xF8645114 (0xF863600
Re: [edk2] [PATCH v2] MdeModulePkg/Bus: Enable ascending resource list
Laszlo, After changing gcd allocate type to EfiGcdAllocateAnySearchBottomUp we are getting a correct placement. However when using the original descending order, we are getting a crash later unless we change the resource order to ascending. That is why we would like to enable ascending order. This is the crash we are getting without the patch: PciBus: Resource Map for Bridge [00|00|00] Type = PMem32; Base = 0x6000; Length = 0x10; Alignment = 0xF Base = 0x6000; Length = 0x2; Alignment = 0x1; Owner = PCI [01|00|03:18]; Type = PMem64 Base = 0x6002; Length = 0x2; Alignment = 0x1; Owner = PCI [01|00|02:18]; Type = PMem64 Base = 0x6004; Length = 0x2; Alignment = 0x1; Owner = PCI [01|00|01:18]; Type = PMem64 Base = 0x6006; Length = 0x2; Alignment = 0x1; Owner = PCI [01|00|00:18]; Type = PMem64 Base = 0x6008; Length = 0x1; Alignment = 0x; Owner = PCI [01|00|03:10]; Type = PMem64 Base = 0x6009; Length = 0x1; Alignment = 0x; Owner = PCI [01|00|02:10]; Type = PMem64 Base = 0x600A; Length = 0x1; Alignment = 0x; Owner = PCI [01|00|01:10]; Type = PMem64 Base = 0x600B; Length = 0x1; Alignment = 0x; Owner = PCI [01|00|00:10]; Type = PMem64 Base = 0x600C; Length = 0x1000;Alignment = 0xFFF; Owner = PCI [01|00|03:20]; Type = PMem64 Base = 0x600C1000; Length = 0x1000;Alignment = 0xFFF; Owner = PCI [01|00|02:20]; Type = PMem64 Base = 0x600C2000; Length = 0x1000;Alignment = 0xFFF; Owner = PCI [01|00|01:20]; Type = PMem64 Base = 0x600C3000; Length = 0x1000;Alignment = 0xFFF; Owner = PCI [01|00|00:20]; Type = PMem64 ... SError Exception at 0xFDDF6338 PC 0xFDDF6338 (0xFDDF4000+0x2338) [ 0] CpuIo2Dxe.dll PC 0xFDDF62F4 (0xFDDF4000+0x22F4) [ 0] CpuIo2Dxe.dll PC 0xFDDF5634 (0xFDDF4000+0x1634) [ 0] CpuIo2Dxe.dll PC 0xF87EAFAC (0xF87E7000+0x3FAC) [ 1] PciHostBridge.dll PC 0xF81E926C (0xF81DA000+0xF26C) [ 2] PciBusDxe.dll PC 0xF8765F8C (0xF8757000+0xEF8C) [ 3] cxundi.dll PC 0xF875C96C (0xF8757000+0x596C) [ 3] cxundi.dll PC 0xF875B68C (0xF8757000+0x468C) [ 3] cxundi.dll PC 0xF8762480 (0xF8757000+0xB480) [ 3] cxundi.dll PC 0xFE66191C (0xFE653000+0xE91C) [ 4] DxeCore.dll PC 0xFE660CE8 (0xFE653000+0xDCE8) [ 4] DxeCore.dll PC 0xFE661038 (0xFE653000+0xE038) [ 4] DxeCore.dll PC 0xF8645114 (0xF8636000+0xF114) [ 5] BdsDxe.dll PC 0xF8645184 (0xF8636000+0xF184) [ 5] BdsDxe.dll PC 0xF8653B38 (0xF8636000+0x0001DB38) [ 5] BdsDxe.dll PC 0xF8638E34 (0xF8636000+0x2E34) [ 5] BdsDxe.dll PC 0xFE655524 (0xFE653000+0x2524) [ 6] DxeCore.dll PC 0xFE6544A0 (0xFE653000+0x14A0) [ 6] DxeCore.dll PC 0xFE654024 (0xFE653000+0x1024) [ 6] DxeCore.dll PC 0x850099F4 PC 0x85009BD8 PC 0x85000DCC PC 0x85000FC4 PC 0x85000F18 PC 0x850008BC [ 0] /local/ldk/edk2/Build/NS2Pkg/NOOPT_GCC5/AARCH64/UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe/DEBUG/CpuIo2Dxe.dll [ 1] /local/ldk/edk2/Build/NS2Pkg/NOOPT_GCC5/AARCH64/BroadcomPlatformPkg/Drivers/BrcmPciHostBridgeDxe/PciHostBridgeDxe/DEBUG/PciHostBridge.dll [ 2] /local/ldk/edk2/Build/NS2Pkg/NOOPT_GCC5/AARCH64/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe/DEBUG/PciBusDxe.dll [ 3] /local/ldk/edk2/Build/NS2Pkg/NOOPT_GCC5/AARCH64/BroadcomPlatformPkg/Drivers/CxUndiDxe/cxundi_auto/DEBUG/cxundi.dll [ 4] /local/ldk/edk2/Build/NS2Pkg/NOOPT_GCC5/AARCH64/MdeModulePkg/Core/Dxe/DxeMain/DEBUG/DxeCore.dll [ 5] /local/ldk/edk2/Build/NS2Pkg/NOOPT_GCC5/AARCH64/MdeModulePkg/Universal/BdsDxe/BdsDxe/DEBUG/BdsDxe.dll [ 6] /local/ldk/edk2/Build/NS2Pkg/NOOPT_GCC5/AARCH64/MdeModulePkg/Core/Dxe/DxeMain/DEBUG/DxeCore.dll Thanks, Roman > -Original Message- > From: Roman Bacik [mailto:roman.ba...@broadcom.com] > Sent: Thursday, May 3, 2018 8:23 AM > To: 'Laszlo Ersek' > Cc: 'edk2-devel@lists.01.org'; 'Ruiyu Ni'; Vladimir Olovyannikov > Subject: RE: [edk2] [PATCH v2] MdeModulePkg/Bus: Enable ascending > resource list > > Without any patches we would get the following undesirable placement: > > PCI Bus First Scanning > PciBus: Discovered PPB @ [00|00|00] > > PciBus: Discovered PCI @ [01|00|00] > ARI: CapOffset = 0x1B8 >BAR[0]: Type = PMem64; Alignment = 0x; Length = 0x1; > Offset > = 0x10 >BAR[1]: Type = PMem64; Alignment = 0x1; Length = 0x2; > Offset > = 0x18 >BAR[2]: Type = PMem64; Alignment = 0xFFF;Length = 0x1000; > Offset = > 0x20 > > PciBus: Discovered PCI @ [01|00|01] > ARI: CapOffset = 0x1B8 >BAR[0]: Type = PMem64; Alignment = 0x; Length = 0x1; > Offset > = 0x10 >BAR[1]: Type = PMem64; Alignment = 0x1;
Re: [edk2] [PATCH v2] MdeModulePkg/Core: Fix MaxAddress in Gcd
Hi Star,
It turns out we can use EfiGcdAllocateAnySearchBottomUp instead of
EfiGcdAllocateMaxAddressSearchBottomUp after few changes to our code.
Thank you very much for clarification of our misunderstanding. We withdraw
the patch and will close the associated bug ticket.
Thanks,
Roman
> -Original Message-
> From: Roman Bacik [mailto:roman.ba...@broadcom.com]
> Sent: Thursday, May 3, 2018 7:47 AM
> To: 'Zeng, Star'; 'edk2-devel@lists.01.org'
> Cc: 'Ni, Ruiyu'; Vladimir Olovyannikov
> Subject: RE: [edk2] [PATCH v2] MdeModulePkg/Core: Fix MaxAddress in Gcd
>
> Hi Star,
>
> Thank you very much for your reply. Since the BaseAddress is the lowest
> address in our case we thought this is a bug and the bottom up search
starts
> at BaseAddress and ends at BaseAddress+length-1. What would be the best
> way to do a search starting from the BaseAddress as the lowest? Do we
need
> to add another search type?
> Thanks,
>
> Roman
>
> -Original Message-
> From: Zeng, Star [mailto:star.z...@intel.com]
> Sent: Wednesday, May 2, 2018 6:35 PM
> To: Roman Bacik; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu; Vladimir Olovyannikov; Zeng, Star
> Subject: RE: [edk2] [PATCH v2] MdeModulePkg/Core: Fix MaxAddress in Gcd
>
> The GcdAllocateType are EfiGcdAllocateMaxAddress*, the MaxAddress
> comes from input BaseAddress parameter.
>
> The original code logic is correct according to PI spec.
>
> PI Spec:
> If GcdAllocateType is EfiGcdAllocateMaxAddressSearchBottomUp, then the
> GCD memory space map is searched from the lowest address up to
> BaseAddress looking for unallocated memory ranges of Length bytes
> beginning on a boundary specified by Alignment that matches
> GcdMemoryType.
> If GcdAllocateType is EfiGcdAllocateMaxAddressSearchTopDown, then the
> GCD memory space map is searched from BaseAddress down to the lowest
> address looking for unallocated memory ranges of Length bytes beginning
on
> a boundary specified by Alignment that matches GcdMemoryType.
>
>
> Thanks,
> Star
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Roman Bacik
> Sent: Thursday, May 3, 2018 2:01 AM
> To: edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu...@intel.com>; Vladimir Olovyannikov
> <vladimir.olovyanni...@broadcom.com>
> Subject: [edk2] [PATCH v2] MdeModulePkg/Core: Fix MaxAddress in Gcd
>
> When BottomUp search is used the MaxAddress is incorrectly chosen to be
> the BaseAddress instead of the EndAddress.
>
> Cc: Ruiyu Ni <ruiyu...@intel.com>
> Cc: Vladimir Olovyannikov <vladimir.olovyanni...@broadcom.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Roman Bacik <roman.ba...@broadcom.com>
> ---
> MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c index
> e17e98230b79..9eeb2bd74599 100644
> --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> @@ -1170,8 +1170,8 @@ CoreAllocateSpace ( // // Compute the maximum
> address to use in the search algorithm //
> - if (GcdAllocateType == EfiGcdAllocateMaxAddressSearchBottomUp ||
> - GcdAllocateType == EfiGcdAllocateMaxAddressSearchTopDown ) {
> + if (GcdAllocateType == EfiGcdAllocateMaxAddressSearchTopDown ||
> + GcdAllocateType == EfiGcdAllocateAnySearchTopDown ) {
> MaxAddress = *BaseAddress;
> } else {
> MaxAddress = Entry->EndAddress;
> --
> 1.9.1
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2] MdeModulePkg/Bus: Enable ascending resource list
Without any patches we would get the following undesirable placement: PCI Bus First Scanning PciBus: Discovered PPB @ [00|00|00] PciBus: Discovered PCI @ [01|00|00] ARI: CapOffset = 0x1B8 BAR[0]: Type = PMem64; Alignment = 0x; Length = 0x1; Offset = 0x10 BAR[1]: Type = PMem64; Alignment = 0x1; Length = 0x2; Offset = 0x18 BAR[2]: Type = PMem64; Alignment = 0xFFF;Length = 0x1000; Offset = 0x20 PciBus: Discovered PCI @ [01|00|01] ARI: CapOffset = 0x1B8 BAR[0]: Type = PMem64; Alignment = 0x; Length = 0x1; Offset = 0x10 BAR[1]: Type = PMem64; Alignment = 0x1; Length = 0x2; Offset = 0x18 BAR[2]: Type = PMem64; Alignment = 0xFFF;Length = 0x1000; Offset = 0x20 PciBus: Discovered PCI @ [01|00|02] ARI: CapOffset = 0x1B8 BAR[0]: Type = PMem64; Alignment = 0x; Length = 0x1; Offset = 0x10 BAR[1]: Type = PMem64; Alignment = 0x1; Length = 0x2; Offset = 0x18 BAR[2]: Type = PMem64; Alignment = 0xFFF;Length = 0x1000; Offset = 0x20 PciBus: Discovered PCI @ [01|00|03] ARI: CapOffset = 0x1B8 BAR[0]: Type = PMem64; Alignment = 0x; Length = 0x1; Offset = 0x10 BAR[1]: Type = PMem64; Alignment = 0x1; Length = 0x2; Offset = 0x18 BAR[2]: Type = PMem64; Alignment = 0xFFF;Length = 0x1000; Offset = 0x20 PciBus: Discovered PPB @ [00|00|00] PciBus: Discovered PCI @ [01|00|00] ARI: CapOffset = 0x1B8 BAR[0]: Type = PMem64; Alignment = 0x; Length = 0x1; Offset = 0x10 BAR[1]: Type = PMem64; Alignment = 0x1; Length = 0x2; Offset = 0x18 BAR[2]: Type = PMem64; Alignment = 0xFFF;Length = 0x1000; Offset = 0x20 PciBus: Discovered PCI @ [01|00|01] ARI: CapOffset = 0x1B8 BAR[0]: Type = PMem64; Alignment = 0x; Length = 0x1; Offset = 0x10 BAR[1]: Type = PMem64; Alignment = 0x1; Length = 0x2; Offset = 0x18 BAR[2]: Type = PMem64; Alignment = 0xFFF;Length = 0x1000; Offset = 0x20 PciBus: Discovered PCI @ [01|00|02] ARI: CapOffset = 0x1B8 BAR[0]: Type = PMem64; Alignment = 0x; Length = 0x1; Offset = 0x10 BAR[1]: Type = PMem64; Alignment = 0x1; Length = 0x2; Offset = 0x18 BAR[2]: Type = PMem64; Alignment = 0xFFF;Length = 0x1000; Offset = 0x20 PciBus: Discovered PCI @ [01|00|03] ARI: CapOffset = 0x1B8 BAR[0]: Type = PMem64; Alignment = 0x; Length = 0x1; Offset = 0x10 BAR[1]: Type = PMem64; Alignment = 0x1; Length = 0x2; Offset = 0x18 BAR[2]: Type = PMem64; Alignment = 0xFFF;Length = 0x1000; Offset = 0x20 PciBus: HostBridge->SubmitResources() - Success PciBus: HostBridge->NotifyPhase(AllocateResources) - Success PciBus: Resource Map for Root Bridge Acpi(PNP0A05,0x0) Type = PMem32; Base = 0x60B0; Length = 0x10; Alignment = 0xF Base = 0x60B0; Length = 0x10; Alignment = 0xF; Owner = PPB [00|00|00:**] PciBus: Resource Map for Bridge [00|00|00] Type = PMem32; Base = 0x60B0; Length = 0x10; Alignment = 0xF Base = 0x60B0; Length = 0x2; Alignment = 0x1; Owner = PCI [01|00|03:18]; Type = PMem64 Base = 0x60B2; Length = 0x2; Alignment = 0x1; Owner = PCI [01|00|02:18]; Type = PMem64 Base = 0x60B4; Length = 0x2; Alignment = 0x1; Owner = PCI [01|00|01:18]; Type = PMem64 Base = 0x60B6; Length = 0x2; Alignment = 0x1; Owner = PCI [01|00|00:18]; Type = PMem64 Base = 0x60B8; Length = 0x1; Alignment = 0x; Owner = PCI [01|00|03:10]; Type = PMem64 Base = 0x60B9; Length = 0x1; Alignment = 0x; Owner = PCI [01|00|02:10]; Type = PMem64 Base = 0x60BA; Length = 0x1; Alignment = 0x; Owner = PCI [01|00|01:10]; Type = PMem64 Base = 0x60BB; Length = 0x1; Alignment = 0x; Owner = PCI [01|00|00:10]; Type = PMem64 Base = 0x60BC; Length = 0x1000;Alignment = 0xFFF; Owner = PCI [01|00|03:20]; Type = PMem64 Base = 0x60BC1000; Length = 0x1000;Alignment = 0xFFF; Owner = PCI [01|00|02:20]; Type = PMem64 Base = 0x60BC2000; Length = 0x1000;Alignment = 0xFFF; Owner = PCI [01|00|01:20]; Type = PMem64 Base = 0x60BC3000; Length = 0x1000;Alignment = 0xFFF; Owner = PCI [01|00|00:20]; Type = PMem64 Thanks, Roman > -Original Message- > From: Roman Bacik [mailto:roman.ba...@broadcom.com] > Sent: Thursday, May 3, 2018 7:58 AM > To: 'Laszlo Ersek' > Cc: 'edk2-devel@lists.01.org'; 'Ruiyu Ni'; Vladimir Olovyannikov > Subject: RE: [edk2] [PATCH v2] MdeModulePkg/Bus: Enable ascending > resource list > > Laszlo, > > Thank you very much for your explanation. > > > -Original Message- > > From: Laszlo Ersek [mailto:ler...@redhat.com] > > Sent: Thursday, May 3, 2018 6:52 AM > > To: Roman Bacik > > Cc: edk2-devel@lists.01.org; Ruiyu Ni; Vladimir Olovyannik
Re: [edk2] [PATCH v2] MdeModulePkg/Bus: Enable ascending resource list
Laszlo, Thank you very much for your explanation. > -Original Message- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Thursday, May 3, 2018 6:52 AM > To: Roman Bacik > Cc: edk2-devel@lists.01.org; Ruiyu Ni; Vladimir Olovyannikov > Subject: Re: [edk2] [PATCH v2] MdeModulePkg/Bus: Enable ascending > resource list > > On 05/02/18 20:07, Roman Bacik wrote: > > Some processors require resource list sorted in ascending order. > > > > Cc: Ruiyu Ni <ruiyu...@intel.com> > > Cc: Vladimir Olovyannikov <vladimir.olovyanni...@broadcom.com> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Roman Bacik <roman.ba...@broadcom.com> > > --- > > MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf | 1 + > > .../Bus/Pci/PciBusDxe/PciResourceSupport.c | 43 > ++ > > MdeModulePkg/MdeModulePkg.dec | 3 ++ > > MdeModulePkg/MdeModulePkg.dsc | 1 + > > 4 files changed, 41 insertions(+), 7 deletions(-) > > I don't understand the goal of this patch. > > (Please don't take my comments as "review" or arguments against this > patch, > I'd just like to understand the issue.) > > To my understanding, InsertResourceNode() keeps PCI resources sorted in > descending *alignment* order -- because a larger power-of-two alignment is > also suitable for a smaller power-of-two alignment. > > Say, we have a bridge with two devices behind it. The first device has one > MMIO BAR of size 32MB (requiring the same 32MB natural alignment), while > the other device has one MMIO BAR, of size 4MB (requiring the same 4MB > natural alignment). > > Ordering the resources in descending *alignment* order means that we'll > 1st > allocate the 32MB BAR, at a suitably aligned address. The important thing > is > that the end of that allocation will *also* be aligned at 32MB, hence we > can > allocate, as 2nd step, the 4MB BAR right there. No gaps needed. > > If the 4MB BAR was allocated 1st (naturally aligned), then its end address > might not be naturally aligned for becoming the base address of the > remaining 32MB BAR. Thus we might have to waste some MMIO aperture to > align the large BAR correctly. > > Here's an example output from PciBusDxe running as part of OVMF: > > > PciBus: Resource Map for Root Bridge PciRoot(0x0) [...] > > Type = Mem32; Base = 0x8000; Length = 0x810; > > Alignment = > 0x3FF > >Base = 0x8000; Length = 0x400; Alignment = 0x3FF; > Owner = PCI [00|02|00:14] > >Base = 0x8400; Length = 0x400; Alignment = 0x3FF; > Owner = PCI [00|02|00:10] > >Base = 0x8800; Length = 0x2000;Alignment = 0x1FFF; > > Owner = > PCI [00|02|00:18] > >Base = 0x88002000; Length = 0x1000;Alignment = 0xFFF; > > Owner = > PCI [00|07|00:14] > >Base = 0x88003000; Length = 0x1000;Alignment = 0xFFF; > > Owner = > PCI [00|06|07:10] > >Base = 0x88004000; Length = 0x1000;Alignment = 0xFFF; > > Owner = > PCI [00|05|00:14] > >Base = 0x88005000; Length = 0x1000;Alignment = 0xFFF; > > Owner = > PCI [00|03|00:14] > > [...] > > The base addresses are already sorted in ascending order; what's > descending > is the alignment -- and that seems correct, because it conserves MMIO > aperture (no gaps needed). > > Can you please explain what's wrong with this approach for your platform, > and what the desired behavior is? > > Can you post a log snippet that shows a placement that's right for your > platform? We are also patching allocation order bottom up search starting at BaseAddress. This placement is desirable and working for us after applying the submitted two patches: PciBus: Resource Map for Bridge [00|00|00] Type = PMem32; Base = 0x6000; Length = 0x10; Alignment = 0xF Base = 0x6000; Length = 0x2; Alignment = 0x1; Owner = PCI [01|00|00:18]; Type = PMem64 Base = 0x6002; Length = 0x2; Alignment = 0x1; Owner = PCI [01|00|01:18]; Type = PMem64 Base = 0x6004; Length = 0x2; Alignment = 0x1; Owner = PCI [01|00|02:18]; Type = PMem64 Base = 0x6006; Length = 0x2; Alignment = 0x1; Owner = PCI [01|00|03:18]; Type = PMem64 Base = 0x6008; Length = 0x1; Alignment = 0x; Owner = PCI [01|00|00:10]; Type = PMem64 Base = 0x6009; Length = 0x1; Alignment = 0x; Owner = PCI [01|00|01:10]; Type = PMem64 Base = 0x600A; Length = 0x1; Alignment = 0x; Owner = PCI [01|00|02:10]; Type = PMem64 Base = 0x60
Re: [edk2] [PATCH v2] MdeModulePkg/Core: Fix MaxAddress in Gcd
Hi Star,
Thank you very much for your reply. Since the BaseAddress is the lowest
address in our case we thought this is a bug and the bottom up search
starts at BaseAddress and ends at BaseAddress+length-1. What would be the
best way to do a search starting from the BaseAddress as the lowest? Do we
need to add another search type?
Thanks,
Roman
-Original Message-
From: Zeng, Star [mailto:star.z...@intel.com]
Sent: Wednesday, May 2, 2018 6:35 PM
To: Roman Bacik; edk2-devel@lists.01.org
Cc: Ni, Ruiyu; Vladimir Olovyannikov; Zeng, Star
Subject: RE: [edk2] [PATCH v2] MdeModulePkg/Core: Fix MaxAddress in Gcd
The GcdAllocateType are EfiGcdAllocateMaxAddress*, the MaxAddress comes
from input BaseAddress parameter.
The original code logic is correct according to PI spec.
PI Spec:
If GcdAllocateType is EfiGcdAllocateMaxAddressSearchBottomUp, then the GCD
memory space map is searched from the lowest address up to BaseAddress
looking for unallocated memory ranges of Length bytes beginning on a
boundary specified by Alignment that matches GcdMemoryType.
If GcdAllocateType is EfiGcdAllocateMaxAddressSearchTopDown, then the GCD
memory space map is searched from BaseAddress down to the lowest address
looking for unallocated memory ranges of Length bytes beginning on a
boundary specified by Alignment that matches GcdMemoryType.
Thanks,
Star
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
Roman Bacik
Sent: Thursday, May 3, 2018 2:01 AM
To: edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu...@intel.com>; Vladimir Olovyannikov
<vladimir.olovyanni...@broadcom.com>
Subject: [edk2] [PATCH v2] MdeModulePkg/Core: Fix MaxAddress in Gcd
When BottomUp search is used the MaxAddress is incorrectly chosen to be
the BaseAddress instead of the EndAddress.
Cc: Ruiyu Ni <ruiyu...@intel.com>
Cc: Vladimir Olovyannikov <vladimir.olovyanni...@broadcom.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Roman Bacik <roman.ba...@broadcom.com>
---
MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c index e17e98230b79..9eeb2bd74599 100644
--- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
+++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
@@ -1170,8 +1170,8 @@ CoreAllocateSpace ( // // Compute the maximum
address to use in the search algorithm //
- if (GcdAllocateType == EfiGcdAllocateMaxAddressSearchBottomUp ||
- GcdAllocateType == EfiGcdAllocateMaxAddressSearchTopDown ) {
+ if (GcdAllocateType == EfiGcdAllocateMaxAddressSearchTopDown ||
+ GcdAllocateType == EfiGcdAllocateAnySearchTopDown ) {
MaxAddress = *BaseAddress;
} else {
MaxAddress = Entry->EndAddress;
--
1.9.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH v2] MdeModulePkg/Bus: Enable ascending resource list
Some processors require resource list sorted in ascending order.
Cc: Ruiyu Ni <ruiyu...@intel.com>
Cc: Vladimir Olovyannikov <vladimir.olovyanni...@broadcom.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Roman Bacik <roman.ba...@broadcom.com>
---
MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf | 1 +
.../Bus/Pci/PciBusDxe/PciResourceSupport.c | 43 ++
MdeModulePkg/MdeModulePkg.dec | 3 ++
MdeModulePkg/MdeModulePkg.dsc | 1 +
4 files changed, 41 insertions(+), 7 deletions(-)
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
index 97608bf..5cb3761 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
@@ -110,6 +110,7 @@
gEfiMdeModulePkgTokenSpaceGuid.PcdAriSupport ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdMrIovSupport## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration##
SOMETIMES_CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdListAscending ## CONSUMES
[UserExtensions.TianoCore."ExtraFiles"]
PciBusDxeExtra.uni
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
index 2f713fc..45575fa 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
@@ -106,19 +106,30 @@ InsertResourceNode (
PCI_RESOURCE_NODE *Temp;
UINT64ResNodeAlignRest;
UINT64TempAlignRest;
+ BOOLEAN Ascending;
ASSERT (Bridge != NULL);
ASSERT (ResNode != NULL);
- InsertHeadList (>ChildList, >Link);
+ Ascending = FeaturePcdGet (PcdListAscending);
+
+ if (!Ascending) {
+InsertHeadList (>ChildList, >Link);
+CurrentLink = Bridge->ChildList.ForwardLink->ForwardLink;
+ } else {
+CurrentLink = Bridge->ChildList.BackLink;
+InsertTailList (>ChildList, >Link);
+ }
- CurrentLink = Bridge->ChildList.ForwardLink->ForwardLink;
while (CurrentLink != >ChildList) {
Temp = RESOURCE_NODE_FROM_LINK (CurrentLink);
-if (ResNode->Alignment > Temp->Alignment) {
+if ((Ascending && Temp->Alignment >= ResNode->Alignment) ||
+ (!Ascending && ResNode->Alignment > Temp->Alignment)) {
break;
-} else if (ResNode->Alignment == Temp->Alignment) {
+}
+
+if (!Ascending && ResNode->Alignment == Temp->Alignment) {
ResNodeAlignRest = ResNode->Length & ResNode->Alignment;
TempAlignRest = Temp->Length & Temp->Alignment;
if ((ResNodeAlignRest == 0) || (ResNodeAlignRest >= TempAlignRest)) {
@@ -128,7 +139,11 @@ InsertResourceNode (
SwapListEntries (>Link, CurrentLink);
-CurrentLink = ResNode->Link.ForwardLink;
+if (Ascending) {
+ CurrentLink = ResNode->Link.BackLink;
+} else {
+ CurrentLink = ResNode->Link.ForwardLink;
+}
}
}
@@ -1269,6 +1284,7 @@ ProgramBar (
EFI_PCI_IO_PROTOCOL *PciIo;
UINT64 Address;
UINT32 Address32;
+ BOOLEAN Ascending;
ASSERT (Node->Bar < PCI_MAX_BAR);
@@ -1282,6 +1298,7 @@ ProgramBar (
Address = 0;
PciIo = &(Node->PciDev->PciIo);
+ Ascending = FeaturePcdGet (PcdListAscending);
Address = Base + Node->Offset;
@@ -1300,6 +1317,10 @@ ProgramBar (
case PciBarTypeMem32:
case PciBarTypePMem32:
+if (Ascending) {
+ Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
+ Address %= Base;
+}
PciIo->Pci.Write (
PciIo,
EfiPciIoWidthUint32,
@@ -1308,13 +1329,19 @@ ProgramBar (
);
-Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
+if (!Ascending) {
+ Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
+}
break;
case PciBarTypeMem64:
case PciBarTypePMem64:
+if (Ascending) {
+ Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
+ Address %= Base;
+}
Address32 = (UINT32) (Address & 0x);
PciIo->Pci.Write (
@@ -1335,7 +1362,9 @@ ProgramBar (
);
-Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
+if (!Ascending) {
+ Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
+}
break;
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index cc39718..911f33a 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -1005,6 +1005,9 @@
# @Prompt Enable UEFI Stack Guard.
gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|FALSE|BOOLEAN|0x30001055
+ ## Indicates if the
[edk2] [PATCH v2] MdeModulePkg/Core: Fix MaxAddress in Gcd
When BottomUp search is used the MaxAddress is incorrectly chosen to
be the BaseAddress instead of the EndAddress.
Cc: Ruiyu Ni <ruiyu...@intel.com>
Cc: Vladimir Olovyannikov <vladimir.olovyanni...@broadcom.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Roman Bacik <roman.ba...@broadcom.com>
---
MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
index e17e98230b79..9eeb2bd74599 100644
--- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
+++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
@@ -1170,8 +1170,8 @@ CoreAllocateSpace (
//
// Compute the maximum address to use in the search algorithm
//
- if (GcdAllocateType == EfiGcdAllocateMaxAddressSearchBottomUp ||
- GcdAllocateType == EfiGcdAllocateMaxAddressSearchTopDown ) {
+ if (GcdAllocateType == EfiGcdAllocateMaxAddressSearchTopDown ||
+ GcdAllocateType == EfiGcdAllocateAnySearchTopDown ) {
MaxAddress = *BaseAddress;
} else {
MaxAddress = Entry->EndAddress;
--
1.9.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v1] MdeModulePkg/Bus: Enable ascending resource list
On Sat, Apr 28, 2018 at 11:53 AM, Roman Bacik <roman.ba...@broadcom.com>
wrote:
> Enable resource list sorted in ascending order required by some SoCs.
>
> Cc: Vladimir Olovyannikov <vladimir.olovyanni...@broadcom.com>
> Cc: Ruiyu Ni <ruiyu...@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Roman Bacik <roman.ba...@broadcom.com>
> ---
> MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf | 1 +
> .../Bus/Pci/PciBusDxe/PciResourceSupport.c | 43
> ++
> MdeModulePkg/MdeModulePkg.dec | 3 ++
> MdeModulePkg/MdeModulePkg.dsc | 1 +
> 4 files changed, 41 insertions(+), 7 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> index 97608bf..5cb3761 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> @@ -110,6 +110,7 @@
>gEfiMdeModulePkgTokenSpaceGuid.PcdAriSupport ##
> CONSUMES
>gEfiMdeModulePkgTokenSpaceGuid.PcdMrIovSupport##
> CONSUMES
>gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration##
> SOMETIMES_CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdListAscending ##
> CONSUMES
>
> [UserExtensions.TianoCore."ExtraFiles"]
>PciBusDxeExtra.uni
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> index 2f713fc..45575fa 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> @@ -106,19 +106,30 @@ InsertResourceNode (
>PCI_RESOURCE_NODE *Temp;
>UINT64ResNodeAlignRest;
>UINT64TempAlignRest;
> + BOOLEAN Ascending;
>
>ASSERT (Bridge != NULL);
>ASSERT (ResNode != NULL);
>
> - InsertHeadList (>ChildList, >Link);
> + Ascending = FeaturePcdGet (PcdListAscending);
> +
> + if (Ascending) {
>
The line above should be:
if (!Ascending) {
+InsertHeadList (>ChildList, >Link);
> +CurrentLink = Bridge->ChildList.ForwardLink->ForwardLink;
> + } else {
> +CurrentLink = Bridge->ChildList.BackLink;
> +InsertTailList (>ChildList, >Link);
> + }
>
> - CurrentLink = Bridge->ChildList.ForwardLink->ForwardLink;
>while (CurrentLink != >ChildList) {
> Temp = RESOURCE_NODE_FROM_LINK (CurrentLink);
>
> -if (ResNode->Alignment > Temp->Alignment) {
> +if ((Ascending && Temp->Alignment >= ResNode->Alignment) ||
> + (!Ascending && ResNode->Alignment > Temp->Alignment)) {
>break;
> -} else if (ResNode->Alignment == Temp->Alignment) {
> +}
> +
> +if (!Ascending && ResNode->Alignment == Temp->Alignment) {
>ResNodeAlignRest = ResNode->Length & ResNode->Alignment;
>TempAlignRest = Temp->Length & Temp->Alignment;
>if ((ResNodeAlignRest == 0) || (ResNodeAlignRest >= TempAlignRest))
> {
> @@ -128,7 +139,11 @@ InsertResourceNode (
>
> SwapListEntries (>Link, CurrentLink);
>
> -CurrentLink = ResNode->Link.ForwardLink;
> +if (Ascending) {
> + CurrentLink = ResNode->Link.BackLink;
> +} else {
> + CurrentLink = ResNode->Link.ForwardLink;
> +}
>}
> }
>
> @@ -1269,6 +1284,7 @@ ProgramBar (
>EFI_PCI_IO_PROTOCOL *PciIo;
>UINT64 Address;
>UINT32 Address32;
> + BOOLEAN Ascending;
>
>ASSERT (Node->Bar < PCI_MAX_BAR);
>
> @@ -1282,6 +1298,7 @@ ProgramBar (
>
>Address = 0;
>PciIo = &(Node->PciDev->PciIo);
> + Ascending = FeaturePcdGet (PcdListAscending);
>
>Address = Base + Node->Offset;
>
> @@ -1300,6 +1317,10 @@ ProgramBar (
>case PciBarTypeMem32:
>case PciBarTypePMem32:
>
> +if (Ascending) {
> + Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
> + Address %= Base;
> +}
> PciIo->Pci.Write (
> PciIo,
> EfiPciIoWidthUint32,
> @@ -1308,13 +1329,19 @@ ProgramBar (
>
> );
>
> -Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
> +if (!Ascending) {
> + Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
> +}
>
> break;
>
>case PciBarTypeMem64:
>case PciBarTypePMem64:
>
> +if (Ascending) {
> + Node->PciDev->Pc
Re: [edk2] [PATCH] [PATCH v1] MdeModulePkg/Core: Fix MaxAddress in Gcd
Will change subject to: "[edk2] [PATCH v2] MdeModulePkg/Core: Fix
MaxAddress in Gcd" for the next version if required.
On Fri, Apr 27, 2018 at 3:36 PM, Roman Bacik <roman.ba...@broadcom.com>
wrote:
> When BottomUp search is used the MaxAddress is incorrectly chosen to
> be BaseAddress instead of EndAddress.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Roman Bacik <roman.ba...@broadcom.com>
> ---
> MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> index e17e98230b79..9eeb2bd74599 100644
> --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> @@ -1170,8 +1170,8 @@ CoreAllocateSpace (
> //
> // Compute the maximum address to use in the search algorithm
> //
> -if (GcdAllocateType == EfiGcdAllocateMaxAddressSearchBottomUp ||
> -GcdAllocateType == EfiGcdAllocateMaxAddressSearchTopDown ) {
> +if (GcdAllocateType == EfiGcdAllocateMaxAddressSearchTopDown ||
> +GcdAllocateType == EfiGcdAllocateAnySearchTopDown ) {
>MaxAddress = *BaseAddress;
> } else {
>MaxAddress = Entry->EndAddress;
> --
> 1.9.1
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH v1] MdeModulePkg/Bus: Enable ascending resource list
Enable resource list sorted in ascending order required by some SoCs.
Cc: Vladimir Olovyannikov <vladimir.olovyanni...@broadcom.com>
Cc: Ruiyu Ni <ruiyu...@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Roman Bacik <roman.ba...@broadcom.com>
---
MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf | 1 +
.../Bus/Pci/PciBusDxe/PciResourceSupport.c | 43
++
MdeModulePkg/MdeModulePkg.dec | 3 ++
MdeModulePkg/MdeModulePkg.dsc | 1 +
4 files changed, 41 insertions(+), 7 deletions(-)
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
index 97608bf..5cb3761 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
@@ -110,6 +110,7 @@
gEfiMdeModulePkgTokenSpaceGuid.PcdAriSupport ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdMrIovSupport## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration##
SOMETIMES_CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdListAscending ## CONSUMES
[UserExtensions.TianoCore."ExtraFiles"]
PciBusDxeExtra.uni
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
index 2f713fc..45575fa 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
@@ -106,19 +106,30 @@ InsertResourceNode (
PCI_RESOURCE_NODE *Temp;
UINT64ResNodeAlignRest;
UINT64TempAlignRest;
+ BOOLEAN Ascending;
ASSERT (Bridge != NULL);
ASSERT (ResNode != NULL);
- InsertHeadList (>ChildList, >Link);
+ Ascending = FeaturePcdGet (PcdListAscending);
+
+ if (Ascending) {
+InsertHeadList (>ChildList, >Link);
+CurrentLink = Bridge->ChildList.ForwardLink->ForwardLink;
+ } else {
+CurrentLink = Bridge->ChildList.BackLink;
+InsertTailList (>ChildList, >Link);
+ }
- CurrentLink = Bridge->ChildList.ForwardLink->ForwardLink;
while (CurrentLink != >ChildList) {
Temp = RESOURCE_NODE_FROM_LINK (CurrentLink);
-if (ResNode->Alignment > Temp->Alignment) {
+if ((Ascending && Temp->Alignment >= ResNode->Alignment) ||
+ (!Ascending && ResNode->Alignment > Temp->Alignment)) {
break;
-} else if (ResNode->Alignment == Temp->Alignment) {
+}
+
+if (!Ascending && ResNode->Alignment == Temp->Alignment) {
ResNodeAlignRest = ResNode->Length & ResNode->Alignment;
TempAlignRest = Temp->Length & Temp->Alignment;
if ((ResNodeAlignRest == 0) || (ResNodeAlignRest >= TempAlignRest)) {
@@ -128,7 +139,11 @@ InsertResourceNode (
SwapListEntries (>Link, CurrentLink);
-CurrentLink = ResNode->Link.ForwardLink;
+if (Ascending) {
+ CurrentLink = ResNode->Link.BackLink;
+} else {
+ CurrentLink = ResNode->Link.ForwardLink;
+}
}
}
@@ -1269,6 +1284,7 @@ ProgramBar (
EFI_PCI_IO_PROTOCOL *PciIo;
UINT64 Address;
UINT32 Address32;
+ BOOLEAN Ascending;
ASSERT (Node->Bar < PCI_MAX_BAR);
@@ -1282,6 +1298,7 @@ ProgramBar (
Address = 0;
PciIo = &(Node->PciDev->PciIo);
+ Ascending = FeaturePcdGet (PcdListAscending);
Address = Base + Node->Offset;
@@ -1300,6 +1317,10 @@ ProgramBar (
case PciBarTypeMem32:
case PciBarTypePMem32:
+if (Ascending) {
+ Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
+ Address %= Base;
+}
PciIo->Pci.Write (
PciIo,
EfiPciIoWidthUint32,
@@ -1308,13 +1329,19 @@ ProgramBar (
);
-Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
+if (!Ascending) {
+ Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
+}
break;
case PciBarTypeMem64:
case PciBarTypePMem64:
+if (Ascending) {
+ Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
+ Address %= Base;
+}
Address32 = (UINT32) (Address & 0x);
PciIo->Pci.Write (
@@ -1335,7 +1362,9 @@ ProgramBar (
);
-Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
+if (!Ascending) {
+ Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
+}
break;
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index cc39718..911f33a 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -1005,6 +1005,9 @@
# @Prompt Enable UEFI Stack Guard.
gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|FALSE|BOOLEAN|0x30001055
+ ## Indicates if the
[edk2] [PATCH] [PATCH v1] MdeModulePkg/Core: Fix MaxAddress in Gcd
When BottomUp search is used the MaxAddress is incorrectly chosen to
be BaseAddress instead of EndAddress.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Roman Bacik <roman.ba...@broadcom.com>
---
MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
index e17e98230b79..9eeb2bd74599 100644
--- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
+++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
@@ -1170,8 +1170,8 @@ CoreAllocateSpace (
//
// Compute the maximum address to use in the search algorithm
//
-if (GcdAllocateType == EfiGcdAllocateMaxAddressSearchBottomUp ||
-GcdAllocateType == EfiGcdAllocateMaxAddressSearchTopDown ) {
+if (GcdAllocateType == EfiGcdAllocateMaxAddressSearchTopDown ||
+GcdAllocateType == EfiGcdAllocateAnySearchTopDown ) {
MaxAddress = *BaseAddress;
} else {
MaxAddress = Entry->EndAddress;
--
1.9.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
