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

[Pierre Morel <pmorel@linux.ibm.com>]

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