[PATCH] arm/arm64: KVM: introduce new mapping API for percpu mappings

[PATCH] arm/arm64: KVM: introduce new mapping API for percpu mappings

[Marc Zyngier]

Using virt_to_phys on percpu mappings is horribly wrong (my own bad).
Thankfully, the kernel offers a way to obtain the physical address
of such a mapping.

Add a new create_hyp_percpu_mappings function to deal with those.

Reported-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Christoffer Dall <christoffer.dall@linaro.org>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
Santosh, can you please give this new patch a spin on your HW?

Thanks,

	M.

 arch/arm/include/asm/kvm_mmu.h   |  1 +
 arch/arm/kvm/arm.c               |  2 +-
 arch/arm/kvm/mmu.c               | 32 ++++++++++++++++++++++++++++++++
 arch/arm64/include/asm/kvm_mmu.h |  1 +
 4 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
index 9b28c41..6dcb9ff 100644
--- a/arch/arm/include/asm/kvm_mmu.h
+++ b/arch/arm/include/asm/kvm_mmu.h
@@ -43,6 +43,7 @@
 #include <asm/pgalloc.h>
 
 int create_hyp_mappings(void *from, void *to);
+int create_hyp_percpu_mappings(void *from, void *to);
 int create_hyp_io_mappings(void *from, void *to, phys_addr_t);
 void free_boot_hyp_pgd(void);
 void free_hyp_pgds(void);
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 9c697db..6191960 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -911,7 +911,7 @@ static int init_hyp_mode(void)
 		kvm_cpu_context_t *cpu_ctxt;
 
 		cpu_ctxt = per_cpu_ptr(kvm_host_cpu_state, cpu);
-		err = create_hyp_mappings(cpu_ctxt, cpu_ctxt + 1);
+		err = create_hyp_percpu_mappings(cpu_ctxt, cpu_ctxt + 1);
 
 		if (err) {
 			kvm_err("Cannot map host CPU state: %d\n", err);
diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index b0de86b..f2a552b 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -331,6 +331,38 @@ int create_hyp_mappings(void *from, void *to)
 }
 
 /**
+ * create_hyp_percpu_mappings - duplicate a percpu kernel virtual address
+ *				range in Hyp mode
+ * @from:	The virtual kernel start address of the range
+ * @to:		The virtual kernel end address of the range (exclusive)
+ *
+ * The same virtual address as the kernel virtual address is also used
+ * in Hyp-mode mapping (modulo HYP_PAGE_OFFSET) to the same underlying
+ * physical pages. It *has* to be a percpu region.
+ */
+int create_hyp_percpu_mappings(void *from, void *to)
+{
+	unsigned long phys_addr;
+	unsigned long virt_addr;
+	unsigned long start = KERN_TO_HYP((unsigned long)from);
+	unsigned long end = KERN_TO_HYP((unsigned long)to);
+
+	for (virt_addr = start; virt_addr < end; virt_addr += PAGE_SIZE) {
+		int err;
+
+		phys_addr = per_cpu_ptr_to_phys(from + virt_addr - start);
+		err = __create_hyp_mappings(hyp_pgd, virt_addr,
+					    virt_addr + PAGE_SIZE,
+					    __phys_to_pfn(phys_addr),
+					    PAGE_HYP);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+/**
  * 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)
diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
index efe609c..a97a92d 100644
--- a/arch/arm64/include/asm/kvm_mmu.h
+++ b/arch/arm64/include/asm/kvm_mmu.h
@@ -71,6 +71,7 @@
 #define S2_PGD_ORDER	get_order(PTRS_PER_S2_PGD * sizeof(pgd_t))
 
 int create_hyp_mappings(void *from, void *to);
+int create_hyp_percpu_mappings(void *from, void *to);
 int create_hyp_io_mappings(void *from, void *to, phys_addr_t);
 void free_boot_hyp_pgd(void);
 void free_hyp_pgds(void);
-- 
1.8.2.3

[PATCH] arm/arm64: KVM: introduce new mapping API for percpu mappings

[Santosh Shilimkar]

On Friday 15 November 2013 10:40 AM, Marc Zyngier wrote:
> Using virt_to_phys on percpu mappings is horribly wrong (my own bad).
> Thankfully, the kernel offers a way to obtain the physical address
> of such a mapping.
> 
> Add a new create_hyp_percpu_mappings function to deal with those.
> 
> Reported-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Christoffer Dall <christoffer.dall@linaro.org>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
> Santosh, can you please give this new patch a spin on your HW?
> 
Works except to make guest boot on my machine, phys_addr_t
update is needed as discussed ;-)


> 
>  arch/arm/include/asm/kvm_mmu.h   |  1 +
>  arch/arm/kvm/arm.c               |  2 +-
>  arch/arm/kvm/mmu.c               | 32 ++++++++++++++++++++++++++++++++
>  arch/arm64/include/asm/kvm_mmu.h |  1 +
>  4 files changed, 35 insertions(+), 1 deletion(-)
> 
[..]

> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index b0de86b..f2a552b 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -331,6 +331,38 @@ int create_hyp_mappings(void *from, void *to)
>  }
>  
>  /**
> + * create_hyp_percpu_mappings - duplicate a percpu kernel virtual address
> + *				range in Hyp mode
> + * @from:	The virtual kernel start address of the range
> + * @to:		The virtual kernel end address of the range (exclusive)
> + *
> + * The same virtual address as the kernel virtual address is also used
> + * in Hyp-mode mapping (modulo HYP_PAGE_OFFSET) to the same underlying
> + * physical pages. It *has* to be a percpu region.
> + */
> +int create_hyp_percpu_mappings(void *from, void *to)
> +{
> +	unsigned long phys_addr;
s/unsigned long/phys_addr_t

regards,
Santosh

[PATCH] arm/arm64: KVM: introduce new mapping API for percpu mappings

[Christoffer Dall]

On Fri, Nov 15, 2013 at 03:40:08PM +0000, Marc Zyngier wrote:
> Using virt_to_phys on percpu mappings is horribly wrong (my own bad).
> Thankfully, the kernel offers a way to obtain the physical address
> of such a mapping.
> 
> Add a new create_hyp_percpu_mappings function to deal with those.
> 
> Reported-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Christoffer Dall <christoffer.dall@linaro.org>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---


So, I find this nicer, somehow, what do you think:


diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index 3719583..dd531ba 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -334,6 +334,15 @@ out:
 	return err;
 }
 
+static phys_addr_t kvm_kaddr_to_phys(void *kaddr)
+{
+	if (!is_vmalloc_addr(kaddr))
+		return __pa(kaddr);
+	else
+		return page_to_phys(vmalloc_to_page(kaddr)) +
+		       offset_in_page(kaddr);
+}
+
 /**
  * create_hyp_mappings - duplicate a kernel virtual address range in Hyp mode
  * @from:	The virtual kernel start address of the range
@@ -345,16 +354,24 @@ out:
  */
 int create_hyp_mappings(void *from, void *to)
 {
-	unsigned long phys_addr = virt_to_phys(from);
+	phys_addr_t phys_addr;
+	unsigned long virt_addr;
 	unsigned long start = KERN_TO_HYP((unsigned long)from);
 	unsigned long end = KERN_TO_HYP((unsigned long)to);
 
-	/* Check for a valid kernel memory mapping */
-	if (!virt_addr_valid(from) || !virt_addr_valid(to - 1))
-		return -EINVAL;
+	for (virt_addr = start; virt_addr < end; virt_addr += PAGE_SIZE) {
+		int err;
 
-	return __create_hyp_mappings(hyp_pgd, start, end,
-				     __phys_to_pfn(phys_addr), PAGE_HYP);
+		phys_addr = kvm_kaddr_to_phys(from + virt_addr - start);
+		err = __create_hyp_mappings(hyp_pgd, virt_addr,
+					    virt_addr + PAGE_SIZE,
+					    __phys_to_pfn(phys_addr),
+					    PAGE_HYP);
+		if (err)
+			return err;
+	}
+
+	return 0;
 }
 
 /**

[PATCH] arm/arm64: KVM: introduce new mapping API for percpu mappings

[Marc Zyngier]

On 15/11/13 16:10, Christoffer Dall wrote:
> On Fri, Nov 15, 2013 at 03:40:08PM +0000, Marc Zyngier wrote:
>> Using virt_to_phys on percpu mappings is horribly wrong (my own bad).
>> Thankfully, the kernel offers a way to obtain the physical address
>> of such a mapping.
>>
>> Add a new create_hyp_percpu_mappings function to deal with those.
>>
>> Reported-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> Cc: Christoffer Dall <christoffer.dall@linaro.org>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
> 
> 
> So, I find this nicer, somehow, what do you think:
> 
> 
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index 3719583..dd531ba 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -334,6 +334,15 @@ out:
>  	return err;
>  }
>  
> +static phys_addr_t kvm_kaddr_to_phys(void *kaddr)
> +{
> +	if (!is_vmalloc_addr(kaddr))
> +		return __pa(kaddr);
> +	else
> +		return page_to_phys(vmalloc_to_page(kaddr)) +
> +		       offset_in_page(kaddr);
> +}
> +
>  /**
>   * create_hyp_mappings - duplicate a kernel virtual address range in Hyp mode
>   * @from:	The virtual kernel start address of the range
> @@ -345,16 +354,24 @@ out:
>   */
>  int create_hyp_mappings(void *from, void *to)
>  {
> -	unsigned long phys_addr = virt_to_phys(from);
> +	phys_addr_t phys_addr;
> +	unsigned long virt_addr;
>  	unsigned long start = KERN_TO_HYP((unsigned long)from);
>  	unsigned long end = KERN_TO_HYP((unsigned long)to);
>  
> -	/* Check for a valid kernel memory mapping */
> -	if (!virt_addr_valid(from) || !virt_addr_valid(to - 1))
> -		return -EINVAL;
> +	for (virt_addr = start; virt_addr < end; virt_addr += PAGE_SIZE) {
> +		int err;
>  
> -	return __create_hyp_mappings(hyp_pgd, start, end,
> -				     __phys_to_pfn(phys_addr), PAGE_HYP);
> +		phys_addr = kvm_kaddr_to_phys(from + virt_addr - start);
> +		err = __create_hyp_mappings(hyp_pgd, virt_addr,
> +					    virt_addr + PAGE_SIZE,

I think I've introduced a bug here. It probably should read:

	err = __create_hyp_mappings(hyp_pgd, virt_addr & PAGE_MASK,
				    (virt_addr + PAGE_SIZE) & PAGE_MASK,
				    [...]

> +					    __phys_to_pfn(phys_addr),
> +					    PAGE_HYP);
> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
>  }
>  
>  /**
> 

So that would work, but I'm slightly uncomfortable with what is
basically an open-coded version of per_cpu_ptr_to_phys, and I think
there is some value in having an explicit function for dealing with
percpu mappings, at least for educational purpose.

Also, we loose the virt_addr_valid() check, which has been a valuable
debugging tool for me in the past.

But maybe that's just me being a chicken... ;-)

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH] arm/arm64: KVM: introduce new mapping API for percpu mappings

[Christoffer Dall]

On Fri, Nov 15, 2013 at 04:33:07PM +0000, Marc Zyngier wrote:
> On 15/11/13 16:10, Christoffer Dall wrote:
> > On Fri, Nov 15, 2013 at 03:40:08PM +0000, Marc Zyngier wrote:
> >> Using virt_to_phys on percpu mappings is horribly wrong (my own bad).
> >> Thankfully, the kernel offers a way to obtain the physical address
> >> of such a mapping.
> >>
> >> Add a new create_hyp_percpu_mappings function to deal with those.
> >>
> >> Reported-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> >> Cc: Christoffer Dall <christoffer.dall@linaro.org>
> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> >> ---
> > 
> > 
> > So, I find this nicer, somehow, what do you think:
> > 
> > 
> > diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> > index 3719583..dd531ba 100644
> > --- a/arch/arm/kvm/mmu.c
> > +++ b/arch/arm/kvm/mmu.c
> > @@ -334,6 +334,15 @@ out:
> >  	return err;
> >  }
> >  
> > +static phys_addr_t kvm_kaddr_to_phys(void *kaddr)
> > +{
> > +	if (!is_vmalloc_addr(kaddr))
> > +		return __pa(kaddr);
> > +	else
> > +		return page_to_phys(vmalloc_to_page(kaddr)) +
> > +		       offset_in_page(kaddr);
> > +}
> > +
> >  /**
> >   * create_hyp_mappings - duplicate a kernel virtual address range in Hyp mode
> >   * @from:	The virtual kernel start address of the range
> > @@ -345,16 +354,24 @@ out:
> >   */
> >  int create_hyp_mappings(void *from, void *to)
> >  {
> > -	unsigned long phys_addr = virt_to_phys(from);
> > +	phys_addr_t phys_addr;
> > +	unsigned long virt_addr;
> >  	unsigned long start = KERN_TO_HYP((unsigned long)from);
> >  	unsigned long end = KERN_TO_HYP((unsigned long)to);
> >  
> > -	/* Check for a valid kernel memory mapping */
> > -	if (!virt_addr_valid(from) || !virt_addr_valid(to - 1))
> > -		return -EINVAL;
> > +	for (virt_addr = start; virt_addr < end; virt_addr += PAGE_SIZE) {
> > +		int err;
> >  
> > -	return __create_hyp_mappings(hyp_pgd, start, end,
> > -				     __phys_to_pfn(phys_addr), PAGE_HYP);
> > +		phys_addr = kvm_kaddr_to_phys(from + virt_addr - start);
> > +		err = __create_hyp_mappings(hyp_pgd, virt_addr,
> > +					    virt_addr + PAGE_SIZE,
> 
> I think I've introduced a bug here. It probably should read:
> 
> 	err = __create_hyp_mappings(hyp_pgd, virt_addr & PAGE_MASK,
> 				    (virt_addr + PAGE_SIZE) & PAGE_MASK,
> 				    [...]
> 
> > +					    __phys_to_pfn(phys_addr),
> > +					    PAGE_HYP);
> > +		if (err)
> > +			return err;
> > +	}
> > +
> > +	return 0;
> >  }
> >  
> >  /**
> > 
> 
> So that would work, but I'm slightly uncomfortable with what is
> basically an open-coded version of per_cpu_ptr_to_phys, and I think
> there is some value in having an explicit function for dealing with
> percpu mappings, at least for educational purpose.

I'm not concerned about the open-coded; this is a pretty fundamental
functionality to distinguish between a vmalloc and a kmalloc and do the
virt_to_phys translation accordingly - that's not specific to percpu
allocated memory - they only add more complexity as an optimization.
After all, this is a single if-statement that I'm sure we can master.

I think it's slightly more strange to have a call to map memory that
depends on how you allocated it, and would prefer having something that
just works, regardless.  But maybe that's utopian.  However, would we
end up potentially having a create_vmalloc_hyp_mappings as well then?

Another concern with your proposal is that it duplicates more code and
makes it a bit harder to track what's going on (who calls what when to
allocate something, etc.).

> 
> Also, we loose the virt_addr_valid() check, which has been a valuable
> debugging tool for me in the past.
> 
> But maybe that's just me being a chicken... ;-)
> 
Of course it is.  No, but really, virt_addr_valid just checks that it's
linearly mapped mapped memory.  So we're checking that it's not
highmeme, and assuming it's linearly mapped now.  We can add a
BUG_ON(!virt_addr_valid(x)) in the (!is_vmalloc_addr(kaddr)) case to
make sure nobody is passing module addresses or DMA memory or something
else crazy here, but I doubt that's a bug we need to concerne ourselves
about.

-Christoffer

[PATCH] arm/arm64: KVM: introduce new mapping API for percpu mappings

[Marc Zyngier]

On 15/11/13 16:43, Christoffer Dall wrote:
> On Fri, Nov 15, 2013 at 04:33:07PM +0000, Marc Zyngier wrote:
>> On 15/11/13 16:10, Christoffer Dall wrote:
>>> On Fri, Nov 15, 2013 at 03:40:08PM +0000, Marc Zyngier wrote:
>>>> Using virt_to_phys on percpu mappings is horribly wrong (my own bad).
>>>> Thankfully, the kernel offers a way to obtain the physical address
>>>> of such a mapping.
>>>>
>>>> Add a new create_hyp_percpu_mappings function to deal with those.
>>>>
>>>> Reported-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>>> Cc: Christoffer Dall <christoffer.dall@linaro.org>
>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>> ---
>>>
>>>
>>> So, I find this nicer, somehow, what do you think:
>>>
>>>
>>> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
>>> index 3719583..dd531ba 100644
>>> --- a/arch/arm/kvm/mmu.c
>>> +++ b/arch/arm/kvm/mmu.c
>>> @@ -334,6 +334,15 @@ out:
>>>  	return err;
>>>  }
>>>  
>>> +static phys_addr_t kvm_kaddr_to_phys(void *kaddr)
>>> +{
>>> +	if (!is_vmalloc_addr(kaddr))
>>> +		return __pa(kaddr);
>>> +	else
>>> +		return page_to_phys(vmalloc_to_page(kaddr)) +
>>> +		       offset_in_page(kaddr);
>>> +}
>>> +
>>>  /**
>>>   * create_hyp_mappings - duplicate a kernel virtual address range in Hyp mode
>>>   * @from:	The virtual kernel start address of the range
>>> @@ -345,16 +354,24 @@ out:
>>>   */
>>>  int create_hyp_mappings(void *from, void *to)
>>>  {
>>> -	unsigned long phys_addr = virt_to_phys(from);
>>> +	phys_addr_t phys_addr;
>>> +	unsigned long virt_addr;
>>>  	unsigned long start = KERN_TO_HYP((unsigned long)from);
>>>  	unsigned long end = KERN_TO_HYP((unsigned long)to);
>>>  
>>> -	/* Check for a valid kernel memory mapping */
>>> -	if (!virt_addr_valid(from) || !virt_addr_valid(to - 1))
>>> -		return -EINVAL;
>>> +	for (virt_addr = start; virt_addr < end; virt_addr += PAGE_SIZE) {
>>> +		int err;
>>>  
>>> -	return __create_hyp_mappings(hyp_pgd, start, end,
>>> -				     __phys_to_pfn(phys_addr), PAGE_HYP);
>>> +		phys_addr = kvm_kaddr_to_phys(from + virt_addr - start);
>>> +		err = __create_hyp_mappings(hyp_pgd, virt_addr,
>>> +					    virt_addr + PAGE_SIZE,
>>
>> I think I've introduced a bug here. It probably should read:
>>
>> 	err = __create_hyp_mappings(hyp_pgd, virt_addr & PAGE_MASK,
>> 				    (virt_addr + PAGE_SIZE) & PAGE_MASK,
>> 				    [...]
>>
>>> +					    __phys_to_pfn(phys_addr),
>>> +					    PAGE_HYP);
>>> +		if (err)
>>> +			return err;
>>> +	}
>>> +
>>> +	return 0;
>>>  }
>>>  
>>>  /**
>>>
>>
>> So that would work, but I'm slightly uncomfortable with what is
>> basically an open-coded version of per_cpu_ptr_to_phys, and I think
>> there is some value in having an explicit function for dealing with
>> percpu mappings, at least for educational purpose.
> 
> I'm not concerned about the open-coded; this is a pretty fundamental
> functionality to distinguish between a vmalloc and a kmalloc and do the
> virt_to_phys translation accordingly - that's not specific to percpu
> allocated memory - they only add more complexity as an optimization.
> After all, this is a single if-statement that I'm sure we can master.

Hey, I've been seen messing up that kind of code very easily! ;-)

> I think it's slightly more strange to have a call to map memory that
> depends on how you allocated it, and would prefer having something that
> just works, regardless.  But maybe that's utopian.  However, would we
> end up potentially having a create_vmalloc_hyp_mappings as well then?
> 
> Another concern with your proposal is that it duplicates more code and
> makes it a bit harder to track what's going on (who calls what when to
> allocate something, etc.).
> 
>>
>> Also, we loose the virt_addr_valid() check, which has been a valuable
>> debugging tool for me in the past.
>>
>> But maybe that's just me being a chicken... ;-)
>>
> Of course it is.  No, but really, virt_addr_valid just checks that it's
> linearly mapped mapped memory.  So we're checking that it's not
> highmeme, and assuming it's linearly mapped now.  We can add a
> BUG_ON(!virt_addr_valid(x)) in the (!is_vmalloc_addr(kaddr)) case to
> make sure nobody is passing module addresses or DMA memory or something
> else crazy here, but I doubt that's a bug we need to concerne ourselves
> about.

Fair enough. Do you write the patch, or do I update mine? Don't mind
either way.

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH] arm/arm64: KVM: introduce new mapping API for percpu mappings

[Christoffer Dall]

On Fri, Nov 15, 2013 at 04:59:53PM +0000, Marc Zyngier wrote:
> On 15/11/13 16:43, Christoffer Dall wrote:
> > On Fri, Nov 15, 2013 at 04:33:07PM +0000, Marc Zyngier wrote:
> >> On 15/11/13 16:10, Christoffer Dall wrote:
> >>> On Fri, Nov 15, 2013 at 03:40:08PM +0000, Marc Zyngier wrote:
> >>>> Using virt_to_phys on percpu mappings is horribly wrong (my own bad).
> >>>> Thankfully, the kernel offers a way to obtain the physical address
> >>>> of such a mapping.
> >>>>
> >>>> Add a new create_hyp_percpu_mappings function to deal with those.
> >>>>
> >>>> Reported-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> >>>> Cc: Christoffer Dall <christoffer.dall@linaro.org>
> >>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> >>>> ---
> >>>
> >>>
> >>> So, I find this nicer, somehow, what do you think:
> >>>
> >>>
> >>> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> >>> index 3719583..dd531ba 100644
> >>> --- a/arch/arm/kvm/mmu.c
> >>> +++ b/arch/arm/kvm/mmu.c
> >>> @@ -334,6 +334,15 @@ out:
> >>>  	return err;
> >>>  }
> >>>  
> >>> +static phys_addr_t kvm_kaddr_to_phys(void *kaddr)
> >>> +{
> >>> +	if (!is_vmalloc_addr(kaddr))
> >>> +		return __pa(kaddr);
> >>> +	else
> >>> +		return page_to_phys(vmalloc_to_page(kaddr)) +
> >>> +		       offset_in_page(kaddr);
> >>> +}
> >>> +
> >>>  /**
> >>>   * create_hyp_mappings - duplicate a kernel virtual address range in Hyp mode
> >>>   * @from:	The virtual kernel start address of the range
> >>> @@ -345,16 +354,24 @@ out:
> >>>   */
> >>>  int create_hyp_mappings(void *from, void *to)
> >>>  {
> >>> -	unsigned long phys_addr = virt_to_phys(from);
> >>> +	phys_addr_t phys_addr;
> >>> +	unsigned long virt_addr;
> >>>  	unsigned long start = KERN_TO_HYP((unsigned long)from);
> >>>  	unsigned long end = KERN_TO_HYP((unsigned long)to);
> >>>  
> >>> -	/* Check for a valid kernel memory mapping */
> >>> -	if (!virt_addr_valid(from) || !virt_addr_valid(to - 1))
> >>> -		return -EINVAL;
> >>> +	for (virt_addr = start; virt_addr < end; virt_addr += PAGE_SIZE) {
> >>> +		int err;
> >>>  
> >>> -	return __create_hyp_mappings(hyp_pgd, start, end,
> >>> -				     __phys_to_pfn(phys_addr), PAGE_HYP);
> >>> +		phys_addr = kvm_kaddr_to_phys(from + virt_addr - start);
> >>> +		err = __create_hyp_mappings(hyp_pgd, virt_addr,
> >>> +					    virt_addr + PAGE_SIZE,
> >>
> >> I think I've introduced a bug here. It probably should read:
> >>
> >> 	err = __create_hyp_mappings(hyp_pgd, virt_addr & PAGE_MASK,
> >> 				    (virt_addr + PAGE_SIZE) & PAGE_MASK,
> >> 				    [...]
> >>
> >>> +					    __phys_to_pfn(phys_addr),
> >>> +					    PAGE_HYP);
> >>> +		if (err)
> >>> +			return err;
> >>> +	}
> >>> +
> >>> +	return 0;
> >>>  }
> >>>  
> >>>  /**
> >>>
> >>
> >> So that would work, but I'm slightly uncomfortable with what is
> >> basically an open-coded version of per_cpu_ptr_to_phys, and I think
> >> there is some value in having an explicit function for dealing with
> >> percpu mappings, at least for educational purpose.
> > 
> > I'm not concerned about the open-coded; this is a pretty fundamental
> > functionality to distinguish between a vmalloc and a kmalloc and do the
> > virt_to_phys translation accordingly - that's not specific to percpu
> > allocated memory - they only add more complexity as an optimization.
> > After all, this is a single if-statement that I'm sure we can master.
> 
> Hey, I've been seen messing up that kind of code very easily! ;-)
> 
> > I think it's slightly more strange to have a call to map memory that
> > depends on how you allocated it, and would prefer having something that
> > just works, regardless.  But maybe that's utopian.  However, would we
> > end up potentially having a create_vmalloc_hyp_mappings as well then?
> > 
> > Another concern with your proposal is that it duplicates more code and
> > makes it a bit harder to track what's going on (who calls what when to
> > allocate something, etc.).
> > 
> >>
> >> Also, we loose the virt_addr_valid() check, which has been a valuable
> >> debugging tool for me in the past.
> >>
> >> But maybe that's just me being a chicken... ;-)
> >>
> > Of course it is.  No, but really, virt_addr_valid just checks that it's
> > linearly mapped mapped memory.  So we're checking that it's not
> > highmeme, and assuming it's linearly mapped now.  We can add a
> > BUG_ON(!virt_addr_valid(x)) in the (!is_vmalloc_addr(kaddr)) case to
> > make sure nobody is passing module addresses or DMA memory or something
> > else crazy here, but I doubt that's a bug we need to concerne ourselves
> > about.
> 
> Fair enough. Do you write the patch, or do I update mine? Don't mind
> either way.
> 
I'll write it and send it out.

Thanks,
-Christoffer