From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web12.179.1589472349478045788 for ; Thu, 14 May 2020 09:05:49 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=M2H8wIs+; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1589472348; 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=WNM+2YkyFJBbpV+jWP05xFTZv5ShIO9vuV2KfAfvjLE=; b=M2H8wIs+WiZ03kU7hXT9mZ0yVdR3uaTQcT8bvoADGuPnxhy/mSWtadCZ2HwxIFe9zm9hV/ WUSEN9VaJmwcJgD1EtweQRbsS6Gyo3H0mIh131kUS1b6WOehHZOdGIKcUbl6bqUxWXMJe2 Sb+b7hKQ8TTGO59ugOQdcWdbmdSQtuY= 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-217-ZlM2rbRaN5CGS0X0PsTK7g-1; Thu, 14 May 2020 12:05:36 -0400 X-MC-Unique: ZlM2rbRaN5CGS0X0PsTK7g-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 AC203107ACCD; Thu, 14 May 2020 16:05:34 +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 9B7915C1BE; Thu, 14 May 2020 16:05:32 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 06/11] ArmVirtPkg: Add kvmtool platform driver To: Ard Biesheuvel , 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: "Laszlo Ersek" Message-ID: Date: Thu, 14 May 2020 18:05:31 +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.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: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: >>>>       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. >> > > 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. 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-test kvmtool stuff. Thanks Laszlo