Hi Sami,
Inline reply.
> -----Original Message-----
> From: Sami Mujawar <sami.muja...@arm.com>
> Sent: Tuesday, November 22, 2022 11:48 PM
> To: Jianyong Wu <jianyong...@arm.com>; devel@edk2.groups.io
> Cc: ardb+tianoc...@kernel.org; Justin He <justin...@arm.com>; nd
> <n...@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...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
