From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-vk1-f174.google.com (mail-vk1-f174.google.com [209.85.221.174]) by mx.groups.io with SMTP id smtpd.web09.12171.1631293719333055867 for ; Fri, 10 Sep 2021 10:08:39 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20210112 header.b=TEZpyl2C; spf=pass (domain: gmail.com, ip: 209.85.221.174, mailfrom: pedro.falcato@gmail.com) Received: by mail-vk1-f174.google.com with SMTP id t19so877412vkk.2 for ; Fri, 10 Sep 2021 10:08:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=6kVXf2HgHKZOdKDEjk61iodB6yUoK3D4hvq6K1uzLY4=; b=TEZpyl2CvGm64r1jWe9J/xfLW2v24DHTLlhrwzQ9u29FV6rR9sBm/KGkx0N0/kjEag 5z+IykIq7pgNBwN+S8w28n98EXjvFasE2mCApwoQTcU5mh+tEQoJ7bCfFAYJ0XQpLAWe F/vaEwpGZlSY14zzXE9BH+yUdzcynqoV2CY50Yp4ji9LSVXCqRBqUYP8f0upyjWiLXt2 3eT+Vx6XEjszOUxSZrw5hG85zOspGYlXGgGxj2un9iRxRqgXfO4XpznSKAb0JSgyPfcO uqabZ/othFjOdczQVUcTb65Br03fxcbXtb/7fcHpJEz5oHyFwswZgy2sB4YnR2rvg7CO ETxg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=6kVXf2HgHKZOdKDEjk61iodB6yUoK3D4hvq6K1uzLY4=; b=hbYEXzs6zy687d86mTml6qHcLWZkMFuvBSJJpEt2yGWL6cgMFz+0h9HkLNg4l/xbLg MVDEMtHAvEuwGX+LNZeTptnKUYtAJf5dS9b9McY2vzhv6XJRtno25pgTy7RRsKHZ6Ov+ 0Hb5KiIIzUZvhqMOuSVpHdBvOS4FftdwGdbacUkbJlloF70u5rNRkxgxNUhjSBGIHer0 LxYaW7HahRx4kVJKs+Qvv8VcdleqsP5Kwe6cCZOskJvdIJI3Gfzb5J6nUUbz/EkZRMDy 34Ve5zk7VdOsRGqFwtlPb/e9iEGEfpTldLsUbd6rKoj81RNNZ2Z+o8a21kadFhylRCYW JkRA== X-Gm-Message-State: AOAM532EdRQqUU+8U/fQRqvRO7pRoe/DXboNa7SoRiBqdka5jMBlfa9D d5daDtSv/7Fprk8Qqki8VXT+WUjajWItP3Q6GQo= X-Google-Smtp-Source: ABdhPJzEfgE7WPIJqN5HXBlPVnyzZf/LD5P4AtqLwTqKIXUhWbr3mwwJjumulMXIYLkUSF4kc1yKs/ooHbDUwjmBlbE= X-Received: by 2002:a1f:9815:: with SMTP id a21mr5420850vke.25.1631293717363; Fri, 10 Sep 2021 10:08:37 -0700 (PDT) MIME-Version: 1.0 References: <00f58fdf6a9bce566ad5d145b0449ef3e6fdd94e.1631289393.git.jbrasen@nvidia.com> <45555d82-b6ab-da53-7662-bcbeab1ad6ce@posteo.de> In-Reply-To: <45555d82-b6ab-da53-7662-bcbeab1ad6ce@posteo.de> From: "Pedro Falcato" Date: Fri, 10 Sep 2021 18:08:26 +0100 Message-ID: Subject: Re: [edk2-devel] [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior To: =?UTF-8?Q?Marvin_H=C3=A4user?= Cc: edk2-devel-groups-io , Jeff Brasen Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Ah yes, thanks! Although I didn't find the part where they say that you need to use the same attributes, after re-reading the spec it seems they recommend using BY_DRIVER. So, change it to BY_DRIVER. The performance should be the same and it's more consistent. On Fri, Sep 10, 2021 at 5:56 PM Marvin H=C3=A4user wro= te: > > On 10/09/2021 18:52, Pedro Falcato wrote: > > Like Marvin raised in v1, it's a good idea to change the first > > OpenProtocol's EFI_OPEN_PROTOCOL_BY_DRIVER > > into EFI_OPEN_PROTOCOL_GET_PROTOCOL, for consistency's sake. > > Since we're not keeping these protocols open, using BY_DRIVER makes no > > difference. > > No, the UEFI spec demands Supported() to use the same Attributes as > Start() (likely for these very performance reasons), actually I thought > both need BY_DRIVER. > > Best regards, > Marvin > > > You didn't update Ext4SuperblockCheckMagic's comment in Superblock.c > > when you changed it in the header. > > Just a nitpick, but if you could quickly align the > > Ext4SuperblockCheckMagic's parameters' descriptions like > > > > @param[in] DiskIo Pointer to the DiskIo. > > @param[in] BlockIo Pointer to the BlockIo. > > > > it would be excellent. > > > > Apart from that, looks great to me and the patch series will get my RB > > after that quick change. > > > > Best regards, > > > > Pedro > > > > On Fri, Sep 10, 2021 at 4:58 PM Jeff Brasen wrote: > >> A couple of improvements to improve performance. > >> Add check to return ACCESS_DENIED if already connected > >> Add check to verify superblock magic during supported to reduce start = calls > >> > >> Signed-off-by: Jeff Brasen > >> --- > >> Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h | 14 +++++++ > >> Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c | 58 +++++++++++++++++++++----= -- > >> Features/Ext4Pkg/Ext4Dxe/Superblock.c | 35 ++++++++++++++++ > >> 3 files changed, 95 insertions(+), 12 deletions(-) > >> > >> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext= 4Dxe/Ext4Dxe.h > >> index 64eab455db..5c60860894 100644 > >> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h > >> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h > >> @@ -1117,4 +1117,18 @@ Ext4GetVolumeName ( > >> OUT UINTN *VolNameLen > >> ); > >> > >> +/** > >> + Checks the superblock's magic value. > >> + > >> + @param[in] DiskIo Pointer to the DiskIo. > >> + @param[in] BlockIo Pointer to the BlockIo. > >> + > >> + @return TRUE if a valid ext4 superblock, else FALSE. > >> +**/ > >> +BOOLEAN > >> +Ext4SuperblockCheckMagic ( > >> + IN EFI_DISK_IO_PROTOCOL *DiskIo, > >> + IN EFI_BLOCK_IO_PROTOCOL *BlockIo > >> + ); > >> + > >> #endif > >> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c b/Features/Ext4Pkg/Ext= 4Dxe/Ext4Dxe.c > >> index ea2e048d77..5ae93d0484 100644 > >> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c > >> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c > >> @@ -631,7 +631,6 @@ Ext4Unload ( > >> @retval EFI_ACCESS_DENIED The device specified by Controlle= rHandle and > >> RemainingDevicePath is already be= ing managed by a different > >> driver or an application that req= uires exclusive access. > >> - Currently not implemented. > >> @retval EFI_UNSUPPORTED The device specified by Controlle= rHandle and > >> RemainingDevicePath is not suppor= ted by the driver specified by This. > >> **/ > >> @@ -643,32 +642,67 @@ Ext4IsBindingSupported ( > >> IN EFI_DEVICE_PATH *RemainingDevicePath OPTIONAL > >> ) > >> { > >> - // Note to self: EFI_OPEN_PROTOCOL_TEST_PROTOCOL lets us not close = the > >> - // protocol and ignore the output argument entirely > >> + EFI_STATUS Status; > >> + EFI_DISK_IO_PROTOCOL *DiskIo; > >> + EFI_BLOCK_IO_PROTOCOL *BlockIo; > >> > >> - EFI_STATUS Status; > >> + DiskIo =3D NULL; > >> + BlockIo =3D NULL; > >> > >> + // > >> + // Open the IO Abstraction(s) needed to perform the supported test > >> + // > >> Status =3D gBS->OpenProtocol ( > >> ControllerHandle, > >> &gEfiDiskIoProtocolGuid, > >> - NULL, > >> - BindingProtocol->ImageHandle, > >> + (VOID **) &DiskIo, > >> + BindingProtocol->DriverBindingHandle, > >> ControllerHandle, > >> - EFI_OPEN_PROTOCOL_TEST_PROTOCOL > >> + EFI_OPEN_PROTOCOL_BY_DRIVER > >> ); > >> > >> if (EFI_ERROR (Status)) { > >> - return Status; > >> + goto Exit; > >> } > >> - > >> + // > >> + // Open the IO Abstraction(s) needed to perform the supported test > >> + // > >> Status =3D gBS->OpenProtocol ( > >> ControllerHandle, > >> &gEfiBlockIoProtocolGuid, > >> - NULL, > >> - BindingProtocol->ImageHandle, > >> + (VOID **) &BlockIo, > >> + BindingProtocol->DriverBindingHandle, > >> ControllerHandle, > >> - EFI_OPEN_PROTOCOL_TEST_PROTOCOL > >> + EFI_OPEN_PROTOCOL_GET_PROTOCOL > >> ); > >> + if (EFI_ERROR (Status)) { > >> + goto Exit; > >> + } > >> + > >> + if (!Ext4SuperblockCheckMagic (DiskIo, BlockIo)) { > >> + Status =3D EFI_UNSUPPORTED; > >> + } > >> + > >> +Exit: > >> + // > >> + // Close the I/O Abstraction(s) used to perform the supported test > >> + // > >> + if (DiskIo !=3D NULL) { > >> + gBS->CloseProtocol ( > >> + ControllerHandle, > >> + &gEfiDiskIoProtocolGuid, > >> + BindingProtocol->DriverBindingHandle, > >> + ControllerHandle > >> + ); > >> + } > >> + if (BlockIo !=3D NULL) { > >> + gBS->CloseProtocol ( > >> + ControllerHandle, > >> + &gEfiBlockIoProtocolGuid, > >> + BindingProtocol->DriverBindingHandle, > >> + ControllerHandle > >> + ); > >> + } > >> return Status; > >> } > >> > >> diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/= Ext4Dxe/Superblock.c > >> index c321d8c3d8..1ceb0d5cbb 100644 > >> --- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c > >> +++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c > >> @@ -34,6 +34,41 @@ STATIC CONST UINT32 gSupportedIncompatFeat =3D > >> // this is desired, it's fairly trivial to look for EFI_VOLUME_CORRU= PTED > >> // references and add some Ext4SignalCorruption function + function = call. > >> > >> +/** > >> + Checks only superblock magic value. > >> + > >> + @param[in] DiskIo Pointer to the DiskIo. > >> + @param[in] BlockIo Pointer to the BlockIo. > >> + > >> + @return TRUE if a valid ext4 superblock, else FALSE. > >> +**/ > >> +BOOLEAN > >> +Ext4SuperblockCheckMagic ( > >> + IN EFI_DISK_IO_PROTOCOL *DiskIo, > >> + IN EFI_BLOCK_IO_PROTOCOL *BlockIo > >> + ) > >> +{ > >> + UINT16 Magic; > >> + EFI_STATUS Status; > >> + > >> + Status =3D DiskIo->ReadDisk ( > >> + DiskIo, > >> + BlockIo->Media->MediaId, > >> + EXT4_SUPERBLOCK_OFFSET + OFFSET_OF (EXT4_SUPERBL= OCK, s_magic), > >> + sizeof (Magic), > >> + &Magic > >> + ); > >> + if (EFI_ERROR (Status)) { > >> + return FALSE; > >> + } > >> + > >> + if (Magic !=3D EXT4_SIGNATURE) { > >> + return FALSE; > >> + } > >> + > >> + return TRUE; > >> +} > >> + > >> /** > >> Does brief validation of the ext4 superblock. > >> > >> -- > >> 2.17.1 > >> > > > --=20 Pedro Falcato