* [Qemu-devel] [PATCH 1/5] Add the standby-mem machine option
2013-12-16 20:51 [Qemu-devel] [PATCH 0/5] s390: Support for Hotplug of Standby Memory Matthew Rosato
@ 2013-12-16 20:51 ` Matthew Rosato
2013-12-17 10:09 ` Paolo Bonzini
2013-12-16 20:51 ` [Qemu-devel] [PATCH 2/5] virtio-ccw: Include standby memory when calculating storage increment Matthew Rosato
` (3 subsequent siblings)
4 siblings, 1 reply; 19+ messages in thread
From: Matthew Rosato @ 2013-12-16 20:51 UTC (permalink / raw)
To: qemu-devel
Cc: gleb, agraf, borntraeger, aliguori, cornelia.huck, pbonzini, rth
Add the machine=...,standby-mem={size} option and associated
documentation.
Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
---
qemu-options.hx | 6 +++++-
vl.c | 6 ++++++
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/qemu-options.hx b/qemu-options.hx
index af34483..def4493 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -35,7 +35,8 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
" kernel_irqchip=on|off controls accelerated irqchip support\n"
" kvm_shadow_mem=size of KVM shadow MMU\n"
" dump-guest-core=on|off include guest memory in a core dump (default=on)\n"
- " mem-merge=on|off controls memory merge support (default: on)\n",
+ " mem-merge=on|off controls memory merge support (default: on)\n"
+ " standby-mem=size sets size of additional offline memory\n",
QEMU_ARCH_ALL)
STEXI
@item -machine [type=]@var{name}[,prop=@var{value}[,...]]
@@ -58,6 +59,9 @@ Include guest memory in a core dump. The default is on.
Enables or disables memory merge support. This feature, when supported by
the host, de-duplicates identical memory pages among VMs instances
(enabled by default).
+@item standby-mem=size
+Defines the size, in megabytes, of additional memory to be left offline for
+future hotplug by the guest.
@end table
ETEXI
diff --git a/vl.c b/vl.c
index b0399de..7d58d24 100644
--- a/vl.c
+++ b/vl.c
@@ -187,6 +187,7 @@ DisplayType display_type = DT_DEFAULT;
static int display_remote;
const char* keyboard_layout = NULL;
ram_addr_t ram_size;
+ram_addr_t standby_mem_size;
const char *mem_path = NULL;
int mem_prealloc = 0; /* force preallocation of physical target memory */
int nb_nics;
@@ -430,6 +431,10 @@ static QemuOptsList qemu_machine_opts = {
.name = "firmware",
.type = QEMU_OPT_STRING,
.help = "firmware image",
+ },{
+ .name = "standby-mem",
+ .type = QEMU_OPT_SIZE,
+ .help = "standby memory size",
},
{ /* End of list */ }
},
@@ -2906,6 +2911,7 @@ int main(int argc, char **argv, char **envp)
machine = find_default_machine();
cpu_model = NULL;
ram_size = 0;
+ standby_mem_size = 0;
snapshot = 0;
cyls = heads = secs = 0;
translation = BIOS_ATA_TRANSLATION_AUTO;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH 1/5] Add the standby-mem machine option
2013-12-16 20:51 ` [Qemu-devel] [PATCH 1/5] Add the standby-mem machine option Matthew Rosato
@ 2013-12-17 10:09 ` Paolo Bonzini
2013-12-17 15:53 ` Matthew Rosato
0 siblings, 1 reply; 19+ messages in thread
From: Paolo Bonzini @ 2013-12-17 10:09 UTC (permalink / raw)
To: Matthew Rosato
Cc: gleb, qemu-devel, agraf, borntraeger, aliguori, cornelia.huck,
rth
Il 16/12/2013 21:51, Matthew Rosato ha scritto:
> Add the machine=...,standby-mem={size} option and associated
> documentation.
See how Igor Mammedov's x86 memory hotplug instead added "-m NN,maxmem=NN".
You could use these two patches:
http://lists.gnu.org/archive/html/qemu-devel/2013-11/msg03438.html
http://lists.gnu.org/archive/html/qemu-devel/2013-11/msg03439.html
Paolo
> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> ---
> qemu-options.hx | 6 +++++-
> vl.c | 6 ++++++
> 2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index af34483..def4493 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -35,7 +35,8 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
> " kernel_irqchip=on|off controls accelerated irqchip support\n"
> " kvm_shadow_mem=size of KVM shadow MMU\n"
> " dump-guest-core=on|off include guest memory in a core dump (default=on)\n"
> - " mem-merge=on|off controls memory merge support (default: on)\n",
> + " mem-merge=on|off controls memory merge support (default: on)\n"
> + " standby-mem=size sets size of additional offline memory\n",
> QEMU_ARCH_ALL)
> STEXI
> @item -machine [type=]@var{name}[,prop=@var{value}[,...]]
> @@ -58,6 +59,9 @@ Include guest memory in a core dump. The default is on.
> Enables or disables memory merge support. This feature, when supported by
> the host, de-duplicates identical memory pages among VMs instances
> (enabled by default).
> +@item standby-mem=size
> +Defines the size, in megabytes, of additional memory to be left offline for
> +future hotplug by the guest.
> @end table
> ETEXI
>
> diff --git a/vl.c b/vl.c
> index b0399de..7d58d24 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -187,6 +187,7 @@ DisplayType display_type = DT_DEFAULT;
> static int display_remote;
> const char* keyboard_layout = NULL;
> ram_addr_t ram_size;
> +ram_addr_t standby_mem_size;
> const char *mem_path = NULL;
> int mem_prealloc = 0; /* force preallocation of physical target memory */
> int nb_nics;
> @@ -430,6 +431,10 @@ static QemuOptsList qemu_machine_opts = {
> .name = "firmware",
> .type = QEMU_OPT_STRING,
> .help = "firmware image",
> + },{
> + .name = "standby-mem",
> + .type = QEMU_OPT_SIZE,
> + .help = "standby memory size",
> },
> { /* End of list */ }
> },
> @@ -2906,6 +2911,7 @@ int main(int argc, char **argv, char **envp)
> machine = find_default_machine();
> cpu_model = NULL;
> ram_size = 0;
> + standby_mem_size = 0;
> snapshot = 0;
> cyls = heads = secs = 0;
> translation = BIOS_ATA_TRANSLATION_AUTO;
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH 1/5] Add the standby-mem machine option
2013-12-17 10:09 ` Paolo Bonzini
@ 2013-12-17 15:53 ` Matthew Rosato
0 siblings, 0 replies; 19+ messages in thread
From: Matthew Rosato @ 2013-12-17 15:53 UTC (permalink / raw)
To: Paolo Bonzini
Cc: gleb, qemu-devel, agraf, borntraeger, aliguori, cornelia.huck,
rth
On 12/17/2013 05:09 AM, Paolo Bonzini wrote:
> Il 16/12/2013 21:51, Matthew Rosato ha scritto:
>> Add the machine=...,standby-mem={size} option and associated
>> documentation.
>
> See how Igor Mammedov's x86 memory hotplug instead added "-m NN,maxmem=NN".
>
> You could use these two patches:
>
> http://lists.gnu.org/archive/html/qemu-devel/2013-11/msg03438.html
> http://lists.gnu.org/archive/html/qemu-devel/2013-11/msg03439.html
>
> Paolo
Thanks Paolo, good point. I'll take a look at this for the next version.
>
>> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
>> ---
>> qemu-options.hx | 6 +++++-
>> vl.c | 6 ++++++
>> 2 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index af34483..def4493 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -35,7 +35,8 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
>> " kernel_irqchip=on|off controls accelerated irqchip support\n"
>> " kvm_shadow_mem=size of KVM shadow MMU\n"
>> " dump-guest-core=on|off include guest memory in a core dump (default=on)\n"
>> - " mem-merge=on|off controls memory merge support (default: on)\n",
>> + " mem-merge=on|off controls memory merge support (default: on)\n"
>> + " standby-mem=size sets size of additional offline memory\n",
>> QEMU_ARCH_ALL)
>> STEXI
>> @item -machine [type=]@var{name}[,prop=@var{value}[,...]]
>> @@ -58,6 +59,9 @@ Include guest memory in a core dump. The default is on.
>> Enables or disables memory merge support. This feature, when supported by
>> the host, de-duplicates identical memory pages among VMs instances
>> (enabled by default).
>> +@item standby-mem=size
>> +Defines the size, in megabytes, of additional memory to be left offline for
>> +future hotplug by the guest.
>> @end table
>> ETEXI
>>
>> diff --git a/vl.c b/vl.c
>> index b0399de..7d58d24 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -187,6 +187,7 @@ DisplayType display_type = DT_DEFAULT;
>> static int display_remote;
>> const char* keyboard_layout = NULL;
>> ram_addr_t ram_size;
>> +ram_addr_t standby_mem_size;
>> const char *mem_path = NULL;
>> int mem_prealloc = 0; /* force preallocation of physical target memory */
>> int nb_nics;
>> @@ -430,6 +431,10 @@ static QemuOptsList qemu_machine_opts = {
>> .name = "firmware",
>> .type = QEMU_OPT_STRING,
>> .help = "firmware image",
>> + },{
>> + .name = "standby-mem",
>> + .type = QEMU_OPT_SIZE,
>> + .help = "standby memory size",
>> },
>> { /* End of list */ }
>> },
>> @@ -2906,6 +2911,7 @@ int main(int argc, char **argv, char **envp)
>> machine = find_default_machine();
>> cpu_model = NULL;
>> ram_size = 0;
>> + standby_mem_size = 0;
>> snapshot = 0;
>> cyls = heads = secs = 0;
>> translation = BIOS_ATA_TRANSLATION_AUTO;
>>
>
>
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH 2/5] virtio-ccw: Include standby memory when calculating storage increment
2013-12-16 20:51 [Qemu-devel] [PATCH 0/5] s390: Support for Hotplug of Standby Memory Matthew Rosato
2013-12-16 20:51 ` [Qemu-devel] [PATCH 1/5] Add the standby-mem machine option Matthew Rosato
@ 2013-12-16 20:51 ` Matthew Rosato
2013-12-16 20:51 ` [Qemu-devel] [PATCH 3/5] target-s390: Check for standby memory specification Matthew Rosato
` (2 subsequent siblings)
4 siblings, 0 replies; 19+ messages in thread
From: Matthew Rosato @ 2013-12-16 20:51 UTC (permalink / raw)
To: qemu-devel
Cc: gleb, agraf, borntraeger, aliguori, cornelia.huck, pbonzini, rth
When determining the memory increment size, include the standby
memory as well as the core memory.
Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
---
hw/s390x/s390-virtio-ccw.c | 30 +++++++++++++++++++++++++-----
target-s390x/cpu.h | 4 ++++
2 files changed, 29 insertions(+), 5 deletions(-)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 733d988..75deb24 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -17,6 +17,8 @@
#include "css.h"
#include "virtio-ccw.h"
+ram_addr_t pad_size;
+
void io_subsystem_reset(void)
{
DeviceState *css, *sclp;
@@ -84,11 +86,22 @@ static void ccw_init(QEMUMachineInitArgs *args)
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++;
+ if (standby_mem_size) {
+ /* The storage increment size is a multiple of 1M and is a power of 2.
+ * The number of storage increments must be 1020 or fewer. */
+ while ((my_ram_size >> (20 + shift)) > 1020) {
+ shift++;
+ }
+ while ((standby_mem_size >> (20 + shift)) > 1020) {
+ shift++;
+ }
+ standby_mem_size = standby_mem_size >> (20 + shift) << (20 + shift);
+ } else {
+ while ((my_ram_size >> (20 + shift)) > 65535) {
+ shift++;
+ }
}
+
my_ram_size = my_ram_size >> (20 + shift) << (20 + shift);
/* let's propagate the changed ram size into the global variable. */
@@ -103,11 +116,18 @@ 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);
vmstate_register_ram_global(ram);
memory_region_add_subregion(sysmem, 0, ram);
+ 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;
+ }
+
/* 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 a2c077b..3383d55 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -385,6 +385,10 @@ unsigned s390_del_running_cpu(S390CPU *cpu);
/* service interrupts are floating therefore we must not pass an cpustate */
void s390_sclp_extint(uint32_t parm);
+/* from s390-virtio-ccw */
+#define MEM_SECTION_SIZE 0x10000000UL
+extern ram_addr_t standby_mem_size;
+
/* from s390-virtio-bus */
extern const hwaddr virtio_size;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 19+ messages in thread* [Qemu-devel] [PATCH 3/5] target-s390: Check for standby memory specification
2013-12-16 20:51 [Qemu-devel] [PATCH 0/5] s390: Support for Hotplug of Standby Memory Matthew Rosato
2013-12-16 20:51 ` [Qemu-devel] [PATCH 1/5] Add the standby-mem machine option Matthew Rosato
2013-12-16 20:51 ` [Qemu-devel] [PATCH 2/5] virtio-ccw: Include standby memory when calculating storage increment Matthew Rosato
@ 2013-12-16 20:51 ` Matthew Rosato
2013-12-16 21:25 ` Alexander Graf
2013-12-16 20:51 ` [Qemu-devel] [PATCH 4/5] sclp-s390: Define new SCLP codes and structures Matthew Rosato
2013-12-16 20:51 ` [Qemu-devel] [PATCH 5/5] sclp-s390: Add memory hotplug SCLPs Matthew Rosato
4 siblings, 1 reply; 19+ messages in thread
From: Matthew Rosato @ 2013-12-16 20:51 UTC (permalink / raw)
To: qemu-devel
Cc: gleb, agraf, borntraeger, aliguori, cornelia.huck, pbonzini, rth
When machine=...,standby-mem={size} has been specified, convert the value
to bytes and store it for use.
Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
---
target-s390x/kvm.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index 02ac4ba..d4081f4 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -97,11 +97,27 @@ static void *legacy_s390_alloc(size_t size);
int kvm_arch_init(KVMState *s)
{
+ int64_t value;
+
cap_sync_regs = kvm_check_extension(s, KVM_CAP_SYNC_REGS);
if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
|| !kvm_check_extension(s, KVM_CAP_S390_COW)) {
phys_mem_set_alloc(legacy_s390_alloc);
}
+
+ value = qemu_opt_get_size(qemu_get_machine_opts(), "standby-mem", -1);
+
+ if (value < 0) {
+ fprintf(stderr, "qemu: invalid standby-mem size:%"PRId64"\n", value);
+ exit(1);
+ }
+
+ if (value != (int64_t)(ram_addr_t)value) {
+ fprintf(stderr, "qemu: standby size too large\n");
+ exit(1);
+ }
+ standby_mem_size = value * 1024 * 1024;
+
return 0;
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH 3/5] target-s390: Check for standby memory specification
2013-12-16 20:51 ` [Qemu-devel] [PATCH 3/5] target-s390: Check for standby memory specification Matthew Rosato
@ 2013-12-16 21:25 ` Alexander Graf
2013-12-17 15:58 ` Matthew Rosato
0 siblings, 1 reply; 19+ messages in thread
From: Alexander Graf @ 2013-12-16 21:25 UTC (permalink / raw)
To: Matthew Rosato
Cc: Gleb Natapov, QEMU Developers, Christian Borntraeger, aliguori,
Cornelia Huck, Paolo Bonzini, Richard Henderson
On 16.12.2013, at 21:51, Matthew Rosato <mjrosato@linux.vnet.ibm.com> wrote:
> When machine=...,standby-mem={size} has been specified, convert the value
> to bytes and store it for use.
>
> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> ---
> target-s390x/kvm.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> index 02ac4ba..d4081f4 100644
> --- a/target-s390x/kvm.c
> +++ b/target-s390x/kvm.c
> @@ -97,11 +97,27 @@ static void *legacy_s390_alloc(size_t size);
>
> int kvm_arch_init(KVMState *s)
> {
> + int64_t value;
> +
> cap_sync_regs = kvm_check_extension(s, KVM_CAP_SYNC_REGS);
> if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
> || !kvm_check_extension(s, KVM_CAP_S390_COW)) {
> phys_mem_set_alloc(legacy_s390_alloc);
> }
> +
> + value = qemu_opt_get_size(qemu_get_machine_opts(), "standby-mem", -1);
> +
> + if (value < 0) {
> + fprintf(stderr, "qemu: invalid standby-mem size:%"PRId64"\n", value);
> + exit(1);
> + }
> +
> + if (value != (int64_t)(ram_addr_t)value) {
> + fprintf(stderr, "qemu: standby size too large\n");
> + exit(1);
> + }
> + standby_mem_size = value * 1024 * 1024;
I would hope qemu_opt_get_size() returns a value in bytes. Why multiply it here?
Alex
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH 3/5] target-s390: Check for standby memory specification
2013-12-16 21:25 ` Alexander Graf
@ 2013-12-17 15:58 ` Matthew Rosato
2013-12-17 16:48 ` Alexander Graf
0 siblings, 1 reply; 19+ messages in thread
From: Matthew Rosato @ 2013-12-17 15:58 UTC (permalink / raw)
To: Alexander Graf
Cc: Gleb Natapov, QEMU Developers, Christian Borntraeger, aliguori,
Cornelia Huck, Paolo Bonzini, Richard Henderson
On 12/16/2013 04:25 PM, Alexander Graf wrote:
>
> On 16.12.2013, at 21:51, Matthew Rosato <mjrosato@linux.vnet.ibm.com> wrote:
>
>> When machine=...,standby-mem={size} has been specified, convert the value
>> to bytes and store it for use.
>>
>> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
>> ---
>> target-s390x/kvm.c | 16 ++++++++++++++++
>> 1 file changed, 16 insertions(+)
>>
>> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
>> index 02ac4ba..d4081f4 100644
>> --- a/target-s390x/kvm.c
>> +++ b/target-s390x/kvm.c
>> @@ -97,11 +97,27 @@ static void *legacy_s390_alloc(size_t size);
>>
>> int kvm_arch_init(KVMState *s)
>> {
>> + int64_t value;
>> +
>> cap_sync_regs = kvm_check_extension(s, KVM_CAP_SYNC_REGS);
>> if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
>> || !kvm_check_extension(s, KVM_CAP_S390_COW)) {
>> phys_mem_set_alloc(legacy_s390_alloc);
>> }
>> +
>> + value = qemu_opt_get_size(qemu_get_machine_opts(), "standby-mem", -1);
>> +
>> + if (value < 0) {
>> + fprintf(stderr, "qemu: invalid standby-mem size:%"PRId64"\n", value);
>> + exit(1);
>> + }
>> +
>> + if (value != (int64_t)(ram_addr_t)value) {
>> + fprintf(stderr, "qemu: standby size too large\n");
>> + exit(1);
>> + }
>> + standby_mem_size = value * 1024 * 1024;
>
> I would hope qemu_opt_get_size() returns a value in bytes. Why multiply it here?
>
It's actually in megabytes, and this is converting to bytes -- Based on
your comment, it sounds like qemu_opt_get_size should always be
returning a byte value.
FWIW, if I adopt Paolo's comments re: patch 1, this behavior goes away
(and the parsing moves out of s390). Either way, I'll adjust for v2.
>
> Alex
>
>
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH 3/5] target-s390: Check for standby memory specification
2013-12-17 15:58 ` Matthew Rosato
@ 2013-12-17 16:48 ` Alexander Graf
0 siblings, 0 replies; 19+ messages in thread
From: Alexander Graf @ 2013-12-17 16:48 UTC (permalink / raw)
To: Matthew Rosato
Cc: Gleb Natapov, QEMU Developers, Christian Borntraeger, aliguori,
Cornelia Huck, Paolo Bonzini, Richard Henderson
On 17.12.2013, at 16:58, Matthew Rosato <mjrosato@linux.vnet.ibm.com> wrote:
> On 12/16/2013 04:25 PM, Alexander Graf wrote:
>>
>> On 16.12.2013, at 21:51, Matthew Rosato <mjrosato@linux.vnet.ibm.com> wrote:
>>
>>> When machine=...,standby-mem={size} has been specified, convert the value
>>> to bytes and store it for use.
>>>
>>> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
>>> ---
>>> target-s390x/kvm.c | 16 ++++++++++++++++
>>> 1 file changed, 16 insertions(+)
>>>
>>> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
>>> index 02ac4ba..d4081f4 100644
>>> --- a/target-s390x/kvm.c
>>> +++ b/target-s390x/kvm.c
>>> @@ -97,11 +97,27 @@ static void *legacy_s390_alloc(size_t size);
>>>
>>> int kvm_arch_init(KVMState *s)
>>> {
>>> + int64_t value;
>>> +
>>> cap_sync_regs = kvm_check_extension(s, KVM_CAP_SYNC_REGS);
>>> if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
>>> || !kvm_check_extension(s, KVM_CAP_S390_COW)) {
>>> phys_mem_set_alloc(legacy_s390_alloc);
>>> }
>>> +
>>> + value = qemu_opt_get_size(qemu_get_machine_opts(), "standby-mem", -1);
>>> +
>>> + if (value < 0) {
>>> + fprintf(stderr, "qemu: invalid standby-mem size:%"PRId64"\n", value);
>>> + exit(1);
>>> + }
>>> +
>>> + if (value != (int64_t)(ram_addr_t)value) {
>>> + fprintf(stderr, "qemu: standby size too large\n");
>>> + exit(1);
>>> + }
>>> + standby_mem_size = value * 1024 * 1024;
>>
>> I would hope qemu_opt_get_size() returns a value in bytes. Why multiply it here?
>>
>
> It's actually in megabytes, and this is converting to bytes -- Based on
> your comment, it sounds like qemu_opt_get_size should always be
> returning a byte value.
At least I don't see code that converts it into MB. After all, you should be able to say size=1M which with your code probably ends up as 1T, no?
> FWIW, if I adopt Paolo's comments re: patch 1, this behavior goes away
> (and the parsing moves out of s390). Either way, I'll adjust for v2.
Ok :).
Alex
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH 4/5] sclp-s390: Define new SCLP codes and structures
2013-12-16 20:51 [Qemu-devel] [PATCH 0/5] s390: Support for Hotplug of Standby Memory Matthew Rosato
` (2 preceding siblings ...)
2013-12-16 20:51 ` [Qemu-devel] [PATCH 3/5] target-s390: Check for standby memory specification Matthew Rosato
@ 2013-12-16 20:51 ` Matthew Rosato
2014-01-22 10:08 ` Christian Borntraeger
2014-03-20 9:56 ` Christian Borntraeger
2013-12-16 20:51 ` [Qemu-devel] [PATCH 5/5] sclp-s390: Add memory hotplug SCLPs Matthew Rosato
4 siblings, 2 replies; 19+ messages in thread
From: Matthew Rosato @ 2013-12-16 20:51 UTC (permalink / raw)
To: qemu-devel
Cc: gleb, agraf, borntraeger, aliguori, cornelia.huck, pbonzini, rth
Define new SCLP codes and structures that will be needed for s390 memory
hotplug.
Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
---
hw/s390x/sclp.c | 2 +-
include/hw/s390x/sclp.h | 46 ++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 47 insertions(+), 1 deletion(-)
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 86d6ae0..cb53d7e 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -45,7 +45,7 @@ static void sclp_execute(SCCB *sccb, uint64_t code)
{
S390SCLPDevice *sdev = get_event_facility();
- switch (code) {
+ switch (code & SCLP_NO_CMD_PARM) {
case SCLP_CMDW_READ_SCP_INFO:
case SCLP_CMDW_READ_SCP_INFO_FORCED:
read_SCP_info(sccb);
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index 231a38a..e80cb23 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -20,18 +20,31 @@
/* SCLP command codes */
#define SCLP_CMDW_READ_SCP_INFO 0x00020001
#define SCLP_CMDW_READ_SCP_INFO_FORCED 0x00120001
+#define SCLP_READ_STORAGE_ELEMENT_INFO 0x00040001
+#define SCLP_ATTACH_STORAGE_ELEMENT 0x00080001
+#define SCLP_ASSIGN_STORAGE 0x000D0001
+#define SCLP_UNASSIGN_STORAGE 0x000C0001
#define SCLP_CMD_READ_EVENT_DATA 0x00770005
#define SCLP_CMD_WRITE_EVENT_DATA 0x00760005
#define SCLP_CMD_READ_EVENT_DATA 0x00770005
#define SCLP_CMD_WRITE_EVENT_DATA 0x00760005
#define SCLP_CMD_WRITE_EVENT_MASK 0x00780005
+/* SCLP Memory hotplug codes */
+#define SCLP_NO_CMD_PARM 0xffff00ff
+#define SCLP_FC_ASSIGN_ATTACH_READ_STOR 0xE00000000000ULL
+#define SCLP_STARTING_SUBINCREMENT_ID 0x10001
+#define SCLP_INCREMENT_UNIT 0x10000
+#define MAX_AVAIL_SLOTS 32
+
/* SCLP response codes */
#define SCLP_RC_NORMAL_READ_COMPLETION 0x0010
#define SCLP_RC_NORMAL_COMPLETION 0x0020
+#define SCLP_RC_SCCB_BOUNDARY_VIOLATION 0x0100
#define SCLP_RC_INVALID_SCLP_COMMAND 0x01f0
#define SCLP_RC_CONTAINED_EQUIPMENT_CHECK 0x0340
#define SCLP_RC_INSUFFICIENT_SCCB_LENGTH 0x0300
+#define SCLP_RC_STANDBY_READ_COMPLETION 0x0410
#define SCLP_RC_INVALID_FUNCTION 0x40f0
#define SCLP_RC_NO_EVENT_BUFFERS_STORED 0x60f0
#define SCLP_RC_INVALID_SELECTION_MASK 0x70f0
@@ -75,8 +88,41 @@ typedef struct ReadInfo {
SCCBHeader h;
uint16_t rnmax;
uint8_t rnsize;
+ uint8_t _reserved1[16 - 11]; /* 11-15 */
+ uint16_t entries_cpu; /* 16-17 */
+ uint16_t offset_cpu; /* 18-19 */
+ uint8_t _reserved2[24 - 20]; /* 20-23 */
+ uint8_t loadparm[8]; /* 24-31 */
+ uint8_t _reserved3[48 - 32]; /* 32-47 */
+ uint64_t facilities; /* 48-55 */
+ uint8_t _reserved0[100 - 56];
+ uint32_t rnsize2;
+ uint64_t rnmax2;
} QEMU_PACKED ReadInfo;
+typedef struct ReadStorageElementInfo {
+ SCCBHeader h;
+ uint16_t max_id;
+ uint16_t assigned;
+ uint16_t standby;
+ uint8_t _reserved0[16 - 14]; /* 14-15 */
+ uint32_t entries[0];
+} QEMU_PACKED ReadStorageElementInfo;
+
+typedef struct AttachStorageElement {
+ SCCBHeader h;
+ uint8_t _reserved0[10 - 8]; /* 8-9 */
+ uint16_t assigned;
+ uint8_t _reserved1[16 - 12]; /* 12-15 */
+ uint32_t entries[0];
+} QEMU_PACKED AttachStorageElement;
+
+typedef struct AssignStorage {
+ SCCBHeader h;
+ uint16_t rn;
+} QEMU_PACKED AssignStorage;
+
+
typedef struct SCCB {
SCCBHeader h;
char data[SCCB_DATA_LEN];
--
1.7.9.5
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH 4/5] sclp-s390: Define new SCLP codes and structures
2013-12-16 20:51 ` [Qemu-devel] [PATCH 4/5] sclp-s390: Define new SCLP codes and structures Matthew Rosato
@ 2014-01-22 10:08 ` Christian Borntraeger
2014-03-20 9:56 ` Christian Borntraeger
1 sibling, 0 replies; 19+ messages in thread
From: Christian Borntraeger @ 2014-01-22 10:08 UTC (permalink / raw)
To: Matthew Rosato, qemu-devel
Cc: gleb, agraf, aliguori, cornelia.huck, pbonzini, rth
On 16/12/13 21:51, Matthew Rosato wrote:
> Define new SCLP codes and structures that will be needed for s390 memory
> hotplug.
>
> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
Can you rebase this patch against
git://github.com/borntraeger/qemu.git s390-next
and send it separately? This patch is pretty much non-controversial and I would apply
it to s390-next.
You could then base your v2 on that.
Christian
> ---
> hw/s390x/sclp.c | 2 +-
> include/hw/s390x/sclp.h | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 47 insertions(+), 1 deletion(-)
>
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 86d6ae0..cb53d7e 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -45,7 +45,7 @@ static void sclp_execute(SCCB *sccb, uint64_t code)
> {
> S390SCLPDevice *sdev = get_event_facility();
>
> - switch (code) {
> + switch (code & SCLP_NO_CMD_PARM) {
> case SCLP_CMDW_READ_SCP_INFO:
> case SCLP_CMDW_READ_SCP_INFO_FORCED:
> read_SCP_info(sccb);
> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
> index 231a38a..e80cb23 100644
> --- a/include/hw/s390x/sclp.h
> +++ b/include/hw/s390x/sclp.h
> @@ -20,18 +20,31 @@
> /* SCLP command codes */
> #define SCLP_CMDW_READ_SCP_INFO 0x00020001
> #define SCLP_CMDW_READ_SCP_INFO_FORCED 0x00120001
> +#define SCLP_READ_STORAGE_ELEMENT_INFO 0x00040001
> +#define SCLP_ATTACH_STORAGE_ELEMENT 0x00080001
> +#define SCLP_ASSIGN_STORAGE 0x000D0001
> +#define SCLP_UNASSIGN_STORAGE 0x000C0001
> #define SCLP_CMD_READ_EVENT_DATA 0x00770005
> #define SCLP_CMD_WRITE_EVENT_DATA 0x00760005
> #define SCLP_CMD_READ_EVENT_DATA 0x00770005
> #define SCLP_CMD_WRITE_EVENT_DATA 0x00760005
> #define SCLP_CMD_WRITE_EVENT_MASK 0x00780005
>
> +/* SCLP Memory hotplug codes */
> +#define SCLP_NO_CMD_PARM 0xffff00ff
> +#define SCLP_FC_ASSIGN_ATTACH_READ_STOR 0xE00000000000ULL
> +#define SCLP_STARTING_SUBINCREMENT_ID 0x10001
> +#define SCLP_INCREMENT_UNIT 0x10000
> +#define MAX_AVAIL_SLOTS 32
> +
> /* SCLP response codes */
> #define SCLP_RC_NORMAL_READ_COMPLETION 0x0010
> #define SCLP_RC_NORMAL_COMPLETION 0x0020
> +#define SCLP_RC_SCCB_BOUNDARY_VIOLATION 0x0100
> #define SCLP_RC_INVALID_SCLP_COMMAND 0x01f0
> #define SCLP_RC_CONTAINED_EQUIPMENT_CHECK 0x0340
> #define SCLP_RC_INSUFFICIENT_SCCB_LENGTH 0x0300
> +#define SCLP_RC_STANDBY_READ_COMPLETION 0x0410
> #define SCLP_RC_INVALID_FUNCTION 0x40f0
> #define SCLP_RC_NO_EVENT_BUFFERS_STORED 0x60f0
> #define SCLP_RC_INVALID_SELECTION_MASK 0x70f0
> @@ -75,8 +88,41 @@ typedef struct ReadInfo {
> SCCBHeader h;
> uint16_t rnmax;
> uint8_t rnsize;
> + uint8_t _reserved1[16 - 11]; /* 11-15 */
> + uint16_t entries_cpu; /* 16-17 */
> + uint16_t offset_cpu; /* 18-19 */
> + uint8_t _reserved2[24 - 20]; /* 20-23 */
> + uint8_t loadparm[8]; /* 24-31 */
> + uint8_t _reserved3[48 - 32]; /* 32-47 */
> + uint64_t facilities; /* 48-55 */
> + uint8_t _reserved0[100 - 56];
> + uint32_t rnsize2;
> + uint64_t rnmax2;
> } QEMU_PACKED ReadInfo;
>
> +typedef struct ReadStorageElementInfo {
> + SCCBHeader h;
> + uint16_t max_id;
> + uint16_t assigned;
> + uint16_t standby;
> + uint8_t _reserved0[16 - 14]; /* 14-15 */
> + uint32_t entries[0];
> +} QEMU_PACKED ReadStorageElementInfo;
> +
> +typedef struct AttachStorageElement {
> + SCCBHeader h;
> + uint8_t _reserved0[10 - 8]; /* 8-9 */
> + uint16_t assigned;
> + uint8_t _reserved1[16 - 12]; /* 12-15 */
> + uint32_t entries[0];
> +} QEMU_PACKED AttachStorageElement;
> +
> +typedef struct AssignStorage {
> + SCCBHeader h;
> + uint16_t rn;
> +} QEMU_PACKED AssignStorage;
> +
> +
> typedef struct SCCB {
> SCCBHeader h;
> char data[SCCB_DATA_LEN];
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH 4/5] sclp-s390: Define new SCLP codes and structures
2013-12-16 20:51 ` [Qemu-devel] [PATCH 4/5] sclp-s390: Define new SCLP codes and structures Matthew Rosato
2014-01-22 10:08 ` Christian Borntraeger
@ 2014-03-20 9:56 ` Christian Borntraeger
2014-03-20 16:33 ` Matthew Rosato
1 sibling, 1 reply; 19+ messages in thread
From: Christian Borntraeger @ 2014-03-20 9:56 UTC (permalink / raw)
To: Matthew Rosato, qemu-devel
Cc: gleb, agraf, aliguori, cornelia.huck, pbonzini, rth
On 16/12/13 21:51, Matthew Rosato wrote:
> Define new SCLP codes and structures that will be needed for s390 memory
> hotplug.
>
> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> ---
> hw/s390x/sclp.c | 2 +-
> include/hw/s390x/sclp.h | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 47 insertions(+), 1 deletion(-)
>
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 86d6ae0..cb53d7e 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -45,7 +45,7 @@ static void sclp_execute(SCCB *sccb, uint64_t code)
> {
> S390SCLPDevice *sdev = get_event_facility();
>
> - switch (code) {
> + switch (code & SCLP_NO_CMD_PARM) {
This is already done in latest upstream, so please provide a newer version of
these patches against s390-next. Will apply then.
Christian
> case SCLP_CMDW_READ_SCP_INFO:
> case SCLP_CMDW_READ_SCP_INFO_FORCED:
> read_SCP_info(sccb);
> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
> index 231a38a..e80cb23 100644
> --- a/include/hw/s390x/sclp.h
> +++ b/include/hw/s390x/sclp.h
> @@ -20,18 +20,31 @@
> /* SCLP command codes */
> #define SCLP_CMDW_READ_SCP_INFO 0x00020001
> #define SCLP_CMDW_READ_SCP_INFO_FORCED 0x00120001
> +#define SCLP_READ_STORAGE_ELEMENT_INFO 0x00040001
> +#define SCLP_ATTACH_STORAGE_ELEMENT 0x00080001
> +#define SCLP_ASSIGN_STORAGE 0x000D0001
> +#define SCLP_UNASSIGN_STORAGE 0x000C0001
> #define SCLP_CMD_READ_EVENT_DATA 0x00770005
> #define SCLP_CMD_WRITE_EVENT_DATA 0x00760005
> #define SCLP_CMD_READ_EVENT_DATA 0x00770005
> #define SCLP_CMD_WRITE_EVENT_DATA 0x00760005
> #define SCLP_CMD_WRITE_EVENT_MASK 0x00780005
>
> +/* SCLP Memory hotplug codes */
> +#define SCLP_NO_CMD_PARM 0xffff00ff
This can then go as well.
> +#define SCLP_FC_ASSIGN_ATTACH_READ_STOR 0xE00000000000ULL
> +#define SCLP_STARTING_SUBINCREMENT_ID 0x10001
> +#define SCLP_INCREMENT_UNIT 0x10000
> +#define MAX_AVAIL_SLOTS 32
> +
> /* SCLP response codes */
> #define SCLP_RC_NORMAL_READ_COMPLETION 0x0010
> #define SCLP_RC_NORMAL_COMPLETION 0x0020
> +#define SCLP_RC_SCCB_BOUNDARY_VIOLATION 0x0100
> #define SCLP_RC_INVALID_SCLP_COMMAND 0x01f0
> #define SCLP_RC_CONTAINED_EQUIPMENT_CHECK 0x0340
> #define SCLP_RC_INSUFFICIENT_SCCB_LENGTH 0x0300
> +#define SCLP_RC_STANDBY_READ_COMPLETION 0x0410
> #define SCLP_RC_INVALID_FUNCTION 0x40f0
> #define SCLP_RC_NO_EVENT_BUFFERS_STORED 0x60f0
> #define SCLP_RC_INVALID_SELECTION_MASK 0x70f0
> @@ -75,8 +88,41 @@ typedef struct ReadInfo {
> SCCBHeader h;
> uint16_t rnmax;
> uint8_t rnsize;
> + uint8_t _reserved1[16 - 11]; /* 11-15 */
> + uint16_t entries_cpu; /* 16-17 */
> + uint16_t offset_cpu; /* 18-19 */
> + uint8_t _reserved2[24 - 20]; /* 20-23 */
> + uint8_t loadparm[8]; /* 24-31 */
> + uint8_t _reserved3[48 - 32]; /* 32-47 */
> + uint64_t facilities; /* 48-55 */
> + uint8_t _reserved0[100 - 56];
> + uint32_t rnsize2;
> + uint64_t rnmax2;
> } QEMU_PACKED ReadInfo;
>
> +typedef struct ReadStorageElementInfo {
> + SCCBHeader h;
> + uint16_t max_id;
> + uint16_t assigned;
> + uint16_t standby;
> + uint8_t _reserved0[16 - 14]; /* 14-15 */
> + uint32_t entries[0];
> +} QEMU_PACKED ReadStorageElementInfo;
> +
> +typedef struct AttachStorageElement {
> + SCCBHeader h;
> + uint8_t _reserved0[10 - 8]; /* 8-9 */
> + uint16_t assigned;
> + uint8_t _reserved1[16 - 12]; /* 12-15 */
> + uint32_t entries[0];
> +} QEMU_PACKED AttachStorageElement;
> +
> +typedef struct AssignStorage {
> + SCCBHeader h;
> + uint16_t rn;
> +} QEMU_PACKED AssignStorage;
> +
> +
Only one space, I guess.
> typedef struct SCCB {
> SCCBHeader h;
> char data[SCCB_DATA_LEN];
>
Otherwise:
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH 4/5] sclp-s390: Define new SCLP codes and structures
2014-03-20 9:56 ` Christian Borntraeger
@ 2014-03-20 16:33 ` Matthew Rosato
2014-03-20 16:36 ` Christian Borntraeger
0 siblings, 1 reply; 19+ messages in thread
From: Matthew Rosato @ 2014-03-20 16:33 UTC (permalink / raw)
To: Christian Borntraeger, qemu-devel
Cc: gleb, agraf, aliguori, cornelia.huck, pbonzini, rth
On 03/20/2014 05:56 AM, Christian Borntraeger wrote:
> On 16/12/13 21:51, Matthew Rosato wrote:
>> Define new SCLP codes and structures that will be needed for s390 memory
>> hotplug.
>>
>> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
>> ---
>> hw/s390x/sclp.c | 2 +-
>> include/hw/s390x/sclp.h | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 47 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index 86d6ae0..cb53d7e 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -45,7 +45,7 @@ static void sclp_execute(SCCB *sccb, uint64_t code)
>> {
>> S390SCLPDevice *sdev = get_event_facility();
>>
>> - switch (code) {
>> + switch (code & SCLP_NO_CMD_PARM) {
>
> This is already done in latest upstream, so please provide a newer version of
> these patches against s390-next. Will apply then.
>
> Christian
Hi Christian -- You already accepted this patch into s390-next with
these changes as 234eef51a12e2f0f8dfd71cb49d2469d462b1855. Or am I
missing something else you wanted changed?
This is part of v1 -- v2 of the whole patch set excluded this patch as a
result (among other changes).
I noticed that v2 doesn't fit anymore. I'll submit a trimmed-down and
re-fit v3 of the patch set once Igor's 'convert -m to QemuOpts' set goes
in (it just got applied to trivial).
>
>> case SCLP_CMDW_READ_SCP_INFO:
>> case SCLP_CMDW_READ_SCP_INFO_FORCED:
>> read_SCP_info(sccb);
>> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
>> index 231a38a..e80cb23 100644
>> --- a/include/hw/s390x/sclp.h
>> +++ b/include/hw/s390x/sclp.h
>> @@ -20,18 +20,31 @@
>> /* SCLP command codes */
>> #define SCLP_CMDW_READ_SCP_INFO 0x00020001
>> #define SCLP_CMDW_READ_SCP_INFO_FORCED 0x00120001
>> +#define SCLP_READ_STORAGE_ELEMENT_INFO 0x00040001
>> +#define SCLP_ATTACH_STORAGE_ELEMENT 0x00080001
>> +#define SCLP_ASSIGN_STORAGE 0x000D0001
>> +#define SCLP_UNASSIGN_STORAGE 0x000C0001
>> #define SCLP_CMD_READ_EVENT_DATA 0x00770005
>> #define SCLP_CMD_WRITE_EVENT_DATA 0x00760005
>> #define SCLP_CMD_READ_EVENT_DATA 0x00770005
>> #define SCLP_CMD_WRITE_EVENT_DATA 0x00760005
>> #define SCLP_CMD_WRITE_EVENT_MASK 0x00780005
>>
>> +/* SCLP Memory hotplug codes */
>> +#define SCLP_NO_CMD_PARM 0xffff00ff
> This can then go as well.
>
>> +#define SCLP_FC_ASSIGN_ATTACH_READ_STOR 0xE00000000000ULL
>> +#define SCLP_STARTING_SUBINCREMENT_ID 0x10001
>> +#define SCLP_INCREMENT_UNIT 0x10000
>> +#define MAX_AVAIL_SLOTS 32
>> +
>> /* SCLP response codes */
>> #define SCLP_RC_NORMAL_READ_COMPLETION 0x0010
>> #define SCLP_RC_NORMAL_COMPLETION 0x0020
>> +#define SCLP_RC_SCCB_BOUNDARY_VIOLATION 0x0100
>> #define SCLP_RC_INVALID_SCLP_COMMAND 0x01f0
>> #define SCLP_RC_CONTAINED_EQUIPMENT_CHECK 0x0340
>> #define SCLP_RC_INSUFFICIENT_SCCB_LENGTH 0x0300
>> +#define SCLP_RC_STANDBY_READ_COMPLETION 0x0410
>> #define SCLP_RC_INVALID_FUNCTION 0x40f0
>> #define SCLP_RC_NO_EVENT_BUFFERS_STORED 0x60f0
>> #define SCLP_RC_INVALID_SELECTION_MASK 0x70f0
>> @@ -75,8 +88,41 @@ typedef struct ReadInfo {
>> SCCBHeader h;
>> uint16_t rnmax;
>> uint8_t rnsize;
>> + uint8_t _reserved1[16 - 11]; /* 11-15 */
>> + uint16_t entries_cpu; /* 16-17 */
>> + uint16_t offset_cpu; /* 18-19 */
>> + uint8_t _reserved2[24 - 20]; /* 20-23 */
>> + uint8_t loadparm[8]; /* 24-31 */
>> + uint8_t _reserved3[48 - 32]; /* 32-47 */
>> + uint64_t facilities; /* 48-55 */
>> + uint8_t _reserved0[100 - 56];
>> + uint32_t rnsize2;
>> + uint64_t rnmax2;
>> } QEMU_PACKED ReadInfo;
>>
>> +typedef struct ReadStorageElementInfo {
>> + SCCBHeader h;
>> + uint16_t max_id;
>> + uint16_t assigned;
>> + uint16_t standby;
>> + uint8_t _reserved0[16 - 14]; /* 14-15 */
>> + uint32_t entries[0];
>> +} QEMU_PACKED ReadStorageElementInfo;
>> +
>> +typedef struct AttachStorageElement {
>> + SCCBHeader h;
>> + uint8_t _reserved0[10 - 8]; /* 8-9 */
>> + uint16_t assigned;
>> + uint8_t _reserved1[16 - 12]; /* 12-15 */
>> + uint32_t entries[0];
>> +} QEMU_PACKED AttachStorageElement;
>> +
>> +typedef struct AssignStorage {
>> + SCCBHeader h;
>> + uint16_t rn;
>> +} QEMU_PACKED AssignStorage;
>> +
>> +
>
> Only one space, I guess.
>
>> typedef struct SCCB {
>> SCCBHeader h;
>> char data[SCCB_DATA_LEN];
>>
>
> Otherwise:
>
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>
>
>
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH 4/5] sclp-s390: Define new SCLP codes and structures
2014-03-20 16:33 ` Matthew Rosato
@ 2014-03-20 16:36 ` Christian Borntraeger
0 siblings, 0 replies; 19+ messages in thread
From: Christian Borntraeger @ 2014-03-20 16:36 UTC (permalink / raw)
To: Matthew Rosato, qemu-devel
Cc: gleb, agraf, aliguori, cornelia.huck, pbonzini, rth
On 20/03/14 17:33, Matthew Rosato wrote:
>
> Hi Christian -- You already accepted this patch into s390-next with
> these changes as 234eef51a12e2f0f8dfd71cb49d2469d462b1855. Or am I
> missing something else you wanted changed?
Gnarf. Yes I wanted to start review and took the wrong version :-(
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH 5/5] sclp-s390: Add memory hotplug SCLPs
2013-12-16 20:51 [Qemu-devel] [PATCH 0/5] s390: Support for Hotplug of Standby Memory Matthew Rosato
` (3 preceding siblings ...)
2013-12-16 20:51 ` [Qemu-devel] [PATCH 4/5] sclp-s390: Define new SCLP codes and structures Matthew Rosato
@ 2013-12-16 20:51 ` Matthew Rosato
2013-12-16 21:42 ` Alexander Graf
4 siblings, 1 reply; 19+ messages in thread
From: Matthew Rosato @ 2013-12-16 20:51 UTC (permalink / raw)
To: qemu-devel
Cc: gleb, agraf, borntraeger, aliguori, cornelia.huck, pbonzini, rth
Add memory information to read SCP info and add handlers for
Read Storage Element Information, Attach Storage Element,
Assign Storage and Unassign Storage.
Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
---
hw/s390x/sclp.c | 233 +++++++++++++++++++++++++++++++++++++++++++++--
include/hw/s390x/sclp.h | 2 +
2 files changed, 229 insertions(+), 6 deletions(-)
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index cb53d7e..5d27fc3 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -15,9 +15,15 @@
#include "cpu.h"
#include "sysemu/kvm.h"
#include "exec/memory.h"
-
+#include "exec/address-spaces.h"
#include "hw/s390x/sclp.h"
+int shift;
+ram_addr_t standby_subregion_size;
+ram_addr_t padded_ram_size;
+ram_addr_t rzm;
+char *standby_state_map;
+
static inline S390SCLPDevice *get_event_facility(void)
{
ObjectProperty *op = object_property_find(qdev_get_machine(),
@@ -31,16 +37,215 @@ static inline S390SCLPDevice *get_event_facility(void)
static void read_SCP_info(SCCB *sccb)
{
ReadInfo *read_info = (ReadInfo *) sccb;
- int shift = 0;
+ int rnsize, rnmax;
+ int max_avail_slots = MAX_AVAIL_SLOTS;
+#ifdef CONFIG_KVM
+ max_avail_slots = kvm_check_extension(kvm_state, KVM_CAP_NR_MEMSLOTS);
+#endif
+
+ read_info->facilities = 0;
+
+ if (standby_mem_size) {
+ /*
+ * The storage increment size is a multiple of 1M and is a power of 2.
+ * The number of storage increments must be 1020 or fewer.
+ */
+ while ((ram_size >> (20 + shift)) > 1020) {
+ shift++;
+ }
+ while ((standby_mem_size >> (20 + shift)) > 1020) {
+ shift++;
+ }
+
+ standby_subregion_size = MEM_SECTION_SIZE;
+ /* Deduct the memory slot already used for core */
+ while ((standby_subregion_size * (max_avail_slots - 1)
+ < standby_mem_size)) {
+ standby_subregion_size = standby_subregion_size << 1;
+ }
+ /*
+ * Initialize mapping of guest standby memory sections indicating which
+ * are and are not online. Assume all standby memory begins offline.
+ */
+ if (standby_mem_size % standby_subregion_size) {
+ standby_state_map = g_malloc0((standby_mem_size /
+ standby_subregion_size + 1) *
+ (standby_subregion_size /
+ MEM_SECTION_SIZE));
+ } else {
+ standby_state_map = g_malloc0(standby_mem_size / MEM_SECTION_SIZE);
+ }
+
+ padded_ram_size = ram_size + pad_size;
+ rzm = 1 << (20 + shift);
+
+ } else {
+ while ((ram_size >> (20 + shift)) > 65535) {
+ shift++;
+ }
+ }
- while ((ram_size >> (20 + shift)) > 65535) {
- shift++;
+ rnsize = 1 << shift;
+ if (rnsize <= 128) {
+ read_info->rnsize = rnsize;
+ } else {
+ read_info->rnsize = 0;
+ read_info->rnsize2 = cpu_to_be32(rnsize);
+ }
+
+ rnmax = (ram_size + standby_mem_size + pad_size) >> (20 + shift);
+ if (rnmax < 0x10000) {
+ read_info->rnmax = cpu_to_be16(rnmax);
+ } else {
+ read_info->rnmax = cpu_to_be16(0);
+ read_info->rnmax2 = cpu_to_be64(rnmax);
+ }
+
+ if (standby_mem_size) {
+ read_info->facilities |= cpu_to_be64(SCLP_FC_ASSIGN_ATTACH_READ_STOR);
}
- read_info->rnmax = cpu_to_be16(ram_size >> (20 + shift));
- read_info->rnsize = 1 << shift;
sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_READ_COMPLETION);
}
+static void read_storage_element0_info(SCCB *sccb)
+{
+ int i, assigned;
+ int subincrement_id = SCLP_STARTING_SUBINCREMENT_ID;
+ ReadStorageElementInfo *storage_info = (ReadStorageElementInfo *) sccb;
+
+ if ((ram_size >> (20 + shift)) >= 0x10000) {
+ sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
+ return;
+ }
+
+ storage_info->max_id = cpu_to_be16(standby_mem_size ? 1 : 0);
+ assigned = ram_size >> (20 + shift);
+ storage_info->assigned = cpu_to_be16(assigned);
+
+ for (i = 0; i < assigned; i++) {
+ storage_info->entries[i] = cpu_to_be32(subincrement_id);
+ subincrement_id += SCLP_INCREMENT_UNIT;
+ }
+ sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_READ_COMPLETION);
+}
+
+static void read_storage_element1_info(SCCB *sccb)
+{
+ ReadStorageElementInfo *storage_info = (ReadStorageElementInfo *) sccb;
+
+ if ((standby_mem_size >> (20 + shift)) >= 0x10000) {
+ sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
+ return;
+ }
+
+ storage_info->max_id = cpu_to_be16(standby_mem_size ? 1 : 0);
+ storage_info->assigned = cpu_to_be16(standby_mem_size >> (20 + shift));
+ storage_info->standby = cpu_to_be16(standby_mem_size >> (20 + shift));
+ sccb->h.response_code = cpu_to_be16(SCLP_RC_STANDBY_READ_COMPLETION);
+}
+
+static void attach_storage_element(SCCB *sccb, uint16_t element)
+{
+ int i, assigned, subincrement_id;
+ AttachStorageElement *attach_info = (AttachStorageElement *) sccb;
+
+ if (element != 1) {
+ sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
+ return;
+ }
+
+ assigned = standby_mem_size >> (20 + shift);
+ attach_info->assigned = cpu_to_be16(assigned);
+ subincrement_id = ((ram_size >> (20 + shift)) << 16)
+ + SCLP_STARTING_SUBINCREMENT_ID;
+ for (i = 0; i < assigned; i++) {
+ attach_info->entries[i] = cpu_to_be32(subincrement_id);
+ subincrement_id += SCLP_INCREMENT_UNIT;
+ }
+ sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
+}
+
+static void assign_storage(SCCB *sccb)
+{
+ MemoryRegion *mr = NULL;
+ int this_subregion_size;
+ AssignStorage *assign_info = (AssignStorage *) sccb;
+ ram_addr_t assign_addr = (assign_info->rn - 1) * rzm;
+ MemoryRegion *sysmem = get_system_memory();
+
+ if ((assign_addr % MEM_SECTION_SIZE == 0) &&
+ (assign_addr >= padded_ram_size)) {
+ mr = memory_region_find(sysmem, assign_addr, 1).mr;
+ if (!mr) {
+
+ MemoryRegion *standby_ram = g_new(MemoryRegion, 1);
+
+ /* offset to align to standby_subregion_size for allocation */
+ ram_addr_t offset = assign_addr -
+ (assign_addr - padded_ram_size)
+ % standby_subregion_size;
+
+ /* strlen("standby.ram") + 4 (Max of KVM_MEMORY_SLOTS) + NULL */
+ char id[16];
+ snprintf(id, 16, "standby.ram%d", (int)((offset - padded_ram_size)
+ / standby_subregion_size) + 1);
+
+ /* Allocate a subregion of the calculated standby_subregion_size */
+ if (offset + standby_subregion_size >
+ padded_ram_size + standby_mem_size) {
+ this_subregion_size = padded_ram_size + standby_mem_size
+ - offset;
+ } else {
+ this_subregion_size = standby_subregion_size;
+ }
+
+ memory_region_init_ram(standby_ram, NULL, id, this_subregion_size);
+ vmstate_register_ram_global(standby_ram);
+ memory_region_add_subregion(sysmem, offset, standby_ram);
+ }
+ standby_state_map[(assign_addr - padded_ram_size)
+ / MEM_SECTION_SIZE] = 1;
+ }
+ sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
+}
+
+static void unassign_storage(SCCB *sccb)
+{
+ MemoryRegion *mr = NULL;
+ AssignStorage *assign_info = (AssignStorage *) sccb;
+ ram_addr_t unassign_addr = (assign_info->rn - 1) * rzm;
+ MemoryRegion *sysmem = get_system_memory();
+
+ /* if the addr is a multiple of 256 MB */
+ if ((unassign_addr % MEM_SECTION_SIZE == 0) &&
+ (unassign_addr >= padded_ram_size)) {
+ standby_state_map[(unassign_addr -
+ padded_ram_size) / MEM_SECTION_SIZE] = 0;
+ mr = memory_region_find(sysmem, unassign_addr, 1).mr;
+ if (mr) {
+ int i;
+ int is_removable = 1;
+ ram_addr_t map_offset = (unassign_addr - padded_ram_size -
+ (unassign_addr - padded_ram_size)
+ % standby_subregion_size);
+ for (i = 0;
+ i < (standby_subregion_size / MEM_SECTION_SIZE);
+ i++) {
+
+ if (standby_state_map[i + map_offset / MEM_SECTION_SIZE]) {
+ is_removable = 0;
+ break;
+ }
+ }
+ if (is_removable) {
+ memory_region_del_subregion(sysmem, mr);
+ memory_region_destroy(mr);
+ }
+ }
+ }
+ sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
+}
+
static void sclp_execute(SCCB *sccb, uint64_t code)
{
S390SCLPDevice *sdev = get_event_facility();
@@ -50,6 +255,22 @@ static void sclp_execute(SCCB *sccb, uint64_t code)
case SCLP_CMDW_READ_SCP_INFO_FORCED:
read_SCP_info(sccb);
break;
+ case SCLP_READ_STORAGE_ELEMENT_INFO:
+ if (code & 0xff00) {
+ read_storage_element1_info(sccb);
+ } else {
+ read_storage_element0_info(sccb);
+ }
+ break;
+ case SCLP_ATTACH_STORAGE_ELEMENT:
+ attach_storage_element(sccb, (code & 0xff00) >> 8);
+ break;
+ case SCLP_ASSIGN_STORAGE:
+ assign_storage(sccb);
+ break;
+ case SCLP_UNASSIGN_STORAGE:
+ unassign_storage(sccb);
+ break;
default:
sdev->sclp_command_handler(sdev->ef, sccb, code);
break;
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index e80cb23..1211d69 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -143,6 +143,8 @@ static inline int sccb_data_len(SCCB *sccb)
OBJECT_GET_CLASS(S390SCLPDeviceClass, (obj), \
TYPE_DEVICE_S390_SCLP)
+extern ram_addr_t pad_size;
+
typedef struct SCLPEventFacility SCLPEventFacility;
typedef struct S390SCLPDevice {
--
1.7.9.5
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH 5/5] sclp-s390: Add memory hotplug SCLPs
2013-12-16 20:51 ` [Qemu-devel] [PATCH 5/5] sclp-s390: Add memory hotplug SCLPs Matthew Rosato
@ 2013-12-16 21:42 ` Alexander Graf
2013-12-16 23:18 ` Alexander Graf
2013-12-17 16:06 ` Matthew Rosato
0 siblings, 2 replies; 19+ messages in thread
From: Alexander Graf @ 2013-12-16 21:42 UTC (permalink / raw)
To: Matthew Rosato
Cc: Gleb Natapov, QEMU Developers, Christian Borntraeger, aliguori,
Cornelia Huck, Paolo Bonzini, Richard Henderson
On 16.12.2013, at 21:51, Matthew Rosato <mjrosato@linux.vnet.ibm.com> wrote:
> Add memory information to read SCP info and add handlers for
> Read Storage Element Information, Attach Storage Element,
> Assign Storage and Unassign Storage.
>
> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> ---
> hw/s390x/sclp.c | 233 +++++++++++++++++++++++++++++++++++++++++++++--
> include/hw/s390x/sclp.h | 2 +
> 2 files changed, 229 insertions(+), 6 deletions(-)
>
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index cb53d7e..5d27fc3 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -15,9 +15,15 @@
> #include "cpu.h"
> #include "sysemu/kvm.h"
> #include "exec/memory.h"
> -
> +#include "exec/address-spaces.h"
> #include "hw/s390x/sclp.h"
>
> +int shift;
> +ram_addr_t standby_subregion_size;
> +ram_addr_t padded_ram_size;
> +ram_addr_t rzm;
> +char *standby_state_map;
Do you really need all these globals?
> +
> static inline S390SCLPDevice *get_event_facility(void)
> {
> ObjectProperty *op = object_property_find(qdev_get_machine(),
> @@ -31,16 +37,215 @@ static inline S390SCLPDevice *get_event_facility(void)
> static void read_SCP_info(SCCB *sccb)
> {
> ReadInfo *read_info = (ReadInfo *) sccb;
> - int shift = 0;
> + int rnsize, rnmax;
> + int max_avail_slots = MAX_AVAIL_SLOTS;
> +#ifdef CONFIG_KVM
> + max_avail_slots = kvm_check_extension(kvm_state, KVM_CAP_NR_MEMSLOTS);
Please don't call kvm specific code from non-kvm specific code. Check out kvm_s390_io_interrupt() as an example how to plumb it in. Please also check kvm_enabled(), as CONFIG_KVM doesn't mean that you actually end up using KVM. It's probably best to completely abstract this away in a helper.
> +#endif
> +
> + read_info->facilities = 0;
> +
> + if (standby_mem_size) {
I don't understand why we have 2 code paths - one for standby and one without. Can't we just handle all cases as standby?
> + /*
> + * The storage increment size is a multiple of 1M and is a power of 2.
> + * The number of storage increments must be 1020 or fewer.
> + */
> + while ((ram_size >> (20 + shift)) > 1020) {
> + shift++;
> + }
> + while ((standby_mem_size >> (20 + shift)) > 1020) {
> + shift++;
> + }
> +
> + standby_subregion_size = MEM_SECTION_SIZE;
> + /* Deduct the memory slot already used for core */
> + while ((standby_subregion_size * (max_avail_slots - 1)
> + < standby_mem_size)) {
> + standby_subregion_size = standby_subregion_size << 1;
> + }
> + /*
> + * Initialize mapping of guest standby memory sections indicating which
> + * are and are not online. Assume all standby memory begins offline.
> + */
> + if (standby_mem_size % standby_subregion_size) {
> + standby_state_map = g_malloc0((standby_mem_size /
> + standby_subregion_size + 1) *
> + (standby_subregion_size /
> + MEM_SECTION_SIZE));
> + } else {
> + standby_state_map = g_malloc0(standby_mem_size / MEM_SECTION_SIZE);
> + }
You don't free this thing when the guest calls read_SCP_info twice.
> +
> + padded_ram_size = ram_size + pad_size;
> + rzm = 1 << (20 + shift);
> +
> + } else {
> + while ((ram_size >> (20 + shift)) > 65535) {
> + shift++;
> + }
> + }
>
> - while ((ram_size >> (20 + shift)) > 65535) {
> - shift++;
> + rnsize = 1 << shift;
> + if (rnsize <= 128) {
> + read_info->rnsize = rnsize;
> + } else {
> + read_info->rnsize = 0;
> + read_info->rnsize2 = cpu_to_be32(rnsize);
> + }
> +
> + rnmax = (ram_size + standby_mem_size + pad_size) >> (20 + shift);
> + if (rnmax < 0x10000) {
> + read_info->rnmax = cpu_to_be16(rnmax);
> + } else {
> + read_info->rnmax = cpu_to_be16(0);
> + read_info->rnmax2 = cpu_to_be64(rnmax);
> + }
> +
> + if (standby_mem_size) {
> + read_info->facilities |= cpu_to_be64(SCLP_FC_ASSIGN_ATTACH_READ_STOR);
Any reason to make this conditional?
> }
> - read_info->rnmax = cpu_to_be16(ram_size >> (20 + shift));
> - read_info->rnsize = 1 << shift;
> sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_READ_COMPLETION);
> }
>
> +static void read_storage_element0_info(SCCB *sccb)
> +{
> + int i, assigned;
> + int subincrement_id = SCLP_STARTING_SUBINCREMENT_ID;
> + ReadStorageElementInfo *storage_info = (ReadStorageElementInfo *) sccb;
> +
> + if ((ram_size >> (20 + shift)) >= 0x10000) {
> + sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
> + return;
> + }
> +
> + storage_info->max_id = cpu_to_be16(standby_mem_size ? 1 : 0);
> + assigned = ram_size >> (20 + shift);
> + storage_info->assigned = cpu_to_be16(assigned);
> +
> + for (i = 0; i < assigned; i++) {
> + storage_info->entries[i] = cpu_to_be32(subincrement_id);
> + subincrement_id += SCLP_INCREMENT_UNIT;
> + }
> + sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_READ_COMPLETION);
> +}
> +
> +static void read_storage_element1_info(SCCB *sccb)
> +{
> + ReadStorageElementInfo *storage_info = (ReadStorageElementInfo *) sccb;
> +
> + if ((standby_mem_size >> (20 + shift)) >= 0x10000) {
> + sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
> + return;
> + }
> +
> + storage_info->max_id = cpu_to_be16(standby_mem_size ? 1 : 0);
> + storage_info->assigned = cpu_to_be16(standby_mem_size >> (20 + shift));
> + storage_info->standby = cpu_to_be16(standby_mem_size >> (20 + shift));
> + sccb->h.response_code = cpu_to_be16(SCLP_RC_STANDBY_READ_COMPLETION);
> +}
> +
> +static void attach_storage_element(SCCB *sccb, uint16_t element)
> +{
> + int i, assigned, subincrement_id;
> + AttachStorageElement *attach_info = (AttachStorageElement *) sccb;
> +
> + if (element != 1) {
> + sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
> + return;
> + }
> +
> + assigned = standby_mem_size >> (20 + shift);
> + attach_info->assigned = cpu_to_be16(assigned);
> + subincrement_id = ((ram_size >> (20 + shift)) << 16)
> + + SCLP_STARTING_SUBINCREMENT_ID;
> + for (i = 0; i < assigned; i++) {
> + attach_info->entries[i] = cpu_to_be32(subincrement_id);
> + subincrement_id += SCLP_INCREMENT_UNIT;
> + }
> + sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
> +}
> +
> +static void assign_storage(SCCB *sccb)
> +{
> + MemoryRegion *mr = NULL;
> + int this_subregion_size;
> + AssignStorage *assign_info = (AssignStorage *) sccb;
> + ram_addr_t assign_addr = (assign_info->rn - 1) * rzm;
> + MemoryRegion *sysmem = get_system_memory();
> +
> + if ((assign_addr % MEM_SECTION_SIZE == 0) &&
> + (assign_addr >= padded_ram_size)) {
> + mr = memory_region_find(sysmem, assign_addr, 1).mr;
> + if (!mr) {
> +
> + MemoryRegion *standby_ram = g_new(MemoryRegion, 1);
> +
> + /* offset to align to standby_subregion_size for allocation */
> + ram_addr_t offset = assign_addr -
> + (assign_addr - padded_ram_size)
> + % standby_subregion_size;
> +
> + /* strlen("standby.ram") + 4 (Max of KVM_MEMORY_SLOTS) + NULL */
> + char id[16];
> + snprintf(id, 16, "standby.ram%d", (int)((offset - padded_ram_size)
> + / standby_subregion_size) + 1);
> +
> + /* Allocate a subregion of the calculated standby_subregion_size */
> + if (offset + standby_subregion_size >
> + padded_ram_size + standby_mem_size) {
> + this_subregion_size = padded_ram_size + standby_mem_size
> + - offset;
> + } else {
> + this_subregion_size = standby_subregion_size;
> + }
> +
> + memory_region_init_ram(standby_ram, NULL, id, this_subregion_size);
> + vmstate_register_ram_global(standby_ram);
> + memory_region_add_subregion(sysmem, offset, standby_ram);
> + }
> + standby_state_map[(assign_addr - padded_ram_size)
> + / MEM_SECTION_SIZE] = 1;
> + }
> + sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
> +}
> +
> +static void unassign_storage(SCCB *sccb)
> +{
> + MemoryRegion *mr = NULL;
> + AssignStorage *assign_info = (AssignStorage *) sccb;
> + ram_addr_t unassign_addr = (assign_info->rn - 1) * rzm;
> + MemoryRegion *sysmem = get_system_memory();
> +
> + /* if the addr is a multiple of 256 MB */
> + if ((unassign_addr % MEM_SECTION_SIZE == 0) &&
> + (unassign_addr >= padded_ram_size)) {
> + standby_state_map[(unassign_addr -
> + padded_ram_size) / MEM_SECTION_SIZE] = 0;
> + mr = memory_region_find(sysmem, unassign_addr, 1).mr;
> + if (mr) {
> + int i;
> + int is_removable = 1;
> + ram_addr_t map_offset = (unassign_addr - padded_ram_size -
> + (unassign_addr - padded_ram_size)
> + % standby_subregion_size);
> + for (i = 0;
> + i < (standby_subregion_size / MEM_SECTION_SIZE);
> + i++) {
> +
> + if (standby_state_map[i + map_offset / MEM_SECTION_SIZE]) {
> + is_removable = 0;
> + break;
> + }
> + }
> + if (is_removable) {
> + memory_region_del_subregion(sysmem, mr);
> + memory_region_destroy(mr);
no g_free()?
> + }
> + }
> + }
> + sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
> +}
> +
> static void sclp_execute(SCCB *sccb, uint64_t code)
> {
> S390SCLPDevice *sdev = get_event_facility();
> @@ -50,6 +255,22 @@ static void sclp_execute(SCCB *sccb, uint64_t code)
> case SCLP_CMDW_READ_SCP_INFO_FORCED:
> read_SCP_info(sccb);
> break;
> + case SCLP_READ_STORAGE_ELEMENT_INFO:
> + if (code & 0xff00) {
> + read_storage_element1_info(sccb);
> + } else {
> + read_storage_element0_info(sccb);
> + }
> + break;
> + case SCLP_ATTACH_STORAGE_ELEMENT:
> + attach_storage_element(sccb, (code & 0xff00) >> 8);
> + break;
> + case SCLP_ASSIGN_STORAGE:
> + assign_storage(sccb);
> + break;
> + case SCLP_UNASSIGN_STORAGE:
> + unassign_storage(sccb);
> + break;
Do you think it'd be possible to model this whole thing as a device that has its own state? That's where you could keep the bitmap for example. You'd only need some callback mechanism to hook into the SCLP calls, but the PPC guys already have something similar with their hypercalls.
Overall, the code could use _a lot_ more comments.
Alex
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH 5/5] sclp-s390: Add memory hotplug SCLPs
2013-12-16 21:42 ` Alexander Graf
@ 2013-12-16 23:18 ` Alexander Graf
2013-12-17 16:09 ` Matthew Rosato
2013-12-17 16:06 ` Matthew Rosato
1 sibling, 1 reply; 19+ messages in thread
From: Alexander Graf @ 2013-12-16 23:18 UTC (permalink / raw)
To: Matthew Rosato
Cc: Gleb Natapov, QEMU Developers, Christian Borntraeger, aliguori,
Cornelia Huck, Paolo Bonzini, Richard Henderson
On 16.12.2013, at 22:42, Alexander Graf <agraf@suse.de> wrote:
>
> On 16.12.2013, at 21:51, Matthew Rosato <mjrosato@linux.vnet.ibm.com> wrote:
>
[...]
>
>> + }
>> + }
>> + }
>> + sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
>> +}
>> +
>> static void sclp_execute(SCCB *sccb, uint64_t code)
>> {
>> S390SCLPDevice *sdev = get_event_facility();
>> @@ -50,6 +255,22 @@ static void sclp_execute(SCCB *sccb, uint64_t code)
>> case SCLP_CMDW_READ_SCP_INFO_FORCED:
>> read_SCP_info(sccb);
>> break;
>> + case SCLP_READ_STORAGE_ELEMENT_INFO:
>> + if (code & 0xff00) {
>> + read_storage_element1_info(sccb);
>> + } else {
>> + read_storage_element0_info(sccb);
>> + }
>> + break;
>> + case SCLP_ATTACH_STORAGE_ELEMENT:
>> + attach_storage_element(sccb, (code & 0xff00) >> 8);
>> + break;
>> + case SCLP_ASSIGN_STORAGE:
>> + assign_storage(sccb);
>> + break;
>> + case SCLP_UNASSIGN_STORAGE:
>> + unassign_storage(sccb);
>> + break;
>
> Do you think it'd be possible to model this whole thing as a device that has its own state? That's where you could keep the bitmap for example. You'd only need some callback mechanism to hook into the SCLP calls, but the PPC guys already have something similar with their hypercalls.
Speaking of state - in the current model the "is standby storage active" bitmap doesn't get migrated, no?
Alex
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH 5/5] sclp-s390: Add memory hotplug SCLPs
2013-12-16 23:18 ` Alexander Graf
@ 2013-12-17 16:09 ` Matthew Rosato
0 siblings, 0 replies; 19+ messages in thread
From: Matthew Rosato @ 2013-12-17 16:09 UTC (permalink / raw)
To: Alexander Graf
Cc: Gleb Natapov, QEMU Developers, Christian Borntraeger, aliguori,
Cornelia Huck, Paolo Bonzini, Richard Henderson
On 12/16/2013 06:18 PM, Alexander Graf wrote:
>
> On 16.12.2013, at 22:42, Alexander Graf <agraf@suse.de> wrote:
>
>>
>> On 16.12.2013, at 21:51, Matthew Rosato <mjrosato@linux.vnet.ibm.com> wrote:
>>
>
> [...]
>
>>
>>> + }
>>> + }
>>> + }
>>> + sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
>>> +}
>>> +
>>> static void sclp_execute(SCCB *sccb, uint64_t code)
>>> {
>>> S390SCLPDevice *sdev = get_event_facility();
>>> @@ -50,6 +255,22 @@ static void sclp_execute(SCCB *sccb, uint64_t code)
>>> case SCLP_CMDW_READ_SCP_INFO_FORCED:
>>> read_SCP_info(sccb);
>>> break;
>>> + case SCLP_READ_STORAGE_ELEMENT_INFO:
>>> + if (code & 0xff00) {
>>> + read_storage_element1_info(sccb);
>>> + } else {
>>> + read_storage_element0_info(sccb);
>>> + }
>>> + break;
>>> + case SCLP_ATTACH_STORAGE_ELEMENT:
>>> + attach_storage_element(sccb, (code & 0xff00) >> 8);
>>> + break;
>>> + case SCLP_ASSIGN_STORAGE:
>>> + assign_storage(sccb);
>>> + break;
>>> + case SCLP_UNASSIGN_STORAGE:
>>> + unassign_storage(sccb);
>>> + break;
>>
>> Do you think it'd be possible to model this whole thing as a device that has its own state? That's where you could keep the bitmap for example. You'd only need some callback mechanism to hook into the SCLP calls, but the PPC guys already have something similar with their hypercalls.
>
> Speaking of state - in the current model the "is standby storage active" bitmap doesn't get migrated, no?
>
Yes, you are correct, migration support still needs to be added - But I
wanted to get this much out for review at least.
>
> Alex
>
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 5/5] sclp-s390: Add memory hotplug SCLPs
2013-12-16 21:42 ` Alexander Graf
2013-12-16 23:18 ` Alexander Graf
@ 2013-12-17 16:06 ` Matthew Rosato
1 sibling, 0 replies; 19+ messages in thread
From: Matthew Rosato @ 2013-12-17 16:06 UTC (permalink / raw)
To: Alexander Graf
Cc: Gleb Natapov, QEMU Developers, Christian Borntraeger, aliguori,
Cornelia Huck, Paolo Bonzini, Richard Henderson
On 12/16/2013 04:42 PM, Alexander Graf wrote:
>
> On 16.12.2013, at 21:51, Matthew Rosato <mjrosato@linux.vnet.ibm.com> wrote:
>
>> Add memory information to read SCP info and add handlers for
>> Read Storage Element Information, Attach Storage Element,
>> Assign Storage and Unassign Storage.
>>
>> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
>> ---
>> hw/s390x/sclp.c | 233 +++++++++++++++++++++++++++++++++++++++++++++--
>> include/hw/s390x/sclp.h | 2 +
>> 2 files changed, 229 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index cb53d7e..5d27fc3 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -15,9 +15,15 @@
>> #include "cpu.h"
>> #include "sysemu/kvm.h"
>> #include "exec/memory.h"
>> -
>> +#include "exec/address-spaces.h"
>> #include "hw/s390x/sclp.h"
>>
>> +int shift;
>> +ram_addr_t standby_subregion_size;
>> +ram_addr_t padded_ram_size;
>> +ram_addr_t rzm;
>> +char *standby_state_map;
>
> Do you really need all these globals?
>
I think this ties into your device comment below - I agree that these
should be encapsulated.
>> +
>> static inline S390SCLPDevice *get_event_facility(void)
>> {
>> ObjectProperty *op = object_property_find(qdev_get_machine(),
>> @@ -31,16 +37,215 @@ static inline S390SCLPDevice *get_event_facility(void)
>> static void read_SCP_info(SCCB *sccb)
>> {
>> ReadInfo *read_info = (ReadInfo *) sccb;
>> - int shift = 0;
>> + int rnsize, rnmax;
>> + int max_avail_slots = MAX_AVAIL_SLOTS;
>> +#ifdef CONFIG_KVM
>> + max_avail_slots = kvm_check_extension(kvm_state, KVM_CAP_NR_MEMSLOTS);
>
> Please don't call kvm specific code from non-kvm specific code. Check out kvm_s390_io_interrupt() as an example how to plumb it in. Please also check kvm_enabled(), as CONFIG_KVM doesn't mean that you actually end up using KVM. It's probably best to completely abstract this away in a helper.
>
OK, will do.
>> +#endif
>> +
>> + read_info->facilities = 0;
>> +
>> + if (standby_mem_size) {
>
> I don't understand why we have 2 code paths - one for standby and one without. Can't we just handle all cases as standby?
>
Yes I think so - everything should fall-through naturally and we can
just skip the malloc of standby_state_map.
>> + /*
>> + * The storage increment size is a multiple of 1M and is a power of 2.
>> + * The number of storage increments must be 1020 or fewer.
>> + */
>> + while ((ram_size >> (20 + shift)) > 1020) {
>> + shift++;
>> + }
>> + while ((standby_mem_size >> (20 + shift)) > 1020) {
>> + shift++;
>> + }
>> +
>> + standby_subregion_size = MEM_SECTION_SIZE;
>> + /* Deduct the memory slot already used for core */
>> + while ((standby_subregion_size * (max_avail_slots - 1)
>> + < standby_mem_size)) {
>> + standby_subregion_size = standby_subregion_size << 1;
>> + }
>> + /*
>> + * Initialize mapping of guest standby memory sections indicating which
>> + * are and are not online. Assume all standby memory begins offline.
>> + */
>> + if (standby_mem_size % standby_subregion_size) {
>> + standby_state_map = g_malloc0((standby_mem_size /
>> + standby_subregion_size + 1) *
>> + (standby_subregion_size /
>> + MEM_SECTION_SIZE));
>> + } else {
>> + standby_state_map = g_malloc0(standby_mem_size / MEM_SECTION_SIZE);
>> + }
>
> You don't free this thing when the guest calls read_SCP_info twice.
>
Oops. Acknowledged.
>> +
>> + padded_ram_size = ram_size + pad_size;
>> + rzm = 1 << (20 + shift);
>> +
>> + } else {
>> + while ((ram_size >> (20 + shift)) > 65535) {
>> + shift++;
>> + }
>> + }
>>
>> - while ((ram_size >> (20 + shift)) > 65535) {
>> - shift++;
>> + rnsize = 1 << shift;
>> + if (rnsize <= 128) {
>> + read_info->rnsize = rnsize;
>> + } else {
>> + read_info->rnsize = 0;
>> + read_info->rnsize2 = cpu_to_be32(rnsize);
>> + }
>> +
>> + rnmax = (ram_size + standby_mem_size + pad_size) >> (20 + shift);
>> + if (rnmax < 0x10000) {
>> + read_info->rnmax = cpu_to_be16(rnmax);
>> + } else {
>> + read_info->rnmax = cpu_to_be16(0);
>> + read_info->rnmax2 = cpu_to_be64(rnmax);
>> + }
>> +
>> + if (standby_mem_size) {
>> + read_info->facilities |= cpu_to_be64(SCLP_FC_ASSIGN_ATTACH_READ_STOR);
>
> Any reason to make this conditional?
>
I suppose not -- I'll make this unconditional.
>> }
>> - read_info->rnmax = cpu_to_be16(ram_size >> (20 + shift));
>> - read_info->rnsize = 1 << shift;
>> sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_READ_COMPLETION);
>> }
[...]
>> +static void unassign_storage(SCCB *sccb)
>> +{
>> + MemoryRegion *mr = NULL;
>> + AssignStorage *assign_info = (AssignStorage *) sccb;
>> + ram_addr_t unassign_addr = (assign_info->rn - 1) * rzm;
>> + MemoryRegion *sysmem = get_system_memory();
>> +
>> + /* if the addr is a multiple of 256 MB */
>> + if ((unassign_addr % MEM_SECTION_SIZE == 0) &&
>> + (unassign_addr >= padded_ram_size)) {
>> + standby_state_map[(unassign_addr -
>> + padded_ram_size) / MEM_SECTION_SIZE] = 0;
>> + mr = memory_region_find(sysmem, unassign_addr, 1).mr;
>> + if (mr) {
>> + int i;
>> + int is_removable = 1;
>> + ram_addr_t map_offset = (unassign_addr - padded_ram_size -
>> + (unassign_addr - padded_ram_size)
>> + % standby_subregion_size);
>> + for (i = 0;
>> + i < (standby_subregion_size / MEM_SECTION_SIZE);
>> + i++) {
>> +
>> + if (standby_state_map[i + map_offset / MEM_SECTION_SIZE]) {
>> + is_removable = 0;
>> + break;
>> + }
>> + }
>> + if (is_removable) {
>> + memory_region_del_subregion(sysmem, mr);
>> + memory_region_destroy(mr);
>
> no g_free()?
Oops. Acknowledged.
>
>> + }
>> + }
>> + }
>> + sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
>> +}
>> +
>> static void sclp_execute(SCCB *sccb, uint64_t code)
>> {
>> S390SCLPDevice *sdev = get_event_facility();
>> @@ -50,6 +255,22 @@ static void sclp_execute(SCCB *sccb, uint64_t code)
>> case SCLP_CMDW_READ_SCP_INFO_FORCED:
>> read_SCP_info(sccb);
>> break;
>> + case SCLP_READ_STORAGE_ELEMENT_INFO:
>> + if (code & 0xff00) {
>> + read_storage_element1_info(sccb);
>> + } else {
>> + read_storage_element0_info(sccb);
>> + }
>> + break;
>> + case SCLP_ATTACH_STORAGE_ELEMENT:
>> + attach_storage_element(sccb, (code & 0xff00) >> 8);
>> + break;
>> + case SCLP_ASSIGN_STORAGE:
>> + assign_storage(sccb);
>> + break;
>> + case SCLP_UNASSIGN_STORAGE:
>> + unassign_storage(sccb);
>> + break;
>
> Do you think it'd be possible to model this whole thing as a device that has its own state? That's where you could keep the bitmap for example. You'd only need some callback mechanism to hook into the SCLP calls, but the PPC guys already have something similar with their hypercalls.
>
OK, let me look into this for the next version.
> Overall, the code could use _a lot_ more comments.
Will do. Thanks for your comments.
>
>
> Alex
>
>
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread