On Thu, 21 Feb 2019 18:21:11 +0100
Auger Eric <eric.au...@redhat.com> wrote:
> 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.
Yep, that's the trick.
It is too much subtle for my taste,
is it possible to make extended_memmap sizing more explicit/trivial,
so one could see it right away without figuring out how indexes influence array
size?
> /* 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)
> >
> >
