Re: [PATCH v3 13/17] s390x: protvirt: Move diag 308 data over SIDAD

[Janosch Frank]

On 2/20/20 12:00 PM, Cornelia Huck wrote:
> On Fri, 14 Feb 2020 10:16:32 -0500
> Janosch Frank <address@hidden> wrote:
> 
>> For protected guests the IPIB is written/read to/from the satellite
>> block, so we need to make those accesses virtual to make them go
>> through KVM mem ops.

..we need those accesses to go through s390_cpu_pv_mem_read/write() so
we use the memop IOCTL to access the sattelite block.

> 
> Confused. What does 'make those accesses virtual' mean?

I need to update the patch description.
Virtual memory access was needed when I overloaded the normal memops to
access the SIDA. Real access would have accessed guest memory directly
which results in a guest crash for protected VMs.

> 
>>
>> Signed-off-by: Janosch Frank <address@hidden>
>> ---
>>  target/s390x/diag.c | 32 +++++++++++++++++++++++++-------
>>  1 file changed, 25 insertions(+), 7 deletions(-)
>>
>> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
>> index 6aaeef6029..59ae122e82 100644
>> --- a/target/s390x/diag.c
>> +++ b/target/s390x/diag.c
>> @@ -88,6 +88,7 @@ static int diag308_parm_check(CPUS390XState *env, uint64_t 
>> r1, uint64_t addr,
>>  void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, 
>> uintptr_t ra)
>>  {
>>      CPUState *cs = env_cpu(env);
>> +    S390CPU *cpu = S390_CPU(cs);
>>      uint64_t addr =  env->regs[r1];
>>      uint64_t subcode = env->regs[r3];
>>      IplParameterBlock *iplb;
>> @@ -118,14 +119,24 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, 
>> uint64_t r3, uintptr_t ra)
>>          if (diag308_parm_check(env, r1, addr, ra, false)) {
>>              return;
>>          }
>> +
> 
> Whitespace.

Ack.

> 
>>          iplb = g_new0(IplParameterBlock, 1);
>> -        cpu_physical_memory_read(addr, iplb, sizeof(iplb->len));
>> +        if (!env->pv) {
>> +            cpu_physical_memory_read(addr, iplb, sizeof(iplb->len));
>> +        } else {
>> +            s390_cpu_pv_mem_read(cpu, 0, iplb, sizeof(iplb->len));
>> +        }
>> +
>>          if (!iplb_valid_len(iplb)) {
>>              env->regs[r1 + 1] = DIAG_308_RC_INVALID;
>>              goto out;
>>          }
>>  
>> -        cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
>> +        if (!env->pv) {
>> +            cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
>> +        } else {
>> +            s390_cpu_pv_mem_read(cpu, 0, iplb, be32_to_cpu(iplb->len));
>> +        }
>>  
>>          if (!iplb_valid_ccw(iplb) && !iplb_valid_fcp(iplb) &&
>>              !(iplb_valid_pv(iplb) && s390_ipl_pv_check_components(iplb) >= 
>> 0)) {
>> @@ -137,23 +148,30 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, 
>> uint64_t r3, uintptr_t ra)
>>          env->regs[r1 + 1] = DIAG_308_RC_OK;
>>  out:
>>          g_free(iplb);
>> -        return;
>> +        break;
>>      case DIAG308_STORE:
>>      case DIAG308_PV_STORE:
>>          if (diag308_parm_check(env, r1, addr, ra, true)) {
>>              return;
>>          }
>> +
> 
> Whitespace.

Ack.

> 
>>          if (subcode == DIAG308_PV_STORE) {
>>              iplb = s390_ipl_get_iplb_secure();
>>          } else {
>>              iplb = s390_ipl_get_iplb();
>>          }
>> -        if (iplb) {
>> -            cpu_physical_memory_write(addr, iplb, be32_to_cpu(iplb->len));
>> -            env->regs[r1 + 1] = DIAG_308_RC_OK;
>> -        } else {
>> +        if (!iplb) {
>>              env->regs[r1 + 1] = DIAG_308_RC_NO_CONF;
>> +            return;
>>          }
>> +
>> +        if (!env->pv) {
>> +            cpu_physical_memory_write(addr, iplb, be32_to_cpu(iplb->len));
>> +        } else {
>> +            s390_cpu_pv_mem_write(cpu, 0, iplb, be32_to_cpu(iplb->len));
>> +        }
>> +
>> +        env->regs[r1 + 1] = DIAG_308_RC_OK;
>>          break;
>>      case DIAG308_PV_START:
>>          iplb = s390_ipl_get_iplb_secure();
> 
>

signature.asc
Description: OpenPGP digital signature