[Qemu-devel] [PATCH v2 0/3] Guest Support for Diagnose 318

[Qemu-devel] [PATCH v2 0/3] Guest Support for Diagnose 318

[Collin Walling]

Changelog:

    RFC -> v1
        - introduced kvm stubs for set/get cpc
        - s/fac134/byte_134
        - moved diag318 vmstate description to diag.c
        - reduced S390_VCPU_MAX to 247

     v1 -> v2
        - split patches into header sync, cpu model feat, and diag migration / reset
        - tiny cleanups

Collin Walling (3):
  s390: linux header sync for diag318
  s390: cpu feature for diagnose 318 andlimit max VCPUs to 247
  s390: migration and reset support for diagnose 318

 hw/s390x/ipl.c                  |  3 +++
 hw/s390x/s390-virtio-ccw.c      |  3 +++
 hw/s390x/sclp.c                 |  2 ++
 include/hw/s390x/sclp.h         |  2 ++
 linux-headers/asm-s390/kvm.h    |  5 ++++
 target/s390x/cpu.h              |  2 +-
 target/s390x/cpu_features.c     |  3 +++
 target/s390x/cpu_features.h     |  1 +
 target/s390x/cpu_features_def.h |  3 +++
 target/s390x/diag.c             | 52 +++++++++++++++++++++++++++++++++++++++++
 target/s390x/gen-features.c     |  1 +
 target/s390x/internal.h         |  4 +++-
 target/s390x/kvm-stub.c         | 10 ++++++++
 target/s390x/kvm.c              | 23 ++++++++++++++++++
 target/s390x/kvm_s390x.h        |  2 ++
 15 files changed, 114 insertions(+), 2 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v2 1/3] s390: linux header sync for diagnose 318

[Collin Walling]

Introduce VM group, attribute, and CPU feat defines for
diagnose 318.

Signed-off-by: Collin Walling <walling@linux.ibm.com>
---
 linux-headers/asm-s390/kvm.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/linux-headers/asm-s390/kvm.h b/linux-headers/asm-s390/kvm.h
index 0265482..4b655e3 100644
--- a/linux-headers/asm-s390/kvm.h
+++ b/linux-headers/asm-s390/kvm.h
@@ -74,6 +74,7 @@ struct kvm_s390_io_adapter_req {
 #define KVM_S390_VM_CRYPTO		2
 #define KVM_S390_VM_CPU_MODEL		3
 #define KVM_S390_VM_MIGRATION		4
+#define KVM_S390_VM_MACHINE		5
 
 /* kvm attributes for mem_ctrl */
 #define KVM_S390_VM_MEM_ENABLE_CMMA	0
@@ -130,6 +131,7 @@ struct kvm_s390_vm_cpu_machine {
 #define KVM_S390_VM_CPU_FEAT_PFMFI	11
 #define KVM_S390_VM_CPU_FEAT_SIGPIF	12
 #define KVM_S390_VM_CPU_FEAT_KSS	13
+#define KVM_S390_VM_CPU_FEAT_DIAG318	14
 struct kvm_s390_vm_cpu_feat {
 	__u64 feat[16];
 };
@@ -168,6 +170,9 @@ struct kvm_s390_vm_cpu_subfunc {
 #define KVM_S390_VM_MIGRATION_START	1
 #define KVM_S390_VM_MIGRATION_STATUS	2
 
+/* kvm attributes for KVM_S390_VM_MACHINE */
+#define KVM_S390_VM_MACHINE_CPC	0
+
 /* for KVM_GET_REGS and KVM_SET_REGS */
 struct kvm_regs {
 	/* general purpose regs for s390 */
-- 
2.7.4

[Qemu-devel] [PATCH v2 2/3] s390: cpu feature for diagnose 318 andlimit max VCPUs to 247

[Collin Walling]

Diagnose 318 is a new z14.2 CPU feature. Since we are able to emulate
it entirely via KVM, we can add guest support for earlier models. A
new CPU feature for diagnose 318 (shortened to diag318) will be made
available to guests starting with the zEC12-full CPU model.

The z14.2 adds a new read SCP info byte (let's call it byte 134) to
detect the availability of diag318. Because of this, we have room for
one less VCPU and thus limit the max VPUs supported in a configuration
to 247 (down from 248).

Signed-off-by: Collin Walling <walling@linux.ibm.com>.
---
 hw/s390x/sclp.c                 | 2 ++
 include/hw/s390x/sclp.h         | 2 ++
 target/s390x/cpu.h              | 2 +-
 target/s390x/cpu_features.c     | 3 +++
 target/s390x/cpu_features.h     | 1 +
 target/s390x/cpu_features_def.h | 3 +++
 target/s390x/gen-features.c     | 1 +
 target/s390x/kvm.c              | 1 +
 8 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 4510a80..183c627 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -73,6 +73,8 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
                          read_info->conf_char);
     s390_get_feat_block(S390_FEAT_TYPE_SCLP_CONF_CHAR_EXT,
                          read_info->conf_char_ext);
+    /* Read Info byte 134 */
+    s390_get_feat_block(S390_FEAT_TYPE_SCLP_BYTE_134, read_info->byte_134);
 
     read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO |
                                         SCLP_HAS_IOA_RECONFIG);
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index f9db243..eb12ba2 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -133,6 +133,8 @@ typedef struct ReadInfo {
     uint16_t highest_cpu;
     uint8_t  _reserved5[124 - 122];     /* 122-123 */
     uint32_t hmfai;
+    uint8_t  _reserved7[134 - 128];     /* 128-133 */
+    uint8_t  byte_134[1];
     struct CPUEntry entries[0];
 } QEMU_PACKED ReadInfo;
 
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 8c2320e..594b4a4 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -52,7 +52,7 @@
 
 #define MMU_USER_IDX 0
 
-#define S390_MAX_CPUS 248
+#define S390_MAX_CPUS 247
 
 typedef struct PSW {
     uint64_t mask;
diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
index 60cfeba..d05afa5 100644
--- a/target/s390x/cpu_features.c
+++ b/target/s390x/cpu_features.c
@@ -121,6 +121,9 @@ static const S390FeatDef s390_features[] = {
     FEAT_INIT("pfmfi", S390_FEAT_TYPE_SCLP_CONF_CHAR_EXT, 9, "SIE: PFMF interpretation facility"),
     FEAT_INIT("ibs", S390_FEAT_TYPE_SCLP_CONF_CHAR_EXT, 10, "SIE: Interlock-and-broadcast-suppression facility"),
 
+    /* SCLP SCCB Byte 134 */
+    FEAT_INIT("diag318", S390_FEAT_TYPE_SCLP_BYTE_134, 0, "SIE: Diagnose 318"),
+
     FEAT_INIT("sief2", S390_FEAT_TYPE_SCLP_CPU, 4, "SIE: interception format 2 (Virtual SIE)"),
     FEAT_INIT("skey", S390_FEAT_TYPE_SCLP_CPU, 5, "SIE: Storage-key facility"),
     FEAT_INIT("gpereh", S390_FEAT_TYPE_SCLP_CPU, 10, "SIE: Guest-PER enhancement facility"),
diff --git a/target/s390x/cpu_features.h b/target/s390x/cpu_features.h
index effe790..e7248df 100644
--- a/target/s390x/cpu_features.h
+++ b/target/s390x/cpu_features.h
@@ -23,6 +23,7 @@ typedef enum {
     S390_FEAT_TYPE_STFL,
     S390_FEAT_TYPE_SCLP_CONF_CHAR,
     S390_FEAT_TYPE_SCLP_CONF_CHAR_EXT,
+    S390_FEAT_TYPE_SCLP_BYTE_134,
     S390_FEAT_TYPE_SCLP_CPU,
     S390_FEAT_TYPE_MISC,
     S390_FEAT_TYPE_PLO,
diff --git a/target/s390x/cpu_features_def.h b/target/s390x/cpu_features_def.h
index 5fc7e7b..d99da1d 100644
--- a/target/s390x/cpu_features_def.h
+++ b/target/s390x/cpu_features_def.h
@@ -109,6 +109,9 @@ typedef enum {
     S390_FEAT_SIE_PFMFI,
     S390_FEAT_SIE_IBS,
 
+    /* Read Info Byte 134 */
+    S390_FEAT_DIAG318,
+
     /* Sclp Cpu */
     S390_FEAT_SIE_F2,
     S390_FEAT_SIE_SKEY,
diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
index 70015ea..a3d1457 100644
--- a/target/s390x/gen-features.c
+++ b/target/s390x/gen-features.c
@@ -450,6 +450,7 @@ static uint16_t full_GEN12_GA1[] = {
     S390_FEAT_AP_QUERY_CONFIG_INFO,
     S390_FEAT_AP_FACILITIES_TEST,
     S390_FEAT_AP,
+    S390_FEAT_DIAG318,
 };
 
 static uint16_t full_GEN12_GA2[] = {
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 2ebf26a..3915e36 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -2142,6 +2142,7 @@ static int kvm_to_feat[][2] = {
     { KVM_S390_VM_CPU_FEAT_PFMFI, S390_FEAT_SIE_PFMFI},
     { KVM_S390_VM_CPU_FEAT_SIGPIF, S390_FEAT_SIE_SIGPIF},
     { KVM_S390_VM_CPU_FEAT_KSS, S390_FEAT_SIE_KSS},
+    { KVM_S390_VM_CPU_FEAT_DIAG318, S390_FEAT_DIAG318},
 };
 
 static int query_cpu_feat(S390FeatBitmap features)
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 2/3] s390: cpu feature for diagnose 318 andlimit max VCPUs to 247

[Cornelia Huck]

On Thu,  6 Dec 2018 17:24:17 -0500
Collin Walling <walling@linux.ibm.com> wrote:

> Diagnose 318 is a new z14.2 CPU feature. Since we are able to emulate
> it entirely via KVM, we can add guest support for earlier models. A
> new CPU feature for diagnose 318 (shortened to diag318) will be made
> available to guests starting with the zEC12-full CPU model.
> 
> The z14.2 adds a new read SCP info byte (let's call it byte 134) to
> detect the availability of diag318. Because of this, we have room for
> one less VCPU and thus limit the max VPUs supported in a configuration
> to 247 (down from 248).
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>.
> ---
>  hw/s390x/sclp.c                 | 2 ++
>  include/hw/s390x/sclp.h         | 2 ++
>  target/s390x/cpu.h              | 2 +-
>  target/s390x/cpu_features.c     | 3 +++
>  target/s390x/cpu_features.h     | 1 +
>  target/s390x/cpu_features_def.h | 3 +++
>  target/s390x/gen-features.c     | 1 +
>  target/s390x/kvm.c              | 1 +
>  8 files changed, 14 insertions(+), 1 deletion(-)
> 

> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 8c2320e..594b4a4 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -52,7 +52,7 @@
>  
>  #define MMU_USER_IDX 0
>  
> -#define S390_MAX_CPUS 248
> +#define S390_MAX_CPUS 247

Isn't that already problematic if you try to migrate from an older QEMU
with all possible vcpus defined? IOW, don't you really need a way that
older machines can still run with one more vcpu?

>  
>  typedef struct PSW {
>      uint64_t mask;

Re: [Qemu-devel] [qemu-s390x] [PATCH v2 2/3] s390: cpu feature for diagnose 318 andlimit max VCPUs to 247

[Collin Walling]

On 12/7/18 7:08 AM, Cornelia Huck wrote:
> On Thu,  6 Dec 2018 17:24:17 -0500
> Collin Walling <walling@linux.ibm.com> wrote:
> 
>> Diagnose 318 is a new z14.2 CPU feature. Since we are able to emulate
>> it entirely via KVM, we can add guest support for earlier models. A
>> new CPU feature for diagnose 318 (shortened to diag318) will be made
>> available to guests starting with the zEC12-full CPU model.
>>
>> The z14.2 adds a new read SCP info byte (let's call it byte 134) to
>> detect the availability of diag318. Because of this, we have room for
>> one less VCPU and thus limit the max VPUs supported in a configuration
>> to 247 (down from 248).
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>.
>> ---
>>  hw/s390x/sclp.c                 | 2 ++
>>  include/hw/s390x/sclp.h         | 2 ++
>>  target/s390x/cpu.h              | 2 +-
>>  target/s390x/cpu_features.c     | 3 +++
>>  target/s390x/cpu_features.h     | 1 +
>>  target/s390x/cpu_features_def.h | 3 +++
>>  target/s390x/gen-features.c     | 1 +
>>  target/s390x/kvm.c              | 1 +
>>  8 files changed, 14 insertions(+), 1 deletion(-)
>>
> 
>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>> index 8c2320e..594b4a4 100644
>> --- a/target/s390x/cpu.h
>> +++ b/target/s390x/cpu.h
>> @@ -52,7 +52,7 @@
>>  
>>  #define MMU_USER_IDX 0
>>  
>> -#define S390_MAX_CPUS 248
>> +#define S390_MAX_CPUS 247
> 
> Isn't that already problematic if you try to migrate from an older QEMU
> with all possible vcpus defined? IOW, don't you really need a way that
> older machines can still run with one more vcpu?
> 

Good call. I'll run some tests on this and see what happens. I'll report
here on those results.

>>  
>>  typedef struct PSW {
>>      uint64_t mask;
> 


-- 
Respectfully,
- Collin Walling

Re: [Qemu-devel] [qemu-s390x] [PATCH v2 2/3] s390: cpu feature for diagnose 318 andlimit max VCPUs to 247

[Collin Walling]

On 12/11/18 11:47 AM, Collin Walling wrote:
> On 12/7/18 7:08 AM, Cornelia Huck wrote:
>> On Thu,  6 Dec 2018 17:24:17 -0500
>> Collin Walling <walling@linux.ibm.com> wrote:
>>
>>> Diagnose 318 is a new z14.2 CPU feature. Since we are able to emulate
>>> it entirely via KVM, we can add guest support for earlier models. A
>>> new CPU feature for diagnose 318 (shortened to diag318) will be made
>>> available to guests starting with the zEC12-full CPU model.
>>>
>>> The z14.2 adds a new read SCP info byte (let's call it byte 134) to
>>> detect the availability of diag318. Because of this, we have room for
>>> one less VCPU and thus limit the max VPUs supported in a configuration
>>> to 247 (down from 248).
>>>
>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>.
>>> ---
>>>  hw/s390x/sclp.c                 | 2 ++
>>>  include/hw/s390x/sclp.h         | 2 ++
>>>  target/s390x/cpu.h              | 2 +-
>>>  target/s390x/cpu_features.c     | 3 +++
>>>  target/s390x/cpu_features.h     | 1 +
>>>  target/s390x/cpu_features_def.h | 3 +++
>>>  target/s390x/gen-features.c     | 1 +
>>>  target/s390x/kvm.c              | 1 +
>>>  8 files changed, 14 insertions(+), 1 deletion(-)
>>>
>>
>>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>>> index 8c2320e..594b4a4 100644
>>> --- a/target/s390x/cpu.h
>>> +++ b/target/s390x/cpu.h
>>> @@ -52,7 +52,7 @@
>>>  
>>>  #define MMU_USER_IDX 0
>>>  
>>> -#define S390_MAX_CPUS 248
>>> +#define S390_MAX_CPUS 247
>>
>> Isn't that already problematic if you try to migrate from an older QEMU
>> with all possible vcpus defined? IOW, don't you really need a way that
>> older machines can still run with one more vcpu?
>>
> 
> Good call. I'll run some tests on this and see what happens. I'll report
> here on those results.
> 

Migrating to a machine that supports less vCPUs will report

error: unsupported configuration: Maximum CPUs greater than specified machine type limit

I revisited the code to see if there's a way to dynamically set the max vcpu count based 
on the read scp info size, but it gets really tricky and code looks very complicated.
(Having a packed struct contain the CPU entries whose maximum is determined by hardware
limitations makes things difficult -- but who said s390 is easy? :) )

In reality, do we often have guests running with 248 or even 247 vcpus? If so, I imagine
the performance isn't too significant?

>>>  
>>>  typedef struct PSW {
>>>      uint64_t mask;
>>
> 
> 


-- 
Respectfully,
- Collin Walling

Re: [Qemu-devel] [qemu-s390x] [PATCH v2 2/3] s390: cpu feature for diagnose 318 andlimit max VCPUs to 247

[David Hildenbrand]

On 11.12.18 22:12, Collin Walling wrote:
> On 12/11/18 11:47 AM, Collin Walling wrote:
>> On 12/7/18 7:08 AM, Cornelia Huck wrote:
>>> On Thu,  6 Dec 2018 17:24:17 -0500
>>> Collin Walling <walling@linux.ibm.com> wrote:
>>>
>>>> Diagnose 318 is a new z14.2 CPU feature. Since we are able to emulate
>>>> it entirely via KVM, we can add guest support for earlier models. A
>>>> new CPU feature for diagnose 318 (shortened to diag318) will be made
>>>> available to guests starting with the zEC12-full CPU model.
>>>>
>>>> The z14.2 adds a new read SCP info byte (let's call it byte 134) to
>>>> detect the availability of diag318. Because of this, we have room for
>>>> one less VCPU and thus limit the max VPUs supported in a configuration
>>>> to 247 (down from 248).
>>>>
>>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>.
>>>> ---
>>>>  hw/s390x/sclp.c                 | 2 ++
>>>>  include/hw/s390x/sclp.h         | 2 ++
>>>>  target/s390x/cpu.h              | 2 +-
>>>>  target/s390x/cpu_features.c     | 3 +++
>>>>  target/s390x/cpu_features.h     | 1 +
>>>>  target/s390x/cpu_features_def.h | 3 +++
>>>>  target/s390x/gen-features.c     | 1 +
>>>>  target/s390x/kvm.c              | 1 +
>>>>  8 files changed, 14 insertions(+), 1 deletion(-)
>>>>
>>>
>>>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>>>> index 8c2320e..594b4a4 100644
>>>> --- a/target/s390x/cpu.h
>>>> +++ b/target/s390x/cpu.h
>>>> @@ -52,7 +52,7 @@
>>>>  
>>>>  #define MMU_USER_IDX 0
>>>>  
>>>> -#define S390_MAX_CPUS 248
>>>> +#define S390_MAX_CPUS 247
>>>
>>> Isn't that already problematic if you try to migrate from an older QEMU
>>> with all possible vcpus defined? IOW, don't you really need a way that
>>> older machines can still run with one more vcpu?
>>>
>>
>> Good call. I'll run some tests on this and see what happens. I'll report
>> here on those results.
>>
> 
> Migrating to a machine that supports less vCPUs will report
> 
> error: unsupported configuration: Maximum CPUs greater than specified machine type limit
> 
> I revisited the code to see if there's a way to dynamically set the max vcpu count based 
> on the read scp info size, but it gets really tricky and code looks very complicated.
> (Having a packed struct contain the CPU entries whose maximum is determined by hardware
> limitations makes things difficult -- but who said s390 is easy? :) )
> 
> In reality, do we often have guests running with 248 or even 247 vcpus? If so, I imagine
> the performance isn't too significant?
Gluing CPU feature availability to machines is plain ugly. This sounds
like going back to pre-cpu model times ;)

There are two alternatives:

a) Don't model it as a CPU feature in QEMU. Glue it completely to the
QEMU machine. This goes hand-in-hand with the proposal I made in the KVM
thread, that diag318 is to be handled completely in QEMU, always. The
KVM setting part is optional (if KVM + HW support it).

Then we can have two different max_cpus/ReadInfo layouts based on the
machine type. No need to worry about QEMU cpu features.

Once we have other SCLP features (eventually requiring KVM/HW support)
announced in the same feature block, things might get more involved, but
I guess we could handle it somehow.


b) Glue the ReadInfo layout to the CPU feature, we would have to
default-disable the CPU feature for legacy machines. And bail out if
more CPUs are used when the feature is enabled. Hairy.


I guess a) would be the best thing to do. After all this really does not
sound like a CPU feature but more like a machine feature. But there is
usually a fine line between them.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH v2 2/3] s390: cpu feature for diagnose 318 andlimit max VCPUs to 247

[Cornelia Huck]

On Wed, 12 Dec 2018 12:20:08 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 11.12.18 22:12, Collin Walling wrote:
> > On 12/11/18 11:47 AM, Collin Walling wrote:  
> >> On 12/7/18 7:08 AM, Cornelia Huck wrote:  
> >>> On Thu,  6 Dec 2018 17:24:17 -0500
> >>> Collin Walling <walling@linux.ibm.com> wrote:
> >>>  
> >>>> Diagnose 318 is a new z14.2 CPU feature. Since we are able to emulate
> >>>> it entirely via KVM, we can add guest support for earlier models. A
> >>>> new CPU feature for diagnose 318 (shortened to diag318) will be made
> >>>> available to guests starting with the zEC12-full CPU model.
> >>>>
> >>>> The z14.2 adds a new read SCP info byte (let's call it byte 134) to
> >>>> detect the availability of diag318. Because of this, we have room for
> >>>> one less VCPU and thus limit the max VPUs supported in a configuration
> >>>> to 247 (down from 248).
> >>>>
> >>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>.
> >>>> ---
> >>>>  hw/s390x/sclp.c                 | 2 ++
> >>>>  include/hw/s390x/sclp.h         | 2 ++
> >>>>  target/s390x/cpu.h              | 2 +-
> >>>>  target/s390x/cpu_features.c     | 3 +++
> >>>>  target/s390x/cpu_features.h     | 1 +
> >>>>  target/s390x/cpu_features_def.h | 3 +++
> >>>>  target/s390x/gen-features.c     | 1 +
> >>>>  target/s390x/kvm.c              | 1 +
> >>>>  8 files changed, 14 insertions(+), 1 deletion(-)
> >>>>  
> >>>  
> >>>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> >>>> index 8c2320e..594b4a4 100644
> >>>> --- a/target/s390x/cpu.h
> >>>> +++ b/target/s390x/cpu.h
> >>>> @@ -52,7 +52,7 @@
> >>>>  
> >>>>  #define MMU_USER_IDX 0
> >>>>  
> >>>> -#define S390_MAX_CPUS 248
> >>>> +#define S390_MAX_CPUS 247  
> >>>
> >>> Isn't that already problematic if you try to migrate from an older QEMU
> >>> with all possible vcpus defined? IOW, don't you really need a way that
> >>> older machines can still run with one more vcpu?
> >>>  
> >>
> >> Good call. I'll run some tests on this and see what happens. I'll report
> >> here on those results.
> >>  
> > 
> > Migrating to a machine that supports less vCPUs will report
> > 
> > error: unsupported configuration: Maximum CPUs greater than specified machine type limit
> > 
> > I revisited the code to see if there's a way to dynamically set the max vcpu count based 
> > on the read scp info size, but it gets really tricky and code looks very complicated.
> > (Having a packed struct contain the CPU entries whose maximum is determined by hardware
> > limitations makes things difficult -- but who said s390 is easy? :) )
> > 
> > In reality, do we often have guests running with 248 or even 247 vcpus? If so, I imagine
> > the performance isn't too significant?  
> Gluing CPU feature availability to machines is plain ugly. This sounds
> like going back to pre-cpu model times ;)
> 
> There are two alternatives:
> 
> a) Don't model it as a CPU feature in QEMU. Glue it completely to the
> QEMU machine. This goes hand-in-hand with the proposal I made in the KVM
> thread, that diag318 is to be handled completely in QEMU, always. The
> KVM setting part is optional (if KVM + HW support it).
> 
> Then we can have two different max_cpus/ReadInfo layouts based on the
> machine type. No need to worry about QEMU cpu features.
> 
> Once we have other SCLP features (eventually requiring KVM/HW support)
> announced in the same feature block, things might get more involved, but
> I guess we could handle it somehow.

Perhaps via a capability to be enabled?

> 
> 
> b) Glue the ReadInfo layout to the CPU feature, we would have to
> default-disable the CPU feature for legacy machines. And bail out if
> more CPUs are used when the feature is enabled. Hairy.
> 
> 
> I guess a) would be the best thing to do. After all this really does not
> sound like a CPU feature but more like a machine feature. But there is
> usually a fine line between them.

a) sounds like the better option to me as well.

Re: [Qemu-devel] [qemu-s390x] [PATCH v2 2/3] s390: cpu feature for diagnose 318 andlimit max VCPUs to 247

[Collin Walling]

On 12/12/18 8:41 AM, Cornelia Huck wrote:
> On Wed, 12 Dec 2018 12:20:08 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 11.12.18 22:12, Collin Walling wrote:
>>> On 12/11/18 11:47 AM, Collin Walling wrote:  
>>>> On 12/7/18 7:08 AM, Cornelia Huck wrote:  
>>>>> On Thu,  6 Dec 2018 17:24:17 -0500
>>>>> Collin Walling <walling@linux.ibm.com> wrote:
>>>>>  
>>>>>> Diagnose 318 is a new z14.2 CPU feature. Since we are able to emulate
>>>>>> it entirely via KVM, we can add guest support for earlier models. A
>>>>>> new CPU feature for diagnose 318 (shortened to diag318) will be made
>>>>>> available to guests starting with the zEC12-full CPU model.
>>>>>>
>>>>>> The z14.2 adds a new read SCP info byte (let's call it byte 134) to
>>>>>> detect the availability of diag318. Because of this, we have room for
>>>>>> one less VCPU and thus limit the max VPUs supported in a configuration
>>>>>> to 247 (down from 248).
>>>>>>
>>>>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>.
>>>>>> ---
>>>>>>  hw/s390x/sclp.c                 | 2 ++
>>>>>>  include/hw/s390x/sclp.h         | 2 ++
>>>>>>  target/s390x/cpu.h              | 2 +-
>>>>>>  target/s390x/cpu_features.c     | 3 +++
>>>>>>  target/s390x/cpu_features.h     | 1 +
>>>>>>  target/s390x/cpu_features_def.h | 3 +++
>>>>>>  target/s390x/gen-features.c     | 1 +
>>>>>>  target/s390x/kvm.c              | 1 +
>>>>>>  8 files changed, 14 insertions(+), 1 deletion(-)
>>>>>>  
>>>>>  
>>>>>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>>>>>> index 8c2320e..594b4a4 100644
>>>>>> --- a/target/s390x/cpu.h
>>>>>> +++ b/target/s390x/cpu.h
>>>>>> @@ -52,7 +52,7 @@
>>>>>>  
>>>>>>  #define MMU_USER_IDX 0
>>>>>>  
>>>>>> -#define S390_MAX_CPUS 248
>>>>>> +#define S390_MAX_CPUS 247  
>>>>>
>>>>> Isn't that already problematic if you try to migrate from an older QEMU
>>>>> with all possible vcpus defined? IOW, don't you really need a way that
>>>>> older machines can still run with one more vcpu?
>>>>>  
>>>>
>>>> Good call. I'll run some tests on this and see what happens. I'll report
>>>> here on those results.
>>>>  
>>>
>>> Migrating to a machine that supports less vCPUs will report
>>>
>>> error: unsupported configuration: Maximum CPUs greater than specified machine type limit
>>>
>>> I revisited the code to see if there's a way to dynamically set the max vcpu count based 
>>> on the read scp info size, but it gets really tricky and code looks very complicated.
>>> (Having a packed struct contain the CPU entries whose maximum is determined by hardware
>>> limitations makes things difficult -- but who said s390 is easy? :) )
>>>
>>> In reality, do we often have guests running with 248 or even 247 vcpus? If so, I imagine
>>> the performance isn't too significant?  
>> Gluing CPU feature availability to machines is plain ugly. This sounds
>> like going back to pre-cpu model times ;)
>>
>> There are two alternatives:
>>
>> a) Don't model it as a CPU feature in QEMU. Glue it completely to the
>> QEMU machine. This goes hand-in-hand with the proposal I made in the KVM
>> thread, that diag318 is to be handled completely in QEMU, always. The
>> KVM setting part is optional (if KVM + HW support it).
>>
>> Then we can have two different max_cpus/ReadInfo layouts based on the
>> machine type. No need to worry about QEMU cpu features.
>>
>> Once we have other SCLP features (eventually requiring KVM/HW support)
>> announced in the same feature block, things might get more involved, but
>> I guess we could handle it somehow.
> 
> Perhaps via a capability to be enabled?
> 
>>
>>
>> b) Glue the ReadInfo layout to the CPU feature, we would have to
>> default-disable the CPU feature for legacy machines. And bail out if
>> more CPUs are used when the feature is enabled. Hairy.
>>
>>
>> I guess a) would be the best thing to do. After all this really does not
>> sound like a CPU feature but more like a machine feature. But there is
>> usually a fine line between them.
> 
> a) sounds like the better option to me as well.
> 

I think this makes sense as well. A CPU feat really doesn't make sense if we 
just want to enable this "always" so-to-speak. I'll get cracking on a rework
of this patch series. It'll take me some time.

In the mean time, I'll return the favor and take a look at the PCI stuff you
guys have posted ;)

-- 
Respectfully,
- Collin Walling

Re: [Qemu-devel] [PATCH v2 2/3] s390: cpu feature for diagnose 318 andlimit max VCPUs to 247

[Christian Borntraeger]

On 06.12.2018 23:24, Collin Walling wrote:
> Diagnose 318 is a new z14.2 CPU feature. Since we are able to emulate
> it entirely via KVM, we can add guest support for earlier models. A
> new CPU feature for diagnose 318 (shortened to diag318) will be made
> available to guests starting with the zEC12-full CPU model.
> 
> The z14.2 adds a new read SCP info byte (let's call it byte 134) to
> detect the availability of diag318. Because of this, we have room for
> one less VCPU and thus limit the max VPUs supported in a configuration
> to 247 (down from 248).
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>.
> ---
>  hw/s390x/sclp.c                 | 2 ++
>  include/hw/s390x/sclp.h         | 2 ++
>  target/s390x/cpu.h              | 2 +-
>  target/s390x/cpu_features.c     | 3 +++
>  target/s390x/cpu_features.h     | 1 +
>  target/s390x/cpu_features_def.h | 3 +++
>  target/s390x/gen-features.c     | 1 +
>  target/s390x/kvm.c              | 1 +
>  8 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 4510a80..183c627 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -73,6 +73,8 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>                           read_info->conf_char);
>      s390_get_feat_block(S390_FEAT_TYPE_SCLP_CONF_CHAR_EXT,
>                           read_info->conf_char_ext);
> +    /* Read Info byte 134 */
> +    s390_get_feat_block(S390_FEAT_TYPE_SCLP_BYTE_134, read_info->byte_134);
>  
>      read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO |
>                                          SCLP_HAS_IOA_RECONFIG);
> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
> index f9db243..eb12ba2 100644
> --- a/include/hw/s390x/sclp.h
> +++ b/include/hw/s390x/sclp.h
> @@ -133,6 +133,8 @@ typedef struct ReadInfo {
>      uint16_t highest_cpu;
>      uint8_t  _reserved5[124 - 122];     /* 122-123 */
>      uint32_t hmfai;
> +    uint8_t  _reserved7[134 - 128];     /* 128-133 */
> +    uint8_t  byte_134[1];
>      struct CPUEntry entries[0];
>  } QEMU_PACKED ReadInfo;
 
The size must be a multiple of 16. Can you add a reserved field to fill up
until 144?

[Qemu-devel] [PATCH v2 3/3] s390: migration and reset support for diagnose 318

[Collin Walling]

Diagnose 318 contains data regarding the guest's name code
and version code (collectively called the cpc) and must be
captured for migration. Since this instruction is executed
in the kernel during IPL, we register the migration handlers
during IPL as well.

Also add a reset method for the diagnose 318 related data so
we can clear it during a machine reset.

Signed-off-by: Collin Walling <walling@linux.ibm.com>
---
 hw/s390x/ipl.c             |  3 +++
 hw/s390x/s390-virtio-ccw.c |  3 +++
 target/s390x/diag.c        | 52 ++++++++++++++++++++++++++++++++++++++++++++++
 target/s390x/internal.h    |  4 +++-
 target/s390x/kvm-stub.c    | 10 +++++++++
 target/s390x/kvm.c         | 22 ++++++++++++++++++++
 target/s390x/kvm_s390x.h   |  2 ++
 7 files changed, 95 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 21f64ad..6e3af65 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -27,6 +27,7 @@
 #include "qemu/cutils.h"
 #include "qemu/option.h"
 #include "exec/exec-all.h"
+#include "internal.h"
 
 #define KERN_IMAGE_START                0x010000UL
 #define LINUX_MAGIC_ADDR                0x010008UL
@@ -230,6 +231,8 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
     ipl->compat_start_addr = ipl->start_addr;
     ipl->compat_bios_start_addr = ipl->bios_start_addr;
     qemu_register_reset(qdev_reset_all_fn, dev);
+
+    diag318_register();
 error:
     error_propagate(errp, err);
 }
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index a0615a8..2c670fc 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -37,6 +37,7 @@
 #include "cpu_models.h"
 #include "hw/nmi.h"
 #include "hw/s390x/tod.h"
+#include "internal.h"
 
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
 {
@@ -357,6 +358,7 @@ static void s390_machine_reset(void)
         }
         subsystem_reset();
         s390_crypto_reset();
+        diag318_reset();
         run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
         break;
     case S390_RESET_LOAD_NORMAL:
@@ -364,6 +366,7 @@ static void s390_machine_reset(void)
             run_on_cpu(t, s390_do_cpu_reset, RUN_ON_CPU_NULL);
         }
         subsystem_reset();
+        diag318_reset();
         run_on_cpu(cs, s390_do_cpu_initial_reset, RUN_ON_CPU_NULL);
         run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
         break;
diff --git a/target/s390x/diag.c b/target/s390x/diag.c
index acb0f3d..935862d 100644
--- a/target/s390x/diag.c
+++ b/target/s390x/diag.c
@@ -20,6 +20,7 @@
 #include "sysemu/cpus.h"
 #include "hw/s390x/ipl.h"
 #include "hw/s390x/s390-virtio-ccw.h"
+#include "kvm_s390x.h"
 
 int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3)
 {
@@ -134,3 +135,54 @@ out:
         break;
     }
 }
+
+typedef struct Diag318Data {
+    uint64_t cpc;
+} Diag318Data;
+static Diag318Data diag318data;
+
+void diag318_reset(void)
+{
+    if (s390_has_feat(S390_FEAT_DIAG318)) {
+        kvm_s390_set_cpc(0);
+    }
+}
+
+static int diag318_post_load(void *opaque, int version_id)
+{
+    Diag318Data *d = opaque;
+
+    kvm_s390_set_cpc(d->cpc);
+    return 0;
+}
+
+static int diag318_pre_save(void *opaque)
+{
+    Diag318Data *d = opaque;
+
+    kvm_s390_get_cpc(&d->cpc);
+    return 0;
+}
+
+static bool diag318_needed(void *opaque)
+{
+    return s390_has_feat(S390_FEAT_DIAG318);
+}
+
+const VMStateDescription vmstate_diag318 = {
+    .name = "diag318",
+    .post_load = diag318_post_load,
+    .pre_save = diag318_pre_save,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = diag318_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT64(cpc, Diag318Data),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+void diag318_register(void)
+{
+    vmstate_register(NULL, 0, &vmstate_diag318, &diag318data);
+}
diff --git a/target/s390x/internal.h b/target/s390x/internal.h
index f2a771e..d53b1d0 100644
--- a/target/s390x/internal.h
+++ b/target/s390x/internal.h
@@ -383,10 +383,12 @@ int mmu_translate_real(CPUS390XState *env, target_ulong raddr, int rw,
                        target_ulong *addr, int *flags);
 
 
-/* misc_helper.c */
+/* diag.c */
 int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3);
 void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3,
                      uintptr_t ra);
+void diag318_register(void);
+void diag318_reset(void);
 
 
 /* translate.c */
diff --git a/target/s390x/kvm-stub.c b/target/s390x/kvm-stub.c
index bf7795e..1f0bee3 100644
--- a/target/s390x/kvm-stub.c
+++ b/target/s390x/kvm-stub.c
@@ -104,3 +104,13 @@ void kvm_s390_stop_interrupt(S390CPU *cpu)
 void kvm_s390_restart_interrupt(S390CPU *cpu)
 {
 }
+
+int kvm_s390_get_cpc(uint64_t *cpc)
+{
+    return 0;
+}
+
+int kvm_s390_set_cpc(uint64_t cpc)
+{
+    return 0;
+}
\ No newline at end of file
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 3915e36..432f8ea 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -749,6 +749,28 @@ int kvm_s390_set_clock_ext(uint8_t tod_high, uint64_t tod_low)
     return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
 }
 
+int kvm_s390_get_cpc(uint64_t *cpc)
+{
+    struct kvm_device_attr attr = {
+        .group = KVM_S390_VM_MACHINE,
+        .attr = KVM_S390_VM_MACHINE_CPC,
+        .addr = (uint64_t)cpc,
+    };
+
+    return kvm_vm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr);
+}
+
+int kvm_s390_set_cpc(uint64_t cpc)
+{
+    struct kvm_device_attr attr = {
+        .group = KVM_S390_VM_MACHINE,
+        .attr = KVM_S390_VM_MACHINE_CPC,
+        .addr = (uint64_t)&cpc,
+    };
+
+    return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
+}
+
 /**
  * kvm_s390_mem_op:
  * @addr:      the logical start address in guest memory
diff --git a/target/s390x/kvm_s390x.h b/target/s390x/kvm_s390x.h
index 6e52287..8c7b6a8 100644
--- a/target/s390x/kvm_s390x.h
+++ b/target/s390x/kvm_s390x.h
@@ -29,6 +29,8 @@ int kvm_s390_get_clock(uint8_t *tod_high, uint64_t *tod_clock);
 int kvm_s390_get_clock_ext(uint8_t *tod_high, uint64_t *tod_clock);
 int kvm_s390_set_clock(uint8_t tod_high, uint64_t tod_clock);
 int kvm_s390_set_clock_ext(uint8_t tod_high, uint64_t tod_clock);
+int kvm_s390_get_cpc(uint64_t *cpc);
+int kvm_s390_set_cpc(uint64_t cpc);
 void kvm_s390_enable_css_support(S390CPU *cpu);
 int kvm_s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch,
                                     int vq, bool assign);
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 0/3] Guest Support for Diagnose 318

[no-reply]

Patchew URL: https://patchew.org/QEMU/1544135058-21380-1-git-send-email-walling@linux.ibm.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Subject: [Qemu-devel] [PATCH v2 0/3] Guest Support for Diagnose 318
Message-id: 1544135058-21380-1-git-send-email-walling@linux.ibm.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
5a34ba6 s390: migration and reset support for diagnose 318
744ef90 s390: cpu feature for diagnose 318 andlimit max VCPUs to 247
4205e50 s390: linux header sync for diagnose 318

=== OUTPUT BEGIN ===
Checking PATCH 1/3: s390: linux header sync for diagnose 318...
Checking PATCH 2/3: s390: cpu feature for diagnose 318 andlimit max VCPUs to 247...
Checking PATCH 3/3: s390: migration and reset support for diagnose 318...
ERROR: adding a line without newline at end of file
#169: FILE: target/s390x/kvm-stub.c:116:
+}

total: 1 errors, 0 warnings, 159 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/1544135058-21380-1-git-send-email-walling@linux.ibm.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com