diff mbox

[edk2] MdeModulePkg: consistent AllocatePages() support for tables above 4 GB's

Message ID 1459267586-2980-1-git-send-email-leo.duran@amd.com
State New
Headers show

Commit Message

Duran, Leo March 29, 2016, 4:06 p.m. UTC 
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

Comments

Ard Biesheuvel March 29, 2016, 4:14 p.m. UTC | #1 
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
Duran, Leo March 29, 2016, 5:06 p.m. UTC | #2 
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
Ard Biesheuvel March 29, 2016, 5:09 p.m. UTC | #3 
(+ 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
Duran, Leo March 29, 2016, 5:14 p.m. UTC | #4 
> -----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
Ard Biesheuvel March 29, 2016, 5:20 p.m. UTC | #5 
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
Duran, Leo March 29, 2016, 5:23 p.m. UTC | #6 
> -----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 mbox

Patch

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