Re: [Qemu-devel] [PATCH -V3 1/4] target-ppc: Update slb array with correct index values.

Re: [Qemu-devel] [PATCH -V3 1/4] target-ppc: Update slb array with correct index values.

[Alexander Graf]

On 23.08.2013, at 06:20, Aneesh Kumar K.V wrote:

> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
> 
> Without this, a value of rb=0 and rs=0 results in replacing the 0th
> index. This can be observed when using gdb remote debugging support.
> 
> (gdb) x/10i do_fork
> 0xc000000000085330 <do_fork>:        Cannot access memory at address 0xc000000000085330
> (gdb)
> 
> This is because when we do the slb sync via kvm_cpu_synchronize_state,
> we overwrite the slb entry (0th entry) for 0xc000000000085330
> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> ---
> target-ppc/kvm.c | 20 ++++++++++++++++++--
> 1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 30a870e..6878af2 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -1033,9 +1033,25 @@ int kvm_arch_get_registers(CPUState *cs)
> 
>     /* Sync SLB */
> #ifdef TARGET_PPC64
> +        /*
> +         * KVM_GET_SREGS doesn't return slb entry with slot information
> +         * same as index. The ioctl zero fills the array and update only
> +         * upto slb_max entries. We cannot depend on the slot value
> +         * in the slbe field for update, because a zero slbe value would
> +         * result in us wrongly updating the 0th index. Instead we zero fill
> +         * the env->slb array first so that we mark all entries invalid and
> +         * update with only valid SLB entries.

Still too negative. How about something like this:

/*
* The packed SLB array we get from KVM only contains information
*  about valid entries. So we flush our internal copy to get rid of stale
*  ones, then put all valid SLB entries back in.
*/

> +         */
> +        memset(env->slb, 0, 64 * sizeof(ppc_slb_t));

Can't we use ARRAY_SIZE here and below?


Alex

>     for (i = 0; i < 64; i++) {
> -            ppc_store_slb(env, sregs.u.s.ppc64.slb[i].slbe,
> -                               sregs.u.s.ppc64.slb[i].slbv);
> +            target_ulong rb = sregs.u.s.ppc64.slb[i].slbe;
> +            target_ulong rs = sregs.u.s.ppc64.slb[i].slbv;
> +            /*
> +             * Only restore valid entries
> +             */
> +            if (rb & SLB_ESID_V) {
> +                ppc_store_slb(env, rb, rs);
> +            }
>     }
> #endif
> 
> -- 
> 1.8.1.2
> 

Re: [Qemu-devel] [PATCH -V3 2/4] target-ppc: Fix page table lookup with kvm enabled

[Alexander Graf]

On 23.08.2013, at 06:20, Aneesh Kumar K.V wrote:

> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
> 
> With kvm enabled, we store the hash page table information in the hypervisor.
> Use ioctl to read the htab contents. Without this we get the below error when
> trying to read the guest address
> 
> (gdb) x/10 do_fork
> 0xc000000000098660 <do_fork>:   Cannot access memory at address 0xc000000000098660
> (gdb)
> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> ---
> target-ppc/kvm.c        | 45 +++++++++++++++++++++++++++++++++++++++++++++
> target-ppc/kvm_ppc.h    |  9 ++++++++-
> target-ppc/mmu-hash64.c | 25 ++++++++++++++++---------
> 3 files changed, 69 insertions(+), 10 deletions(-)
> 
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 6878af2..bcc6544 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -1891,3 +1891,48 @@ int kvm_arch_on_sigbus(int code, void *addr)
> void kvm_arch_init_irq_routing(KVMState *s)
> {
> }
> +
> +int kvmppc_hash64_load_hpte(PowerPCCPU *cpu, uint64_t index,
> +                            target_ulong *hpte0, target_ulong *hpte1)
> +{
> +    int htab_fd;
> +    struct kvm_get_htab_fd ghf;
> +    struct kvm_get_htab_buf {
> +        struct kvm_get_htab_header header;
> +        /*
> +         * Older kernel required one extra byte.
> +         */
> +        unsigned long hpte[3];
> +    } hpte_buf;
> +
> +    *hpte0 = 0;
> +    *hpte1 = 0;
> +    if (!cap_htab_fd) {
> +        return 0;
> +    }
> +    /*
> +     * At this point we are only interested in reading only bolted entries
> +     */
> +    ghf.flags = KVM_GET_HTAB_BOLTED_ONLY;
> +    ghf.start_index = index;
> +    htab_fd = kvm_vm_ioctl(kvm_state, KVM_PPC_GET_HTAB_FD, &ghf);

We should cache this.

> +    if (htab_fd < 0) {
> +        return htab_fd;
> +    }
> +
> +    if (read(htab_fd, &hpte_buf, sizeof(hpte_buf)) < 0) {
> +        goto out;
> +    }
> +    /*
> +     * We only requested for one entry, So we should get only 1
> +     * valid entry at the same index
> +     */
> +    if (hpte_buf.header.n_valid != 1 || hpte_buf.header.index != index) {
> +        goto out;
> +    }
> +    *hpte0 = hpte_buf.hpte[0];
> +    *hpte1 = hpte_buf.hpte[1];
> +out:
> +    close(htab_fd);
> +    return 0;
> +}
> diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
> index 4ae7bf2..e25373a 100644
> --- a/target-ppc/kvm_ppc.h
> +++ b/target-ppc/kvm_ppc.h
> @@ -42,7 +42,8 @@ int kvmppc_get_htab_fd(bool write);
> int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t max_ns);
> int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
>                          uint16_t n_valid, uint16_t n_invalid);
> -
> +int kvmppc_hash64_load_hpte(PowerPCCPU *cpu, uint64_t index,
> +                            target_ulong *hpte0, target_ulong *hpte1);
> #else
> 
> static inline uint32_t kvmppc_get_tbfreq(void)
> @@ -181,6 +182,12 @@ static inline int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
>   abort();
> }
> 
> +static inline int kvmppc_hash64_load_hpte(PowerPCCPU *cpu, uint64_t index,
> +                                          target_ulong *hpte0,
> +                                          target_ulong *hpte1)
> +{
> +    abort();
> +}
> #endif
> 
> #ifndef CONFIG_KVM
> diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
> index 67fc1b5..4d8120c 100644
> --- a/target-ppc/mmu-hash64.c
> +++ b/target-ppc/mmu-hash64.c
> @@ -302,17 +302,26 @@ static int ppc_hash64_amr_prot(CPUPPCState *env, ppc_hash_pte64_t pte)
>   return prot;
> }
> 
> -static hwaddr ppc_hash64_pteg_search(CPUPPCState *env, hwaddr pteg_off,
> +static hwaddr ppc_hash64_pteg_search(CPUPPCState *env, hwaddr hash,
>                                    bool secondary, target_ulong ptem,
>                                    ppc_hash_pte64_t *pte)
> {
> -    hwaddr pte_offset = pteg_off;
> +    uint64_t index;
> +    hwaddr pte_offset;
>   target_ulong pte0, pte1;
>   int i;
> 
> +    pte_offset = (hash * HASH_PTEG_SIZE_64) & env->htab_mask;;
> +    index = (hash * HPTES_PER_GROUP) & env->htab_mask;
> +
>   for (i = 0; i < HPTES_PER_GROUP; i++) {
> -        pte0 = ppc_hash64_load_hpte0(env, pte_offset);
> -        pte1 = ppc_hash64_load_hpte1(env, pte_offset);
> +        if (kvm_enabled()) {
> +            index += i;
> +            kvmppc_hash64_load_hpte(ppc_env_get_cpu(env), index, &pte0, &pte1);

This breaks PR KVM which doesn't have an HTAB fd.

I think what you want is code in kvmppc_set_papr() that tries to fetch an HTAB fd. You can then modify the check to if (kvm_enabled() && kvmppc_has_htab_fd()), as the below case should work just fine on PR KVM.


Alex

> +        } else {
> +            pte0 = ppc_hash64_load_hpte0(env, pte_offset);
> +            pte1 = ppc_hash64_load_hpte1(env, pte_offset);
> +        }
> 
>       if ((pte0 & HPTE64_V_VALID)
>           && (secondary == !!(pte0 & HPTE64_V_SECONDARY))
> @@ -332,7 +341,7 @@ static hwaddr ppc_hash64_htab_lookup(CPUPPCState *env,
>                                    ppc_slb_t *slb, target_ulong eaddr,
>                                    ppc_hash_pte64_t *pte)
> {
> -    hwaddr pteg_off, pte_offset;
> +    hwaddr pte_offset;
>   hwaddr hash;
>   uint64_t vsid, epnshift, epnmask, epn, ptem;
> 
> @@ -367,8 +376,7 @@ static hwaddr ppc_hash64_htab_lookup(CPUPPCState *env,
>           " vsid=" TARGET_FMT_lx " ptem=" TARGET_FMT_lx
>           " hash=" TARGET_FMT_plx "\n",
>           env->htab_base, env->htab_mask, vsid, ptem,  hash);
> -    pteg_off = (hash * HASH_PTEG_SIZE_64) & env->htab_mask;
> -    pte_offset = ppc_hash64_pteg_search(env, pteg_off, 0, ptem, pte);
> +    pte_offset = ppc_hash64_pteg_search(env, hash, 0, ptem, pte);
> 
>   if (pte_offset == -1) {
>       /* Secondary PTEG lookup */
> @@ -377,8 +385,7 @@ static hwaddr ppc_hash64_htab_lookup(CPUPPCState *env,
>               " hash=" TARGET_FMT_plx "\n", env->htab_base,
>               env->htab_mask, vsid, ptem, ~hash);
> 
> -        pteg_off = (~hash * HASH_PTEG_SIZE_64) & env->htab_mask;
> -        pte_offset = ppc_hash64_pteg_search(env, pteg_off, 1, ptem, pte);
> +        pte_offset = ppc_hash64_pteg_search(env, ~hash, 1, ptem, pte);
>   }
> 
>   return pte_offset;
> -- 
> 1.8.1.2
> 

Re: [Qemu-devel] [PATCH -V3 3/4] target-ppc: Check for error on address translation in memsave command

[Alexander Graf]

On 23.08.2013, at 06:20, Aneesh Kumar K.V wrote:

> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
> 
> When we translate the virtual address to physical check for error.
> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>

I think this change is sane, but I'd really prefer to see an ack from (or get this applied by) Luiz.


Alex

> ---
> cpus.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 0f65e76..658366d 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1309,7 +1309,10 @@ void qmp_memsave(int64_t addr, int64_t size, const char *filename,
>       l = sizeof(buf);
>       if (l > size)
>           l = size;
> -        cpu_memory_rw_debug(cpu, addr, buf, l, 0);
> +        if (cpu_memory_rw_debug(cpu, addr, buf, l, 0) != 0) {
> +            error_set(errp, QERR_INVALID_PARAMETER, "addr");
> +            goto exit;
> +        }
>       if (fwrite(buf, 1, l, f) != l) {
>           error_set(errp, QERR_IO_ERROR);
>           goto exit;
> -- 
> 1.8.1.2
> 

Re: [Qemu-devel] [PATCH -V3 4/4] target-ppc: Use #define for max slb entries

[Alexander Graf]

On 23.08.2013, at 06:20, Aneesh Kumar K.V wrote:

> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
> 
> Instead of opencoding 64 use MAX_SLB_ENTRIES. We don't update the kernel
> header here.

Ah, here you're fixing up the hardcoded 64 :). Could you please check whether ARRAY_SIZE() works in all these as well? If not, this patch is good.


Alex

> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> ---
> target-ppc/cpu.h     | 3 ++-
> target-ppc/kvm.c     | 6 +++---
> target-ppc/machine.c | 2 +-
> 3 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 711db08..b06818e 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -405,6 +405,7 @@ struct ppc_slb_t {
>  uint64_t vsid;
> };
> 
> +#define MAX_SLB_ENTRIES         64
> #define SEGMENT_SHIFT_256M      28
> #define SEGMENT_MASK_256M       (~((1ULL << SEGMENT_SHIFT_256M) - 1))
> 
> @@ -947,7 +948,7 @@ struct CPUPPCState {
> #if !defined(CONFIG_USER_ONLY)
> #if defined(TARGET_PPC64)
>  /* PowerPC 64 SLB area */
> -    ppc_slb_t slb[64];
> +    ppc_slb_t slb[MAX_SLB_ENTRIES];
>  int32_t slb_nr;
> #endif
>  /* segment registers */
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index bcc6544..fce8835 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -818,7 +818,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
> 
>      /* Sync SLB */
> #ifdef TARGET_PPC64
> -        for (i = 0; i < 64; i++) {
> +        for (i = 0; i < MAX_SLB_ENTRIES; i++) {
>          sregs.u.s.ppc64.slb[i].slbe = env->slb[i].esid;
>          sregs.u.s.ppc64.slb[i].slbv = env->slb[i].vsid;
>      }
> @@ -1042,8 +1042,8 @@ int kvm_arch_get_registers(CPUState *cs)
>       * the env->slb array first so that we mark all entries invalid and
>       * update with only valid SLB entries.
>       */
> -        memset(env->slb, 0, 64 * sizeof(ppc_slb_t));
> -        for (i = 0; i < 64; i++) {
> +        memset(env->slb, 0, MAX_SLB_ENTRIES * sizeof(ppc_slb_t));
> +        for (i = 0; i < MAX_SLB_ENTRIES; i++) {
>          target_ulong rb = sregs.u.s.ppc64.slb[i].slbe;
>          target_ulong rs = sregs.u.s.ppc64.slb[i].slbv;
>          /*
> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
> index 12e1512..12c174f 100644
> --- a/target-ppc/machine.c
> +++ b/target-ppc/machine.c
> @@ -312,7 +312,7 @@ static const VMStateDescription vmstate_slb = {
>  .minimum_version_id_old = 1,
>  .fields      = (VMStateField []) {
>      VMSTATE_INT32_EQUAL(env.slb_nr, PowerPCCPU),
> -        VMSTATE_SLB_ARRAY(env.slb, PowerPCCPU, 64),
> +        VMSTATE_SLB_ARRAY(env.slb, PowerPCCPU, MAX_SLB_ENTRIES),
>      VMSTATE_END_OF_LIST()
>  }
> };
> -- 
> 1.8.1.2
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH -V3 2/4] target-ppc: Fix page table lookup with kvm enabled

[Benjamin Herrenschmidt]

On Sun, 2013-08-25 at 19:32 +0100, Alexander Graf wrote:
> > +     * At this point we are only interested in reading only bolted
> entries
> > +     */
> > +    ghf.flags = KVM_GET_HTAB_BOLTED_ONLY;
> > +    ghf.start_index = index;
> > +    htab_fd = kvm_vm_ioctl(kvm_state, KVM_PPC_GET_HTAB_FD, &ghf);
> 
> We should cache this.
> 
Also why bolted only ?

Cheers,
Ben.

Re: [Qemu-devel] [PATCH -V3 3/4] target-ppc: Check for error on address translation in memsave command

[Andreas Färber]

Am 25.08.2013 20:32, schrieb Alexander Graf:
> 
> On 23.08.2013, at 06:20, Aneesh Kumar K.V wrote:
> 
>> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>>
>> When we translate the virtual address to physical check for error.
>>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> 
> I think this change is sane, but I'd really prefer to see an ack from (or get this applied by) Luiz.
> 
> 
> Alex
> 
>> ---
>> cpus.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/cpus.c b/cpus.c
>> index 0f65e76..658366d 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -1309,7 +1309,10 @@ void qmp_memsave(int64_t addr, int64_t size, const char *filename,
>>       l = sizeof(buf);
>>       if (l > size)
>>           l = size;
>> -        cpu_memory_rw_debug(cpu, addr, buf, l, 0);
>> +        if (cpu_memory_rw_debug(cpu, addr, buf, l, 0) != 0) {
>> +            error_set(errp, QERR_INVALID_PARAMETER, "addr");

I've been repeatedly told error_set() should no longer be used, in favor
of error_setg(). :)

Andreas

>> +            goto exit;
>> +        }
>>       if (fwrite(buf, 1, l, f) != l) {
>>           error_set(errp, QERR_IO_ERROR);
>>           goto exit;
>> -- 
>> 1.8.1.2
>>
> 
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH -V3 4/4] target-ppc: Use #define for max slb entries

[Andreas Färber]

Am 25.08.2013 20:33, schrieb Alexander Graf:
> 
> On 23.08.2013, at 06:20, Aneesh Kumar K.V wrote:
> 
>> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>>
>> Instead of opencoding 64 use MAX_SLB_ENTRIES. We don't update the kernel
>> header here.
> 
> Ah, here you're fixing up the hardcoded 64 :). Could you please check whether ARRAY_SIZE() works in all these as well? If not, this patch is good.
> 
> 
> Alex
> 
>>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>> ---
>> target-ppc/cpu.h     | 3 ++-
>> target-ppc/kvm.c     | 6 +++---
>> target-ppc/machine.c | 2 +-
>> 3 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
>> index 711db08..b06818e 100644
>> --- a/target-ppc/cpu.h
>> +++ b/target-ppc/cpu.h
>> @@ -405,6 +405,7 @@ struct ppc_slb_t {
>>  uint64_t vsid;
>> };
>>
>> +#define MAX_SLB_ENTRIES         64
>> #define SEGMENT_SHIFT_256M      28
>> #define SEGMENT_MASK_256M       (~((1ULL << SEGMENT_SHIFT_256M) - 1))
>>
>> @@ -947,7 +948,7 @@ struct CPUPPCState {
>> #if !defined(CONFIG_USER_ONLY)
>> #if defined(TARGET_PPC64)
>>  /* PowerPC 64 SLB area */
>> -    ppc_slb_t slb[64];
>> +    ppc_slb_t slb[MAX_SLB_ENTRIES];
>>  int32_t slb_nr;
>> #endif
>>  /* segment registers */
>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>> index bcc6544..fce8835 100644
>> --- a/target-ppc/kvm.c
>> +++ b/target-ppc/kvm.c
>> @@ -818,7 +818,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>>
>>      /* Sync SLB */
>> #ifdef TARGET_PPC64
>> -        for (i = 0; i < 64; i++) {
>> +        for (i = 0; i < MAX_SLB_ENTRIES; i++) {
>>          sregs.u.s.ppc64.slb[i].slbe = env->slb[i].esid;
>>          sregs.u.s.ppc64.slb[i].slbv = env->slb[i].vsid;
>>      }
>> @@ -1042,8 +1042,8 @@ int kvm_arch_get_registers(CPUState *cs)
>>       * the env->slb array first so that we mark all entries invalid and
>>       * update with only valid SLB entries.
>>       */
>> -        memset(env->slb, 0, 64 * sizeof(ppc_slb_t));
>> -        for (i = 0; i < 64; i++) {
>> +        memset(env->slb, 0, MAX_SLB_ENTRIES * sizeof(ppc_slb_t));
>> +        for (i = 0; i < MAX_SLB_ENTRIES; i++) {
>>          target_ulong rb = sregs.u.s.ppc64.slb[i].slbe;
>>          target_ulong rs = sregs.u.s.ppc64.slb[i].slbv;
>>          /*
>> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
>> index 12e1512..12c174f 100644
>> --- a/target-ppc/machine.c
>> +++ b/target-ppc/machine.c
>> @@ -312,7 +312,7 @@ static const VMStateDescription vmstate_slb = {
>>  .minimum_version_id_old = 1,
>>  .fields      = (VMStateField []) {
>>      VMSTATE_INT32_EQUAL(env.slb_nr, PowerPCCPU),
>> -        VMSTATE_SLB_ARRAY(env.slb, PowerPCCPU, 64),
>> +        VMSTATE_SLB_ARRAY(env.slb, PowerPCCPU, MAX_SLB_ENTRIES),

IMO use of ARRAY_SIZE() makes it too easy to break VMState here.
Don't know how likely it is to be touched in the future, of course.

Andreas

>>      VMSTATE_END_OF_LIST()
>>  }
>> };
>> -- 
>> 1.8.1.2
>>
> 
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [Qemu-ppc] [PATCH -V3 2/4] target-ppc: Fix page table lookup with kvm enabled

[Aneesh Kumar K.V]

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

> On Sun, 2013-08-25 at 19:32 +0100, Alexander Graf wrote:
>> > +     * At this point we are only interested in reading only bolted
>> entries
>> > +     */
>> > +    ghf.flags = KVM_GET_HTAB_BOLTED_ONLY;
>> > +    ghf.start_index = index;
>> > +    htab_fd = kvm_vm_ioctl(kvm_state, KVM_PPC_GET_HTAB_FD, &ghf);
>> 
>> We should cache this.
>> 
> Also why bolted only ?

because non bolted entries could be invalidated and reused by the time
we look at the returned hpte values. I am not sure, whether it is ok or
we need to make sure such a thing doesn't happen. For the use case i am
looking at, ie, to dump the kernel address looking at bolted entries was
enough. 

-aneesh

Re: [Qemu-devel] [Qemu-ppc] [PATCH -V3 2/4] target-ppc: Fix page table lookup with kvm enabled

[Benjamin Herrenschmidt]

On Mon, 2013-08-26 at 09:03 +0530, Aneesh Kumar K.V wrote:
> 
> because non bolted entries could be invalidated and reused by the time
> we look at the returned hpte values. I am not sure, whether it is ok or
> we need to make sure such a thing doesn't happen. For the use case i am
> looking at, ie, to dump the kernel address looking at bolted entries was
> enough. 

It's general debugging right ?, if we stop the simulation we are fine, if we
look at it "live", anything is fair game. Without non-bolted entries, you don't
get vmalloc space for example, which can be annoying for debugging :-)

Cheers,
Ben.

Re: [Qemu-devel] [PATCH -V3 2/4] target-ppc: Fix page table lookup with kvm enabled

[Aneesh Kumar K.V]

Alexander Graf <agraf@suse.de> writes:

> On 23.08.2013, at 06:20, Aneesh Kumar K.V wrote:
>
>> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>> 
>> With kvm enabled, we store the hash page table information in the hypervisor.
>> Use ioctl to read the htab contents. Without this we get the below error when
>> trying to read the guest address
>> 
>> (gdb) x/10 do_fork
>> 0xc000000000098660 <do_fork>:   Cannot access memory at address 0xc000000000098660
>> (gdb)
>> 
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>> ---
>> target-ppc/kvm.c        | 45 +++++++++++++++++++++++++++++++++++++++++++++
>> target-ppc/kvm_ppc.h    |  9 ++++++++-
>> target-ppc/mmu-hash64.c | 25 ++++++++++++++++---------
>> 3 files changed, 69 insertions(+), 10 deletions(-)
>> 
>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>> index 6878af2..bcc6544 100644
>> --- a/target-ppc/kvm.c
>> +++ b/target-ppc/kvm.c
>> @@ -1891,3 +1891,48 @@ int kvm_arch_on_sigbus(int code, void *addr)
>> void kvm_arch_init_irq_routing(KVMState *s)
>> {
>> }
>> +
>> +int kvmppc_hash64_load_hpte(PowerPCCPU *cpu, uint64_t index,
>> +                            target_ulong *hpte0, target_ulong *hpte1)
>> +{
>> +    int htab_fd;
>> +    struct kvm_get_htab_fd ghf;
>> +    struct kvm_get_htab_buf {
>> +        struct kvm_get_htab_header header;
>> +        /*
>> +         * Older kernel required one extra byte.
>> +         */
>> +        unsigned long hpte[3];
>> +    } hpte_buf;
>> +
>> +    *hpte0 = 0;
>> +    *hpte1 = 0;
>> +    if (!cap_htab_fd) {
>> +        return 0;
>> +    }
>> +    /*
>> +     * At this point we are only interested in reading only bolted entries
>> +     */
>> +    ghf.flags = KVM_GET_HTAB_BOLTED_ONLY;
>> +    ghf.start_index = index;
>> +    htab_fd = kvm_vm_ioctl(kvm_state, KVM_PPC_GET_HTAB_FD, &ghf);
>
> We should cache this.

The fd returned by KVM_PPC_GET_HTAB_FD doesn't support a proper lseek
interface. ie, we cannot seek around to read the hpte entries at index.
The call paths are also not in the hot path. Hence I didn't look at
caching. We could definitely avoid doing the ioctl in loop. See below
for the changes.


>
>> +    if (htab_fd < 0) {
>> +        return htab_fd;
>> +    }
>> +
>> +    if (read(htab_fd, &hpte_buf, sizeof(hpte_buf)) < 0) {
>> +        goto out;
>> +    }
>> +    /*
>> +     * We only requested for one entry, So we should get only 1
>> +     * valid entry at the same index
>> +     */
>> +    if (hpte_buf.header.n_valid != 1 || hpte_buf.header.index != index) {
>> +        goto out;
>> +    }
>> +    *hpte0 = hpte_buf.hpte[0];
>> +    *hpte1 = hpte_buf.hpte[1];
>> +out:
>> +    close(htab_fd);
>> +    return 0;
>> +}
>> diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
>> index 4ae7bf2..e25373a 100644
>> --- a/target-ppc/kvm_ppc.h
>> +++ b/target-ppc/kvm_ppc.h
>> @@ -42,7 +42,8 @@ int kvmppc_get_htab_fd(bool write);
>> int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t max_ns);
>> int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
>>                          uint16_t n_valid, uint16_t n_invalid);
>> -
>> +int kvmppc_hash64_load_hpte(PowerPCCPU *cpu, uint64_t index,
>> +                            target_ulong *hpte0, target_ulong *hpte1);
>> #else
>> 
>> static inline uint32_t kvmppc_get_tbfreq(void)
>> @@ -181,6 +182,12 @@ static inline int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
>>   abort();
>> }
>> 
>> +static inline int kvmppc_hash64_load_hpte(PowerPCCPU *cpu, uint64_t index,
>> +                                          target_ulong *hpte0,
>> +                                          target_ulong *hpte1)
>> +{
>> +    abort();
>> +}
>> #endif
>> 
>> #ifndef CONFIG_KVM
>> diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
>> index 67fc1b5..4d8120c 100644
>> --- a/target-ppc/mmu-hash64.c
>> +++ b/target-ppc/mmu-hash64.c
>> @@ -302,17 +302,26 @@ static int ppc_hash64_amr_prot(CPUPPCState *env, ppc_hash_pte64_t pte)
>>   return prot;
>> }
>> 
>> -static hwaddr ppc_hash64_pteg_search(CPUPPCState *env, hwaddr pteg_off,
>> +static hwaddr ppc_hash64_pteg_search(CPUPPCState *env, hwaddr hash,
>>                                    bool secondary, target_ulong ptem,
>>                                    ppc_hash_pte64_t *pte)
>> {
>> -    hwaddr pte_offset = pteg_off;
>> +    uint64_t index;
>> +    hwaddr pte_offset;
>>   target_ulong pte0, pte1;
>>   int i;
>> 
>> +    pte_offset = (hash * HASH_PTEG_SIZE_64) & env->htab_mask;;
>> +    index = (hash * HPTES_PER_GROUP) & env->htab_mask;
>> +
>>   for (i = 0; i < HPTES_PER_GROUP; i++) {
>> -        pte0 = ppc_hash64_load_hpte0(env, pte_offset);
>> -        pte1 = ppc_hash64_load_hpte1(env, pte_offset);
>> +        if (kvm_enabled()) {
>> +            index += i;
>> +            kvmppc_hash64_load_hpte(ppc_env_get_cpu(env), index, &pte0, &pte1);
>
> This breaks PR KVM which doesn't have an HTAB fd.
>
> I think what you want is code in kvmppc_set_papr() that tries to fetch
> an HTAB fd. You can then modify the check to if (kvm_enabled() &&
> kvmppc_has_htab_fd()), as the below case should work just fine on PR
> KVM.

As explained before caching htab fd may not really work. How about 

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index fce8835..cf6aca4 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -1892,19 +1892,24 @@ void kvm_arch_init_irq_routing(KVMState *s)
 {
 }
 
-int kvmppc_hash64_load_hpte(PowerPCCPU *cpu, uint64_t index,
-                            target_ulong *hpte0, target_ulong *hpte1)
+hwaddr kvmppc_hash64_pteg_search(PowerPCCPU *cpu, hwaddr hash,
+                                 bool secondary, target_ulong ptem,
+                                 target_ulong *hpte0, target_ulong *hpte1)
 {
     int htab_fd;
+    uint64_t index;
+    hwaddr pte_offset;
+    target_ulong pte0, pte1;
     struct kvm_get_htab_fd ghf;
     struct kvm_get_htab_buf {
         struct kvm_get_htab_header header;
         /*
          * Older kernel required one extra byte.
          */
-        unsigned long hpte[3];
+        unsigned long hpte[(HPTES_PER_GROUP * 2) + 1];
     } hpte_buf;
 
+    index = (hash * HPTES_PER_GROUP) & cpu->env.htab_mask;
     *hpte0 = 0;
     *hpte1 = 0;
     if (!cap_htab_fd) {
@@ -1917,22 +1922,33 @@ int kvmppc_hash64_load_hpte(PowerPCCPU *cpu, uint64_t index,
     ghf.start_index = index;
     htab_fd = kvm_vm_ioctl(kvm_state, KVM_PPC_GET_HTAB_FD, &ghf);
     if (htab_fd < 0) {
-        return htab_fd;
-    }
-
-    if (read(htab_fd, &hpte_buf, sizeof(hpte_buf)) < 0) {
-        goto out;
+        goto error_out;
     }
     /*
-     * We only requested for one entry, So we should get only 1
-     * valid entry at the same index
+     * Read the hpte group
      */
-    if (hpte_buf.header.n_valid != 1 || hpte_buf.header.index != index) {
+    if (read(htab_fd, &hpte_buf, sizeof(hpte_buf)) < 0) {
         goto out;
     }
-    *hpte0 = hpte_buf.hpte[0];
-    *hpte1 = hpte_buf.hpte[1];
+
+    index = 0;
+    pte_offset = (hash * HASH_PTEG_SIZE_64) & cpu->env.htab_mask;;
+    while (index < hpte_buf.header.n_valid) {
+        pte0 = hpte_buf.hpte[(index * 2)];
+        pte1 = hpte_buf.hpte[(index * 2) + 1];
+        if ((pte0 & HPTE64_V_VALID)
+            && (secondary == !!(pte0 & HPTE64_V_SECONDARY))
+            && HPTE64_V_COMPARE(pte0, ptem)) {
+            *hpte0 = pte0;
+            *hpte1 = pte1;
+            close(htab_fd);
+            return pte_offset;
+        }
+        index++;
+        pte_offset += HASH_PTE_SIZE_64;
+    }
 out:
     close(htab_fd);
-    return 0;
+error_out:
+    return -1;
 }
diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
index e25373a..dad0e57 100644
--- a/target-ppc/kvm_ppc.h
+++ b/target-ppc/kvm_ppc.h
@@ -42,8 +42,9 @@ int kvmppc_get_htab_fd(bool write);
 int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t max_ns);
 int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
                            uint16_t n_valid, uint16_t n_invalid);
-int kvmppc_hash64_load_hpte(PowerPCCPU *cpu, uint64_t index,
-                            target_ulong *hpte0, target_ulong *hpte1);
+hwaddr kvmppc_hash64_pteg_search(PowerPCCPU *cpu, hwaddr hash,
+                                 bool secondary, target_ulong ptem,
+                                 target_ulong *hpte0, target_ulong *hpte1);
 #else
 
 static inline uint32_t kvmppc_get_tbfreq(void)
@@ -182,9 +183,11 @@ static inline int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
     abort();
 }
 
-static inline int kvmppc_hash64_load_hpte(PowerPCCPU *cpu, uint64_t index,
-                                          target_ulong *hpte0,
-                                          target_ulong *hpte1)
+static inline hwaddr kvmppc_hash64_pteg_search(PowerPCCPU *cpu, hwaddr hash,
+                                               bool secondary,
+                                               target_ulong ptem,
+                                               target_ulong *hpte0,
+                                               target_ulong *hpte1)
 {
     abort();
 }
diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
index 4d8120c..2288fe8 100644
--- a/target-ppc/mmu-hash64.c
+++ b/target-ppc/mmu-hash64.c
@@ -306,35 +306,39 @@ static hwaddr ppc_hash64_pteg_search(CPUPPCState *env, hwaddr hash,
                                      bool secondary, target_ulong ptem,
                                      ppc_hash_pte64_t *pte)
 {
-    uint64_t index;
     hwaddr pte_offset;
     target_ulong pte0, pte1;
-    int i;
-
-    pte_offset = (hash * HASH_PTEG_SIZE_64) & env->htab_mask;;
-    index = (hash * HPTES_PER_GROUP) & env->htab_mask;
-
-    for (i = 0; i < HPTES_PER_GROUP; i++) {
-        if (kvm_enabled()) {
-            index += i;
-            kvmppc_hash64_load_hpte(ppc_env_get_cpu(env), index, &pte0, &pte1);
-        } else {
+    int i, ret = 0;
+
+    if (kvm_enabled()) {
+        ret = kvmppc_hash64_pteg_search(ppc_env_get_cpu(env), hash,
+                                        secondary, ptem,
+                                        &pte->pte0, &pte->pte1);
+    }
+    /*
+     * We don't support htab fd, check whether we have a copy of htab
+     */
+    if (!ret) {
+        pte_offset = (hash * HASH_PTEG_SIZE_64) & env->htab_mask;;
+        for (i = 0; i < HPTES_PER_GROUP; i++) {
             pte0 = ppc_hash64_load_hpte0(env, pte_offset);
             pte1 = ppc_hash64_load_hpte1(env, pte_offset);
-        }
 
-        if ((pte0 & HPTE64_V_VALID)
-            && (secondary == !!(pte0 & HPTE64_V_SECONDARY))
-            && HPTE64_V_COMPARE(pte0, ptem)) {
-            pte->pte0 = pte0;
-            pte->pte1 = pte1;
-            return pte_offset;
+            if ((pte0 & HPTE64_V_VALID)
+                && (secondary == !!(pte0 & HPTE64_V_SECONDARY))
+                && HPTE64_V_COMPARE(pte0, ptem)) {
+                pte->pte0 = pte0;
+                pte->pte1 = pte1;
+                return pte_offset;
+            }
+            pte_offset += HASH_PTE_SIZE_64;
         }
-
-        pte_offset += HASH_PTE_SIZE_64;
+        /*
+         * We didn't find a valid entry.
+         */
+        ret = -1;
     }
-
-    return -1;
+    return ret;
 }
 
 static hwaddr ppc_hash64_htab_lookup(CPUPPCState *env,



-aneesh

Re: [Qemu-devel] [PATCH -V3 1/4] target-ppc: Update slb array with correct index values.

[Aneesh Kumar K.V]

Alexander Graf <agraf@suse.de> writes:

> On 23.08.2013, at 06:20, Aneesh Kumar K.V wrote:
>
>> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>> 
>> Without this, a value of rb=0 and rs=0 results in replacing the 0th
>> index. This can be observed when using gdb remote debugging support.
>> 
>> (gdb) x/10i do_fork
>> 0xc000000000085330 <do_fork>:        Cannot access memory at address 0xc000000000085330
>> (gdb)
>> 
>> This is because when we do the slb sync via kvm_cpu_synchronize_state,
>> we overwrite the slb entry (0th entry) for 0xc000000000085330
>> 
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>> ---
>> target-ppc/kvm.c | 20 ++++++++++++++++++--
>> 1 file changed, 18 insertions(+), 2 deletions(-)
>> 
>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>> index 30a870e..6878af2 100644
>> --- a/target-ppc/kvm.c
>> +++ b/target-ppc/kvm.c
>> @@ -1033,9 +1033,25 @@ int kvm_arch_get_registers(CPUState *cs)
>> 
>>     /* Sync SLB */
>> #ifdef TARGET_PPC64
>> +        /*
>> +         * KVM_GET_SREGS doesn't return slb entry with slot information
>> +         * same as index. The ioctl zero fills the array and update only
>> +         * upto slb_max entries. We cannot depend on the slot value
>> +         * in the slbe field for update, because a zero slbe value would
>> +         * result in us wrongly updating the 0th index. Instead we zero fill
>> +         * the env->slb array first so that we mark all entries invalid and
>> +         * update with only valid SLB entries.
>
> Still too negative. How about something like this:
>
> /*
> * The packed SLB array we get from KVM only contains information
> *  about valid entries. So we flush our internal copy to get rid of stale
> *  ones, then put all valid SLB entries back in.
> */

updated

>
>> +         */
>> +        memset(env->slb, 0, 64 * sizeof(ppc_slb_t));
>
> Can't we use ARRAY_SIZE here and below?

I was thinking we want to be explicit there saying we are zeroing out all
the 64 entries. I could do sizeof(env->slb).

-anesh

Re: [Qemu-devel] [PATCH -V3 1/4] target-ppc: Update slb array with correct index values.

[Alexander Graf]

On 26.08.2013, at 08:46, Aneesh Kumar K.V wrote:

> Alexander Graf <agraf@suse.de> writes:
> 
>> On 23.08.2013, at 06:20, Aneesh Kumar K.V wrote:
>> 
>>> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>>> 
>>> Without this, a value of rb=0 and rs=0 results in replacing the 0th
>>> index. This can be observed when using gdb remote debugging support.
>>> 
>>> (gdb) x/10i do_fork
>>> 0xc000000000085330 <do_fork>:        Cannot access memory at address 0xc000000000085330
>>> (gdb)
>>> 
>>> This is because when we do the slb sync via kvm_cpu_synchronize_state,
>>> we overwrite the slb entry (0th entry) for 0xc000000000085330
>>> 
>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>>> ---
>>> target-ppc/kvm.c | 20 ++++++++++++++++++--
>>> 1 file changed, 18 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>>> index 30a870e..6878af2 100644
>>> --- a/target-ppc/kvm.c
>>> +++ b/target-ppc/kvm.c
>>> @@ -1033,9 +1033,25 @@ int kvm_arch_get_registers(CPUState *cs)
>>> 
>>>    /* Sync SLB */
>>> #ifdef TARGET_PPC64
>>> +        /*
>>> +         * KVM_GET_SREGS doesn't return slb entry with slot information
>>> +         * same as index. The ioctl zero fills the array and update only
>>> +         * upto slb_max entries. We cannot depend on the slot value
>>> +         * in the slbe field for update, because a zero slbe value would
>>> +         * result in us wrongly updating the 0th index. Instead we zero fill
>>> +         * the env->slb array first so that we mark all entries invalid and
>>> +         * update with only valid SLB entries.
>> 
>> Still too negative. How about something like this:
>> 
>> /*
>> * The packed SLB array we get from KVM only contains information
>> *  about valid entries. So we flush our internal copy to get rid of stale
>> *  ones, then put all valid SLB entries back in.
>> */
> 
> updated
> 
>> 
>>> +         */
>>> +        memset(env->slb, 0, 64 * sizeof(ppc_slb_t));
>> 
>> Can't we use ARRAY_SIZE here and below?
> 
> I was thinking we want to be explicit there saying we are zeroing out all
> the 64 entries. I could do sizeof(env->slb).

Yup, that works too. In the loop below s/64/ARRAY_SIZE(env->slb)/ should work too I think.


Alex

Re: [Qemu-devel] [PATCH -V3 2/4] target-ppc: Fix page table lookup with kvm enabled

[Alexander Graf]

On 26.08.2013, at 07:46, Aneesh Kumar K.V wrote:

> Alexander Graf <agraf@suse.de> writes:
> 
>> On 23.08.2013, at 06:20, Aneesh Kumar K.V wrote:
>> 
>>> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>>> 
>>> With kvm enabled, we store the hash page table information in the hypervisor.
>>> Use ioctl to read the htab contents. Without this we get the below error when
>>> trying to read the guest address
>>> 
>>> (gdb) x/10 do_fork
>>> 0xc000000000098660 <do_fork>:   Cannot access memory at address 0xc000000000098660
>>> (gdb)
>>> 
>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>>> ---
>>> target-ppc/kvm.c        | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>> target-ppc/kvm_ppc.h    |  9 ++++++++-
>>> target-ppc/mmu-hash64.c | 25 ++++++++++++++++---------
>>> 3 files changed, 69 insertions(+), 10 deletions(-)
>>> 
>>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>>> index 6878af2..bcc6544 100644
>>> --- a/target-ppc/kvm.c
>>> +++ b/target-ppc/kvm.c
>>> @@ -1891,3 +1891,48 @@ int kvm_arch_on_sigbus(int code, void *addr)
>>> void kvm_arch_init_irq_routing(KVMState *s)
>>> {
>>> }
>>> +
>>> +int kvmppc_hash64_load_hpte(PowerPCCPU *cpu, uint64_t index,
>>> +                            target_ulong *hpte0, target_ulong *hpte1)
>>> +{
>>> +    int htab_fd;
>>> +    struct kvm_get_htab_fd ghf;
>>> +    struct kvm_get_htab_buf {
>>> +        struct kvm_get_htab_header header;
>>> +        /*
>>> +         * Older kernel required one extra byte.
>>> +         */
>>> +        unsigned long hpte[3];
>>> +    } hpte_buf;
>>> +
>>> +    *hpte0 = 0;
>>> +    *hpte1 = 0;
>>> +    if (!cap_htab_fd) {
>>> +        return 0;
>>> +    }
>>> +    /*
>>> +     * At this point we are only interested in reading only bolted entries
>>> +     */
>>> +    ghf.flags = KVM_GET_HTAB_BOLTED_ONLY;
>>> +    ghf.start_index = index;
>>> +    htab_fd = kvm_vm_ioctl(kvm_state, KVM_PPC_GET_HTAB_FD, &ghf);
>> 
>> We should cache this.
> 
> The fd returned by KVM_PPC_GET_HTAB_FD doesn't support a proper lseek
> interface. ie, we cannot seek around to read the hpte entries at index.
> The call paths are also not in the hot path. Hence I didn't look at
> caching. We could definitely avoid doing the ioctl in loop. See below
> for the changes.
> 
> 
>> 
>>> +    if (htab_fd < 0) {
>>> +        return htab_fd;
>>> +    }
>>> +
>>> +    if (read(htab_fd, &hpte_buf, sizeof(hpte_buf)) < 0) {
>>> +        goto out;
>>> +    }
>>> +    /*
>>> +     * We only requested for one entry, So we should get only 1
>>> +     * valid entry at the same index
>>> +     */
>>> +    if (hpte_buf.header.n_valid != 1 || hpte_buf.header.index != index) {
>>> +        goto out;
>>> +    }
>>> +    *hpte0 = hpte_buf.hpte[0];
>>> +    *hpte1 = hpte_buf.hpte[1];
>>> +out:
>>> +    close(htab_fd);
>>> +    return 0;
>>> +}
>>> diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
>>> index 4ae7bf2..e25373a 100644
>>> --- a/target-ppc/kvm_ppc.h
>>> +++ b/target-ppc/kvm_ppc.h
>>> @@ -42,7 +42,8 @@ int kvmppc_get_htab_fd(bool write);
>>> int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t max_ns);
>>> int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
>>>                         uint16_t n_valid, uint16_t n_invalid);
>>> -
>>> +int kvmppc_hash64_load_hpte(PowerPCCPU *cpu, uint64_t index,
>>> +                            target_ulong *hpte0, target_ulong *hpte1);
>>> #else
>>> 
>>> static inline uint32_t kvmppc_get_tbfreq(void)
>>> @@ -181,6 +182,12 @@ static inline int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
>>>  abort();
>>> }
>>> 
>>> +static inline int kvmppc_hash64_load_hpte(PowerPCCPU *cpu, uint64_t index,
>>> +                                          target_ulong *hpte0,
>>> +                                          target_ulong *hpte1)
>>> +{
>>> +    abort();
>>> +}
>>> #endif
>>> 
>>> #ifndef CONFIG_KVM
>>> diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
>>> index 67fc1b5..4d8120c 100644
>>> --- a/target-ppc/mmu-hash64.c
>>> +++ b/target-ppc/mmu-hash64.c
>>> @@ -302,17 +302,26 @@ static int ppc_hash64_amr_prot(CPUPPCState *env, ppc_hash_pte64_t pte)
>>>  return prot;
>>> }
>>> 
>>> -static hwaddr ppc_hash64_pteg_search(CPUPPCState *env, hwaddr pteg_off,
>>> +static hwaddr ppc_hash64_pteg_search(CPUPPCState *env, hwaddr hash,
>>>                                   bool secondary, target_ulong ptem,
>>>                                   ppc_hash_pte64_t *pte)
>>> {
>>> -    hwaddr pte_offset = pteg_off;
>>> +    uint64_t index;
>>> +    hwaddr pte_offset;
>>>  target_ulong pte0, pte1;
>>>  int i;
>>> 
>>> +    pte_offset = (hash * HASH_PTEG_SIZE_64) & env->htab_mask;;
>>> +    index = (hash * HPTES_PER_GROUP) & env->htab_mask;
>>> +
>>>  for (i = 0; i < HPTES_PER_GROUP; i++) {
>>> -        pte0 = ppc_hash64_load_hpte0(env, pte_offset);
>>> -        pte1 = ppc_hash64_load_hpte1(env, pte_offset);
>>> +        if (kvm_enabled()) {
>>> +            index += i;
>>> +            kvmppc_hash64_load_hpte(ppc_env_get_cpu(env), index, &pte0, &pte1);
>> 
>> This breaks PR KVM which doesn't have an HTAB fd.
>> 
>> I think what you want is code in kvmppc_set_papr() that tries to fetch
>> an HTAB fd. You can then modify the check to if (kvm_enabled() &&
>> kvmppc_has_htab_fd()), as the below case should work just fine on PR
>> KVM.
> 
> As explained before caching htab fd may not really work. How about 
> 
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index fce8835..cf6aca4 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -1892,19 +1892,24 @@ void kvm_arch_init_irq_routing(KVMState *s)
> {
> }
> 
> -int kvmppc_hash64_load_hpte(PowerPCCPU *cpu, uint64_t index,
> -                            target_ulong *hpte0, target_ulong *hpte1)

Against which tree is this? I don't even have this function in mine.

> +hwaddr kvmppc_hash64_pteg_search(PowerPCCPU *cpu, hwaddr hash,
> +                                 bool secondary, target_ulong ptem,

I'm having a hard time following this code. How do you tell the kernel which PTE group you're interested in?

> +                                 target_ulong *hpte0, target_ulong *hpte1)
> {
>     int htab_fd;
> +    uint64_t index;
> +    hwaddr pte_offset;
> +    target_ulong pte0, pte1;
>     struct kvm_get_htab_fd ghf;
>     struct kvm_get_htab_buf {
>         struct kvm_get_htab_header header;
>         /*
>          * Older kernel required one extra byte.
>          */
> -        unsigned long hpte[3];
> +        unsigned long hpte[(HPTES_PER_GROUP * 2) + 1];

Does this need your kernel patch to work?


Alex

Re: [Qemu-devel] [PATCH -V3 1/4] target-ppc: Update slb array with correct index values.

[Aneesh Kumar K.V]

Alexander Graf <agraf@suse.de> writes:

> On 26.08.2013, at 08:46, Aneesh Kumar K.V wrote:
>
>> Alexander Graf <agraf@suse.de> writes:
>> 

....

>>> 
>>>> +         */
>>>> +        memset(env->slb, 0, 64 * sizeof(ppc_slb_t));
>>> 
>>> Can't we use ARRAY_SIZE here and below?
>> 
>> I was thinking we want to be explicit there saying we are zeroing out all
>> the 64 entries. I could do sizeof(env->slb).
>
> Yup, that works too. In the loop below s/64/ARRAY_SIZE(env->slb)/ should work too I think.
>

Updated with the above change.

-aneesh

Re: [Qemu-devel] [PATCH -V3 2/4] target-ppc: Fix page table lookup with kvm enabled

[Aneesh Kumar K.V]

Alexander Graf <agraf@suse.de> writes:

> On 26.08.2013, at 07:46, Aneesh Kumar K.V wrote:
>
>> Alexander Graf <agraf@suse.de> writes:

...

>>> 
>>> This breaks PR KVM which doesn't have an HTAB fd.
>>> 
>>> I think what you want is code in kvmppc_set_papr() that tries to fetch
>>> an HTAB fd. You can then modify the check to if (kvm_enabled() &&
>>> kvmppc_has_htab_fd()), as the below case should work just fine on PR
>>> KVM.
>> 
>> As explained before caching htab fd may not really work. How about 
>> 
>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>> index fce8835..cf6aca4 100644
>> --- a/target-ppc/kvm.c
>> +++ b/target-ppc/kvm.c
>> @@ -1892,19 +1892,24 @@ void kvm_arch_init_irq_routing(KVMState *s)
>> {
>> }
>> 
>> -int kvmppc_hash64_load_hpte(PowerPCCPU *cpu, uint64_t index,
>> -                            target_ulong *hpte0, target_ulong *hpte1)
>
> Against which tree is this? I don't even have this function in mine.

Sorry for the confusion, i wanted to show how it changed from the
current function. So it was done as a patch against this series. 

>
>> +hwaddr kvmppc_hash64_pteg_search(PowerPCCPU *cpu, hwaddr hash,
>> +                                 bool secondary, target_ulong ptem,
>
> I'm having a hard time following this code. How do you tell the kernel which PTE group you're interested in?
>
>> +                                 target_ulong *hpte0, target_ulong *hpte1)
>> {
>>     int htab_fd;
>> +    uint64_t index;
>> +    hwaddr pte_offset;
>> +    target_ulong pte0, pte1;
>>     struct kvm_get_htab_fd ghf;
>>     struct kvm_get_htab_buf {
>>         struct kvm_get_htab_header header;
>>         /*
>>          * Older kernel required one extra byte.
>>          */
>> -        unsigned long hpte[3];
>> +        unsigned long hpte[(HPTES_PER_GROUP * 2) + 1];
>
> Does this need your kernel patch to work?

This is the updated patch.

commit f38b39855c383c55e4ad5d0a4ab617a99de134d8
Author: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
Date:   Wed Aug 7 11:34:02 2013 +0530

    target-ppc: Fix page table lookup with kvm enabled
    
    With kvm enabled, we store the hash page table information in the hypervisor.
    Use ioctl to read the htab contents. Without this we get the below error when
    trying to read the guest address
    
     (gdb) x/10 do_fork
     0xc000000000098660 <do_fork>:   Cannot access memory at address 0xc000000000098660
     (gdb)
    
    Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 1838465..05b066c 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -1888,3 +1888,62 @@ int kvm_arch_on_sigbus(int code, void *addr)
 void kvm_arch_init_irq_routing(KVMState *s)
 {
 }
+
+hwaddr kvmppc_hash64_pteg_search(PowerPCCPU *cpu, hwaddr hash,
+                                 bool secondary, target_ulong ptem,
+                                 target_ulong *hpte0, target_ulong *hpte1)
+{
+    int htab_fd;
+    uint64_t index;
+    hwaddr pte_offset;
+    target_ulong pte0, pte1;
+    struct kvm_get_htab_fd ghf;
+    struct kvm_get_htab_buf {
+        struct kvm_get_htab_header header;
+        /*
+         * Older kernel required one extra byte.
+         */
+        unsigned long hpte[(HPTES_PER_GROUP * 2) + 1];
+    } hpte_buf;
+
+    index = (hash * HPTES_PER_GROUP) & cpu->env.htab_mask;
+    *hpte0 = 0;
+    *hpte1 = 0;
+    if (!cap_htab_fd) {
+        return 0;
+    }
+
+    ghf.flags = 0;
+    ghf.start_index = index;
+    htab_fd = kvm_vm_ioctl(kvm_state, KVM_PPC_GET_HTAB_FD, &ghf);
+    if (htab_fd < 0) {
+        goto error_out;
+    }
+    /*
+     * Read the hpte group
+     */
+    if (read(htab_fd, &hpte_buf, sizeof(hpte_buf)) < 0) {
+        goto out;
+    }
+
+    index = 0;
+    pte_offset = (hash * HASH_PTEG_SIZE_64) & cpu->env.htab_mask;;
+    while (index < hpte_buf.header.n_valid) {
+        pte0 = hpte_buf.hpte[(index * 2)];
+        pte1 = hpte_buf.hpte[(index * 2) + 1];
+        if ((pte0 & HPTE64_V_VALID)
+            && (secondary == !!(pte0 & HPTE64_V_SECONDARY))
+            && HPTE64_V_COMPARE(pte0, ptem)) {
+            *hpte0 = pte0;
+            *hpte1 = pte1;
+            close(htab_fd);
+            return pte_offset;
+        }
+        index++;
+        pte_offset += HASH_PTE_SIZE_64;
+    }
+out:
+    close(htab_fd);
+error_out:
+    return -1;
+}
diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
index 4ae7bf2..dad0e57 100644
--- a/target-ppc/kvm_ppc.h
+++ b/target-ppc/kvm_ppc.h
@@ -42,7 +42,9 @@ int kvmppc_get_htab_fd(bool write);
 int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t max_ns);
 int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
                            uint16_t n_valid, uint16_t n_invalid);
-
+hwaddr kvmppc_hash64_pteg_search(PowerPCCPU *cpu, hwaddr hash,
+                                 bool secondary, target_ulong ptem,
+                                 target_ulong *hpte0, target_ulong *hpte1);
 #else
 
 static inline uint32_t kvmppc_get_tbfreq(void)
@@ -181,6 +183,14 @@ static inline int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
     abort();
 }
 
+static inline hwaddr kvmppc_hash64_pteg_search(PowerPCCPU *cpu, hwaddr hash,
+                                               bool secondary,
+                                               target_ulong ptem,
+                                               target_ulong *hpte0,
+                                               target_ulong *hpte1)
+{
+    abort();
+}
 #endif
 
 #ifndef CONFIG_KVM
diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
index 67fc1b5..2288fe8 100644
--- a/target-ppc/mmu-hash64.c
+++ b/target-ppc/mmu-hash64.c
@@ -302,37 +302,50 @@ static int ppc_hash64_amr_prot(CPUPPCState *env, ppc_hash_pte64_t pte)
     return prot;
 }
 
-static hwaddr ppc_hash64_pteg_search(CPUPPCState *env, hwaddr pteg_off,
+static hwaddr ppc_hash64_pteg_search(CPUPPCState *env, hwaddr hash,
                                      bool secondary, target_ulong ptem,
                                      ppc_hash_pte64_t *pte)
 {
-    hwaddr pte_offset = pteg_off;
+    hwaddr pte_offset;
     target_ulong pte0, pte1;
-    int i;
-
-    for (i = 0; i < HPTES_PER_GROUP; i++) {
-        pte0 = ppc_hash64_load_hpte0(env, pte_offset);
-        pte1 = ppc_hash64_load_hpte1(env, pte_offset);
-
-        if ((pte0 & HPTE64_V_VALID)
-            && (secondary == !!(pte0 & HPTE64_V_SECONDARY))
-            && HPTE64_V_COMPARE(pte0, ptem)) {
-            pte->pte0 = pte0;
-            pte->pte1 = pte1;
-            return pte_offset;
+    int i, ret = 0;
+
+    if (kvm_enabled()) {
+        ret = kvmppc_hash64_pteg_search(ppc_env_get_cpu(env), hash,
+                                        secondary, ptem,
+                                        &pte->pte0, &pte->pte1);
+    }
+    /*
+     * We don't support htab fd, check whether we have a copy of htab
+     */
+    if (!ret) {
+        pte_offset = (hash * HASH_PTEG_SIZE_64) & env->htab_mask;;
+        for (i = 0; i < HPTES_PER_GROUP; i++) {
+            pte0 = ppc_hash64_load_hpte0(env, pte_offset);
+            pte1 = ppc_hash64_load_hpte1(env, pte_offset);
+
+            if ((pte0 & HPTE64_V_VALID)
+                && (secondary == !!(pte0 & HPTE64_V_SECONDARY))
+                && HPTE64_V_COMPARE(pte0, ptem)) {
+                pte->pte0 = pte0;
+                pte->pte1 = pte1;
+                return pte_offset;
+            }
+            pte_offset += HASH_PTE_SIZE_64;
         }
-
-        pte_offset += HASH_PTE_SIZE_64;
+        /*
+         * We didn't find a valid entry.
+         */
+        ret = -1;
     }
-
-    return -1;
+    return ret;
 }
 
 static hwaddr ppc_hash64_htab_lookup(CPUPPCState *env,
                                      ppc_slb_t *slb, target_ulong eaddr,
                                      ppc_hash_pte64_t *pte)
 {
-    hwaddr pteg_off, pte_offset;
+    hwaddr pte_offset;
     hwaddr hash;
     uint64_t vsid, epnshift, epnmask, epn, ptem;
 
@@ -367,8 +380,7 @@ static hwaddr ppc_hash64_htab_lookup(CPUPPCState *env,
             " vsid=" TARGET_FMT_lx " ptem=" TARGET_FMT_lx
             " hash=" TARGET_FMT_plx "\n",
             env->htab_base, env->htab_mask, vsid, ptem,  hash);
-    pteg_off = (hash * HASH_PTEG_SIZE_64) & env->htab_mask;
-    pte_offset = ppc_hash64_pteg_search(env, pteg_off, 0, ptem, pte);
+    pte_offset = ppc_hash64_pteg_search(env, hash, 0, ptem, pte);
 
     if (pte_offset == -1) {
         /* Secondary PTEG lookup */
@@ -377,8 +389,7 @@ static hwaddr ppc_hash64_htab_lookup(CPUPPCState *env,
                 " hash=" TARGET_FMT_plx "\n", env->htab_base,
                 env->htab_mask, vsid, ptem, ~hash);
 
-        pteg_off = (~hash * HASH_PTEG_SIZE_64) & env->htab_mask;
-        pte_offset = ppc_hash64_pteg_search(env, pteg_off, 1, ptem, pte);
+        pte_offset = ppc_hash64_pteg_search(env, ~hash, 1, ptem, pte);
     }
 
     return pte_offset;

Re: [Qemu-devel] [PATCH -V3 3/4] target-ppc: Check for error on address translation in memsave command

[Aneesh Kumar K.V]

Andreas Färber <afaerber@suse.de> writes:

> Am 25.08.2013 20:32, schrieb Alexander Graf:
>> 
>> On 23.08.2013, at 06:20, Aneesh Kumar K.V wrote:
>> 
>>> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>>>
>>> When we translate the virtual address to physical check for error.
>>>
>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>> 
>> I think this change is sane, but I'd really prefer to see an ack from (or get this applied by) Luiz.
>> 
>> 
>> Alex
>> 
>>> ---
>>> cpus.c | 5 ++++-
>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/cpus.c b/cpus.c
>>> index 0f65e76..658366d 100644
>>> --- a/cpus.c
>>> +++ b/cpus.c
>>> @@ -1309,7 +1309,10 @@ void qmp_memsave(int64_t addr, int64_t size, const char *filename,
>>>       l = sizeof(buf);
>>>       if (l > size)
>>>           l = size;
>>> -        cpu_memory_rw_debug(cpu, addr, buf, l, 0);
>>> +        if (cpu_memory_rw_debug(cpu, addr, buf, l, 0) != 0) {
>>> +            error_set(errp, QERR_INVALID_PARAMETER, "addr");
>
> I've been repeatedly told error_set() should no longer be used, in favor
> of error_setg(). :)
>

Updated to 

commit 23aa279e11f54808dd9f0f87b3c85e6303d00d9c
Author: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
Date:   Tue Aug 20 16:14:23 2013 +0530

    target-ppc: Check for error on address translation in memsave command
    
    When we translate the virtual address to physical check for error.
    
    Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>

diff --git a/cpus.c b/cpus.c
index 0f65e76..51c38a0 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1309,7 +1309,10 @@ void qmp_memsave(int64_t addr, int64_t size, const char *filename,
         l = sizeof(buf);
         if (l > size)
             l = size;
-        cpu_memory_rw_debug(cpu, addr, buf, l, 0);
+        if (cpu_memory_rw_debug(cpu, addr, buf, l, 0) != 0) {
+            error_setg(errp, "Invalid addr 0x%016" PRIx64 "specified\n", addr);
+            goto exit;
+        }
         if (fwrite(buf, 1, l, f) != l) {
             error_set(errp, QERR_IO_ERROR);
             goto exit;

Re: [Qemu-devel] [PATCH -V3 3/4] target-ppc: Check for error on address translation in memsave command

[Andreas Färber]

Am 26.08.2013 14:20, schrieb Aneesh Kumar K.V:
> Andreas Färber <afaerber@suse.de> writes:
> 
>> Am 25.08.2013 20:32, schrieb Alexander Graf:
>>>
>>> On 23.08.2013, at 06:20, Aneesh Kumar K.V wrote:
>>>
>>>> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>>>>
>>>> When we translate the virtual address to physical check for error.
>>>>
>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>>>
>>> I think this change is sane, but I'd really prefer to see an ack from (or get this applied by) Luiz.
>>>
>>>
>>> Alex
>>>
>>>> ---
>>>> cpus.c | 5 ++++-
>>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/cpus.c b/cpus.c
>>>> index 0f65e76..658366d 100644
>>>> --- a/cpus.c
>>>> +++ b/cpus.c
>>>> @@ -1309,7 +1309,10 @@ void qmp_memsave(int64_t addr, int64_t size, const char *filename,
>>>>       l = sizeof(buf);
>>>>       if (l > size)
>>>>           l = size;
>>>> -        cpu_memory_rw_debug(cpu, addr, buf, l, 0);
>>>> +        if (cpu_memory_rw_debug(cpu, addr, buf, l, 0) != 0) {
>>>> +            error_set(errp, QERR_INVALID_PARAMETER, "addr");
>>
>> I've been repeatedly told error_set() should no longer be used, in favor
>> of error_setg(). :)
>>
> 
> Updated to 
> 
> commit 23aa279e11f54808dd9f0f87b3c85e6303d00d9c
> Author: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> Date:   Tue Aug 20 16:14:23 2013 +0530
> 
>     target-ppc: Check for error on address translation in memsave command
>     
>     When we translate the virtual address to physical check for error.
>     
>     Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> 
> diff --git a/cpus.c b/cpus.c
> index 0f65e76..51c38a0 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1309,7 +1309,10 @@ void qmp_memsave(int64_t addr, int64_t size, const char *filename,
>          l = sizeof(buf);
>          if (l > size)
>              l = size;
> -        cpu_memory_rw_debug(cpu, addr, buf, l, 0);
> +        if (cpu_memory_rw_debug(cpu, addr, buf, l, 0) != 0) {
> +            error_setg(errp, "Invalid addr 0x%016" PRIx64 "specified\n", addr);

Next trap is no \n please. ;)

Andreas

> +            goto exit;
> +        }
>          if (fwrite(buf, 1, l, f) != l) {
>              error_set(errp, QERR_IO_ERROR);
>              goto exit;
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH -V3 3/4] target-ppc: Check for error on address translation in memsave command

[Luiz Capitulino]

On Mon, 26 Aug 2013 14:22:25 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Am 26.08.2013 14:20, schrieb Aneesh Kumar K.V:
> > Andreas Färber <afaerber@suse.de> writes:
> > 
> >> Am 25.08.2013 20:32, schrieb Alexander Graf:
> >>>
> >>> On 23.08.2013, at 06:20, Aneesh Kumar K.V wrote:
> >>>
> >>>> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
> >>>>
> >>>> When we translate the virtual address to physical check for error.
> >>>>
> >>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> >>>
> >>> I think this change is sane, but I'd really prefer to see an ack from (or get this applied by) Luiz.
> >>>
> >>>
> >>> Alex
> >>>
> >>>> ---
> >>>> cpus.c | 5 ++++-
> >>>> 1 file changed, 4 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/cpus.c b/cpus.c
> >>>> index 0f65e76..658366d 100644
> >>>> --- a/cpus.c
> >>>> +++ b/cpus.c
> >>>> @@ -1309,7 +1309,10 @@ void qmp_memsave(int64_t addr, int64_t size, const char *filename,
> >>>>       l = sizeof(buf);
> >>>>       if (l > size)
> >>>>           l = size;
> >>>> -        cpu_memory_rw_debug(cpu, addr, buf, l, 0);
> >>>> +        if (cpu_memory_rw_debug(cpu, addr, buf, l, 0) != 0) {
> >>>> +            error_set(errp, QERR_INVALID_PARAMETER, "addr");
> >>
> >> I've been repeatedly told error_set() should no longer be used, in favor
> >> of error_setg(). :)
> >>
> > 
> > Updated to 
> > 
> > commit 23aa279e11f54808dd9f0f87b3c85e6303d00d9c
> > Author: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> > Date:   Tue Aug 20 16:14:23 2013 +0530
> > 
> >     target-ppc: Check for error on address translation in memsave command
> >     
> >     When we translate the virtual address to physical check for error.
> >     
> >     Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> > 
> > diff --git a/cpus.c b/cpus.c
> > index 0f65e76..51c38a0 100644
> > --- a/cpus.c
> > +++ b/cpus.c
> > @@ -1309,7 +1309,10 @@ void qmp_memsave(int64_t addr, int64_t size, const char *filename,
> >          l = sizeof(buf);
> >          if (l > size)
> >              l = size;
> > -        cpu_memory_rw_debug(cpu, addr, buf, l, 0);
> > +        if (cpu_memory_rw_debug(cpu, addr, buf, l, 0) != 0) {
> > +            error_setg(errp, "Invalid addr 0x%016" PRIx64 "specified\n", addr);
> 
> Next trap is no \n please. ;)

Otherwise looks good.