On 11/28/18 20:16, Ard Biesheuvel wrote: > QEMU/mach-virt is rather unhelpful when it comes to tracking down > NULL pointer dereferences that occur while running in UEFI: since > we have NOR flash mapped at address 0x0, inadvertent reads go > unnoticed, and even most writes are silently dropped, unless you're > unlucky and the instruction in question is one that KVM cannot > emulate, in which case you end up with a QEMU crash like this: > > error: kvm run failed Function not implemented > PC=000000013f7ff804 X00=000000013f7ab108 X01=0000000000000064 > X02=000000013f801988 X03=00000000800003c4 X04=0000000000000000 > X05=0000000096000044 X06=fffffffffffd8270 X07=000000013f7ab4a0 > X08=0000000000000001 X09=000000013f803b88 X10=000000013f7e88d0 > X11=0000000000000009 X12=000000013f7ab554 X13=0000000000000008 > X14=0000000000000002 X15=0000000000000000 X16=0000000000000000 > X17=0000000000000000 X18=0000000000000000 X19=0000000000000000 > X20=000000013f81c000 X21=000000013f7ab170 X22=000000013f81c000 > X23=0000000009000018 X24=000000013f407020 X25=000000013f81c000 > X26=000000013f803530 X27=000000013f802000 X28=000000013f7ab270 > X29=000000013f7ab0d0 X30=000000013f7fee10 SP=000000013f7a6f30 > PSTATE=800003c5 N--- EL1h > > and a warning in the host kernel log that load/store instruction > decoding is not supported by KVM. > > Given that the first page of the flash device is not actually > used anyway, let's reduce the mappings of the peripheral space > and the flash device (both of which cover page #0) to only cover > what is actually required.
Please insert a pointer here, to the PcdFvBaseAddress / PcdFvSize settings in the "ArmVirtQemu.fdf" and "ArmVirtQemuKernel.fdf" files. You could even quote them verbatim (from both FDF files): ArmVirtQemu.fdf: > 0x00001000|0x001ff000 > gArmTokenSpaceGuid.PcdFvBaseAddress|gArmTokenSpaceGuid.PcdFvSize ArmVirtQemuKernel.fdf: > 0x00008000|0x001f8000 > gArmTokenSpaceGuid.PcdFvBaseAddress|gArmTokenSpaceGuid.PcdFvSize > After this change, any inadvertent read or write from/to the first > physical page will trigger a translation fault inside the guest, > regardless of the nature of the instruction, without crashing QEMU. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ard Biesheuvel <[email protected]> > --- > ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf | 4 ++-- > ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf | 2 ++ > ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c | 18 > +++++++++++------- > 3 files changed, 15 insertions(+), 9 deletions(-) > > diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf > b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf > index 5c5b841051ad..b6abc52531a8 100644 > --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf > +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf > @@ -39,9 +39,9 @@ [LibraryClasses] > PcdLib > > [Pcd] > - gArmTokenSpaceGuid.PcdFdBaseAddress > + gArmTokenSpaceGuid.PcdFvBaseAddress > gArmTokenSpaceGuid.PcdSystemMemoryBase > gArmTokenSpaceGuid.PcdSystemMemorySize > > [FixedPcd] > - gArmTokenSpaceGuid.PcdFdSize > + gArmTokenSpaceGuid.PcdFvSize > diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf > b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf > index d12089760b22..16802c5c414b 100644 > --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf > +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf > @@ -43,9 +43,11 @@ [LibraryClasses] > > [Pcd] > gArmTokenSpaceGuid.PcdFdBaseAddress > + gArmTokenSpaceGuid.PcdFvBaseAddress > gArmTokenSpaceGuid.PcdSystemMemoryBase > gArmTokenSpaceGuid.PcdSystemMemorySize > > [FixedPcd] > gArmTokenSpaceGuid.PcdFdSize > + gArmTokenSpaceGuid.PcdFvSize > gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress > diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c > b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c > index 0285a11b1d77..0818d0b42d6c 100644 > --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c > +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c > @@ -21,6 +21,10 @@ > // Number of Virtual Memory Map Descriptors > #define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 5 > > +// mach-virt's 'miscellaneous device I/O' region > +#define MACH_VIRT_PERIPH_BASE 0x08000000 > +#define MACH_VIRT_PERIPH_SIZE SIZE_128MB > + So this extends from the end of VIRT_FLASH (that is, one past it) to just before VIRT_PCIE_MMIO. Correct? If so, can you work that into the comment? (Not necessarily with the QEMU enum constants.) > /** > Return the Virtual Memory Map of your platform > > @@ -66,16 +70,16 @@ ArmVirtGetMemoryMap ( > VirtualMemoryTable[0].VirtualBase, > VirtualMemoryTable[0].Length)); > > - // Peripheral space before DRAM > - VirtualMemoryTable[1].PhysicalBase = 0x0; > - VirtualMemoryTable[1].VirtualBase = 0x0; > - VirtualMemoryTable[1].Length = VirtualMemoryTable[0].PhysicalBase; > + // Memory mapped peripherals (UART, RTC, GIC, virtio-mmio, etc) The comment helps. > + VirtualMemoryTable[1].PhysicalBase = MACH_VIRT_PERIPH_BASE; > + VirtualMemoryTable[1].VirtualBase = MACH_VIRT_PERIPH_BASE; > + VirtualMemoryTable[1].Length = MACH_VIRT_PERIPH_SIZE; > VirtualMemoryTable[1].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE; So we lose coverage for: [VIRT_PCIE_MMIO] = { 0x10000000, 0x2eff0000 }, [VIRT_PCIE_PIO] = { 0x3eff0000, 0x00010000 }, [VIRT_PCIE_ECAM] = { 0x3f000000, 0x01000000 }, Among these, VIRT_PCIE_MMIO is handled by our FDT-based PciHostBridgeLib (just the exposure), and then added to GCD / mapped indirectly by PciHostBridgeLib. VIRT_PCIE_PIO is covered by your "[edk2] [PATCH v3 03/16] ArmVirtPkg/FdtPciHostBridgeLib: map ECAM and I/O spaces in GCD memory map". Ditto for VIRT_PCIE_ECAM. Should be safe. > > - // Remap the FD region as normal executable memory > - VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFdBaseAddress); > + // Map the FV region as normal executable memory > + VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFvBaseAddress); > VirtualMemoryTable[2].VirtualBase = VirtualMemoryTable[2].PhysicalBase; > - VirtualMemoryTable[2].Length = FixedPcdGet32 (PcdFdSize); > + VirtualMemoryTable[2].Length = FixedPcdGet32 (PcdFvSize); > VirtualMemoryTable[2].Attributes = > ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK; > > // End of Table > Seems OK. The end remains exactly the same. Can you add a summary to the commit message, about the mappings we end up here with: - [0, 4K): flash, unmapped - [4K, 128M): flash, mapped as WB+X RAM - [128M, 256M): periferals, mapped as device - [256M, 1GB): 32-bit MMIO aperture, translated IO aperture, ECAM -- none of them mapped here just yet - [1GB, ...): RAM, mapped. ... So all my remarks are about documentation; please incorporate what you see fit. :) Thanks Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
