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

["Laszlo Ersek" <lersek@redhat.com>]

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