Re: [Qemu-devel] [PATCH -V7 3/3] target-ppc: Fix page table lookup with kvm enabled

[Aneesh Kumar K.V]

Alexander Graf <address@hidden> writes:

> On 07.11.2013, at 15:31, Aneesh Kumar K.V <address@hidden> 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>
>> ---
>> Changes from V6:
>> * drop htab_fd argument and use global variable kvmppc_kern_htab instead
>> 
>> hw/ppc/spapr.c          |  1 +
>> hw/ppc/spapr_hcall.c    | 50 +++++++++++++++++++------------
>> target-ppc/kvm.c        | 53 +++++++++++++++++++++++++++++++++
>> target-ppc/kvm_ppc.h    | 19 ++++++++++++
>> target-ppc/mmu-hash64.c | 78 
>> ++++++++++++++++++++++++++++++++++++++++---------
>> target-ppc/mmu-hash64.h | 23 ++++++++++-----
>> 6 files changed, 183 insertions(+), 41 deletions(-)
>> 
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index d4f3502..8bf886e 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c

............

>> +    hpte += index * HASH_PTE_SIZE_64;
>> +
>>     ppc_hash64_store_hpte1(env, hpte, ptel);
>
> I'm not a big fan of fixing the read part, but leaving the write part
> broken. However I can see value in read only already, so I'm fine if
> the write part follows later.

When do we want store support per hpte entry w.r.t to hash page table 
maintained by the
hypervisor. I didn't find a use case while doing this, hence i left that
part out.

>
>>     /* eieio();  FIXME: need some sort of barrier for smp? */
>>     ppc_hash64_store_hpte0(env, hpte, pteh | HPTE64_V_HPTE_DIRTY);
>> 
>> -    args[0] = pte_index + i;
>> +    args[0] = pte_index + index;
>>     return H_SUCCESS;
>> }
>> 
.....

>> #ifndef CONFIG_KVM
>> diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
>> index 67fc1b5..f59d199 100644
>> --- a/target-ppc/mmu-hash64.c
>> +++ b/target-ppc/mmu-hash64.c
>> @@ -41,6 +41,11 @@
>> #endif
>> 
>> /*
>> + * Used to indicate whether we have allocated htab in the
>> + * host kernel
>> + */
>> +bool kvmppc_kern_htab;
>> +/*
>>  * SLB handling
>>  */
>> 
>> @@ -302,29 +307,76 @@ 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,
>> +void *ppc_hash64_start_access(PowerPCCPU *cpu, unsigned long pte_index)
>> +{
>> +    void *token = NULL;
>> +    hwaddr pte_offset;
>> +
>> +    pte_offset = pte_index * HASH_PTE_SIZE_64;
>> +    if (kvmppc_kern_htab) {
>> +        /*
>> +         * HTAB is controlled by KVM. Fetch the PTEG into a new buffer.
>> +         */
>> +        token = kvmppc_hash64_read_pteg(cpu, pte_index);
>> +        if (token) {
>> +            return token;
>> +        }
>> +        /*
>> +         * pteg read failed, even though we have allocated htab via
>> +         * kvmppc_reset_htab.
>> +         */
>> +        return NULL;
>> +    }
>> +    /*
>> +     * HTAB is controlled by QEMU. Just point to the internally
>> +     * accessible PTEG.
>> +     */
>> +    if (cpu->env.external_htab) {
>> +        token = cpu->env.external_htab + pte_offset;
>> +    } else if (cpu->env.htab_base) {
>> +        token = (uint8_t *) cpu->env.htab_base + pte_offset;
>
> This breaks if you run a 64-bit guest on a 32-bit host trying to
> access memory beyond 4GB. In that case htab_base is hwaddr (64bit)
> while uint8_t is only 32bit wide.

Wow!! didn't know that is possible.

>
> Just pass a 64bit token around. That makes it safe and easy.
>

I converted that to uint64_t. How about Patch 1 and 2. Are they ok to go
upstream ?

-aneesh