public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Jianyong Wu" <jianyong.wu@arm.com>
To: "devel@edk2.groups.io" <devel@edk2.groups.io>,
	Jianyong Wu <Jianyong.Wu@arm.com>,
	Sami Mujawar <Sami.Mujawar@arm.com>
Cc: "ardb+tianocore@kernel.org" <ardb+tianocore@kernel.org>,
	Justin He <Justin.He@arm.com>, nd <nd@arm.com>
Subject: Re: [edk2-devel] [PATCH 3/3] CloudHv:arm: add kernel load driver into dsc/fdf
Date: Mon, 24 Apr 2023 02:36:45 +0000	[thread overview]
Message-ID: <DB9PR08MB75115FF45AF774328D136BCCF4679@DB9PR08MB7511.eurprd08.prod.outlook.com> (raw)
In-Reply-To: <17393714CBDB554C.25137@groups.io>

[-- Attachment #1: Type: text/plain, Size: 10557 bytes --]

Hi Sami,

Following the last discussion, I check the Cpu stack in edk2 and draw a draft for the memory layout of Cpu stack, Initrd and Kernel image: see it below:

|-------------------------| <-- top of memory   -------|
|                         |                            |
|primary cpu stack (16KB) |                            \
|                         |                            / reserve 2MB is enough
|-------------------------|                            |
|  secondary cpus stack   |                            |
|  (4KB * (cputcount-1))  |                            |
|-------------------------| <-- top of cpu stack ------|
|                         |
|     initrd image        |
|        (64MB)           |
|                         |
|-------------------------|
|                         |
|     kernel image        |
|        (64MB)           |
|                         |
|-------------------------|
|                         |
|                         |
|                         |
|  normal memory region   |
|                         |
|                         |
|                         |
|-------------------------| <-- memory base address
That’s say, the top 2MB of memory region reserved for Cpu stack. It’s enough as it can accommodate for more than 500 Cpus. There are two 64MB regions reserved below Cpu stack, one for Initrd and another for Kernel image.
We can guarantee there is no confliction between each other in cloud hypervisor and double check it in edk2 by checking if there is 2MB region reserved for Cpu stack.

WDYT?

Thanks
Jianyong
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Jianyong Wu via groups.io
Sent: 2023年1月11日 17:28
To: Sami Mujawar <Sami.Mujawar@arm.com>; devel@edk2.groups.io
Cc: ardb+tianocore@kernel.org; Justin He <Justin.He@arm.com>; nd <nd@arm.com>
Subject: Re: [edk2-devel] [PATCH 3/3] CloudHv:arm: add kernel load driver into dsc/fdf

Hi Sami,

Thanks for quick response.

The other comment (probably for patch 2/3 in this series) was with regards to the memory reservation for the Initrd and Kernel arguments. I think these regions should be reserved (in addition to the kernel region) to avoid any accidental overwriting. I believe if you start reducing the System Memory size at some point you may find that the Initrd and Kernel arguments are overwritten. Can you check this, please?

For the current implementation, initrd has been considered and placed on the top of memory in Cloud Hypervisor. And for kernel image, in the current design, is placed under initrd image. As to kernel parameters, it is just a string in “chosen node” and will not occupy memory. So, I think, in general, it won’t happen of accidental overwriting. But I need to set to memory region for initrd image to read-only like kernel does to avoid the memory being allocated.

Can you also check if there is a possibility that these regions could overlap the CPU stack, etc. and if so, how do we make sure this never happens?

Generally, kernel stack locates at the lowest part of memory. I think it’s safe to place Kernel and Initrd image on the top of memory, unless the memory size is too small.

WDYT?

Thanks
Jianyong

From: Sami Mujawar <Sami.Mujawar@arm.com<mailto:Sami.Mujawar@arm.com>>
Sent: Wednesday, January 11, 2023 4:35 PM
To: Jianyong Wu <Jianyong.Wu@arm.com<mailto:Jianyong.Wu@arm.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org>; Justin He <Justin.He@arm.com<mailto:Justin.He@arm.com>>; nd <nd@arm.com<mailto:nd@arm.com>>
Subject: Re: [edk2-devel] [PATCH 3/3] CloudHv:arm: add kernel load driver into dsc/fdf

Hi Jianyong,

Thank you for your kind wishes and happy new year to you.

I think linking with a NULL lib implementation of QemuBootOrderLib would be good.

The other comment (probably for patch 2/3 in this series) was with regards to the memory reservation for the Initrd and Kernel arguments. I think these regions should be reserved (in addition to the kernel region) to avoid any accidental overwriting. I believe if you start reducing the System Memory size at some point you may find that the Initrd and Kernel arguments are overwritten. Can you check this, please?
Can you also check if there is a possibility that these regions could overlap the CPU stack, etc. and if so, how do we make sure this never happens?

Regards,

Sami Mujawar

From: Jianyong Wu <Jianyong.Wu@arm.com<mailto:Jianyong.Wu@arm.com>>
Date: Wednesday, 11 January 2023 at 08:12
To: "devel@edk2.groups.io<mailto:devel@edk2.groups.io>" <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>, Jianyong Wu <Jianyong.Wu@arm.com<mailto:Jianyong.Wu@arm.com>>, Sami Mujawar <Sami.Mujawar@arm.com<mailto:Sami.Mujawar@arm.com>>
Cc: "ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org>" <ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org>>, Justin He <Justin.He@arm.com<mailto:Justin.He@arm.com>>, nd <nd@arm.com<mailto:nd@arm.com>>
Subject: RE: [edk2-devel] [PATCH 3/3] CloudHv:arm: add kernel load driver into dsc/fdf

Hi Sami,

Happy new year and hope you are well.

Following up last discussion. I try to add null lib instead of QemuBootOrderLib and it works. If there are no other comments, I will post the change in the next version.

Regards,
Jianyong

From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Jianyong Wu via groups.io
Sent: Wednesday, November 23, 2022 1:44 PM
To: Sami Mujawar <Sami.Mujawar@arm.com<mailto:Sami.Mujawar@arm.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org>; Justin He <Justin.He@arm.com<mailto:Justin.He@arm.com>>; nd <nd@arm.com<mailto:nd@arm.com>>
Subject: Re: [edk2-devel] [PATCH 3/3] CloudHv:arm: add kernel load driver into dsc/fdf

Hi Sami,

Inline reply

From: Sami Mujawar <Sami.Mujawar@arm.com<mailto:Sami.Mujawar@arm.com>>
Sent: Tuesday, November 22, 2022 11:48 PM
To: Jianyong Wu <Jianyong.Wu@arm.com<mailto:Jianyong.Wu@arm.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org>; Justin He <Justin.He@arm.com<mailto:Justin.He@arm.com>>; nd <nd@arm.com<mailto:nd@arm.com>>
Subject: Re: [PATCH 3/3] CloudHv:arm: add kernel load driver into dsc/fdf


Hi Jianyong,

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar
On 16/09/2022 03:46 am, Jianyong Wu wrote:

As CloudHv kernel load fs driver is implemented, add it into dsc/fdf.



Signed-off-by: Jianyong Wu <jianyong.wu@arm.com><mailto:jianyong.wu@arm.com>

---

 ArmVirtPkg/ArmVirtCloudHv.dsc                             | 8 +++++++-

 ArmVirtPkg/ArmVirtCloudHv.fdf                             | 1 +

 .../CloudHvKernelLoaderFsDxe/CloudHvKernelLoaderFsDxe.inf | 1 -

 3 files changed, 8 insertions(+), 2 deletions(-)



diff --git a/ArmVirtPkg/ArmVirtCloudHv.dsc b/ArmVirtPkg/ArmVirtCloudHv.dsc

index 7ca7a391d9..92ccd4ef12 100644

--- a/ArmVirtPkg/ArmVirtCloudHv.dsc

+++ b/ArmVirtPkg/ArmVirtCloudHv.dsc

@@ -37,13 +37,15 @@

   # Virtio Support

   VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf

   VirtioMmioDeviceLib|OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf

+  QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibMmio.inf
[SAMI] How does this work for CloudHv?

Yes, like you said below, it’s due to the change of BootManagerLib.




+  QemuLoadImageLib|OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf



   ArmPlatformLib|ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNull.inf



   TimerLib|ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.inf

   CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf

   BootLogoLib|MdeModulePkg/Library/BootLogoLib/BootLogoLib.inf

-  PlatformBootManagerLib|ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf

+  PlatformBootManagerLib|ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf

[SAMI] I believe this brings the dependency on QemuBootOrderLib which requires QemuFwCfgLib. Is there a way to to implement QemuBootOrderLibNull to remove the dependency on QemuFwCfgLib? The QemuBootOrderLib APIs  ConnectDevicesFromQem(), StoreQemuBootOrder(), SetBootOrderFromQemu () and GetFrontPageTimeoutFromQemu ()  could return something like EFI_UNSUPPORTED in QemuBootOrderLibNull.

Can you check, please?

Sounds a good idea, let me have a try.

[/SAMI]



   PlatformBmPrintScLib|OvmfPkg/Library/PlatformBmPrintScLib/PlatformBmPrintScLib.inf

   CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf

   FrameBufferBltLib|MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.inf

@@ -330,6 +332,10 @@

       NULL|MdeModulePkg/Library/BootManagerUiLib/BootManagerUiLib.inf

       NULL|MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenanceManagerUiLib.inf

   }

+  ArmVirtPkg/CloudHvKernelLoaderFsDxe/CloudHvKernelLoaderFsDxe.inf {

+    <LibraryClasses>

+      NULL|OvmfPkg/Library/BlobVerifierLibNull/BlobVerifierLibNull.inf

+  }



   #

   # SCSI Bus and Disk Driver

diff --git a/ArmVirtPkg/ArmVirtCloudHv.fdf b/ArmVirtPkg/ArmVirtCloudHv.fdf

index 81c539590a..15b9c13c59 100644

--- a/ArmVirtPkg/ArmVirtCloudHv.fdf

+++ b/ArmVirtPkg/ArmVirtCloudHv.fdf

@@ -180,6 +180,7 @@ READ_LOCK_STATUS   = TRUE

   INF MdeModulePkg/Universal/DriverHealthManagerDxe/DriverHealthManagerDxe.inf

   INF MdeModulePkg/Universal/BdsDxe/BdsDxe.inf

   INF MdeModulePkg/Application/UiApp/UiApp.inf

+  INF ArmVirtPkg/CloudHvKernelLoaderFsDxe/CloudHvKernelLoaderFsDxe.inf



   #

   # SCSI Bus and Disk Driver

diff --git a/ArmVirtPkg/CloudHvKernelLoaderFsDxe/CloudHvKernelLoaderFsDxe.inf b/ArmVirtPkg/CloudHvKernelLoaderFsDxe/CloudHvKernelLoaderFsDxe.inf

index b7aa6ebb4e..f7b53d0747 100644

--- a/ArmVirtPkg/CloudHvKernelLoaderFsDxe/CloudHvKernelLoaderFsDxe.inf

+++ b/ArmVirtPkg/CloudHvKernelLoaderFsDxe/CloudHvKernelLoaderFsDxe.inf

@@ -24,7 +24,6 @@

   EmbeddedPkg/EmbeddedPkg.dec

   MdePkg/MdePkg.dec

   OvmfPkg/OvmfPkg.dec

-  UnitTestFrameworkPkg/UnitTestFrameworkPkg.dec
[SAMI] This should be part of patch 1/1.

Yes, I will fix it next version



Thanks

Jianyong



 [LibraryClasses]

   BaseLib


[-- Attachment #2: Type: text/html, Size: 31072 bytes --]

  parent reply	other threads:[~2023-04-24  2:37 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-16  2:46 [PATCH 0/3] CloudHv:arm: Enable direct kernel boot Jianyong Wu
2022-09-16  2:46 ` [PATCH 1/3] CloudHv:arm: add kernel load fs driver Jianyong Wu
2022-11-22 15:44   ` Sami Mujawar
2022-11-22 15:46   ` Sami Mujawar
2022-11-23  6:26     ` Jianyong Wu
2022-09-16  2:46 ` [PATCH 2/3] CloudHv:arm: build hob for kernel image memory as read-only Jianyong Wu
2022-11-22 15:47   ` Sami Mujawar
2022-11-23  2:56     ` Jianyong Wu
2022-09-16  2:46 ` [PATCH 3/3] CloudHv:arm: add kernel load driver into dsc/fdf Jianyong Wu
2022-11-22 15:48   ` Sami Mujawar
2022-11-23  5:44     ` Jianyong Wu
     [not found]     ` <172A2070EC93BBE5.3230@groups.io>
2023-01-11  8:12       ` [edk2-devel] " Jianyong Wu
2023-01-11  8:35         ` Sami Mujawar
2023-01-11  9:27           ` Jianyong Wu
     [not found]           ` <17393714CBDB554C.25137@groups.io>
2023-04-24  2:36             ` Jianyong Wu [this message]
2023-05-23  6:48               ` Jianyong Wu
2022-11-15  3:00 ` [PATCH 0/3] CloudHv:arm: Enable direct kernel boot Jianyong Wu
2022-11-15  8:51   ` Sami Mujawar
2022-11-15  9:01     ` Jianyong Wu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DB9PR08MB75115FF45AF774328D136BCCF4679@DB9PR08MB7511.eurprd08.prod.outlook.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox