* [PATCH] KVM: s390: fix memory slot handling for KVM_SET_USER_MEMORY_REGION
@ 2019-05-24 14:06 Christian Borntraeger
2019-05-24 19:21 ` Paolo Bonzini
2019-05-27 7:39 ` Thomas Huth
0 siblings, 2 replies; 4+ messages in thread
From: Christian Borntraeger @ 2019-05-24 14:06 UTC (permalink / raw)
To: Janosch Frank
Cc: KVM, Cornelia Huck, Christian Borntraeger, David Hildenbrand,
Paolo Bonzini, Radim Krčmář,
Shuah Khan, Andrew Jones, linux-kselftest, linux-s390
kselftests exposed a problem in the s390 handling for memory slots.
Right now we only do proper memory slot handling for creation of new
memory slots. Neither MOVE, nor DELETION are handled properly. Let us
implement those.
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
arch/s390/kvm/kvm-s390.c | 35 +++++++++++++++++++++--------------
1 file changed, 21 insertions(+), 14 deletions(-)
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 871d2e99b156..6ec0685ab2c7 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -4525,21 +4525,28 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
const struct kvm_memory_slot *new,
enum kvm_mr_change change)
{
- int rc;
+ int rc = 0;
- /* If the basics of the memslot do not change, we do not want
- * to update the gmap. Every update causes several unnecessary
- * segment translation exceptions. This is usually handled just
- * fine by the normal fault handler + gmap, but it will also
- * cause faults on the prefix page of running guest CPUs.
- */
- if (old->userspace_addr == mem->userspace_addr &&
- old->base_gfn * PAGE_SIZE == mem->guest_phys_addr &&
- old->npages * PAGE_SIZE == mem->memory_size)
- return;
-
- rc = gmap_map_segment(kvm->arch.gmap, mem->userspace_addr,
- mem->guest_phys_addr, mem->memory_size);
+ switch (change) {
+ case KVM_MR_DELETE:
+ rc = gmap_unmap_segment(kvm->arch.gmap, old->base_gfn * PAGE_SIZE,
+ old->npages * PAGE_SIZE);
+ break;
+ case KVM_MR_MOVE:
+ rc = gmap_unmap_segment(kvm->arch.gmap, old->base_gfn * PAGE_SIZE,
+ old->npages * PAGE_SIZE);
+ if (rc)
+ break;
+ /* FALLTHROUGH */
+ case KVM_MR_CREATE:
+ rc = gmap_map_segment(kvm->arch.gmap, mem->userspace_addr,
+ mem->guest_phys_addr, mem->memory_size);
+ break;
+ case KVM_MR_FLAGS_ONLY:
+ break;
+ default:
+ WARN(1, "Unknown KVM MR CHANGE: %d\n", change);
+ }
if (rc)
pr_warn("failed to commit memory region\n");
return;
--
2.21.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] KVM: s390: fix memory slot handling for KVM_SET_USER_MEMORY_REGION
2019-05-24 14:06 [PATCH] KVM: s390: fix memory slot handling for KVM_SET_USER_MEMORY_REGION Christian Borntraeger
@ 2019-05-24 19:21 ` Paolo Bonzini
2019-05-27 7:39 ` Thomas Huth
1 sibling, 0 replies; 4+ messages in thread
From: Paolo Bonzini @ 2019-05-24 19:21 UTC (permalink / raw)
To: Christian Borntraeger, Janosch Frank
Cc: KVM, Cornelia Huck, David Hildenbrand,
Radim Krčmář,
Shuah Khan, Andrew Jones, linux-kselftest, linux-s390
On 24/05/19 16:06, Christian Borntraeger wrote:
> kselftests exposed a problem in the s390 handling for memory slots.
> Right now we only do proper memory slot handling for creation of new
> memory slots. Neither MOVE, nor DELETION are handled properly. Let us
> implement those.
>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
> arch/s390/kvm/kvm-s390.c | 35 +++++++++++++++++++++--------------
> 1 file changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 871d2e99b156..6ec0685ab2c7 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -4525,21 +4525,28 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
> const struct kvm_memory_slot *new,
> enum kvm_mr_change change)
> {
> - int rc;
> + int rc = 0;
>
> - /* If the basics of the memslot do not change, we do not want
> - * to update the gmap. Every update causes several unnecessary
> - * segment translation exceptions. This is usually handled just
> - * fine by the normal fault handler + gmap, but it will also
> - * cause faults on the prefix page of running guest CPUs.
> - */
> - if (old->userspace_addr == mem->userspace_addr &&
> - old->base_gfn * PAGE_SIZE == mem->guest_phys_addr &&
> - old->npages * PAGE_SIZE == mem->memory_size)
> - return;
> -
> - rc = gmap_map_segment(kvm->arch.gmap, mem->userspace_addr,
> - mem->guest_phys_addr, mem->memory_size);
> + switch (change) {
> + case KVM_MR_DELETE:
> + rc = gmap_unmap_segment(kvm->arch.gmap, old->base_gfn * PAGE_SIZE,
> + old->npages * PAGE_SIZE);
> + break;
> + case KVM_MR_MOVE:
> + rc = gmap_unmap_segment(kvm->arch.gmap, old->base_gfn * PAGE_SIZE,
> + old->npages * PAGE_SIZE);
> + if (rc)
> + break;
> + /* FALLTHROUGH */
> + case KVM_MR_CREATE:
> + rc = gmap_map_segment(kvm->arch.gmap, mem->userspace_addr,
> + mem->guest_phys_addr, mem->memory_size);
> + break;
> + case KVM_MR_FLAGS_ONLY:
> + break;
> + default:
> + WARN(1, "Unknown KVM MR CHANGE: %d\n", change);
> + }
> if (rc)
> pr_warn("failed to commit memory region\n");
> return;
>
Queued for 5.2-rc2, thanks.
Paolo
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] KVM: s390: fix memory slot handling for KVM_SET_USER_MEMORY_REGION
2019-05-24 14:06 [PATCH] KVM: s390: fix memory slot handling for KVM_SET_USER_MEMORY_REGION Christian Borntraeger
2019-05-24 19:21 ` Paolo Bonzini
@ 2019-05-27 7:39 ` Thomas Huth
2019-05-27 8:04 ` Christian Borntraeger
1 sibling, 1 reply; 4+ messages in thread
From: Thomas Huth @ 2019-05-27 7:39 UTC (permalink / raw)
To: Christian Borntraeger, Janosch Frank
Cc: KVM, Cornelia Huck, David Hildenbrand, Paolo Bonzini,
Radim Krčmář,
Shuah Khan, Andrew Jones, linux-kselftest, linux-s390
On 24/05/2019 16.06, Christian Borntraeger wrote:
> kselftests exposed a problem in the s390 handling for memory slots.
> Right now we only do proper memory slot handling for creation of new
> memory slots. Neither MOVE, nor DELETION are handled properly. Let us
> implement those.
>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
> arch/s390/kvm/kvm-s390.c | 35 +++++++++++++++++++++--------------
> 1 file changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 871d2e99b156..6ec0685ab2c7 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -4525,21 +4525,28 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
> const struct kvm_memory_slot *new,
> enum kvm_mr_change change)
> {
> - int rc;
> + int rc = 0;
>
> - /* If the basics of the memslot do not change, we do not want
> - * to update the gmap. Every update causes several unnecessary
> - * segment translation exceptions. This is usually handled just
> - * fine by the normal fault handler + gmap, but it will also
> - * cause faults on the prefix page of running guest CPUs.
> - */
> - if (old->userspace_addr == mem->userspace_addr &&
> - old->base_gfn * PAGE_SIZE == mem->guest_phys_addr &&
> - old->npages * PAGE_SIZE == mem->memory_size)
> - return;
This check is now gone in the new code. Maybe mention the reason in the
patch description?
> - rc = gmap_map_segment(kvm->arch.gmap, mem->userspace_addr,
> - mem->guest_phys_addr, mem->memory_size);
> + switch (change) {
> + case KVM_MR_DELETE:
> + rc = gmap_unmap_segment(kvm->arch.gmap, old->base_gfn * PAGE_SIZE,
> + old->npages * PAGE_SIZE);
> + break;
> + case KVM_MR_MOVE:
> + rc = gmap_unmap_segment(kvm->arch.gmap, old->base_gfn * PAGE_SIZE,
> + old->npages * PAGE_SIZE);
> + if (rc)
> + break;
> + /* FALLTHROUGH */
> + case KVM_MR_CREATE:
> + rc = gmap_map_segment(kvm->arch.gmap, mem->userspace_addr,
> + mem->guest_phys_addr, mem->memory_size);
> + break;
> + case KVM_MR_FLAGS_ONLY:
> + break;
> + default:
> + WARN(1, "Unknown KVM MR CHANGE: %d\n", change);
> + }
> if (rc)
> pr_warn("failed to commit memory region\n");
> return;
>
Looks good to me, but I'm not an expert in this area, so FWIW a weak:
Reviewed-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Re: [PATCH] KVM: s390: fix memory slot handling for KVM_SET_USER_MEMORY_REGION
2019-05-27 7:39 ` Thomas Huth
@ 2019-05-27 8:04 ` Christian Borntraeger
0 siblings, 0 replies; 4+ messages in thread
From: Christian Borntraeger @ 2019-05-27 8:04 UTC (permalink / raw)
To: Thomas Huth, Janosch Frank
Cc: KVM, Cornelia Huck, David Hildenbrand, Paolo Bonzini,
Radim Krčmář,
Shuah Khan, Andrew Jones, linux-kselftest, linux-s390
On 27.05.19 09:39, Thomas Huth wrote:
> On 24/05/2019 16.06, Christian Borntraeger wrote:
>> kselftests exposed a problem in the s390 handling for memory slots.
>> Right now we only do proper memory slot handling for creation of new
>> memory slots. Neither MOVE, nor DELETION are handled properly. Let us
>> implement those.
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>> arch/s390/kvm/kvm-s390.c | 35 +++++++++++++++++++++--------------
>> 1 file changed, 21 insertions(+), 14 deletions(-)
>>
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index 871d2e99b156..6ec0685ab2c7 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -4525,21 +4525,28 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
>> const struct kvm_memory_slot *new,
>> enum kvm_mr_change change)
>> {
>> - int rc;
>> + int rc = 0;
>>
>> - /* If the basics of the memslot do not change, we do not want
>> - * to update the gmap. Every update causes several unnecessary
>> - * segment translation exceptions. This is usually handled just
>> - * fine by the normal fault handler + gmap, but it will also
>> - * cause faults on the prefix page of running guest CPUs.
>> - */
>> - if (old->userspace_addr == mem->userspace_addr &&
>> - old->base_gfn * PAGE_SIZE == mem->guest_phys_addr &&
>> - old->npages * PAGE_SIZE == mem->memory_size)
>> - return;
>
> This check is now gone in the new code. Maybe mention the reason in the
> patch description?
Yes, all these checks are done in common code (and they result in the KVM_MR types).
>
>> - rc = gmap_map_segment(kvm->arch.gmap, mem->userspace_addr,
>> - mem->guest_phys_addr, mem->memory_size);
>> + switch (change) {
>> + case KVM_MR_DELETE:
>> + rc = gmap_unmap_segment(kvm->arch.gmap, old->base_gfn * PAGE_SIZE,
>> + old->npages * PAGE_SIZE);
>> + break;
>> + case KVM_MR_MOVE:
>> + rc = gmap_unmap_segment(kvm->arch.gmap, old->base_gfn * PAGE_SIZE,
>> + old->npages * PAGE_SIZE);
>> + if (rc)
>> + break;
>> + /* FALLTHROUGH */
>> + case KVM_MR_CREATE:
>> + rc = gmap_map_segment(kvm->arch.gmap, mem->userspace_addr,
>> + mem->guest_phys_addr, mem->memory_size);
>> + break;
>> + case KVM_MR_FLAGS_ONLY:
>> + break;
>> + default:
>> + WARN(1, "Unknown KVM MR CHANGE: %d\n", change);
>> + }
>> if (rc)
>> pr_warn("failed to commit memory region\n");
>> return;
>>
>
> Looks good to me, but I'm not an expert in this area, so FWIW a weak:
>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-05-27 8:05 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-24 14:06 [PATCH] KVM: s390: fix memory slot handling for KVM_SET_USER_MEMORY_REGION Christian Borntraeger
2019-05-24 19:21 ` Paolo Bonzini
2019-05-27 7:39 ` Thomas Huth
2019-05-27 8:04 ` Christian Borntraeger
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).