Hi Marc,
On Tue, May 29, 2012 at 03:47:42AM +0100, Marc Zyngier wrote:
> 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;
> +
> +void hyp_idmap_teardown(void);
> #endif
Maybe stick this with the idmap_pgd in asm/idmap.h?
> #endif /* !__ASSEMBLY__ */
> diff --git a/arch/arm/kernel/vmlinux.lds.S b/arch/arm/kernel/vmlinux.lds.S
> index 43a31fb..6bfd6ea 100644
> --- a/arch/arm/kernel/vmlinux.lds.S
> +++ b/arch/arm/kernel/vmlinux.lds.S
> @@ -21,6 +21,12 @@
> *(.idmap.text) \
> VMLINUX_SYMBOL(__idmap_text_end) = .;
>
> +#define HYP_IDMAP_TEXT \
> + . = ALIGN(PAGE_SIZE); \
Why does this need to be page aligned?
> + VMLINUX_SYMBOL(__hyp_idmap_text_start) = .; \
> + *(.hyp_idmap.text) \
.hyp.idmap.text
> + VMLINUX_SYMBOL(__hyp_idmap_text_end) = .;
This could probably just go on the end of IDMAP_TEXT.
> - /*
> * Execute the init code on each CPU.
> *
> * Note: The stack is not mapped yet, so don't do anything else than
> * initializing the hypervisor mode on each CPU using a local stack
> * space for temporary storage.
> */
> + init_phys_addr = virt_to_phys(__kvm_hyp_init);
> + BUG_ON(init_phys_addr & 0x1f);
I appreciate this was in the old code, but is this alignment check really
necessary? Why isn't __kvm_hyp_init suitably aligned at build-time?
> @@ -132,7 +130,38 @@ void hyp_idmap_del(pgd_t *pgd, unsigned long addr, unsigned long end)
> hyp_idmap_del_pmd(pgd, addr);
> } while (pgd++, addr = next, addr < end);
> }
> -EXPORT_SYMBOL_GPL(hyp_idmap_del);
> +
> +extern char __hyp_idmap_text_start[], __hyp_idmap_text_end[];
> +
> +void hyp_idmap_teardown(void)
> +{
> + phys_addr_t idmap_start, idmap_end;
> +
> + idmap_start = virt_to_phys((void *)__hyp_idmap_text_start);
> + idmap_end = virt_to_phys((void *)__hyp_idmap_text_end);
> + hyp_idmap_del(hyp_pgd, idmap_start, idmap_end);
Can you ensure that you don't have problems where physical addresses could
alias with virtual addresses from the non-identity mapping that you
assumedly switch to after this?
Maybe you're better off just leaving whatever is left of the idmap in place
after populating the pgd properly (since you don't do any freeing here
anyway).
> +static int __init hyp_init_static_idmap(void)
> +{
> + phys_addr_t idmap_start, idmap_end;
> +
> + hyp_pgd = kzalloc(PTRS_PER_PGD * sizeof(pgd_t), GFP_KERNEL);
> + if (!hyp_pgd)
> + return -ENOMEM;
> +
> + /* Add an identity mapping for the physical address of the section. */
> + idmap_start = virt_to_phys((void *)__hyp_idmap_text_start);
> + idmap_end = virt_to_phys((void *)__hyp_idmap_text_end);
> +
> + pr_info("Setting up static HYP identity map for 0x%llx - 0x%llx\n",
> + (long long)idmap_start, (long long)idmap_end);
> + identity_mapping_add(hyp_pgd, idmap_start, idmap_end, PMD_SECT_AP1);
We could probably clean up identity_mapping_add to take the labels as
parameters and do the printing itself.
Will
