From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web10.10976.1589458864444737110 for ; Thu, 14 May 2020 05:21:04 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Vnsg1n97; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1589458863; h=from:from: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=rLfd/29lDR9YVokrrAzsFZFdEH99kDaT5WQeCSoI6Wk=; b=Vnsg1n97TFg8l1amFsV8lGVwS2VfDSn6Hvn7l7w5le3geUqZ2zZgIbJpJM5CQx4FxLUVE7 eKxhxrpRj0EgbdDMbynT4fiHC8TZKw68lLk0x5NzsZJNPGl9qmFJIL2ZnAyvM9U6XrISyL okO+mGZm++SRSd+CoKxW78wPx1V6Brw= 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-378-Jr9R8bocNk--YzDHrXE0vw-1; Thu, 14 May 2020 08:20:59 -0400 X-MC-Unique: Jr9R8bocNk--YzDHrXE0vw-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id F34EF1902EA4; Thu, 14 May 2020 12:20:57 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-179.ams2.redhat.com [10.36.113.179]) by smtp.corp.redhat.com (Postfix) with ESMTP id BA9DE5C1D3; Thu, 14 May 2020 12:20:55 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 06/11] ArmVirtPkg: Add kvmtool platform driver From: "Laszlo Ersek" To: devel@edk2.groups.io, ard.biesheuvel@arm.com, 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> Message-ID: Date: Thu, 14 May 2020 14:20:54 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <71feb4af-94a7-defe-c978-5a1775cc7665@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit On 05/14/20 14:12, 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: >>>      v2: >>>        - Updated according to review comments.                     [Sami] >>>           v1: >>>        - Add kvmtool platform driver to support loading platform   [Sami] >>>          specific information. >>>        - Keep code to initialise the variable storage PCDs in the  >>> [Laszlo] >>>          platform-specific FVB driver. >>>        - Document code derived from                                >>> [Laszlo] >>>          "ArmVirtPkg/PlatformHasAcpiDtDxe" >>>          Ref: https://edk2.groups.io/g/devel/topic/30915278#30757 >>> >>>   ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.c   | 93 >>> ++++++++++++++++++++ >>>   ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.inf | 47 ++++++++++ >>>   2 files changed, 140 insertions(+) >>> >>> diff --git a/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.c >>> b/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.c >>> new file mode 100644 >>> index >>> 0000000000000000000000000000000000000000..e7568f66f5ebeb0423fc1c10345cd8dad0800d94 >>> >>> --- /dev/null >>> +++ b/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.c >>> @@ -0,0 +1,93 @@ >>> +/** @file >>> + >>> +  The KvmtoolPlatformDxe performs the platform specific >>> initialization like: >>> +  - It decides if the firmware should expose ACPI or Device Tree-based >>> +    hardware description to the operating system. >>> + >>> +  Copyright (c) 2018 - 2020, ARM Limited. All rights reserved. >>> + >>> +  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 >>> +    install the appropriate protocol interface. >>> + >>> +  Note: This function is derived from "ArmVirtPkg/PlatformHasAcpiDtDxe", >>> +        by dropping the word size check, and the fw_cfg check. >>> + >>> +  @param [in]  ImageHandle  Handle for this image. >>> + >>> +  @retval EFI_SUCCESS             Success. >>> +  @retval EFI_OUT_OF_RESOURCES    There was not enough memory to >>> install the >>> +                                  protocols. >>> +  @retval EFI_INVALID_PARAMETER   A parameter is invalid. >>> + >>> +**/ >>> +STATIC >>> +EFI_STATUS >>> +PlatformHasAcpiDt ( >>> +  IN EFI_HANDLE           ImageHandle >>> +  ) >>> +{ >>> +  if (!PcdGetBool (PcdForceNoAcpi)) { >>> +    // Expose ACPI tables >>> +    return gBS->InstallProtocolInterface ( >>> +                  &ImageHandle, >>> +                  &gEdkiiPlatformHasAcpiGuid, >>> +                  EFI_NATIVE_INTERFACE, >>> +                  NULL >>> +                  ); >>> +  } >>> + >>> +  // Expose the Device Tree. >>> +  return gBS->InstallProtocolInterface ( >>> +                &ImageHandle, >>> +                &gEdkiiPlatformHasDeviceTreeGuid, >>> +                EFI_NATIVE_INTERFACE, >>> +                NULL >>> +                ); >>> +} >>> + >>> +/** Entry point for Kvmtool Platform Dxe >>> + >>> +  @param [in]  ImageHandle  Handle for this image. >>> +  @param [in]  SystemTable  Pointer to the EFI system table. >>> + >>> +  @retval EFI_SUCCESS             Success. >>> +  @retval EFI_OUT_OF_RESOURCES    There was not enough memory to >>> install the >>> +                                  protocols. >>> +  @retval EFI_INVALID_PARAMETER   A parameter is invalid. >>> + >>> +**/ >>> +EFI_STATUS >>> +EFIAPI >>> +KvmtoolPlatformDxeEntryPoint ( >>> +  IN EFI_HANDLE           ImageHandle, >>> +  IN EFI_SYSTEM_TABLE     *SystemTable >>> +  ) >>> +{ >>> +  EFI_STATUS                     Status; >>> + >>> +  Status = PlatformHasAcpiDt (ImageHandle); >>> +  if (EFI_ERROR (Status)) { >>> +    goto Failed; >>> +  } >>> + >>> +  return Status; >>> + >>> +Failed: >>> +  ASSERT_EFI_ERROR (Status); >>> +  CpuDeadLoop (); >>> + >>> +  return Status; >>> +} >> >> Please don't use CpuDeadLoop()s in your drivers. >> >> Installing a protocol on an image handle like this should not ever fail, >> 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.txt) > for the kvmtool-specific modules under ArmVirtPkg? Personally I'm > unlikely to use kvmtool. Apologies -- I now see that I had requested that under v1 (in October 2018... a long time ago), and I've also found the last patch now. (I guess I thought of documenting the new modules in Maintainers.txt in the same patches that added the new modules, but end-of-series is fine too.) Thanks! Laszlo > > Thanks > Laszlo > >> >> >>> diff --git a/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.inf >>> b/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.inf >>> new file mode 100644 >>> index >>> 0000000000000000000000000000000000000000..08a0fe5ce14469133479046385bdd48c22698639 >>> >>> --- /dev/null >>> +++ b/ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.inf >>> @@ -0,0 +1,47 @@ >>> +#/** @file >>> +# >>> +#  The KvmtoolPlatformDxe performs the platform specific >>> initialization like: >>> +#  - It decides if the firmware should expose ACPI or Device Tree-based >>> +#    hardware description to the operating system. >>> +# >>> +#  Copyright (c) 2018 - 2020, ARM Limited. All rights reserved. >>> +# >>> +#  SPDX-License-Identifier: BSD-2-Clause-Patent >>> +# >>> +#**/ >>> + >>> +[Defines] >>> +  INF_VERSION                    = 0x0001001B >>> +  BASE_NAME                      = KvmtoolPlatformDxe >>> +  FILE_GUID                      = 7479CCCD-D721-442A-8C73-A72DBB886669 >>> +  MODULE_TYPE                    = DXE_DRIVER >>> +  VERSION_STRING                 = 1.0 >>> +  ENTRY_POINT                    = KvmtoolPlatformDxeEntryPoint >>> + >>> +[Sources] >>> +  KvmtoolPlatformDxe.c >>> + >>> +[Packages] >>> +  ArmVirtPkg/ArmVirtPkg.dec >>> +  EmbeddedPkg/EmbeddedPkg.dec >>> +  MdePkg/MdePkg.dec >>> +  MdeModulePkg/MdeModulePkg.dec >>> + >>> +[LibraryClasses] >>> +  BaseLib >>> +  BaseMemoryLib >>> +  DebugLib >>> +  DxeServicesTableLib >>> +  MemoryAllocationLib >>> +  UefiBootServicesTableLib >>> +  UefiDriverEntryPoint >>> + >>> +[Guids] >>> +  gEdkiiPlatformHasAcpiGuid       ## SOMETIMES_PRODUCES ## PROTOCOL >>> +  gEdkiiPlatformHasDeviceTreeGuid ## SOMETIMES_PRODUCES ## PROTOCOL >>> + >>> +[Pcd] >>> +  gArmVirtTokenSpaceGuid.PcdForceNoAcpi >>> + >>> +[Depex] >>> +  TRUE >>> >> >> >> >> >