Re: [edk2-devel] [PATCH 5/7] OvmfPkg/VirtNorFlashDxe: Not add memory space if it exists

["Ard Biesheuvel" <ardb@kernel.org>]

On Wed, 24 May 2023 at 20:13, Tuan Phan <tphan@ventanamicro.com> wrote:
>
>
>
> On Mon, Mar 6, 2023 at 9:53 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> On Mon, 6 Mar 2023 at 18:33, Tuan Phan <tphan@ventanamicro.com> wrote:
>> >
>> > The flash base address can be added to GCD before this driver run.
>> > So only add it if it has not been done.
>> >
>>
>> How do you end up in this situation?
>>
>> You cannot skip this registration, as it is required to get the region
>> marked as EFI_MEMORY_RUNTIME, and without that, EFI variables will be
>> broken when running under the OS.
>
>
> Ard,
> The patch only skips AddMemorySpace if it is already done in the early SEC phase for RiscV platform.
> The EFI_MEMORY_RUNTIME always be set in the next line with SetMemorySpaceAttributes.
>

So how does the SEC phase create GCD regions to begin with?

This really sounds like there is a problem elsewhere tbh.


>>
>> > Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
>> > ---
>> >  OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c | 25 +++++++++++++++--------
>> >  1 file changed, 16 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c
>> > index f9a41f6aab0f..8875824f3333 100644
>> > --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c
>> > +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c
>> > @@ -372,10 +372,11 @@ NorFlashFvbInitialize (
>> >    IN NOR_FLASH_INSTANCE  *Instance
>> >    )
>> >  {
>> > -  EFI_STATUS     Status;
>> > -  UINT32         FvbNumLba;
>> > -  EFI_BOOT_MODE  BootMode;
>> > -  UINTN          RuntimeMmioRegionSize;
>> > +  EFI_STATUS                      Status;
>> > +  UINT32                          FvbNumLba;
>> > +  EFI_BOOT_MODE                   BootMode;
>> > +  UINTN                           RuntimeMmioRegionSize;
>> > +  EFI_GCD_MEMORY_SPACE_DESCRIPTOR Desc;
>> >
>> >    DEBUG ((DEBUG_BLKIO, "NorFlashFvbInitialize\n"));
>> >    ASSERT ((Instance != NULL));
>> > @@ -390,13 +391,19 @@ NorFlashFvbInitialize (
>> >    //       is written as the base of the flash region (ie: Instance->DeviceBaseAddress)
>> >    RuntimeMmioRegionSize = (Instance->RegionBaseAddress - Instance->DeviceBaseAddress) + Instance->Size;
>> >
>> > -  Status = gDS->AddMemorySpace (
>> > -                  EfiGcdMemoryTypeMemoryMappedIo,
>> > +  Status = gDS->GetMemorySpaceDescriptor (
>> >                    Instance->DeviceBaseAddress,
>> > -                  RuntimeMmioRegionSize,
>> > -                  EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
>> > +                  &Desc
>> >                    );
>> > -  ASSERT_EFI_ERROR (Status);
>> > +  if (Status == EFI_NOT_FOUND) {
>> > +    Status = gDS->AddMemorySpace (
>> > +                    EfiGcdMemoryTypeMemoryMappedIo,
>> > +                    Instance->DeviceBaseAddress,
>> > +                    RuntimeMmioRegionSize,
>> > +                    EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
>> > +                    );
>> > +    ASSERT_EFI_ERROR (Status);
>> > +  }
>> >
>> >    Status = gDS->SetMemorySpaceAttributes (
>> >                    Instance->DeviceBaseAddress,
>> > --
>> > 2.25.1
>> >
>> >
>> >
>> > ------------
>> > Groups.io Links: You receive all messages sent to this group.
>> > View/Reply Online (#100754): https://edk2.groups.io/g/devel/message/100754
>> > Mute This Topic: https://groups.io/mt/97430554/1131722
>> > Group Owner: devel+owner@edk2.groups.io
>> > Unsubscribe: https://edk2.groups.io/g/devel/unsub [ardb@kernel.org]
>> > ------------
>> >
>> >
>
>