| Message ID | 20190205173306.20483-15-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | ARM virt: Initial RAM expansion and PCDIMM/NVDIMM support | expand |
On Tue, 5 Feb 2019 18:33:02 +0100 Eric Auger <eric.auger@redhat.com> wrote: > The device memory region is located after the initial RAM. > its start/size are 1GB aligned. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > Signed-off-by: Kwangwoo Lee <kwangwoo.lee@sk.com> > > --- > v4 -> v5: > - device memory set after the initial RAM > > v3 -> v4: > - remove bootinfo.device_memory_start/device_memory_size > - rename VIRT_HOTPLUG_MEM into VIRT_DEVICE_MEM > --- > hw/arm/virt.c | 36 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index 783468ba77..b683902991 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -61,6 +61,7 @@ > #include "hw/arm/smmuv3.h" > #include "hw/mem/pc-dimm.h" > #include "hw/mem/nvdimm.h" > +#include "hw/acpi/acpi.h" > > #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \ > static void virt_##major##_##minor##_class_init(ObjectClass *oc, \ > @@ -1260,6 +1261,37 @@ static void create_secure_ram(VirtMachineState *vms, > g_free(nodename); > } > > +static void create_device_memory(VirtMachineState *vms, MemoryRegion *sysmem) > +{ > + MachineState *ms = MACHINE(vms); > + uint64_t device_memory_size = ms->maxram_size - ms->ram_size; should size it with 1Gb alignment per slot from the start (to avoid x86 mistakes), see enforce_aligned_dimm usage and associated commit for more details > + uint64_t align = GiB; > + > + if (!device_memory_size) { > + return; > + } > + > + if (ms->ram_slots > ACPI_MAX_RAM_SLOTS) { > + error_report("unsupported number of memory slots: %"PRIu64, > + ms->ram_slots); > + exit(EXIT_FAILURE); > + } > + > + if (QEMU_ALIGN_UP(ms->maxram_size, align) != ms->maxram_size) { > + error_report("maximum memory size must be aligned to multiple of 0x%" > + PRIx64, align); > + exit(EXIT_FAILURE); > + } > + > + ms->device_memory = g_malloc0(sizeof(*ms->device_memory)); > + ms->device_memory->base = QEMU_ALIGN_UP(GiB + ms->ram_size, GiB); ^^^ where does this come from? > + > + memory_region_init(&ms->device_memory->mr, OBJECT(vms), > + "device-memory", device_memory_size); > + memory_region_add_subregion(sysmem, ms->device_memory->base, > + &ms->device_memory->mr); > +} > + > static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size) > { > const VirtMachineState *board = container_of(binfo, VirtMachineState, > @@ -1569,6 +1601,10 @@ static void machvirt_init(MachineState *machine) > machine->ram_size); > memory_region_add_subregion(sysmem, vms->memmap[VIRT_MEM].base, ram); > > + if (vms->extended_memmap) { > + create_device_memory(vms, sysmem); > + } > + > create_flash(vms, sysmem, secure_sysmem ? secure_sysmem : sysmem); > > create_gic(vms, pic);
Hi Igor, On 2/18/19 10:31 AM, Igor Mammedov wrote: > On Tue, 5 Feb 2019 18:33:02 +0100 > Eric Auger <eric.auger@redhat.com> wrote: > >> The device memory region is located after the initial RAM. >> its start/size are 1GB aligned. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> Signed-off-by: Kwangwoo Lee <kwangwoo.lee@sk.com> >> >> --- >> v4 -> v5: >> - device memory set after the initial RAM >> >> v3 -> v4: >> - remove bootinfo.device_memory_start/device_memory_size >> - rename VIRT_HOTPLUG_MEM into VIRT_DEVICE_MEM >> --- >> hw/arm/virt.c | 36 ++++++++++++++++++++++++++++++++++++ >> 1 file changed, 36 insertions(+) >> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >> index 783468ba77..b683902991 100644 >> --- a/hw/arm/virt.c >> +++ b/hw/arm/virt.c >> @@ -61,6 +61,7 @@ >> #include "hw/arm/smmuv3.h" >> #include "hw/mem/pc-dimm.h" >> #include "hw/mem/nvdimm.h" >> +#include "hw/acpi/acpi.h" >> >> #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \ >> static void virt_##major##_##minor##_class_init(ObjectClass *oc, \ >> @@ -1260,6 +1261,37 @@ static void create_secure_ram(VirtMachineState *vms, >> g_free(nodename); >> } >> >> +static void create_device_memory(VirtMachineState *vms, MemoryRegion *sysmem) >> +{ >> + MachineState *ms = MACHINE(vms); >> + uint64_t device_memory_size = ms->maxram_size - ms->ram_size; > should size it with 1Gb alignment per slot from the start (to avoid x86 mistakes), > see enforce_aligned_dimm usage and associated commit for more details I don't understand the computation done in pc machine. eventually we are likely to have more device memory than requested by the user. Why don't we check (machine->maxram_size - machine->ram_size) >= machine->ram_slots * GiB instead of adding 1GiB/slot to the initial user requirements? Also machine->maxram_size - machine->ram_size is checked to be aligned with TARGET_PAGE_SIZE. Is TARGET_PAGE_SIZE representative of the guest PAGE in accelerated mode? Is it valid ro require an alignment on 1GB boundary as I do in this patch? > >> + uint64_t align = GiB; >> + >> + if (!device_memory_size) { >> + return; >> + } >> + >> + if (ms->ram_slots > ACPI_MAX_RAM_SLOTS) { >> + error_report("unsupported number of memory slots: %"PRIu64, >> + ms->ram_slots); >> + exit(EXIT_FAILURE); >> + } >> + >> + if (QEMU_ALIGN_UP(ms->maxram_size, align) != ms->maxram_size) { >> + error_report("maximum memory size must be aligned to multiple of 0x%" >> + PRIx64, align); >> + exit(EXIT_FAILURE); >> + } >> + >> + ms->device_memory = g_malloc0(sizeof(*ms->device_memory)); >> + ms->device_memory->base = QEMU_ALIGN_UP(GiB + ms->ram_size, GiB); > ^^^ where does this come from? OK, introduced RAMBASE macro Thanks Eric > > >> + >> + memory_region_init(&ms->device_memory->mr, OBJECT(vms), >> + "device-memory", device_memory_size); >> + memory_region_add_subregion(sysmem, ms->device_memory->base, >> + &ms->device_memory->mr); >> +} >> + >> static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size) >> { >> const VirtMachineState *board = container_of(binfo, VirtMachineState, >> @@ -1569,6 +1601,10 @@ static void machvirt_init(MachineState *machine) >> machine->ram_size); >> memory_region_add_subregion(sysmem, vms->memmap[VIRT_MEM].base, ram); >> >> + if (vms->extended_memmap) { >> + create_device_memory(vms, sysmem); >> + } >> + >> create_flash(vms, sysmem, secure_sysmem ? secure_sysmem : sysmem); >> >> create_gic(vms, pic); >
On 19.02.19 16:53, Auger Eric wrote: > Hi Igor, > > On 2/18/19 10:31 AM, Igor Mammedov wrote: >> On Tue, 5 Feb 2019 18:33:02 +0100 >> Eric Auger <eric.auger@redhat.com> wrote: >> >>> The device memory region is located after the initial RAM. >>> its start/size are 1GB aligned. >>> >>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>> Signed-off-by: Kwangwoo Lee <kwangwoo.lee@sk.com> >>> >>> --- >>> v4 -> v5: >>> - device memory set after the initial RAM >>> >>> v3 -> v4: >>> - remove bootinfo.device_memory_start/device_memory_size >>> - rename VIRT_HOTPLUG_MEM into VIRT_DEVICE_MEM >>> --- >>> hw/arm/virt.c | 36 ++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 36 insertions(+) >>> >>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >>> index 783468ba77..b683902991 100644 >>> --- a/hw/arm/virt.c >>> +++ b/hw/arm/virt.c >>> @@ -61,6 +61,7 @@ >>> #include "hw/arm/smmuv3.h" >>> #include "hw/mem/pc-dimm.h" >>> #include "hw/mem/nvdimm.h" >>> +#include "hw/acpi/acpi.h" >>> >>> #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \ >>> static void virt_##major##_##minor##_class_init(ObjectClass *oc, \ >>> @@ -1260,6 +1261,37 @@ static void create_secure_ram(VirtMachineState *vms, >>> g_free(nodename); >>> } >>> >>> +static void create_device_memory(VirtMachineState *vms, MemoryRegion *sysmem) >>> +{ >>> + MachineState *ms = MACHINE(vms); >>> + uint64_t device_memory_size = ms->maxram_size - ms->ram_size; >> should size it with 1Gb alignment per slot from the start (to avoid x86 mistakes), >> see enforce_aligned_dimm usage and associated commit for more details > I don't understand the computation done in pc machine. eventually we are > likely to have more device memory than requested by the user. Why don't > we check (machine->maxram_size - machine->ram_size) >= > machine->ram_slots * GiB > instead of adding 1GiB/slot to the initial user requirements? This is to be able to potentially align each slot as far as I know, so the "memory device address space" cannot that easily be fragmented. E.g. Linux requires a certain alignment to make full use of a DIMM. > > Also machine->maxram_size - machine->ram_size is checked to be aligned > with TARGET_PAGE_SIZE. Is TARGET_PAGE_SIZE representative of the guest > PAGE in accelerated mode? Is it valid ro require an alignment on 1GB > boundary as I do in this patch? I guess the alignment check is only done because for that target, anything having sub-page granularity cannot be used either way.
On Tue, 19 Feb 2019 16:53:22 +0100 Auger Eric <eric.auger@redhat.com> wrote: > Hi Igor, > > On 2/18/19 10:31 AM, Igor Mammedov wrote: > > On Tue, 5 Feb 2019 18:33:02 +0100 > > Eric Auger <eric.auger@redhat.com> wrote: > > > >> The device memory region is located after the initial RAM. > >> its start/size are 1GB aligned. > >> > >> Signed-off-by: Eric Auger <eric.auger@redhat.com> > >> Signed-off-by: Kwangwoo Lee <kwangwoo.lee@sk.com> > >> > >> --- > >> v4 -> v5: > >> - device memory set after the initial RAM > >> > >> v3 -> v4: > >> - remove bootinfo.device_memory_start/device_memory_size > >> - rename VIRT_HOTPLUG_MEM into VIRT_DEVICE_MEM > >> --- > >> hw/arm/virt.c | 36 ++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 36 insertions(+) > >> > >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c > >> index 783468ba77..b683902991 100644 > >> --- a/hw/arm/virt.c > >> +++ b/hw/arm/virt.c > >> @@ -61,6 +61,7 @@ > >> #include "hw/arm/smmuv3.h" > >> #include "hw/mem/pc-dimm.h" > >> #include "hw/mem/nvdimm.h" > >> +#include "hw/acpi/acpi.h" > >> > >> #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \ > >> static void virt_##major##_##minor##_class_init(ObjectClass *oc, \ > >> @@ -1260,6 +1261,37 @@ static void create_secure_ram(VirtMachineState *vms, > >> g_free(nodename); > >> } > >> > >> +static void create_device_memory(VirtMachineState *vms, MemoryRegion *sysmem) > >> +{ > >> + MachineState *ms = MACHINE(vms); > >> + uint64_t device_memory_size = ms->maxram_size - ms->ram_size; > > should size it with 1Gb alignment per slot from the start (to avoid x86 mistakes), > > see enforce_aligned_dimm usage and associated commit for more details > I don't understand the computation done in pc machine. eventually we are > likely to have more device memory than requested by the user. Why don't > we check (machine->maxram_size - machine->ram_size) >= > machine->ram_slots * GiB > instead of adding 1GiB/slot to the initial user requirements? > > Also machine->maxram_size - machine->ram_size is checked to be aligned > with TARGET_PAGE_SIZE. Is TARGET_PAGE_SIZE representative of the guest > PAGE in accelerated mode? Is it valid ro require an alignment on 1GB > boundary as I do in this patch? See commit 085f8e88b for explanation, What we are basically are doing there is sizing hotpluggbale address space to allow max possible huge page aligned DIMM to be successfully plugged in even if address space if fragmented. > > > > >> + uint64_t align = GiB; > >> + > >> + if (!device_memory_size) { > >> + return; > >> + } > >> + > >> + if (ms->ram_slots > ACPI_MAX_RAM_SLOTS) { > >> + error_report("unsupported number of memory slots: %"PRIu64, > >> + ms->ram_slots); > >> + exit(EXIT_FAILURE); > >> + } > >> + > >> + if (QEMU_ALIGN_UP(ms->maxram_size, align) != ms->maxram_size) { > >> + error_report("maximum memory size must be aligned to multiple of 0x%" > >> + PRIx64, align); > >> + exit(EXIT_FAILURE); > >> + } > >> + > >> + ms->device_memory = g_malloc0(sizeof(*ms->device_memory)); > >> + ms->device_memory->base = QEMU_ALIGN_UP(GiB + ms->ram_size, GiB); > > ^^^ where does this come from? > OK, introduced RAMBASE macro > > Thanks > > Eric > > > > > >> + > >> + memory_region_init(&ms->device_memory->mr, OBJECT(vms), > >> + "device-memory", device_memory_size); > >> + memory_region_add_subregion(sysmem, ms->device_memory->base, > >> + &ms->device_memory->mr); > >> +} > >> + > >> static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size) > >> { > >> const VirtMachineState *board = container_of(binfo, VirtMachineState, > >> @@ -1569,6 +1601,10 @@ static void machvirt_init(MachineState *machine) > >> machine->ram_size); > >> memory_region_add_subregion(sysmem, vms->memmap[VIRT_MEM].base, ram); > >> > >> + if (vms->extended_memmap) { > >> + create_device_memory(vms, sysmem); > >> + } > >> + > >> create_flash(vms, sysmem, secure_sysmem ? secure_sysmem : sysmem); > >> > >> create_gic(vms, pic); > > >
Hi Igor, On 2/21/19 10:36 AM, Igor Mammedov wrote: > On Tue, 19 Feb 2019 16:53:22 +0100 > Auger Eric <eric.auger@redhat.com> wrote: > >> Hi Igor, >> >> On 2/18/19 10:31 AM, Igor Mammedov wrote: >>> On Tue, 5 Feb 2019 18:33:02 +0100 >>> Eric Auger <eric.auger@redhat.com> wrote: >>> >>>> The device memory region is located after the initial RAM. >>>> its start/size are 1GB aligned. >>>> >>>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>>> Signed-off-by: Kwangwoo Lee <kwangwoo.lee@sk.com> >>>> >>>> --- >>>> v4 -> v5: >>>> - device memory set after the initial RAM >>>> >>>> v3 -> v4: >>>> - remove bootinfo.device_memory_start/device_memory_size >>>> - rename VIRT_HOTPLUG_MEM into VIRT_DEVICE_MEM >>>> --- >>>> hw/arm/virt.c | 36 ++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 36 insertions(+) >>>> >>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >>>> index 783468ba77..b683902991 100644 >>>> --- a/hw/arm/virt.c >>>> +++ b/hw/arm/virt.c >>>> @@ -61,6 +61,7 @@ >>>> #include "hw/arm/smmuv3.h" >>>> #include "hw/mem/pc-dimm.h" >>>> #include "hw/mem/nvdimm.h" >>>> +#include "hw/acpi/acpi.h" >>>> >>>> #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \ >>>> static void virt_##major##_##minor##_class_init(ObjectClass *oc, \ >>>> @@ -1260,6 +1261,37 @@ static void create_secure_ram(VirtMachineState *vms, >>>> g_free(nodename); >>>> } >>>> >>>> +static void create_device_memory(VirtMachineState *vms, MemoryRegion *sysmem) >>>> +{ >>>> + MachineState *ms = MACHINE(vms); >>>> + uint64_t device_memory_size = ms->maxram_size - ms->ram_size; >>> should size it with 1Gb alignment per slot from the start (to avoid x86 mistakes), >>> see enforce_aligned_dimm usage and associated commit for more details >> I don't understand the computation done in pc machine. eventually we are >> likely to have more device memory than requested by the user. Why don't >> we check (machine->maxram_size - machine->ram_size) >= >> machine->ram_slots * GiB >> instead of adding 1GiB/slot to the initial user requirements? >> >> Also machine->maxram_size - machine->ram_size is checked to be aligned >> with TARGET_PAGE_SIZE. Is TARGET_PAGE_SIZE representative of the guest >> PAGE in accelerated mode? Is it valid ro require an alignment on 1GB >> boundary as I do in this patch? > See commit 085f8e88b for explanation, > What we are basically are doing there is sizing hotpluggbale address space > to allow max possible huge page aligned DIMM to be successfully plugged in > even if address space if fragmented. In v7, I also added ram_slots * GiB to (maxram_size - ram_size). Thanks Eric > >> >>> >>>> + uint64_t align = GiB; >>>> + >>>> + if (!device_memory_size) { >>>> + return; >>>> + } >>>> + >>>> + if (ms->ram_slots > ACPI_MAX_RAM_SLOTS) { >>>> + error_report("unsupported number of memory slots: %"PRIu64, >>>> + ms->ram_slots); >>>> + exit(EXIT_FAILURE); >>>> + } >>>> + >>>> + if (QEMU_ALIGN_UP(ms->maxram_size, align) != ms->maxram_size) { >>>> + error_report("maximum memory size must be aligned to multiple of 0x%" >>>> + PRIx64, align); >>>> + exit(EXIT_FAILURE); >>>> + } >>>> + >>>> + ms->device_memory = g_malloc0(sizeof(*ms->device_memory)); >>>> + ms->device_memory->base = QEMU_ALIGN_UP(GiB + ms->ram_size, GiB); >>> ^^^ where does this come from? >> OK, introduced RAMBASE macro >> >> Thanks >> >> Eric >>> >>> >>>> + >>>> + memory_region_init(&ms->device_memory->mr, OBJECT(vms), >>>> + "device-memory", device_memory_size); >>>> + memory_region_add_subregion(sysmem, ms->device_memory->base, >>>> + &ms->device_memory->mr); >>>> +} >>>> + >>>> static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size) >>>> { >>>> const VirtMachineState *board = container_of(binfo, VirtMachineState, >>>> @@ -1569,6 +1601,10 @@ static void machvirt_init(MachineState *machine) >>>> machine->ram_size); >>>> memory_region_add_subregion(sysmem, vms->memmap[VIRT_MEM].base, ram); >>>> >>>> + if (vms->extended_memmap) { >>>> + create_device_memory(vms, sysmem); >>>> + } >>>> + >>>> create_flash(vms, sysmem, secure_sysmem ? secure_sysmem : sysmem); >>>> >>>> create_gic(vms, pic); >>> >> > >
On 21.02.19 13:37, Auger Eric wrote: > Hi Igor, > > On 2/21/19 10:36 AM, Igor Mammedov wrote: >> On Tue, 19 Feb 2019 16:53:22 +0100 >> Auger Eric <eric.auger@redhat.com> wrote: >> >>> Hi Igor, >>> >>> On 2/18/19 10:31 AM, Igor Mammedov wrote: >>>> On Tue, 5 Feb 2019 18:33:02 +0100 >>>> Eric Auger <eric.auger@redhat.com> wrote: >>>> >>>>> The device memory region is located after the initial RAM. >>>>> its start/size are 1GB aligned. >>>>> >>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>>>> Signed-off-by: Kwangwoo Lee <kwangwoo.lee@sk.com> >>>>> >>>>> --- >>>>> v4 -> v5: >>>>> - device memory set after the initial RAM >>>>> >>>>> v3 -> v4: >>>>> - remove bootinfo.device_memory_start/device_memory_size >>>>> - rename VIRT_HOTPLUG_MEM into VIRT_DEVICE_MEM >>>>> --- >>>>> hw/arm/virt.c | 36 ++++++++++++++++++++++++++++++++++++ >>>>> 1 file changed, 36 insertions(+) >>>>> >>>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >>>>> index 783468ba77..b683902991 100644 >>>>> --- a/hw/arm/virt.c >>>>> +++ b/hw/arm/virt.c >>>>> @@ -61,6 +61,7 @@ >>>>> #include "hw/arm/smmuv3.h" >>>>> #include "hw/mem/pc-dimm.h" >>>>> #include "hw/mem/nvdimm.h" >>>>> +#include "hw/acpi/acpi.h" >>>>> >>>>> #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \ >>>>> static void virt_##major##_##minor##_class_init(ObjectClass *oc, \ >>>>> @@ -1260,6 +1261,37 @@ static void create_secure_ram(VirtMachineState *vms, >>>>> g_free(nodename); >>>>> } >>>>> >>>>> +static void create_device_memory(VirtMachineState *vms, MemoryRegion *sysmem) >>>>> +{ >>>>> + MachineState *ms = MACHINE(vms); >>>>> + uint64_t device_memory_size = ms->maxram_size - ms->ram_size; >>>> should size it with 1Gb alignment per slot from the start (to avoid x86 mistakes), >>>> see enforce_aligned_dimm usage and associated commit for more details >>> I don't understand the computation done in pc machine. eventually we are >>> likely to have more device memory than requested by the user. Why don't >>> we check (machine->maxram_size - machine->ram_size) >= >>> machine->ram_slots * GiB >>> instead of adding 1GiB/slot to the initial user requirements? >>> >>> Also machine->maxram_size - machine->ram_size is checked to be aligned >>> with TARGET_PAGE_SIZE. Is TARGET_PAGE_SIZE representative of the guest >>> PAGE in accelerated mode? Is it valid ro require an alignment on 1GB >>> boundary as I do in this patch? >> See commit 085f8e88b for explanation, >> What we are basically are doing there is sizing hotpluggbale address space >> to allow max possible huge page aligned DIMM to be successfully plugged in >> even if address space if fragmented. > In v7, I also added ram_slots * GiB to (maxram_size - ram_size). > Depending on the way the system handles it, this might be confusing for the end user and has to be documented somewhere. E.g. if there are certain memory limits (say 2TB) and the user specifies something like "maxmem=2TB,slots=20" it might be confusing if he gets an error like "more than 2TB are not supported". > Thanks > > Eric
Hi David, On 2/21/19 1:44 PM, David Hildenbrand wrote: > On 21.02.19 13:37, Auger Eric wrote: >> Hi Igor, >> >> On 2/21/19 10:36 AM, Igor Mammedov wrote: >>> On Tue, 19 Feb 2019 16:53:22 +0100 >>> Auger Eric <eric.auger@redhat.com> wrote: >>> >>>> Hi Igor, >>>> >>>> On 2/18/19 10:31 AM, Igor Mammedov wrote: >>>>> On Tue, 5 Feb 2019 18:33:02 +0100 >>>>> Eric Auger <eric.auger@redhat.com> wrote: >>>>> >>>>>> The device memory region is located after the initial RAM. >>>>>> its start/size are 1GB aligned. >>>>>> >>>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>>>>> Signed-off-by: Kwangwoo Lee <kwangwoo.lee@sk.com> >>>>>> >>>>>> --- >>>>>> v4 -> v5: >>>>>> - device memory set after the initial RAM >>>>>> >>>>>> v3 -> v4: >>>>>> - remove bootinfo.device_memory_start/device_memory_size >>>>>> - rename VIRT_HOTPLUG_MEM into VIRT_DEVICE_MEM >>>>>> --- >>>>>> hw/arm/virt.c | 36 ++++++++++++++++++++++++++++++++++++ >>>>>> 1 file changed, 36 insertions(+) >>>>>> >>>>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >>>>>> index 783468ba77..b683902991 100644 >>>>>> --- a/hw/arm/virt.c >>>>>> +++ b/hw/arm/virt.c >>>>>> @@ -61,6 +61,7 @@ >>>>>> #include "hw/arm/smmuv3.h" >>>>>> #include "hw/mem/pc-dimm.h" >>>>>> #include "hw/mem/nvdimm.h" >>>>>> +#include "hw/acpi/acpi.h" >>>>>> >>>>>> #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \ >>>>>> static void virt_##major##_##minor##_class_init(ObjectClass *oc, \ >>>>>> @@ -1260,6 +1261,37 @@ static void create_secure_ram(VirtMachineState *vms, >>>>>> g_free(nodename); >>>>>> } >>>>>> >>>>>> +static void create_device_memory(VirtMachineState *vms, MemoryRegion *sysmem) >>>>>> +{ >>>>>> + MachineState *ms = MACHINE(vms); >>>>>> + uint64_t device_memory_size = ms->maxram_size - ms->ram_size; >>>>> should size it with 1Gb alignment per slot from the start (to avoid x86 mistakes), >>>>> see enforce_aligned_dimm usage and associated commit for more details >>>> I don't understand the computation done in pc machine. eventually we are >>>> likely to have more device memory than requested by the user. Why don't >>>> we check (machine->maxram_size - machine->ram_size) >= >>>> machine->ram_slots * GiB >>>> instead of adding 1GiB/slot to the initial user requirements? >>>> >>>> Also machine->maxram_size - machine->ram_size is checked to be aligned >>>> with TARGET_PAGE_SIZE. Is TARGET_PAGE_SIZE representative of the guest >>>> PAGE in accelerated mode? Is it valid ro require an alignment on 1GB >>>> boundary as I do in this patch? >>> See commit 085f8e88b for explanation, >>> What we are basically are doing there is sizing hotpluggbale address space >>> to allow max possible huge page aligned DIMM to be successfully plugged in >>> even if address space if fragmented. >> In v7, I also added ram_slots * GiB to (maxram_size - ram_size). >> > > Depending on the way the system handles it, this might be confusing for > the end user and has to be documented somewhere. > > E.g. if there are certain memory limits (say 2TB) and the user specifies > something like "maxmem=2TB,slots=20" it might be confusing if he gets an > error like "more than 2TB are not supported". I Agree. On ARM we also intend to put high IO regions above the RAM so if we overshoot the host limit, we warn the user the memory map requires more PA bits than the host can support and the end user is invited to lower maxmem/slots. Thanks Eric > >> Thanks >> >> Eric
diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 783468ba77..b683902991 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -61,6 +61,7 @@ #include "hw/arm/smmuv3.h" #include "hw/mem/pc-dimm.h" #include "hw/mem/nvdimm.h" +#include "hw/acpi/acpi.h" #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \ static void virt_##major##_##minor##_class_init(ObjectClass *oc, \ @@ -1260,6 +1261,37 @@ static void create_secure_ram(VirtMachineState *vms, g_free(nodename); } +static void create_device_memory(VirtMachineState *vms, MemoryRegion *sysmem) +{ + MachineState *ms = MACHINE(vms); + uint64_t device_memory_size = ms->maxram_size - ms->ram_size; + uint64_t align = GiB; + + if (!device_memory_size) { + return; + } + + if (ms->ram_slots > ACPI_MAX_RAM_SLOTS) { + error_report("unsupported number of memory slots: %"PRIu64, + ms->ram_slots); + exit(EXIT_FAILURE); + } + + if (QEMU_ALIGN_UP(ms->maxram_size, align) != ms->maxram_size) { + error_report("maximum memory size must be aligned to multiple of 0x%" + PRIx64, align); + exit(EXIT_FAILURE); + } + + ms->device_memory = g_malloc0(sizeof(*ms->device_memory)); + ms->device_memory->base = QEMU_ALIGN_UP(GiB + ms->ram_size, GiB); + + memory_region_init(&ms->device_memory->mr, OBJECT(vms), + "device-memory", device_memory_size); + memory_region_add_subregion(sysmem, ms->device_memory->base, + &ms->device_memory->mr); +} + static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size) { const VirtMachineState *board = container_of(binfo, VirtMachineState, @@ -1569,6 +1601,10 @@ static void machvirt_init(MachineState *machine) machine->ram_size); memory_region_add_subregion(sysmem, vms->memmap[VIRT_MEM].base, ram); + if (vms->extended_memmap) { + create_device_memory(vms, sysmem); + } + create_flash(vms, sysmem, secure_sysmem ? secure_sysmem : sysmem); create_gic(vms, pic);