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

From: David Hildenbrand <david@redhat.com>

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

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


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

Commit Message

Comments

Patch