Re: [PATCH v4 07/16] s390x: Add SIDA memory ops

[Janosch Frank]

On 2/25/20 10:59 AM, David Hildenbrand wrote:
> On 20.02.20 13:56, Janosch Frank wrote:
>> Protected guests save the instruction control blocks in the SIDA
>> instead of QEMU/KVM directly accessing the guest's memory.
>>
>> Let's introduce new functions to access the SIDA.
>>
>> Signed-off-by: Janosch Frank <address@hidden>
>> ---
>>  linux-headers/linux/kvm.h |  2 ++
>>  target/s390x/cpu.h        |  7 ++++++-
>>  target/s390x/kvm.c        | 23 +++++++++++++++++++++++
>>  target/s390x/kvm_s390x.h  |  2 ++
>>  target/s390x/mmu_helper.c |  9 +++++++++
>>  5 files changed, 42 insertions(+), 1 deletion(-)
>>
>> diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
>> index 2e647f2d9b..7ccf5988d2 100644
>> --- a/linux-headers/linux/kvm.h
>> +++ b/linux-headers/linux/kvm.h
>> @@ -483,6 +483,8 @@ struct kvm_s390_mem_op {
>>  /* types for kvm_s390_mem_op->op */
>>  #define KVM_S390_MEMOP_LOGICAL_READ 0
>>  #define KVM_S390_MEMOP_LOGICAL_WRITE        1
>> +#define KVM_S390_MEMOP_SIDA_READ    2
>> +#define KVM_S390_MEMOP_SIDA_WRITE   3
>>  /* flags for kvm_s390_mem_op->flags */
>>  #define KVM_S390_MEMOP_F_CHECK_ONLY         (1ULL << 0)
>>  #define KVM_S390_MEMOP_F_INJECT_EXCEPTION   (1ULL << 1)
>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>> index cbc53c99cf..491d6860a8 100644
>> --- a/target/s390x/cpu.h
>> +++ b/target/s390x/cpu.h
>> @@ -823,7 +823,12 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, 
>> uint8_t ar, void *hostbuf,
>>  #define s390_cpu_virt_mem_check_write(cpu, laddr, ar, len)   \
>>          s390_cpu_virt_mem_rw(cpu, laddr, ar, NULL, len, true)
>>  void s390_cpu_virt_mem_handle_exc(S390CPU *cpu, uintptr_t ra);
>> -
>> +int s390_cpu_pv_mem_rw(S390CPU *cpu, unsigned int offset,  void *hostbuf,
>> +                       int len, bool is_write);
>> +#define s390_cpu_pv_mem_read(cpu, offset, dest, len)    \
>> +        s390_cpu_pv_mem_rw(cpu, offset, dest, len, false)
>> +#define s390_cpu_pv_mem_write(cpu, offset, dest, len)       \
>> +        s390_cpu_pv_mem_rw(cpu, offset, dest, len, true)
>>  
>>  /* sigp.c */
>>  int s390_cpu_restart(S390CPU *cpu);
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index eec0b92479..f222836df5 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -846,6 +846,29 @@ int kvm_s390_mem_op(S390CPU *cpu, vaddr addr, uint8_t 
>> ar, void *hostbuf,
>>      return ret;
>>  }
>>  
>> +int kvm_s390_mem_op_pv(S390CPU *cpu, uint64_t offset, void *hostbuf,
>> +                       int len, bool is_write)
>> +{
>> +    int ret = 0;
> 
> move that after the struct declaration/definition. No need to initialize
> to 0.

Ack

> 
>> +    struct kvm_s390_mem_op mem_op = {
>> +        .sida_offset = offset,
>> +        .size = len,
>> +        .op = is_write ? KVM_S390_MEMOP_SIDA_WRITE
>> +                       : KVM_S390_MEMOP_SIDA_READ,
>> +        .buf = (uint64_t)hostbuf,
>> +    };
>> +
>> +    if (!cap_mem_op) {
>> +        return -ENOSYS;
>> +    }
> 
> if (!cap_mem_op || !cap_protected)
> 
> And move cap_protected from patch 15 in here.

Ack

> 
>> +
>> +    ret = kvm_vcpu_ioctl(CPU(cpu), KVM_S390_MEM_OP, &mem_op);
>> +    if (ret < 0) {
>> +        warn_report("KVM_S390_MEM_OP failed: %s", strerror(-ret));
> 
> I'd even error_report(). After all, this should never fail.

Hmm, might even make sense to exit here.
In my experience a failed memop will soon result in bigger problems as
it effectively stops a cpu.

> 
>> +    }
>> +    return ret;
>> +}
>> +
>>  /*
>>   * Legacy layout for s390:
>>   * Older S390 KVM requires the topmost vma of the RAM to be
>> diff --git a/target/s390x/kvm_s390x.h b/target/s390x/kvm_s390x.h
>> index 0b21789796..9c38f6ccce 100644
>> --- a/target/s390x/kvm_s390x.h
>> +++ b/target/s390x/kvm_s390x.h
>> @@ -19,6 +19,8 @@ void kvm_s390_vcpu_interrupt(S390CPU *cpu, struct 
>> kvm_s390_irq *irq);
>>  void kvm_s390_access_exception(S390CPU *cpu, uint16_t code, uint64_t 
>> te_code);
>>  int kvm_s390_mem_op(S390CPU *cpu, vaddr addr, uint8_t ar, void *hostbuf,
>>                      int len, bool is_write);
>> +int kvm_s390_mem_op_pv(S390CPU *cpu, vaddr addr, void *hostbuf, int len,
>> +                       bool is_write);
>>  void kvm_s390_program_interrupt(S390CPU *cpu, uint16_t code);
>>  int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state);
>>  void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu);
>> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
>> index c9f3f34750..ad485399db 100644
>> --- a/target/s390x/mmu_helper.c
>> +++ b/target/s390x/mmu_helper.c
>> @@ -474,6 +474,15 @@ static int translate_pages(S390CPU *cpu, vaddr addr, 
>> int nr_pages,
>>      return 0;
>>  }
>>  
>> +int s390_cpu_pv_mem_rw(S390CPU *cpu, unsigned int offset, void *hostbuf,
>> +                       int len, bool is_write)
>> +{
>> +    int ret;
>> +
>> +    ret = kvm_s390_mem_op_pv(cpu, offset, hostbuf, len, is_write);
>> +    return ret;
> 
> if (kvm_enabled()) {
> ...
> } else {
>     /* Helpful comment */
>     g_assert_not_reached();
> }
> 

Ack.

>

signature.asc
Description: OpenPGP digital signature