| Message ID | 53707BB3.1060904@de.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 05/12/2014 03:43 AM, Christian Borntraeger wrote: > On 07/05/14 20:05, Matthew Rosato wrote: >> When determining the memory increment size, use the maxmem size if >> it was specified. >> >> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> >> --- >> hw/s390x/s390-virtio-ccw.c | 44 ++++++++++++++++++++++++++++++++++++-------- >> target-s390x/cpu.h | 3 +++ >> 2 files changed, 39 insertions(+), 8 deletions(-) >> >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >> index 0d4f6ae..a8be0f7 100644 >> --- a/hw/s390x/s390-virtio-ccw.c >> +++ b/hw/s390x/s390-virtio-ccw.c >> @@ -17,6 +17,7 @@ >> #include "ioinst.h" >> #include "css.h" >> #include "virtio-ccw.h" >> +#include "qemu/config-file.h" >> >> void io_subsystem_reset(void) >> { >> @@ -84,17 +85,33 @@ static void ccw_init(QEMUMachineInitArgs *args) >> ram_addr_t my_ram_size = args->ram_size; >> MemoryRegion *sysmem = get_system_memory(); >> MemoryRegion *ram = g_new(MemoryRegion, 1); >> - int shift = 0; >> + sclpMemoryHotplugDev *mhd = get_sclp_memory_hotplug_dev(); >> uint8_t *storage_keys; >> int ret; >> VirtualCssBus *css_bus; >> - >> - /* s390x ram size detection needs a 16bit multiplier + an increment. So >> - guests > 64GB can be specified in 2MB steps etc. */ >> - while ((my_ram_size >> (20 + shift)) > 65535) { >> - shift++; >> + QemuOpts *opts = qemu_opts_find(qemu_find_opts("memory"), NULL); >> + ram_addr_t pad_size = 0; >> + ram_addr_t maxmem = qemu_opt_get_size(opts, "maxmem", 0); >> + ram_addr_t standby_mem_size = maxmem - my_ram_size; >> + >> + /* The storage increment size is a multiple of 1M and is a power of 2. >> + * The number of storage increments must be MAX_STORAGE_INCREMENTS or fewer. >> + * The variable 'mhd->increment_size' is an exponent of 2 that can be >> + * used to calculate the size (in bytes) of an increment. */ >> + mhd->increment_size = 20; >> + while ((my_ram_size >> mhd->increment_size) > MAX_STORAGE_INCREMENTS) { >> + mhd->increment_size++; >> + } >> + while ((standby_mem_size >> mhd->increment_size) > MAX_STORAGE_INCREMENTS) { >> + mhd->increment_size++; >> } > > Looking back into the mail thread, Alex requested to make the code for standy/non-standby identical. > Now: The limit of 1020 (MAX_STORAGE_INCREMENTS) is only given if standby memory exists. Without standby memory, we could still used 64k as a divider.(zVM also does only impose this limit with standby memory). > What does that mean: With this patch the memory size granularity gets bigger. e.g. a guest can have > 1019MB, 1020MB, 1022MB, 1024MB and so on (1021MB is no longer possible, but it was before). Hmm, this is a good point. I didn't think about it when I made the change per Alex. If nobody has a strong opinion here, I think I'd rather go back to special casing this in the next version, to keep the 'normal case' (without standby memory) more robust. > > We could now special case this or just leave it as is in this patch to make the code simpler. I think this is not a big problem, but we should add this to the comment and maybe also to the help text of the command line option (e.g. > > diff --git a/qemu-options.hx b/qemu-options.hx > index 5edffa6..8c71283 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -217,7 +217,8 @@ DEF("m", HAS_ARG, QEMU_OPTION_m, > "-m [size=]megs\n" > " configure guest RAM\n" > " size: initial amount of guest memory (default: " > - stringify(DEFAULT_RAM_SIZE) "MiB)\n", > + stringify(DEFAULT_RAM_SIZE) "MiB)\n" > + "NOTE: Some architectures might enforce a specific granularity\n", > QEMU_ARCH_ALL) > STEXI > @item -m [size=]@var{megs} > > @Alex,Conny: Any preference? > Sure, I'll add the doc hit & some extra commentary in the next version. >> - my_ram_size = my_ram_size >> (20 + shift) << (20 + shift); >> + >> + /* The core and standby memory areas need to be aligned with >> + * the increment size */ >> + standby_mem_size = standby_mem_size >> mhd->increment_size >> + << mhd->increment_size; >> + my_ram_size = my_ram_size >> mhd->increment_size >> + << mhd->increment_size; >> >> /* let's propagate the changed ram size into the global variable. */ >> ram_size = my_ram_size; >> @@ -109,11 +126,22 @@ static void ccw_init(QEMUMachineInitArgs *args) >> /* register hypercalls */ >> virtio_ccw_register_hcalls(); >> >> - /* allocate RAM */ >> + /* allocate RAM for core */ >> memory_region_init_ram(ram, NULL, "s390.ram", my_ram_size);memory_region_init_ram >> vmstate_register_ram_global(ram); >> memory_region_add_subregion(sysmem, 0, ram); >> >> + /* If the size of ram is not on a MEM_SECTION_SIZE boundary, >> + calculate the pad size necessary to force this boundary. */ >> + if (standby_mem_size) { >> + if (my_ram_size % MEM_SECTION_SIZE) { >> + pad_size = MEM_SECTION_SIZE - my_ram_size % MEM_SECTION_SIZE; >> + } >> + my_ram_size += standby_mem_size + pad_size; >> + mhd->pad_size = pad_size; >> + mhd->standby_mem_size = standby_mem_size; >> + } >> + >> /* allocate storage keys */ >> storage_keys = g_malloc0(my_ram_size / TARGET_PAGE_SIZE); >> >> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h >> index aad277a..193eac3 100644 >> --- a/target-s390x/cpu.h >> +++ b/target-s390x/cpu.h >> @@ -1047,6 +1047,9 @@ static inline void cpu_inject_crw_mchk(S390CPU *cpu) >> cpu_interrupt(CPU(cpu), CPU_INTERRUPT_HARD); >> } >> >> +/* from s390-virtio-ccw */ >> +#define MEM_SECTION_SIZE 0x10000000UL >> + >> /* fpu_helper.c */ >> uint32_t set_cc_nz_f32(float32 v); >> uint32_t set_cc_nz_f64(float64 v); >> > > > >
On 13.05.14 15:16, Matthew Rosato wrote: > On 05/12/2014 03:43 AM, Christian Borntraeger wrote: >> On 07/05/14 20:05, Matthew Rosato wrote: >>> When determining the memory increment size, use the maxmem size if >>> it was specified. >>> >>> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> >>> --- >>> hw/s390x/s390-virtio-ccw.c | 44 ++++++++++++++++++++++++++++++++++++-------- >>> target-s390x/cpu.h | 3 +++ >>> 2 files changed, 39 insertions(+), 8 deletions(-) >>> >>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >>> index 0d4f6ae..a8be0f7 100644 >>> --- a/hw/s390x/s390-virtio-ccw.c >>> +++ b/hw/s390x/s390-virtio-ccw.c >>> @@ -17,6 +17,7 @@ >>> #include "ioinst.h" >>> #include "css.h" >>> #include "virtio-ccw.h" >>> +#include "qemu/config-file.h" >>> >>> void io_subsystem_reset(void) >>> { >>> @@ -84,17 +85,33 @@ static void ccw_init(QEMUMachineInitArgs *args) >>> ram_addr_t my_ram_size = args->ram_size; >>> MemoryRegion *sysmem = get_system_memory(); >>> MemoryRegion *ram = g_new(MemoryRegion, 1); >>> - int shift = 0; >>> + sclpMemoryHotplugDev *mhd = get_sclp_memory_hotplug_dev(); >>> uint8_t *storage_keys; >>> int ret; >>> VirtualCssBus *css_bus; >>> - >>> - /* s390x ram size detection needs a 16bit multiplier + an increment. So >>> - guests > 64GB can be specified in 2MB steps etc. */ >>> - while ((my_ram_size >> (20 + shift)) > 65535) { >>> - shift++; >>> + QemuOpts *opts = qemu_opts_find(qemu_find_opts("memory"), NULL); >>> + ram_addr_t pad_size = 0; >>> + ram_addr_t maxmem = qemu_opt_get_size(opts, "maxmem", 0); >>> + ram_addr_t standby_mem_size = maxmem - my_ram_size; >>> + >>> + /* The storage increment size is a multiple of 1M and is a power of 2. >>> + * The number of storage increments must be MAX_STORAGE_INCREMENTS or fewer. >>> + * The variable 'mhd->increment_size' is an exponent of 2 that can be >>> + * used to calculate the size (in bytes) of an increment. */ >>> + mhd->increment_size = 20; >>> + while ((my_ram_size >> mhd->increment_size) > MAX_STORAGE_INCREMENTS) { >>> + mhd->increment_size++; >>> + } >>> + while ((standby_mem_size >> mhd->increment_size) > MAX_STORAGE_INCREMENTS) { >>> + mhd->increment_size++; >>> } >> Looking back into the mail thread, Alex requested to make the code for standy/non-standby identical. >> Now: The limit of 1020 (MAX_STORAGE_INCREMENTS) is only given if standby memory exists. Without standby memory, we could still used 64k as a divider.(zVM also does only impose this limit with standby memory). >> What does that mean: With this patch the memory size granularity gets bigger. e.g. a guest can have >> 1019MB, 1020MB, 1022MB, 1024MB and so on (1021MB is no longer possible, but it was before). > Hmm, this is a good point. I didn't think about it when I made the > change per Alex. If nobody has a strong opinion here, I think I'd > rather go back to special casing this in the next version, to keep the > 'normal case' (without standby memory) more robust. Wouldn't it be more confusing if the guest configuration suddenly changes when you add standby memory? Alex
On 05/13/2014 09:43 AM, Alexander Graf wrote: > > On 13.05.14 15:16, Matthew Rosato wrote: >> On 05/12/2014 03:43 AM, Christian Borntraeger wrote: >>> On 07/05/14 20:05, Matthew Rosato wrote: >>>> When determining the memory increment size, use the maxmem size if >>>> it was specified. >>>> >>>> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> >>>> --- >>>> hw/s390x/s390-virtio-ccw.c | 44 >>>> ++++++++++++++++++++++++++++++++++++-------- >>>> target-s390x/cpu.h | 3 +++ >>>> 2 files changed, 39 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >>>> index 0d4f6ae..a8be0f7 100644 >>>> --- a/hw/s390x/s390-virtio-ccw.c >>>> +++ b/hw/s390x/s390-virtio-ccw.c >>>> @@ -17,6 +17,7 @@ >>>> #include "ioinst.h" >>>> #include "css.h" >>>> #include "virtio-ccw.h" >>>> +#include "qemu/config-file.h" >>>> >>>> void io_subsystem_reset(void) >>>> { >>>> @@ -84,17 +85,33 @@ static void ccw_init(QEMUMachineInitArgs *args) >>>> ram_addr_t my_ram_size = args->ram_size; >>>> MemoryRegion *sysmem = get_system_memory(); >>>> MemoryRegion *ram = g_new(MemoryRegion, 1); >>>> - int shift = 0; >>>> + sclpMemoryHotplugDev *mhd = get_sclp_memory_hotplug_dev(); >>>> uint8_t *storage_keys; >>>> int ret; >>>> VirtualCssBus *css_bus; >>>> - >>>> - /* s390x ram size detection needs a 16bit multiplier + an >>>> increment. So >>>> - guests > 64GB can be specified in 2MB steps etc. */ >>>> - while ((my_ram_size >> (20 + shift)) > 65535) { >>>> - shift++; >>>> + QemuOpts *opts = qemu_opts_find(qemu_find_opts("memory"), NULL); >>>> + ram_addr_t pad_size = 0; >>>> + ram_addr_t maxmem = qemu_opt_get_size(opts, "maxmem", 0); >>>> + ram_addr_t standby_mem_size = maxmem - my_ram_size; >>>> + >>>> + /* The storage increment size is a multiple of 1M and is a >>>> power of 2. >>>> + * The number of storage increments must be >>>> MAX_STORAGE_INCREMENTS or fewer. >>>> + * The variable 'mhd->increment_size' is an exponent of 2 that >>>> can be >>>> + * used to calculate the size (in bytes) of an increment. */ >>>> + mhd->increment_size = 20; >>>> + while ((my_ram_size >> mhd->increment_size) > >>>> MAX_STORAGE_INCREMENTS) { >>>> + mhd->increment_size++; >>>> + } >>>> + while ((standby_mem_size >> mhd->increment_size) > >>>> MAX_STORAGE_INCREMENTS) { >>>> + mhd->increment_size++; >>>> } >>> Looking back into the mail thread, Alex requested to make the code >>> for standy/non-standby identical. >>> Now: The limit of 1020 (MAX_STORAGE_INCREMENTS) is only given if >>> standby memory exists. Without standby memory, we could still used >>> 64k as a divider.(zVM also does only impose this limit with standby >>> memory). >>> What does that mean: With this patch the memory size granularity gets >>> bigger. e.g. a guest can have >>> 1019MB, 1020MB, 1022MB, 1024MB and so on (1021MB is no longer >>> possible, but it was before). >> Hmm, this is a good point. I didn't think about it when I made the >> change per Alex. If nobody has a strong opinion here, I think I'd >> rather go back to special casing this in the next version, to keep the >> 'normal case' (without standby memory) more robust. > > Wouldn't it be more confusing if the guest configuration suddenly > changes when you add standby memory? > In fairness, you are already changing the guest memory layout with the introduction of standby memory in the first place, so what's a little more change? :) But, yes, I hear you -- The value in keeping the environment uniform across configurations outweighs the benefits of allowing odd boundaries in some cases (probably only test cases anyway). I can live with that. Thanks for the feedback. Matt
diff --git a/qemu-options.hx b/qemu-options.hx index 5edffa6..8c71283 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -217,7 +217,8 @@ DEF("m", HAS_ARG, QEMU_OPTION_m, "-m [size=]megs\n" " configure guest RAM\n" " size: initial amount of guest memory (default: " - stringify(DEFAULT_RAM_SIZE) "MiB)\n", + stringify(DEFAULT_RAM_SIZE) "MiB)\n" + "NOTE: Some architectures might enforce a specific granularity\n", QEMU_ARCH_ALL) STEXI @item -m [size=]@var{megs}