Re: [Qemu-devel] [PATCH 09/10] s390x/mmu: Use ioctl for reading and writing from/to guest memory

[Alexander Graf]

On 04/21/2015 09:34 PM, Christian Borntraeger wrote:
> Am 21.04.2015 um 21:12 schrieb Alexander Graf:
> > On 04/20/2015 04:55 PM, Christian Borntraeger wrote:
> > > Am 09.04.2015 um 10:51 schrieb Cornelia Huck:
> > > > From: Thomas Huth <address@hidden>
> > > >
> > > > Add code to make use of the new ioctl for reading from / writing to
> > > > virtual guest memory. By using the ioctl, the memory accesses are now
> > > > protected with the so-called ipte-lock in the kernel.
> > > >
> > > > Signed-off-by: Thomas Huth <address@hidden>
> > > > Signed-off-by: Cornelia Huck <address@hidden>
> > > Acked-by: Christian Borntraeger <address@hidden>
> > >
> > > > ---
> > > >    target-s390x/cpu.h        |  7 +++++++
> > > >    target-s390x/kvm.c        | 35 +++++++++++++++++++++++++++++++++++
> > > >    target-s390x/mmu_helper.c |  9 +++++++++
> > > >    3 files changed, 51 insertions(+)
> > > >
> > > > diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> > > > index 79bc80b..9c42743 100644
> > > > --- a/target-s390x/cpu.h
> > > > +++ b/target-s390x/cpu.h
> > > > @@ -401,6 +401,8 @@ void kvm_s390_vcpu_interrupt(S390CPU *cpu, struct 
> > > > kvm_s390_irq *irq);
> > > >    void kvm_s390_floating_interrupt(struct kvm_s390_irq *irq);
> > > >    int kvm_s390_inject_flic(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, void *hostbuf, int len,
> > > > +                    bool is_write);
> > > >    int kvm_s390_get_clock(uint8_t *tod_high, uint64_t *tod_clock);
> > > >    int kvm_s390_set_clock(uint8_t *tod_high, uint64_t *tod_clock);
> > > >    #else
> > > > @@ -418,6 +420,11 @@ static inline int kvm_s390_set_clock(uint8_t *tod_high, 
> > > > uint64_t *tod_low)
> > > >    {
> > > >        return -ENOSYS;
> > > >    }
> > > > +static inline int kvm_s390_mem_op(S390CPU *cpu, vaddr addr, void *hostbuf,
> > > > +                                  int len, bool is_write)
> > > > +{
> > > > +    return -ENOSYS;
> > > > +}
> > > >    static inline void kvm_s390_access_exception(S390CPU *cpu, uint16_t code,
> > > >                                                 uint64_t te_code)
> > > >    {
> > > > diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> > > > index 619684b..e2ae2b3 100644
> > > > --- a/target-s390x/kvm.c
> > > > +++ b/target-s390x/kvm.c
> > > > @@ -123,6 +123,7 @@ const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
> > > >
> > > >    static int cap_sync_regs;
> > > >    static int cap_async_pf;
> > > > +static int cap_mem_op;
> > > >
> > > >    static void *legacy_s390_alloc(size_t size, uint64_t *align);
> > > >
> > > > @@ -247,6 +248,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
> > > >    {
> > > >        cap_sync_regs = kvm_check_extension(s, KVM_CAP_SYNC_REGS);
> > > >        cap_async_pf = kvm_check_extension(s, KVM_CAP_ASYNC_PF);
> > > > +    cap_mem_op = kvm_check_extension(s, KVM_CAP_S390_MEM_OP);
> > > >
> > > >        kvm_s390_enable_cmma(s);
> > > >
> > > > @@ -550,6 +552,39 @@ int kvm_s390_set_clock(uint8_t *tod_high, uint64_t 
> > > > *tod_low)
> > > >        return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
> > > >    }
> > > >
> > > > +/**
> > > > + * kvm_s390_mem_op:
> > > > + * @addr:      the logical start address in guest memory
> > > > + * @hostbuf:   buffer in host memory. NULL = do only checks w/o copying
> > > > + * @len:       length that should be transfered
> > > > + * @is_write:  true = write, false = read
> > > > + * Returns:    0 on success, non-zero if an exception or error occured
> > > > + *
> > > > + * Use KVM ioctl to read/write from/to guest memory. An access exception
> > > > + * is injected into the vCPU in case of translation errors.
> > > > + */
> > > > +int kvm_s390_mem_op(S390CPU *cpu, vaddr addr, void *hostbuf, int len,
> > > > +                    bool is_write)
> > > > +{
> > > > +    struct kvm_s390_mem_op mem_op = {
> > > > +        .gaddr = addr,
> > > > +        .flags = KVM_S390_MEMOP_F_INJECT_EXCEPTION,
> > > > +        .size = len,
> > > > +        .op = is_write ? KVM_S390_MEMOP_LOGICAL_WRITE
> > > > +                       : KVM_S390_MEMOP_LOGICAL_READ,
> > > > +        .buf = (uint64_t)hostbuf,
> > > > +    };
> > > > +
> > > > +    if (!cap_mem_op) {
> > > > +        return -ENOSYS;
> > > > +    }
> > > > +    if (!hostbuf) {
> > > > +        mem_op.flags |= KVM_S390_MEMOP_F_CHECK_ONLY;
> > > > +    }
> > > > +
> > > > +    return kvm_vcpu_ioctl(CPU(cpu), KVM_S390_MEM_OP, &mem_op);
> > > > +}
> > > > +
> > > >    /*
> > > >     * Legacy layout for s390:
> > > >     * Older S390 KVM requires the topmost vma of the RAM to be
> > > > diff --git a/target-s390x/mmu_helper.c b/target-s390x/mmu_helper.c
> > > > index 9b88498..1e7412c 100644
> > > > --- a/target-s390x/mmu_helper.c
> > > > +++ b/target-s390x/mmu_helper.c
> > > > @@ -450,6 +450,15 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, void 
> > > > *hostbuf,
> > > >        target_ulong *pages;
> > > >        int ret;
> > > >
> > > > +    if (kvm_enabled()) {
> > > > +        ret = kvm_s390_mem_op(cpu, laddr, hostbuf, len, is_write);
> > > > +        if (ret >= 0) {
> > > > +            return ret;
> > > > +        } else if (ret != -ENOSYS) {
> > > > +            error_printf("kvm_s390_mem_op() failed: %s\n", strerror(-ret));
> > Won't this spill the logs like crazy when running new qemu on old kernels?
> kvm_s390_mem_op will return -ENOSYS if cap_mem_op is not set.

Oh, I see the logic now. Can you think of any way to make it more 
obvious to a reader what's going on?


Alex