As we use memory to pass kernel image, the memory region where kernel
image locates should be added into hob as read-only.
Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
---
.../CloudHvVirtMemInfoLib.c | 66 +++++++++++++++++--
1 file changed, 62 insertions(+), 4 deletions(-)
diff --git a/ArmVirtPkg/Library/CloudHvVirtMemInfoLib/CloudHvVirtMemInfoLib.c b/ArmVirtPkg/Library/CloudHvVirtMemInfoLib/CloudHvVirtMemInfoLib.c
index 28a0c0b078..d9b7d51a16 100644
--- a/ArmVirtPkg/Library/CloudHvVirtMemInfoLib/CloudHvVirtMemInfoLib.c
+++ b/ArmVirtPkg/Library/CloudHvVirtMemInfoLib/CloudHvVirtMemInfoLib.c
@@ -37,13 +37,14 @@ CloudHvVirtMemInfoPeiLibConstructor (
)
{
VOID *DeviceTreeBase;
- EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes;
+ EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes, ReadOnlyResourceAttributes;
INT32 Node, Prev;
UINT64 FirMemNodeBase, FirMemNodeSize;
- UINT64 CurBase, MemBase;
+ UINT64 CurBase, MemBase, CurSizeOff;
UINT64 CurSize;
+ UINT64 KernelStart, KernelSize;
CONST CHAR8 *Type;
- INT32 Len;
+ INT32 Len, ChosenNode;
CONST UINT64 *RegProp;
RETURN_STATUS PcdStatus;
UINT8 Index;
@@ -53,6 +54,8 @@ CloudHvVirtMemInfoPeiLibConstructor (
FirMemNodeBase = 0;
FirMemNodeSize = 0;
Index = 0;
+ CurSizeOff = 0;
+ KernelSize = 0;
MemBase = FixedPcdGet64 (PcdSystemMemoryBase);
ResourceAttributes = (
EFI_RESOURCE_ATTRIBUTE_PRESENT |
@@ -60,6 +63,12 @@ CloudHvVirtMemInfoPeiLibConstructor (
EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
EFI_RESOURCE_ATTRIBUTE_TESTED
);
+ ReadOnlyResourceAttributes = (
+ EFI_RESOURCE_ATTRIBUTE_PRESENT |
+ EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
+ EFI_RESOURCE_ATTRIBUTE_TESTED |
+ EFI_RESOURCE_ATTRIBUTE_READ_ONLY_PROTECTED
+ );
DeviceTreeBase = (VOID *)(UINTN)PcdGet64 (PcdDeviceTreeInitialBaseAddress);
if (DeviceTreeBase == NULL) {
return EFI_NOT_FOUND;
@@ -72,6 +81,21 @@ CloudHvVirtMemInfoPeiLibConstructor (
return EFI_NOT_FOUND;
}
+ //
+ // Try to get kernel image info from DT
+ //
+ ChosenNode = fdt_path_offset (DeviceTreeBase, "/chosen");
+ if (ChosenNode >= 0) {
+ RegProp = fdt_getprop (DeviceTreeBase, ChosenNode, "linux,kernel-start", &Len);
+ if ((RegProp != NULL) && (Len > 0)) {
+ KernelStart = (UINT64)fdt64_to_cpu (ReadUnaligned64 (RegProp));
+ RegProp = fdt_getprop (DeviceTreeBase, ChosenNode, "linux,kernel-size", &Len);
+ if ((RegProp != NULL) && (Len > 0)) {
+ KernelSize = (UINT64)fdt64_to_cpu (ReadUnaligned64 (RegProp));
+ }
+ }
+ }
+
//
// Look for the lowest memory node
//
@@ -105,11 +129,26 @@ CloudHvVirtMemInfoPeiLibConstructor (
// We should build Hob seperately for the memory node except the first one
if (CurBase != MemBase) {
+ // If kernel image resides in current memory node, build hob from CurBase to the beginning of kernel image.
+ if ((KernelSize != 0) && (KernelStart >= CurBase) && (KernelStart + KernelSize <= CurBase + CurSize)) {
+ CurSizeOff = CurBase + CurSize - KernelStart;
+ // align up with 0x1000
+ CurSizeOff = (CurSizeOff + 0xfff) & ~0xfffUL;
+ }
+
BuildResourceDescriptorHob (
EFI_RESOURCE_SYSTEM_MEMORY,
ResourceAttributes,
CurBase,
- CurSize
+ CurSize - CurSizeOff
+ );
+
+ // Add kernel image memory region to hob as read only
+ BuildResourceDescriptorHob (
+ EFI_RESOURCE_SYSTEM_MEMORY,
+ ReadOnlyResourceAttributes,
+ CurBase + CurSize - CurSizeOff,
+ CurSizeOff
);
} else {
FirMemNodeBase = CurBase;
@@ -146,8 +185,27 @@ CloudHvVirtMemInfoPeiLibConstructor (
return EFI_NOT_FOUND;
}
+ CurSizeOff = 0;
+ // Build hob for the lowest memory node from its base to the beginning of kernel image once the kernel image reside here
+ if ((KernelSize != 0) && (KernelStart >= FirMemNodeBase) && (KernelStart + KernelSize <= FirMemNodeBase + FirMemNodeSize)) {
+ CurSizeOff = FirMemNodeBase + FirMemNodeSize - KernelStart;
+ // Caution the alignment
+ CurSizeOff = (CurSizeOff + 0xfff) & ~0xfffUL;
+
+ // Add kernel image memory region to hob as read only
+ BuildResourceDescriptorHob (
+ EFI_RESOURCE_SYSTEM_MEMORY,
+ ReadOnlyResourceAttributes,
+ FirMemNodeBase + FirMemNodeSize - CurSizeOff,
+ CurSizeOff
+ );
+ }
+
+ FirMemNodeSize -= CurSizeOff;
+
PcdStatus = PcdSet64S (PcdSystemMemorySize, FirMemNodeSize);
ASSERT_RETURN_ERROR (PcdStatus);
+
ASSERT (
(((UINT64)PcdGet64 (PcdFdBaseAddress) +
(UINT64)PcdGet32 (PcdFdSize)) <= FirMemNodeBase) ||
--
2.17.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#93884): https://edk2.groups.io/g/devel/message/93884
Mute This Topic: https://groups.io/mt/93715217/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-Hi Jianyong,
Please see my feedback marked inline as [SAMI].
Regards,
Sami Mujawar
On 16/09/2022 03:46 am, Jianyong Wu wrote:
> As we use memory to pass kernel image, the memory region where kernel
> image locates should be added into hob as read-only.
>
> Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> ---
> .../CloudHvVirtMemInfoLib.c | 66 +++++++++++++++++--
> 1 file changed, 62 insertions(+), 4 deletions(-)
>
> diff --git a/ArmVirtPkg/Library/CloudHvVirtMemInfoLib/CloudHvVirtMemInfoLib.c b/ArmVirtPkg/Library/CloudHvVirtMemInfoLib/CloudHvVirtMemInfoLib.c
> index 28a0c0b078..d9b7d51a16 100644
> --- a/ArmVirtPkg/Library/CloudHvVirtMemInfoLib/CloudHvVirtMemInfoLib.c
> +++ b/ArmVirtPkg/Library/CloudHvVirtMemInfoLib/CloudHvVirtMemInfoLib.c
> @@ -37,13 +37,14 @@ CloudHvVirtMemInfoPeiLibConstructor (
> )
> {
> VOID *DeviceTreeBase;
> - EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes;
> + EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes, ReadOnlyResourceAttributes;
> INT32 Node, Prev;
> UINT64 FirMemNodeBase, FirMemNodeSize;
> - UINT64 CurBase, MemBase;
> + UINT64 CurBase, MemBase, CurSizeOff;
> UINT64 CurSize;
> + UINT64 KernelStart, KernelSize;
> CONST CHAR8 *Type;
> - INT32 Len;
> + INT32 Len, ChosenNode;
> CONST UINT64 *RegProp;
> RETURN_STATUS PcdStatus;
> UINT8 Index;
> @@ -53,6 +54,8 @@ CloudHvVirtMemInfoPeiLibConstructor (
> FirMemNodeBase = 0;
> FirMemNodeSize = 0;
> Index = 0;
> + CurSizeOff = 0;
> + KernelSize = 0;
> MemBase = FixedPcdGet64 (PcdSystemMemoryBase);
> ResourceAttributes = (
> EFI_RESOURCE_ATTRIBUTE_PRESENT |
> @@ -60,6 +63,12 @@ CloudHvVirtMemInfoPeiLibConstructor (
> EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
> EFI_RESOURCE_ATTRIBUTE_TESTED
> );
> + ReadOnlyResourceAttributes = (
> + EFI_RESOURCE_ATTRIBUTE_PRESENT |
> + EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
> + EFI_RESOURCE_ATTRIBUTE_TESTED |
> + EFI_RESOURCE_ATTRIBUTE_READ_ONLY_PROTECTED
> + );
> DeviceTreeBase = (VOID *)(UINTN)PcdGet64 (PcdDeviceTreeInitialBaseAddress);
> if (DeviceTreeBase == NULL) {
> return EFI_NOT_FOUND;
> @@ -72,6 +81,21 @@ CloudHvVirtMemInfoPeiLibConstructor (
> return EFI_NOT_FOUND;
> }
>
> + //
> + // Try to get kernel image info from DT
> + //
> + ChosenNode = fdt_path_offset (DeviceTreeBase, "/chosen");
> + if (ChosenNode >= 0) {
> + RegProp = fdt_getprop (DeviceTreeBase, ChosenNode, "linux,kernel-start", &Len);
> + if ((RegProp != NULL) && (Len > 0)) {
> + KernelStart = (UINT64)fdt64_to_cpu (ReadUnaligned64 (RegProp));
> + RegProp = fdt_getprop (DeviceTreeBase, ChosenNode, "linux,kernel-size", &Len);
> + if ((RegProp != NULL) && (Len > 0)) {
> + KernelSize = (UINT64)fdt64_to_cpu (ReadUnaligned64 (RegProp));
> + }
> + }
> + }
> +
> //
> // Look for the lowest memory node
> //
> @@ -105,11 +129,26 @@ CloudHvVirtMemInfoPeiLibConstructor (
>
> // We should build Hob seperately for the memory node except the first one
> if (CurBase != MemBase) {
> + // If kernel image resides in current memory node, build hob from CurBase to the beginning of kernel image.
> + if ((KernelSize != 0) && (KernelStart >= CurBase) && (KernelStart + KernelSize <= CurBase + CurSize)) {
> + CurSizeOff = CurBase + CurSize - KernelStart;
> + // align up with 0x1000
> + CurSizeOff = (CurSizeOff + 0xfff) & ~0xfffUL;
> + }
> +
> BuildResourceDescriptorHob (
> EFI_RESOURCE_SYSTEM_MEMORY,
> ResourceAttributes,
> CurBase,
> - CurSize
> + CurSize - CurSizeOff
> + );
> +
> + // Add kernel image memory region to hob as read only
> + BuildResourceDescriptorHob (
> + EFI_RESOURCE_SYSTEM_MEMORY,
> + ReadOnlyResourceAttributes,
> + CurBase + CurSize - CurSizeOff,
> + CurSizeOff
> );
[SAMI] Can you explain why this is required and what would happen if
this is not done, please? It would be good to add this description to
the commit message.
Also, what about the initrd and the commandline?
[/SAMI]
> } else {
> FirMemNodeBase = CurBase;
> @@ -146,8 +185,27 @@ CloudHvVirtMemInfoPeiLibConstructor (
> return EFI_NOT_FOUND;
> }
>
> + CurSizeOff = 0;
> + // Build hob for the lowest memory node from its base to the beginning of kernel image once the kernel image reside here
> + if ((KernelSize != 0) && (KernelStart >= FirMemNodeBase) && (KernelStart + KernelSize <= FirMemNodeBase + FirMemNodeSize)) {
> + CurSizeOff = FirMemNodeBase + FirMemNodeSize - KernelStart;
> + // Caution the alignment
> + CurSizeOff = (CurSizeOff + 0xfff) & ~0xfffUL;
> +
> + // Add kernel image memory region to hob as read only
> + BuildResourceDescriptorHob (
> + EFI_RESOURCE_SYSTEM_MEMORY,
> + ReadOnlyResourceAttributes,
> + FirMemNodeBase + FirMemNodeSize - CurSizeOff,
> + CurSizeOff
> + );
> + }
> +
> + FirMemNodeSize -= CurSizeOff;
> +
> PcdStatus = PcdSet64S (PcdSystemMemorySize, FirMemNodeSize);
> ASSERT_RETURN_ERROR (PcdStatus);
> +
> ASSERT (
> (((UINT64)PcdGet64 (PcdFdBaseAddress) +
> (UINT64)PcdGet32 (PcdFdSize)) <= FirMemNodeBase) ||
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#96568): https://edk2.groups.io/g/devel/message/96568
Mute This Topic: https://groups.io/mt/93715217/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Hi Sami,
Inline reply.
> -----Original Message-----
> From: Sami Mujawar <Sami.Mujawar@arm.com>
> Sent: Tuesday, November 22, 2022 11:48 PM
> To: Jianyong Wu <Jianyong.Wu@arm.com>; devel@edk2.groups.io
> Cc: ardb+tianocore@kernel.org; Justin He <Justin.He@arm.com>; nd
> <nd@arm.com>
> Subject: Re: [PATCH 2/3] CloudHv:arm: build hob for kernel image memory as
> read-only
>
> Hi Jianyong,
>
> Please see my feedback marked inline as [SAMI].
>
> Regards,
>
> Sami Mujawar
>
> On 16/09/2022 03:46 am, Jianyong Wu wrote:
> > As we use memory to pass kernel image, the memory region where kernel
> > image locates should be added into hob as read-only.
> >
> > Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> > ---
> > .../CloudHvVirtMemInfoLib.c | 66 +++++++++++++++++--
> > 1 file changed, 62 insertions(+), 4 deletions(-)
> >
> > diff --git
> > a/ArmVirtPkg/Library/CloudHvVirtMemInfoLib/CloudHvVirtMemInfoLib.c
> > b/ArmVirtPkg/Library/CloudHvVirtMemInfoLib/CloudHvVirtMemInfoLib.c
> > index 28a0c0b078..d9b7d51a16 100644
> > ---
> a/ArmVirtPkg/Library/CloudHvVirtMemInfoLib/CloudHvVirtMemInfoLib.c
> > +++
> b/ArmVirtPkg/Library/CloudHvVirtMemInfoLib/CloudHvVirtMemInfoLib.c
> > @@ -37,13 +37,14 @@ CloudHvVirtMemInfoPeiLibConstructor (
> > )
> > {
> > VOID *DeviceTreeBase;
> > - EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes;
> > + EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes,
> > + ReadOnlyResourceAttributes;
> > INT32 Node, Prev;
> > UINT64 FirMemNodeBase, FirMemNodeSize;
> > - UINT64 CurBase, MemBase;
> > + UINT64 CurBase, MemBase, CurSizeOff;
> > UINT64 CurSize;
> > + UINT64 KernelStart, KernelSize;
> > CONST CHAR8 *Type;
> > - INT32 Len;
> > + INT32 Len, ChosenNode;
> > CONST UINT64 *RegProp;
> > RETURN_STATUS PcdStatus;
> > UINT8 Index;
> > @@ -53,6 +54,8 @@ CloudHvVirtMemInfoPeiLibConstructor (
> > FirMemNodeBase = 0;
> > FirMemNodeSize = 0;
> > Index = 0;
> > + CurSizeOff = 0;
> > + KernelSize = 0;
> > MemBase = FixedPcdGet64 (PcdSystemMemoryBase);
> > ResourceAttributes = (
> > EFI_RESOURCE_ATTRIBUTE_PRESENT | @@ -60,6
> > +63,12 @@ CloudHvVirtMemInfoPeiLibConstructor (
> > EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
> > EFI_RESOURCE_ATTRIBUTE_TESTED
> > );
> > + ReadOnlyResourceAttributes = (
> > + EFI_RESOURCE_ATTRIBUTE_PRESENT |
> > + EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
> > + EFI_RESOURCE_ATTRIBUTE_TESTED |
> > + EFI_RESOURCE_ATTRIBUTE_READ_ONLY_PROTECTED
> > + );
> > DeviceTreeBase = (VOID *)(UINTN)PcdGet64
> (PcdDeviceTreeInitialBaseAddress);
> > if (DeviceTreeBase == NULL) {
> > return EFI_NOT_FOUND;
> > @@ -72,6 +81,21 @@ CloudHvVirtMemInfoPeiLibConstructor (
> > return EFI_NOT_FOUND;
> > }
> >
> > + //
> > + // Try to get kernel image info from DT // ChosenNode =
> > + fdt_path_offset (DeviceTreeBase, "/chosen"); if (ChosenNode >= 0) {
> > + RegProp = fdt_getprop (DeviceTreeBase, ChosenNode, "linux,kernel-
> start", &Len);
> > + if ((RegProp != NULL) && (Len > 0)) {
> > + KernelStart = (UINT64)fdt64_to_cpu (ReadUnaligned64 (RegProp));
> > + RegProp = fdt_getprop (DeviceTreeBase, ChosenNode,
> "linux,kernel-size", &Len);
> > + if ((RegProp != NULL) && (Len > 0)) {
> > + KernelSize = (UINT64)fdt64_to_cpu (ReadUnaligned64 (RegProp));
> > + }
> > + }
> > + }
> > +
> > //
> > // Look for the lowest memory node
> > //
> > @@ -105,11 +129,26 @@ CloudHvVirtMemInfoPeiLibConstructor (
> >
> > // We should build Hob seperately for the memory node except the
> first one
> > if (CurBase != MemBase) {
> > + // If kernel image resides in current memory node, build hob from
> CurBase to the beginning of kernel image.
> > + if ((KernelSize != 0) && (KernelStart >= CurBase) && (KernelStart +
> KernelSize <= CurBase + CurSize)) {
> > + CurSizeOff = CurBase + CurSize - KernelStart;
> > + // align up with 0x1000
> > + CurSizeOff = (CurSizeOff + 0xfff) & ~0xfffUL;
> > + }
> > +
> > BuildResourceDescriptorHob (
> > EFI_RESOURCE_SYSTEM_MEMORY,
> > ResourceAttributes,
> > CurBase,
> > - CurSize
> > + CurSize - CurSizeOff
> > + );
> > +
> > + // Add kernel image memory region to hob as read only
> > + BuildResourceDescriptorHob (
> > + EFI_RESOURCE_SYSTEM_MEMORY,
> > + ReadOnlyResourceAttributes,
> > + CurBase + CurSize - CurSizeOff,
> > + CurSizeOff
> > );
>
> [SAMI] Can you explain why this is required and what would happen if this is
> not done, please? It would be good to add this description to the commit
> message.
>
As we need put kernel image into a range of memory, we can't let uefi modify that region, so we need set it as read-only.
If not, kernel won't start after load.
But I'm not sure how does it impact kernel, for example, the memory used for kernel may decrease, however, I have no better way to do this.
> Also, what about the initrd and the commandline?
I have not considered initrd yet. If there is a requirement for it, we can add later.
Command line is just a string, so it can be conveyed by fdt directly without occupy memory.
Thanks
Jianyong
>
> [/SAMI]
>
> > } else {
> > FirMemNodeBase = CurBase;
> > @@ -146,8 +185,27 @@ CloudHvVirtMemInfoPeiLibConstructor (
> > return EFI_NOT_FOUND;
> > }
> >
> > + CurSizeOff = 0;
> > + // Build hob for the lowest memory node from its base to the
> > + beginning of kernel image once the kernel image reside here if
> ((KernelSize != 0) && (KernelStart >= FirMemNodeBase) && (KernelStart +
> KernelSize <= FirMemNodeBase + FirMemNodeSize)) {
> > + CurSizeOff = FirMemNodeBase + FirMemNodeSize - KernelStart;
> > + // Caution the alignment
> > + CurSizeOff = (CurSizeOff + 0xfff) & ~0xfffUL;
> > +
> > + // Add kernel image memory region to hob as read only
> > + BuildResourceDescriptorHob (
> > + EFI_RESOURCE_SYSTEM_MEMORY,
> > + ReadOnlyResourceAttributes,
> > + FirMemNodeBase + FirMemNodeSize - CurSizeOff,
> > + CurSizeOff
> > + );
> > + }
> > +
> > + FirMemNodeSize -= CurSizeOff;
> > +
> > PcdStatus = PcdSet64S (PcdSystemMemorySize, FirMemNodeSize);
> > ASSERT_RETURN_ERROR (PcdStatus);
> > +
> > ASSERT (
> > (((UINT64)PcdGet64 (PcdFdBaseAddress) +
> > (UINT64)PcdGet32 (PcdFdSize)) <= FirMemNodeBase) ||
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#96583): https://edk2.groups.io/g/devel/message/96583
Mute This Topic: https://groups.io/mt/93715217/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.