[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v7 03/17] hw/arm/virt: Split the memory map desc
From: |
Auger Eric |
Subject: |
Re: [Qemu-devel] [PATCH v7 03/17] hw/arm/virt: Split the memory map description |
Date: |
Thu, 21 Feb 2019 18:21:11 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 |
Hi Igor,
On 2/21/19 5:19 PM, Igor Mammedov wrote:
> On Wed, 20 Feb 2019 23:39:49 +0100
> Eric Auger <address@hidden> 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 <address@hidden>
>> Reviewed-by: Peter Maydell <address@hidden>
>>
>> ---
>> 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)
>
>
Re: [Qemu-devel] [PATCH v7 03/17] hw/arm/virt: Split the memory map description, Heyi Guo, 2019/02/22
[Qemu-devel] [PATCH v7 04/17] hw/boards: Add a MachineState parameter to kvm_type callback, Eric Auger, 2019/02/20
[Qemu-devel] [PATCH v7 05/17] kvm: add kvm_arm_get_max_vm_ipa_size, Eric Auger, 2019/02/20
[Qemu-devel] [PATCH v7 06/17] vl: Set machine ram_size, maxram_size and ram_slots earlier, Eric Auger, 2019/02/20