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>
Date: Wednesday, 11 January 2023 at 08:12
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

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 <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>; 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,

Inline reply

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 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>
---
 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