[PATCH v1 0/4] s390x: one fix and three cleanups

[PATCH v1 0/4] s390x: one fix and three cleanups

[David Hildenbrand]

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

[PATCH v1 1/4] KVM: s390: guestdbg: fix range check

[David Hildenbrand]

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

Re: [PATCH v1 1/4] KVM: s390: guestdbg: fix range check

[Cornelia Huck]

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>

Re: [PATCH v1 1/4] KVM: s390: guestdbg: fix range check

[Christian Borntraeger]

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)
> 

[PATCH v1 2/4] KVM: s390: use WARN_ON_ONCE only for checking

[David Hildenbrand]

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

Re: [PATCH v1 2/4] KVM: s390: use WARN_ON_ONCE only for checking

[Cornelia Huck]

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>

Re: [PATCH v1 2/4] KVM: s390: use WARN_ON_ONCE only for checking

[Christian Borntraeger]

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,
> 

[PATCH v1 3/4] KVM: s390: vsie: cleanup mcck reinjection

[David Hildenbrand]

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

Re: [PATCH v1 3/4] KVM: s390: vsie: cleanup mcck reinjection

[Christian Borntraeger]

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;
>  	}
> 

Re: [PATCH v1 3/4] KVM: s390: vsie: cleanup mcck reinjection

[Cornelia Huck]

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;
> >  	}
> >   
> 

[PATCH v1 4/4] KVM: s390: vsie: use common code functions for pinning

[David Hildenbrand]

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

Re: [PATCH v1 4/4] KVM: s390: vsie: use common code functions for pinning

[Christian Borntraeger]

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).

Re: [PATCH v1 4/4] KVM: s390: vsie: use common code functions for pinning

[David Hildenbrand]

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

Re: [PATCH v1 4/4] KVM: s390: vsie: use common code functions for pinning

[Christian Borntraeger]

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)

Re: [PATCH v1 4/4] KVM: s390: vsie: use common code functions for pinning

[David Hildenbrand]

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

Re: [PATCH v1 4/4] KVM: s390: vsie: use common code functions for pinning

[Christian Borntraeger]

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? 

Re: [PATCH v1 4/4] KVM: s390: vsie: use common code functions for pinning

[David Hildenbrand]

>> 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