[Qemu-devel] [PATCH] s390: Storage key global access

[Qemu-devel] [PATCH] s390: Storage key global access

[Jason J. Herne]

From: "Jason J. Herne" <jjherne@us.ibm.com>

Introduces global access to storage key data so we can set it for each cpu in
the S390 cpu initialization routine.

Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
---
 hw/s390x/s390-virtio-ccw.c | 3 +--
 hw/s390x/s390-virtio.c     | 6 +++---
 hw/s390x/s390-virtio.h     | 2 +-
 target-s390x/cpu.h         | 3 +++
 4 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 733d988..62319b9 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -80,7 +80,6 @@ static void ccw_init(QEMUMachineInitArgs *args)
     MemoryRegion *sysmem = get_system_memory();
     MemoryRegion *ram = g_new(MemoryRegion, 1);
     int shift = 0;
-    uint8_t *storage_keys;
     int ret;
     VirtualCssBus *css_bus;
 
@@ -112,7 +111,7 @@ static void ccw_init(QEMUMachineInitArgs *args)
     storage_keys = g_malloc0(my_ram_size / TARGET_PAGE_SIZE);
 
     /* init CPUs */
-    s390_init_cpus(args->cpu_model, storage_keys);
+    s390_init_cpus(args->cpu_model);
 
     if (kvm_enabled()) {
         kvm_s390_enable_css_support(s390_cpu_addr2state(0));
diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index 7adf92a..804483f 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -53,6 +53,7 @@
 
 static VirtIOS390Bus *s390_bus;
 static S390CPU **ipi_states;
+uint8_t *storage_keys;
 
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
 {
@@ -176,7 +177,7 @@ void s390_init_ipl_dev(const char *kernel_filename,
     qdev_init_nofail(dev);
 }
 
-void s390_init_cpus(const char *cpu_model, uint8_t *storage_keys)
+void s390_init_cpus(const char *cpu_model)
 {
     int i;
 
@@ -231,7 +232,6 @@ static void s390_init(QEMUMachineInitArgs *args)
     MemoryRegion *sysmem = get_system_memory();
     MemoryRegion *ram = g_new(MemoryRegion, 1);
     int shift = 0;
-    uint8_t *storage_keys;
     void *virtio_region;
     hwaddr virtio_region_len;
     hwaddr virtio_region_start;
@@ -273,7 +273,7 @@ static void s390_init(QEMUMachineInitArgs *args)
     storage_keys = g_malloc0(my_ram_size / TARGET_PAGE_SIZE);
 
     /* init CPUs */
-    s390_init_cpus(args->cpu_model, storage_keys);
+    s390_init_cpus(args->cpu_model);
 
     /* Create VirtIO network adapters */
     s390_create_virtio_net((BusState *)s390_bus, "virtio-net-s390");
diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h
index 5c405e7..c1cb042 100644
--- a/hw/s390x/s390-virtio.h
+++ b/hw/s390x/s390-virtio.h
@@ -20,7 +20,7 @@
 typedef int (*s390_virtio_fn)(const uint64_t *args);
 void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn);
 
-void s390_init_cpus(const char *cpu_model, uint8_t *storage_keys);
+void s390_init_cpus(const char *cpu_model);
 void s390_init_ipl_dev(const char *kernel_filename,
                        const char *kernel_cmdline,
                        const char *initrd_filename,
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index 68b5ab7..b1432c7 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -381,6 +381,9 @@ static inline void kvm_s390_interrupt_internal(S390CPU *cpu, int type,
 {
 }
 #endif
+
+extern uint8_t *storage_keys;
+
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
 void s390_add_running_cpu(S390CPU *cpu);
 unsigned s390_del_running_cpu(S390CPU *cpu);
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH] s390: Storage key global access

[Jason J. Herne]

On 01/22/2014 10:48 AM, Jason J. Herne wrote:
> From: "Jason J. Herne" <jjherne@us.ibm.com>
>
> Introduces global access to storage key data so we can set it for each cpu in
> the S390 cpu initialization routine.
>
> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
> ---
>   hw/s390x/s390-virtio-ccw.c | 3 +--
>   hw/s390x/s390-virtio.c     | 6 +++---
>   hw/s390x/s390-virtio.h     | 2 +-
>   target-s390x/cpu.h         | 3 +++
>   4 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 733d988..62319b9 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -80,7 +80,6 @@ static void ccw_init(QEMUMachineInitArgs *args)
>       MemoryRegion *sysmem = get_system_memory();
>       MemoryRegion *ram = g_new(MemoryRegion, 1);
>       int shift = 0;
> -    uint8_t *storage_keys;
>       int ret;
>       VirtualCssBus *css_bus;
>
> @@ -112,7 +111,7 @@ static void ccw_init(QEMUMachineInitArgs *args)
>       storage_keys = g_malloc0(my_ram_size / TARGET_PAGE_SIZE);
>
>       /* init CPUs */
> -    s390_init_cpus(args->cpu_model, storage_keys);
> +    s390_init_cpus(args->cpu_model);
>
>       if (kvm_enabled()) {
>           kvm_s390_enable_css_support(s390_cpu_addr2state(0));
> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
> index 7adf92a..804483f 100644
> --- a/hw/s390x/s390-virtio.c
> +++ b/hw/s390x/s390-virtio.c
> @@ -53,6 +53,7 @@
>
>   static VirtIOS390Bus *s390_bus;
>   static S390CPU **ipi_states;
> +uint8_t *storage_keys;
>
>   S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>   {
> @@ -176,7 +177,7 @@ void s390_init_ipl_dev(const char *kernel_filename,
>       qdev_init_nofail(dev);
>   }
>
> -void s390_init_cpus(const char *cpu_model, uint8_t *storage_keys)
> +void s390_init_cpus(const char *cpu_model)
>   {
>       int i;
>
> @@ -231,7 +232,6 @@ static void s390_init(QEMUMachineInitArgs *args)
>       MemoryRegion *sysmem = get_system_memory();
>       MemoryRegion *ram = g_new(MemoryRegion, 1);
>       int shift = 0;
> -    uint8_t *storage_keys;
>       void *virtio_region;
>       hwaddr virtio_region_len;
>       hwaddr virtio_region_start;
> @@ -273,7 +273,7 @@ static void s390_init(QEMUMachineInitArgs *args)
>       storage_keys = g_malloc0(my_ram_size / TARGET_PAGE_SIZE);
>
>       /* init CPUs */
> -    s390_init_cpus(args->cpu_model, storage_keys);
> +    s390_init_cpus(args->cpu_model);
>
>       /* Create VirtIO network adapters */
>       s390_create_virtio_net((BusState *)s390_bus, "virtio-net-s390");
> diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h
> index 5c405e7..c1cb042 100644
> --- a/hw/s390x/s390-virtio.h
> +++ b/hw/s390x/s390-virtio.h
> @@ -20,7 +20,7 @@
>   typedef int (*s390_virtio_fn)(const uint64_t *args);
>   void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn);
>
> -void s390_init_cpus(const char *cpu_model, uint8_t *storage_keys);
> +void s390_init_cpus(const char *cpu_model);
>   void s390_init_ipl_dev(const char *kernel_filename,
>                          const char *kernel_cmdline,
>                          const char *initrd_filename,
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index 68b5ab7..b1432c7 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -381,6 +381,9 @@ static inline void kvm_s390_interrupt_internal(S390CPU *cpu, int type,
>   {
>   }
>   #endif
> +
> +extern uint8_t *storage_keys;
> +
>   S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
>   void s390_add_running_cpu(S390CPU *cpu);
>   unsigned s390_del_running_cpu(S390CPU *cpu);
>

Ping.

-- 
-- Jason J. Herne (jjherne@linux.vnet.ibm.com)

Re: [Qemu-devel] [PATCH] s390: Storage key global access

[Christian Borntraeger]

On 22/01/14 16:48, Jason J. Herne wrote:
> From: "Jason J. Herne" <jjherne@us.ibm.com>
> 
> Introduces global access to storage key data so we can set it for each cpu in
> the S390 cpu initialization routine.
> 
> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
> ---
>  hw/s390x/s390-virtio-ccw.c | 3 +--
>  hw/s390x/s390-virtio.c     | 6 +++---
>  hw/s390x/s390-virtio.h     | 2 +-
>  target-s390x/cpu.h         | 3 +++
>  4 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 733d988..62319b9 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -80,7 +80,6 @@ static void ccw_init(QEMUMachineInitArgs *args)
>      MemoryRegion *sysmem = get_system_memory();
>      MemoryRegion *ram = g_new(MemoryRegion, 1);
>      int shift = 0;
> -    uint8_t *storage_keys;
>      int ret;
>      VirtualCssBus *css_bus;
> 
> @@ -112,7 +111,7 @@ static void ccw_init(QEMUMachineInitArgs *args)
>      storage_keys = g_malloc0(my_ram_size / TARGET_PAGE_SIZE);
> 
>      /* init CPUs */
> -    s390_init_cpus(args->cpu_model, storage_keys);
> +    s390_init_cpus(args->cpu_model);
> 
>      if (kvm_enabled()) {
>          kvm_s390_enable_css_support(s390_cpu_addr2state(0));
> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
> index 7adf92a..804483f 100644
> --- a/hw/s390x/s390-virtio.c
> +++ b/hw/s390x/s390-virtio.c
> @@ -53,6 +53,7 @@
> 
>  static VirtIOS390Bus *s390_bus;
>  static S390CPU **ipi_states;
> +uint8_t *storage_keys;

This would add another global variable. I am find with this right now
but somewhen in the future we might want to take care of storage keys 
in regard to migration as well. 

Could we add a container-device/memory? We could make it
a child of the machine then? Dont know if that will work out with
migration, though.

Christian

Re: [Qemu-devel] [PATCH] s390: Storage key global access

[Jason J. Herne]

On 02/06/2014 10:19 AM, Christian Borntraeger wrote:
> On 22/01/14 16:48, Jason J. Herne wrote:
>> From: "Jason J. Herne" <jjherne@us.ibm.com>
>>
>> Introduces global access to storage key data so we can set it for each cpu in
>> the S390 cpu initialization routine.
>>
>> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
>> ---
>>   hw/s390x/s390-virtio-ccw.c | 3 +--
>>   hw/s390x/s390-virtio.c     | 6 +++---
>>   hw/s390x/s390-virtio.h     | 2 +-
>>   target-s390x/cpu.h         | 3 +++
>>   4 files changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 733d988..62319b9 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -80,7 +80,6 @@ static void ccw_init(QEMUMachineInitArgs *args)
>>       MemoryRegion *sysmem = get_system_memory();
>>       MemoryRegion *ram = g_new(MemoryRegion, 1);
>>       int shift = 0;
>> -    uint8_t *storage_keys;
>>       int ret;
>>       VirtualCssBus *css_bus;
>>
>> @@ -112,7 +111,7 @@ static void ccw_init(QEMUMachineInitArgs *args)
>>       storage_keys = g_malloc0(my_ram_size / TARGET_PAGE_SIZE);
>>
>>       /* init CPUs */
>> -    s390_init_cpus(args->cpu_model, storage_keys);
>> +    s390_init_cpus(args->cpu_model);
>>
>>       if (kvm_enabled()) {
>>           kvm_s390_enable_css_support(s390_cpu_addr2state(0));
>> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
>> index 7adf92a..804483f 100644
>> --- a/hw/s390x/s390-virtio.c
>> +++ b/hw/s390x/s390-virtio.c
>> @@ -53,6 +53,7 @@
>>
>>   static VirtIOS390Bus *s390_bus;
>>   static S390CPU **ipi_states;
>> +uint8_t *storage_keys;
>
> This would add another global variable. I am find with this right now
> but somewhen in the future we might want to take care of storage keys
> in regard to migration as well.
>
> Could we add a container-device/memory? We could make it
> a child of the machine then? Dont know if that will work out with
> migration, though.
>
> Christian

Sorry for the delay in responding to this. I've been busy tracking down 
some kernel bugs.

I'm not 100% sure what a storage key device would look like at the 
moment. However I agree that a device is the cleanest approach. I 
support sticking with the current implementation for now as it gets us 
cpu hotplug now, as opposed to later.

I do have some concerns with respect to migration if storage keys are a 
separate device. I think that is a discussion for a different time though.

Christian, at one point you mentioned that it might be helpful to see 
this patch in the context of the rest of the hotplug patches. If you 
still feel this way let me know and I'll post the 4-patch series. If 
not, I still propose this one for s390-next. Thanks :).

-- 
-- Jason J. Herne (jjherne@linux.vnet.ibm.com)

Re: [Qemu-devel] [PATCH] s390: Storage key global access

[Christian Borntraeger]

On 25/02/14 15:34, Jason J. Herne wrote:

> Christian, at one point you mentioned that it might be helpful to see this patch in the context of the rest of the hotplug patches. If you still feel this way let me know and I'll post the 4-patch series. If not, I still propose this one for s390-next. Thanks :).

Do you feel your series is ready for upstream, then yet please post the whole series. 
Posting independent things is good, but I feel that the storage key rework makes more
sense if the followup patches make clear why.

Christian

Re: [Qemu-devel] [PATCH] s390: Storage key global access

[Andreas Färber]

Hi,

Am 25.02.2014 20:15, schrieb Christian Borntraeger:
> On 25/02/14 15:34, Jason J. Herne wrote:
> 
>> Christian, at one point you mentioned that it might be helpful to see this patch in the context of the rest of the hotplug patches. If you still feel this way let me know and I'll post the 4-patch series. If not, I still propose this one for s390-next. Thanks :).
> 
> Do you feel your series is ready for upstream, then yet please post the whole series. 
> Posting independent things is good, but I feel that the storage key rework makes more
> sense if the followup patches make clear why.

I had requested changes to that series that apparently I could not
communicate in a form Jason could digest, and I have since been caught
in downstream work and a backlog of other patches, not getting to
writing the alternative myself yet nor will I the next few days.

An outline of the idea as far as I remember was dropping the ipi array
instead of refactoring it to dynamic allocation and - having discussed
that a topology will not be needed - add them as cpu[n] child<s390-cpu>
properties of /machine, allowing access via QOM property getters instead
of some self-cooked solution. Open question was link<> or child<>
property and, if link<>, whether some setter hook in QOM infrastructure
may be needed to trigger the hot-add or whether QOM realize event will
be sufficient.

I'd still be interested in getting vCPU hotplug for s390x in 2.0, so
maybe you can re-read my previous comments with a view to making -device
and cpu-add work with minimum workarounds on your own? We still don't
have model subclasses BTW, do we?

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] s390: Storage key global access

[Christian Borntraeger]

On 25/02/14 20:34, Andreas Färber wrote:
> Hi,
> 
> Am 25.02.2014 20:15, schrieb Christian Borntraeger:
>> On 25/02/14 15:34, Jason J. Herne wrote:
>>
>>> Christian, at one point you mentioned that it might be helpful to see this patch in the context of the rest of the hotplug patches. If you still feel this way let me know and I'll post the 4-patch series. If not, I still propose this one for s390-next. Thanks :).
>>
>> Do you feel your series is ready for upstream, then yet please post the whole series. 
>> Posting independent things is good, but I feel that the storage key rework makes more
>> sense if the followup patches make clear why.
> 
> I had requested changes to that series that apparently I could not
> communicate in a form Jason could digest, and I have since been caught
> in downstream work and a backlog of other patches, not getting to
> writing the alternative myself yet nor will I the next few days.

I think posting the full series is the right thing to do. We already merged
the sclp related changes, so the leftover patch(es) should be pretty small.
Maybe the current state is already pretty close to what you want.

> 
> An outline of the idea as far as I remember was dropping the ipi array
> instead of refactoring it to dynamic allocation and - having discussed
> that a topology will not be needed - add them as cpu[n] child<s390-cpu>
> properties of /machine, allowing access via QOM property getters instead
> of some self-cooked solution. Open question was link<> or child<>
> property and, if link<>, whether some setter hook in QOM infrastructure
> may be needed to trigger the hot-add or whether QOM realize event will
> be sufficient.
> 
> I'd still be interested in getting vCPU hotplug for s390x in 2.0, so
> maybe you can re-read my previous comments with a view to making -device
> and cpu-add work with minimum workarounds on your own? We still don't
> have model subclasses BTW, do we?
> 
> Regards,
> Andreas
> 

Re: [Qemu-devel] [PATCH] s390: Storage key global access

[Jason J. Herne]

On 02/25/2014 02:34 PM, Andreas Färber wrote:
> Hi,
>
> Am 25.02.2014 20:15, schrieb Christian Borntraeger:
>> On 25/02/14 15:34, Jason J. Herne wrote:
>>
>>> Christian, at one point you mentioned that it might be helpful to see this patch in the context of the rest of the hotplug patches. If you still feel this way let me know and I'll post the 4-patch series. If not, I still propose this one for s390-next. Thanks :).
>>
>> Do you feel your series is ready for upstream, then yet please post the whole series.
>> Posting independent things is good, but I feel that the storage key rework makes more
>> sense if the followup patches make clear why.
>
> I had requested changes to that series that apparently I could not
> communicate in a form Jason could digest, and I have since been caught
> in downstream work and a backlog of other patches, not getting to
> writing the alternative myself yet nor will I the next few days.
>

Andreas,

I believe I understood about 90% of what you were asking for. I had made 
many
of the changes you requested. The last version of my patches can be seen 
here

https://www.mail-archive.com/qemu-devel@nongnu.org/msg201796.html

Most of the requested changes should be found within this set of patches.

> An outline of the idea as far as I remember was dropping the ipi array
> instead of refactoring it to dynamic allocation and - having discussed
> that a topology will not be needed - add them as cpu[n] child<s390-cpu>
> properties of /machine, allowing access via QOM property getters instead
> of some self-cooked solution. Open question was link<> or child<>
> property and, if link<>, whether some setter hook in QOM infrastructure
> may be needed to trigger the hot-add or whether QOM realize event will
> be sufficient.

As per your original suggestion to try link<>, I wrote the code to do 
exactly
that. Please see patch 6/8 in the above series.

I have a version of these patches rebased for the latest master, but my test
system is currently in use. I would like a chance to regression test them
before I post them. I anticipate I should be able to do this today.

At your option, you may opt to review the patches as posted at the above 
link.
While some modifications have since been made, the overall design and 
function
is the same.

I look forward to your comments. Thank you for your time.

-- 
-- Jason J. Herne (jjherne@linux.vnet.ibm.com)