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

[Thomas Huth <thuth@redhat.com>]

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