Re: [Qemu-ppc] [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 <address@hidden> writes:
> 
>> On 23.08.2013, at 06:20, Aneesh Kumar K.V wrote:
>> 
>>> From: "Aneesh Kumar K.V" <address@hidden>
>>> 
>>> 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 <address@hidden>
>>> ---
>>> 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