Re: [PATCH v9 08/15] s390x: protvirt: SCLP interpretation

[Christian Borntraeger]

On 11.03.20 14:21, Janosch Frank wrote:
> SCLP for a protected guest is done over the SIDAD, so we need to use
> the s390_cpu_pv_mem_* functions to access the SIDAD instead of guest
> memory when reading/writing SCBs.
> 
> To not confuse the sclp emulation, we set 0x4000 as the SCCB address,
> since the function that injects the sclp external interrupt would
> reject a zero sccb address.
> 
> Signed-off-by: Janosch Frank <address@hidden>
> Reviewed-by: David Hildenbrand <address@hidden>
> ---
>  hw/s390x/sclp.c         | 30 ++++++++++++++++++++++++++++++
>  include/hw/s390x/sclp.h |  2 ++
>  target/s390x/kvm.c      | 24 +++++++++++++++++++-----
>  3 files changed, 51 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index af0bfbc2eca74767..5f3aa30d6283dce5 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -193,6 +193,36 @@ static void sclp_execute(SCLPDevice *sclp, SCCB *sccb, 
> uint32_t code)
>      }
>  }
>  
> +/*
> + * We only need the address to have something valid for the
> + * service_interrupt call.
> + */
> +#define SCLP_PV_DUMMY_ADDR 0x4000
> +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);
> +
> +    /*
> +     * Only a very limited amount of calls is permitted by the
> +     * Ultravisor and we support all of them, so we don't check for
> +     * them. All other specification exceptions are also interpreted
> +     * by the Ultravisor and hence never cause an exit we need to
> +     * handle.
> +     *
> +     * Setting the CC is also done by the Ultravisor.
> +     */

This is fine for the current architecture which specifies a list of sclp 
commands that are passed through (and this is fine). Question is still if
we replace this comment with an assertion that this is the case?
Or maybe even really do the same as sclp_service_call and return 0x1f0 for
unknown commands?

Anyway, whatever you decide.

Reviewed-by: Christian Borntraeger <address@hidden>

> +    s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len);
> +    sclp_c->execute(sclp, &work_sccb, code);
> +    s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb,
> +                          be16_to_cpu(work_sccb.h.length));
> +    sclp_c->service_interrupt(sclp, SCLP_PV_DUMMY_ADDR);
> +    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 c54413b78cf01b27..c0a3faa37d730453 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 8085d5030e7c6454..ff6027036ec2f14a 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -1227,12 +1227,26 @@ static void 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);
> -    if (r < 0) {
> -        kvm_s390_program_interrupt(cpu, -r);
> -        return;
> +    switch (run->s390_sieic.icptcode) {
> +    case ICPT_PV_INSTR_NOTIFICATION:
> +        g_assert(s390_is_pv());
> +        /* The notification intercepts are currently handled by KVM */
> +        error_report("unexpected SCLP PV notification");
> +        exit(1);
> +        break;
> +    case ICPT_PV_INSTR:
> +        g_assert(s390_is_pv());
> +        sclp_service_call_protected(env, sccb, code);
> +        break;
> +    case ICPT_INSTRUCTION:
> +        g_assert(!s390_is_pv());
> +        r = sclp_service_call(env, sccb, code);
> +        if (r < 0) {
> +            kvm_s390_program_interrupt(cpu, -r);
> +            return;
> +        }
> +        setcc(cpu, r);
>      }
> -    setcc(cpu, r);
>  }
>  
>  static int handle_b2(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)
>