Hi Ard, On Thu, Jan 19, 2017 at 10:01:07PM +0000, Ard Biesheuvel wrote: > On 19 January 2017 at 21:57, Achin Gupta <[email protected]> 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 <[email protected]> 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, [email protected] wrote: > >> >> > From: Achin Gupta <[email protected]> > >> >> > > >> >> > 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 _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
