[v1] RISC-V: Add MMU support

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

Posted by Tuan Phan 2 years, 11 months ago

The flash base address can be added to GCD before this driver run.
So only add it if it has not been done.

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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Ard Biesheuvel 2 years, 11 months ago

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.

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100758): https://edk2.groups.io/g/devel/message/100758
Mute This Topic: https://groups.io/mt/97430554/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Tuan Phan 2 years, 8 months ago

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.


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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105276): https://edk2.groups.io/g/devel/message/105276
Mute This Topic: https://groups.io/mt/97430554/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Ard Biesheuvel 2 years, 8 months ago

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105299): https://edk2.groups.io/g/devel/message/105299
Mute This Topic: https://groups.io/mt/97430554/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Tuan Phan 2 years, 8 months ago

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105311): https://edk2.groups.io/g/devel/message/105311
Mute This Topic: https://groups.io/mt/97430554/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 1/7] MdePkg/BaseLib: RISC-V: Support getting satp register value
[edk2-devel] [PATCH 2/7] MdePkg/Register: RISC-V: Add satp mode bits shift definition
[edk2-devel] [PATCH 3/7] UefiCpuPkg: RISC-V: Support MMU with SV39/48/57 mode
[edk2-devel] [PATCH 4/7] OvmfPkg/RiscVVirt: VirtNorFlashPlatformLib: Fix wrong flash size
[edk2-devel] [PATCH 5/7] OvmfPkg/VirtNorFlashDxe: Not add memory space if it exists
[edk2-devel] [PATCH 6/7] OvmfPkg/RiscVVirt: SEC: Add IO memory resource hob for platform devices
[edk2-devel] [PATCH 7/7] OvmfPkg/RiscVVirt: Enable MMU with SV39 mode