[PATCH 0/6] KVM: handling of MMIO pass-through regions

[PATCH 0/6] KVM: handling of MMIO pass-through regions

[Ard Biesheuvel]

This is a followup to the patches 'kvm: define PAGE_S2_DEVICE as
read-only by default' I sent out last week. Essentially build tested
only: while QEMU on KVM still works correctly for my use case with
these patches applied, it does not in fact use passthrough mappings
of device memory.

Patch #1 is a trivial fix for an issue identified by sparse where we
are calling memset(0) on 2 MB worth of pages.

Patch #2 fixes a potential NULL dereference in user_mem_abort()

Patch #3 adds a 'writable' parameter to kmv_phys_addr_ioremap() so that
read-only device regions can be mapped using this function. The existing
callers are updated to pass 'true' for this parameter.

Patch #4 and #5 were sent out before, and change the value of
PAGE_S2_DEVICE to read-only, so that read-only regions can be
ioremap()'ed

Patch #6 ensures that VM_PFNMAP linear mappings of non-system RAM host
memory are mapped eagerly rather than faulted in page by page.


Ard Biesheuvel (6):
  arm/arm64: KVM: use __GFP_ZERO not memset() to get zeroed pages
  arm/arm64: KVM: fix potential NULL dereference in user_mem_abort()
  arm/arm64: KVM: add 'writable' parameter to kvm_phys_addr_ioremap
  ARM: kvm: define PAGE_S2_DEVICE as read-only by default
  arm64: kvm: define PAGE_S2_DEVICE as read-only by default
  arm/arm64: KVM: map MMIO regions at creation time

 arch/arm/include/asm/pgtable.h   |  2 +-
 arch/arm/kvm/mmu.c               | 64 ++++++++++++++++++++++++++++++++++++----
 arch/arm64/include/asm/kvm_mmu.h |  2 +-
 arch/arm64/include/asm/pgtable.h |  2 +-
 virt/kvm/arm/vgic.c              |  3 +-
 5 files changed, 64 insertions(+), 9 deletions(-)

-- 
1.8.3.2

[PATCH 1/6] arm/arm64: KVM: use __GFP_ZERO not memset() to get zeroed pages

[Ard Biesheuvel]

Pass __GFP_ZERO to __get_free_pages() instead of calling memset()
explicitly.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm/kvm/mmu.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index c68ec28f17c3..152e0f896e63 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -528,11 +528,10 @@ int kvm_alloc_stage2_pgd(struct kvm *kvm)
 		return -EINVAL;
 	}
 
-	pgd = (pgd_t *)__get_free_pages(GFP_KERNEL, S2_PGD_ORDER);
+	pgd = (pgd_t *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, S2_PGD_ORDER);
 	if (!pgd)
 		return -ENOMEM;
 
-	memset(pgd, 0, PTRS_PER_S2_PGD * sizeof(pgd_t));
 	kvm_clean_pgd(pgd);
 	kvm->arch.pgd = pgd;
 
-- 
1.8.3.2

[PATCH 1/6] arm/arm64: KVM: use __GFP_ZERO not memset() to get zeroed pages

[Christoffer Dall]

On Wed, Sep 17, 2014 at 02:56:16PM -0700, Ard Biesheuvel wrote:
> Pass __GFP_ZERO to __get_free_pages() instead of calling memset()
> explicitly.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm/kvm/mmu.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index c68ec28f17c3..152e0f896e63 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -528,11 +528,10 @@ int kvm_alloc_stage2_pgd(struct kvm *kvm)
>  		return -EINVAL;
>  	}
>  
> -	pgd = (pgd_t *)__get_free_pages(GFP_KERNEL, S2_PGD_ORDER);
> +	pgd = (pgd_t *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, S2_PGD_ORDER);
>  	if (!pgd)
>  		return -ENOMEM;
>  
> -	memset(pgd, 0, PTRS_PER_S2_PGD * sizeof(pgd_t));
>  	kvm_clean_pgd(pgd);
>  	kvm->arch.pgd = pgd;
>  

So I think the point here was that if you use concatenated first-level
page tables, your MMU would only ever look in the first few entries of
the first-level page table, and we didn't want to zero-out more memory
than necessary.

However, there's something to be said for the fact that for sanity, we
should probably be clearing out the entire pgd anyhow.

Acked-by: Christoffer Dall <christoffer.dall@linaro.org>

Thanks,
-Christoffer

[PATCH 1/6] arm/arm64: KVM: use __GFP_ZERO not memset() to get zeroed pages

[Marc Zyngier]

On 17/09/14 22:56, Ard Biesheuvel wrote:
> Pass __GFP_ZERO to __get_free_pages() instead of calling memset()
> explicitly.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

	M.

> ---
>  arch/arm/kvm/mmu.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index c68ec28f17c3..152e0f896e63 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -528,11 +528,10 @@ int kvm_alloc_stage2_pgd(struct kvm *kvm)
>  		return -EINVAL;
>  	}
>  
> -	pgd = (pgd_t *)__get_free_pages(GFP_KERNEL, S2_PGD_ORDER);
> +	pgd = (pgd_t *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, S2_PGD_ORDER);
>  	if (!pgd)
>  		return -ENOMEM;
>  
> -	memset(pgd, 0, PTRS_PER_S2_PGD * sizeof(pgd_t));
>  	kvm_clean_pgd(pgd);
>  	kvm->arch.pgd = pgd;
>  
> 


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

[PATCH 2/6] arm/arm64: KVM: fix potential NULL dereference in user_mem_abort()

[Ard Biesheuvel]

Handle the potential NULL return value of find_vma_intersection()
before dereferencing it.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm/kvm/mmu.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index 152e0f896e63..c093e95ff7ef 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -776,7 +776,11 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 	/* Let's check if we will get back a huge page backed by hugetlbfs */
 	down_read(&current->mm->mmap_sem);
 	vma = find_vma_intersection(current->mm, hva, hva + 1);
-	if (is_vm_hugetlb_page(vma)) {
+	if (unlikely(!vma)) {
+		kvm_err("Failed to find VMA for hva 0x%lx\n", hva);
+		up_read(&current->mm->mmap_sem);
+		return -EFAULT;
+	} else if (is_vm_hugetlb_page(vma)) {
 		hugetlb = true;
 		gfn = (fault_ipa & PMD_MASK) >> PAGE_SHIFT;
 	} else {
-- 
1.8.3.2

[PATCH 2/6] arm/arm64: KVM: fix potential NULL dereference in user_mem_abort()

[Christoffer Dall]

On Wed, Sep 17, 2014 at 02:56:17PM -0700, Ard Biesheuvel wrote:
> Handle the potential NULL return value of find_vma_intersection()
> before dereferencing it.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm/kvm/mmu.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index 152e0f896e63..c093e95ff7ef 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -776,7 +776,11 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>  	/* Let's check if we will get back a huge page backed by hugetlbfs */
>  	down_read(&current->mm->mmap_sem);
>  	vma = find_vma_intersection(current->mm, hva, hva + 1);
> -	if (is_vm_hugetlb_page(vma)) {
> +	if (unlikely(!vma)) {
> +		kvm_err("Failed to find VMA for hva 0x%lx\n", hva);
> +		up_read(&current->mm->mmap_sem);
> +		return -EFAULT;
> +	} else if (is_vm_hugetlb_page(vma)) {
>  		hugetlb = true;
>  		gfn = (fault_ipa & PMD_MASK) >> PAGE_SHIFT;
>  	} else {
> -- 
> 1.8.3.2
> 
Nice catch!

(This would only happen if the guest unmaps an mmap region after
registering the memslot I believe, but still, we should absolutely merge
this.).

Thanks,
-Christoffer

[PATCH 2/6] arm/arm64: KVM: fix potential NULL dereference in user_mem_abort()

[Marc Zyngier]

On 17/09/14 22:56, Ard Biesheuvel wrote:
> Handle the potential NULL return value of find_vma_intersection()
> before dereferencing it.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm/kvm/mmu.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index 152e0f896e63..c093e95ff7ef 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -776,7 +776,11 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>  	/* Let's check if we will get back a huge page backed by hugetlbfs */
>  	down_read(&current->mm->mmap_sem);
>  	vma = find_vma_intersection(current->mm, hva, hva + 1);
> -	if (is_vm_hugetlb_page(vma)) {
> +	if (unlikely(!vma)) {
> +		kvm_err("Failed to find VMA for hva 0x%lx\n", hva);
> +		up_read(&current->mm->mmap_sem);
> +		return -EFAULT;
> +	} else if (is_vm_hugetlb_page(vma)) {

I'm not overly fond of this "else if" construct. The !vma case is final,
so what's after cannot be reached.

>  		hugetlb = true;
>  		gfn = (fault_ipa & PMD_MASK) >> PAGE_SHIFT;
>  	} else {
> 

Despite the above nit:

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

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

[PATCH 2/6] arm/arm64: KVM: fix potential NULL dereference in user_mem_abort()

[Ard Biesheuvel]

On 9 October 2014 15:05, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 17/09/14 22:56, Ard Biesheuvel wrote:
>> Handle the potential NULL return value of find_vma_intersection()
>> before dereferencing it.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  arch/arm/kvm/mmu.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
>> index 152e0f896e63..c093e95ff7ef 100644
>> --- a/arch/arm/kvm/mmu.c
>> +++ b/arch/arm/kvm/mmu.c
>> @@ -776,7 +776,11 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>>       /* Let's check if we will get back a huge page backed by hugetlbfs */
>>       down_read(&current->mm->mmap_sem);
>>       vma = find_vma_intersection(current->mm, hva, hva + 1);
>> -     if (is_vm_hugetlb_page(vma)) {
>> +     if (unlikely(!vma)) {
>> +             kvm_err("Failed to find VMA for hva 0x%lx\n", hva);
>> +             up_read(&current->mm->mmap_sem);
>> +             return -EFAULT;
>> +     } else if (is_vm_hugetlb_page(vma)) {
>
> I'm not overly fond of this "else if" construct. The !vma case is final,
> so what's after cannot be reached.
>

I don't have a strong preference either way. I can respin but perhaps
Christoffer can fix it up when applying?

>>               hugetlb = true;
>>               gfn = (fault_ipa & PMD_MASK) >> PAGE_SHIFT;
>>       } else {
>>
>
> Despite the above nit:
>
> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
>

Thanks.

[PATCH 2/6] arm/arm64: KVM: fix potential NULL dereference in user_mem_abort()

[Christoffer Dall]

On Thu, Oct 09, 2014 at 03:10:08PM +0200, Ard Biesheuvel wrote:
> On 9 October 2014 15:05, Marc Zyngier <marc.zyngier@arm.com> wrote:
> > On 17/09/14 22:56, Ard Biesheuvel wrote:
> >> Handle the potential NULL return value of find_vma_intersection()
> >> before dereferencing it.
> >>
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >>  arch/arm/kvm/mmu.c | 6 +++++-
> >>  1 file changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> >> index 152e0f896e63..c093e95ff7ef 100644
> >> --- a/arch/arm/kvm/mmu.c
> >> +++ b/arch/arm/kvm/mmu.c
> >> @@ -776,7 +776,11 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> >>       /* Let's check if we will get back a huge page backed by hugetlbfs */
> >>       down_read(&current->mm->mmap_sem);
> >>       vma = find_vma_intersection(current->mm, hva, hva + 1);
> >> -     if (is_vm_hugetlb_page(vma)) {
> >> +     if (unlikely(!vma)) {
> >> +             kvm_err("Failed to find VMA for hva 0x%lx\n", hva);
> >> +             up_read(&current->mm->mmap_sem);
> >> +             return -EFAULT;
> >> +     } else if (is_vm_hugetlb_page(vma)) {
> >
> > I'm not overly fond of this "else if" construct. The !vma case is final,
> > so what's after cannot be reached.
> >
> 
> I don't have a strong preference either way. I can respin but perhaps
> Christoffer can fix it up when applying?
> 
Sure!  Don't respin these just for that.

-Christoffer

[PATCH 2/6] arm/arm64: KVM: fix potential NULL dereference in user_mem_abort()

[Marc Zyngier]

On 09/10/14 14:11, Christoffer Dall wrote:
> On Thu, Oct 09, 2014 at 03:10:08PM +0200, Ard Biesheuvel wrote:
>> On 9 October 2014 15:05, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>> On 17/09/14 22:56, Ard Biesheuvel wrote:
>>>> Handle the potential NULL return value of find_vma_intersection()
>>>> before dereferencing it.
>>>>
>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> ---
>>>>  arch/arm/kvm/mmu.c | 6 +++++-
>>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
>>>> index 152e0f896e63..c093e95ff7ef 100644
>>>> --- a/arch/arm/kvm/mmu.c
>>>> +++ b/arch/arm/kvm/mmu.c
>>>> @@ -776,7 +776,11 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>>>>       /* Let's check if we will get back a huge page backed by hugetlbfs */
>>>>       down_read(&current->mm->mmap_sem);
>>>>       vma = find_vma_intersection(current->mm, hva, hva + 1);
>>>> -     if (is_vm_hugetlb_page(vma)) {
>>>> +     if (unlikely(!vma)) {
>>>> +             kvm_err("Failed to find VMA for hva 0x%lx\n", hva);
>>>> +             up_read(&current->mm->mmap_sem);
>>>> +             return -EFAULT;
>>>> +     } else if (is_vm_hugetlb_page(vma)) {
>>>
>>> I'm not overly fond of this "else if" construct. The !vma case is final,
>>> so what's after cannot be reached.
>>>
>>
>> I don't have a strong preference either way. I can respin but perhaps
>> Christoffer can fix it up when applying?
>>
> Sure!  Don't respin these just for that.

In which case, please add my

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

when you do the fixup.

Thanks,

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

[PATCH 3/6] arm/arm64: KVM: add 'writable' parameter to kvm_phys_addr_ioremap

[Ard Biesheuvel]

Add support for read-only MMIO passthrough mappings by adding a
'writable' parameter to kvm_phys_addr_ioremap. For the moment,
mappings will be read-write even if 'writable' is false, but once
the definition of PAGE_S2_DEVICE gets changed, those mappings will
be created read-only.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm/kvm/mmu.c               | 5 ++++-
 arch/arm64/include/asm/kvm_mmu.h | 2 +-
 virt/kvm/arm/vgic.c              | 3 ++-
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index c093e95ff7ef..fe53c3a30383 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -674,7 +674,7 @@ static int stage2_set_pte(struct kvm *kvm, struct kvm_mmu_memory_cache *cache,
  * @size:	The size of the mapping
  */
 int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
-			  phys_addr_t pa, unsigned long size)
+			  phys_addr_t pa, unsigned long size, bool writable)
 {
 	phys_addr_t addr, end;
 	int ret = 0;
@@ -687,6 +687,9 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
 	for (addr = guest_ipa; addr < end; addr += PAGE_SIZE) {
 		pte_t pte = pfn_pte(pfn, PAGE_S2_DEVICE);
 
+		if (writable)
+			kvm_set_s2pte_writable(&pte);
+
 		ret = mmu_topup_memory_cache(&cache, 2, 2);
 		if (ret)
 			goto out;
diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
index 737da742b293..7474e611bb2a 100644
--- a/arch/arm64/include/asm/kvm_mmu.h
+++ b/arch/arm64/include/asm/kvm_mmu.h
@@ -78,7 +78,7 @@ void free_hyp_pgds(void);
 int kvm_alloc_stage2_pgd(struct kvm *kvm);
 void kvm_free_stage2_pgd(struct kvm *kvm);
 int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
-			  phys_addr_t pa, unsigned long size);
+			  phys_addr_t pa, unsigned long size, bool writable);
 
 int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run);
 
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 73eba793b17f..c2bdbf4e25c2 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -1628,7 +1628,8 @@ int kvm_vgic_init(struct kvm *kvm)
 	}
 
 	ret = kvm_phys_addr_ioremap(kvm, kvm->arch.vgic.vgic_cpu_base,
-				    vgic->vcpu_base, KVM_VGIC_V2_CPU_SIZE);
+				    vgic->vcpu_base, KVM_VGIC_V2_CPU_SIZE,
+				    true);
 	if (ret) {
 		kvm_err("Unable to remap VGIC CPU to VCPU\n");
 		goto out;
-- 
1.8.3.2

[PATCH 3/6] arm/arm64: KVM: add 'writable' parameter to kvm_phys_addr_ioremap

[Mario Smarduch]

Is DEVICE type the correct default, for device
regions that are prefetchable? The guest might
want something less restrictive to optimize device
access.

On 09/17/2014 02:56 PM, Ard Biesheuvel wrote:
> Add support for read-only MMIO passthrough mappings by adding a
> 'writable' parameter to kvm_phys_addr_ioremap. For the moment,
> mappings will be read-write even if 'writable' is false, but once
> the definition of PAGE_S2_DEVICE gets changed, those mappings will
> be created read-only.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm/kvm/mmu.c               | 5 ++++-
>  arch/arm64/include/asm/kvm_mmu.h | 2 +-
>  virt/kvm/arm/vgic.c              | 3 ++-
>  3 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index c093e95ff7ef..fe53c3a30383 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -674,7 +674,7 @@ static int stage2_set_pte(struct kvm *kvm, struct kvm_mmu_memory_cache *cache,
>   * @size:	The size of the mapping
>   */
>  int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
> -			  phys_addr_t pa, unsigned long size)
> +			  phys_addr_t pa, unsigned long size, bool writable)
>  {
>  	phys_addr_t addr, end;
>  	int ret = 0;
> @@ -687,6 +687,9 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
>  	for (addr = guest_ipa; addr < end; addr += PAGE_SIZE) {
>  		pte_t pte = pfn_pte(pfn, PAGE_S2_DEVICE);
>  
> +		if (writable)
> +			kvm_set_s2pte_writable(&pte);
> +
>  		ret = mmu_topup_memory_cache(&cache, 2, 2);
>  		if (ret)
>  			goto out;
> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> index 737da742b293..7474e611bb2a 100644
> --- a/arch/arm64/include/asm/kvm_mmu.h
> +++ b/arch/arm64/include/asm/kvm_mmu.h
> @@ -78,7 +78,7 @@ void free_hyp_pgds(void);
>  int kvm_alloc_stage2_pgd(struct kvm *kvm);
>  void kvm_free_stage2_pgd(struct kvm *kvm);
>  int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
> -			  phys_addr_t pa, unsigned long size);
> +			  phys_addr_t pa, unsigned long size, bool writable);
>  
>  int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run);
>  
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 73eba793b17f..c2bdbf4e25c2 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1628,7 +1628,8 @@ int kvm_vgic_init(struct kvm *kvm)
>  	}
>  
>  	ret = kvm_phys_addr_ioremap(kvm, kvm->arch.vgic.vgic_cpu_base,
> -				    vgic->vcpu_base, KVM_VGIC_V2_CPU_SIZE);
> +				    vgic->vcpu_base, KVM_VGIC_V2_CPU_SIZE,
> +				    true);
>  	if (ret) {
>  		kvm_err("Unable to remap VGIC CPU to VCPU\n");
>  		goto out;
> 

[PATCH 3/6] arm/arm64: KVM: add 'writable' parameter to kvm_phys_addr_ioremap

[Christoffer Dall]

On Wed, Sep 24, 2014 at 05:03:18PM -0700, Mario Smarduch wrote:
> Is DEVICE type the correct default, for device
> regions that are prefetchable? The guest might
> want something less restrictive to optimize device
> access.
> 
We don't have a use case for this yet, and we aren't supporting generic
platform device passthrough yet.

So far this patch is solving a real problem, but if we need something
more flexible, then patches are welcome :)

-Christoffer

[PATCH 3/6] arm/arm64: KVM: add 'writable' parameter to kvm_phys_addr_ioremap

[Mario Smarduch]

On 09/29/2014 05:23 AM, Christoffer Dall wrote:
> On Wed, Sep 24, 2014 at 05:03:18PM -0700, Mario Smarduch wrote:
>> Is DEVICE type the correct default, for device
>> regions that are prefetchable? The guest might
>> want something less restrictive to optimize device
>> access.
>>
> We don't have a use case for this yet, and we aren't supporting generic
> platform device passthrough yet.
> 
> So far this patch is solving a real problem, but if we need something
> more flexible, then patches are welcome :)
> 
> -Christoffer
> 

I understand the fix addresses a real issue, but once it's in,
it may go unnoticed for quite a while.

I would recommend some warning or comment that guest has no
control over device memory attributes. In my opinion
setting 2nd stage attributes to cacheble (with exception of
GIC range) should work letting the guest driver manage
attributes.

- Mario

[PATCH 3/6] arm/arm64: KVM: add 'writable' parameter to kvm_phys_addr_ioremap

[Christoffer Dall]

On Mon, Sep 29, 2014 at 11:02:16AM -0700, Mario Smarduch wrote:
> On 09/29/2014 05:23 AM, Christoffer Dall wrote:
> > On Wed, Sep 24, 2014 at 05:03:18PM -0700, Mario Smarduch wrote:
> >> Is DEVICE type the correct default, for device
> >> regions that are prefetchable? The guest might
> >> want something less restrictive to optimize device
> >> access.
> >>
> > We don't have a use case for this yet, and we aren't supporting generic
> > platform device passthrough yet.
> > 
> > So far this patch is solving a real problem, but if we need something
> > more flexible, then patches are welcome :)
> > 
> > -Christoffer
> > 
> 
> I understand the fix addresses a real issue, but once it's in,
> it may go unnoticed for quite a while.
> 
> I would recommend some warning or comment that guest has no
> control over device memory attributes. In my opinion
> setting 2nd stage attributes to cacheble (with exception of
> GIC range) should work letting the guest driver manage
> attributes.
> 
Your case is only relevant for device-passthrough which we don't even
support yet, and we would have to somehow mandate that we only allow the
guest cacheable device memory regions for devices where it's safe to do
so.

As long as we don't have anything measurable or any infrastructure
supporting the case you are referring to, it's really not on my radar
just yet.

Plus, we are fixing current functionality and in no way precluding later
patches from optimizing something in the future, so as I said, patches
are welcome.

Thanks,
-Christoffer

[PATCH 3/6] arm/arm64: KVM: add 'writable' parameter to kvm_phys_addr_ioremap

[Christoffer Dall]

On Wed, Sep 17, 2014 at 02:56:18PM -0700, Ard Biesheuvel wrote:
> Add support for read-only MMIO passthrough mappings by adding a
> 'writable' parameter to kvm_phys_addr_ioremap. For the moment,
> mappings will be read-write even if 'writable' is false, but once
> the definition of PAGE_S2_DEVICE gets changed, those mappings will
> be created read-only.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm/kvm/mmu.c               | 5 ++++-
>  arch/arm64/include/asm/kvm_mmu.h | 2 +-
>  virt/kvm/arm/vgic.c              | 3 ++-
>  3 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index c093e95ff7ef..fe53c3a30383 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -674,7 +674,7 @@ static int stage2_set_pte(struct kvm *kvm, struct kvm_mmu_memory_cache *cache,
>   * @size:	The size of the mapping
>   */
>  int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
> -			  phys_addr_t pa, unsigned long size)
> +			  phys_addr_t pa, unsigned long size, bool writable)
>  {
>  	phys_addr_t addr, end;
>  	int ret = 0;
> @@ -687,6 +687,9 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
>  	for (addr = guest_ipa; addr < end; addr += PAGE_SIZE) {
>  		pte_t pte = pfn_pte(pfn, PAGE_S2_DEVICE);
>  
> +		if (writable)
> +			kvm_set_s2pte_writable(&pte);
> +
>  		ret = mmu_topup_memory_cache(&cache, 2, 2);
>  		if (ret)
>  			goto out;
> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> index 737da742b293..7474e611bb2a 100644
> --- a/arch/arm64/include/asm/kvm_mmu.h
> +++ b/arch/arm64/include/asm/kvm_mmu.h
> @@ -78,7 +78,7 @@ void free_hyp_pgds(void);
>  int kvm_alloc_stage2_pgd(struct kvm *kvm);
>  void kvm_free_stage2_pgd(struct kvm *kvm);
>  int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
> -			  phys_addr_t pa, unsigned long size);
> +			  phys_addr_t pa, unsigned long size, bool writable);
>  
>  int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run);
>  
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 73eba793b17f..c2bdbf4e25c2 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1628,7 +1628,8 @@ int kvm_vgic_init(struct kvm *kvm)
>  	}
>  
>  	ret = kvm_phys_addr_ioremap(kvm, kvm->arch.vgic.vgic_cpu_base,
> -				    vgic->vcpu_base, KVM_VGIC_V2_CPU_SIZE);
> +				    vgic->vcpu_base, KVM_VGIC_V2_CPU_SIZE,
> +				    true);
>  	if (ret) {
>  		kvm_err("Unable to remap VGIC CPU to VCPU\n");
>  		goto out;
> -- 
> 1.8.3.2
> 

You forgot the 32-bit prototype, but I can fix that up when applying it.

Thanks,
-Christoffer

[PATCH 3/6] arm/arm64: KVM: add 'writable' parameter to kvm_phys_addr_ioremap

[Marc Zyngier]

On 17/09/14 22:56, Ard Biesheuvel wrote:
> Add support for read-only MMIO passthrough mappings by adding a
> 'writable' parameter to kvm_phys_addr_ioremap. For the moment,
> mappings will be read-write even if 'writable' is false, but once
> the definition of PAGE_S2_DEVICE gets changed, those mappings will
> be created read-only.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm/kvm/mmu.c               | 5 ++++-
>  arch/arm64/include/asm/kvm_mmu.h | 2 +-
>  virt/kvm/arm/vgic.c              | 3 ++-
>  3 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index c093e95ff7ef..fe53c3a30383 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -674,7 +674,7 @@ static int stage2_set_pte(struct kvm *kvm, struct kvm_mmu_memory_cache *cache,
>   * @size:	The size of the mapping
>   */
>  int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
> -			  phys_addr_t pa, unsigned long size)
> +			  phys_addr_t pa, unsigned long size, bool writable)
>  {
>  	phys_addr_t addr, end;
>  	int ret = 0;
> @@ -687,6 +687,9 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
>  	for (addr = guest_ipa; addr < end; addr += PAGE_SIZE) {
>  		pte_t pte = pfn_pte(pfn, PAGE_S2_DEVICE);
>  
> +		if (writable)
> +			kvm_set_s2pte_writable(&pte);
> +
>  		ret = mmu_topup_memory_cache(&cache, 2, 2);
>  		if (ret)
>  			goto out;
> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> index 737da742b293..7474e611bb2a 100644
> --- a/arch/arm64/include/asm/kvm_mmu.h
> +++ b/arch/arm64/include/asm/kvm_mmu.h
> @@ -78,7 +78,7 @@ void free_hyp_pgds(void);
>  int kvm_alloc_stage2_pgd(struct kvm *kvm);
>  void kvm_free_stage2_pgd(struct kvm *kvm);
>  int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
> -			  phys_addr_t pa, unsigned long size);
> +			  phys_addr_t pa, unsigned long size, bool writable);

I'm afraid you missed the equivalent 32bit declaration.

>  
>  int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run);
>  
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 73eba793b17f..c2bdbf4e25c2 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1628,7 +1628,8 @@ int kvm_vgic_init(struct kvm *kvm)
>  	}
>  
>  	ret = kvm_phys_addr_ioremap(kvm, kvm->arch.vgic.vgic_cpu_base,
> -				    vgic->vcpu_base, KVM_VGIC_V2_CPU_SIZE);
> +				    vgic->vcpu_base, KVM_VGIC_V2_CPU_SIZE,
> +				    true);
>  	if (ret) {
>  		kvm_err("Unable to remap VGIC CPU to VCPU\n");
>  		goto out;
> 

Thanks,

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

[PATCH 3/6] arm/arm64: KVM: add 'writable' parameter to kvm_phys_addr_ioremap

[Ard Biesheuvel]

On 9 October 2014 15:07, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 17/09/14 22:56, Ard Biesheuvel wrote:
>> Add support for read-only MMIO passthrough mappings by adding a
>> 'writable' parameter to kvm_phys_addr_ioremap. For the moment,
>> mappings will be read-write even if 'writable' is false, but once
>> the definition of PAGE_S2_DEVICE gets changed, those mappings will
>> be created read-only.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  arch/arm/kvm/mmu.c               | 5 ++++-
>>  arch/arm64/include/asm/kvm_mmu.h | 2 +-
>>  virt/kvm/arm/vgic.c              | 3 ++-
>>  3 files changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
>> index c093e95ff7ef..fe53c3a30383 100644
>> --- a/arch/arm/kvm/mmu.c
>> +++ b/arch/arm/kvm/mmu.c
>> @@ -674,7 +674,7 @@ static int stage2_set_pte(struct kvm *kvm, struct kvm_mmu_memory_cache *cache,
>>   * @size:    The size of the mapping
>>   */
>>  int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
>> -                       phys_addr_t pa, unsigned long size)
>> +                       phys_addr_t pa, unsigned long size, bool writable)
>>  {
>>       phys_addr_t addr, end;
>>       int ret = 0;
>> @@ -687,6 +687,9 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
>>       for (addr = guest_ipa; addr < end; addr += PAGE_SIZE) {
>>               pte_t pte = pfn_pte(pfn, PAGE_S2_DEVICE);
>>
>> +             if (writable)
>> +                     kvm_set_s2pte_writable(&pte);
>> +
>>               ret = mmu_topup_memory_cache(&cache, 2, 2);
>>               if (ret)
>>                       goto out;
>> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
>> index 737da742b293..7474e611bb2a 100644
>> --- a/arch/arm64/include/asm/kvm_mmu.h
>> +++ b/arch/arm64/include/asm/kvm_mmu.h
>> @@ -78,7 +78,7 @@ void free_hyp_pgds(void);
>>  int kvm_alloc_stage2_pgd(struct kvm *kvm);
>>  void kvm_free_stage2_pgd(struct kvm *kvm);
>>  int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
>> -                       phys_addr_t pa, unsigned long size);
>> +                       phys_addr_t pa, unsigned long size, bool writable);
>
> I'm afraid you missed the equivalent 32bit declaration.
>

Christoffer had spotted that as well, and indicated he would not mind
fixing it up when applying.

>>
>>  int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run);
>>
>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>> index 73eba793b17f..c2bdbf4e25c2 100644
>> --- a/virt/kvm/arm/vgic.c
>> +++ b/virt/kvm/arm/vgic.c
>> @@ -1628,7 +1628,8 @@ int kvm_vgic_init(struct kvm *kvm)
>>       }
>>
>>       ret = kvm_phys_addr_ioremap(kvm, kvm->arch.vgic.vgic_cpu_base,
>> -                                 vgic->vcpu_base, KVM_VGIC_V2_CPU_SIZE);
>> +                                 vgic->vcpu_base, KVM_VGIC_V2_CPU_SIZE,
>> +                                 true);
>>       if (ret) {
>>               kvm_err("Unable to remap VGIC CPU to VCPU\n");
>>               goto out;
>>
>
> Thanks,
>
>         M.
> --
> Jazz is not dead. It just smells funny...
>

[PATCH 3/6] arm/arm64: KVM: add 'writable' parameter to kvm_phys_addr_ioremap

[Marc Zyngier]

On 09/10/14 14:11, Ard Biesheuvel wrote:
> On 9 October 2014 15:07, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> On 17/09/14 22:56, Ard Biesheuvel wrote:
>>> Add support for read-only MMIO passthrough mappings by adding a
>>> 'writable' parameter to kvm_phys_addr_ioremap. For the moment,
>>> mappings will be read-write even if 'writable' is false, but once
>>> the definition of PAGE_S2_DEVICE gets changed, those mappings will
>>> be created read-only.
>>>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>  arch/arm/kvm/mmu.c               | 5 ++++-
>>>  arch/arm64/include/asm/kvm_mmu.h | 2 +-
>>>  virt/kvm/arm/vgic.c              | 3 ++-
>>>  3 files changed, 7 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
>>> index c093e95ff7ef..fe53c3a30383 100644
>>> --- a/arch/arm/kvm/mmu.c
>>> +++ b/arch/arm/kvm/mmu.c
>>> @@ -674,7 +674,7 @@ static int stage2_set_pte(struct kvm *kvm, struct kvm_mmu_memory_cache *cache,
>>>   * @size:    The size of the mapping
>>>   */
>>>  int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
>>> -                       phys_addr_t pa, unsigned long size)
>>> +                       phys_addr_t pa, unsigned long size, bool writable)
>>>  {
>>>       phys_addr_t addr, end;
>>>       int ret = 0;
>>> @@ -687,6 +687,9 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
>>>       for (addr = guest_ipa; addr < end; addr += PAGE_SIZE) {
>>>               pte_t pte = pfn_pte(pfn, PAGE_S2_DEVICE);
>>>
>>> +             if (writable)
>>> +                     kvm_set_s2pte_writable(&pte);
>>> +
>>>               ret = mmu_topup_memory_cache(&cache, 2, 2);
>>>               if (ret)
>>>                       goto out;
>>> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
>>> index 737da742b293..7474e611bb2a 100644
>>> --- a/arch/arm64/include/asm/kvm_mmu.h
>>> +++ b/arch/arm64/include/asm/kvm_mmu.h
>>> @@ -78,7 +78,7 @@ void free_hyp_pgds(void);
>>>  int kvm_alloc_stage2_pgd(struct kvm *kvm);
>>>  void kvm_free_stage2_pgd(struct kvm *kvm);
>>>  int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
>>> -                       phys_addr_t pa, unsigned long size);
>>> +                       phys_addr_t pa, unsigned long size, bool writable);
>>
>> I'm afraid you missed the equivalent 32bit declaration.
>>
> 
> Christoffer had spotted that as well, and indicated he would not mind
> fixing it up when applying.

Ah, missed that. In which case:

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

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

[PATCH 4/6] ARM: kvm: define PAGE_S2_DEVICE as read-only by default

[Ard Biesheuvel]

Now that we support read-only memslots, we need to make sure that
pass-through device mappings are not mapped writable if the guest
has requested them to be read-only. The existing implementation
already honours this by calling kvm_set_s2pte_writable() on the new
pte in case of writable mappings, so all we need to do is define
the default pgprot_t value used for devices to be PTE_S2_RDONLY.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm/include/asm/pgtable.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
index 01baef07cd0c..92b2fbe18868 100644
--- a/arch/arm/include/asm/pgtable.h
+++ b/arch/arm/include/asm/pgtable.h
@@ -100,7 +100,7 @@ extern pgprot_t		pgprot_s2_device;
 #define PAGE_HYP		_MOD_PROT(pgprot_kernel, L_PTE_HYP)
 #define PAGE_HYP_DEVICE		_MOD_PROT(pgprot_hyp_device, L_PTE_HYP)
 #define PAGE_S2			_MOD_PROT(pgprot_s2, L_PTE_S2_RDONLY)
-#define PAGE_S2_DEVICE		_MOD_PROT(pgprot_s2_device, L_PTE_S2_RDWR)
+#define PAGE_S2_DEVICE		_MOD_PROT(pgprot_s2_device, L_PTE_S2_RDONLY)
 
 #define __PAGE_NONE		__pgprot(_L_PTE_DEFAULT | L_PTE_RDONLY | L_PTE_XN | L_PTE_NONE)
 #define __PAGE_SHARED		__pgprot(_L_PTE_DEFAULT | L_PTE_USER | L_PTE_XN)
-- 
1.8.3.2

[PATCH 4/6] ARM: kvm: define PAGE_S2_DEVICE as read-only by default

[Christoffer Dall]

On Wed, Sep 17, 2014 at 02:56:19PM -0700, Ard Biesheuvel wrote:
> Now that we support read-only memslots, we need to make sure that
> pass-through device mappings are not mapped writable if the guest
> has requested them to be read-only. The existing implementation
> already honours this by calling kvm_set_s2pte_writable() on the new
> pte in case of writable mappings, so all we need to do is define
> the default pgprot_t value used for devices to be PTE_S2_RDONLY.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm/include/asm/pgtable.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
> index 01baef07cd0c..92b2fbe18868 100644
> --- a/arch/arm/include/asm/pgtable.h
> +++ b/arch/arm/include/asm/pgtable.h
> @@ -100,7 +100,7 @@ extern pgprot_t		pgprot_s2_device;
>  #define PAGE_HYP		_MOD_PROT(pgprot_kernel, L_PTE_HYP)
>  #define PAGE_HYP_DEVICE		_MOD_PROT(pgprot_hyp_device, L_PTE_HYP)
>  #define PAGE_S2			_MOD_PROT(pgprot_s2, L_PTE_S2_RDONLY)
> -#define PAGE_S2_DEVICE		_MOD_PROT(pgprot_s2_device, L_PTE_S2_RDWR)
> +#define PAGE_S2_DEVICE		_MOD_PROT(pgprot_s2_device, L_PTE_S2_RDONLY)
>  
>  #define __PAGE_NONE		__pgprot(_L_PTE_DEFAULT | L_PTE_RDONLY | L_PTE_XN | L_PTE_NONE)
>  #define __PAGE_SHARED		__pgprot(_L_PTE_DEFAULT | L_PTE_USER | L_PTE_XN)
> -- 
> 1.8.3.2
> 
Acked-by: Christoffer Dall <christoffer.dall@linaro.org>

Russell, unless you object to this one, I will just queue it through the
kvmarm tree?

Thanks,
-Christoffer

[PATCH 4/6] ARM: kvm: define PAGE_S2_DEVICE as read-only by default

[Marc Zyngier]

On 17/09/14 22:56, Ard Biesheuvel wrote:
> Now that we support read-only memslots, we need to make sure that
> pass-through device mappings are not mapped writable if the guest
> has requested them to be read-only. The existing implementation
> already honours this by calling kvm_set_s2pte_writable() on the new
> pte in case of writable mappings, so all we need to do is define
> the default pgprot_t value used for devices to be PTE_S2_RDONLY.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

	M.

> ---
>  arch/arm/include/asm/pgtable.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
> index 01baef07cd0c..92b2fbe18868 100644
> --- a/arch/arm/include/asm/pgtable.h
> +++ b/arch/arm/include/asm/pgtable.h
> @@ -100,7 +100,7 @@ extern pgprot_t		pgprot_s2_device;
>  #define PAGE_HYP		_MOD_PROT(pgprot_kernel, L_PTE_HYP)
>  #define PAGE_HYP_DEVICE		_MOD_PROT(pgprot_hyp_device, L_PTE_HYP)
>  #define PAGE_S2			_MOD_PROT(pgprot_s2, L_PTE_S2_RDONLY)
> -#define PAGE_S2_DEVICE		_MOD_PROT(pgprot_s2_device, L_PTE_S2_RDWR)
> +#define PAGE_S2_DEVICE		_MOD_PROT(pgprot_s2_device, L_PTE_S2_RDONLY)
>  
>  #define __PAGE_NONE		__pgprot(_L_PTE_DEFAULT | L_PTE_RDONLY | L_PTE_XN | L_PTE_NONE)
>  #define __PAGE_SHARED		__pgprot(_L_PTE_DEFAULT | L_PTE_USER | L_PTE_XN)
> 


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

[PATCH 5/6] arm64: kvm: define PAGE_S2_DEVICE as read-only by default

[Ard Biesheuvel]

Now that we support read-only memslots, we need to make sure that
pass-through device mappings are not mapped writable if the guest
has requested them to be read-only. The existing implementation
already honours this by calling kvm_set_s2pte_writable() on the new
pte in case of writable mappings, so all we need to do is define
the default pgprot_t value used for devices to be PTE_S2_RDONLY.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/include/asm/pgtable.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
index ffe1ba0506d1..51f6f5284ce5 100644
--- a/arch/arm64/include/asm/pgtable.h
+++ b/arch/arm64/include/asm/pgtable.h
@@ -79,7 +79,7 @@ extern void __pgd_error(const char *file, int line, unsigned long val);
 #define PAGE_HYP_DEVICE		__pgprot(PROT_DEVICE_nGnRE | PTE_HYP)
 
 #define PAGE_S2			__pgprot(PROT_DEFAULT | PTE_S2_MEMATTR(MT_S2_NORMAL) | PTE_S2_RDONLY)
-#define PAGE_S2_DEVICE		__pgprot(PROT_DEFAULT | PTE_S2_MEMATTR(MT_S2_DEVICE_nGnRE) | PTE_S2_RDWR | PTE_UXN)
+#define PAGE_S2_DEVICE		__pgprot(PROT_DEFAULT | PTE_S2_MEMATTR(MT_S2_DEVICE_nGnRE) | PTE_S2_RDONLY | PTE_UXN)
 
 #define PAGE_NONE		__pgprot(((_PAGE_DEFAULT) & ~PTE_TYPE_MASK) | PTE_PROT_NONE | PTE_PXN | PTE_UXN)
 #define PAGE_SHARED		__pgprot(_PAGE_DEFAULT | PTE_USER | PTE_NG | PTE_PXN | PTE_UXN | PTE_WRITE)
-- 
1.8.3.2

[PATCH 5/6] arm64: kvm: define PAGE_S2_DEVICE as read-only by default

[Christoffer Dall]

On Wed, Sep 17, 2014 at 02:56:20PM -0700, Ard Biesheuvel wrote:
> Now that we support read-only memslots, we need to make sure that
> pass-through device mappings are not mapped writable if the guest
> has requested them to be read-only. The existing implementation
> already honours this by calling kvm_set_s2pte_writable() on the new
> pte in case of writable mappings, so all we need to do is define
> the default pgprot_t value used for devices to be PTE_S2_RDONLY.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/include/asm/pgtable.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> index ffe1ba0506d1..51f6f5284ce5 100644
> --- a/arch/arm64/include/asm/pgtable.h
> +++ b/arch/arm64/include/asm/pgtable.h
> @@ -79,7 +79,7 @@ extern void __pgd_error(const char *file, int line, unsigned long val);
>  #define PAGE_HYP_DEVICE		__pgprot(PROT_DEVICE_nGnRE | PTE_HYP)
>  
>  #define PAGE_S2			__pgprot(PROT_DEFAULT | PTE_S2_MEMATTR(MT_S2_NORMAL) | PTE_S2_RDONLY)
> -#define PAGE_S2_DEVICE		__pgprot(PROT_DEFAULT | PTE_S2_MEMATTR(MT_S2_DEVICE_nGnRE) | PTE_S2_RDWR | PTE_UXN)
> +#define PAGE_S2_DEVICE		__pgprot(PROT_DEFAULT | PTE_S2_MEMATTR(MT_S2_DEVICE_nGnRE) | PTE_S2_RDONLY | PTE_UXN)
>  
>  #define PAGE_NONE		__pgprot(((_PAGE_DEFAULT) & ~PTE_TYPE_MASK) | PTE_PROT_NONE | PTE_PXN | PTE_UXN)
>  #define PAGE_SHARED		__pgprot(_PAGE_DEFAULT | PTE_USER | PTE_NG | PTE_PXN | PTE_UXN | PTE_WRITE)
> -- 
> 1.8.3.2
> 
Acked-by: Christoffer Dall <christoffer.dall@linaro.org>

Catalin, Will,

Are you fine with me queueing this through the kvmarm tree?

Thanks,
-Christoffer

[PATCH 5/6] arm64: kvm: define PAGE_S2_DEVICE as read-only by default

[Will Deacon]

On Mon, Sep 29, 2014 at 02:06:17PM +0100, Christoffer Dall wrote:
> On Wed, Sep 17, 2014 at 02:56:20PM -0700, Ard Biesheuvel wrote:
> > Now that we support read-only memslots, we need to make sure that
> > pass-through device mappings are not mapped writable if the guest
> > has requested them to be read-only. The existing implementation
> > already honours this by calling kvm_set_s2pte_writable() on the new
> > pte in case of writable mappings, so all we need to do is define
> > the default pgprot_t value used for devices to be PTE_S2_RDONLY.
> > 
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  arch/arm64/include/asm/pgtable.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> > index ffe1ba0506d1..51f6f5284ce5 100644
> > --- a/arch/arm64/include/asm/pgtable.h
> > +++ b/arch/arm64/include/asm/pgtable.h
> > @@ -79,7 +79,7 @@ extern void __pgd_error(const char *file, int line, unsigned long val);
> >  #define PAGE_HYP_DEVICE		__pgprot(PROT_DEVICE_nGnRE | PTE_HYP)
> >  
> >  #define PAGE_S2			__pgprot(PROT_DEFAULT | PTE_S2_MEMATTR(MT_S2_NORMAL) | PTE_S2_RDONLY)
> > -#define PAGE_S2_DEVICE		__pgprot(PROT_DEFAULT | PTE_S2_MEMATTR(MT_S2_DEVICE_nGnRE) | PTE_S2_RDWR | PTE_UXN)
> > +#define PAGE_S2_DEVICE		__pgprot(PROT_DEFAULT | PTE_S2_MEMATTR(MT_S2_DEVICE_nGnRE) | PTE_S2_RDONLY | PTE_UXN)
> >  
> >  #define PAGE_NONE		__pgprot(((_PAGE_DEFAULT) & ~PTE_TYPE_MASK) | PTE_PROT_NONE | PTE_PXN | PTE_UXN)
> >  #define PAGE_SHARED		__pgprot(_PAGE_DEFAULT | PTE_USER | PTE_NG | PTE_PXN | PTE_UXN | PTE_WRITE)
> > -- 
> > 1.8.3.2
> > 
> Acked-by: Christoffer Dall <christoffer.dall@linaro.org>
> 
> Catalin, Will,
> 
> Are you fine with me queueing this through the kvmarm tree?

Fine by me.

Will

[PATCH 5/6] arm64: kvm: define PAGE_S2_DEVICE as read-only by default

[Marc Zyngier]

On 17/09/14 22:56, Ard Biesheuvel wrote:
> Now that we support read-only memslots, we need to make sure that
> pass-through device mappings are not mapped writable if the guest
> has requested them to be read-only. The existing implementation
> already honours this by calling kvm_set_s2pte_writable() on the new
> pte in case of writable mappings, so all we need to do is define
> the default pgprot_t value used for devices to be PTE_S2_RDONLY.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

> ---
>  arch/arm64/include/asm/pgtable.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> index ffe1ba0506d1..51f6f5284ce5 100644
> --- a/arch/arm64/include/asm/pgtable.h
> +++ b/arch/arm64/include/asm/pgtable.h
> @@ -79,7 +79,7 @@ extern void __pgd_error(const char *file, int line, unsigned long val);
>  #define PAGE_HYP_DEVICE		__pgprot(PROT_DEVICE_nGnRE | PTE_HYP)
>  
>  #define PAGE_S2			__pgprot(PROT_DEFAULT | PTE_S2_MEMATTR(MT_S2_NORMAL) | PTE_S2_RDONLY)
> -#define PAGE_S2_DEVICE		__pgprot(PROT_DEFAULT | PTE_S2_MEMATTR(MT_S2_DEVICE_nGnRE) | PTE_S2_RDWR | PTE_UXN)
> +#define PAGE_S2_DEVICE		__pgprot(PROT_DEFAULT | PTE_S2_MEMATTR(MT_S2_DEVICE_nGnRE) | PTE_S2_RDONLY | PTE_UXN)
>  
>  #define PAGE_NONE		__pgprot(((_PAGE_DEFAULT) & ~PTE_TYPE_MASK) | PTE_PROT_NONE | PTE_PXN | PTE_UXN)
>  #define PAGE_SHARED		__pgprot(_PAGE_DEFAULT | PTE_USER | PTE_NG | PTE_PXN | PTE_UXN | PTE_WRITE)
> 


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

[PATCH 6/6] arm/arm64: KVM: map MMIO regions at creation time

[Ard Biesheuvel]

There is really no point in faulting in memory regions page by page
if they are not backed by demand paged system RAM but by a linear
passthrough mapping of a host MMIO region.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm/kvm/mmu.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index fe53c3a30383..b153ef0c6d9f 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -1162,7 +1162,55 @@ void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *free,
 int kvm_arch_create_memslot(struct kvm *kvm, struct kvm_memory_slot *slot,
 			    unsigned long npages)
 {
-	return 0;
+	hva_t hva = slot->userspace_addr;
+	phys_addr_t gpa = slot->base_gfn << PAGE_SHIFT;
+	phys_addr_t size = slot->npages << PAGE_SHIFT;
+	int ret = 0;
+
+	/*
+	 * A memslot could potentially cover multiple VMAs, so iterate
+	 * over all of them to find out if we can map any of them right now.
+	 *
+	 *     +--------------------------------------------+
+	 * +---+---------+-------------------+--------------+----+
+	 * |   : VMA 1   |       VMA 2       |       VMA 3  :    |
+	 * +---+---------+-------------------+--------------+----+
+	 *     |                   memslot                  |
+	 *     +--------------------------------------------+
+	 */
+	do {
+		struct vm_area_struct *vma = find_vma(current->mm, hva);
+		hva_t start, end;
+
+		if (!vma || vma->vm_start > hva) {
+			ret = -EFAULT;
+			break;
+		}
+
+		start = max(slot->userspace_addr, vma->vm_start);
+		end = min((hva_t)(slot->userspace_addr + size), vma->vm_end);
+
+		if (vma->vm_flags & VM_PFNMAP) {
+			phys_addr_t pa = (vma->vm_pgoff << PAGE_SHIFT) + start -
+					 vma->vm_start;
+			bool writable = vma->vm_flags & VM_WRITE &&
+					!(slot->flags & KVM_MEM_READONLY);
+
+			ret = kvm_phys_addr_ioremap(kvm, gpa, pa, end - start,
+						    writable);
+			if (ret)
+				break;
+		}
+		hva += end - start;
+		gpa += end - start;
+	} while (hva < slot->userspace_addr + size);
+
+	if (ret) {
+		spin_lock(&kvm->mmu_lock);
+		unmap_stage2_range(kvm, slot->base_gfn << PAGE_SHIFT, size);
+		spin_unlock(&kvm->mmu_lock);
+	}
+	return ret;
 }
 
 void kvm_arch_memslots_updated(struct kvm *kvm)
-- 
1.8.3.2

[PATCH 6/6] arm/arm64: KVM: map MMIO regions at creation time

[Christoffer Dall]

On Wed, Sep 17, 2014 at 02:56:21PM -0700, Ard Biesheuvel wrote:
> There is really no point in faulting in memory regions page by page
> if they are not backed by demand paged system RAM but by a linear
> passthrough mapping of a host MMIO region.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm/kvm/mmu.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index fe53c3a30383..b153ef0c6d9f 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -1162,7 +1162,55 @@ void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *free,
>  int kvm_arch_create_memslot(struct kvm *kvm, struct kvm_memory_slot *slot,
>  			    unsigned long npages)
>  {
> -	return 0;
> +	hva_t hva = slot->userspace_addr;
> +	phys_addr_t gpa = slot->base_gfn << PAGE_SHIFT;
> +	phys_addr_t size = slot->npages << PAGE_SHIFT;
> +	int ret = 0;
> +
> +	/*
> +	 * A memslot could potentially cover multiple VMAs, so iterate
> +	 * over all of them to find out if we can map any of them right now.
> +	 *
> +	 *     +--------------------------------------------+
> +	 * +---+---------+-------------------+--------------+----+
> +	 * |   : VMA 1   |       VMA 2       |       VMA 3  :    |
> +	 * +---+---------+-------------------+--------------+----+
> +	 *     |                   memslot                  |
> +	 *     +--------------------------------------------+
> +	 */
> +	do {
> +		struct vm_area_struct *vma = find_vma(current->mm, hva);
> +		hva_t start, end;
> +
> +		if (!vma || vma->vm_start > hva) {
> +			ret = -EFAULT;
> +			break;
> +		}
> +
> +		start = max(slot->userspace_addr, vma->vm_start);
> +		end = min((hva_t)(slot->userspace_addr + size), vma->vm_end);
> +
> +		if (vma->vm_flags & VM_PFNMAP) {
> +			phys_addr_t pa = (vma->vm_pgoff << PAGE_SHIFT) + start -
> +					 vma->vm_start;
> +			bool writable = vma->vm_flags & VM_WRITE &&
> +					!(slot->flags & KVM_MEM_READONLY);
> +
> +			ret = kvm_phys_addr_ioremap(kvm, gpa, pa, end - start,
> +						    writable);
> +			if (ret)
> +				break;
> +		}
> +		hva += end - start;
> +		gpa += end - start;
> +	} while (hva < slot->userspace_addr + size);
> +
> +	if (ret) {
> +		spin_lock(&kvm->mmu_lock);
> +		unmap_stage2_range(kvm, slot->base_gfn << PAGE_SHIFT, size);
> +		spin_unlock(&kvm->mmu_lock);
> +	}
> +	return ret;
>  }
>  
>  void kvm_arch_memslots_updated(struct kvm *kvm)
> -- 
> 1.8.3.2
> 

Looks really good!  But we should handle moving a memslot as well, which
also tells me we should probably move this logic to
kvm_arch_prepare_memory_region() instead...

Thanks,
-Christoffer

[PATCH 0/6] KVM: handling of MMIO pass-through regions

[Christoffer Dall]

On Wed, Sep 17, 2014 at 02:56:15PM -0700, Ard Biesheuvel wrote:
> This is a followup to the patches 'kvm: define PAGE_S2_DEVICE as
> read-only by default' I sent out last week. Essentially build tested
> only: while QEMU on KVM still works correctly for my use case with
> these patches applied, it does not in fact use passthrough mappings
> of device memory.
> 
> Patch #1 is a trivial fix for an issue identified by sparse where we
> are calling memset(0) on 2 MB worth of pages.
> 
> Patch #2 fixes a potential NULL dereference in user_mem_abort()
> 
> Patch #3 adds a 'writable' parameter to kmv_phys_addr_ioremap() so that
> read-only device regions can be mapped using this function. The existing
> callers are updated to pass 'true' for this parameter.
> 
> Patch #4 and #5 were sent out before, and change the value of
> PAGE_S2_DEVICE to read-only, so that read-only regions can be
> ioremap()'ed
> 
> Patch #6 ensures that VM_PFNMAP linear mappings of non-system RAM host
> memory are mapped eagerly rather than faulted in page by page.
> 
> 
> Ard Biesheuvel (6):
>   arm/arm64: KVM: use __GFP_ZERO not memset() to get zeroed pages
>   arm/arm64: KVM: fix potential NULL dereference in user_mem_abort()
>   arm/arm64: KVM: add 'writable' parameter to kvm_phys_addr_ioremap
>   ARM: kvm: define PAGE_S2_DEVICE as read-only by default
>   arm64: kvm: define PAGE_S2_DEVICE as read-only by default
>   arm/arm64: KVM: map MMIO regions at creation time
> 

So I only had comments on that last one and the others are fairly benign
and small patches, so assuming the arch maintainers don't complain about
the mm header changes, I can queue the first 5 patches and add the last
one when we sort that out.

Thanks,
-Christoffer

[PATCH 0/6] KVM: handling of MMIO pass-through regions

[Ard Biesheuvel]

On 29 September 2014 15:07, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> On Wed, Sep 17, 2014 at 02:56:15PM -0700, Ard Biesheuvel wrote:
>> This is a followup to the patches 'kvm: define PAGE_S2_DEVICE as
>> read-only by default' I sent out last week. Essentially build tested
>> only: while QEMU on KVM still works correctly for my use case with
>> these patches applied, it does not in fact use passthrough mappings
>> of device memory.
>>
>> Patch #1 is a trivial fix for an issue identified by sparse where we
>> are calling memset(0) on 2 MB worth of pages.
>>
>> Patch #2 fixes a potential NULL dereference in user_mem_abort()
>>
>> Patch #3 adds a 'writable' parameter to kmv_phys_addr_ioremap() so that
>> read-only device regions can be mapped using this function. The existing
>> callers are updated to pass 'true' for this parameter.
>>
>> Patch #4 and #5 were sent out before, and change the value of
>> PAGE_S2_DEVICE to read-only, so that read-only regions can be
>> ioremap()'ed
>>
>> Patch #6 ensures that VM_PFNMAP linear mappings of non-system RAM host
>> memory are mapped eagerly rather than faulted in page by page.
>>
>>
>> Ard Biesheuvel (6):
>>   arm/arm64: KVM: use __GFP_ZERO not memset() to get zeroed pages
>>   arm/arm64: KVM: fix potential NULL dereference in user_mem_abort()
>>   arm/arm64: KVM: add 'writable' parameter to kvm_phys_addr_ioremap
>>   ARM: kvm: define PAGE_S2_DEVICE as read-only by default
>>   arm64: kvm: define PAGE_S2_DEVICE as read-only by default
>>   arm/arm64: KVM: map MMIO regions at creation time
>>
>
> So I only had comments on that last one and the others are fairly benign
> and small patches, so assuming the arch maintainers don't complain about
> the mm header changes, I can queue the first 5 patches and add the last
> one when we sort that out.
>

Fine with me. Let's at least wait until we have the userland bits in
place to test this patch thoroughly.

-- 
Ard.

[PATCH 0/6] KVM: handling of MMIO pass-through regions

[Christoffer Dall]

On Wed, Sep 17, 2014 at 02:56:15PM -0700, Ard Biesheuvel wrote:
> This is a followup to the patches 'kvm: define PAGE_S2_DEVICE as
> read-only by default' I sent out last week. Essentially build tested
> only: while QEMU on KVM still works correctly for my use case with
> these patches applied, it does not in fact use passthrough mappings
> of device memory.
> 
> Patch #1 is a trivial fix for an issue identified by sparse where we
> are calling memset(0) on 2 MB worth of pages.
> 
> Patch #2 fixes a potential NULL dereference in user_mem_abort()
> 
> Patch #3 adds a 'writable' parameter to kmv_phys_addr_ioremap() so that
> read-only device regions can be mapped using this function. The existing
> callers are updated to pass 'true' for this parameter.
> 
> Patch #4 and #5 were sent out before, and change the value of
> PAGE_S2_DEVICE to read-only, so that read-only regions can be
> ioremap()'ed
> 
> Patch #6 ensures that VM_PFNMAP linear mappings of non-system RAM host
> memory are mapped eagerly rather than faulted in page by page.
> 
> 
Thanks, applied patches 1 through 5 to kvmarm/queue with the fixups
discussed.

-Christoffer