On Thu, May 25, 2023 at 7:27 AM Ard Biesheuvel <ardb@kernel.org> wrote:

> 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.
>

The SEC phase just simply adds that region to the memory hob with
BuildResourceDescriptorHob.
Agree, the problem is VariableRuntimeDxe.efi accessing the region before
this VirtNorFlashDxe starts so when
MMU enabled, edk2 will hang due to page fault exception.

>
>
> >>
> >> > 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]
> >> > ------------
> >> >
> >> >
> >
> > 
>