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

[David Hildenbrand]

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.

> +    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.

> +
> +    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.

> +    }
> +    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();
}


-- 
Thanks,

David / dhildenb