| Message ID | 1468328413-6010-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | Accepted |
| Commit | cb9f629e882251b7456176ee555f1b6b0c097d20 |
| Headers | show |
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
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
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
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
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]
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