|
Re: [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior
Jeff Brasen
We actually can't open the BlockIo protocol as BY_DRIVER as it is already owned by the DiskIoDxe driver. Will upload a v3 with the other feedback though.
Thanks, Jeff
From: Marvin Häuser <mhaeuser@...>
Sent: Friday, September 10, 2021 12:09 PM To: Pedro Falcato <pedro.falcato@...> Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Jeff Brasen <jbrasen@...> Subject: Re: [edk2-devel] [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior External email: Use caution opening links or attachments
On 10/09/2021 19:08, Pedro Falcato wrote: > 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. UEFI 2.9, 11.1, "Device Driver", 2.: "It must use the same Attribute value that was used in Supported()." > So, change it to BY_DRIVER. The performance should be the same and > it's more consistent. Should be better because all handles opened by BY_DRIVER already will immediately fail. Best regards, Marvin > > On Fri, Sep 10, 2021 at 5:56 PM Marvin Häuser <mhaeuser@...> wrote: >> 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 <jbrasen@...> 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 <jbrasen@...> >>>> --- >>>> 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/Ext4Dxe/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/Ext4Dxe/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 ControllerHandle and >>>> RemainingDevicePath is already being managed by a different >>>> driver or an application that requires exclusive access. >>>> - Currently not implemented. >>>> @retval EFI_UNSUPPORTED The device specified by ControllerHandle and >>>> RemainingDevicePath is not supported 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 = NULL; >>>> + BlockIo = NULL; >>>> >>>> + // >>>> + // Open the IO Abstraction(s) needed to perform the supported test >>>> + // >>>> Status = 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 = 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 = EFI_UNSUPPORTED; >>>> + } >>>> + >>>> +Exit: >>>> + // >>>> + // Close the I/O Abstraction(s) used to perform the supported test >>>> + // >>>> + if (DiskIo != NULL) { >>>> + gBS->CloseProtocol ( >>>> + ControllerHandle, >>>> + &gEfiDiskIoProtocolGuid, >>>> + BindingProtocol->DriverBindingHandle, >>>> + ControllerHandle >>>> + ); >>>> + } >>>> + if (BlockIo != 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 = >>>> // this is desired, it's fairly trivial to look for EFI_VOLUME_CORRUPTED >>>> // 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 = DiskIo->ReadDisk ( >>>> + DiskIo, >>>> + BlockIo->Media->MediaId, >>>> + EXT4_SUPERBLOCK_OFFSET + OFFSET_OF (EXT4_SUPERBLOCK, s_magic), >>>> + sizeof (Magic), >>>> + &Magic >>>> + ); >>>> + if (EFI_ERROR (Status)) { >>>> + return FALSE; >>>> + } >>>> + >>>> + if (Magic != EXT4_SIGNATURE) { >>>> + return FALSE; >>>> + } >>>> + >>>> + return TRUE; >>>> +} >>>> + >>>> /** >>>> Does brief validation of the ext4 superblock. >>>> >>>> -- >>>> 2.17.1 >>>> >
|
|
|
|
Re: [PATCH v7 0/9] Ovmf: Disable the TPM2 platform hierarchy
Stefan Berger
On 9/10/21 12:15 PM, Stefan Berger wrote:
Before I post yet another series...: The problem is that PPI may require interaction with the console, so it seems we have to handle it in PlatformBootManagerAfterConsole(). The disablement of the TPM 2 platform hierarchy may only occur after that, so we have to move this part here after TPM-PPI-Handling from BeforeConsole() into AfterConsole() because this is what triggers that new code from edk2-platforms to disable that TPM 2 platform hierarchy: Status = gBS->InstallProtocolInterface (&Handle, &gEfiDxeSmmReadyToLockProtocolGuid, EFI_NATIVE_INTERFACE, NULL); And then we have this part here also in BeforeConsole() that has to be moved as well into AfterConsole(). // // Dispatch deferred images after EndOfDxe event and ReadyToLock // installation. // EfiBootManagerDispatchDeferredImages (); This then leads to something like this with the sequence (TPM-PPI-handling, gEfiDxeSmmReadyToLockProtocol, EfiBootManagerDispatchDeferredImages) needing to stay in that order. However, I am not sure know whether one can just move these parts around like this. diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c index 71f63b2448..266d58dfbe 100644 --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c @@ -354,7 +354,6 @@ PlatformBootManagerBeforeConsole ( VOID ) { - EFI_HANDLE Handle; EFI_STATUS Status; UINT16 FrontPageTimeout; RETURN_STATUS PcdStatus; @@ -387,8 +386,10 @@ PlatformBootManagerBeforeConsole ( SaveS3BootScript (); } +#if 0 // // Prevent further changes to LockBoxes or SMRAM. + // Any TPM 2 Physical Presence Interface opcode must be handled BEFORE // Handle = NULL; Status = gBS->InstallProtocolInterface (&Handle, @@ -401,6 +402,7 @@ PlatformBootManagerBeforeConsole ( // installation. // EfiBootManagerDispatchDeferredImages (); +#endif PlatformInitializeConsole ( XenDetected() ? gXenPlatformConsole : gPlatformConsole); @@ -437,6 +439,7 @@ PlatformBootManagerBeforeConsole ( // VisitAllInstancesOfProtocol (&gEfiPciIoProtocolGuid, ConnectVirtioPciRng, NULL); + } @@ -1474,6 +1477,8 @@ PlatformBootManagerAfterConsole ( VOID ) { + EFI_STATUS Status; + EFI_HANDLE Handle; EFI_BOOT_MODE BootMode; DEBUG ((DEBUG_INFO, "PlatformBootManagerAfterConsole\n")); @@ -1511,11 +1516,29 @@ PlatformBootManagerAfterConsole ( // PciAcpiInitialization (); +#if 1 // - // Process TPM PPI request + // Process TPM PPI request; this may require interaction via console // Tcg2PhysicalPresenceLibProcessRequest (NULL); + // + // Prevent further changes to LockBoxes or SMRAM. + // Any TPM 2 Physical Presence Interface opcode must be handled BEFORE + // + Handle = NULL; + Status = gBS->InstallProtocolInterface (&Handle, + &gEfiDxeSmmReadyToLockProtocolGuid, EFI_NATIVE_INTERFACE, + NULL); + ASSERT_EFI_ERROR (Status); + + // + // Dispatch deferred images after EndOfDxe event and ReadyToLock + // installation. + // + EfiBootManagerDispatchDeferredImages (); +#endif + // // Process QEMU's -kernel command line option // Stefan
|
|
|
|
Re: [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior
Marvin Häuser <mhaeuser@...>
On 10/09/2021 19:08, Pedro Falcato wrote:
Ah yes, thanks! Although I didn't find the part where they say thatUEFI 2.9, 11.1, "Device Driver", 2.: "It must use the same Attribute value that was used in Supported()." So, change it to BY_DRIVER. The performance should be the same andShould be better because all handles opened by BY_DRIVER already will immediately fail. Best regards, Marvin
|
|
|
|
Re: [edk2-libc Patch 1/1] edk2-libc/Readme.md: Updated Readme.md with Python 3.6.8 License details
Rebecca Cran <rebecca@...>
Pushed as ce35be387725cf6bb1caefa4d4b0bd48ea72cbfa.
toggle quoted messageShow quoted text
-- Rebecca Cran
On 9/8/21 10:54 AM, Jayaprakash Nevara wrote:
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3611
|
|
|
|
Re: [edk2-libc Patch 1/1] edk2-libc/Readme.md: Updated Readme.md with Python 3.6.8 License details
Rebecca Cran <rebecca@...>
Sorry for the delay -- I've been trying to find time to make sure I can follow the process correctly. I've just pushed the change as ce35be387725cf6bb1caefa4d4b0bd48ea72cbfa. For future patches, please add a Signed-off-by line.
--
On 9/9/21 12:12 AM, Jayaprakash, N
wrote:
Hello Rebecca, Would you be able to review and merge this change? Regards, JP -----Original Message----- From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Jayaprakash, N Sent: 08 September 2021 22:24 To: devel@edk2.groups.io Cc: Rebecca Cran <rebecca@...>; Kinney, Michael D <michael.d.kinney@...> Subject: [edk2-devel] [edk2-libc Patch 1/1] edk2-libc/Readme.md: Updated Readme.md with Python 3.6.8 License details REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3611 This commit contains updates made to the Readme.md file of edk2-libc repo to add the Python 3.6.8 license at appropriate section in the file Cc: Rebecca Cran <rebecca@...> Cc: Michael D Kinney <michael.d.kinney@...> --- Readme.md | 1 + 1 file changed, 1 insertion(+) diff --git a/Readme.md b/Readme.md index 341be14..0012cd5 100644 --- a/Readme.md +++ b/Readme.md @@ -24,6 +24,7 @@ contains the following components that are covered by additional licenses: * [AppPkg/Applications/Python/Python-2.7.2/Tools/pybench](AppPkg/Applications/Python/Python-2.7.2/Tools/pybench/LICENSE) * [AppPkg/Applications/Python/Python-2.7.2](AppPkg/Applications/Python/Python-2.7.2/LICENSE) * [AppPkg/Applications/Python/Python-2.7.10](AppPkg/Applications/Python/Python-2.7.10/LICENSE) +* +[AppPkg/Applications/Python/Python-3.6.8](AppPkg/Applications/Python/Py +thon-3.6.8/LICENSE) The EDK II LIBC Project is composed of packages. The maintainers for each package are listed in [Maintainers.txt](Maintainers.txt). -- 2.32.0.windows.2
|
|
|
|
TianoCore Community Meeting Minutes - September 2021
TianoCore Community Meeting
September 2, 2021
Events:
Google Summer of Code:
UEFI Plugfest update from Dick Wilkins:
Stable Tag updates:
Stewards Download:
Opens: none
--------------
Regards,
Soumya Guptha
TianoCore Community Manager
|
|
|
|
Re: [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior
Pedro Falcato
Ah yes, thanks! Although I didn't find the part where they say that
toggle quoted messageShow quoted text
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äuser <mhaeuser@...> wrote:
--
Pedro Falcato
|
|
|
|
Re: [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior
Jeff Brasen
Yeah we need by_driver on at least one protocol to prevent re-binding to the controller. It will return ALREADY_STARTED if this driver already has it open (from in Start) or ACCESS_DENIED if another driver is holding this.
I can covert both to by_driver if that is desired.
Will make the other little changes as well once I get a desired direction on how to open BlockIo.
Thanks, Jeff
From: Marvin Häuser <mhaeuser@...>
Sent: Friday, September 10, 2021 10:56 AM To: devel@edk2.groups.io <devel@edk2.groups.io>; pedro.falcato@... <pedro.falcato@...>; Jeff Brasen <jbrasen@...> Subject: Re: [edk2-devel] [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior External email: Use caution opening links or attachments
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 <jbrasen@...> 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 <jbrasen@...> >> --- >> 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/Ext4Dxe/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/Ext4Dxe/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 ControllerHandle and >> RemainingDevicePath is already being managed by a different >> driver or an application that requires exclusive access. >> - Currently not implemented. >> @retval EFI_UNSUPPORTED The device specified by ControllerHandle and >> RemainingDevicePath is not supported 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 = NULL; >> + BlockIo = NULL; >> >> + // >> + // Open the IO Abstraction(s) needed to perform the supported test >> + // >> Status = 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 = 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 = EFI_UNSUPPORTED; >> + } >> + >> +Exit: >> + // >> + // Close the I/O Abstraction(s) used to perform the supported test >> + // >> + if (DiskIo != NULL) { >> + gBS->CloseProtocol ( >> + ControllerHandle, >> + &gEfiDiskIoProtocolGuid, >> + BindingProtocol->DriverBindingHandle, >> + ControllerHandle >> + ); >> + } >> + if (BlockIo != 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 = >> // this is desired, it's fairly trivial to look for EFI_VOLUME_CORRUPTED >> // 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 = DiskIo->ReadDisk ( >> + DiskIo, >> + BlockIo->Media->MediaId, >> + EXT4_SUPERBLOCK_OFFSET + OFFSET_OF (EXT4_SUPERBLOCK, s_magic), >> + sizeof (Magic), >> + &Magic >> + ); >> + if (EFI_ERROR (Status)) { >> + return FALSE; >> + } >> + >> + if (Magic != EXT4_SIGNATURE) { >> + return FALSE; >> + } >> + >> + return TRUE; >> +} >> + >> /** >> Does brief validation of the ext4 superblock. >> >> -- >> 2.17.1 >> >
|
|
|
|
Re: [PATCH V5 1/2] OvmfPkg: Introduce Tdx BFV/CFV PCDs and PcdOvmfImageSizeInKb
Erdem Aktas
I have few naive questions. Sorry if the answers were obvious.
If I understand correctly, this means that the BFV is encrypted andTDVF also include a configuration firmware volume (CFV) that is separated measured during TD build time. Since CFV is not included in the MRTD, CFV region is not encrypted with the guest key, is it? Even if it is measured at runtime, the content needs to be copied toThe measurement value of the CFV (provisioned configuration data) is extended to somewhere else, otherwise what stops VMM to change the content after it is being measured (assuming that it is not encrypted). I am confused about how this memory is initialized. If it isAs to the spare part in varstore, it is not external input, is it? It's produced and consumed encrypted, then no need to measure it but also it becomes useless as the key will change in the next boot. If it is not encrypted, VMM can always modify the content and might cause unexpected behavior at runtime, right? I might be missing something here but if this region is not encrypted: - CFV content needs to be copied into an encrypted buffer after being measured and should never be used again. - Allowing variables to be stored in SPARE part seems like opening an attack surface as no one knows what will be stored in that region. Is this correct understanding? -Erdem On Wed, Sep 1, 2021 at 10:20 PM Andrew Fish <afish@...> wrote: On Sep 1, 2021, at 9:53 AM, James Bottomley <jejb@...> wrote:James,
|
|
|
|
Re: [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior
Marvin Häuser <mhaeuser@...>
On 10/09/2021 18:52, Pedro Falcato wrote:
Like Marvin raised in v1, it's a good idea to change the firstNo, 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
|
|
|
|
Re: [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior
Pedro Falcato
Like Marvin raised in v1, it's a good idea to change the first
toggle quoted messageShow quoted text
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. 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 <jbrasen@...> wrote:
--
Pedro Falcato
|
|
|
|
Re: [PATCH 1/2] Ext4Pkg: Improve Binding support behavior
Marvin Häuser <mhaeuser@...>
Good day,
On 09/09/2021 22:41, Jeff Brasen via groups.io wrote: A couple improvements to improve performance.This "performance improvement" actually aligns the load behaviour with the UEFI spec, maybe it should be mentioned? Add check to verify superblock magic during supported to deduce start callsThis change is not required as nothing was opened on failure; see below. }Why do the open modes mismatch if both protocols are equally stored in the private partition data? Is the owner of Block I/O a different one than the one of Disk I/O? );This can easily be rewritten to not require "goto": if (!EFI_ERROR (Status) && !Ext4SuperblockCheckMagic (DiskIo, BlockIo)) { Status = EFI_UNSUPPORTED; } With the change above this allows to drop the label and any "goto" code. +Protocols retrieved by GET_PROTOCOL are not closed. return Status;Whether it "only" checks the magic is more of a functional detail than an actual description. +Maybe use "@returns Whether the partition has a valid EXT4 superblock magic" for readability? Best regards, Marvin +**/
|
|
|
|
Re: [PATCH v7 0/9] Ovmf: Disable the TPM2 platform hierarchy
Stefan Berger
On 9/10/21 11:32 AM, Yao, Jiewen wrote:
According to the security policy, PP request must be processed before EndOfDxe.OVMF has 3 implementations invoking it in PlatformBootManagerAfterConsole(): https://github.com/tianocore/edk2/blob/master/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c#L1517 https://github.com/tianocore/edk2/blob/master/OvmfPkg/Library/PlatformBootManagerLibBhyve/BdsPlatform.c#L1451 https://github.com/tianocore/edk2/blob/master/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c#L1316 Stefan
|
|
|
|
[PATCH v2 2/2] Ext4Pkg: Support uncleanly unmounted filesystems
Jeff Brasen
Support for uncleanly mounted filesystems, if there is a recovery
journal mark filesystem as read-only. Signed-off-by: Jeff Brasen <jbrasen@...> --- Features/Ext4Pkg/Ext4Dxe/Superblock.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/Ext4Dxe/Superblock.c index 1ceb0d5cbb..251babb320 100644 --- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c +++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c @@ -18,7 +18,7 @@ STATIC CONST UINT32 gSupportedIncompatFeat = EXT4_FEATURE_INCOMPAT_64BIT | EXT4_FEATURE_INCOMPAT_DIRDATA | EXT4_FEATURE_INCOMPAT_FLEX_BG | EXT4_FEATURE_INCOMPAT_FILETYPE | EXT4_FEATURE_INCOMPAT_EXTENTS | EXT4_FEATURE_INCOMPAT_LARGEDIR | - EXT4_FEATURE_INCOMPAT_MMP; + EXT4_FEATURE_INCOMPAT_MMP | EXT4_FEATURE_INCOMPAT_RECOVER; // Future features that may be nice additions in the future: // 1) Btree support: Required for write support and would speed up lookups in large directories. @@ -89,10 +89,8 @@ Ext4SuperblockValidate ( return FALSE; } - // Is this correct behaviour? Imagine the power cuts out, should the system fail to boot because - // we're scared of touching something corrupt? if ((Sb->s_state & EXT4_FS_STATE_UNMOUNTED) == 0) { - return FALSE; + DEBUG ((DEBUG_WARN, "[ext4] Filesystem was not unmounted cleanly\n")); } return TRUE; @@ -215,6 +213,11 @@ Ext4OpenSuperblock ( return EFI_UNSUPPORTED; } + if (EXT4_HAS_INCOMPAT (Partition, EXT4_FEATURE_INCOMPAT_RECOVER)) { + DEBUG ((DEBUG_WARN, "[ext4] Needs journal recovery, mounting read-only\n")); + Partition->ReadOnly = TRUE; + } + // At the time of writing, it's the only supported checksum. if (Partition->FeaturesCompat & EXT4_FEATURE_RO_COMPAT_METADATA_CSUM && Sb->s_checksum_type != EXT4_CHECKSUM_CRC32C) { -- 2.17.1
|
|
|
|
[PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior
Jeff Brasen
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 <jbrasen@...> --- 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/Ext4Dxe/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/Ext4Dxe/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 ControllerHandle and RemainingDevicePath is already being managed by a different driver or an application that requires exclusive access. - Currently not implemented. @retval EFI_UNSUPPORTED The device specified by ControllerHandle and RemainingDevicePath is not supported 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 = NULL; + BlockIo = NULL; + // + // Open the IO Abstraction(s) needed to perform the supported test + // Status = 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 = 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 = EFI_UNSUPPORTED; + } + +Exit: + // + // Close the I/O Abstraction(s) used to perform the supported test + // + if (DiskIo != NULL) { + gBS->CloseProtocol ( + ControllerHandle, + &gEfiDiskIoProtocolGuid, + BindingProtocol->DriverBindingHandle, + ControllerHandle + ); + } + if (BlockIo != 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 = // this is desired, it's fairly trivial to look for EFI_VOLUME_CORRUPTED // 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 = DiskIo->ReadDisk ( + DiskIo, + BlockIo->Media->MediaId, + EXT4_SUPERBLOCK_OFFSET + OFFSET_OF (EXT4_SUPERBLOCK, s_magic), + sizeof (Magic), + &Magic + ); + if (EFI_ERROR (Status)) { + return FALSE; + } + + if (Magic != EXT4_SIGNATURE) { + return FALSE; + } + + return TRUE; +} + /** Does brief validation of the ext4 superblock. -- 2.17.1
|
|
|
|
[PATCH v2 0/2] ExtPkg Updates
Jeff Brasen
I have been using the new Ext4Pkg and been pretty successful and it is solving a use case we had.
Had a couple updates to propose 1. Changed the implementation of the binding protocol to both check if the driver is already bound to the partition as well as added a really quick check to validate the magic value in supported. This improves performance when you have a large number of non-ext4 partitions on the system. 2. As we are planning on using this for boot support we want to support unclean filesystem states in case the user doesn't reset cleanly. I added a check if the recovery journal is present and if so treat the filesystem as read-only (I know the driver is only RO at this point, but figured if you added write support prior to recovery journal support we would want that). With this everything seems to work great. I can add this under a FeaturePcd if desired as well. Change log v2 - Minor code review comments v1 - Initial revision Jeff Brasen (2): Ext4Pkg: Improve Ext4IsBindingSupported() behavior Ext4Pkg: Support uncleanly unmounted filesystems Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h | 14 +++++++ Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c | 58 +++++++++++++++++++++------ Features/Ext4Pkg/Ext4Dxe/Superblock.c | 46 +++++++++++++++++++-- 3 files changed, 102 insertions(+), 16 deletions(-) -- 2.17.1
|
|
|
|
Re: [PATCH v7 0/9] Ovmf: Disable the TPM2 platform hierarchy
Yao, Jiewen
According to the security policy, PP request must be processed before EndOfDxe.
toggle quoted messageShow quoted text
May I know when you trigger PP request? Thank you Yao Jiewen
-----Original Message-----
|
|
|
|
Re: [PATCH v7 0/9] Ovmf: Disable the TPM2 platform hierarchy
Stefan Berger
On 9/9/21 1:35 PM, Stefan Berger wrote:
This series imports code from the edk2-platforms project related to While disabling the platform hierarchy works, the unfortunate problem is now that the signal to disable the TPM 2 platform hierarchy is received before handling the physical presence interface (PPI) opcodes, which is bad because some of the opcodes will not go through. The question now is what is wrong? Are the PPI opcodes handled too late or the signal is sent to early or is it the wrong signal? Event = EfiCreateProtocolNotifyEvent ( &gEfiDxeSmmReadyToLockProtocolGuid, TPL_CALLBACK, SmmReadyToLockEventCallBack, NULL, &Registration ); Stefan
|
|
|
|
Re: [RFC] RISC-V QEMU virtual package
Ni, Ray
I asked similar question to Mike who initially set up the CI.
toggle quoted messageShow quoted text
The answer was: it's ok to pull a edk2-platform code in CI process to verify edk2 code change.
-----Original Message-----
|
|
|
|
Re: [PATCH] UefiPayloadPkg: Add script to build UniversalPayload in UefiPayloadPkg
Ni, Ray
+ Bob for general python language level review.
more comments embedded. +def BuildUniversalPayload(args, MacroList):1. Lots of hardcode file/directory names here. Can we write in below way? entry_module_inf = "UefiPayloadPkg/UefiPayloadEntry/UniversalPayloadEntry.inf" entry_module_inf = os.path.normpath(entry_module_inf) + fv = os.path.join(os.environ['WORKSPACE'], "Build", "UefiPayloadPkgX64", "%s_%s"%(build_target, tool_chain_tag), "FV",2. build_directory = os.path.join(os.environ['WORKSPACE'], os.path.normpath ("Build/UefiPayloadPkgX64") fv = os.path.join(build_directory, "%s_%s"%(build_target, tool_chain_tag), os.path.normpath("FV/DXEFV.fv")) entry = ...... + dsc_patch = os.path.join("UefiPayloadPkg", "UefiPayloadPkg.dsc")3. Similar change can be applied to dsc_patch (typo? dsc_path?) + macro_list = " "4. you can set macro_list = "", then insert space before each "-D" in the loop. +5. you could write in following way build_payload = f"build -p {dsc_patch} ..." + RunCommand(build_payload)6. can you please make sure that there is no file generated in the workspace directory. All files are generated under build directory including the report file and the ELF file. + shutil.copy (entry, os.path.join(os.environ['WORKSPACE'], 'UPL.elf'))7. do you copy out. +8. above message is not needed when printing help. maybe just remove the above message completely. + parser = argparse.ArgumentParser(description='For build Universal Payload')9. set TARGET to DEBUG as default?
|
|
|