Hi Igor, On 2/21/19 5:19 PM, Igor Mammedov wrote: > On Wed, 20 Feb 2019 23:39:49 +0100 > Eric Auger <eric.au...@redhat.com> wrote: > >> In the prospect to introduce an extended memory map supporting more >> RAM, let's split the memory map array into two parts: >> >> - the former a15memmap contains regions below and including the RAM > >> - extended_memmap, only initialized with entries located after the RAM. >> Only the size of the region is initialized there since their base >> address will be dynamically computed, depending on the top of the >> RAM (initial RAM at the moment), with same alignment as their size. > can't parse this part and pinpoint what is 'their', care to rephrase? Only the size of the High IO region entries is initialized (there are currently 3 entries: VIRT_HIGH_GIC_REDIST2, VIRT_HIGH_PCIE_ECAM, VIRT_HIGH_PCIE_MMIO). The base address is dynamically computed so it is not initialized. > > >> This new split will allow to grow the RAM size without changing the >> description of the high regions. >> >> The patch also moves the memory map setup > s/moves/makes/ > s/$/dynamic and moves it/ > >> into machvirt_init(). > >> The rationale is the memory map will be soon affected by the > >> kvm_type() call that happens after virt_instance_init() and > is dependency on kvm_type() still valid, > shouldn't split memmap work for TCG just fine as well? See in 08/17: in TCG mode the memory map will be "frozen" (set_memmap) in machvirt_init. Otherwise set_memmap is called from kvm_type().
Split memmap works both in TCG and in accelerated mode. I will rephrase the commit message. > >> before machvirt_init(). >> >> The memory map is unchanged (the top of the initial RAM still is >> 256GiB). Then come the high IO regions with same layout as before. >> >> Signed-off-by: Eric Auger <eric.au...@redhat.com> >> Reviewed-by: Peter Maydell <peter.mayd...@linaro.org> >> >> --- >> v6 -> v7: >> - s/a15memmap/base_memmap >> - slight rewording of the commit message >> - add "if there is less than 256GiB of RAM then the floating area >> starts at the 256GiB mark" in the comment associated to the floating >> memory map >> - Added Peter's R-b >> >> v5 -> v6 >> - removal of many macros in units.h >> - introduce the virt_set_memmap helper >> - new computation for offsets of high IO regions >> - add comments >> --- >> hw/arm/virt.c | 48 +++++++++++++++++++++++++++++++++++++------ >> include/hw/arm/virt.h | 14 +++++++++---- >> 2 files changed, 52 insertions(+), 10 deletions(-) >> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >> index a1955e7764..12039a0367 100644 >> --- a/hw/arm/virt.c >> +++ b/hw/arm/virt.c >> @@ -29,6 +29,7 @@ >> */ >> >> #include "qemu/osdep.h" >> +#include "qemu/units.h" >> #include "qapi/error.h" >> #include "hw/sysbus.h" >> #include "hw/arm/arm.h" >> @@ -121,7 +122,7 @@ >> * Note that devices should generally be placed at multiples of 0x10000, >> * to accommodate guests using 64K pages. >> */ >> -static const MemMapEntry a15memmap[] = { >> +static const MemMapEntry base_memmap[] = { >> /* Space up to 0x8000000 is reserved for a boot ROM */ >> [VIRT_FLASH] = { 0, 0x08000000 }, >> [VIRT_CPUPERIPHS] = { 0x08000000, 0x00020000 }, >> @@ -149,11 +150,21 @@ static const MemMapEntry a15memmap[] = { >> [VIRT_PCIE_PIO] = { 0x3eff0000, 0x00010000 }, >> [VIRT_PCIE_ECAM] = { 0x3f000000, 0x01000000 }, >> [VIRT_MEM] = { 0x40000000, RAMLIMIT_BYTES }, >> +}; >> + >> +/* >> + * Highmem IO Regions: This memory map is floating, located after the RAM. >> + * Each IO region offset will be dynamically computed, depending on the > s/IO region offset/MemMapEntry base (GPA)/ >> + * top of the RAM, so that its base get the same alignment as the size, > >> + * ie. a 512GiB region will be aligned on a 512GiB boundary. If there is > s/region/entry/ > >> + * less than 256GiB of RAM, the floating area starts at the 256GiB mark. >> + */ >> +static MemMapEntry extended_memmap[] = { >> /* Additional 64 MB redist region (can contain up to 512 >> redistributors) */ >> - [VIRT_HIGH_GIC_REDIST2] = { 0x4000000000ULL, 0x4000000 }, >> - [VIRT_HIGH_PCIE_ECAM] = { 0x4010000000ULL, 0x10000000 }, >> - /* Second PCIe window, 512GB wide at the 512GB boundary */ >> - [VIRT_HIGH_PCIE_MMIO] = { 0x8000000000ULL, 0x8000000000ULL }, >> + [VIRT_HIGH_GIC_REDIST2] = { 0x0, 64 * MiB }, >> + [VIRT_HIGH_PCIE_ECAM] = { 0x0, 256 * MiB }, >> + /* Second PCIe window */ >> + [VIRT_HIGH_PCIE_MMIO] = { 0x0, 512 * GiB }, >> }; >> >> static const int a15irqmap[] = { >> @@ -1354,6 +1365,30 @@ static uint64_t virt_cpu_mp_affinity(VirtMachineState >> *vms, int idx) >> return arm_cpu_mp_affinity(idx, clustersz); >> } >> >> +static void virt_set_memmap(VirtMachineState *vms) >> +{ >> + hwaddr base; >> + int i; >> + >> + vms->memmap = extended_memmap; > I probably don't see something but ... > >> + >> + for (i = 0; i < ARRAY_SIZE(base_memmap); i++) { >> + vms->memmap[i] = base_memmap[i]; > > ARRAY_SIZE(base_memmap) > 3 > ARRAY_SIZE(extended_memmap) == 3 > as result shouldn't we observe OOB at vms->memmap[i] access > starting from i==3 ? ARRAY_SIZE(extended_memmap) = ARRAY_SIZE(base_memmap) + 3 VIRT_HIGH_GIC_REDIST2 = VIRT_LOWMEMMAP_LAST is what you miss. /* indices of IO regions located after the RAM */ enum { VIRT_HIGH_GIC_REDIST2 = VIRT_LOWMEMMAP_LAST, VIRT_HIGH_PCIE_ECAM, VIRT_HIGH_PCIE_MMIO, }; > >> + } >> + >> + vms->high_io_base = 256 * GiB; /* Top of the legacy initial RAM region >> */ >> + base = vms->high_io_base; >> + >> + for (i = VIRT_LOWMEMMAP_LAST; i < ARRAY_SIZE(extended_memmap); i++) { > not sure why VIRT_LOWMEMMAP_LAST is needed at all, one could just continue > with current 'i' value, provided extended_memmap wasn't corrupted by previous > loop. Yep maybe. But I think it is less error prone like this if someone later on adds some intermediate manipulation on i. > And does this loop ever executes? VIRT_LOWMEMMAP_LAST > > ARRAY_SIZE(extended_memmap) yes it does Thanks Eric > >> + hwaddr size = extended_memmap[i].size; >> + >> + base = ROUND_UP(base, size); >> + vms->memmap[i].base = base; >> + vms->memmap[i].size = size; >> + base += size; >> + } >> +} >> + >> static void machvirt_init(MachineState *machine) >> { >> VirtMachineState *vms = VIRT_MACHINE(machine); >> @@ -1368,6 +1403,8 @@ static void machvirt_init(MachineState *machine) >> bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0); >> bool aarch64 = true; >> >> + virt_set_memmap(vms); >> + >> /* We can probe only here because during property set >> * KVM is not available yet >> */ >> @@ -1843,7 +1880,6 @@ static void virt_instance_init(Object *obj) >> "Valid values are none and smmuv3", >> NULL); >> >> - vms->memmap = a15memmap; >> vms->irqmap = a15irqmap; >> } >> >> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h >> index a27086d524..3dc7a6c5d5 100644 >> --- a/include/hw/arm/virt.h >> +++ b/include/hw/arm/virt.h >> @@ -64,7 +64,6 @@ enum { >> VIRT_GIC_VCPU, >> VIRT_GIC_ITS, >> VIRT_GIC_REDIST, >> - VIRT_HIGH_GIC_REDIST2, >> VIRT_SMMU, >> VIRT_UART, >> VIRT_MMIO, >> @@ -74,12 +73,18 @@ enum { >> VIRT_PCIE_MMIO, >> VIRT_PCIE_PIO, >> VIRT_PCIE_ECAM, >> - VIRT_HIGH_PCIE_ECAM, >> VIRT_PLATFORM_BUS, >> - VIRT_HIGH_PCIE_MMIO, >> VIRT_GPIO, >> VIRT_SECURE_UART, >> VIRT_SECURE_MEM, >> + VIRT_LOWMEMMAP_LAST, >> +}; >> + >> +/* indices of IO regions located after the RAM */ >> +enum { >> + VIRT_HIGH_GIC_REDIST2 = VIRT_LOWMEMMAP_LAST, >> + VIRT_HIGH_PCIE_ECAM, >> + VIRT_HIGH_PCIE_MMIO, >> }; >> >> typedef enum VirtIOMMUType { >> @@ -116,7 +121,7 @@ typedef struct { >> int32_t gic_version; >> VirtIOMMUType iommu; >> struct arm_boot_info bootinfo; >> - const MemMapEntry *memmap; >> + MemMapEntry *memmap; >> const int *irqmap; >> int smp_cpus; >> void *fdt; >> @@ -126,6 +131,7 @@ typedef struct { >> uint32_t msi_phandle; >> uint32_t iommu_phandle; >> int psci_conduit; >> + hwaddr high_io_base; >> } VirtMachineState; >> >> #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM : VIRT_PCIE_ECAM) > >