[PATCH] ArmPlatformPkg/ArmVExpressPkg: Fix memory attributes for NOR Flash

[PATCH] ArmPlatformPkg/ArmVExpressPkg: Fix memory attributes for NOR Flash

[achin.gupta]

From: Achin Gupta <achin.gupta@arm.com>

The NOR flash banks were being mapped in the translation tables with the same
memory attributes as RAM in the system. These attributes mark the region as
Normal Memory and could additionally be cacheable or non-cacheable.

Either type of attributes are unsuitable for NOR Flash since write operations
could be performed on it. Normal Memory does not guarantee ordering of
transactions that Device memory does. So the commands sent to the Flash device
may not arrive in the right order unless barriers are used. Commands might not
get past the CPU caches in case the region has been mapped with cacheable
attributes.

This patch fixes the problem by mapping the NOR Flash memory region with Device
memory attributes.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Achin Gupta <achin.gupta@arm.com>
---
 ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
index 14c7e8e..2685114 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
+++ b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
@@ -116,7 +116,7 @@ ArmPlatformGetVirtualMemoryMap (
   VirtualMemoryTable[++Index].PhysicalBase = ARM_VE_SMB_NOR0_BASE;
   VirtualMemoryTable[Index].VirtualBase = ARM_VE_SMB_NOR0_BASE;
   VirtualMemoryTable[Index].Length = ARM_VE_SMB_NOR0_SZ + ARM_VE_SMB_NOR1_SZ;
-  VirtualMemoryTable[Index].Attributes = CacheAttributes;
+  VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
 
   // SMB CS2 - SRAM
   VirtualMemoryTable[++Index].PhysicalBase = ARM_VE_SMB_SRAM_BASE;
-- 
1.9.1

Re: [PATCH] ArmPlatformPkg/ArmVExpressPkg: Fix memory attributes for NOR Flash

[Ard Biesheuvel]

On 18 January 2017 at 20:24,  <achin.gupta@arm.com> wrote:
> From: Achin Gupta <achin.gupta@arm.com>
>
> The NOR flash banks were being mapped in the translation tables with the same
> memory attributes as RAM in the system. These attributes mark the region as
> Normal Memory and could additionally be cacheable or non-cacheable.
>
> Either type of attributes are unsuitable for NOR Flash since write operations
> could be performed on it. Normal Memory does not guarantee ordering of
> transactions that Device memory does. So the commands sent to the Flash device
> may not arrive in the right order unless barriers are used. Commands might not
> get past the CPU caches in case the region has been mapped with cacheable
> attributes.
>
> This patch fixes the problem by mapping the NOR Flash memory region with Device
> memory attributes.
>

Device attributes should imply NX, which means we can no longer
execute from the NOR flash

> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Achin Gupta <achin.gupta@arm.com>
> ---
>  ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> index 14c7e8e..2685114 100644
> --- a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> +++ b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> @@ -116,7 +116,7 @@ ArmPlatformGetVirtualMemoryMap (
>    VirtualMemoryTable[++Index].PhysicalBase = ARM_VE_SMB_NOR0_BASE;
>    VirtualMemoryTable[Index].VirtualBase = ARM_VE_SMB_NOR0_BASE;
>    VirtualMemoryTable[Index].Length = ARM_VE_SMB_NOR0_SZ + ARM_VE_SMB_NOR1_SZ;
> -  VirtualMemoryTable[Index].Attributes = CacheAttributes;
> +  VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>
>    // SMB CS2 - SRAM
>    VirtualMemoryTable[++Index].PhysicalBase = ARM_VE_SMB_SRAM_BASE;
> --
> 1.9.1
>

Re: [PATCH] ArmPlatformPkg/ArmVExpressPkg: Fix memory attributes for NOR Flash

[Leif Lindholm]

Hi Achin,

On Wed, Jan 18, 2017 at 08:24:06PM +0000, achin.gupta@arm.com wrote:
> From: Achin Gupta <achin.gupta@arm.com>
> 
> The NOR flash banks were being mapped in the translation tables with the same
> memory attributes as RAM in the system. These attributes mark the region as
> Normal Memory and could additionally be cacheable or non-cacheable.
> 
> Either type of attributes are unsuitable for NOR Flash since write operations
> could be performed on it. Normal Memory does not guarantee ordering of
> transactions that Device memory does. So the commands sent to the Flash device
> may not arrive in the right order unless barriers are used. Commands might not
> get past the CPU caches in case the region has been mapped with cacheable
> attributes.
> 
> This patch fixes the problem by mapping the NOR Flash memory region with Device
> memory attributes.

To add some background to Ard's comment - this was not unintentionally
done:
https://github.com/tianocore/edk2/commit/19bb46c411279dcd30d540c56e5993c5f771c319

Was the reasoning behind this commit incorrect - do you have a
(pre-DXE?) use-case that creates a problem?

Regards,

Leif

> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Achin Gupta <achin.gupta@arm.com>
> ---
>  ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> index 14c7e8e..2685114 100644
> --- a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> +++ b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> @@ -116,7 +116,7 @@ ArmPlatformGetVirtualMemoryMap (
>    VirtualMemoryTable[++Index].PhysicalBase = ARM_VE_SMB_NOR0_BASE;
>    VirtualMemoryTable[Index].VirtualBase = ARM_VE_SMB_NOR0_BASE;
>    VirtualMemoryTable[Index].Length = ARM_VE_SMB_NOR0_SZ + ARM_VE_SMB_NOR1_SZ;
> -  VirtualMemoryTable[Index].Attributes = CacheAttributes;
> +  VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>  
>    // SMB CS2 - SRAM
>    VirtualMemoryTable[++Index].PhysicalBase = ARM_VE_SMB_SRAM_BASE;
> -- 
> 1.9.1
> 

Re: [PATCH] ArmPlatformPkg/ArmVExpressPkg: Fix memory attributes for NOR Flash

[Supreeth Venkatesh]

On Wed, 2017-01-18 at 22:05 +0000, Leif Lindholm wrote:
> Hi Achin,
> 
> On Wed, Jan 18, 2017 at 08:24:06PM +0000, achin.gupta@arm.com wrote:
> > 
> > From: Achin Gupta <achin.gupta@arm.com>
> > 
> > The NOR flash banks were being mapped in the translation tables
> > with the same
> > memory attributes as RAM in the system. These attributes mark the
> > region as
> > Normal Memory and could additionally be cacheable or non-cacheable.
> > 
> > Either type of attributes are unsuitable for NOR Flash since write
> > operations
> > could be performed on it. Normal Memory does not guarantee ordering
> > of
> > transactions that Device memory does. So the commands sent to the
> > Flash device
> > may not arrive in the right order unless barriers are used.
> > Commands might not
> > get past the CPU caches in case the region has been mapped with
> > cacheable
> > attributes.
> > 
> > This patch fixes the problem by mapping the NOR Flash memory region
> > with Device
> > memory attributes.
> To add some background to Ard's comment - this was not
> unintentionally
> done:
> https://github.com/tianocore/edk2/commit/19bb46c411279dcd30d540c56e59
> 93c5f771c319
> 
> Was the reasoning behind this commit incorrect - do you have a
> (pre-DXE?) use-case that creates a problem?
> 
Use Case that creates a problem:
With cache state modeling enabled (i.e, setting it to "1") in FVP, it
will not boot up. 
Perhaps, need to check FVP implementation is correct? 

> Regards,
> 
> Leif
> 
> > 
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Achin Gupta <achin.gupta@arm.com>
> > ---
> >  ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> > | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git
> > a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.
> > c
> > b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.
> > c
> > index 14c7e8e..2685114 100644
> > ---
> > a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.
> > c
> > +++
> > b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.
> > c
> > @@ -116,7 +116,7 @@ ArmPlatformGetVirtualMemoryMap (
> >    VirtualMemoryTable[++Index].PhysicalBase = ARM_VE_SMB_NOR0_BASE;
> >    VirtualMemoryTable[Index].VirtualBase = ARM_VE_SMB_NOR0_BASE;
> >    VirtualMemoryTable[Index].Length = ARM_VE_SMB_NOR0_SZ +
> > ARM_VE_SMB_NOR1_SZ;
> > -  VirtualMemoryTable[Index].Attributes = CacheAttributes;
> > +  VirtualMemoryTable[Index].Attributes =
> > ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> >  
> >    // SMB CS2 - SRAM
> >    VirtualMemoryTable[++Index].PhysicalBase = ARM_VE_SMB_SRAM_BASE;

Re: [PATCH] ArmPlatformPkg/ArmVExpressPkg: Fix memory attributes for NOR Flash

[Achin Gupta]

Hi Leif/Ard,

On Wed, Jan 18, 2017 at 10:05:00PM +0000, Leif Lindholm wrote:
> Hi Achin,
>
> On Wed, Jan 18, 2017 at 08:24:06PM +0000, achin.gupta@arm.com wrote:
> > From: Achin Gupta <achin.gupta@arm.com>
> >
> > The NOR flash banks were being mapped in the translation tables with the same
> > memory attributes as RAM in the system. These attributes mark the region as
> > Normal Memory and could additionally be cacheable or non-cacheable.
> >
> > Either type of attributes are unsuitable for NOR Flash since write operations
> > could be performed on it. Normal Memory does not guarantee ordering of
> > transactions that Device memory does. So the commands sent to the Flash device
> > may not arrive in the right order unless barriers are used. Commands might not
> > get past the CPU caches in case the region has been mapped with cacheable
> > attributes.
> >
> > This patch fixes the problem by mapping the NOR Flash memory region with Device
> > memory attributes.
>
> To add some background to Ard's comment - this was not unintentionally
> done:
> https://github.com/tianocore/edk2/commit/19bb46c411279dcd30d540c56e5993c5f771c319

Thanks! I missed this commit. There is some background to the problem I am
facing below.

>
> Was the reasoning behind this commit incorrect - do you have a
> (pre-DXE?) use-case that creates a problem?

AFAIU, The current memory attributes for NOR Flash work only for reads. They
should additionally be RO to flag any unexpected writes.

Mine is a DXE use case! In NorFlashDxe.c, commands are send to the Flash (erase
block etc.). They might never reach the device if there is a write-back cache in
between. So either device or Normal memory with non-cacheable/write-through
attributes and barriers should work.

If I turn on cache state modelling in the Base FVP, the code gets stuck in
NorFlashReadStatusRegister() in the below loop in NorFlashEraseSingleBlock():

  // Wait until the status register gives us the all clear
  do {
    StatusRegister = NorFlashReadStatusRegister (Instance, BlockAddress);
  } while ((StatusRegister & P30_SR_BIT_WRITE) != P30_SR_BIT_WRITE);

I think the SEND_NOR_COMMANDs at the beginning of the function get stuck in the
cache. Changing the flash memory attributes as per this patch solves the
problem.

The original patch from Ard mentions that the NOR Flash DXE driver should change
the attributes of the region it wants to write to. Is this what is missing?

Please do let me know if I am missing any subtleties of the driver. I am not a
NOR flash expert :(

cheers,
Achin



>
> Regards,
>
> Leif
>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Achin Gupta <achin.gupta@arm.com>
> > ---
> >  ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> > index 14c7e8e..2685114 100644
> > --- a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> > +++ b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> > @@ -116,7 +116,7 @@ ArmPlatformGetVirtualMemoryMap (
> >    VirtualMemoryTable[++Index].PhysicalBase = ARM_VE_SMB_NOR0_BASE;
> >    VirtualMemoryTable[Index].VirtualBase = ARM_VE_SMB_NOR0_BASE;
> >    VirtualMemoryTable[Index].Length = ARM_VE_SMB_NOR0_SZ + ARM_VE_SMB_NOR1_SZ;
> > -  VirtualMemoryTable[Index].Attributes = CacheAttributes;
> > +  VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> >
> >    // SMB CS2 - SRAM
> >    VirtualMemoryTable[++Index].PhysicalBase = ARM_VE_SMB_SRAM_BASE;
> > --
> > 1.9.1
> >

Re: [PATCH] ArmPlatformPkg/ArmVExpressPkg: Fix memory attributes for NOR Flash

[Ard Biesheuvel]

On 19 January 2017 at 12:31, Achin Gupta <achin.gupta@arm.com> wrote:
> Hi Leif/Ard,
>
> On Wed, Jan 18, 2017 at 10:05:00PM +0000, Leif Lindholm wrote:
>> Hi Achin,
>>
>> On Wed, Jan 18, 2017 at 08:24:06PM +0000, achin.gupta@arm.com wrote:
>> > From: Achin Gupta <achin.gupta@arm.com>
>> >
>> > The NOR flash banks were being mapped in the translation tables with the same
>> > memory attributes as RAM in the system. These attributes mark the region as
>> > Normal Memory and could additionally be cacheable or non-cacheable.
>> >
>> > Either type of attributes are unsuitable for NOR Flash since write operations
>> > could be performed on it. Normal Memory does not guarantee ordering of
>> > transactions that Device memory does. So the commands sent to the Flash device
>> > may not arrive in the right order unless barriers are used. Commands might not
>> > get past the CPU caches in case the region has been mapped with cacheable
>> > attributes.
>> >
>> > This patch fixes the problem by mapping the NOR Flash memory region with Device
>> > memory attributes.
>>
>> To add some background to Ard's comment - this was not unintentionally
>> done:
>> https://github.com/tianocore/edk2/commit/19bb46c411279dcd30d540c56e5993c5f771c319
>
> Thanks! I missed this commit. There is some background to the problem I am
> facing below.
>
>>
>> Was the reasoning behind this commit incorrect - do you have a
>> (pre-DXE?) use-case that creates a problem?
>
> AFAIU, The current memory attributes for NOR Flash work only for reads. They
> should additionally be RO to flag any unexpected writes.
>
> Mine is a DXE use case! In NorFlashDxe.c, commands are send to the Flash (erase
> block etc.). They might never reach the device if there is a write-back cache in
> between. So either device or Normal memory with non-cacheable/write-through
> attributes and barriers should work.
>
> If I turn on cache state modelling in the Base FVP, the code gets stuck in
> NorFlashReadStatusRegister() in the below loop in NorFlashEraseSingleBlock():
>
>   // Wait until the status register gives us the all clear
>   do {
>     StatusRegister = NorFlashReadStatusRegister (Instance, BlockAddress);
>   } while ((StatusRegister & P30_SR_BIT_WRITE) != P30_SR_BIT_WRITE);
>
> I think the SEND_NOR_COMMANDs at the beginning of the function get stuck in the
> cache. Changing the flash memory attributes as per this patch solves the
> problem.
>
> The original patch from Ard mentions that the NOR Flash DXE driver should change
> the attributes of the region it wants to write to. Is this what is missing?
>

NorFlashFvbInitialize() in
ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c contains the
following calls:

  Status = gDS->AddMemorySpace (
      EfiGcdMemoryTypeMemoryMappedIo,
      Instance->DeviceBaseAddress, RuntimeMmioRegionSize,
      EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
      );
  ASSERT_EFI_ERROR (Status);

  Status = gDS->SetMemorySpaceAttributes (
      Instance->DeviceBaseAddress, RuntimeMmioRegionSize,
      EFI_MEMORY_UC | EFI_MEMORY_RUNTIME);
  ASSERT_EFI_ERROR (Status);

which is supposed to set device attributes on the NOR region that is
actually exposed to the upper layers as read-write capable.

Perhaps you can enable GCD debugging to trace these calls, to ensure
that the address you are stalled on is actually covered?

Re: [PATCH] ArmPlatformPkg/ArmVExpressPkg: Fix memory attributes for NOR Flash

[Achin Gupta]

Hi Ard,

On Thu, Jan 19, 2017 at 06:16:00PM +0000, Ard Biesheuvel wrote:
> On 19 January 2017 at 12:31, Achin Gupta <achin.gupta@arm.com> wrote:
> > Hi Leif/Ard,
> >
> > On Wed, Jan 18, 2017 at 10:05:00PM +0000, Leif Lindholm wrote:
> >> Hi Achin,
> >>
> >> On Wed, Jan 18, 2017 at 08:24:06PM +0000, achin.gupta@arm.com wrote:
> >> > From: Achin Gupta <achin.gupta@arm.com>
> >> >
> >> > The NOR flash banks were being mapped in the translation tables with the same
> >> > memory attributes as RAM in the system. These attributes mark the region as
> >> > Normal Memory and could additionally be cacheable or non-cacheable.
> >> >
> >> > Either type of attributes are unsuitable for NOR Flash since write operations
> >> > could be performed on it. Normal Memory does not guarantee ordering of
> >> > transactions that Device memory does. So the commands sent to the Flash device
> >> > may not arrive in the right order unless barriers are used. Commands might not
> >> > get past the CPU caches in case the region has been mapped with cacheable
> >> > attributes.
> >> >
> >> > This patch fixes the problem by mapping the NOR Flash memory region with Device
> >> > memory attributes.
> >>
> >> To add some background to Ard's comment - this was not unintentionally
> >> done:
> >> https://github.com/tianocore/edk2/commit/19bb46c411279dcd30d540c56e5993c5f771c319
> >
> > Thanks! I missed this commit. There is some background to the problem I am
> > facing below.
> >
> >>
> >> Was the reasoning behind this commit incorrect - do you have a
> >> (pre-DXE?) use-case that creates a problem?
> >
> > AFAIU, The current memory attributes for NOR Flash work only for reads. They
> > should additionally be RO to flag any unexpected writes.
> >
> > Mine is a DXE use case! In NorFlashDxe.c, commands are send to the Flash (erase
> > block etc.). They might never reach the device if there is a write-back cache in
> > between. So either device or Normal memory with non-cacheable/write-through
> > attributes and barriers should work.
> >
> > If I turn on cache state modelling in the Base FVP, the code gets stuck in
> > NorFlashReadStatusRegister() in the below loop in NorFlashEraseSingleBlock():
> >
> >   // Wait until the status register gives us the all clear
> >   do {
> >     StatusRegister = NorFlashReadStatusRegister (Instance, BlockAddress);
> >   } while ((StatusRegister & P30_SR_BIT_WRITE) != P30_SR_BIT_WRITE);
> >
> > I think the SEND_NOR_COMMANDs at the beginning of the function get stuck in the
> > cache. Changing the flash memory attributes as per this patch solves the
> > problem.
> >
> > The original patch from Ard mentions that the NOR Flash DXE driver should change
> > the attributes of the region it wants to write to. Is this what is missing?
> >
>
> NorFlashFvbInitialize() in
> ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c contains the
> following calls:
>
>   Status = gDS->AddMemorySpace (
>       EfiGcdMemoryTypeMemoryMappedIo,
>       Instance->DeviceBaseAddress, RuntimeMmioRegionSize,
>       EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
>       );
>   ASSERT_EFI_ERROR (Status);
>
>   Status = gDS->SetMemorySpaceAttributes (
>       Instance->DeviceBaseAddress, RuntimeMmioRegionSize,
>       EFI_MEMORY_UC | EFI_MEMORY_RUNTIME);
>   ASSERT_EFI_ERROR (Status);
>
> which is supposed to set device attributes on the NOR region that is
> actually exposed to the upper layers as read-write capable.
>
> Perhaps you can enable GCD debugging to trace these calls, to ensure
> that the address you are stalled on is actually covered?

Thanks for the pointer. Somehow NorFlashFvbInitialize() gets called and a valid
firmware volume header is not found. This irrespective of the state of cache
modelling. The function proceeds to install a header for which it first tries to
erase the Flash blocks reserved for variable storage.

I am not sure why a FV header is not found. However the flash erase in response
happens with the pre-DXE memory attributes for the flash region. The GCD
services to change the attributes are called later. So this seems like a logical
error in the code. Does this make sense to you? Here is the trace for
reference. The last print was added by me.

>>>>
add-symbol-file /home/achgup01/work/genfw/uefi/edk2/Build/ArmVExpress-FVP-AArch64/DEBUG_GCC49/AARCH64/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe/DEBUG/ArmVeNorFlashDxe.dll 0xEAEA0000
Loading driver at 0x000EAE90000 EntryPoint=0x000EAEA0044 ArmVeNorFlashDxe.efi
NorFlashWriteBlocks: informational - Had to enable HSYS_FLASH flag.
FvbRead(Parameters: Lba=0, Offset=0x0, *NumBytes=0x40, Buffer @ 0xF3FFF8A8)
NorFlashFvbInitialize
ValidateFvHeader: No Firmware Volume header present
NorFlashFvbInitialize: The FVB Header is not valid.
NorFlashFvbInitialize: Installing a correct one for this volume.
FvbEraseBlocks()
FvbEraseBlocks: Check if: ( StartingLba=0 + NumOfLba=3 - 1 ) > LastBlock=2.
FvbEraseBlocks: Erasing Lba=0 @ 0x0FFC0000.
NorFlashEraseSingleBlock: BlockAddress=0x0FFC0000
>>>>

Any pointers on the absent FV header and how NorFlashFvbInitialize() gets called
would be helpful.

cheers,
Achin

Re: [PATCH] ArmPlatformPkg/ArmVExpressPkg: Fix memory attributes for NOR Flash

[Ard Biesheuvel]

On 19 January 2017 at 21:57, Achin Gupta <achin.gupta@arm.com> wrote:
> Hi Ard,
>
> On Thu, Jan 19, 2017 at 06:16:00PM +0000, Ard Biesheuvel wrote:
>> On 19 January 2017 at 12:31, Achin Gupta <achin.gupta@arm.com> wrote:
>> > Hi Leif/Ard,
>> >
>> > On Wed, Jan 18, 2017 at 10:05:00PM +0000, Leif Lindholm wrote:
>> >> Hi Achin,
>> >>
>> >> On Wed, Jan 18, 2017 at 08:24:06PM +0000, achin.gupta@arm.com wrote:
>> >> > From: Achin Gupta <achin.gupta@arm.com>
>> >> >
>> >> > The NOR flash banks were being mapped in the translation tables with the same
>> >> > memory attributes as RAM in the system. These attributes mark the region as
>> >> > Normal Memory and could additionally be cacheable or non-cacheable.
>> >> >
>> >> > Either type of attributes are unsuitable for NOR Flash since write operations
>> >> > could be performed on it. Normal Memory does not guarantee ordering of
>> >> > transactions that Device memory does. So the commands sent to the Flash device
>> >> > may not arrive in the right order unless barriers are used. Commands might not
>> >> > get past the CPU caches in case the region has been mapped with cacheable
>> >> > attributes.
>> >> >
>> >> > This patch fixes the problem by mapping the NOR Flash memory region with Device
>> >> > memory attributes.
>> >>
>> >> To add some background to Ard's comment - this was not unintentionally
>> >> done:
>> >> https://github.com/tianocore/edk2/commit/19bb46c411279dcd30d540c56e5993c5f771c319
>> >
>> > Thanks! I missed this commit. There is some background to the problem I am
>> > facing below.
>> >
>> >>
>> >> Was the reasoning behind this commit incorrect - do you have a
>> >> (pre-DXE?) use-case that creates a problem?
>> >
>> > AFAIU, The current memory attributes for NOR Flash work only for reads. They
>> > should additionally be RO to flag any unexpected writes.
>> >
>> > Mine is a DXE use case! In NorFlashDxe.c, commands are send to the Flash (erase
>> > block etc.). They might never reach the device if there is a write-back cache in
>> > between. So either device or Normal memory with non-cacheable/write-through
>> > attributes and barriers should work.
>> >
>> > If I turn on cache state modelling in the Base FVP, the code gets stuck in
>> > NorFlashReadStatusRegister() in the below loop in NorFlashEraseSingleBlock():
>> >
>> >   // Wait until the status register gives us the all clear
>> >   do {
>> >     StatusRegister = NorFlashReadStatusRegister (Instance, BlockAddress);
>> >   } while ((StatusRegister & P30_SR_BIT_WRITE) != P30_SR_BIT_WRITE);
>> >
>> > I think the SEND_NOR_COMMANDs at the beginning of the function get stuck in the
>> > cache. Changing the flash memory attributes as per this patch solves the
>> > problem.
>> >
>> > The original patch from Ard mentions that the NOR Flash DXE driver should change
>> > the attributes of the region it wants to write to. Is this what is missing?
>> >
>>
>> NorFlashFvbInitialize() in
>> ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c contains the
>> following calls:
>>
>>   Status = gDS->AddMemorySpace (
>>       EfiGcdMemoryTypeMemoryMappedIo,
>>       Instance->DeviceBaseAddress, RuntimeMmioRegionSize,
>>       EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
>>       );
>>   ASSERT_EFI_ERROR (Status);
>>
>>   Status = gDS->SetMemorySpaceAttributes (
>>       Instance->DeviceBaseAddress, RuntimeMmioRegionSize,
>>       EFI_MEMORY_UC | EFI_MEMORY_RUNTIME);
>>   ASSERT_EFI_ERROR (Status);
>>
>> which is supposed to set device attributes on the NOR region that is
>> actually exposed to the upper layers as read-write capable.
>>
>> Perhaps you can enable GCD debugging to trace these calls, to ensure
>> that the address you are stalled on is actually covered?
>
> Thanks for the pointer. Somehow NorFlashFvbInitialize() gets called and a valid
> firmware volume header is not found. This irrespective of the state of cache
> modelling. The function proceeds to install a header for which it first tries to
> erase the Flash blocks reserved for variable storage.
>
> I am not sure why a FV header is not found. However the flash erase in response
> happens with the pre-DXE memory attributes for the flash region. The GCD
> services to change the attributes are called later. So this seems like a logical
> error in the code. Does this make sense to you? Here is the trace for
> reference. The last print was added by me.
>
>>>>>
> add-symbol-file /home/achgup01/work/genfw/uefi/edk2/Build/ArmVExpress-FVP-AArch64/DEBUG_GCC49/AARCH64/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe/DEBUG/ArmVeNorFlashDxe.dll 0xEAEA0000
> Loading driver at 0x000EAE90000 EntryPoint=0x000EAEA0044 ArmVeNorFlashDxe.efi
> NorFlashWriteBlocks: informational - Had to enable HSYS_FLASH flag.
> FvbRead(Parameters: Lba=0, Offset=0x0, *NumBytes=0x40, Buffer @ 0xF3FFF8A8)
> NorFlashFvbInitialize
> ValidateFvHeader: No Firmware Volume header present
> NorFlashFvbInitialize: The FVB Header is not valid.
> NorFlashFvbInitialize: Installing a correct one for this volume.
> FvbEraseBlocks()
> FvbEraseBlocks: Check if: ( StartingLba=0 + NumOfLba=3 - 1 ) > LastBlock=2.
> FvbEraseBlocks: Erasing Lba=0 @ 0x0FFC0000.
> NorFlashEraseSingleBlock: BlockAddress=0x0FFC0000
>>>>>
>
> Any pointers on the absent FV header and how NorFlashFvbInitialize() gets called
> would be helpful.
>

Right, there is a 'feature' in the code to reinit the FV if it is
corrupted, which is useful for emulators given that their NOR flash
images are usually not backed by anything. For QEMU, we've added an
FDF definition similar to what OVMF uses which is simply a binary dump
of a pristine FV header.

So yes, this is a logic error in the code: the reinit should not occur
before the remapping of the region.

Re: [PATCH] ArmPlatformPkg/ArmVExpressPkg: Fix memory attributes for NOR Flash

[Achin Gupta]

Hi Ard,

On Thu, Jan 19, 2017 at 10:01:07PM +0000, Ard Biesheuvel wrote:
> On 19 January 2017 at 21:57, Achin Gupta <achin.gupta@arm.com> wrote:
> > Hi Ard,
> >
> > On Thu, Jan 19, 2017 at 06:16:00PM +0000, Ard Biesheuvel wrote:
> >> On 19 January 2017 at 12:31, Achin Gupta <achin.gupta@arm.com> wrote:
> >> > Hi Leif/Ard,
> >> >
> >> > On Wed, Jan 18, 2017 at 10:05:00PM +0000, Leif Lindholm wrote:
> >> >> Hi Achin,
> >> >>
> >> >> On Wed, Jan 18, 2017 at 08:24:06PM +0000, achin.gupta@arm.com wrote:
> >> >> > From: Achin Gupta <achin.gupta@arm.com>
> >> >> >
> >> >> > The NOR flash banks were being mapped in the translation tables with the same
> >> >> > memory attributes as RAM in the system. These attributes mark the region as
> >> >> > Normal Memory and could additionally be cacheable or non-cacheable.
> >> >> >
> >> >> > Either type of attributes are unsuitable for NOR Flash since write operations
> >> >> > could be performed on it. Normal Memory does not guarantee ordering of
> >> >> > transactions that Device memory does. So the commands sent to the Flash device
> >> >> > may not arrive in the right order unless barriers are used. Commands might not
> >> >> > get past the CPU caches in case the region has been mapped with cacheable
> >> >> > attributes.
> >> >> >
> >> >> > This patch fixes the problem by mapping the NOR Flash memory region with Device
> >> >> > memory attributes.
> >> >>
> >> >> To add some background to Ard's comment - this was not unintentionally
> >> >> done:
> >> >> https://github.com/tianocore/edk2/commit/19bb46c411279dcd30d540c56e5993c5f771c319
> >> >
> >> > Thanks! I missed this commit. There is some background to the problem I am
> >> > facing below.
> >> >
> >> >>
> >> >> Was the reasoning behind this commit incorrect - do you have a
> >> >> (pre-DXE?) use-case that creates a problem?
> >> >
> >> > AFAIU, The current memory attributes for NOR Flash work only for reads. They
> >> > should additionally be RO to flag any unexpected writes.
> >> >
> >> > Mine is a DXE use case! In NorFlashDxe.c, commands are send to the Flash (erase
> >> > block etc.). They might never reach the device if there is a write-back cache in
> >> > between. So either device or Normal memory with non-cacheable/write-through
> >> > attributes and barriers should work.
> >> >
> >> > If I turn on cache state modelling in the Base FVP, the code gets stuck in
> >> > NorFlashReadStatusRegister() in the below loop in NorFlashEraseSingleBlock():
> >> >
> >> >   // Wait until the status register gives us the all clear
> >> >   do {
> >> >     StatusRegister = NorFlashReadStatusRegister (Instance, BlockAddress);
> >> >   } while ((StatusRegister & P30_SR_BIT_WRITE) != P30_SR_BIT_WRITE);
> >> >
> >> > I think the SEND_NOR_COMMANDs at the beginning of the function get stuck in the
> >> > cache. Changing the flash memory attributes as per this patch solves the
> >> > problem.
> >> >
> >> > The original patch from Ard mentions that the NOR Flash DXE driver should change
> >> > the attributes of the region it wants to write to. Is this what is missing?
> >> >
> >>
> >> NorFlashFvbInitialize() in
> >> ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c contains the
> >> following calls:
> >>
> >>   Status = gDS->AddMemorySpace (
> >>       EfiGcdMemoryTypeMemoryMappedIo,
> >>       Instance->DeviceBaseAddress, RuntimeMmioRegionSize,
> >>       EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
> >>       );
> >>   ASSERT_EFI_ERROR (Status);
> >>
> >>   Status = gDS->SetMemorySpaceAttributes (
> >>       Instance->DeviceBaseAddress, RuntimeMmioRegionSize,
> >>       EFI_MEMORY_UC | EFI_MEMORY_RUNTIME);
> >>   ASSERT_EFI_ERROR (Status);
> >>
> >> which is supposed to set device attributes on the NOR region that is
> >> actually exposed to the upper layers as read-write capable.
> >>
> >> Perhaps you can enable GCD debugging to trace these calls, to ensure
> >> that the address you are stalled on is actually covered?
> >
> > Thanks for the pointer. Somehow NorFlashFvbInitialize() gets called and a valid
> > firmware volume header is not found. This irrespective of the state of cache
> > modelling. The function proceeds to install a header for which it first tries to
> > erase the Flash blocks reserved for variable storage.
> >
> > I am not sure why a FV header is not found. However the flash erase in response
> > happens with the pre-DXE memory attributes for the flash region. The GCD
> > services to change the attributes are called later. So this seems like a logical
> > error in the code. Does this make sense to you? Here is the trace for
> > reference. The last print was added by me.
> >
> >>>>>
> > add-symbol-file /home/achgup01/work/genfw/uefi/edk2/Build/ArmVExpress-FVP-AArch64/DEBUG_GCC49/AARCH64/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe/DEBUG/ArmVeNorFlashDxe.dll 0xEAEA0000
> > Loading driver at 0x000EAE90000 EntryPoint=0x000EAEA0044 ArmVeNorFlashDxe.efi
> > NorFlashWriteBlocks: informational - Had to enable HSYS_FLASH flag.
> > FvbRead(Parameters: Lba=0, Offset=0x0, *NumBytes=0x40, Buffer @ 0xF3FFF8A8)
> > NorFlashFvbInitialize
> > ValidateFvHeader: No Firmware Volume header present
> > NorFlashFvbInitialize: The FVB Header is not valid.
> > NorFlashFvbInitialize: Installing a correct one for this volume.
> > FvbEraseBlocks()
> > FvbEraseBlocks: Check if: ( StartingLba=0 + NumOfLba=3 - 1 ) > LastBlock=2.
> > FvbEraseBlocks: Erasing Lba=0 @ 0x0FFC0000.
> > NorFlashEraseSingleBlock: BlockAddress=0x0FFC0000
> >>>>>
> >
> > Any pointers on the absent FV header and how NorFlashFvbInitialize() gets called
> > would be helpful.
> >
>
> Right, there is a 'feature' in the code to reinit the FV if it is
> corrupted, which is useful for emulators given that their NOR flash
> images are usually not backed by anything. For QEMU, we've added an
> FDF definition similar to what OVMF uses which is simply a binary dump
> of a pristine FV header.
>
> So yes, this is a logic error in the code: the reinit should not occur
> before the remapping of the region.

OK! I moved the remapping code to the beginning of the function i.e. first remap
the flash before poking it and this seems to have solved the problem. I will
send a separate patch for that. Further issues can be discussed in that thread.

cheers,
Achin