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@xxxxxxxxxx>
>>>>> ---
>>>>> 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@xxxxxxxxxx>
> 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@xxxxxxxxxx>
Can you send it as a new patch please?
