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.1638.1589477123813236694 for ; Thu, 14 May 2020 10:25:23 -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 702C61042; Thu, 14 May 2020 10:25:22 -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 ECCD53F71E; Thu, 14 May 2020 10:25:19 -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: <19552590-40c3-99ef-73ca-0ece525a7af7@arm.com> Date: Thu, 14 May 2020 19:25:17 +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: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 5/14/20 6:05 PM, Laszlo Ersek wrote: > On 05/14/20 14:17, Ard Biesheuvel wrote: >> 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=C2=A0 v2: >>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 - Updated according to = review comments. >>>>> [Sami] >>>>> =C2=A0=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=C2=A0 - Add kvmtool platform = driver to support loading platform >>>>> [Sami] >>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 specific in= formation. >>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 - Keep code to initiali= se the variable storage PCDs in the >>>>> [Laszlo] >>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 platform-sp= ecific FVB driver. >>>>> =C2=A0=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=C2=A0 "ArmVirtPkg= /PlatformHasAcpiDtDxe" >>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Ref: https:= //edk2.groups.io/g/devel/topic/30915278#30757 >>>>> >>>>> =C2=A0=C2=A0 ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.c=C2= =A0=C2=A0 | 93 >>>>> ++++++++++++++++++++ >>>>> =C2=A0=C2=A0 ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.inf = | 47 ++++++++++ >>>>> =C2=A0=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..e7568f66f5ebeb0423fc1c103= 45cd8dad0800d94 >>>>> >>>>> >>>>> --- /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 T= ree-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 Tre= e >>>>> and >>>>> +=C2=A0=C2=A0=C2=A0 install the appropriate protocol interface. >>>>> + >>>>> +=C2=A0 Note: This function is derived from >>>>> "ArmVirtPkg/PlatformHasAcpiDtDxe", >>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 by dropping the word si= ze 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 no= t 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 in= valid. >>>>> + >>>>> +**/ >>>>> +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 syst= em 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 no= t 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 in= valid. >>>>> + >>>>> +**/ >>>>> +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 f= ail, >>>> 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. >>> >>> I think Sami just followed the original code in >>> "ArmVirtPkg/PlatformHasAcpiDtDxe". >>> >>> I'm fine either way: >>> >>> Acked-by: Laszlo Ersek >>> >>> Different question: >>> >>> Should we ask Sami to become a designated reviewer (in Maintainers.tx= t) >>> for the kvmtool-specific modules under ArmVirtPkg? Personally I'm >>> unlikely to use kvmtool. >>> >> >> Not sure if you saw patch 11/11, but I agree that in general, but for >> ArmVirtPkg in particular as well, having package level maintainers is >> sufficient, and there is no need for maintainer roles beyond that. >=20 > OK -- If you think patch#11 is unnecessary, I won't insist; I'd just > like to avoid the expectation for me to deeply review or regression-tes= t > kvmtool stuff. >=20 I don't think it is unnecessary. I think it should be clear who is in=20 charge of these files if it isn't the top-level maintainers of the packag= e. We just have to decide whether designated reviewer or maintainer is the=20 most appropriate. I am fine with either, and I am willing to share the=20 burden with Sami too.