Re: [RFC PATCH 2/2] ArmVirtPkg/QemuVirtMemInfoLib: trim the MMIO region mapping

[Laszlo Ersek <lersek@redhat.com>]

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 <ard.biesheuvel@linaro.org>
> ---
>  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