[PATCH] KVM: Mapping IOMMU pages after updating memslot

[PATCH] KVM: Mapping IOMMU pages after updating memslot

[Yang Zhang]

From: Yang Zhang <yang.z.zhang@Intel.com>

In kvm_iommu_map_pages(), we need to know the page size via call
kvm_host_page_size(). And it will check whether the target slot
is valid before return the right page size.
Currently, we will map the iommu pages when creating a new slot.
But we call kvm_iommu_map_pages() during preparing the new slot.
At that time, the new slot is not visible by domain(still in preparing).
So we cannot get the right page size from kvm_host_page_size() and
this will break the IOMMU super page logic.
The solution is to map the iommu pages after we insert the new slot
into domain.

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
Tested-by: Patrick Lu <patrick.lu@intel.com>
---
 virt/kvm/kvm_main.c |   29 ++++++++++++++---------------
 1 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index d469114..9ec60a2 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -873,21 +873,6 @@ int __kvm_set_memory_region(struct kvm *kvm,
 			goto out_free;
 	}
 
-	/*
-	 * IOMMU mapping:  New slots need to be mapped.  Old slots need to be
-	 * un-mapped and re-mapped if their base changes.  Since base change
-	 * unmapping is handled above with slot deletion, mapping alone is
-	 * needed here.  Anything else the iommu might care about for existing
-	 * slots (size changes, userspace addr changes and read-only flag
-	 * changes) is disallowed above, so any other attribute changes getting
-	 * here can be skipped.
-	 */
-	if ((change == KVM_MR_CREATE) || (change == KVM_MR_MOVE)) {
-		r = kvm_iommu_map_pages(kvm, &new);
-		if (r)
-			goto out_slots;
-	}
-
 	/* actual memory is freed via old in kvm_free_physmem_slot below */
 	if (change == KVM_MR_DELETE) {
 		new.dirty_bitmap = NULL;
@@ -901,6 +886,20 @@ int __kvm_set_memory_region(struct kvm *kvm,
 	kvm_free_physmem_slot(&old, &new);
 	kfree(old_memslots);
 
+	/*
+	 * IOMMU mapping:  New slots need to be mapped.  Old slots need to be
+	 * un-mapped and re-mapped if their base changes.  Since base change
+	 * unmapping is handled above with slot deletion, mapping alone is
+	 * needed here.  Anything else the iommu might care about for existing
+	 * slots (size changes, userspace addr changes and read-only flag
+	 * changes) is disallowed above, so any other attribute changes getting
+	 * here can be skipped.
+	 */
+	if ((change == KVM_MR_CREATE) || (change == KVM_MR_MOVE)) {
+		r = kvm_iommu_map_pages(kvm, &new);
+		return r;
+	}
+
 	return 0;
 
 out_slots:
-- 
1.7.1

Re: [PATCH] KVM: Mapping IOMMU pages after updating memslot

[Paolo Bonzini]

Il 24/10/2013 03:56, Yang Zhang ha scritto:
> From: Yang Zhang <yang.z.zhang@Intel.com>
> 
> In kvm_iommu_map_pages(), we need to know the page size via call
> kvm_host_page_size(). And it will check whether the target slot
> is valid before return the right page size.
> Currently, we will map the iommu pages when creating a new slot.
> But we call kvm_iommu_map_pages() during preparing the new slot.
> At that time, the new slot is not visible by domain(still in preparing).
> So we cannot get the right page size from kvm_host_page_size() and
> this will break the IOMMU super page logic.
> The solution is to map the iommu pages after we insert the new slot
> into domain.
> 
> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> Tested-by: Patrick Lu <patrick.lu@intel.com>
> ---
>  virt/kvm/kvm_main.c |   29 ++++++++++++++---------------
>  1 files changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index d469114..9ec60a2 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -873,21 +873,6 @@ int __kvm_set_memory_region(struct kvm *kvm,
>  			goto out_free;
>  	}
>  
> -	/*
> -	 * IOMMU mapping:  New slots need to be mapped.  Old slots need to be
> -	 * un-mapped and re-mapped if their base changes.  Since base change
> -	 * unmapping is handled above with slot deletion, mapping alone is
> -	 * needed here.  Anything else the iommu might care about for existing
> -	 * slots (size changes, userspace addr changes and read-only flag
> -	 * changes) is disallowed above, so any other attribute changes getting
> -	 * here can be skipped.
> -	 */
> -	if ((change == KVM_MR_CREATE) || (change == KVM_MR_MOVE)) {
> -		r = kvm_iommu_map_pages(kvm, &new);
> -		if (r)
> -			goto out_slots;
> -	}
> -
>  	/* actual memory is freed via old in kvm_free_physmem_slot below */
>  	if (change == KVM_MR_DELETE) {
>  		new.dirty_bitmap = NULL;
> @@ -901,6 +886,20 @@ int __kvm_set_memory_region(struct kvm *kvm,
>  	kvm_free_physmem_slot(&old, &new);
>  	kfree(old_memslots);
>  
> +	/*
> +	 * IOMMU mapping:  New slots need to be mapped.  Old slots need to be
> +	 * un-mapped and re-mapped if their base changes.  Since base change
> +	 * unmapping is handled above with slot deletion, mapping alone is
> +	 * needed here.  Anything else the iommu might care about for existing
> +	 * slots (size changes, userspace addr changes and read-only flag
> +	 * changes) is disallowed above, so any other attribute changes getting
> +	 * here can be skipped.
> +	 */
> +	if ((change == KVM_MR_CREATE) || (change == KVM_MR_MOVE)) {
> +		r = kvm_iommu_map_pages(kvm, &new);
> +		return r;
> +	}
> +
>  	return 0;
>  
>  out_slots:
> 

Applied to kvm.git queue, thanks.

Paolo

Re: [PATCH] KVM: Mapping IOMMU pages after updating memslot

[Alex Williamson]

On Thu, 2013-10-24 at 09:56 +0800, Yang Zhang wrote:
> From: Yang Zhang <yang.z.zhang@Intel.com>
> 
> In kvm_iommu_map_pages(), we need to know the page size via call
> kvm_host_page_size(). And it will check whether the target slot
> is valid before return the right page size.
> Currently, we will map the iommu pages when creating a new slot.
> But we call kvm_iommu_map_pages() during preparing the new slot.
> At that time, the new slot is not visible by domain(still in preparing).
> So we cannot get the right page size from kvm_host_page_size() and
> this will break the IOMMU super page logic.
> The solution is to map the iommu pages after we insert the new slot
> into domain.
> 
> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> Tested-by: Patrick Lu <patrick.lu@intel.com>
> ---
>  virt/kvm/kvm_main.c |   29 ++++++++++++++---------------
>  1 files changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index d469114..9ec60a2 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -873,21 +873,6 @@ int __kvm_set_memory_region(struct kvm *kvm,
>  			goto out_free;
>  	}
>  
> -	/*
> -	 * IOMMU mapping:  New slots need to be mapped.  Old slots need to be
> -	 * un-mapped and re-mapped if their base changes.  Since base change
> -	 * unmapping is handled above with slot deletion, mapping alone is
> -	 * needed here.  Anything else the iommu might care about for existing
> -	 * slots (size changes, userspace addr changes and read-only flag
> -	 * changes) is disallowed above, so any other attribute changes getting
> -	 * here can be skipped.
> -	 */
> -	if ((change == KVM_MR_CREATE) || (change == KVM_MR_MOVE)) {
> -		r = kvm_iommu_map_pages(kvm, &new);
> -		if (r)
> -			goto out_slots;
> -	}
> -
>  	/* actual memory is freed via old in kvm_free_physmem_slot below */
>  	if (change == KVM_MR_DELETE) {
>  		new.dirty_bitmap = NULL;
> @@ -901,6 +886,20 @@ int __kvm_set_memory_region(struct kvm *kvm,
>  	kvm_free_physmem_slot(&old, &new);
>  	kfree(old_memslots);
>  
> +	/*
> +	 * IOMMU mapping:  New slots need to be mapped.  Old slots need to be
> +	 * un-mapped and re-mapped if their base changes.  Since base change
> +	 * unmapping is handled above with slot deletion, mapping alone is
> +	 * needed here.  Anything else the iommu might care about for existing
> +	 * slots (size changes, userspace addr changes and read-only flag
> +	 * changes) is disallowed above, so any other attribute changes getting
> +	 * here can be skipped.
> +	 */
> +	if ((change == KVM_MR_CREATE) || (change == KVM_MR_MOVE)) {
> +		r = kvm_iommu_map_pages(kvm, &new);
> +		return r;
> +	}
> +

An addition to the comment noting that this needs to be done after
install/commit would be helpful.  Thanks,

Alex

>  	return 0;
>  
>  out_slots: