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

["Jianyong Wu" <jianyong.wu@arm.com>]

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.