From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web10.10919.1589458639680060602 for ; Thu, 14 May 2020 05:17:19 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 43ACE1042; Thu, 14 May 2020 05:17:19 -0700 (PDT) Received: from [192.168.1.81] (unknown [10.37.8.255]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id BD5AE3F305; Thu, 14 May 2020 05:17:15 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH v2 06/11] ArmVirtPkg: Add kvmtool platform driver To: Laszlo Ersek , devel@edk2.groups.io, Sami Mujawar Cc: leif@nuviainc.com, Alexandru.Elisei@arm.com, Andre.Przywara@arm.com, Matteo.Carlini@arm.com, Laura.Moretta@arm.com, nd@arm.com References: <20200514084019.71368-1-sami.mujawar@arm.com> <20200514084019.71368-7-sami.mujawar@arm.com> <71feb4af-94a7-defe-c978-5a1775cc7665@redhat.com> From: "Ard Biesheuvel" Message-ID: Date: Thu, 14 May 2020 14:17:11 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <71feb4af-94a7-defe-c978-5a1775cc7665@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 5/14/20 2:12 PM, Laszlo Ersek wrote: > On 05/14/20 11:29, Ard Biesheuvel wrote: >> On 5/14/20 10:40 AM, Sami Mujawar wrote: >>> Kvmtool is a virtual machine manager that enables >>> hosting KVM guests. It essentially provides an >>> emulated platform for guest operating systems. >>> >>> Kvmtool hands of a device tree containing the >>> current hardware configuration to the firmware. >>> >>> A standards-based operating system would use >>> ACPI to consume the platform hardware >>> information, while some operating systems may >>> prefer to use Device Tree. >>> >>> The KvmtoolPlatformDxe performs the platform >>> actions like determining if the firmware should >>> expose ACPI or the Device Tree based hardware >>> description to the operating system. >>> >>> Signed-off-by: Sami Mujawar >>> --- >>> >>> Notes: >>> =C2=A0=C2=A0=C2=A0=C2=A0 v2: >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 - Updated according to review c= omments.=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 [Sami] >>> =C2=A0=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0 v1: >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 - Add kvmtool platform driver t= o support loading platform=C2=A0=C2=A0 [Sami] >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 specific informatio= n. >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 - Keep code to initialise the v= ariable storage PCDs in the >>> [Laszlo] >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 platform-specific F= VB driver. >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 - Document code derived from >>> [Laszlo] >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 "ArmVirtPkg/Platfor= mHasAcpiDtDxe" >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Ref: https://edk2.g= roups.io/g/devel/topic/30915278#30757 >>> >>> =C2=A0 ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.c=C2=A0=C2=A0= | 93 >>> ++++++++++++++++++++ >>> =C2=A0 ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.inf | 47 +++= +++++++ >>> =C2=A0 2 files changed, 140 insertions(+) >>> >>> diff --git a/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.c >>> b/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.c >>> new file mode 100644 >>> index >>> 0000000000000000000000000000000000000000..e7568f66f5ebeb0423fc1c10345= cd8dad0800d94 >>> >>> --- /dev/null >>> +++ b/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.c >>> @@ -0,0 +1,93 @@ >>> +/** @file >>> + >>> +=C2=A0 The KvmtoolPlatformDxe performs the platform specific >>> initialization like: >>> +=C2=A0 - It decides if the firmware should expose ACPI or Device Tre= e-based >>> +=C2=A0=C2=A0=C2=A0 hardware description to the operating system. >>> + >>> +=C2=A0 Copyright (c) 2018 - 2020, ARM Limited. All rights reserved. >>> + >>> +=C2=A0 SPDX-License-Identifier: BSD-2-Clause-Patent >>> + >>> +**/ >>> + >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +/** Decide if the firmware should expose ACPI tables or Device Tree = and >>> +=C2=A0=C2=A0=C2=A0 install the appropriate protocol interface. >>> + >>> +=C2=A0 Note: This function is derived from "ArmVirtPkg/PlatformHasAc= piDtDxe", >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 by dropping the word size= check, and the fw_cfg check. >>> + >>> +=C2=A0 @param [in]=C2=A0 ImageHandle=C2=A0 Handle for this image. >>> + >>> +=C2=A0 @retval EFI_SUCCESS=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Success. >>> +=C2=A0 @retval EFI_OUT_OF_RESOURCES=C2=A0=C2=A0=C2=A0 There was not = enough memory to >>> install the >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 protocols. >>> +=C2=A0 @retval EFI_INVALID_PARAMETER=C2=A0=C2=A0 A parameter is inva= lid. >>> + >>> +**/ >>> +STATIC >>> +EFI_STATUS >>> +PlatformHasAcpiDt ( >>> +=C2=A0 IN EFI_HANDLE=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0 ImageHandle >>> +=C2=A0 ) >>> +{ >>> +=C2=A0 if (!PcdGetBool (PcdForceNoAcpi)) { >>> +=C2=A0=C2=A0=C2=A0 // Expose ACPI tables >>> +=C2=A0=C2=A0=C2=A0 return gBS->InstallProtocolInterface ( >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 &ImageHandle, >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 &gEdkiiPlatformHasAcpiGuid, >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 EFI_NATIVE_INTERFACE, >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 NULL >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ); >>> +=C2=A0 } >>> + >>> +=C2=A0 // Expose the Device Tree. >>> +=C2=A0 return gBS->InstallProtocolInterface ( >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0 &ImageHandle, >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0 &gEdkiiPlatformHasDeviceTreeGuid, >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0 EFI_NATIVE_INTERFACE, >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0 NULL >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0 ); >>> +} >>> + >>> +/** Entry point for Kvmtool Platform Dxe >>> + >>> +=C2=A0 @param [in]=C2=A0 ImageHandle=C2=A0 Handle for this image. >>> +=C2=A0 @param [in]=C2=A0 SystemTable=C2=A0 Pointer to the EFI system= table. >>> + >>> +=C2=A0 @retval EFI_SUCCESS=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Success. >>> +=C2=A0 @retval EFI_OUT_OF_RESOURCES=C2=A0=C2=A0=C2=A0 There was not = enough memory to >>> install the >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 protocols. >>> +=C2=A0 @retval EFI_INVALID_PARAMETER=C2=A0=C2=A0 A parameter is inva= lid. >>> + >>> +**/ >>> +EFI_STATUS >>> +EFIAPI >>> +KvmtoolPlatformDxeEntryPoint ( >>> +=C2=A0 IN EFI_HANDLE=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0 ImageHandle, >>> +=C2=A0 IN EFI_SYSTEM_TABLE=C2=A0=C2=A0=C2=A0=C2=A0 *SystemTable >>> +=C2=A0 ) >>> +{ >>> +=C2=A0 EFI_STATUS=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Sta= tus; >>> + >>> +=C2=A0 Status =3D PlatformHasAcpiDt (ImageHandle); >>> +=C2=A0 if (EFI_ERROR (Status)) { >>> +=C2=A0=C2=A0=C2=A0 goto Failed; >>> +=C2=A0 } >>> + >>> +=C2=A0 return Status; >>> + >>> +Failed: >>> +=C2=A0 ASSERT_EFI_ERROR (Status); >>> +=C2=A0 CpuDeadLoop (); >>> + >>> +=C2=A0 return Status; >>> +} >> >> Please don't use CpuDeadLoop()s in your drivers. >> >> Installing a protocol on an image handle like this should not ever fai= l, >> and if it does, it is unlikely to be an issue in the driver itself. So >> just use ASSERT_EFI_ERROR() here, and return EFI_SUCCESS. >=20 > I think Sami just followed the original code in > "ArmVirtPkg/PlatformHasAcpiDtDxe". >=20 > I'm fine either way: >=20 > Acked-by: Laszlo Ersek >=20 > Different question: >=20 > Should we ask Sami to become a designated reviewer (in Maintainers.txt) > for the kvmtool-specific modules under ArmVirtPkg? Personally I'm > unlikely to use kvmtool. >=20 Not sure if you saw patch 11/11, but I agree that in general, but for=20 ArmVirtPkg in particular as well, having package level maintainers is=20 sufficient, and there is no need for maintainer roles beyond that. But let's discuss this in reply to 11/11