Message ID | 1459267586-2980-1-git-send-email-leo.duran@amd.com |
---|---|
State | New |
Headers | show |
On 29 March 2016 at 18:06, Leo Duran <leo.duran@amd.com> wrote: > Signed-off-by: Leo Duran <leo.duran@amd.com> Hi Leo, This particular instance was also discussed in the context of the patch that introduces the AcpiExposedTableVersions PCD. The problem with this particular allocation is that it is not located below 4 GB because it needs to be addressable by a legacy ACPI table, but because it needs to be accessible from PEI, which is typically 32-bit on a X64 system. Fortunately, we don't have that table on arm64, so this particular instance does not matter. In general, however, there are numerous instances in the EDK2 code base where an allocation is hardcoded to be limited to 4 GB so that PEI is guaranteed to be able to access it. As it turns out, there are even platforms that do run PEI in 64-bit mode, but only map the lowest 4 GB of memory simply because all DXE code that performs allocations that need to be accessible from PEI is already capped to 4 GB. This is something I brought up with the UEFI forum, and is going to be addressed in the PI spec (hopefully). This should allow us to describe in PEI the range it can access, so that DXE can take this information into account, rather than guess that below 4 GB is okay. As far as this patch is concerned, I think we should just drop it, and perhaps revisit it once we know how to proceed with these PEI accessible allocations. Unless you do expose a FACS table in your firmware? Regards, Ard. > --- > MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c > index 7f95b9d..e1fd928 100644 > --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c > +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c > @@ -518,7 +518,7 @@ AddTableToList ( > // > ASSERT ((EFI_PAGE_SIZE % 64) == 0); > Status = gBS->AllocatePages ( > - AllocateMaxAddress, > + mAcpiTableAllocType, > EfiACPIMemoryNVS, > CurrentTableList->NumberOfPages, > &CurrentTableList->PageAddress > -- > 1.9.1 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Please see reply inline below. Thanks, Leo. > -----Original Message----- > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] > Sent: Tuesday, March 29, 2016 11:14 AM > To: Duran, Leo; Charles Garcia-Tobin; Achin Gupta > Cc: edk2-devel-01; Leif Lindholm > Subject: Re: [PATCH] MdeModulePkg: consistent AllocatePages() support for > tables above 4 GB's > > On 29 March 2016 at 18:06, Leo Duran <leo.duran@amd.com> wrote: > > Signed-off-by: Leo Duran <leo.duran@amd.com> > > Hi Leo, > > This particular instance was also discussed in the context of the patch that > introduces the AcpiExposedTableVersions PCD. > > The problem with this particular allocation is that it is not located below 4 GB > because it needs to be addressable by a legacy ACPI table, but because it > needs to be accessible from PEI, which is typically 32-bit on a X64 system. > Fortunately, we don't have that table on arm64, so this particular instance > does not matter. > > In general, however, there are numerous instances in the EDK2 code base > where an allocation is hardcoded to be limited to 4 GB so that PEI is > guaranteed to be able to access it. As it turns out, there are even platforms > that do run PEI in 64-bit mode, but only map the lowest > 4 GB of memory simply because all DXE code that performs allocations that > need to be accessible from PEI is already capped to 4 GB. > > This is something I brought up with the UEFI forum, and is going to be > addressed in the PI spec (hopefully). This should allow us to describe in PEI > the range it can access, so that DXE can take this information into account, > rather than guess that below 4 GB is okay. > > As far as this patch is concerned, I think we should just drop it, and perhaps > revisit it once we know how to proceed with these PEI accessible allocations. > Unless you do expose a FACS table in your firmware? Yes, I am declaring an FACS table. > > Regards, > Ard. > > > > > --- > > MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git > > a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c > > b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c > > index 7f95b9d..e1fd928 100644 > > --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c > > +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c > > @@ -518,7 +518,7 @@ AddTableToList ( > > // > > ASSERT ((EFI_PAGE_SIZE % 64) == 0); > > Status = gBS->AllocatePages ( > > - AllocateMaxAddress, > > + mAcpiTableAllocType, > > EfiACPIMemoryNVS, > > CurrentTableList->NumberOfPages, > > &CurrentTableList->PageAddress > > -- > > 1.9.1 > > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
(+ Al, Graeme) On 29 March 2016 at 19:06, Duran, Leo <leo.duran@amd.com> wrote: >> -----Original Message----- >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] >> Sent: Tuesday, March 29, 2016 11:14 AM >> To: Duran, Leo; Charles Garcia-Tobin; Achin Gupta >> Cc: edk2-devel-01; Leif Lindholm >> Subject: Re: [PATCH] MdeModulePkg: consistent AllocatePages() support for >> tables above 4 GB's >> >> On 29 March 2016 at 18:06, Leo Duran <leo.duran@amd.com> wrote: >> > Signed-off-by: Leo Duran <leo.duran@amd.com> >> >> Hi Leo, >> >> This particular instance was also discussed in the context of the patch that >> introduces the AcpiExposedTableVersions PCD. >> >> The problem with this particular allocation is that it is not located below 4 GB >> because it needs to be addressable by a legacy ACPI table, but because it >> needs to be accessible from PEI, which is typically 32-bit on a X64 system. >> Fortunately, we don't have that table on arm64, so this particular instance >> does not matter. >> >> In general, however, there are numerous instances in the EDK2 code base >> where an allocation is hardcoded to be limited to 4 GB so that PEI is >> guaranteed to be able to access it. As it turns out, there are even platforms >> that do run PEI in 64-bit mode, but only map the lowest >> 4 GB of memory simply because all DXE code that performs allocations that >> need to be accessible from PEI is already capped to 4 GB. >> >> This is something I brought up with the UEFI forum, and is going to be >> addressed in the PI spec (hopefully). This should allow us to describe in PEI >> the range it can access, so that DXE can take this information into account, >> rather than guess that below 4 GB is okay. >> >> As far as this patch is concerned, I think we should just drop it, and perhaps >> revisit it once we know how to proceed with these PEI accessible allocations. >> Unless you do expose a FACS table in your firmware? > > Yes, I am declaring an FACS table. > It was my understanding that the FACS is of questionable use for arm64, but I could be wrong. Al, Graeme: any thoughts? Thanks -- Ard. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
> -----Original Message----- > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] > Sent: Tuesday, March 29, 2016 12:10 PM > To: Duran, Leo > Cc: Charles Garcia-Tobin; Achin Gupta; edk2-devel-01; Leif Lindholm > Subject: Re: [PATCH] MdeModulePkg: consistent AllocatePages() support for > tables above 4 GB's > > (+ Al, Graeme) > > On 29 March 2016 at 19:06, Duran, Leo <leo.duran@amd.com> wrote: > >> -----Original Message----- > >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] > >> Sent: Tuesday, March 29, 2016 11:14 AM > >> To: Duran, Leo; Charles Garcia-Tobin; Achin Gupta > >> Cc: edk2-devel-01; Leif Lindholm > >> Subject: Re: [PATCH] MdeModulePkg: consistent AllocatePages() support > >> for tables above 4 GB's > >> > >> On 29 March 2016 at 18:06, Leo Duran <leo.duran@amd.com> wrote: > >> > Signed-off-by: Leo Duran <leo.duran@amd.com> > >> > >> Hi Leo, > >> > >> This particular instance was also discussed in the context of the > >> patch that introduces the AcpiExposedTableVersions PCD. > >> > >> The problem with this particular allocation is that it is not located > >> below 4 GB because it needs to be addressable by a legacy ACPI table, > >> but because it needs to be accessible from PEI, which is typically 32-bit on > a X64 system. > >> Fortunately, we don't have that table on arm64, so this particular > >> instance does not matter. > >> > >> In general, however, there are numerous instances in the EDK2 code > >> base where an allocation is hardcoded to be limited to 4 GB so that > >> PEI is guaranteed to be able to access it. As it turns out, there are > >> even platforms that do run PEI in 64-bit mode, but only map the > >> lowest > >> 4 GB of memory simply because all DXE code that performs allocations > >> that need to be accessible from PEI is already capped to 4 GB. > >> > >> This is something I brought up with the UEFI forum, and is going to > >> be addressed in the PI spec (hopefully). This should allow us to > >> describe in PEI the range it can access, so that DXE can take this > >> information into account, rather than guess that below 4 GB is okay. > >> > >> As far as this patch is concerned, I think we should just drop it, > >> and perhaps revisit it once we know how to proceed with these PEI > accessible allocations. > >> Unless you do expose a FACS table in your firmware? > > > > Yes, I am declaring an FACS table. > > > > It was my understanding that the FACS is of questionable use for arm64, but I > could be wrong. > Ummh... I'd expect that it should be OK to include that table, even if the table contains a (valid) header and NULL pointers. Or are you saying that the ACPI code in EDK2 simply should not support having FACS on ARM64? > Al, Graeme: any thoughts? Thanks > > -- > Ard. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 29 March 2016 at 19:14, Duran, Leo <leo.duran@amd.com> wrote: > > >> -----Original Message----- >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] >> Sent: Tuesday, March 29, 2016 12:10 PM >> To: Duran, Leo >> Cc: Charles Garcia-Tobin; Achin Gupta; edk2-devel-01; Leif Lindholm >> Subject: Re: [PATCH] MdeModulePkg: consistent AllocatePages() support for >> tables above 4 GB's >> >> (+ Al, Graeme) >> >> On 29 March 2016 at 19:06, Duran, Leo <leo.duran@amd.com> wrote: >> >> -----Original Message----- >> >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] >> >> Sent: Tuesday, March 29, 2016 11:14 AM >> >> To: Duran, Leo; Charles Garcia-Tobin; Achin Gupta >> >> Cc: edk2-devel-01; Leif Lindholm >> >> Subject: Re: [PATCH] MdeModulePkg: consistent AllocatePages() support >> >> for tables above 4 GB's >> >> >> >> On 29 March 2016 at 18:06, Leo Duran <leo.duran@amd.com> wrote: >> >> > Signed-off-by: Leo Duran <leo.duran@amd.com> >> >> >> >> Hi Leo, >> >> >> >> This particular instance was also discussed in the context of the >> >> patch that introduces the AcpiExposedTableVersions PCD. >> >> >> >> The problem with this particular allocation is that it is not located >> >> below 4 GB because it needs to be addressable by a legacy ACPI table, >> >> but because it needs to be accessible from PEI, which is typically 32-bit on >> a X64 system. >> >> Fortunately, we don't have that table on arm64, so this particular >> >> instance does not matter. >> >> >> >> In general, however, there are numerous instances in the EDK2 code >> >> base where an allocation is hardcoded to be limited to 4 GB so that >> >> PEI is guaranteed to be able to access it. As it turns out, there are >> >> even platforms that do run PEI in 64-bit mode, but only map the >> >> lowest >> >> 4 GB of memory simply because all DXE code that performs allocations >> >> that need to be accessible from PEI is already capped to 4 GB. >> >> >> >> This is something I brought up with the UEFI forum, and is going to >> >> be addressed in the PI spec (hopefully). This should allow us to >> >> describe in PEI the range it can access, so that DXE can take this >> >> information into account, rather than guess that below 4 GB is okay. >> >> >> >> As far as this patch is concerned, I think we should just drop it, >> >> and perhaps revisit it once we know how to proceed with these PEI >> accessible allocations. >> >> Unless you do expose a FACS table in your firmware? >> > >> > Yes, I am declaring an FACS table. >> > >> >> It was my understanding that the FACS is of questionable use for arm64, but I >> could be wrong. >> > Ummh... I'd expect that it should be OK to include that table, even if the table contains a (valid) header and NULL pointers. > Or are you saying that the ACPI code in EDK2 simply should not support having FACS on ARM64? > I am saying that, to my knowledge, the FACS table does not carry any information that we will be able to use in a meaningful way on the OS side on an arm64 system, so there is little point in including it. That nicely dodges the allocation problem, considering that your fix was already shot down when I proposed it as part of my patch. The reason was that, even if you are only interested in supporting ACPI 5.0 and up, you need the FACS to be loaded below 4 GB if you use a 32-bit PEI. And FYI, adding a second PCD just for this was shot down as well so we will have to wait for the PIWG to make up their minds on this one. -- Ard. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
> -----Original Message----- > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] > Sent: Tuesday, March 29, 2016 12:20 PM > To: Duran, Leo > Cc: Charles Garcia-Tobin; Achin Gupta; edk2-devel-01; Leif Lindholm > Subject: Re: [PATCH] MdeModulePkg: consistent AllocatePages() support for > tables above 4 GB's > > On 29 March 2016 at 19:14, Duran, Leo <leo.duran@amd.com> wrote: > > > > > >> -----Original Message----- > >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] > >> Sent: Tuesday, March 29, 2016 12:10 PM > >> To: Duran, Leo > >> Cc: Charles Garcia-Tobin; Achin Gupta; edk2-devel-01; Leif Lindholm > >> Subject: Re: [PATCH] MdeModulePkg: consistent AllocatePages() support > >> for tables above 4 GB's > >> > >> (+ Al, Graeme) > >> > >> On 29 March 2016 at 19:06, Duran, Leo <leo.duran@amd.com> wrote: > >> >> -----Original Message----- > >> >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] > >> >> Sent: Tuesday, March 29, 2016 11:14 AM > >> >> To: Duran, Leo; Charles Garcia-Tobin; Achin Gupta > >> >> Cc: edk2-devel-01; Leif Lindholm > >> >> Subject: Re: [PATCH] MdeModulePkg: consistent AllocatePages() > >> >> support for tables above 4 GB's > >> >> > >> >> On 29 March 2016 at 18:06, Leo Duran <leo.duran@amd.com> wrote: > >> >> > Signed-off-by: Leo Duran <leo.duran@amd.com> > >> >> > >> >> Hi Leo, > >> >> > >> >> This particular instance was also discussed in the context of the > >> >> patch that introduces the AcpiExposedTableVersions PCD. > >> >> > >> >> The problem with this particular allocation is that it is not > >> >> located below 4 GB because it needs to be addressable by a legacy > >> >> ACPI table, but because it needs to be accessible from PEI, which > >> >> is typically 32-bit on > >> a X64 system. > >> >> Fortunately, we don't have that table on arm64, so this particular > >> >> instance does not matter. > >> >> > >> >> In general, however, there are numerous instances in the EDK2 code > >> >> base where an allocation is hardcoded to be limited to 4 GB so > >> >> that PEI is guaranteed to be able to access it. As it turns out, > >> >> there are even platforms that do run PEI in 64-bit mode, but only > >> >> map the lowest > >> >> 4 GB of memory simply because all DXE code that performs > >> >> allocations that need to be accessible from PEI is already capped to 4 > GB. > >> >> > >> >> This is something I brought up with the UEFI forum, and is going > >> >> to be addressed in the PI spec (hopefully). This should allow us > >> >> to describe in PEI the range it can access, so that DXE can take > >> >> this information into account, rather than guess that below 4 GB is > okay. > >> >> > >> >> As far as this patch is concerned, I think we should just drop it, > >> >> and perhaps revisit it once we know how to proceed with these PEI > >> accessible allocations. > >> >> Unless you do expose a FACS table in your firmware? > >> > > >> > Yes, I am declaring an FACS table. > >> > > >> > >> It was my understanding that the FACS is of questionable use for > >> arm64, but I could be wrong. > >> > > Ummh... I'd expect that it should be OK to include that table, even if the > table contains a (valid) header and NULL pointers. > > Or are you saying that the ACPI code in EDK2 simply should not support > having FACS on ARM64? > > > > I am saying that, to my knowledge, the FACS table does not carry any > information that we will be able to use in a meaningful way on the OS side on > an arm64 system, so there is little point in including it. > > That nicely dodges the allocation problem, considering that your fix was > already shot down when I proposed it as part of my patch. The reason was > that, even if you are only interested in supporting ACPI > 5.0 and up, you need the FACS to be loaded below 4 GB if you use a 32-bit > PEI. And FYI, adding a second PCD just for this was shot down as well so we > will have to wait for the PIWG to make up their minds on this one. > Fine, let's drop this patch, pending resolution from PIWG. Thanks, Leo. > -- > Ard. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c index 7f95b9d..e1fd928 100644 --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c @@ -518,7 +518,7 @@ AddTableToList ( // ASSERT ((EFI_PAGE_SIZE % 64) == 0); Status = gBS->AllocatePages ( - AllocateMaxAddress, + mAcpiTableAllocType, EfiACPIMemoryNVS, CurrentTableList->NumberOfPages, &CurrentTableList->PageAddress
Signed-off-by: Leo Duran <leo.duran@amd.com> --- MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 1.9.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel