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 19.12.2013, at 07:55, Aneesh Kumar K.V <address@hidden> wrote:
>
>> 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.
>
> Well, it's mostly a matter of consistency. The only case where it ever
> matters is if we wanted to emulate H_ENTER in QEMU - for tracing or
> debugging purposes. But if we leave it halfway implemented like this,
> you might get the impression it could work, but it doesn't.

>
>> 
>>> 
>>>>    /* 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.
>
> Well, with TCG you can run a 64-bit guest on a 32-bit host system, right?
>
>> 
>>> 
>>> 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 ?
>
> Yup. It took me a while to figure out that the patches are doing "the
> right thing". That code could really use some comments that explain
> what's going on - like the semantics of htab_mask (mask of the PTEG,
> will be shifted by 7 for physical address later), what htab_shift
> really is (log2 of the HTAB size), etc :).


How about

    /*
     * htab_mask is the mask used normalize hash value to PTEG index.
     * htab_shift is log2 of hash table size.
     * We have 8 hpte per group, and each hpte is 16 bytes.
     * ie have 128 bytes per hpte entry.
     */

 -aneesh