On Tue, May 29, 2012 at 12:11 PM, Marc Zyngier <marc.zyngier at arm.com> wrote:
> From: Christoffer Dall [c.dall at virtualopensystems.com]
>> On Tue, May 29, 2012 at 5:47 AM, Marc Zyngier <marc.zyngier at arm.com>
> wrote:
>>> Rework the HYP idmap code to be more consistent with what the kernel
>>> does:
>>>
>>> - Move the HYP pgd to the core code (so it can benefit to other
>>> ?hypervisors)
>>> - Populate this pgd with an identity mapping of the code contained
>>> ?in the .hyp_idmap.text section
>>> - Offer a method to drop the this identity mapping
>> the this?
>>> - Make all the above depend on CONFIG_ARM_VIRT_EXT
>>>
>>> This makes the core code more generic, and remove some of the
>>> clutter from the KVM init.
>> s/remove/removes/
>
> Thanks. Blame it on the HK jet-lag... ;-)
>
absolutely no problem.
>> [snip]
>>
>>> diff --git a/arch/arm/include/asm/pgtable.h
>>> b/arch/arm/include/asm/pgtable.h
>>> index fffc01f..9f549c6 100644
>>> --- a/arch/arm/include/asm/pgtable.h
>>> +++ b/arch/arm/include/asm/pgtable.h
>>> @@ -315,9 +315,10 @@ static inline pte_t pte_modify(pte_t pte, pgprot_t
>>> newprot)
>>>
>>> ?#define pgtable_cache_init() do { } while (0)
>>>
>>> -#ifdef CONFIG_KVM_ARM_HOST
>>> -void hyp_idmap_add(pgd_t *, unsigned long, unsigned long);
>>> -void hyp_idmap_del(pgd_t *pgd, unsigned long addr, unsigned long end);
>>> +#ifdef CONFIG_ARM_VIRT_EXT
>>> +extern pgd_t *hyp_pgd;
>>
>> hmm, I'm a bit worried about limiting the kernel to user only one
>> mapping in Hyp mode in general. What if there were several uses of
>> Hyp-mode on a given host system? Remapping the same virtual addresses
>> involves switching to Hyp-mode to flush TLBs etc. (iirc), so that
>> sounds almost impossible.
>
> Nothing prevents you from using a different set of page tables. The kernel
> provides you with one that contains the identity mapping, and KVM can use
> it. Or not. When it comes to sharing HYP mode, this patch doesn't really
> affect the current situation - it is hard and pretty much unsupported.
>
>> This approach actually seems less clean than the previous approach,
>> which was nicely contained to the use of KVM. Why would this be a
>> potential problem to merge?
>
> The main problem with this approach is that the kernel has moved away from
> it, and now uses a section-based identity mapping (users of the identity
> mapping don't have to populate the idmap page tables, as it is all done at
> build time). Bringing back the old mechanism feels like a regression, while
> this patch aligns the use pattern of the various sets of page tables. It
> also removes a nice chunk of KVM init code.
>
ok, sounds good. I will revisit the patch in its entirety once I'm
caught up with the rest then.
-Christoffer
