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

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

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