Re: [PATCH v5 08/23] KVM: arm/arm64: Move ioremap calls to create_hyp_io_mappings

[Suzuki K Poulose <Suzuki.Poulose@xxxxxxx>]

On 01/03/18 15:55, Marc Zyngier wrote:
> Both HYP io mappings call ioremap, followed by create_hyp_io_mappings.
> Let's move the ioremap call into create_hyp_io_mappings itself, which
> simplifies the code a bit and allows for further refactoring.
>
> Reviewed-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
> ---
>   arch/arm/include/asm/kvm_mmu.h   |  3 ++-
>   arch/arm64/include/asm/kvm_mmu.h |  3 ++-
>   virt/kvm/arm/mmu.c               | 24 ++++++++++++++----------
>   virt/kvm/arm/vgic/vgic-v2.c      | 31 ++++++++-----------------------
>   4 files changed, 26 insertions(+), 35 deletions(-)
>
> diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
> index a6808d2869f5..477c5ed426ef 100644
> --- a/arch/arm/include/asm/kvm_mmu.h
> +++ b/arch/arm/include/asm/kvm_mmu.h
> @@ -50,7 +50,8 @@
>   #include <asm/stage2_pgtable.h>
>   
>   int create_hyp_mappings(void *from, void *to, pgprot_t prot);
> -int create_hyp_io_mappings(void *from, void *to, phys_addr_t);
> +int create_hyp_io_mappings(phys_addr_t phys_addr, size_t size,
> +			   void __iomem **kaddr);
>   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 7120bf3f22c7..5885564f9863 100644
> --- a/arch/arm64/include/asm/kvm_mmu.h
> +++ b/arch/arm64/include/asm/kvm_mmu.h
> @@ -140,7 +140,8 @@ static inline unsigned long __kern_hyp_va(unsigned long v)
>   #include <asm/stage2_pgtable.h>
>   
>   int create_hyp_mappings(void *from, void *to, pgprot_t prot);
> -int create_hyp_io_mappings(void *from, void *to, phys_addr_t);
> +int create_hyp_io_mappings(phys_addr_t phys_addr, size_t size,
> +			   void __iomem **kaddr);
>   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 711473cd1097..d58388d8550e 100644
> --- a/virt/kvm/arm/mmu.c
> +++ b/virt/kvm/arm/mmu.c
> @@ -709,26 +709,30 @@ int create_hyp_mappings(void *from, void *to, pgprot_t prot)
>   }
>   
>   /**
> - * create_hyp_io_mappings - duplicate a kernel IO mapping into Hyp mode
> - * @from:	The kernel start VA of the range
> - * @to:		The kernel end VA of the range (exclusive)
> + * 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
>    *
>    * The resulting HYP VA is the same as the kernel VA, modulo
>    * HYP_PAGE_OFFSET.
>    */
> -int create_hyp_io_mappings(void *from, void *to, phys_addr_t phys_addr)
> +int create_hyp_io_mappings(phys_addr_t phys_addr, size_t size,
> +			   void __iomem **kaddr)
>   {
> -	unsigned long start = kern_hyp_va((unsigned long)from);
> -	unsigned long end = kern_hyp_va((unsigned long)to);
> +	unsigned long start, end;
>   
> -	if (is_kernel_in_hyp_mode())
> +	*kaddr = ioremap(phys_addr, size);
> +	if (!*kaddr)
> +		return -ENOMEM;
> +
> +	if (is_kernel_in_hyp_mode()) {
>   		return 0;
> +	}
>   
> -	/* Check for a valid kernel IO mapping */
> -	if (!is_vmalloc_addr(from) || !is_vmalloc_addr(to - 1))
> -		return -EINVAL;
>   
> +	start = kern_hyp_va((unsigned long)*kaddr);
> +	end = kern_hyp_va((unsigned long)*kaddr + size);
>   	return __create_hyp_mappings(hyp_pgd, PTRS_PER_PGD, start, end,
>   				     __phys_to_pfn(phys_addr), PAGE_HYP_DEVICE);

nit: So, we now ioremap() in create_hyp_io_mappings() and expect the caller to
iounmap() in case we ended up failing to create the hyp mappings. Do we need a
comment here ?

Cheers
Suzuki
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm