On 15/03/18 15:03, Andrew Jones wrote:
> On Wed, Mar 14, 2018 at 04:50:46PM +0000, Marc Zyngier wrote:
>> Until now, all EL2 executable mappings were derived from their
>> EL1 VA. Since we want to decouple the vectors mapping from
>> the rest of the hypervisor, we need to be able to map some
>> text somewhere else.
>>
>> The "idmap" region (for lack of a better name) is ideally suited
>> for this, as we have a huge range that hardly has anything in it.
>>
>> Let's extend the IO allocator to also deal with executable mappings,
>> thus providing the required feature.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
>> ---
>> arch/arm/include/asm/kvm_mmu.h | 2 +
>> arch/arm64/include/asm/kvm_mmu.h | 2 +
>> virt/kvm/arm/mmu.c | 80 +++++++++++++++++++++++++++++-----------
>> 3 files changed, 63 insertions(+), 21 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
>> index 26eb6b1cec9b..1b7f592eae57 100644
>> --- a/arch/arm/include/asm/kvm_mmu.h
>> +++ b/arch/arm/include/asm/kvm_mmu.h
>> @@ -56,6 +56,8 @@ int create_hyp_mappings(void *from, void *to, pgprot_t prot);
>> int create_hyp_io_mappings(phys_addr_t phys_addr, size_t size,
>> void __iomem **kaddr,
>> void __iomem **haddr);
>> +int create_hyp_exec_mappings(phys_addr_t phys_addr, size_t size,
>> + void **haddr);
>> void free_hyp_pgds(void);
>>
>> void stage2_unmap_vm(struct kvm *kvm);
>> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
>> index c2beb2d25170..97af11065bbd 100644
>> --- a/arch/arm64/include/asm/kvm_mmu.h
>> +++ b/arch/arm64/include/asm/kvm_mmu.h
>> @@ -151,6 +151,8 @@ int create_hyp_mappings(void *from, void *to, pgprot_t prot);
>> int create_hyp_io_mappings(phys_addr_t phys_addr, size_t size,
>> void __iomem **kaddr,
>> void __iomem **haddr);
>> +int create_hyp_exec_mappings(phys_addr_t phys_addr, size_t size,
>> + void **haddr);
>> void free_hyp_pgds(void);
>>
>> void stage2_unmap_vm(struct kvm *kvm);
>> diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
>> index b4d1948c8dd6..554ad5493e7d 100644
>> --- a/virt/kvm/arm/mmu.c
>> +++ b/virt/kvm/arm/mmu.c
>> @@ -737,30 +737,13 @@ int create_hyp_mappings(void *from, void *to, pgprot_t prot)
>> return 0;
>> }
>>
>> -/**
>> - * create_hyp_io_mappings - Map IO into both kernel and HYP
>> - * @phys_addr: The physical start address which gets mapped
>> - * @size: Size of the region being mapped
>> - * @kaddr: Kernel VA for this mapping
>> - * @haddr: HYP VA for this mapping
>> - */
>> -int create_hyp_io_mappings(phys_addr_t phys_addr, size_t size,
>> - void __iomem **kaddr,
>> - void __iomem **haddr)
>> +static int __create_hyp_private_mapping(phys_addr_t phys_addr, size_t size,
>> + unsigned long *haddr, pgprot_t prot)
>> {
>> pgd_t *pgd = hyp_pgd;
>> unsigned long base;
>> int ret;
>>
>> - *kaddr = ioremap(phys_addr, size);
>> - if (!*kaddr)
>> - return -ENOMEM;
>> -
>> - if (is_kernel_in_hyp_mode()) {
>> - *haddr = *kaddr;
>> - return 0;
>> - }
>> -
>> mutex_lock(&io_map_lock);
>>
>> /*
>> @@ -789,22 +772,77 @@ int create_hyp_io_mappings(phys_addr_t phys_addr, size_t size,
>>
>> ret = __create_hyp_mappings(pgd, __kvm_idmap_ptrs_per_pgd(),
>> base, base + size,
>> - __phys_to_pfn(phys_addr), PAGE_HYP_DEVICE);
>> + __phys_to_pfn(phys_addr), prot);
>> if (ret)
>> goto out;
>>
>> - *haddr = (void __iomem *)base + offset_in_page(phys_addr);
>> + *haddr = base + offset_in_page(phys_addr);
>> io_map_base = base;
>>
>> out:
>> mutex_unlock(&io_map_lock);
>>
>> + return ret;
>> +}
>> +
>> +/**
>> + * create_hyp_io_mappings - Map IO into both kernel and HYP
>> + * @phys_addr: The physical start address which gets mapped
>> + * @size: Size of the region being mapped
>> + * @kaddr: Kernel VA for this mapping
>> + * @haddr: HYP VA for this mapping
>> + */
>> +int create_hyp_io_mappings(phys_addr_t phys_addr, size_t size,
>> + void __iomem **kaddr,
>> + void __iomem **haddr)
>> +{
>> + unsigned long addr;
>> + int ret;
>> +
>> + *kaddr = ioremap(phys_addr, size);
>> + if (!*kaddr)
>> + return -ENOMEM;
>> +
>> + if (is_kernel_in_hyp_mode()) {
>> + *haddr = *kaddr;
>> + return 0;
>> + }
>> +
>> + ret = __create_hyp_private_mapping(phys_addr, size,
>> + &addr, PAGE_HYP_DEVICE);
>> if (ret) {
>> iounmap(*kaddr);
>> *kaddr = NULL;
>> + *haddr = NULL;
>> + return ret;
>> + }
>> +
>> + *haddr = (void __iomem *)addr;
>> + return 0;
>> +}
>> +
>> +/**
>> + * create_hyp_exec_mappings - Map an executable range into into HYP
>
> s/into into/into/
>
>> + * @phys_addr: The physical start address which gets mapped
>> + * @size: Size of the region being mapped
>> + * @haddr: HYP VA for this mapping
>> + */
>> +int create_hyp_exec_mappings(phys_addr_t phys_addr, size_t size,
>> + void **haddr)
>> +{
>> + unsigned long addr;
>> + int ret;
>> +
>> + BUG_ON(is_kernel_in_hyp_mode());
>
> Why BUG_ON instead of just giving the caller the same address, similar
> to what create_hyp_io_mappings() does?
Because we just don't have that information on VHE:
- create_hyp_io_mapping does an ioremap, and return the same address for
both
- create_hyp_exec_mapping doesn't have a mapping at all to return. It
just has a PA. We could convert it to a VA, but then which one? Linear
mapping, or kernel mapping?
The real problem is that this makes no sense on VHE, because we don't
have an alternative address space similar to !VHE. And if you call this
on VHE, you're doing something very wrong.
In all honesty, the real issue is create_hyp_io_mapping(), because it
gives you a false sense of security and hides the distinction between
VHE and !VHE.
>
>> +
>> + ret = __create_hyp_private_mapping(phys_addr, size,
>> + &addr, PAGE_HYP_EXEC);
>> + if (ret) {
>> + *haddr = NULL;
>> return ret;
>> }
>>
>> + *haddr = (void *)addr;
>> return 0;
>> }
>>
>> --
>> 2.14.2
>>
>
> Otherwise
>
> Reviewed-by: Andrew Jones <drjones@xxxxxxxxxx>
Thanks!
M.
--
Jazz is not dead. It just smells funny...
