From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web11.8780.1619179234963977455 for ; Fri, 23 Apr 2021 05:00:35 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=eL4O5XU/; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1619179234; h=from:from:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=PR3j0MAu/r1U0ECad6gX44a9haZc9I90Ffbz3qD38nU=; b=eL4O5XU/BjoijJASRvq9kt5fgk8+HBZ6CCSMuNHitFFD2xTZy2om0D2j1IeZTjlsjCeofe XNcX3F6aFNjT6vMKWM+uVJiocMPUAJLxUxnx2cQcfU/Wu8ic9EnpiCbevGYD7a4NQnj78a 5nWz3dY0HJEnP5YS/UZk25lR8vNuFbQ= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-552-I4il_0-aOw2zgBqNbQY7WA-1; Fri, 23 Apr 2021 08:00:30 -0400 X-MC-Unique: I4il_0-aOw2zgBqNbQY7WA-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id C3C88100A605; Fri, 23 Apr 2021 12:00:28 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-115-2.ams2.redhat.com [10.36.115.2]) by smtp.corp.redhat.com (Postfix) with ESMTP id 2373F6BC2E; Fri, 23 Apr 2021 12:00:26 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v1 1/4] ArmVirtPkg: Library: Memory initialization for Cloud Hypervisor From: "Laszlo Ersek" To: Jianyong Wu , edk2-devel-groups-io , Sami Mujawar Cc: justin.he@arm.com, Ard Biesheuvel , Leif Lindholm Reply-To: devel@edk2.groups.io, lersek@redhat.com References: <20210422082440.172160-1-jianyong.wu@arm.com> <20210422082440.172160-2-jianyong.wu@arm.com> Message-ID: Date: Fri, 23 Apr 2021 14:00:26 +0200 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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