Re: [PATCH v1 1/4] ArmVirtPkg: Library: Memory initialization for Cloud Hypervisor


Jianyong Wu
 

Hi Laszlo,

-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com>
Sent: Friday, April 23, 2021 8:00 PM
To: Jianyong Wu <Jianyong.Wu@arm.com>; edk2-devel-groups-io
<devel@edk2.groups.io>; Sami Mujawar <Sami.Mujawar@arm.com>
Cc: Justin He <Justin.He@arm.com>; Ard Biesheuvel
<ardb+tianocore@kernel.org>; Leif Lindholm <leif@nuviainc.com>
Subject: Re: [edk2-devel] [PATCH v1 1/4] ArmVirtPkg: Library: Memory
initialization for Cloud Hypervisor

Hi Jianyong,

On 04/22/21 15:56, Laszlo Ersek wrote:

(2) "Clh" is a catastrophically bad abbreviation. The whole point of
your work is to add Cloud Hypervisor support, so why trash the most
relevant information in the file names with an inane abbreviation?

(Not to mention that the name "Cloud Hypervisor" itself is as
nondescript as possible. :/)
In an attempt to approach this constructively, I've given it more thought.
Does "CloudHv" sound acceptable to the community? I've seen "hv" stand
for "hypervisor" frequently.

Yeah, CloudHv is better, as the original name is too long. I will take it as the abbreviation of Cloud Hypervisor.

I have another high-level note. I could delay it until after you post v2, but I
figure I could save you some time by sharing my observation with you right
now.

I think that the ACPI platform stuff, in patch#2, does not belong in
OvmfPkg/AcpiPlatformDxe. What's more, I don't think it belongs in OvmfPkg,
even.

The CloudHvAcpiPlatformDxe and CloudHvPlatformHasAcpiDtDxe drivers
should exist as stand-alone, self-contained drivers; they should be as minimal
as possible. This is already a given for "CloudHvPlatformHasAcpiDtDxe", but it
should also be possible for "CloudHvAcpiPlatformDxe".
OvmfPkg/AcpiPlatformDxe is a complex driver, and the overlap between
what OvmfPkg/AcpiPlatformDxe currently does, and what
CloudHvAcpiPlatformDxe actually *needs*, is virtually nil.

And so, the series shouldn't touch OvmfPkg at all.

Ultimately I suggest following the Xen pattern that can be seen under
ArmVirtPkg already. In detail, the following files and directories should
contain the new platform:

ArmVirtPkg/ArmVirtCloudHv.dsc
ArmVirtPkg/ArmVirtCloudHv.fdf
ArmVirtPkg/CloudHvAcpiPlatformDxe/
ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/
ArmVirtPkg/Library/CloudHvVirtMemInfoLib/
Ok , it seems more coherent. I will reorganize the files according to Acpi.

(And I don't really see the point of an FDF include file.)
Yeah, I can include them into the fdf file directly.

Thanks
Jianyong


Thanks!
Laszlo
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Join devel@edk2.groups.io to automatically receive all group messages.