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

[Achin Gupta <achin.gupta@arm.com>]

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