On 19.12.2013, at 07:55, Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>
wrote:
> Alexander Graf <ag...@suse.de> writes:
>
>> On 07.11.2013, at 15:31, Aneesh Kumar K.V <aneesh.ku...@linux.vnet.ibm.com>
>> wrote:
>>
>>> From: "Aneesh Kumar K.V" <aneesh.ku...@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.ku...@linux.vnet.ibm.com>
>>> ---
>>> 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
:).
Alex
