[edk2] ArmVirtPkg/HighMemDxe: allow patchable PCD for PcdSystemMemoryBase

Message ID 1468328413-6010-1-git-send-email-ard.biesheuvel@linaro.org
State Accepted
Commit cb9f629e882251b7456176ee555f1b6b0c097d20
Headers show

Commit Message

Ard Biesheuvel July 12, 2016, 1 p.m.
Redefine the reference to PcdSystemMemoryBase in HighMemDxe.inf as
a plain [Pcd] rather than [FixedPcd] (and fix up the code as
appropriate). This allows us to align ArmVirtQemuKernel with
ArmVirtQemu, given that the former uses a patchable PCD not a fixed
PCD.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---

Apologies for the sloppiness on my part, but at least I caught it in time :-)

This change is required before we can start using HighMemDxe in
ArmVirtQemuKernel.

 ArmVirtPkg/HighMemDxe/HighMemDxe.c   | 2 +-
 ArmVirtPkg/HighMemDxe/HighMemDxe.inf | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
1.9.1

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

Comments

Laszlo Ersek July 12, 2016, 1:07 p.m. | #1
On 07/12/16 15:00, Ard Biesheuvel wrote:
> Redefine the reference to PcdSystemMemoryBase in HighMemDxe.inf as

> a plain [Pcd] rather than [FixedPcd] (and fix up the code as

> appropriate). This allows us to align ArmVirtQemuKernel with

> ArmVirtQemu, given that the former uses a patchable PCD not a fixed

> PCD.

> 

> Contributed-under: TianoCore Contribution Agreement 1.0

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

> 

> Apologies for the sloppiness on my part, but at least I caught it in time :-)

> 

> This change is required before we can start using HighMemDxe in

> ArmVirtQemuKernel.

> 

>  ArmVirtPkg/HighMemDxe/HighMemDxe.c   | 2 +-

>  ArmVirtPkg/HighMemDxe/HighMemDxe.inf | 2 +-

>  2 files changed, 2 insertions(+), 2 deletions(-)

> 

> diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.c b/ArmVirtPkg/HighMemDxe/HighMemDxe.c

> index 4963164fbd8a..7fd7e8e9a539 100644

> --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.c

> +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.c

> @@ -74,7 +74,7 @@ InitializeHighMemDxe (

>          CurBase = fdt64_to_cpu (((UINT64 *)RegProp)[0]);

>          CurSize = fdt64_to_cpu (((UINT64 *)RegProp)[1]);

>  

> -        if (FixedPcdGet64 (PcdSystemMemoryBase) != CurBase) {

> +        if (PcdGet64 (PcdSystemMemoryBase) != CurBase) {

>            Status = gDS->AddMemorySpace (

>                            EfiGcdMemoryTypeSystemMemory,

>                            CurBase, CurSize,

> diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf

> index 2b397626a450..ae632a8bee93 100644

> --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf

> +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf

> @@ -45,7 +45,7 @@ [LibraryClasses]

>  [Guids]

>    gFdtHobGuid

>  

> -[FixedPcd]

> +[Pcd]

>    gArmTokenSpaceGuid.PcdSystemMemoryBase

>  

>  [Depex]

> 


Reviewed-by: Laszlo Ersek <lersek@redhat.com>


You might also want to port this driver to FdtClientProtocol down the
road :)

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel July 12, 2016, 1:18 p.m. | #2
On 12 July 2016 at 15:07, Laszlo Ersek <lersek@redhat.com> wrote:
> On 07/12/16 15:00, Ard Biesheuvel wrote:

>> Redefine the reference to PcdSystemMemoryBase in HighMemDxe.inf as

>> a plain [Pcd] rather than [FixedPcd] (and fix up the code as

>> appropriate). This allows us to align ArmVirtQemuKernel with

>> ArmVirtQemu, given that the former uses a patchable PCD not a fixed

>> PCD.

>>

>> Contributed-under: TianoCore Contribution Agreement 1.0

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> ---

>>

>> Apologies for the sloppiness on my part, but at least I caught it in time :-)

>>

>> This change is required before we can start using HighMemDxe in

>> ArmVirtQemuKernel.

>>

>>  ArmVirtPkg/HighMemDxe/HighMemDxe.c   | 2 +-

>>  ArmVirtPkg/HighMemDxe/HighMemDxe.inf | 2 +-

>>  2 files changed, 2 insertions(+), 2 deletions(-)

>>

>> diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.c b/ArmVirtPkg/HighMemDxe/HighMemDxe.c

>> index 4963164fbd8a..7fd7e8e9a539 100644

>> --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.c

>> +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.c

>> @@ -74,7 +74,7 @@ InitializeHighMemDxe (

>>          CurBase = fdt64_to_cpu (((UINT64 *)RegProp)[0]);

>>          CurSize = fdt64_to_cpu (((UINT64 *)RegProp)[1]);

>>

>> -        if (FixedPcdGet64 (PcdSystemMemoryBase) != CurBase) {

>> +        if (PcdGet64 (PcdSystemMemoryBase) != CurBase) {

>>            Status = gDS->AddMemorySpace (

>>                            EfiGcdMemoryTypeSystemMemory,

>>                            CurBase, CurSize,

>> diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf

>> index 2b397626a450..ae632a8bee93 100644

>> --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf

>> +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf

>> @@ -45,7 +45,7 @@ [LibraryClasses]

>>  [Guids]

>>    gFdtHobGuid

>>

>> -[FixedPcd]

>> +[Pcd]

>>    gArmTokenSpaceGuid.PcdSystemMemoryBase

>>

>>  [Depex]

>>

>

> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

>


Thanks.

> You might also want to port this driver to FdtClientProtocol down the

> road :)

>


Indeed, I realized the same when looking at the code. Another problem
is that this code assumes that each DT node with device_type=memory
describes a single range in its 'reg' property, but in reality, a
single node can describe several disjoint regions. As
Documentation/devicetree/booting-without-of.txt in the linux repo puts
it:

"""
[...]
You can either create a single node with all memory ranges in its reg
property, or you can create several nodes, as you wish.
[...]

Required properties:

- device_type : has to be "memory"
- reg : This property contains all the physical memory ranges of your
board. It's a list of addresses/sizes concatenated together, with the
number of cells of each defined by the #address-cells and #size-cells
of the root node.
"""

The problem is that I am not exactly sure under which circumstances
QEMU produces disjoint memory regions, so I have no simple way of
testing this.

-- 
Ard.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek July 12, 2016, 1:25 p.m. | #3
On 07/12/16 15:18, Ard Biesheuvel wrote:
> On 12 July 2016 at 15:07, Laszlo Ersek <lersek@redhat.com> wrote:

>> On 07/12/16 15:00, Ard Biesheuvel wrote:

>>> Redefine the reference to PcdSystemMemoryBase in HighMemDxe.inf as

>>> a plain [Pcd] rather than [FixedPcd] (and fix up the code as

>>> appropriate). This allows us to align ArmVirtQemuKernel with

>>> ArmVirtQemu, given that the former uses a patchable PCD not a fixed

>>> PCD.

>>>

>>> Contributed-under: TianoCore Contribution Agreement 1.0

>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>>> ---

>>>

>>> Apologies for the sloppiness on my part, but at least I caught it in time :-)

>>>

>>> This change is required before we can start using HighMemDxe in

>>> ArmVirtQemuKernel.

>>>

>>>  ArmVirtPkg/HighMemDxe/HighMemDxe.c   | 2 +-

>>>  ArmVirtPkg/HighMemDxe/HighMemDxe.inf | 2 +-

>>>  2 files changed, 2 insertions(+), 2 deletions(-)

>>>

>>> diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.c b/ArmVirtPkg/HighMemDxe/HighMemDxe.c

>>> index 4963164fbd8a..7fd7e8e9a539 100644

>>> --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.c

>>> +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.c

>>> @@ -74,7 +74,7 @@ InitializeHighMemDxe (

>>>          CurBase = fdt64_to_cpu (((UINT64 *)RegProp)[0]);

>>>          CurSize = fdt64_to_cpu (((UINT64 *)RegProp)[1]);

>>>

>>> -        if (FixedPcdGet64 (PcdSystemMemoryBase) != CurBase) {

>>> +        if (PcdGet64 (PcdSystemMemoryBase) != CurBase) {

>>>            Status = gDS->AddMemorySpace (

>>>                            EfiGcdMemoryTypeSystemMemory,

>>>                            CurBase, CurSize,

>>> diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf

>>> index 2b397626a450..ae632a8bee93 100644

>>> --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf

>>> +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf

>>> @@ -45,7 +45,7 @@ [LibraryClasses]

>>>  [Guids]

>>>    gFdtHobGuid

>>>

>>> -[FixedPcd]

>>> +[Pcd]

>>>    gArmTokenSpaceGuid.PcdSystemMemoryBase

>>>

>>>  [Depex]

>>>

>>

>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

>>

> 

> Thanks.

> 

>> You might also want to port this driver to FdtClientProtocol down the

>> road :)

>>

> 

> Indeed, I realized the same when looking at the code. Another problem

> is that this code assumes that each DT node with device_type=memory

> describes a single range in its 'reg' property, but in reality, a

> single node can describe several disjoint regions. As

> Documentation/devicetree/booting-without-of.txt in the linux repo puts

> it:

> 

> """

> [...]

> You can either create a single node with all memory ranges in its reg

> property, or you can create several nodes, as you wish.

> [...]

> 

> Required properties:

> 

> - device_type : has to be "memory"

> - reg : This property contains all the physical memory ranges of your

> board. It's a list of addresses/sizes concatenated together, with the

> number of cells of each defined by the #address-cells and #size-cells

> of the root node.

> """

> 

> The problem is that I am not exactly sure under which circumstances

> QEMU produces disjoint memory regions, so I have no simple way of

> testing this.


The usual method we follow here:
- look at the QEMU code
- write edk2 code accordingly
- ASSERT() that the data from QEMU still looks the way it looked when we
last looked. :)

Thanks
Laszlo


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek July 12, 2016, 1:28 p.m. | #4
On 07/12/16 15:25, Laszlo Ersek wrote:
> On 07/12/16 15:18, Ard Biesheuvel wrote:

>> On 12 July 2016 at 15:07, Laszlo Ersek <lersek@redhat.com> wrote:

>>> On 07/12/16 15:00, Ard Biesheuvel wrote:

>>>> Redefine the reference to PcdSystemMemoryBase in HighMemDxe.inf as

>>>> a plain [Pcd] rather than [FixedPcd] (and fix up the code as

>>>> appropriate). This allows us to align ArmVirtQemuKernel with

>>>> ArmVirtQemu, given that the former uses a patchable PCD not a fixed

>>>> PCD.

>>>>

>>>> Contributed-under: TianoCore Contribution Agreement 1.0

>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>>>> ---

>>>>

>>>> Apologies for the sloppiness on my part, but at least I caught it in time :-)

>>>>

>>>> This change is required before we can start using HighMemDxe in

>>>> ArmVirtQemuKernel.

>>>>

>>>>  ArmVirtPkg/HighMemDxe/HighMemDxe.c   | 2 +-

>>>>  ArmVirtPkg/HighMemDxe/HighMemDxe.inf | 2 +-

>>>>  2 files changed, 2 insertions(+), 2 deletions(-)

>>>>

>>>> diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.c b/ArmVirtPkg/HighMemDxe/HighMemDxe.c

>>>> index 4963164fbd8a..7fd7e8e9a539 100644

>>>> --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.c

>>>> +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.c

>>>> @@ -74,7 +74,7 @@ InitializeHighMemDxe (

>>>>          CurBase = fdt64_to_cpu (((UINT64 *)RegProp)[0]);

>>>>          CurSize = fdt64_to_cpu (((UINT64 *)RegProp)[1]);

>>>>

>>>> -        if (FixedPcdGet64 (PcdSystemMemoryBase) != CurBase) {

>>>> +        if (PcdGet64 (PcdSystemMemoryBase) != CurBase) {

>>>>            Status = gDS->AddMemorySpace (

>>>>                            EfiGcdMemoryTypeSystemMemory,

>>>>                            CurBase, CurSize,

>>>> diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf

>>>> index 2b397626a450..ae632a8bee93 100644

>>>> --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf

>>>> +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf

>>>> @@ -45,7 +45,7 @@ [LibraryClasses]

>>>>  [Guids]

>>>>    gFdtHobGuid

>>>>

>>>> -[FixedPcd]

>>>> +[Pcd]

>>>>    gArmTokenSpaceGuid.PcdSystemMemoryBase

>>>>

>>>>  [Depex]

>>>>

>>>

>>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

>>>

>>

>> Thanks.

>>

>>> You might also want to port this driver to FdtClientProtocol down the

>>> road :)

>>>

>>

>> Indeed, I realized the same when looking at the code. Another problem

>> is that this code assumes that each DT node with device_type=memory

>> describes a single range in its 'reg' property, but in reality, a

>> single node can describe several disjoint regions. As

>> Documentation/devicetree/booting-without-of.txt in the linux repo puts

>> it:

>>

>> """

>> [...]

>> You can either create a single node with all memory ranges in its reg

>> property, or you can create several nodes, as you wish.

>> [...]

>>

>> Required properties:

>>

>> - device_type : has to be "memory"

>> - reg : This property contains all the physical memory ranges of your

>> board. It's a list of addresses/sizes concatenated together, with the

>> number of cells of each defined by the #address-cells and #size-cells

>> of the root node.

>> """

>>

>> The problem is that I am not exactly sure under which circumstances

>> QEMU produces disjoint memory regions, so I have no simple way of

>> testing this.

> 

> The usual method we follow here:

> - look at the QEMU code

> - write edk2 code accordingly

> - ASSERT() that the data from QEMU still looks the way it looked when we

> last looked. :)


We can also ask Shannon about the QEMU behavior (Shannon wrote this
feature for ArmVirtPkg, 68312710615c). CC'ing him.

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

Patch

diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.c b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
index 4963164fbd8a..7fd7e8e9a539 100644
--- a/ArmVirtPkg/HighMemDxe/HighMemDxe.c
+++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
@@ -74,7 +74,7 @@  InitializeHighMemDxe (
         CurBase = fdt64_to_cpu (((UINT64 *)RegProp)[0]);
         CurSize = fdt64_to_cpu (((UINT64 *)RegProp)[1]);
 
-        if (FixedPcdGet64 (PcdSystemMemoryBase) != CurBase) {
+        if (PcdGet64 (PcdSystemMemoryBase) != CurBase) {
           Status = gDS->AddMemorySpace (
                           EfiGcdMemoryTypeSystemMemory,
                           CurBase, CurSize,
diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf
index 2b397626a450..ae632a8bee93 100644
--- a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf
+++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf
@@ -45,7 +45,7 @@  [LibraryClasses]
 [Guids]
   gFdtHobGuid
 
-[FixedPcd]
+[Pcd]
   gArmTokenSpaceGuid.PcdSystemMemoryBase
 
 [Depex]