From: "Ard Biesheuvel" <ard.biesheuvel@arm.com>
To: Laszlo Ersek <lersek@redhat.com>,
devel@edk2.groups.io, Sami Mujawar <sami.mujawar@arm.com>
Cc: leif@nuviainc.com, Alexandru.Elisei@arm.com,
Andre.Przywara@arm.com, Matteo.Carlini@arm.com,
Laura.Moretta@arm.com, nd@arm.com
Subject: Re: [edk2-devel] [PATCH v2 06/11] ArmVirtPkg: Add kvmtool platform driver
Date: Thu, 14 May 2020 14:17:11 +0200 [thread overview]
Message-ID: <b9485d8c-3194-f2e8-94bb-87f9ba0e4f5f@arm.com> (raw)
In-Reply-To: <71feb4af-94a7-defe-c978-5a1775cc7665@redhat.com>
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 <sami.mujawar@arm.com>
>>> ---
>>>
>>> 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 <Guid/VariableFormat.h>
>>> +#include <Library/BaseLib.h>
>>> +#include <Library/BaseMemoryLib.h>
>>> +#include <Library/DebugLib.h>
>>> +#include <Library/DxeServicesTableLib.h>
>>> +#include <Library/UefiBootServicesTableLib.h>
>>> +#include <Library/UefiDriverEntryPoint.h>
>>> +#include <Protocol/FdtClient.h>
>>> +
>>> +/** 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 <lersek@redhat.com>
>
> 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.
But let's discuss this in reply to 11/11
next prev parent reply other threads:[~2020-05-14 12:17 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-14 8:40 [PATCH v2 00/11] Kvmtool guest firmware support for Arm Sami Mujawar
2020-05-14 8:40 ` [PATCH v2 01/11] PcAtChipsetPkg: Add MMIO Support to RTC driver Sami Mujawar
2020-05-14 9:24 ` Ard Biesheuvel
2020-05-15 10:50 ` André Przywara
2020-05-27 0:37 ` [edk2-devel] " Guomin Jiang
2020-05-14 8:40 ` [PATCH v1 02/11] MdePkg: Add NULL implementation for PCILib Sami Mujawar
2020-05-14 9:23 ` Ard Biesheuvel
2020-05-14 16:21 ` Michael D Kinney
2020-05-14 8:40 ` [PATCH v1 03/11] MdePkg: Base Memory Lib instance using MMIO Sami Mujawar
2020-05-14 9:22 ` Ard Biesheuvel
2020-05-14 17:21 ` Ard Biesheuvel
2020-05-14 16:33 ` [edk2-devel] " Michael D Kinney
2020-05-14 8:40 ` [PATCH v1 04/11] ArmPlatformPkg: Use MMIO to read device memory Sami Mujawar
2020-05-14 8:40 ` [PATCH v1 05/11] ArmPlatformPkg: Dynamic flash variable base Sami Mujawar
2020-05-14 9:24 ` Ard Biesheuvel
2020-05-27 11:48 ` [edk2-devel] " Philippe Mathieu-Daudé
2020-05-14 8:40 ` [PATCH v2 06/11] ArmVirtPkg: Add kvmtool platform driver Sami Mujawar
2020-05-14 9:29 ` Ard Biesheuvel
2020-05-14 12:12 ` [edk2-devel] " Laszlo Ersek
2020-05-14 12:17 ` Ard Biesheuvel [this message]
2020-05-14 16:05 ` Laszlo Ersek
2020-05-14 17:25 ` Ard Biesheuvel
2020-05-15 7:28 ` Laszlo Ersek
2020-05-14 12:20 ` Laszlo Ersek
2020-05-14 8:40 ` [PATCH v1 07/11] ArmVirtPkg: kvmtool platform memory map Sami Mujawar
2020-05-14 9:30 ` Ard Biesheuvel
2020-05-14 12:15 ` [edk2-devel] " Laszlo Ersek
2020-05-14 8:40 ` [PATCH v1 08/11] ArmVirtPkg: Add Kvmtool NOR flash lib Sami Mujawar
2020-05-14 9:32 ` Ard Biesheuvel
2020-05-14 12:17 ` [edk2-devel] " Laszlo Ersek
2020-05-27 11:59 ` Philippe Mathieu-Daudé
2020-06-04 6:30 ` Philippe Mathieu-Daudé
2020-06-04 15:00 ` Sami Mujawar
2020-05-14 8:40 ` [PATCH v2 09/11] ArmVirtPkg: Support for kvmtool emulated platform Sami Mujawar
2020-05-14 9:56 ` Ard Biesheuvel
2020-05-14 12:24 ` [edk2-devel] " Laszlo Ersek
2020-05-14 8:40 ` [PATCH v1 10/11] ArmVirtPkg: Link NorFlashDxe with BaseMemoryLibMmio Sami Mujawar
2020-05-14 12:28 ` [edk2-devel] " Laszlo Ersek
2020-05-14 8:40 ` [PATCH v1 11/11] Maintainer.txt: Add Kvmtool emulated plat maintainer Sami Mujawar
2020-05-14 12:31 ` [edk2-devel] " Laszlo Ersek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b9485d8c-3194-f2e8-94bb-87f9ba0e4f5f@arm.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox