From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=217.140.101.70; helo=foss.arm.com; envelope-from=thomas.abraham@arm.com; receiver=edk2-devel@lists.01.org Received: from foss.arm.com (foss.arm.com [217.140.101.70]) by ml01.01.org (Postfix) with ESMTP id 246982063D743 for ; Tue, 22 May 2018 21:09:13 -0700 (PDT) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id AE856165D for ; Tue, 22 May 2018 21:09:13 -0700 (PDT) Received: from mail-it0-f45.google.com (usa-sjc-mx-foss1.foss.arm.com [217.140.101.70]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 8DF843F7B7 for ; Tue, 22 May 2018 21:09:13 -0700 (PDT) Received: by mail-it0-f45.google.com with SMTP id 144-v6so2688517iti.5 for ; Tue, 22 May 2018 21:09:13 -0700 (PDT) X-Gm-Message-State: ALKqPwdlDpuch4zSMpVg6JrkV3EJt3m/lnAsA/DCdO8F1OGIqhx6wqZs okKXQM0/OmFwhDkDl0rskuR9IQbwPicVpeJ5X4k= X-Google-Smtp-Source: AB8JxZqzIEdnbECW6gkshgy8KQ9iudVHbl+pGqrx6Ks5ZjJTNcF0UirrCSlihPzgg3xzHbPuNCviGc3s9eNHwE2cHf4= X-Received: by 2002:a24:af45:: with SMTP id l5-v6mr3123150iti.106.1527048552857; Tue, 22 May 2018 21:09:12 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a02:97b8:0:0:0:0:0 with HTTP; Tue, 22 May 2018 21:09:12 -0700 (PDT) In-Reply-To: References: <1526891152-18739-1-git-send-email-thomas.abraham@arm.com> <1526891152-18739-5-git-send-email-thomas.abraham@arm.com> From: Thomas Abraham Date: Wed, 23 May 2018 09:39:12 +0530 X-Gmail-Original-Message-ID: Message-ID: To: Ard Biesheuvel Cc: Thomas Abraham , "edk2-devel@lists.01.org" , Leif Lindholm Subject: Re: [PATCH edk2-platforms v4 4/9] Platform/ARM/Sgi: add support for virtio block device X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 23 May 2018 04:09:14 -0000 Content-Type: text/plain; charset="UTF-8" On Mon, May 21, 2018 at 2:28 PM, Ard Biesheuvel wrote: > On 21 May 2018 at 10:25, Thomas Abraham wrote: >> From: Daniil Egranov >> >> Add the registration of the virtio block device. >> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Daniil Egranov >> Signed-off-by: Thomas Abraham >> --- >> .../ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c | 18 ++++- >> .../ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf | 1 + >> .../ARM/SgiPkg/Drivers/PlatformDxe/VirtioBlockIo.c | 76 ++++++++++++++++++++++ >> 3 files changed, 94 insertions(+), 1 deletion(-) >> create mode 100644 Platform/ARM/SgiPkg/Drivers/PlatformDxe/VirtioBlockIo.c >> >> diff --git a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c >> index eb26fde..fb1e390 100644 >> --- a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c >> +++ b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c >> @@ -15,11 +15,27 @@ >> #include >> >> EFI_STATUS >> +InitVirtioBlockIo ( >> + IN EFI_HANDLE ImageHandle >> +); >> + >> +EFI_STATUS >> EFIAPI >> ArmSgiPkgEntryPoint ( >> IN EFI_HANDLE ImageHandle, >> IN EFI_SYSTEM_TABLE *SystemTable >> ) >> { >> - return EFI_SUCCESS; >> + EFI_STATUS Status; >> + >> + // Install Virtio Block IO. >> + if (FeaturePcdGet (PcdVirtioSupported) == TRUE) { > > No need to compare booleans with TRUE or FALSE. > >> + Status = InitVirtioBlockIo (ImageHandle); >> + if (EFI_ERROR (Status)) { >> + DEBUG ((DEBUG_ERROR, "PlatformDxe: Failed to install Virtio Block IO\n")); >> + return Status; >> + } >> + } >> + >> + return Status; >> } >> diff --git a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf >> index dbe04c5..995f80d 100644 >> --- a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf >> +++ b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf >> @@ -20,6 +20,7 @@ >> >> [Sources.common] >> PlatformDxe.c >> + VirtioBlockIo.c >> >> [Packages] >> ArmPkg/ArmPkg.dec >> diff --git a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/VirtioBlockIo.c b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/VirtioBlockIo.c >> new file mode 100644 >> index 0000000..58dfd22 >> --- /dev/null >> +++ b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/VirtioBlockIo.c >> @@ -0,0 +1,76 @@ >> +/** @file >> + >> + Copyright (c) 2018, ARM Ltd. All rights reserved.
>> + >> + This program and the accompanying materials are licensed and made available >> + under the terms and conditions of the BSD License which accompanies this >> + distribution. The full text of the license may be found at >> + http://opensource.org/licenses/bsd-license.php >> + >> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >> + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. >> + >> +**/ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#pragma pack (1) >> +typedef struct { >> + VENDOR_DEVICE_PATH Vendor; >> + EFI_DEVICE_PATH_PROTOCOL End; >> +} VIRTIO_BLK_DEVICE_PATH; >> +#pragma pack () >> + >> +STATIC VIRTIO_BLK_DEVICE_PATH mVirtioBlockDevicePath = >> +{ >> + { >> + { >> + HARDWARE_DEVICE_PATH, >> + HW_VENDOR_DP, >> + { >> + (UINT8)( sizeof (VENDOR_DEVICE_PATH) ), >> + (UINT8)( (sizeof (VENDOR_DEVICE_PATH) ) >> 8) > > No spaces after ( or before ) please > >> + } >> + }, >> + EFI_CALLER_ID_GUID, >> + }, >> + { >> + END_DEVICE_PATH_TYPE, >> + END_ENTIRE_DEVICE_PATH_SUBTYPE, >> + { >> + sizeof (EFI_DEVICE_PATH_PROTOCOL), >> + 0 >> + } >> + } >> +}; >> + >> +/** >> + * Entrypoint for 'VirtioBlockIo' driver >> + */ >> +EFI_STATUS >> +InitVirtioBlockIo ( >> + IN EFI_HANDLE ImageHandle >> + ) >> +{ >> + EFI_STATUS Status = 0; >> + >> + Status = gBS->InstallProtocolInterface (&ImageHandle, >> + &gEfiDevicePathProtocolGuid, EFI_NATIVE_INTERFACE, >> + &mVirtioBlockDevicePath); >> + > > Please use correct indentation. > >> + if (EFI_ERROR (Status)) { >> + return Status; >> + } >> + >> + // Declare the Virtio BlockIo device >> + Status = VirtioMmioInstallDevice (SGI_EXP_SYSPH_VIRTIO_BLOCK_BASE, ImageHandle); >> + if (EFI_ERROR (Status)) { >> + DEBUG ((DEBUG_ERROR, "PlatformDxe: Failed to install Virtio block device\n")); >> + } >> + > > Please handle this error correctly. If the call to > VirtioMmioInstallDevice() fails, you will exit this function and hence > the DXE entry point with an error, which will result in the driver to > be unloaded. However, you just installed gEfiDevicePathProtocolGuid > with references to the driver's code, and attempts to invoke those > will result in a crash after this. Okay, thanks for pointing this out. This will be fixed in the next version. Thanks, Thomas. > >> + return Status; >> +} >> -- >> 2.7.4 >> > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel