linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] ARM: KVM: Enable the KVM-VFIO device
@ 2014-03-25 22:08 Kim Phillips
  2014-03-25 22:08 ` [PATCH 2/2] ARM: KVM: user_mem_abort: support stage 2 MMIO page mapping Kim Phillips
  0 siblings, 1 reply; 11+ messages in thread
From: Kim Phillips @ 2014-03-25 22:08 UTC (permalink / raw)
  To: Christoffer Dall, Gleb Natapov, Paolo Bonzini, Russell King,
	kvmarm, kvm, linux-arm-kernel
  Cc: linux-kernel, Eric Auger, Alex Williamson, Antonios Motakis,
	Kim Phillips, Kim Phillips

Used by KVM-enabled VFIO-based device passthrough support in QEMU.

Signed-off-by: Kim Phillips <kim.phillips@linaro.org>
---
This is just a couple of patches developed during platform
device passthrough development in QEMU.  Based on linux v3.14-rc8.

 arch/arm/kvm/Kconfig  | 1 +
 arch/arm/kvm/Makefile | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kvm/Kconfig b/arch/arm/kvm/Kconfig
index 466bd29..22ca356 100644
--- a/arch/arm/kvm/Kconfig
+++ b/arch/arm/kvm/Kconfig
@@ -23,6 +23,7 @@ config KVM
 	select HAVE_KVM_CPU_RELAX_INTERCEPT
 	select KVM_MMIO
 	select KVM_ARM_HOST
+	select KVM_VFIO
 	depends on ARM_VIRT_EXT && ARM_LPAE
 	---help---
 	  Support hosting virtualized guest machines. You will also
diff --git a/arch/arm/kvm/Makefile b/arch/arm/kvm/Makefile
index 789bca9..21a6ee0 100644
--- a/arch/arm/kvm/Makefile
+++ b/arch/arm/kvm/Makefile
@@ -15,7 +15,7 @@ AFLAGS_init.o := -Wa,-march=armv7-a$(plus_virt)
 AFLAGS_interrupts.o := -Wa,-march=armv7-a$(plus_virt)
 
 KVM := ../../../virt/kvm
-kvm-arm-y = $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o
+kvm-arm-y = $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o $(KVM)/vfio.o
 
 obj-y += kvm-arm.o init.o interrupts.o
 obj-y += arm.o handle_exit.o guest.o mmu.o emulate.o reset.o
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/2] ARM: KVM: user_mem_abort: support stage 2 MMIO page mapping
  2014-03-25 22:08 [PATCH 1/2] ARM: KVM: Enable the KVM-VFIO device Kim Phillips
@ 2014-03-25 22:08 ` Kim Phillips
  2014-05-06 18:04   ` Christoffer Dall
  0 siblings, 1 reply; 11+ messages in thread
From: Kim Phillips @ 2014-03-25 22:08 UTC (permalink / raw)
  To: Christoffer Dall, Gleb Natapov, Paolo Bonzini, Russell King,
	kvmarm, kvm, linux-arm-kernel
  Cc: linux-kernel, Eric Auger, Alex Williamson, Antonios Motakis,
	Kim Phillips, Kim Phillips

Use the correct memory type for device MMIO mappings: PAGE_S2_DEVICE.

Signed-off-by: Kim Phillips <kim.phillips@linaro.org>
---
 arch/arm/kvm/mmu.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index 7789857..a354610 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -652,6 +652,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 	struct kvm_mmu_memory_cache *memcache = &vcpu->arch.mmu_page_cache;
 	struct vm_area_struct *vma;
 	pfn_t pfn;
+	pgprot_t mem_type = PAGE_S2;
 
 	write_fault = kvm_is_write_fault(kvm_vcpu_get_hsr(vcpu));
 	if (fault_status == FSC_PERM && !write_fault) {
@@ -702,6 +703,9 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 	if (is_error_pfn(pfn))
 		return -EFAULT;
 
+	if (kvm_is_mmio_pfn(pfn))
+		mem_type = PAGE_S2_DEVICE;
+
 	spin_lock(&kvm->mmu_lock);
 	if (mmu_notifier_retry(kvm, mmu_seq))
 		goto out_unlock;
@@ -709,7 +713,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 		hugetlb = transparent_hugepage_adjust(&pfn, &fault_ipa);
 
 	if (hugetlb) {
-		pmd_t new_pmd = pfn_pmd(pfn, PAGE_S2);
+		pmd_t new_pmd = pfn_pmd(pfn, mem_type);
 		new_pmd = pmd_mkhuge(new_pmd);
 		if (writable) {
 			kvm_set_s2pmd_writable(&new_pmd);
@@ -718,13 +722,14 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 		coherent_icache_guest_page(kvm, hva & PMD_MASK, PMD_SIZE);
 		ret = stage2_set_pmd_huge(kvm, memcache, fault_ipa, &new_pmd);
 	} else {
-		pte_t new_pte = pfn_pte(pfn, PAGE_S2);
+		pte_t new_pte = pfn_pte(pfn, mem_type);
 		if (writable) {
 			kvm_set_s2pte_writable(&new_pte);
 			kvm_set_pfn_dirty(pfn);
 		}
 		coherent_icache_guest_page(kvm, hva, PAGE_SIZE);
-		ret = stage2_set_pte(kvm, memcache, fault_ipa, &new_pte, false);
+		ret = stage2_set_pte(kvm, memcache, fault_ipa, &new_pte,
+				     mem_type == PAGE_S2_DEVICE);
 	}
 
 
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] ARM: KVM: user_mem_abort: support stage 2 MMIO page mapping
  2014-03-25 22:08 ` [PATCH 2/2] ARM: KVM: user_mem_abort: support stage 2 MMIO page mapping Kim Phillips
@ 2014-05-06 18:04   ` Christoffer Dall
  2014-05-07  9:00     ` Marc Zyngier
  0 siblings, 1 reply; 11+ messages in thread
From: Christoffer Dall @ 2014-05-06 18:04 UTC (permalink / raw)
  To: Kim Phillips
  Cc: kvmarm, linux-arm-kernel, kvm, linux-kernel, Eric Auger,
	Alex Williamson, Antonios Motakis, Kim Phillips, marc.zyngier

On Tue, Mar 25, 2014 at 05:08:14PM -0500, Kim Phillips wrote:
> Use the correct memory type for device MMIO mappings: PAGE_S2_DEVICE.
> 
> Signed-off-by: Kim Phillips <kim.phillips@linaro.org>
> ---
>  arch/arm/kvm/mmu.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index 7789857..a354610 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -652,6 +652,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>  	struct kvm_mmu_memory_cache *memcache = &vcpu->arch.mmu_page_cache;
>  	struct vm_area_struct *vma;
>  	pfn_t pfn;
> +	pgprot_t mem_type = PAGE_S2;
>  
>  	write_fault = kvm_is_write_fault(kvm_vcpu_get_hsr(vcpu));
>  	if (fault_status == FSC_PERM && !write_fault) {
> @@ -702,6 +703,9 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>  	if (is_error_pfn(pfn))
>  		return -EFAULT;
>  
> +	if (kvm_is_mmio_pfn(pfn))
> +		mem_type = PAGE_S2_DEVICE;
> +
>  	spin_lock(&kvm->mmu_lock);
>  	if (mmu_notifier_retry(kvm, mmu_seq))
>  		goto out_unlock;
> @@ -709,7 +713,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>  		hugetlb = transparent_hugepage_adjust(&pfn, &fault_ipa);
>  
>  	if (hugetlb) {
> -		pmd_t new_pmd = pfn_pmd(pfn, PAGE_S2);
> +		pmd_t new_pmd = pfn_pmd(pfn, mem_type);
>  		new_pmd = pmd_mkhuge(new_pmd);
>  		if (writable) {
>  			kvm_set_s2pmd_writable(&new_pmd);
> @@ -718,13 +722,14 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>  		coherent_icache_guest_page(kvm, hva & PMD_MASK, PMD_SIZE);
>  		ret = stage2_set_pmd_huge(kvm, memcache, fault_ipa, &new_pmd);
>  	} else {
> -		pte_t new_pte = pfn_pte(pfn, PAGE_S2);
> +		pte_t new_pte = pfn_pte(pfn, mem_type);
>  		if (writable) {
>  			kvm_set_s2pte_writable(&new_pte);
>  			kvm_set_pfn_dirty(pfn);
>  		}
>  		coherent_icache_guest_page(kvm, hva, PAGE_SIZE);
> -		ret = stage2_set_pte(kvm, memcache, fault_ipa, &new_pte, false);
> +		ret = stage2_set_pte(kvm, memcache, fault_ipa, &new_pte,
> +				     mem_type == PAGE_S2_DEVICE);
>  	}
>  
>  
> -- 
> 1.9.1
> 

I think this looks reasonable.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] ARM: KVM: user_mem_abort: support stage 2 MMIO page mapping
  2014-05-06 18:04   ` Christoffer Dall
@ 2014-05-07  9:00     ` Marc Zyngier
  2014-05-07 14:55       ` Christoffer Dall
  0 siblings, 1 reply; 11+ messages in thread
From: Marc Zyngier @ 2014-05-07  9:00 UTC (permalink / raw)
  To: Christoffer Dall
  Cc: Kim Phillips, kvmarm, linux-arm-kernel, kvm, linux-kernel,
	Eric Auger, Alex Williamson, Antonios Motakis, Kim Phillips

Kim, Christoffer,

On Tue, May 06 2014 at  7:04:48 pm BST, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> On Tue, Mar 25, 2014 at 05:08:14PM -0500, Kim Phillips wrote:
>> Use the correct memory type for device MMIO mappings: PAGE_S2_DEVICE.
>> 
>> Signed-off-by: Kim Phillips <kim.phillips@linaro.org>
>> ---
>>  arch/arm/kvm/mmu.c | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>> 
>> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
>> index 7789857..a354610 100644
>> --- a/arch/arm/kvm/mmu.c
>> +++ b/arch/arm/kvm/mmu.c
>> @@ -652,6 +652,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>>  	struct kvm_mmu_memory_cache *memcache = &vcpu->arch.mmu_page_cache;
>>  	struct vm_area_struct *vma;
>>  	pfn_t pfn;
>> +	pgprot_t mem_type = PAGE_S2;
>>  
>>  	write_fault = kvm_is_write_fault(kvm_vcpu_get_hsr(vcpu));
>>  	if (fault_status == FSC_PERM && !write_fault) {
>> @@ -702,6 +703,9 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>>  	if (is_error_pfn(pfn))
>>  		return -EFAULT;
>>  
>> +	if (kvm_is_mmio_pfn(pfn))
>> +		mem_type = PAGE_S2_DEVICE;
>> +
>>  	spin_lock(&kvm->mmu_lock);
>>  	if (mmu_notifier_retry(kvm, mmu_seq))
>>  		goto out_unlock;
>> @@ -709,7 +713,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>>  		hugetlb = transparent_hugepage_adjust(&pfn, &fault_ipa);
>>  
>>  	if (hugetlb) {
>> -		pmd_t new_pmd = pfn_pmd(pfn, PAGE_S2);
>> +		pmd_t new_pmd = pfn_pmd(pfn, mem_type);
>>  		new_pmd = pmd_mkhuge(new_pmd);
>>  		if (writable) {
>>  			kvm_set_s2pmd_writable(&new_pmd);
>> @@ -718,13 +722,14 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>>  		coherent_icache_guest_page(kvm, hva & PMD_MASK, PMD_SIZE);
>>  		ret = stage2_set_pmd_huge(kvm, memcache, fault_ipa, &new_pmd);
>>  	} else {
>> -		pte_t new_pte = pfn_pte(pfn, PAGE_S2);
>> +		pte_t new_pte = pfn_pte(pfn, mem_type);
>>  		if (writable) {
>>  			kvm_set_s2pte_writable(&new_pte);
>>  			kvm_set_pfn_dirty(pfn);
>>  		}
>>  		coherent_icache_guest_page(kvm, hva, PAGE_SIZE);
>> -		ret = stage2_set_pte(kvm, memcache, fault_ipa, &new_pte, false);
>> +		ret = stage2_set_pte(kvm, memcache, fault_ipa, &new_pte,
>> +				     mem_type == PAGE_S2_DEVICE);
>>  	}
>>  
>>  
>> -- 
>> 1.9.1
>> 
>
> I think this looks reasonable.
>
> Acked-by: Christoffer Dall <christoffer.dall@linaro.org>

I feel like I'm missing some context here, and the commit message is way
too terse for me to make sense of it.

So far, we can only get into user_mem_abort on a Stage-2 fault
(translation or permission) for memory. How can we suddenly get here for
a *device* fault? Do we get a special kind of memslot?

I'm not saying the patch does anything wrong, but I'd like to understand
the rationale behind it. On its own, it doesn't make much sense.

Thanks,

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] ARM: KVM: user_mem_abort: support stage 2 MMIO page mapping
  2014-05-07  9:00     ` Marc Zyngier
@ 2014-05-07 14:55       ` Christoffer Dall
  2014-06-24 10:23         ` Will Deacon
  0 siblings, 1 reply; 11+ messages in thread
From: Christoffer Dall @ 2014-05-07 14:55 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Kim Phillips, kvmarm, linux-arm-kernel, kvm, linux-kernel,
	Eric Auger, Alex Williamson, Antonios Motakis, Kim Phillips

On Wed, May 07, 2014 at 10:00:21AM +0100, Marc Zyngier wrote:
> Kim, Christoffer,
> 
> On Tue, May 06 2014 at  7:04:48 pm BST, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> > On Tue, Mar 25, 2014 at 05:08:14PM -0500, Kim Phillips wrote:
> >> Use the correct memory type for device MMIO mappings: PAGE_S2_DEVICE.
> >> 
> >> Signed-off-by: Kim Phillips <kim.phillips@linaro.org>
> >> ---
> >>  arch/arm/kvm/mmu.c | 11 ++++++++---
> >>  1 file changed, 8 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> >> index 7789857..a354610 100644
> >> --- a/arch/arm/kvm/mmu.c
> >> +++ b/arch/arm/kvm/mmu.c
> >> @@ -652,6 +652,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> >>  	struct kvm_mmu_memory_cache *memcache = &vcpu->arch.mmu_page_cache;
> >>  	struct vm_area_struct *vma;
> >>  	pfn_t pfn;
> >> +	pgprot_t mem_type = PAGE_S2;
> >>  
> >>  	write_fault = kvm_is_write_fault(kvm_vcpu_get_hsr(vcpu));
> >>  	if (fault_status == FSC_PERM && !write_fault) {
> >> @@ -702,6 +703,9 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> >>  	if (is_error_pfn(pfn))
> >>  		return -EFAULT;
> >>  
> >> +	if (kvm_is_mmio_pfn(pfn))
> >> +		mem_type = PAGE_S2_DEVICE;
> >> +
> >>  	spin_lock(&kvm->mmu_lock);
> >>  	if (mmu_notifier_retry(kvm, mmu_seq))
> >>  		goto out_unlock;
> >> @@ -709,7 +713,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> >>  		hugetlb = transparent_hugepage_adjust(&pfn, &fault_ipa);
> >>  
> >>  	if (hugetlb) {
> >> -		pmd_t new_pmd = pfn_pmd(pfn, PAGE_S2);
> >> +		pmd_t new_pmd = pfn_pmd(pfn, mem_type);
> >>  		new_pmd = pmd_mkhuge(new_pmd);
> >>  		if (writable) {
> >>  			kvm_set_s2pmd_writable(&new_pmd);
> >> @@ -718,13 +722,14 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> >>  		coherent_icache_guest_page(kvm, hva & PMD_MASK, PMD_SIZE);
> >>  		ret = stage2_set_pmd_huge(kvm, memcache, fault_ipa, &new_pmd);
> >>  	} else {
> >> -		pte_t new_pte = pfn_pte(pfn, PAGE_S2);
> >> +		pte_t new_pte = pfn_pte(pfn, mem_type);
> >>  		if (writable) {
> >>  			kvm_set_s2pte_writable(&new_pte);
> >>  			kvm_set_pfn_dirty(pfn);
> >>  		}
> >>  		coherent_icache_guest_page(kvm, hva, PAGE_SIZE);
> >> -		ret = stage2_set_pte(kvm, memcache, fault_ipa, &new_pte, false);
> >> +		ret = stage2_set_pte(kvm, memcache, fault_ipa, &new_pte,
> >> +				     mem_type == PAGE_S2_DEVICE);
> >>  	}
> >>  
> >>  
> >> -- 
> >> 1.9.1
> >> 
> >
> > I think this looks reasonable.
> >
> > Acked-by: Christoffer Dall <christoffer.dall@linaro.org>
> 
> I feel like I'm missing some context here, and the commit message is way
> too terse for me to make sense of it.
> 
> So far, we can only get into user_mem_abort on a Stage-2 fault
> (translation or permission) for memory. How can we suddenly get here for
> a *device* fault? Do we get a special kind of memslot?
> 
> I'm not saying the patch does anything wrong, but I'd like to understand
> the rationale behind it. On its own, it doesn't make much sense.
> 
Think device passthrough.  There's nothing preventing user space from
setting up a memory region to point to device memory (through VFIO or
/dev/mem).  If that's done, we should enforce device memory properties
so writes don't linger around in the cache to be written some time later
when that device memory potentially doesn't belong to the VM anymore.

This is just one tiny piece of all of them to make device passthrough
work, and we could hold off with this patch until we have something more
complete.  On the other hand, we need to start somewhere, and this is
hardly intrusive and is functionally correct even though you don't have
a full device passthrough setup.

-Christoffer

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] ARM: KVM: user_mem_abort: support stage 2 MMIO page mapping
  2014-05-07 14:55       ` Christoffer Dall
@ 2014-06-24 10:23         ` Will Deacon
  2014-06-24 10:39           ` Marc Zyngier
  0 siblings, 1 reply; 11+ messages in thread
From: Will Deacon @ 2014-06-24 10:23 UTC (permalink / raw)
  To: Christoffer Dall
  Cc: Marc Zyngier, Kim Phillips, kvm, Eric Auger, Kim Phillips,
	linux-kernel, Alex Williamson, Antonios Motakis, kvmarm,
	linux-arm-kernel

On Wed, May 07, 2014 at 03:55:57PM +0100, Christoffer Dall wrote:
> On Wed, May 07, 2014 at 10:00:21AM +0100, Marc Zyngier wrote:
> > On Tue, May 06 2014 at  7:04:48 pm BST, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> > > On Tue, Mar 25, 2014 at 05:08:14PM -0500, Kim Phillips wrote:
> > >> Use the correct memory type for device MMIO mappings: PAGE_S2_DEVICE.
> > >> 
> > >> Signed-off-by: Kim Phillips <kim.phillips@linaro.org>
> > >> ---
> > >>  arch/arm/kvm/mmu.c | 11 ++++++++---
> > >>  1 file changed, 8 insertions(+), 3 deletions(-)

[...]

> > > I think this looks reasonable.
> > >
> > > Acked-by: Christoffer Dall <christoffer.dall@linaro.org>
> > 
> > I feel like I'm missing some context here, and the commit message is way
> > too terse for me to make sense of it.
> > 
> > So far, we can only get into user_mem_abort on a Stage-2 fault
> > (translation or permission) for memory. How can we suddenly get here for
> > a *device* fault? Do we get a special kind of memslot?
> > 
> > I'm not saying the patch does anything wrong, but I'd like to understand
> > the rationale behind it. On its own, it doesn't make much sense.
> > 
> Think device passthrough.  There's nothing preventing user space from
> setting up a memory region to point to device memory (through VFIO or
> /dev/mem).  If that's done, we should enforce device memory properties
> so writes don't linger around in the cache to be written some time later
> when that device memory potentially doesn't belong to the VM anymore.
> 
> This is just one tiny piece of all of them to make device passthrough
> work, and we could hold off with this patch until we have something more
> complete.  On the other hand, we need to start somewhere, and this is
> hardly intrusive and is functionally correct even though you don't have
> a full device passthrough setup.

Please can you queue this patch up? I need it for my VFIO work, where I'm
registering the PCI BARs using KVM_SET_USER_MEMORY_REGION.

Without this, I'd have to trap all accesses and do pread/pwrite from
kvmtool instead of mmaping the regions straight through.

Cheers,

Will

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] ARM: KVM: user_mem_abort: support stage 2 MMIO page mapping
  2014-06-24 10:23         ` Will Deacon
@ 2014-06-24 10:39           ` Marc Zyngier
  2014-06-26  0:45             ` [PATCH 2/2 v2] " Kim Phillips
  0 siblings, 1 reply; 11+ messages in thread
From: Marc Zyngier @ 2014-06-24 10:39 UTC (permalink / raw)
  To: Will Deacon, Kim Phillips
  Cc: Christoffer Dall, kvm, Eric Auger, Kim Phillips, linux-kernel,
	Alex Williamson, Antonios Motakis, kvmarm, linux-arm-kernel

On 24/06/14 11:23, Will Deacon wrote:
> On Wed, May 07, 2014 at 03:55:57PM +0100, Christoffer Dall wrote:
>> On Wed, May 07, 2014 at 10:00:21AM +0100, Marc Zyngier wrote:
>>> On Tue, May 06 2014 at  7:04:48 pm BST, Christoffer Dall <christoffer.dall@linaro.org> wrote:
>>>> On Tue, Mar 25, 2014 at 05:08:14PM -0500, Kim Phillips wrote:
>>>>> Use the correct memory type for device MMIO mappings: PAGE_S2_DEVICE.
>>>>>
>>>>> Signed-off-by: Kim Phillips <kim.phillips@linaro.org>
>>>>> ---
>>>>>  arch/arm/kvm/mmu.c | 11 ++++++++---
>>>>>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> [...]
> 
>>>> I think this looks reasonable.
>>>>
>>>> Acked-by: Christoffer Dall <christoffer.dall@linaro.org>
>>>
>>> I feel like I'm missing some context here, and the commit message is way
>>> too terse for me to make sense of it.
>>>
>>> So far, we can only get into user_mem_abort on a Stage-2 fault
>>> (translation or permission) for memory. How can we suddenly get here for
>>> a *device* fault? Do we get a special kind of memslot?
>>>
>>> I'm not saying the patch does anything wrong, but I'd like to understand
>>> the rationale behind it. On its own, it doesn't make much sense.
>>>
>> Think device passthrough.  There's nothing preventing user space from
>> setting up a memory region to point to device memory (through VFIO or
>> /dev/mem).  If that's done, we should enforce device memory properties
>> so writes don't linger around in the cache to be written some time later
>> when that device memory potentially doesn't belong to the VM anymore.
>>
>> This is just one tiny piece of all of them to make device passthrough
>> work, and we could hold off with this patch until we have something more
>> complete.  On the other hand, we need to start somewhere, and this is
>> hardly intrusive and is functionally correct even though you don't have
>> a full device passthrough setup.
> 
> Please can you queue this patch up? I need it for my VFIO work, where I'm
> registering the PCI BARs using KVM_SET_USER_MEMORY_REGION.
> 
> Without this, I'd have to trap all accesses and do pread/pwrite from
> kvmtool instead of mmaping the regions straight through.

I'm afraid there as been quite a bit of churn in this department, and
the patch doesn't apply any more.

Kim, any chance you could respin this patch on top of mainline?

Thanks,

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 2/2 v2] ARM: KVM: user_mem_abort: support stage 2 MMIO page mapping
  2014-06-24 10:39           ` Marc Zyngier
@ 2014-06-26  0:45             ` Kim Phillips
  2014-06-26  8:46               ` Will Deacon
  0 siblings, 1 reply; 11+ messages in thread
From: Kim Phillips @ 2014-06-26  0:45 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Will Deacon, Christoffer Dall, kvm, Eric Auger, Kim Phillips,
	linux-kernel, Alex Williamson, Antonios Motakis, kvmarm,
	linux-arm-kernel

From: Kim Phillips <kim.phillips@linaro.org>

A userspace process can map device MMIO memory via VFIO or /dev/mem,
e.g., for platform device passthrough support in QEMU.

During early development, we found the PAGE_S2 memory type being used
for MMIO mappings.  This patch corrects that by using the more strongly
ordered memory type for device MMIO mappings: PAGE_S2_DEVICE.

Signed-off-by: Kim Phillips <kim.phillips@linaro.org>
Acked-by: Christoffer Dall <christoffer.dall@linaro.org>
---
Hi, here's a v2, upon request:

- rebased onto today's mainline ToT
- mmu.o-build tested only (ToT build doesn't complete)
- made commit text less terse
- added Christoffer's ack

Cheers,

Kim

 arch/arm/kvm/mmu.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index 16f8049..69af021 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -748,6 +748,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 	struct kvm_mmu_memory_cache *memcache = &vcpu->arch.mmu_page_cache;
 	struct vm_area_struct *vma;
 	pfn_t pfn;
+	pgprot_t mem_type = PAGE_S2;
 
 	write_fault = kvm_is_write_fault(kvm_vcpu_get_hsr(vcpu));
 	if (fault_status == FSC_PERM && !write_fault) {
@@ -798,6 +799,9 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 	if (is_error_pfn(pfn))
 		return -EFAULT;
 
+	if (kvm_is_mmio_pfn(pfn))
+		mem_type = PAGE_S2_DEVICE;
+
 	spin_lock(&kvm->mmu_lock);
 	if (mmu_notifier_retry(kvm, mmu_seq))
 		goto out_unlock;
@@ -805,7 +809,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 		hugetlb = transparent_hugepage_adjust(&pfn, &fault_ipa);
 
 	if (hugetlb) {
-		pmd_t new_pmd = pfn_pmd(pfn, PAGE_S2);
+		pmd_t new_pmd = pfn_pmd(pfn, mem_type);
 		new_pmd = pmd_mkhuge(new_pmd);
 		if (writable) {
 			kvm_set_s2pmd_writable(&new_pmd);
@@ -814,13 +818,14 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 		coherent_cache_guest_page(vcpu, hva & PMD_MASK, PMD_SIZE);
 		ret = stage2_set_pmd_huge(kvm, memcache, fault_ipa, &new_pmd);
 	} else {
-		pte_t new_pte = pfn_pte(pfn, PAGE_S2);
+		pte_t new_pte = pfn_pte(pfn, mem_type);
 		if (writable) {
 			kvm_set_s2pte_writable(&new_pte);
 			kvm_set_pfn_dirty(pfn);
 		}
 		coherent_cache_guest_page(vcpu, hva, PAGE_SIZE);
-		ret = stage2_set_pte(kvm, memcache, fault_ipa, &new_pte, false);
+		ret = stage2_set_pte(kvm, memcache, fault_ipa, &new_pte,
+				     mem_type == PAGE_S2_DEVICE);
 	}
 
 
-- 
2.0.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2 v2] ARM: KVM: user_mem_abort: support stage 2 MMIO page mapping
  2014-06-26  0:45             ` [PATCH 2/2 v2] " Kim Phillips
@ 2014-06-26  8:46               ` Will Deacon
  2014-06-30  9:08                 ` Christoffer Dall
  0 siblings, 1 reply; 11+ messages in thread
From: Will Deacon @ 2014-06-26  8:46 UTC (permalink / raw)
  To: Kim Phillips
  Cc: Marc Zyngier, Christoffer Dall, kvm, Eric Auger, Kim Phillips,
	linux-kernel, Alex Williamson, Antonios Motakis, kvmarm,
	linux-arm-kernel

On Thu, Jun 26, 2014 at 01:45:51AM +0100, Kim Phillips wrote:
> From: Kim Phillips <kim.phillips@linaro.org>
> 
> A userspace process can map device MMIO memory via VFIO or /dev/mem,
> e.g., for platform device passthrough support in QEMU.
> 
> During early development, we found the PAGE_S2 memory type being used
> for MMIO mappings.  This patch corrects that by using the more strongly
> ordered memory type for device MMIO mappings: PAGE_S2_DEVICE.
> 
> Signed-off-by: Kim Phillips <kim.phillips@linaro.org>
> Acked-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
> Hi, here's a v2, upon request:
> 
> - rebased onto today's mainline ToT
> - mmu.o-build tested only (ToT build doesn't complete)
> - made commit text less terse
> - added Christoffer's ack

Thanks for reposting this so quickly!

  Acked-by: Will Deacon <will.deacon@arm.com>

Will

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2 v2] ARM: KVM: user_mem_abort: support stage 2 MMIO page mapping
  2014-06-26  8:46               ` Will Deacon
@ 2014-06-30  9:08                 ` Christoffer Dall
  2014-06-30  9:14                   ` Marc Zyngier
  0 siblings, 1 reply; 11+ messages in thread
From: Christoffer Dall @ 2014-06-30  9:08 UTC (permalink / raw)
  To: Will Deacon
  Cc: Kim Phillips, Marc Zyngier, kvm, Eric Auger, Kim Phillips,
	linux-kernel, Alex Williamson, Antonios Motakis, kvmarm,
	linux-arm-kernel

On Thu, Jun 26, 2014 at 09:46:26AM +0100, Will Deacon wrote:
> On Thu, Jun 26, 2014 at 01:45:51AM +0100, Kim Phillips wrote:
> > From: Kim Phillips <kim.phillips@linaro.org>
> > 
> > A userspace process can map device MMIO memory via VFIO or /dev/mem,
> > e.g., for platform device passthrough support in QEMU.
> > 
> > During early development, we found the PAGE_S2 memory type being used
> > for MMIO mappings.  This patch corrects that by using the more strongly
> > ordered memory type for device MMIO mappings: PAGE_S2_DEVICE.
> > 
> > Signed-off-by: Kim Phillips <kim.phillips@linaro.org>
> > Acked-by: Christoffer Dall <christoffer.dall@linaro.org>
> > ---
> > Hi, here's a v2, upon request:
> > 
> > - rebased onto today's mainline ToT
> > - mmu.o-build tested only (ToT build doesn't complete)
> > - made commit text less terse
> > - added Christoffer's ack
> 
> Thanks for reposting this so quickly!
> 
>   Acked-by: Will Deacon <will.deacon@arm.com>
> 
Thanks,

Marc, will you apply this one to kvmarm/queue [and kvmarm/next]?

-Christoffer

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2 v2] ARM: KVM: user_mem_abort: support stage 2 MMIO page mapping
  2014-06-30  9:08                 ` Christoffer Dall
@ 2014-06-30  9:14                   ` Marc Zyngier
  0 siblings, 0 replies; 11+ messages in thread
From: Marc Zyngier @ 2014-06-30  9:14 UTC (permalink / raw)
  To: Christoffer Dall
  Cc: Will Deacon, Kim Phillips, kvm, Eric Auger, Kim Phillips,
	linux-kernel, Alex Williamson, Antonios Motakis, kvmarm,
	linux-arm-kernel

On 30/06/14 10:08, Christoffer Dall wrote:
> On Thu, Jun 26, 2014 at 09:46:26AM +0100, Will Deacon wrote:
>> On Thu, Jun 26, 2014 at 01:45:51AM +0100, Kim Phillips wrote:
>>> From: Kim Phillips <kim.phillips@linaro.org>
>>>
>>> A userspace process can map device MMIO memory via VFIO or /dev/mem,
>>> e.g., for platform device passthrough support in QEMU.
>>>
>>> During early development, we found the PAGE_S2 memory type being used
>>> for MMIO mappings.  This patch corrects that by using the more strongly
>>> ordered memory type for device MMIO mappings: PAGE_S2_DEVICE.
>>>
>>> Signed-off-by: Kim Phillips <kim.phillips@linaro.org>
>>> Acked-by: Christoffer Dall <christoffer.dall@linaro.org>
>>> ---
>>> Hi, here's a v2, upon request:
>>>
>>> - rebased onto today's mainline ToT
>>> - mmu.o-build tested only (ToT build doesn't complete)
>>> - made commit text less terse
>>> - added Christoffer's ack
>>
>> Thanks for reposting this so quickly!
>>
>>   Acked-by: Will Deacon <will.deacon@arm.com>
>>
> Thanks,
> 
> Marc, will you apply this one to kvmarm/queue [and kvmarm/next]?

Yup, adding it.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2014-06-30  9:14 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-03-25 22:08 [PATCH 1/2] ARM: KVM: Enable the KVM-VFIO device Kim Phillips
2014-03-25 22:08 ` [PATCH 2/2] ARM: KVM: user_mem_abort: support stage 2 MMIO page mapping Kim Phillips
2014-05-06 18:04   ` Christoffer Dall
2014-05-07  9:00     ` Marc Zyngier
2014-05-07 14:55       ` Christoffer Dall
2014-06-24 10:23         ` Will Deacon
2014-06-24 10:39           ` Marc Zyngier
2014-06-26  0:45             ` [PATCH 2/2 v2] " Kim Phillips
2014-06-26  8:46               ` Will Deacon
2014-06-30  9:08                 ` Christoffer Dall
2014-06-30  9:14                   ` Marc Zyngier

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).