On Thu, 21 Feb 2013 10:29:05 -0500, Christoffer Dall
<cdall@xxxxxxxxxxxxxxx> wrote:
> On Thu, Feb 21, 2013 at 6:05 AM, Marc Zyngier <marc.zyngier@xxxxxxx>
wrote:
>> On Wed, 20 Feb 2013 15:37:49 -0800, Christoffer Dall
>> <cdall@xxxxxxxxxxxxxxx> wrote:
>>> On Wed, Feb 13, 2013 at 03:47:02PM +0000, Marc Zyngier wrote:
>>>> kvm_set_spte_handler is the last user of stage2_set_pte.
>>>> Convert it to the new get/set_at operations, and kill stage2_set_pte.
>>>>
>>>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
>>>> ---
>>>> arch/arm/kvm/mmu.c | 17 ++++-------------
>>>> 1 file changed, 4 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
>>>> index d033344..633b546 100644
>>>> --- a/arch/arm/kvm/mmu.c
>>>> +++ b/arch/arm/kvm/mmu.c
>>>> @@ -458,18 +458,6 @@ static void stage2_set_pte_at(struct kvm *kvm,
>>>> phys_addr_t addr,
>>>> get_page(virt_to_page(pte));
>>>> }
>>>>
>>>> -static int stage2_set_pte(struct kvm *kvm, struct
kvm_mmu_memory_cache
>>>> *cache,
>>>> - phys_addr_t addr, const pte_t *new_pte)
>>>> -{
>>>> - pte_t *pte;
>>>> -
>>>> - pte = stage2_get_pte(kvm, cache, addr);
>>>> - if (pte)
>>>> - stage2_set_pte_at(kvm, addr, pte, *new_pte);
>>>> -
>>>> - return 0;
>>>> -}
>>>> -
>>>
>>> Ah, you did this later on. I don't see why you want to keep the name
>>> stage2_set_pte_at though. Rationale?
>>
>> set_pte_at is the "normal" kernel interface (for an admittedly twisted
>> definition of "normal"). It really helps to have similar function names
>> (specially when you're trying to explain something to a kernel hacked
>> that
>> is not familiar with KVM's internal stuff).
>>
> I know that part, but set_pte_at also has the other twisted idea of
> setting random other bits.
>
> For consistency, why don't we call the whole thing stage2_get_pte_at
> and stage2_get_pmd_at, ...
Because these function are actually returning the "at", the location where
to write the entry.
To me, set_pte_at (setting aside the fact that it sets random bits by
calling set_pte_ext) makes sense because it clearly shows where we're
writing the PTE (the "at").
> Honestly it sounds to me like the _at postfix was added because
> someone wanted some other level of indirection and couldn't come up
> with a better naming standard. This function is different from the
> standard kernel function and calling it something with a connotation
> that indicates that it's largely the same, instead of using the more
> "natural" name, is just confusing to me.
>
> Maybe this is brain damage from the voodoo bug, but I'm no longer
> friends with set_pte_at :)
Oh well, I'm not going to battle over a function name as long as we agree
on what the code does. I'll rename it to stage2_set_pte in the next
version.
Cheers,
M.
--
Fast, cheap, reliable. Pick two.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
