Re: [PATCH v2 10/14] ArmVirtPkg/ArmVirtXen: add ArmVirtMemInfoLib implementation

[Julien Grall <julien.grall@linaro.org>]

Hi Ard,

On 11/22/2017 10:07 AM, Ard Biesheuvel wrote:
> Clone the existing ArmPlatformGetVirtualMemoryMap () for this platform,
> clean it up slightly (by using a static buffer rather than a heap
> allocation, and removing the support for uncached DRAM mappings), and
> turn it into a new ArmVirtMemInfoLib implementation.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Acked-by: Laszlo Ersek <lersek@redhat.com>
> Cc: Julien Grall <julien.grall@linaro.org>

I have tested the series on both Arm32 and Arm64 guests:

Tested-by: Julien Grall <julien.grall@linaro.org>

[...]

> +VOID
> +EFIAPI
> +ArmVirtGetMemoryMap (
> +  OUT ARM_MEMORY_REGION_DESCRIPTOR   **VirtualMemoryMap
> +  )
> +{
> +  ASSERT (VirtualMemoryMap != NULL);
> +
> +  //
> +  // Map the entire physical memory space as cached. The only device
> +  // we care about is the GIC, which will be stage 2 mapped as a device
> +  // by the hypervisor, overriding the cached mapping we install here.
> +  //

I was about to complain about the fact you rely on the hypervisor 
setting correct stage-2 memory attribute. But I see this is a copy of 
the current code :).

We did relax the attribute for MMIO in the case of the Hardware Domain. 
I will keep the UEFI implementation in mind if we ever decide to relax 
for guests too.


> +  mVirtualMemoryTable[0].PhysicalBase = 0x0;
> +  mVirtualMemoryTable[0].VirtualBase  = 0x0;
> +  mVirtualMemoryTable[0].Length       = ArmGetPhysAddrTop ();
> +  mVirtualMemoryTable[0].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> +
> +  mVirtualMemoryTable[1].PhysicalBase = 0x0;
> +  mVirtualMemoryTable[1].VirtualBase  = 0x0;
> +  mVirtualMemoryTable[1].Length       = 0x0;
> +  mVirtualMemoryTable[1].Attributes   = 0x0;
> +
> +  *VirtualMemoryMap = mVirtualMemoryTable;
> +}

Cheers,

-- 
Julien Grall