[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCH -V4 2/4] target-ppc: Fix page table lookup with kv
From: |
Alexander Graf |
Subject: |
Re: [Qemu-ppc] [PATCH -V4 2/4] target-ppc: Fix page table lookup with kvm enabled |
Date: |
Mon, 7 Oct 2013 16:04:05 +0200 |
On 07.10.2013, at 15:58, Aneesh Kumar K.V <address@hidden> wrote:
> Alexander Graf <address@hidden> writes:
>
>> On 01.10.2013, at 03:27, Aneesh Kumar K.V wrote:
>>
>>> Alexander Graf <address@hidden> writes:
>>>
>>>> On 09/05/2013 10:16 AM, Aneesh Kumar K.V wrote:
>>>>> From: "Aneesh Kumar K.V"<address@hidden>
>>>>>
>
> ....
>
>>>
>>> Can you explain this better ?
>>
>> You're basically doing
>>
>> hwaddr ppc_hash64_pteg_search(...)
>> {
>> if (kvm) {
>> pteg = read_from_kvm();
>> foreach pte in pteg {
>> if (match) return offset;
>> }
>> return -1;
>> } else {
>> foreach pte in pteg {
>> pte = read_pte_from_memory();
>> if (match) return offset;
>> }
>> return -1;
>> }
>> }
>>
>> This is massive code duplication. The only difference between kvm and
>> tcg are the source for the pteg read. David already abstracted the
>> actual pte0/pte1 reads away in ppc_hash64_load_hpte0 and
>> ppc_hash64_load_hpte1 wrapper functions.
>>
>> Now imagine we could add a temporary pteg store in env. Then have something
>> like this in ppc_hash64_load_hpte0:
>>
>> if (need_kvm_htab_access) {
>> if (env->current_cached_pteg != this_pteg) (
>> read_pteg(env->cached_pteg);
>> return env->cached_pteg[x].pte0;
>> }
>> } else {
>> <do what was done before>
>> }
>>
>> That way the actual resolver doesn't care where the PTEG comes from,
>> as it only ever checks pte0/pte1 and leaves all the magic on where
>> those come from to the load function.
>
> I tried to do this and didn't like the end result. For one we
> unnecessarly bloat CPUPPCState struct to now carry a pteg information
> and associated array. ie, we need to have now the below in the CPUPPCState.
How about something like
token = ppc_hash64_start_access();
foreach (hpte entry) {
pte0 = ppc_hash64_load_hpte0(token, ...);
...
}
ppc_hash64_stop_access(token);
That way you could put the buffer and pteg_group into the token struct and only
allocate and use it when KVM with HV is in use.
>
> int pteg_group;
> unsigned long hpte[(HPTES_PER_GROUP * 2) + 1];
>
> Also out serach can be much effective with the current code,
We're anything but performance critical at this point.
>
> while (index < hpte_buf.header.n_valid) {
>
> against
>
> for (i = 0; i < HPTES_PER_GROUP; i++) {
>
> I guess the former is better when we can find invalid hpte entries.
>
> We now also need to update kvm_cpu_synchronize_state to clear
> pte_group so that we would not look at the stale values. If we ever want
> to use reading pteg in any other place we could possibly look at doing
> this. But at this stage, IMHO it unnecessarily make it all complex and
> less efficient.
The point is to make it less complex. I don't like the idea of having 2 hash
lookups in the same code base that do basically the same. And efficiency only
ever counts in the TCG case here.
Alex