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