KVM: handling of MMIO pass-through regions
Subsystems (3)
cover KVM: handling of MMIO pass-through regions
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(-)
Signature · 1 line
1.8.3.2
PATCH 1/6 arm/arm64: KVM: use __GFP_ZERO not memset() to get zeroed pages
Pass __GFP_ZERO to __get_free_pages() instead of calling memset() explicitly.
--- arch/arm/kvm/mmu.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
arch/arm/kvm/mmu.c +1 -2
@@ -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;
Signature · 1 line
1.8.3.2
PATCH 1/6 arm/arm64: KVM: use __GFP_ZERO not memset() to get zeroed pages
Quoted 15 lines · Ard Biesheuvel
Pass
__GFP_ZEROto__get_free_pages()instead of callingmemset()explicitly.Signed-off-by Ard Biesheuvel <[email protected]>--- arch/arm/kvm/mmu.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)arch/arm/kvm/mmu.c +0
-0@@ -528,11 +528,10 @@ int kvm_alloc_stage2_pgd(struct kvm *kvm)return -EINVAL ; }
arch/arm/kvm/mmu.c int kvm_alloc_stage2_pgd(struct kvm *kvm) · line 537
Quoted 10 lines
arch/arm/kvm/mmu.c +1
-2- 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.
Thanks,
-Christoffer
PATCH 1/6 arm/arm64: KVM: use __GFP_ZERO not memset() to get zeroed pages
In reply to Ard BiesheuvelQuoted 4 lines · Ard Biesheuvel
Pass
__GFP_ZEROto__get_free_pages()instead of callingmemset()explicitly.Signed-off-by Ard Biesheuvel <[email protected]>
M.
Quoted 22 lines
--- arch/arm/kvm/mmu.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)arch/arm/kvm/mmu.c +1
-2@@ -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;
Signature · 1 line
Jazz is not dead. It just smells funny...
PATCH 2/6 arm/arm64: KVM: fix potential NULL dereference in user_mem_abort()
In reply to Ard BiesheuvelHandle the potential NULL return value of find_vma_intersection() before dereferencing it.
--- arch/arm/kvm/mmu.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
arch/arm/kvm/mmu.c +5 -1
@@ -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 (¤t->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 (¤t->mm->mmap_sem); +return -EFAULT ; + }else if (is_vm_hugetlb_page (vma)) { hugetlb = true; gfn = (fault_ipa &PMD_MASK ) >>PAGE_SHIFT ; }else {
Signature · 1 line
1.8.3.2
PATCH 2/6 arm/arm64: KVM: fix potential NULL dereference in user_mem_abort()
Quoted 28 lines · Ard Biesheuvel
Handle the potential NULL return value of
find_vma_intersection()before dereferencing it.Signed-off-by Ard Biesheuvel <[email protected]>--- arch/arm/kvm/mmu.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)arch/arm/kvm/mmu.c +5
-1@@ -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 (¤t->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 (¤t->mm->mmap_sem); +return -EFAULT ; + }else if (is_vm_hugetlb_page (vma)) { hugetlb = true; gfn = (fault_ipa &PMD_MASK ) >>PAGE_SHIFT ; }else {Signature · 1 line
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()
In reply to Ard BiesheuvelQuoted 13 lines · Ard Biesheuvel
Handle the potential NULL return value of
find_vma_intersection()before dereferencing it.Signed-off-by Ard Biesheuvel <[email protected]>--- arch/arm/kvm/mmu.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)arch/arm/kvm/mmu.c +0
-0
arch/arm/kvm/mmu.c static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, · line 783
Quoted 9 lines
arch/arm/kvm/mmu.c +5
-1@@ -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 (¤t->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 (¤t->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.
arch/arm/kvm/mmu.c static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, · line 786
Quoted 4 lines
arch/arm/kvm/mmu.c +0
-0hugetlb = true; gfn = (fault_ipa &PMD_MASK ) >>PAGE_SHIFT ; }else {
Despite the above nit:
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
PATCH 2/6 arm/arm64: KVM: fix potential NULL dereference in user_mem_abort()
Quoted 18 lines · Marc Zyngier
Quoted 17 lines · Ard Biesheuvel
Handle the potential NULL return value of
find_vma_intersection()before dereferencing it.Signed-off-by Ard Biesheuvel <[email protected]>--- arch/arm/kvm/mmu.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)arch/arm/kvm/mmu.c +0
-1@@ -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 (¤t->mm->mmap_sem); vma =find_vma_intersection (current->mm, hva, hva +1 ); -if (is_vm_hugetlb_page (vma)) {
Quoted 9 lines
arch/arm/kvm/mmu.c static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, · line 783
Quoted 5 lines
arch/arm/kvm/mmu.c +5
-0+if (unlikely (!vma)) { +kvm_err ("Failed to find VMA for hva 0x%lx\n" , hva); +up_read (¤t->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?
Quoted 9 lines
arch/arm/kvm/mmu.c static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, · line 786
Quoted 4 lines
arch/arm/kvm/mmu.c +0
-0hugetlb = true; gfn = (fault_ipa &PMD_MASK ) >>PAGE_SHIFT ; }else {Despite the above nit:
Acked-by Marc Zyngier <[email protected]>
Thanks.
PATCH 2/6 arm/arm64: KVM: fix potential NULL dereference in user_mem_abort()
Quoted 29 lines · Ard Biesheuvel
Quoted 18 lines · Marc Zyngier
Quoted 17 lines · Ard Biesheuvel
Handle the potential NULL return value of
find_vma_intersection()before dereferencing it.Signed-off-by Ard Biesheuvel <[email protected]>--- arch/arm/kvm/mmu.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)arch/arm/kvm/mmu.c +0
-1@@ -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 (¤t->mm->mmap_sem); vma =find_vma_intersection (current->mm, hva, hva +1 ); -if (is_vm_hugetlb_page (vma)) {Quoted 9 lines
arch/arm/kvm/mmu.c static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, · line 783
Quoted 5 lines
arch/arm/kvm/mmu.c +5
-0+if (unlikely (!vma)) { +kvm_err ("Failed to find VMA for hva 0x%lx\n" , hva); +up_read (¤t->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.
Quoted 3 lines
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()
Quoted 30 lines · Christoffer Dall
Quoted 29 lines · Ard Biesheuvel
Quoted 27 lines · Marc Zyngier
Quoted 13 lines · Ard Biesheuvel
Handle the potential NULL return value of
find_vma_intersection()before dereferencing it.Signed-off-by Ard Biesheuvel <[email protected]>--- arch/arm/kvm/mmu.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)arch/arm/kvm/mmu.c +0
-0arch/arm/kvm/mmu.c static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, · line 783
Quoted 9 lines
arch/arm/kvm/mmu.c +5
-1@@ -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 (¤t->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 (¤t->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.
Quoted 4 lines
Quoted 3 lines
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
when you do the fixup.
Thanks,
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
PATCH 3/6 arm/arm64: KVM: add 'writable' parameter to kvm_phys_addr_ioremap
In reply to Ard BiesheuvelAdd 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.
--- 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(-)
arch/arm/kvm/mmu.c +4 -1
@@ -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;
arch/arm64/include/asm/kvm_mmu.h +1 -1
@@ -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);
virt/kvm/arm/vgic.c +2 -1
@@ -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;
Signature · 1 line
1.8.3.2
PATCH 3/6 arm/arm64: KVM: add 'writable' parameter to kvm_phys_addr_ioremap
Is DEVICE type the correct default, for device regions that are prefetchable? The guest might want something less restrictive to optimize device access.
Quoted 64 lines · 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 ofPAGE_S2_DEVICEgets changed, those mappings will be created read-only.Signed-off-by Ard Biesheuvel <[email protected]>--- 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(-)arch/arm/kvm/mmu.c +4
-1@@ -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;arch/arm64/include/asm/kvm_mmu.h +1
-1@@ -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);virt/kvm/arm/vgic.c +2
-1@@ -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
Quoted 5 lines · 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.
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
Quoted 6 lines · Christoffer Dall
Quoted 5 lines · 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.
Quoted 8 lines
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
Quoted 16 lines · Mario Smarduch
Quoted 6 lines · Christoffer Dall
Quoted 5 lines · 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.
Quoted 8 lines
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
Quoted 9 lines
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
In reply to Ard BiesheuvelQuoted 66 lines · 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 ofPAGE_S2_DEVICEgets changed, those mappings will be created read-only.Signed-off-by Ard Biesheuvel <[email protected]>--- 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(-)arch/arm/kvm/mmu.c +4
-1@@ -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;arch/arm64/include/asm/kvm_mmu.h +1
-1@@ -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);virt/kvm/arm/vgic.c +2
-1@@ -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;Signature · 1 line
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
In reply to Ard BiesheuvelQuoted 41 lines · 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 ofPAGE_S2_DEVICEgets changed, those mappings will be created read-only.Signed-off-by Ard Biesheuvel <[email protected]>--- 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(-)arch/arm/kvm/mmu.c +4
-1@@ -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;arch/arm64/include/asm/kvm_mmu.h +0
-0
arch/arm64/include/asm/kvm_mmu.h void free_hyp_pgds(void); · line 81
Quoted 5 lines
arch/arm64/include/asm/kvm_mmu.h +1
-1@@ -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.
Quoted 9 lines
arch/arm64/include/asm/kvm_mmu.h +0
-0int kvm_handle_guest_abort (struct kvm_vcpu *vcpu,struct kvm_run *run);virt/kvm/arm/vgic.c +0
-0
virt/kvm/arm/vgic.c int kvm_vgic_init(struct kvm *kvm) · line 1635
Quoted 9 lines
virt/kvm/arm/vgic.c +2
-1@@ -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.
Signature · 1 line
Jazz is not dead. It just smells funny...
PATCH 3/6 arm/arm64: KVM: add 'writable' parameter to kvm_phys_addr_ioremap
Quoted 41 lines · Marc Zyngier
Quoted 40 lines · 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 ofPAGE_S2_DEVICEgets changed, those mappings will be created read-only.Signed-off-by Ard Biesheuvel <[email protected]>--- 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(-)arch/arm/kvm/mmu.c +4
-1@@ -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;arch/arm64/include/asm/kvm_mmu.h +0
-0
Quoted 9 lines
arch/arm64/include/asm/kvm_mmu.h void free_hyp_pgds(void); · line 81
Quoted 6 lines
arch/arm64/include/asm/kvm_mmu.h +1
-1@@ -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.
Quoted 25 lines
Quoted 9 lines
arch/arm64/include/asm/kvm_mmu.h +0
-0int kvm_handle_guest_abort (struct kvm_vcpu *vcpu,struct kvm_run *run);virt/kvm/arm/vgic.c +0
-0virt/kvm/arm/vgic.c int kvm_vgic_init(struct kvm *kvm) · line 1635
Quoted 9 lines
virt/kvm/arm/vgic.c +2
-1@@ -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
Quoted 52 lines · Ard Biesheuvel
Quoted 41 lines · Marc Zyngier
Quoted 40 lines · 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 ofPAGE_S2_DEVICEgets changed, those mappings will be created read-only.Signed-off-by Ard Biesheuvel <[email protected]>--- 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(-)arch/arm/kvm/mmu.c +4
-1@@ -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;arch/arm64/include/asm/kvm_mmu.h +0
-0Quoted 9 lines
arch/arm64/include/asm/kvm_mmu.h void free_hyp_pgds(void); · line 81
Quoted 6 lines
arch/arm64/include/asm/kvm_mmu.h +1
-1@@ -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.
Quoted 2 lines
Christoffer had spotted that as well, and indicated he would not mind fixing it up when applying.
Ah, missed that. In which case:
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
PATCH 4/6 ARM: kvm: define PAGE_S2_DEVICE as read-only by default
In reply to Ard BiesheuvelNow 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.
--- arch/arm/include/asm/pgtable.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/include/asm/pgtable.h +1 -1
@@ -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)
Signature · 1 line
1.8.3.2
PATCH 4/6 ARM: kvm: define PAGE_S2_DEVICE as read-only by default
Quoted 28 lines · 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 defaultpgprot_tvalue used for devices to bePTE_S2_RDONLY.Signed-off-by Ard Biesheuvel <[email protected]>--- arch/arm/include/asm/pgtable.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/arm/include/asm/pgtable.h +1
-1@@ -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)Signature · 1 line
1.8.3.2
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
In reply to Ard BiesheuvelQuoted 8 lines · 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 defaultpgprot_tvalue used for devices to bePTE_S2_RDONLY.Signed-off-by Ard Biesheuvel <[email protected]>
M.
Quoted 18 lines
--- arch/arm/include/asm/pgtable.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/arm/include/asm/pgtable.h +1
-1@@ -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)
Signature · 1 line
Jazz is not dead. It just smells funny...
PATCH 5/6 arm64: kvm: define PAGE_S2_DEVICE as read-only by default
In reply to Ard BiesheuvelNow 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.
--- arch/arm64/include/asm/pgtable.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm64/include/asm/pgtable.h +1 -1
@@ -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)
Signature · 1 line
1.8.3.2
PATCH 5/6 arm64: kvm: define PAGE_S2_DEVICE as read-only by default
Quoted 28 lines · 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 defaultpgprot_tvalue used for devices to bePTE_S2_RDONLY.Signed-off-by Ard Biesheuvel <[email protected]>--- arch/arm64/include/asm/pgtable.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/arm64/include/asm/pgtable.h +1
-1@@ -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)Signature · 1 line
1.8.3.2
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
Quoted 31 lines · Christoffer Dall
Quoted 28 lines · 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 defaultpgprot_tvalue used for devices to bePTE_S2_RDONLY.Signed-off-by Ard Biesheuvel <[email protected]>--- arch/arm64/include/asm/pgtable.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/arm64/include/asm/pgtable.h +1
-1@@ -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)Signature · 1 line
1.8.3.2Acked-by Christoffer Dall <[email protected]>
Quoted 3 lines
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
In reply to Ard BiesheuvelQuoted 8 lines · 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 defaultpgprot_tvalue used for devices to bePTE_S2_RDONLY.Signed-off-by Ard Biesheuvel <[email protected]>
Quoted 18 lines
--- arch/arm64/include/asm/pgtable.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/arm64/include/asm/pgtable.h +1
-1@@ -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)
Signature · 1 line
Jazz is not dead. It just smells funny...
PATCH 6/6 arm/arm64: KVM: map MMIO regions at creation time
In reply to Ard BiesheuvelThere 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.
--- arch/arm/kvm/mmu.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-)
arch/arm/kvm/mmu.c +49 -1
@@ -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)
Signature · 1 line
1.8.3.2
PATCH 6/6 arm/arm64: KVM: map MMIO regions at creation time
Quoted 73 lines · 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 <[email protected]>--- arch/arm/kvm/mmu.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-)arch/arm/kvm/mmu.c +49
-1@@ -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)Signature · 1 line
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
cover KVM: handling of MMIO pass-through regions
In reply to Ard BiesheuvelQuoted 23 lines · Ard Biesheuvel
This is a followup to the patches 'kvm: define
PAGE_S2_DEVICEas 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_DEVICEto read-only, so that read-only regions can be ioremap()'edPatch #6 ensures that
VM_PFNMAPlinear mappings of non-system RAM host memory are mapped eagerly rather than faulted in page by page.
Quoted 8 lines
Ard Biesheuvel (6):
arm/arm64: KVM: use__GFP_ZEROnotmemset()to get zeroed pages arm/arm64: KVM: fix potential NULL dereference inuser_mem_abort()arm/arm64: KVM: add 'writable' parameter tokvm_phys_addr_ioremap
ARM: kvm: definePAGE_S2_DEVICEas read-only by default
arm64: kvm: definePAGE_S2_DEVICEas 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
cover KVM: handling of MMIO pass-through regions
Quoted 33 lines · Christoffer Dall
Quoted 23 lines · Ard Biesheuvel
This is a followup to the patches 'kvm: define
PAGE_S2_DEVICEas 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_DEVICEto read-only, so that read-only regions can be ioremap()'edPatch #6 ensures that
VM_PFNMAPlinear mappings of non-system RAM host memory are mapped eagerly rather than faulted in page by page.Quoted 8 lines
Ard Biesheuvel (6):
arm/arm64: KVM: use__GFP_ZEROnotmemset()to get zeroed pages arm/arm64: KVM: fix potential NULL dereference inuser_mem_abort()arm/arm64: KVM: add 'writable' parameter tokvm_phys_addr_ioremap
ARM: kvm: definePAGE_S2_DEVICEas read-only by default
arm64: kvm: definePAGE_S2_DEVICEas read-only by default arm/arm64: KVM: map MMIO regions at creation time
Quoted 5 lines
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.
Signature · 1 line
Ard.
Christoffer Dall
Appliedcover KVM: handling of MMIO pass-through regions
In reply to Ard BiesheuvelQuoted 15 lines · Ard Biesheuvel
This is a followup to the patches 'kvm: define
PAGE_S2_DEVICEas 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.
Quoted 8 lines
Patch #4 and #5 were sent out before, and change the value of
PAGE_S2_DEVICEto read-only, so that read-only regions can be ioremap()'edPatch #6 ensures that
VM_PFNMAPlinear 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