* [PATCH v1 0/4] s390x: one fix and three cleanups
@ 2017-08-30 16:05 David Hildenbrand
2017-08-30 16:06 ` [PATCH v1 1/4] KVM: s390: guestdbg: fix range check David Hildenbrand
` (3 more replies)
0 siblings, 4 replies; 17+ messages in thread
From: David Hildenbrand @ 2017-08-30 16:05 UTC (permalink / raw)
To: kvm
Cc: Christian Borntraeger, Paolo Bonzini, Radim Krčmář,
david, Cornelia Huck
We never ran into the range check case, as Linux guest never use
overflowing intervals.
vsie part smoke tested with a vsie setup under lpar.
David Hildenbrand (4):
KVM: s390: guestdbg: fix range check
KVM: s390: use WARN_ON_ONCE only for checking
KVM: s390: vsie: cleanup mcck reinjection
KVM: s390: vsie: use common code functions for pinning
arch/s390/kvm/guestdbg.c | 2 +-
arch/s390/kvm/interrupt.c | 6 ++++--
arch/s390/kvm/vsie.c | 25 ++++++-------------------
include/linux/kvm_host.h | 1 +
virt/kvm/kvm_main.c | 4 ++--
5 files changed, 14 insertions(+), 24 deletions(-)
--
2.13.5
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v1 1/4] KVM: s390: guestdbg: fix range check
2017-08-30 16:05 [PATCH v1 0/4] s390x: one fix and three cleanups David Hildenbrand
@ 2017-08-30 16:06 ` David Hildenbrand
2017-08-31 10:13 ` Cornelia Huck
2017-08-31 10:36 ` Christian Borntraeger
2017-08-30 16:06 ` [PATCH v1 2/4] KVM: s390: use WARN_ON_ONCE only for checking David Hildenbrand
` (2 subsequent siblings)
3 siblings, 2 replies; 17+ messages in thread
From: David Hildenbrand @ 2017-08-30 16:06 UTC (permalink / raw)
To: kvm
Cc: Christian Borntraeger, Paolo Bonzini, Radim Krčmář,
david, Cornelia Huck
Looks like the "overflowing" range check is wrong.
|=======b-------a=======|
addr >= a || addr <= b
Signed-off-by: David Hildenbrand <david@redhat.com>
---
arch/s390/kvm/guestdbg.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
index c2e0ddc1356e..bcbd86621d01 100644
--- a/arch/s390/kvm/guestdbg.c
+++ b/arch/s390/kvm/guestdbg.c
@@ -308,7 +308,7 @@ static inline int in_addr_range(u64 addr, u64 a, u64 b)
return (addr >= a) && (addr <= b);
else
/* "overflowing" interval */
- return (addr <= a) && (addr >= b);
+ return (addr >= a) || (addr <= b);
}
#define end_of_range(bp_info) (bp_info->addr + bp_info->len - 1)
--
2.13.5
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v1 2/4] KVM: s390: use WARN_ON_ONCE only for checking
2017-08-30 16:05 [PATCH v1 0/4] s390x: one fix and three cleanups David Hildenbrand
2017-08-30 16:06 ` [PATCH v1 1/4] KVM: s390: guestdbg: fix range check David Hildenbrand
@ 2017-08-30 16:06 ` David Hildenbrand
2017-08-31 10:16 ` Cornelia Huck
2017-08-31 10:55 ` Christian Borntraeger
2017-08-30 16:06 ` [PATCH v1 3/4] KVM: s390: vsie: cleanup mcck reinjection David Hildenbrand
2017-08-30 16:06 ` [PATCH v1 4/4] KVM: s390: vsie: use common code functions for pinning David Hildenbrand
3 siblings, 2 replies; 17+ messages in thread
From: David Hildenbrand @ 2017-08-30 16:06 UTC (permalink / raw)
To: kvm
Cc: Christian Borntraeger, Paolo Bonzini, Radim Krčmář,
david, Cornelia Huck
Move the real logic that always has to be executed out of the
WARN_ON_ONCE.
Signed-off-by: David Hildenbrand <david@redhat.com>
---
arch/s390/kvm/interrupt.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index a619ddae610d..a832ad031cee 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -2479,6 +2479,7 @@ void kvm_s390_reinject_machine_check(struct kvm_vcpu *vcpu,
struct kvm_s390_mchk_info *mchk;
union mci mci;
__u64 cr14 = 0; /* upper bits are not used */
+ int rc;
mci.val = mcck_info->mcic;
if (mci.sr)
@@ -2496,12 +2497,13 @@ void kvm_s390_reinject_machine_check(struct kvm_vcpu *vcpu,
if (mci.ck) {
/* Inject the floating machine check */
inti.type = KVM_S390_MCHK;
- WARN_ON_ONCE(__inject_vm(vcpu->kvm, &inti));
+ rc = __inject_vm(vcpu->kvm, &inti);
} else {
/* Inject the machine check to specified vcpu */
irq.type = KVM_S390_MCHK;
- WARN_ON_ONCE(kvm_s390_inject_vcpu(vcpu, &irq));
+ rc = kvm_s390_inject_vcpu(vcpu, &irq);
}
+ WARN_ON_ONCE(rc);
}
int kvm_set_routing_entry(struct kvm *kvm,
--
2.13.5
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v1 3/4] KVM: s390: vsie: cleanup mcck reinjection
2017-08-30 16:05 [PATCH v1 0/4] s390x: one fix and three cleanups David Hildenbrand
2017-08-30 16:06 ` [PATCH v1 1/4] KVM: s390: guestdbg: fix range check David Hildenbrand
2017-08-30 16:06 ` [PATCH v1 2/4] KVM: s390: use WARN_ON_ONCE only for checking David Hildenbrand
@ 2017-08-30 16:06 ` David Hildenbrand
2017-08-31 11:01 ` Christian Borntraeger
2017-08-30 16:06 ` [PATCH v1 4/4] KVM: s390: vsie: use common code functions for pinning David Hildenbrand
3 siblings, 1 reply; 17+ messages in thread
From: David Hildenbrand @ 2017-08-30 16:06 UTC (permalink / raw)
To: kvm
Cc: Christian Borntraeger, Paolo Bonzini, Radim Krčmář,
david, Cornelia Huck
Signed-off-by: David Hildenbrand <david@redhat.com>
---
arch/s390/kvm/vsie.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
index 715c19c45d9a..b5eec30eb37d 100644
--- a/arch/s390/kvm/vsie.c
+++ b/arch/s390/kvm/vsie.c
@@ -806,8 +806,6 @@ static int do_vsie_run(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
{
struct kvm_s390_sie_block *scb_s = &vsie_page->scb_s;
struct kvm_s390_sie_block *scb_o = vsie_page->scb_o;
- struct mcck_volatile_info *mcck_info;
- struct sie_page *sie_page;
int rc;
handle_last_fault(vcpu, vsie_page);
@@ -831,9 +829,7 @@ static int do_vsie_run(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
if (rc == -EINTR) {
VCPU_EVENT(vcpu, 3, "%s", "machine check");
- sie_page = container_of(scb_s, struct sie_page, sie_block);
- mcck_info = &sie_page->mcck_info;
- kvm_s390_reinject_machine_check(vcpu, mcck_info);
+ kvm_s390_reinject_machine_check(vcpu, &vsie_page->mcck_info);
return 0;
}
--
2.13.5
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v1 4/4] KVM: s390: vsie: use common code functions for pinning
2017-08-30 16:05 [PATCH v1 0/4] s390x: one fix and three cleanups David Hildenbrand
` (2 preceding siblings ...)
2017-08-30 16:06 ` [PATCH v1 3/4] KVM: s390: vsie: cleanup mcck reinjection David Hildenbrand
@ 2017-08-30 16:06 ` David Hildenbrand
2017-08-31 11:44 ` Christian Borntraeger
3 siblings, 1 reply; 17+ messages in thread
From: David Hildenbrand @ 2017-08-30 16:06 UTC (permalink / raw)
To: kvm
Cc: Christian Borntraeger, Paolo Bonzini, Radim Krčmář,
david, Cornelia Huck
Signed-off-by: David Hildenbrand <david@redhat.com>
---
arch/s390/kvm/vsie.c | 19 +++++--------------
include/linux/kvm_host.h | 1 +
virt/kvm/kvm_main.c | 4 ++--
3 files changed, 8 insertions(+), 16 deletions(-)
diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
index b5eec30eb37d..a21763b4c229 100644
--- a/arch/s390/kvm/vsie.c
+++ b/arch/s390/kvm/vsie.c
@@ -445,17 +445,12 @@ static int map_prefix(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
static int pin_guest_page(struct kvm *kvm, gpa_t gpa, hpa_t *hpa)
{
struct page *page;
- hva_t hva;
- int rc;
- hva = gfn_to_hva(kvm, gpa_to_gfn(gpa));
- if (kvm_is_error_hva(hva))
- return -EINVAL;
- rc = get_user_pages_fast(hva, 1, 1, &page);
- if (rc < 0)
- return rc;
- else if (rc != 1)
+ page = gfn_to_page(kvm, gpa_to_gfn(gpa));
+ if (PTR_ERR(page) == -ENOMEM)
return -ENOMEM;
+ if (is_error_page(page))
+ return -EINVAL;
*hpa = (hpa_t) page_to_virt(page) + (gpa & ~PAGE_MASK);
return 0;
}
@@ -463,11 +458,7 @@ static int pin_guest_page(struct kvm *kvm, gpa_t gpa, hpa_t *hpa)
/* Unpins a page previously pinned via pin_guest_page, marking it as dirty. */
static void unpin_guest_page(struct kvm *kvm, gpa_t gpa, hpa_t hpa)
{
- struct page *page;
-
- page = virt_to_page(hpa);
- set_page_dirty_lock(page);
- put_page(page);
+ kvm_release_pfn_dirty(hpa >> PAGE_SHIFT);
/* mark the page always as dirty for migration */
mark_page_dirty(kvm, gpa_to_gfn(gpa));
}
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 6882538eda32..2e754b7c282c 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -667,6 +667,7 @@ kvm_pfn_t __gfn_to_pfn_memslot(struct kvm_memory_slot *slot, gfn_t gfn,
bool *writable);
void kvm_release_pfn_clean(kvm_pfn_t pfn);
+void kvm_release_pfn_dirty(kvm_pfn_t pfn);
void kvm_set_pfn_dirty(kvm_pfn_t pfn);
void kvm_set_pfn_accessed(kvm_pfn_t pfn);
void kvm_get_pfn(kvm_pfn_t pfn);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 1b3fa3fc1a78..dc422a007b25 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -122,7 +122,6 @@ static void hardware_disable_all(void);
static void kvm_io_bus_destroy(struct kvm_io_bus *bus);
-static void kvm_release_pfn_dirty(kvm_pfn_t pfn);
static void mark_page_dirty_in_slot(struct kvm_memory_slot *memslot, gfn_t gfn);
__visible bool kvm_rebooting;
@@ -1720,11 +1719,12 @@ void kvm_release_page_dirty(struct page *page)
}
EXPORT_SYMBOL_GPL(kvm_release_page_dirty);
-static void kvm_release_pfn_dirty(kvm_pfn_t pfn)
+void kvm_release_pfn_dirty(kvm_pfn_t pfn)
{
kvm_set_pfn_dirty(pfn);
kvm_release_pfn_clean(pfn);
}
+EXPORT_SYMBOL_GPL(kvm_release_pfn_dirty);
void kvm_set_pfn_dirty(kvm_pfn_t pfn)
{
--
2.13.5
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v1 1/4] KVM: s390: guestdbg: fix range check
2017-08-30 16:06 ` [PATCH v1 1/4] KVM: s390: guestdbg: fix range check David Hildenbrand
@ 2017-08-31 10:13 ` Cornelia Huck
2017-08-31 10:36 ` Christian Borntraeger
1 sibling, 0 replies; 17+ messages in thread
From: Cornelia Huck @ 2017-08-31 10:13 UTC (permalink / raw)
To: David Hildenbrand
Cc: kvm, Christian Borntraeger, Paolo Bonzini, Radim Krčmář
On Wed, 30 Aug 2017 18:06:00 +0200
David Hildenbrand <david@redhat.com> wrote:
> Looks like the "overflowing" range check is wrong.
>
> |=======b-------a=======|
>
> addr >= a || addr <= b
Huh :)
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> arch/s390/kvm/guestdbg.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
> index c2e0ddc1356e..bcbd86621d01 100644
> --- a/arch/s390/kvm/guestdbg.c
> +++ b/arch/s390/kvm/guestdbg.c
> @@ -308,7 +308,7 @@ static inline int in_addr_range(u64 addr, u64 a, u64 b)
> return (addr >= a) && (addr <= b);
> else
> /* "overflowing" interval */
> - return (addr <= a) && (addr >= b);
> + return (addr >= a) || (addr <= b);
> }
>
> #define end_of_range(bp_info) (bp_info->addr + bp_info->len - 1)
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v1 2/4] KVM: s390: use WARN_ON_ONCE only for checking
2017-08-30 16:06 ` [PATCH v1 2/4] KVM: s390: use WARN_ON_ONCE only for checking David Hildenbrand
@ 2017-08-31 10:16 ` Cornelia Huck
2017-08-31 10:55 ` Christian Borntraeger
1 sibling, 0 replies; 17+ messages in thread
From: Cornelia Huck @ 2017-08-31 10:16 UTC (permalink / raw)
To: David Hildenbrand
Cc: kvm, Christian Borntraeger, Paolo Bonzini, Radim Krčmář
On Wed, 30 Aug 2017 18:06:01 +0200
David Hildenbrand <david@redhat.com> wrote:
> Move the real logic that always has to be executed out of the
> WARN_ON_ONCE.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> arch/s390/kvm/interrupt.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index a619ddae610d..a832ad031cee 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -2479,6 +2479,7 @@ void kvm_s390_reinject_machine_check(struct kvm_vcpu *vcpu,
> struct kvm_s390_mchk_info *mchk;
> union mci mci;
> __u64 cr14 = 0; /* upper bits are not used */
> + int rc;
>
> mci.val = mcck_info->mcic;
> if (mci.sr)
> @@ -2496,12 +2497,13 @@ void kvm_s390_reinject_machine_check(struct kvm_vcpu *vcpu,
> if (mci.ck) {
> /* Inject the floating machine check */
> inti.type = KVM_S390_MCHK;
> - WARN_ON_ONCE(__inject_vm(vcpu->kvm, &inti));
> + rc = __inject_vm(vcpu->kvm, &inti);
> } else {
> /* Inject the machine check to specified vcpu */
> irq.type = KVM_S390_MCHK;
> - WARN_ON_ONCE(kvm_s390_inject_vcpu(vcpu, &irq));
> + rc = kvm_s390_inject_vcpu(vcpu, &irq);
> }
> + WARN_ON_ONCE(rc);
> }
>
> int kvm_set_routing_entry(struct kvm *kvm,
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v1 1/4] KVM: s390: guestdbg: fix range check
2017-08-30 16:06 ` [PATCH v1 1/4] KVM: s390: guestdbg: fix range check David Hildenbrand
2017-08-31 10:13 ` Cornelia Huck
@ 2017-08-31 10:36 ` Christian Borntraeger
1 sibling, 0 replies; 17+ messages in thread
From: Christian Borntraeger @ 2017-08-31 10:36 UTC (permalink / raw)
To: David Hildenbrand, kvm
Cc: Paolo Bonzini, Radim Krčmář, Cornelia Huck
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
applied thanks.
On 08/30/2017 06:06 PM, David Hildenbrand wrote:
> Looks like the "overflowing" range check is wrong.
>
> |=======b-------a=======|
>
> addr >= a || addr <= b
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> arch/s390/kvm/guestdbg.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
> index c2e0ddc1356e..bcbd86621d01 100644
> --- a/arch/s390/kvm/guestdbg.c
> +++ b/arch/s390/kvm/guestdbg.c
> @@ -308,7 +308,7 @@ static inline int in_addr_range(u64 addr, u64 a, u64 b)
> return (addr >= a) && (addr <= b);
> else
> /* "overflowing" interval */
> - return (addr <= a) && (addr >= b);
> + return (addr >= a) || (addr <= b);
> }
>
> #define end_of_range(bp_info) (bp_info->addr + bp_info->len - 1)
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v1 2/4] KVM: s390: use WARN_ON_ONCE only for checking
2017-08-30 16:06 ` [PATCH v1 2/4] KVM: s390: use WARN_ON_ONCE only for checking David Hildenbrand
2017-08-31 10:16 ` Cornelia Huck
@ 2017-08-31 10:55 ` Christian Borntraeger
1 sibling, 0 replies; 17+ messages in thread
From: Christian Borntraeger @ 2017-08-31 10:55 UTC (permalink / raw)
To: David Hildenbrand, kvm
Cc: Paolo Bonzini, Radim Krčmář, Cornelia Huck
On 08/30/2017 06:06 PM, David Hildenbrand wrote:
> Move the real logic that always has to be executed out of the
> WARN_ON_ONCE.
The WARN_ON_ONCE macro seems to be clever enough to not require this,
but it is certainly cleaner. There is a similar construct for the pfault
interrupts in kvm-s390.c which might also want to see such a cleanup.
In the future we might want to modify the interrupt code to not make
any allocation at all so that we cannot fail.
But anyway, applied.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> arch/s390/kvm/interrupt.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index a619ddae610d..a832ad031cee 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -2479,6 +2479,7 @@ void kvm_s390_reinject_machine_check(struct kvm_vcpu *vcpu,
> struct kvm_s390_mchk_info *mchk;
> union mci mci;
> __u64 cr14 = 0; /* upper bits are not used */
> + int rc;
>
> mci.val = mcck_info->mcic;
> if (mci.sr)
> @@ -2496,12 +2497,13 @@ void kvm_s390_reinject_machine_check(struct kvm_vcpu *vcpu,
> if (mci.ck) {
> /* Inject the floating machine check */
> inti.type = KVM_S390_MCHK;
> - WARN_ON_ONCE(__inject_vm(vcpu->kvm, &inti));
> + rc = __inject_vm(vcpu->kvm, &inti);
> } else {
> /* Inject the machine check to specified vcpu */
> irq.type = KVM_S390_MCHK;
> - WARN_ON_ONCE(kvm_s390_inject_vcpu(vcpu, &irq));
> + rc = kvm_s390_inject_vcpu(vcpu, &irq);
> }
> + WARN_ON_ONCE(rc);
> }
>
> int kvm_set_routing_entry(struct kvm *kvm,
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v1 3/4] KVM: s390: vsie: cleanup mcck reinjection
2017-08-30 16:06 ` [PATCH v1 3/4] KVM: s390: vsie: cleanup mcck reinjection David Hildenbrand
@ 2017-08-31 11:01 ` Christian Borntraeger
2017-08-31 11:11 ` Cornelia Huck
0 siblings, 1 reply; 17+ messages in thread
From: Christian Borntraeger @ 2017-08-31 11:01 UTC (permalink / raw)
To: David Hildenbrand, kvm
Cc: Paolo Bonzini, Radim Krčmář, Cornelia Huck
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
I will add some patch description like:
We already know that the machine check information was part of the vsie_page.
applied.
On 08/30/2017 06:06 PM, David Hildenbrand wrote:
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> arch/s390/kvm/vsie.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index 715c19c45d9a..b5eec30eb37d 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -806,8 +806,6 @@ static int do_vsie_run(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
> {
> struct kvm_s390_sie_block *scb_s = &vsie_page->scb_s;
> struct kvm_s390_sie_block *scb_o = vsie_page->scb_o;
> - struct mcck_volatile_info *mcck_info;
> - struct sie_page *sie_page;
> int rc;
>
> handle_last_fault(vcpu, vsie_page);
> @@ -831,9 +829,7 @@ static int do_vsie_run(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>
> if (rc == -EINTR) {
> VCPU_EVENT(vcpu, 3, "%s", "machine check");
> - sie_page = container_of(scb_s, struct sie_page, sie_block);
> - mcck_info = &sie_page->mcck_info;
> - kvm_s390_reinject_machine_check(vcpu, mcck_info);
> + kvm_s390_reinject_machine_check(vcpu, &vsie_page->mcck_info);
> return 0;
> }
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v1 3/4] KVM: s390: vsie: cleanup mcck reinjection
2017-08-31 11:01 ` Christian Borntraeger
@ 2017-08-31 11:11 ` Cornelia Huck
0 siblings, 0 replies; 17+ messages in thread
From: Cornelia Huck @ 2017-08-31 11:11 UTC (permalink / raw)
To: Christian Borntraeger
Cc: David Hildenbrand, kvm, Paolo Bonzini, Radim Krčmář
On Thu, 31 Aug 2017 13:01:39 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
>
> I will add some patch description like:
>
> We already know that the machine check information was part of the vsie_page.
Shorten that to "The machine check information is part of the
vsie_page."?
In any case,
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>
> applied.
>
> On 08/30/2017 06:06 PM, David Hildenbrand wrote:
> > Signed-off-by: David Hildenbrand <david@redhat.com>
> > ---
> > arch/s390/kvm/vsie.c | 6 +-----
> > 1 file changed, 1 insertion(+), 5 deletions(-)
> >
> > diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> > index 715c19c45d9a..b5eec30eb37d 100644
> > --- a/arch/s390/kvm/vsie.c
> > +++ b/arch/s390/kvm/vsie.c
> > @@ -806,8 +806,6 @@ static int do_vsie_run(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
> > {
> > struct kvm_s390_sie_block *scb_s = &vsie_page->scb_s;
> > struct kvm_s390_sie_block *scb_o = vsie_page->scb_o;
> > - struct mcck_volatile_info *mcck_info;
> > - struct sie_page *sie_page;
> > int rc;
> >
> > handle_last_fault(vcpu, vsie_page);
> > @@ -831,9 +829,7 @@ static int do_vsie_run(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
> >
> > if (rc == -EINTR) {
> > VCPU_EVENT(vcpu, 3, "%s", "machine check");
> > - sie_page = container_of(scb_s, struct sie_page, sie_block);
> > - mcck_info = &sie_page->mcck_info;
> > - kvm_s390_reinject_machine_check(vcpu, mcck_info);
> > + kvm_s390_reinject_machine_check(vcpu, &vsie_page->mcck_info);
> > return 0;
> > }
> >
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v1 4/4] KVM: s390: vsie: use common code functions for pinning
2017-08-30 16:06 ` [PATCH v1 4/4] KVM: s390: vsie: use common code functions for pinning David Hildenbrand
@ 2017-08-31 11:44 ` Christian Borntraeger
2017-08-31 12:13 ` David Hildenbrand
0 siblings, 1 reply; 17+ messages in thread
From: Christian Borntraeger @ 2017-08-31 11:44 UTC (permalink / raw)
To: David Hildenbrand, kvm
Cc: Paolo Bonzini, Radim Krčmář, Cornelia Huck
On 08/30/2017 06:06 PM, David Hildenbrand wrote:
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> arch/s390/kvm/vsie.c | 19 +++++--------------
> include/linux/kvm_host.h | 1 +
> virt/kvm/kvm_main.c | 4 ++--
> 3 files changed, 8 insertions(+), 16 deletions(-)
>
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index b5eec30eb37d..a21763b4c229 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -445,17 +445,12 @@ static int map_prefix(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
> static int pin_guest_page(struct kvm *kvm, gpa_t gpa, hpa_t *hpa)
> {
> struct page *page;
> - hva_t hva;
> - int rc;
>
> - hva = gfn_to_hva(kvm, gpa_to_gfn(gpa));
> - if (kvm_is_error_hva(hva))
> - return -EINVAL;
> - rc = get_user_pages_fast(hva, 1, 1, &page);
> - if (rc < 0)
> - return rc;
> - else if (rc != 1)
> + page = gfn_to_page(kvm, gpa_to_gfn(gpa));
> + if (PTR_ERR(page) == -ENOMEM)
> return -ENOMEM;
Can this actually happen? Reading gfn_to_page does not seem
to have a path that returns ENOMEM.
> + if (is_error_page(page))
> + return -EINVAL;
So just this might be enough? Most other users of gfn_to_page only check
for is_error_page (but not for ENOMEM).
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v1 4/4] KVM: s390: vsie: use common code functions for pinning
2017-08-31 11:44 ` Christian Borntraeger
@ 2017-08-31 12:13 ` David Hildenbrand
2017-08-31 12:22 ` Christian Borntraeger
0 siblings, 1 reply; 17+ messages in thread
From: David Hildenbrand @ 2017-08-31 12:13 UTC (permalink / raw)
To: Christian Borntraeger, kvm
Cc: Paolo Bonzini, Radim Krčmář, Cornelia Huck
On 31.08.2017 13:44, Christian Borntraeger wrote:
>
>
> On 08/30/2017 06:06 PM, David Hildenbrand wrote:
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>> arch/s390/kvm/vsie.c | 19 +++++--------------
>> include/linux/kvm_host.h | 1 +
>> virt/kvm/kvm_main.c | 4 ++--
>> 3 files changed, 8 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>> index b5eec30eb37d..a21763b4c229 100644
>> --- a/arch/s390/kvm/vsie.c
>> +++ b/arch/s390/kvm/vsie.c
>> @@ -445,17 +445,12 @@ static int map_prefix(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>> static int pin_guest_page(struct kvm *kvm, gpa_t gpa, hpa_t *hpa)
>> {
>> struct page *page;
>> - hva_t hva;
>> - int rc;
>>
>> - hva = gfn_to_hva(kvm, gpa_to_gfn(gpa));
>> - if (kvm_is_error_hva(hva))
>> - return -EINVAL;
>> - rc = get_user_pages_fast(hva, 1, 1, &page);
>> - if (rc < 0)
>> - return rc;
>> - else if (rc != 1)
>> + page = gfn_to_page(kvm, gpa_to_gfn(gpa));
>> + if (PTR_ERR(page) == -ENOMEM)
>> return -ENOMEM;
>
> Can this actually happen? Reading gfn_to_page does not seem
> to have a path that returns ENOMEM.
>
Boils down to what get_user_pages_fast() guarantees (which is called by
gfn_to_hva() now). It can call __get_user_pages(). And there I can at
least spot a -ENOMEM potentially coming out of faultin_page().
This check essentially makes sure that the behavior of pin_guest_page()
doesn't change (this function is specified to return either -ENOMEM or
-EINVAL). So unless there is a very good reason, I think we should keep
it that way.
--
Thanks,
David
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v1 4/4] KVM: s390: vsie: use common code functions for pinning
2017-08-31 12:13 ` David Hildenbrand
@ 2017-08-31 12:22 ` Christian Borntraeger
2017-08-31 13:00 ` David Hildenbrand
0 siblings, 1 reply; 17+ messages in thread
From: Christian Borntraeger @ 2017-08-31 12:22 UTC (permalink / raw)
To: David Hildenbrand, kvm
Cc: Paolo Bonzini, Radim Krčmář, Cornelia Huck
On 08/31/2017 02:13 PM, David Hildenbrand wrote:
> On 31.08.2017 13:44, Christian Borntraeger wrote:
>>
>>
>> On 08/30/2017 06:06 PM, David Hildenbrand wrote:
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>> arch/s390/kvm/vsie.c | 19 +++++--------------
>>> include/linux/kvm_host.h | 1 +
>>> virt/kvm/kvm_main.c | 4 ++--
>>> 3 files changed, 8 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>>> index b5eec30eb37d..a21763b4c229 100644
>>> --- a/arch/s390/kvm/vsie.c
>>> +++ b/arch/s390/kvm/vsie.c
>>> @@ -445,17 +445,12 @@ static int map_prefix(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>> static int pin_guest_page(struct kvm *kvm, gpa_t gpa, hpa_t *hpa)
>>> {
>>> struct page *page;
>>> - hva_t hva;
>>> - int rc;
>>>
>>> - hva = gfn_to_hva(kvm, gpa_to_gfn(gpa));
>>> - if (kvm_is_error_hva(hva))
>>> - return -EINVAL;
>>> - rc = get_user_pages_fast(hva, 1, 1, &page);
>>> - if (rc < 0)
>>> - return rc;
>>> - else if (rc != 1)
>>> + page = gfn_to_page(kvm, gpa_to_gfn(gpa));
>>> + if (PTR_ERR(page) == -ENOMEM)
>>> return -ENOMEM;
>>
>> Can this actually happen? Reading gfn_to_page does not seem
>> to have a path that returns ENOMEM.
>>
>
> Boils down to what get_user_pages_fast() guarantees (which is called by
> gfn_to_hva() now). It can call __get_user_pages(). And there I can at
> least spot a -ENOMEM potentially coming out of faultin_page().
> This check essentially makes sure that the behavior of pin_guest_page()
> doesn't change (this function is specified to return either -ENOMEM or
> -EINVAL). So unless there is a very good reason, I think we should keep
> it that way.
I might be misreading the code, but it looks like that
gfn_to_page will will call
kvm_pfn_to_page and that function will return KVM_ERR_PTR_BAD_PAGE if something
fails (which is (ERR_PTR(-ENOENT)). But it will never return ERR_PTR(-ENOMEM)
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v1 4/4] KVM: s390: vsie: use common code functions for pinning
2017-08-31 12:22 ` Christian Borntraeger
@ 2017-08-31 13:00 ` David Hildenbrand
2017-09-01 12:49 ` Christian Borntraeger
0 siblings, 1 reply; 17+ messages in thread
From: David Hildenbrand @ 2017-08-31 13:00 UTC (permalink / raw)
To: Christian Borntraeger, kvm
Cc: Paolo Bonzini, Radim Krčmář, Cornelia Huck
On 31.08.2017 14:22, Christian Borntraeger wrote:
>
>
> On 08/31/2017 02:13 PM, David Hildenbrand wrote:
>> On 31.08.2017 13:44, Christian Borntraeger wrote:
>>>
>>>
>>> On 08/30/2017 06:06 PM, David Hildenbrand wrote:
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> ---
>>>> arch/s390/kvm/vsie.c | 19 +++++--------------
>>>> include/linux/kvm_host.h | 1 +
>>>> virt/kvm/kvm_main.c | 4 ++--
>>>> 3 files changed, 8 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>>>> index b5eec30eb37d..a21763b4c229 100644
>>>> --- a/arch/s390/kvm/vsie.c
>>>> +++ b/arch/s390/kvm/vsie.c
>>>> @@ -445,17 +445,12 @@ static int map_prefix(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>> static int pin_guest_page(struct kvm *kvm, gpa_t gpa, hpa_t *hpa)
>>>> {
>>>> struct page *page;
>>>> - hva_t hva;
>>>> - int rc;
>>>>
>>>> - hva = gfn_to_hva(kvm, gpa_to_gfn(gpa));
>>>> - if (kvm_is_error_hva(hva))
>>>> - return -EINVAL;
>>>> - rc = get_user_pages_fast(hva, 1, 1, &page);
>>>> - if (rc < 0)
>>>> - return rc;
>>>> - else if (rc != 1)
>>>> + page = gfn_to_page(kvm, gpa_to_gfn(gpa));
>>>> + if (PTR_ERR(page) == -ENOMEM)
>>>> return -ENOMEM;
>>>
>>> Can this actually happen? Reading gfn_to_page does not seem
>>> to have a path that returns ENOMEM.
>>>
>>
>> Boils down to what get_user_pages_fast() guarantees (which is called by
>> gfn_to_hva() now). It can call __get_user_pages(). And there I can at
>> least spot a -ENOMEM potentially coming out of faultin_page().
>> This check essentially makes sure that the behavior of pin_guest_page()
>> doesn't change (this function is specified to return either -ENOMEM or
>> -EINVAL). So unless there is a very good reason, I think we should keep
>> it that way.
>
> I might be misreading the code, but it looks like that
> gfn_to_page will will call
> kvm_pfn_to_page and that function will return KVM_ERR_PTR_BAD_PAGE if something
> fails (which is (ERR_PTR(-ENOENT)). But it will never return ERR_PTR(-ENOMEM)
>
>
Guess I am getting lost in abstraction layers :)
I think you're right, is_error_noslot_pfn() in kvm_pfn_to_page() should
convert all such errors to KVM_ERR_PTR_BAD_PAGE.
-ENOMEM was a theoretical case at that point either way in my opinion.
So, this would boil down to:
>From a2cf5d109fc6342df12e361b665c92b73f66cfee Mon Sep 17 00:00:00 2001
From: David Hildenbrand <david@redhat.com>
Date: Thu, 3 Aug 2017 22:30:40 +0200
Subject: [PATCH v2 1/1] KVM: s390: vsie: use common code functions for
pinning
We will not see -ENOMEM (gfn_to_hva() will return KVM_ERR_PTR_BAD_PAGE
for all errors). So we can also get rid of special handling in the
callers of pin_guest_page() and always assume that it is a g2 error.
As also kvm_s390_inject_program_int() should never fail, we can
simplify pin_scb(), too.
Signed-off-by: David Hildenbrand <david@redhat.com>
---
arch/s390/kvm/vsie.c | 50
+++++++++++++++++-------------------------------
include/linux/kvm_host.h | 1 +
virt/kvm/kvm_main.c | 4 ++--
3 files changed, 21 insertions(+), 34 deletions(-)
diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
index b5eec30eb37d..0878df8ba406 100644
--- a/arch/s390/kvm/vsie.c
+++ b/arch/s390/kvm/vsie.c
@@ -440,22 +440,14 @@ static int map_prefix(struct kvm_vcpu *vcpu,
struct vsie_page *vsie_page)
*
* Returns: - 0 on success
* - -EINVAL if the gpa is not valid guest storage
- * - -ENOMEM if out of memory
*/
static int pin_guest_page(struct kvm *kvm, gpa_t gpa, hpa_t *hpa)
{
struct page *page;
- hva_t hva;
- int rc;
- hva = gfn_to_hva(kvm, gpa_to_gfn(gpa));
- if (kvm_is_error_hva(hva))
+ page = gfn_to_page(kvm, gpa_to_gfn(gpa));
+ if (is_error_page(page))
return -EINVAL;
- rc = get_user_pages_fast(hva, 1, 1, &page);
- if (rc < 0)
- return rc;
- else if (rc != 1)
- return -ENOMEM;
*hpa = (hpa_t) page_to_virt(page) + (gpa & ~PAGE_MASK);
return 0;
}
@@ -463,11 +455,7 @@ static int pin_guest_page(struct kvm *kvm, gpa_t
gpa, hpa_t *hpa)
/* Unpins a page previously pinned via pin_guest_page, marking it as
dirty. */
static void unpin_guest_page(struct kvm *kvm, gpa_t gpa, hpa_t hpa)
{
- struct page *page;
-
- page = virt_to_page(hpa);
- set_page_dirty_lock(page);
- put_page(page);
+ kvm_release_pfn_dirty(hpa >> PAGE_SHIFT);
/* mark the page always as dirty for migration */
mark_page_dirty(kvm, gpa_to_gfn(gpa));
}
@@ -554,7 +542,7 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct
vsie_page *vsie_page)
rc = set_validity_icpt(scb_s, 0x003bU);
if (!rc) {
rc = pin_guest_page(vcpu->kvm, gpa, &hpa);
- if (rc == -EINVAL)
+ if (rc)
rc = set_validity_icpt(scb_s, 0x0034U);
}
if (rc)
@@ -571,10 +559,10 @@ static int pin_blocks(struct kvm_vcpu *vcpu,
struct vsie_page *vsie_page)
}
/* 256 bytes cannot cross page boundaries */
rc = pin_guest_page(vcpu->kvm, gpa, &hpa);
- if (rc == -EINVAL)
+ if (rc) {
rc = set_validity_icpt(scb_s, 0x0080U);
- if (rc)
goto unpin;
+ }
scb_s->itdba = hpa;
}
@@ -589,10 +577,10 @@ static int pin_blocks(struct kvm_vcpu *vcpu,
struct vsie_page *vsie_page)
* if this block gets bigger, we have to shadow it.
*/
rc = pin_guest_page(vcpu->kvm, gpa, &hpa);
- if (rc == -EINVAL)
+ if (rc) {
rc = set_validity_icpt(scb_s, 0x1310U);
- if (rc)
goto unpin;
+ }
scb_s->gvrd = hpa;
}
@@ -604,11 +592,11 @@ static int pin_blocks(struct kvm_vcpu *vcpu,
struct vsie_page *vsie_page)
}
/* 64 bytes cannot cross page boundaries */
rc = pin_guest_page(vcpu->kvm, gpa, &hpa);
- if (rc == -EINVAL)
+ if (rc) {
rc = set_validity_icpt(scb_s, 0x0043U);
- /* Validity 0x0044 will be checked by SIE */
- if (rc)
goto unpin;
+ }
+ /* Validity 0x0044 will be checked by SIE */
scb_s->riccbd = hpa;
}
if ((scb_s->ecb & ECB_GS) && !(scb_s->ecd & ECD_HOSTREGMGMT)) {
@@ -632,10 +620,10 @@ static int pin_blocks(struct kvm_vcpu *vcpu,
struct vsie_page *vsie_page)
* cross page boundaries
*/
rc = pin_guest_page(vcpu->kvm, gpa, &hpa);
- if (rc == -EINVAL)
+ if (rc) {
rc = set_validity_icpt(scb_s, 0x10b0U);
- if (rc)
goto unpin;
+ }
scb_s->sdnxo = hpa | sdnxc;
}
return 0;
@@ -660,7 +648,6 @@ static void unpin_scb(struct kvm_vcpu *vcpu, struct
vsie_page *vsie_page,
*
* Returns: - 0 if the scb was pinned.
* - > 0 if control has to be given to guest 2
- * - -ENOMEM if out of memory
*/
static int pin_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page,
gpa_t gpa)
@@ -669,14 +656,13 @@ static int pin_scb(struct kvm_vcpu *vcpu, struct
vsie_page *vsie_page,
int rc;
rc = pin_guest_page(vcpu->kvm, gpa, &hpa);
- if (rc == -EINVAL) {
+ if (rc) {
rc = kvm_s390_inject_program_int(vcpu, PGM_ADDRESSING);
- if (!rc)
- rc = 1;
+ WARN_ON_ONCE(rc);
+ return 1;
}
- if (!rc)
- vsie_page->scb_o = (struct kvm_s390_sie_block *) hpa;
- return rc;
+ vsie_page->scb_o = (struct kvm_s390_sie_block *) hpa;
+ return 0;
}
/*
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 6882538eda32..2e754b7c282c 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -667,6 +667,7 @@ kvm_pfn_t __gfn_to_pfn_memslot(struct
kvm_memory_slot *slot, gfn_t gfn,
bool *writable);
void kvm_release_pfn_clean(kvm_pfn_t pfn);
+void kvm_release_pfn_dirty(kvm_pfn_t pfn);
void kvm_set_pfn_dirty(kvm_pfn_t pfn);
void kvm_set_pfn_accessed(kvm_pfn_t pfn);
void kvm_get_pfn(kvm_pfn_t pfn);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 1b3fa3fc1a78..dc422a007b25 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -122,7 +122,6 @@ static void hardware_disable_all(void);
static void kvm_io_bus_destroy(struct kvm_io_bus *bus);
-static void kvm_release_pfn_dirty(kvm_pfn_t pfn);
static void mark_page_dirty_in_slot(struct kvm_memory_slot *memslot,
gfn_t gfn);
__visible bool kvm_rebooting;
@@ -1720,11 +1719,12 @@ void kvm_release_page_dirty(struct page *page)
}
EXPORT_SYMBOL_GPL(kvm_release_page_dirty);
-static void kvm_release_pfn_dirty(kvm_pfn_t pfn)
+void kvm_release_pfn_dirty(kvm_pfn_t pfn)
{
kvm_set_pfn_dirty(pfn);
kvm_release_pfn_clean(pfn);
}
+EXPORT_SYMBOL_GPL(kvm_release_pfn_dirty);
void kvm_set_pfn_dirty(kvm_pfn_t pfn)
{
--
2.13.5
--
Thanks,
David
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v1 4/4] KVM: s390: vsie: use common code functions for pinning
2017-08-31 13:00 ` David Hildenbrand
@ 2017-09-01 12:49 ` Christian Borntraeger
2017-09-01 13:18 ` David Hildenbrand
0 siblings, 1 reply; 17+ messages in thread
From: Christian Borntraeger @ 2017-09-01 12:49 UTC (permalink / raw)
To: David Hildenbrand, kvm
Cc: Paolo Bonzini, Radim Krčmář, Cornelia Huck
On 08/31/2017 03:00 PM, David Hildenbrand wrote:
> On 31.08.2017 14:22, Christian Borntraeger wrote:
>>
>>
>> On 08/31/2017 02:13 PM, David Hildenbrand wrote:
>>> On 31.08.2017 13:44, Christian Borntraeger wrote:
>>>>
>>>>
>>>> On 08/30/2017 06:06 PM, David Hildenbrand wrote:
>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>>> ---
>>>>> arch/s390/kvm/vsie.c | 19 +++++--------------
>>>>> include/linux/kvm_host.h | 1 +
>>>>> virt/kvm/kvm_main.c | 4 ++--
>>>>> 3 files changed, 8 insertions(+), 16 deletions(-)
>>>>>
>>>>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>>>>> index b5eec30eb37d..a21763b4c229 100644
>>>>> --- a/arch/s390/kvm/vsie.c
>>>>> +++ b/arch/s390/kvm/vsie.c
>>>>> @@ -445,17 +445,12 @@ static int map_prefix(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>>> static int pin_guest_page(struct kvm *kvm, gpa_t gpa, hpa_t *hpa)
>>>>> {
>>>>> struct page *page;
>>>>> - hva_t hva;
>>>>> - int rc;
>>>>>
>>>>> - hva = gfn_to_hva(kvm, gpa_to_gfn(gpa));
>>>>> - if (kvm_is_error_hva(hva))
>>>>> - return -EINVAL;
>>>>> - rc = get_user_pages_fast(hva, 1, 1, &page);
>>>>> - if (rc < 0)
>>>>> - return rc;
>>>>> - else if (rc != 1)
>>>>> + page = gfn_to_page(kvm, gpa_to_gfn(gpa));
>>>>> + if (PTR_ERR(page) == -ENOMEM)
>>>>> return -ENOMEM;
>>>>
>>>> Can this actually happen? Reading gfn_to_page does not seem
>>>> to have a path that returns ENOMEM.
>>>>
>>>
>>> Boils down to what get_user_pages_fast() guarantees (which is called by
>>> gfn_to_hva() now). It can call __get_user_pages(). And there I can at
>>> least spot a -ENOMEM potentially coming out of faultin_page().
>>> This check essentially makes sure that the behavior of pin_guest_page()
>>> doesn't change (this function is specified to return either -ENOMEM or
>>> -EINVAL). So unless there is a very good reason, I think we should keep
>>> it that way.
>>
>> I might be misreading the code, but it looks like that
>> gfn_to_page will will call
>> kvm_pfn_to_page and that function will return KVM_ERR_PTR_BAD_PAGE if something
>> fails (which is (ERR_PTR(-ENOENT)). But it will never return ERR_PTR(-ENOMEM)
>>
>>
>
> Guess I am getting lost in abstraction layers :)
>
> I think you're right, is_error_noslot_pfn() in kvm_pfn_to_page() should
> convert all such errors to KVM_ERR_PTR_BAD_PAGE.
>
> -ENOMEM was a theoretical case at that point either way in my opinion.
>
> So, this would boil down to:
>
>
> From a2cf5d109fc6342df12e361b665c92b73f66cfee Mon Sep 17 00:00:00 2001
> From: David Hildenbrand <david@redhat.com>
> Date: Thu, 3 Aug 2017 22:30:40 +0200
> Subject: [PATCH v2 1/1] KVM: s390: vsie: use common code functions for
> pinning
>
> We will not see -ENOMEM (gfn_to_hva() will return KVM_ERR_PTR_BAD_PAGE
> for all errors). So we can also get rid of special handling in the
> callers of pin_guest_page() and always assume that it is a g2 error.
>
> As also kvm_s390_inject_program_int() should never fail, we can
> simplify pin_scb(), too.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
Can you send it as a new patch please?
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v1 4/4] KVM: s390: vsie: use common code functions for pinning
2017-09-01 12:49 ` Christian Borntraeger
@ 2017-09-01 13:18 ` David Hildenbrand
0 siblings, 0 replies; 17+ messages in thread
From: David Hildenbrand @ 2017-09-01 13:18 UTC (permalink / raw)
To: Christian Borntraeger, kvm
Cc: Paolo Bonzini, Radim Krčmář, Cornelia Huck
>> Guess I am getting lost in abstraction layers :)
>>
>> I think you're right, is_error_noslot_pfn() in kvm_pfn_to_page() should
>> convert all such errors to KVM_ERR_PTR_BAD_PAGE.
>>
>> -ENOMEM was a theoretical case at that point either way in my opinion.
>>
>> So, this would boil down to:
>>
>>
>> From a2cf5d109fc6342df12e361b665c92b73f66cfee Mon Sep 17 00:00:00 2001
>> From: David Hildenbrand <david@redhat.com>
>> Date: Thu, 3 Aug 2017 22:30:40 +0200
>> Subject: [PATCH v2 1/1] KVM: s390: vsie: use common code functions for
>> pinning
>>
>> We will not see -ENOMEM (gfn_to_hva() will return KVM_ERR_PTR_BAD_PAGE
>> for all errors). So we can also get rid of special handling in the
>> callers of pin_guest_page() and always assume that it is a g2 error.
>>
>> As also kvm_s390_inject_program_int() should never fail, we can
>> simplify pin_scb(), too.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>
> Can you send it as a new patch please?
>
Sure, will do.
--
Thanks,
David
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2017-09-01 13:18 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-30 16:05 [PATCH v1 0/4] s390x: one fix and three cleanups David Hildenbrand
2017-08-30 16:06 ` [PATCH v1 1/4] KVM: s390: guestdbg: fix range check David Hildenbrand
2017-08-31 10:13 ` Cornelia Huck
2017-08-31 10:36 ` Christian Borntraeger
2017-08-30 16:06 ` [PATCH v1 2/4] KVM: s390: use WARN_ON_ONCE only for checking David Hildenbrand
2017-08-31 10:16 ` Cornelia Huck
2017-08-31 10:55 ` Christian Borntraeger
2017-08-30 16:06 ` [PATCH v1 3/4] KVM: s390: vsie: cleanup mcck reinjection David Hildenbrand
2017-08-31 11:01 ` Christian Borntraeger
2017-08-31 11:11 ` Cornelia Huck
2017-08-30 16:06 ` [PATCH v1 4/4] KVM: s390: vsie: use common code functions for pinning David Hildenbrand
2017-08-31 11:44 ` Christian Borntraeger
2017-08-31 12:13 ` David Hildenbrand
2017-08-31 12:22 ` Christian Borntraeger
2017-08-31 13:00 ` David Hildenbrand
2017-09-01 12:49 ` Christian Borntraeger
2017-09-01 13:18 ` David Hildenbrand
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.