[PATCH v1 0/1] s390x: protvirt: SCLP interpretation

[PATCH v1 0/1] s390x: protvirt: SCLP interpretation

[Pierre Morel]

A new proposition:
I think it would be wise to fork directly from handle_instruction
instead to accept per default all instructions with with secure
instruction interception code.
Just in case future firmware with older QEMU.

How ever I let three dors open.

1) This patch accepts the all B2 instructions, mostly I/O.
Some of the instructions will not work correctly for PV until patched.
This should be fixed, and will be, in a separate patch.

2) The same is true for DIAG instructions.

3) Secure notifications are separated from secure instructions and
normal instructions interception because this case is completely new.
For B2 instructions we do not have to do anything this just informative.
However, one information is of interrest, a notification that
SIGP(STOP) is sent to stop the CPUs and terminate QEMU.



Pierre Morel (1):
  s390x: protvirt: SCLP interpretation

 hw/s390x/sclp.c         | 18 +++++++++++++
 include/hw/s390x/sclp.h |  2 ++
 target/s390x/kvm.c      | 56 ++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 75 insertions(+), 1 deletion(-)

-- 
2.17.0

[PATCH v1 1/1] s390x: protvirt: SCLP interpretation

[Pierre Morel]

The SCLP protection handle some of the exceptions due to
mis-constructions of the SCLP Control Block (SCCB) by the guest and
provides notifications to the host when something gets wrong.
We currently do not handle these exceptions, letting all the work to the
firmware therefor, we only need to inject an external interrupt to the
guest.

When the SCCB is correct, the S390x virtualisation protection copies
the SCLP Control Block (SCCB) from the guest inside the kernel to avoid
opening a direct access to the guest memory.
When accessing the kernel memory with standard s390_cpu_virt_mem_*
functions the host opens access to the SCCB shadow at address 0.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 hw/s390x/sclp.c         | 18 +++++++++++++
 include/hw/s390x/sclp.h |  2 ++
 target/s390x/kvm.c      | 56 ++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 75 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index f57ce7b739..02e4e0146f 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -193,6 +193,24 @@ static void sclp_execute(SCLPDevice *sclp, SCCB *sccb, uint32_t code)
     }
 }
 
+int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
+                                uint32_t code)
+{
+    SCLPDevice *sclp = get_sclp_device();
+    SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
+    SCCB work_sccb;
+    hwaddr sccb_len = sizeof(SCCB);
+
+    /* Protected guest SCCB is always seen at address 0 */
+    s390_cpu_virt_mem_read(env_archcpu(env), 0, 0, &work_sccb, sccb_len);
+    sclp_c->execute(sclp, &work_sccb, code);
+    s390_cpu_virt_mem_write(env_archcpu(env), 0, 0, &work_sccb,
+                            be16_to_cpu(work_sccb.h.length));
+
+    sclp_c->service_interrupt(sclp, (uint64_t)&work_sccb);
+    return 0;
+}
+
 int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
 {
     SCLPDevice *sclp = get_sclp_device();
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index c54413b78c..c0a3faa37d 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -217,5 +217,7 @@ void s390_sclp_init(void);
 void sclp_service_interrupt(uint32_t sccb);
 void raise_irq_cpu_hotplug(void);
 int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);
+int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
+                                uint32_t code);
 
 #endif
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 0c9d14b4b1..559f470f51 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -1170,7 +1170,14 @@ static int kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
     sccb = env->regs[ipbh0 & 0xf];
     code = env->regs[(ipbh0 & 0xf0) >> 4];
 
-    r = sclp_service_call(env, sccb, code);
+    switch (run->s390_sieic.icptcode) {
+    case ICPT_PV_INSTR:
+        r = sclp_service_call_protected(env, sccb, code);
+        break;
+    default:
+        r = sclp_service_call(env, sccb, code);
+        break;
+    }
     if (r < 0) {
         kvm_s390_program_interrupt(cpu, -r);
     } else {
@@ -1575,6 +1582,47 @@ static int kvm_s390_handle_sigp(S390CPU *cpu, uint8_t ipa1, uint32_t ipb)
     return 0;
 }
 
+static int handle_secure_notification(S390CPU *cpu, struct kvm_run *run)
+{
+    unsigned int ipa0 = (run->s390_sieic.ipa & 0xff00);
+    uint8_t ipa1 = run->s390_sieic.ipa & 0x00ff;
+
+    switch (ipa0) {
+    case IPA0_SIGP: /* We get the notification that the guest stop */
+        kvm_s390_handle_sigp(cpu, ipa1, run->s390_sieic.ipb);
+        break;
+    case IPA0_B2: /* We accept but do nothing for B2 notifications */
+        break;
+    default: /* We do not expect other instruction's notification */
+        kvm_s390_program_interrupt(cpu, PGM_OPERATION);
+        break;
+    }
+    return 0;
+}
+
+static int handle_secure_instruction(S390CPU *cpu, struct kvm_run *run)
+{
+    unsigned int ipa0 = (run->s390_sieic.ipa & 0xff00);
+    uint8_t ipa1 = run->s390_sieic.ipa & 0x00ff;
+    int r = -1;
+
+    switch (ipa0) {
+    case IPA0_B2:
+        r = handle_b2(cpu, run, ipa1);
+        break;
+    case IPA0_DIAG:
+        r = handle_diag(cpu, run, run->s390_sieic.ipb);
+        break;
+    }
+
+    if (r < 0) {
+        r = 0;
+        kvm_s390_program_interrupt(cpu, PGM_OPERATION);
+    }
+
+    return r;
+}
+
 static int handle_instruction(S390CPU *cpu, struct kvm_run *run)
 {
     unsigned int ipa0 = (run->s390_sieic.ipa & 0xff00);
@@ -1665,6 +1713,12 @@ static int handle_intercept(S390CPU *cpu)
     DPRINTF("intercept: 0x%x (at 0x%lx)\n", icpt_code,
             (long)cs->kvm_run->psw_addr);
     switch (icpt_code) {
+         case ICPT_PV_INSTR_NOT:
+            r = handle_secure_notification(cpu, run);
+            break;
+        case ICPT_PV_INSTR:
+            r = handle_secure_instruction(cpu, run);
+            break;
         case ICPT_INSTRUCTION:
             r = handle_instruction(cpu, run);
             break;
-- 
2.17.0

Re: [PATCH v1 1/1] s390x: protvirt: SCLP interpretation

[Thomas Huth]

On 28/11/2019 11.13, Pierre Morel wrote:
> The SCLP protection handle some of the exceptions due to
> mis-constructions of the SCLP Control Block (SCCB) by the guest and
> provides notifications to the host when something gets wrong.
> We currently do not handle these exceptions, letting all the work to the
> firmware therefor, we only need to inject an external interrupt to the
> guest.
> 
> When the SCCB is correct, the S390x virtualisation protection copies
> the SCLP Control Block (SCCB) from the guest inside the kernel to avoid
> opening a direct access to the guest memory.
> When accessing the kernel memory with standard s390_cpu_virt_mem_*
> functions the host opens access to the SCCB shadow at address 0.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   hw/s390x/sclp.c         | 18 +++++++++++++
>   include/hw/s390x/sclp.h |  2 ++
>   target/s390x/kvm.c      | 56 ++++++++++++++++++++++++++++++++++++++++-
>   3 files changed, 75 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index f57ce7b739..02e4e0146f 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -193,6 +193,24 @@ static void sclp_execute(SCLPDevice *sclp, SCCB *sccb, uint32_t code)
>       }
>   }
>   
> +int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
> +                                uint32_t code)
> +{
> +    SCLPDevice *sclp = get_sclp_device();
> +    SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
> +    SCCB work_sccb;
> +    hwaddr sccb_len = sizeof(SCCB);
> +
> +    /* Protected guest SCCB is always seen at address 0 */

Well, as far as I've understood it, the address is rather ignored (and 
you can only specify an offset into the 4k page)?

> +    s390_cpu_virt_mem_read(env_archcpu(env), 0, 0, &work_sccb, sccb_len);
> +    sclp_c->execute(sclp, &work_sccb, code);
> +    s390_cpu_virt_mem_write(env_archcpu(env), 0, 0, &work_sccb,
> +                            be16_to_cpu(work_sccb.h.length));
> +
> +    sclp_c->service_interrupt(sclp, (uint64_t)&work_sccb);
> +    return 0;
> +}
> +
>   int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
>   {
>       SCLPDevice *sclp = get_sclp_device();
> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
> index c54413b78c..c0a3faa37d 100644
> --- a/include/hw/s390x/sclp.h
> +++ b/include/hw/s390x/sclp.h
> @@ -217,5 +217,7 @@ void s390_sclp_init(void);
>   void sclp_service_interrupt(uint32_t sccb);
>   void raise_irq_cpu_hotplug(void);
>   int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);
> +int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
> +                                uint32_t code);
>   
>   #endif
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 0c9d14b4b1..559f470f51 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -1170,7 +1170,14 @@ static int kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
>       sccb = env->regs[ipbh0 & 0xf];
>       code = env->regs[(ipbh0 & 0xf0) >> 4];
>   
> -    r = sclp_service_call(env, sccb, code);
> +    switch (run->s390_sieic.icptcode) {
> +    case ICPT_PV_INSTR:
> +        r = sclp_service_call_protected(env, sccb, code);
> +        break;
> +    default:
> +        r = sclp_service_call(env, sccb, code);
> +        break;
> +    }

Why not simply

     if (run->s390_sieic.icptcode == ICPT_PV_INSTR) {
         r = sclp_service_call_protected(env, sccb, code);
     } else {
         r = sclp_service_call(env, sccb, code);
     }

... that's way short and easier to read. Or do you expect other 
icptcodes in the near future?

>       if (r < 0) {
>           kvm_s390_program_interrupt(cpu, -r);
>       } else {
> @@ -1575,6 +1582,47 @@ static int kvm_s390_handle_sigp(S390CPU *cpu, uint8_t ipa1, uint32_t ipb)
>       return 0;
>   }
>   
> +static int handle_secure_notification(S390CPU *cpu, struct kvm_run *run)
> +{
> +    unsigned int ipa0 = (run->s390_sieic.ipa & 0xff00);
> +    uint8_t ipa1 = run->s390_sieic.ipa & 0x00ff;
> +
> +    switch (ipa0) {
> +    case IPA0_SIGP: /* We get the notification that the guest stop */
> +        kvm_s390_handle_sigp(cpu, ipa1, run->s390_sieic.ipb);
> +        break;
> +    case IPA0_B2: /* We accept but do nothing for B2 notifications */
> +        break;
> +    default: /* We do not expect other instruction's notification */
> +        kvm_s390_program_interrupt(cpu, PGM_OPERATION);

Maybe add a tracepoint or qemu_log_mask(LOG_UNIMP, ...) or CPU_LOG_INT 
here, so we can spot this condition more easily?

> +        break;
> +    }
> +    return 0;
> +}
> +
> +static int handle_secure_instruction(S390CPU *cpu, struct kvm_run *run)
> +{
> +    unsigned int ipa0 = (run->s390_sieic.ipa & 0xff00);
> +    uint8_t ipa1 = run->s390_sieic.ipa & 0x00ff;
> +    int r = -1;
> +
> +    switch (ipa0) {
> +    case IPA0_B2:
> +        r = handle_b2(cpu, run, ipa1);
> +        break;
> +    case IPA0_DIAG:
> +        r = handle_diag(cpu, run, run->s390_sieic.ipb);
> +        break;
> +    }
> +
> +    if (r < 0) {
> +        r = 0;
> +        kvm_s390_program_interrupt(cpu, PGM_OPERATION);
> +    }
> +
> +    return r;
> +}
> +
>   static int handle_instruction(S390CPU *cpu, struct kvm_run *run)
>   {
>       unsigned int ipa0 = (run->s390_sieic.ipa & 0xff00);
> @@ -1665,6 +1713,12 @@ static int handle_intercept(S390CPU *cpu)
>       DPRINTF("intercept: 0x%x (at 0x%lx)\n", icpt_code,
>               (long)cs->kvm_run->psw_addr);
>       switch (icpt_code) {
> +         case ICPT_PV_INSTR_NOT:
> +            r = handle_secure_notification(cpu, run);
> +            break;
> +        case ICPT_PV_INSTR:
> +            r = handle_secure_instruction(cpu, run);
> +            break;
>           case ICPT_INSTRUCTION:
>               r = handle_instruction(cpu, run);
>               break;
> 

  Thomas

Re: [PATCH v1 0/1] s390x: protvirt: SCLP interpretation

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 1601 bytes --]

On 11/28/19 11:13 AM, Pierre Morel wrote:
> A new proposition:
> I think it would be wise to fork directly from handle_instruction
> instead to accept per default all instructions with with secure
> instruction interception code.
> Just in case future firmware with older QEMU.
> 
> How ever I let three dors open.
> 
> 1) This patch accepts the all B2 instructions, mostly I/O.
> Some of the instructions will not work correctly for PV until patched.
> This should be fixed, and will be, in a separate patch.
> 
> 2) The same is true for DIAG instructions.
> 
> 3) Secure notifications are separated from secure instructions and
> normal instructions interception because this case is completely new.
> For B2 instructions we do not have to do anything this just informative.
> However, one information is of interrest, a notification that
> SIGP(STOP) is sent to stop the CPUs and terminate QEMU.

Pierre, I told you this morning that I don't want this and that you
should leave this untouched until I can explain my thoughts behind the
initial patch in a f2f.
Thomas' review of your change only confirmed my concerns about this patch.

This is the wrong patch at the wrong time, which creates noise and work
for other people. Please stop and work on something else.



> 
> 
> 
> Pierre Morel (1):
>   s390x: protvirt: SCLP interpretation
> 
>  hw/s390x/sclp.c         | 18 +++++++++++++
>  include/hw/s390x/sclp.h |  2 ++
>  target/s390x/kvm.c      | 56 ++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 75 insertions(+), 1 deletion(-)
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 1/1] s390x: protvirt: SCLP interpretation

[Pierre Morel]

On 2019-11-28 13:10, Thomas Huth wrote:
> On 28/11/2019 11.13, Pierre Morel wrote:
>> The SCLP protection handle some of the exceptions due to
>> mis-constructions of the SCLP Control Block (SCCB) by the guest and
>> provides notifications to the host when something gets wrong.
>> We currently do not handle these exceptions, letting all the work to the
>> firmware therefor, we only need to inject an external interrupt to the
>> guest.
>>
>> When the SCCB is correct, the S390x virtualisation protection copies
>> the SCLP Control Block (SCCB) from the guest inside the kernel to avoid
>> opening a direct access to the guest memory.
>> When accessing the kernel memory with standard s390_cpu_virt_mem_*
>> functions the host opens access to the SCCB shadow at address 0.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   hw/s390x/sclp.c         | 18 +++++++++++++
>>   include/hw/s390x/sclp.h |  2 ++
>>   target/s390x/kvm.c      | 56 ++++++++++++++++++++++++++++++++++++++++-
>>   3 files changed, 75 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index f57ce7b739..02e4e0146f 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -193,6 +193,24 @@ static void sclp_execute(SCLPDevice *sclp, SCCB 
>> *sccb, uint32_t code)
>>       }
>>   }
>>   +int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
>> +                                uint32_t code)
>> +{
>> +    SCLPDevice *sclp = get_sclp_device();
>> +    SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
>> +    SCCB work_sccb;
>> +    hwaddr sccb_len = sizeof(SCCB);
>> +
>> +    /* Protected guest SCCB is always seen at address 0 */
>
> Well, as far as I've understood it, the address is rather ignored (and 
> you can only specify an offset into the 4k page)?


You can see it like this, then the offset is 0. However we give here an 
address as argument.


>
>> + s390_cpu_virt_mem_read(env_archcpu(env), 0, 0, &work_sccb, sccb_len);
>> +    sclp_c->execute(sclp, &work_sccb, code);
>> +    s390_cpu_virt_mem_write(env_archcpu(env), 0, 0, &work_sccb,
>> +                            be16_to_cpu(work_sccb.h.length));
>> +
>> +    sclp_c->service_interrupt(sclp, (uint64_t)&work_sccb);
>> +    return 0;
>> +}
>> +
>>   int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t 
>> code)
>>   {
>>       SCLPDevice *sclp = get_sclp_device();
>> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
>> index c54413b78c..c0a3faa37d 100644
>> --- a/include/hw/s390x/sclp.h
>> +++ b/include/hw/s390x/sclp.h
>> @@ -217,5 +217,7 @@ void s390_sclp_init(void);
>>   void sclp_service_interrupt(uint32_t sccb);
>>   void raise_irq_cpu_hotplug(void);
>>   int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t 
>> code);
>> +int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
>> +                                uint32_t code);
>>     #endif
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index 0c9d14b4b1..559f470f51 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -1170,7 +1170,14 @@ static int kvm_sclp_service_call(S390CPU *cpu, 
>> struct kvm_run *run,
>>       sccb = env->regs[ipbh0 & 0xf];
>>       code = env->regs[(ipbh0 & 0xf0) >> 4];
>>   -    r = sclp_service_call(env, sccb, code);
>> +    switch (run->s390_sieic.icptcode) {
>> +    case ICPT_PV_INSTR:
>> +        r = sclp_service_call_protected(env, sccb, code);
>> +        break;
>> +    default:
>> +        r = sclp_service_call(env, sccb, code);
>> +        break;
>> +    }
>
> Why not simply
>
>     if (run->s390_sieic.icptcode == ICPT_PV_INSTR) {
>         r = sclp_service_call_protected(env, sccb, code);
>     } else {
>         r = sclp_service_call(env, sccb, code);
>     }
>
> ... that's way short and easier to read. Or do you expect other 
> icptcodes in the near future?


No you are right, it is better, I just like switches :)


>
>>       if (r < 0) {
>>           kvm_s390_program_interrupt(cpu, -r);
>>       } else {
>> @@ -1575,6 +1582,47 @@ static int kvm_s390_handle_sigp(S390CPU *cpu, 
>> uint8_t ipa1, uint32_t ipb)
>>       return 0;
>>   }
>>   +static int handle_secure_notification(S390CPU *cpu, struct kvm_run 
>> *run)
>> +{
>> +    unsigned int ipa0 = (run->s390_sieic.ipa & 0xff00);
>> +    uint8_t ipa1 = run->s390_sieic.ipa & 0x00ff;
>> +
>> +    switch (ipa0) {
>> +    case IPA0_SIGP: /* We get the notification that the guest stop */
>> +        kvm_s390_handle_sigp(cpu, ipa1, run->s390_sieic.ipb);
>> +        break;
>> +    case IPA0_B2: /* We accept but do nothing for B2 notifications */
>> +        break;
>> +    default: /* We do not expect other instruction's notification */
>> +        kvm_s390_program_interrupt(cpu, PGM_OPERATION);
>
> Maybe add a tracepoint or qemu_log_mask(LOG_UNIMP, ...) or CPU_LOG_INT 
> here, so we can spot this condition more easily?
>
>> +        break;
>> +    }
>> +    return 0;
>> +}
>> +
>> +static int handle_secure_instruction(S390CPU *cpu, struct kvm_run *run)
>> +{
>> +    unsigned int ipa0 = (run->s390_sieic.ipa & 0xff00);
>> +    uint8_t ipa1 = run->s390_sieic.ipa & 0x00ff;
>> +    int r = -1;
>> +
>> +    switch (ipa0) {
>> +    case IPA0_B2:
>> +        r = handle_b2(cpu, run, ipa1);
>> +        break;
>> +    case IPA0_DIAG:
>> +        r = handle_diag(cpu, run, run->s390_sieic.ipb);
>> +        break;
>> +    }
>> +
>> +    if (r < 0) {
>> +        r = 0;
>> +        kvm_s390_program_interrupt(cpu, PGM_OPERATION);
>> +    }
>> +
>> +    return r;
>> +}
>> +
>>   static int handle_instruction(S390CPU *cpu, struct kvm_run *run)
>>   {
>>       unsigned int ipa0 = (run->s390_sieic.ipa & 0xff00);
>> @@ -1665,6 +1713,12 @@ static int handle_intercept(S390CPU *cpu)
>>       DPRINTF("intercept: 0x%x (at 0x%lx)\n", icpt_code,
>>               (long)cs->kvm_run->psw_addr);
>>       switch (icpt_code) {
>> +         case ICPT_PV_INSTR_NOT:
>> +            r = handle_secure_notification(cpu, run);
>> +            break;
>> +        case ICPT_PV_INSTR:
>> +            r = handle_secure_instruction(cpu, run);
>> +            break;
>>           case ICPT_INSTRUCTION:
>>               r = handle_instruction(cpu, run);
>>               break;
>>
>
>  Thomas
>
>
-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v1 0/1] s390x: protvirt: SCLP interpretation

[Pierre Morel]

On 2019-11-28 13:28, Janosch Frank wrote:
> On 11/28/19 11:13 AM, Pierre Morel wrote:
>> A new proposition:
>> I think it would be wise to fork directly from handle_instruction
>> instead to accept per default all instructions with with secure
>> instruction interception code.
>> Just in case future firmware with older QEMU.
>>
>> How ever I let three dors open.
>>
>> 1) This patch accepts the all B2 instructions, mostly I/O.
>> Some of the instructions will not work correctly for PV until patched.
>> This should be fixed, and will be, in a separate patch.
>>
>> 2) The same is true for DIAG instructions.
>>
>> 3) Secure notifications are separated from secure instructions and
>> normal instructions interception because this case is completely new.
>> For B2 instructions we do not have to do anything this just informative.
>> However, one information is of interrest, a notification that
>> SIGP(STOP) is sent to stop the CPUs and terminate QEMU.
> Pierre, I told you this morning that I don't want this and that you
> should leave this untouched until I can explain my thoughts behind the
> initial patch in a f2f.
> Thomas' review of your change only confirmed my concerns about this patch.
>
> This is the wrong patch at the wrong time, which creates noise and work
> for other people. Please stop and work on something else.

!?

- you sent to me info on slack that I did not see until now, next time 
be sure I have acknowledged if it is important.

- You told me to dive into this patch quite abruptly, and it invested 
time to understand how the I/O works with PV, so sorry for the wrong time

- I see no problem with the questions from Thomas, may be you can 
explain this to me

- I have a lot of other things to do so I just close this thread after 
having answered to Thomas.


>
>
>
>>
>>
>> Pierre Morel (1):
>>    s390x: protvirt: SCLP interpretation
>>
>>   hw/s390x/sclp.c         | 18 +++++++++++++
>>   include/hw/s390x/sclp.h |  2 ++
>>   target/s390x/kvm.c      | 56 ++++++++++++++++++++++++++++++++++++++++-
>>   3 files changed, 75 insertions(+), 1 deletion(-)
>>
>
-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH v1 0/1] s390x: protvirt: SCLP interpretation

[no-reply]

Patchew URL: https://patchew.org/QEMU/1574935984-16910-1-git-send-email-pmorel@linux.ibm.com/



Hi,

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

Subject: [PATCH v1 0/1] s390x: protvirt: SCLP interpretation
Type: series
Message-id: 1574935984-16910-1-git-send-email-pmorel@linux.ibm.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
cc42cdc s390x: protvirt: SCLP interpretation

=== OUTPUT BEGIN ===
ERROR: switch and case should be at the same indent
#135: FILE: target/s390x/kvm.c:1715:
     switch (icpt_code) {
+         case ICPT_PV_INSTR_NOT:
[...]
+        case ICPT_PV_INSTR:

total: 1 errors, 0 warnings, 105 lines checked

Commit cc42cdc36171 (s390x: protvirt: SCLP interpretation) 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/1574935984-16910-1-git-send-email-pmorel@linux.ibm.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com