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

[Alexander Graf]

On 20.12.2013, at 06:24, Aneesh Kumar K.V <address@hidden> wrote:

> 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.
>     */

I was thinking that maybe the htab_shift and htab_mask names are just 
confusing. But I haven't managed to come up with really good names either.

Just put some comment on the variable definitions and let's hope that makes us 
remember what they were supposed to mean next time we have to dig through this 
code.


Alex