Re: [PATCH 1/2] KVM: s390: gaccess: Cleanup access to guest frames
From: David Hildenbrand <hidden>
Date: 2021-08-18 07:55:00
Also in:
linux-s390, lkml
On 16.08.21 17:07, Janis Schoetterl-Glausch wrote:
quoted hunk ↗ jump to hunk
Introduce a helper function for guest frame access. Rewrite the calculation of the gpa and the length of the segment to be more readable. Signed-off-by: Janis Schoetterl-Glausch <redacted> --- arch/s390/kvm/gaccess.c | 48 +++++++++++++++++++++++++---------------- 1 file changed, 29 insertions(+), 19 deletions(-)diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c index b9f85b2dc053..df83de0843de 100644 --- a/arch/s390/kvm/gaccess.c +++ b/arch/s390/kvm/gaccess.c@@ -827,11 +827,26 @@ static int guest_page_range(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, return 0; } +static int access_guest_frame(struct kvm *kvm, enum gacc_mode mode, gpa_t gpa, + void *data, unsigned int len)
I know, "frame" is a beautiful term for "page" -- can we just avoid using it because we're not using it anywhere else here? :) What's wrong with "access_guest_page()" ?
+{
+ gfn_t gfn = gpa_to_gfn(gpa);
+ unsigned int offset = offset_in_page(gpa);
+ int rc;You could turn both const. You might want to consider reverse-christmas-treeing this.
+ + if (mode == GACC_STORE) + rc = kvm_write_guest_page(kvm, gfn, data, offset, len); + else + rc = kvm_read_guest_page(kvm, gfn, data, offset, len);
Personally, I prefer passing in pfn + offset instead of a gpa. Also avoids having to convert back and forth.
+ return rc;
+}
+
int access_guest(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, void *data,
unsigned long len, enum gacc_mode mode)
{
psw_t *psw = &vcpu->arch.sie_block->gpsw;
- unsigned long _len, nr_pages, gpa, idx;
+ unsigned long nr_pages, gpa, idx;
+ unsigned int seg;Dito, reverse christmas tree might be worth keeping.
quoted hunk ↗ jump to hunk
unsigned long pages_array[2]; unsigned long *pages; int need_ipte_lock;@@ -855,15 +870,12 @@ int access_guest(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, void *data, ipte_lock(vcpu); rc = guest_page_range(vcpu, ga, ar, pages, nr_pages, asce, mode); for (idx = 0; idx < nr_pages && !rc; idx++) { - gpa = *(pages + idx) + (ga & ~PAGE_MASK); - _len = min(PAGE_SIZE - (gpa & ~PAGE_MASK), len); - if (mode == GACC_STORE) - rc = kvm_write_guest(vcpu->kvm, gpa, data, _len); - else - rc = kvm_read_guest(vcpu->kvm, gpa, data, _len); - len -= _len; - ga += _len; - data += _len; + gpa = pages[idx] + offset_in_page(ga); + seg = min(PAGE_SIZE - offset_in_page(gpa), len); + rc = access_guest_frame(vcpu->kvm, mode, gpa, data, seg); + len -= seg; + ga += seg; + data += seg; } if (need_ipte_lock) ipte_unlock(vcpu);@@ -875,19 +887,17 @@ int access_guest(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, void *data, int access_guest_real(struct kvm_vcpu *vcpu, unsigned long gra, void *data, unsigned long len, enum gacc_mode mode) { - unsigned long _len, gpa; + unsigned long gpa; + unsigned int seg; int rc = 0; while (len && !rc) { gpa = kvm_s390_real_to_abs(vcpu, gra); - _len = min(PAGE_SIZE - (gpa & ~PAGE_MASK), len); - if (mode) - rc = write_guest_abs(vcpu, gpa, data, _len); - else - rc = read_guest_abs(vcpu, gpa, data, _len); - len -= _len; - gra += _len; - data += _len; + seg = min(PAGE_SIZE - offset_in_page(gpa), len);
What does "seg" mean? I certainly know when "len" means -- which is also what the function eats.
+ rc = access_guest_frame(vcpu->kvm, mode, gpa, data, seg); + len -= seg; + gra += seg; + data += seg; } return rc; }
-- Thanks, David / dhildenb