Re: [PATCH v6 10/18] s390x: protvirt: SCLP interpretation

[Janosch Frank]

On 3/4/20 6:48 PM, David Hildenbrand wrote:
> On 04.03.20 12:42, Janosch Frank wrote:
>> SCLP for a protected guest is done over the SIDAD, so we need to use
>> the s390_cpu_virt_mem_* functions to access the SIDAD instead of guest
> 
> nope :)
> 
> s390_cpu_pv_mem_*

Ack

> 
>> 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.
> 
> Please add that as a comment to SCLP_PV_DUMMY_ADDR.
> 
>>
>> Signed-off-by: Janosch Frank <address@hidden>
>> ---
>>  hw/s390x/sclp.c         | 17 +++++++++++++++++
>>  include/hw/s390x/sclp.h |  2 ++
>>  target/s390x/kvm.c      |  5 +++++
>>  3 files changed, 24 insertions(+)
>>
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index af0bfbc2ec..5136f5fcbe 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -193,6 +193,23 @@ static void sclp_execute(SCLPDevice *sclp, SCCB *sccb, 
>> uint32_t code)
>>      }
>>  }
>>  
>> +#define SCLP_PV_DUMMY_ADDR 0x4000
> 
> Should we move that to sclp_c->service_interrupt instead and document it
> properly?
> 
> Or what about providing a
> 
> sclp_c->service_interrupt_pv(sclp) that handles this internally?

The less functions with a pv suffix I have, the happier I am.
I'll have a look into the first suggestion.

> 
>> +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);
>> +
>> +    s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len);
> 
> I assume it's valid to always read the full SCCB length?

SIDA and SCCB are currently both 4k, so no problem there.
If we use extended SCCB, we would also need to increase the SIDA.

> 
>> +    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 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 43fc0c088b..a4cbdc5fc6 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -1226,6 +1226,11 @@ static void kvm_sclp_service_call(S390CPU *cpu, 
>> struct kvm_run *run,
>>      sccb = env->regs[ipbh0 & 0xf];
>>      code = env->regs[(ipbh0 & 0xf0) >> 4];
>>  
>> +    if (run->s390_sieic.icptcode == ICPT_PV_INSTR) {
> 
> I still somewhat prefer checking for env->pv instead - similar to patch #9.

Since we also have a notification for SCLP, I'd like to avoid that.
And that reminds me that we should add a check for the notification
here, so we get notified if KVM changes and let's those through without
qemu being prepared for it.

signature.asc
Description: OpenPGP digital signature