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


Laszlo Ersek
 

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.


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/

(And I don't really see the point of an FDF include file.)

Thanks!
Laszlo

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