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.10859.1589458382895346351 for ; Thu, 14 May 2020 05:13:03 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=cQ6bJwu1; 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=1589458382; 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=5337o0JsVZQwhDoatgUjrm4PTxRi1m2MVybT27gKw5E=; b=cQ6bJwu1UTRznobunap6dJTRiqJ1FUi+w0rTx3zWKxCSfTeWUlZPbA0UJK0GqrS/W9QoYU XFUtWev02CnZg40/DAoAdryFSbuprPfjYfHCOjmidxzFGR70k2fNIG8RhsRhHoOih4co04 J+Mgg+5rd1XJJYjxp4Ndflx4yegHi6M= 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-260-BTplTWMPNUahkMhC7Ts86w-1; Thu, 14 May 2020 08:12:55 -0400 X-MC-Unique: BTplTWMPNUahkMhC7Ts86w-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id C099B1054F90; Thu, 14 May 2020 12:12:53 +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 B06B162B1E; Thu, 14 May 2020 12:12:51 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 06/11] ArmVirtPkg: Add kvmtool platform driver 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> From: "Laszlo Ersek" Message-ID: <71feb4af-94a7-defe-c978-5a1775cc7665@redhat.com> Date: Thu, 14 May 2020 14:12:50 +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: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 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 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. 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 >> > > > >