* [PATCH v2 0/3] KVM: s390: Some gaccess cleanup
@ 2021-10-28 13:55 Janis Schoetterl-Glausch
2021-10-28 13:55 ` [PATCH v2 1/3] KVM: s390: gaccess: Refactor gpa and length calculation Janis Schoetterl-Glausch
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Janis Schoetterl-Glausch @ 2021-10-28 13:55 UTC (permalink / raw)
To: Christian Borntraeger, Janosch Frank, Heiko Carstens, Vasily Gorbik
Cc: Janis Schoetterl-Glausch, David Hildenbrand, Claudio Imbrenda,
Alexander Gordeev, kvm, linux-s390, linux-kernel
Cleanup s390 guest access code a bit, getting rid of some code
duplication and improving readability.
v1 -> v2
separate patch for renamed variable
fragment_len instead of seg
expand comment of guest_range_to_gpas
fix nits
I did not pick up Janosch's Reviewed-by because of the split patch
and the changed variable name.
Janis Schoetterl-Glausch (3):
KVM: s390: gaccess: Refactor gpa and length calculation
KVM: s390: gaccess: Refactor access address range check
KVM: s390: gaccess: Cleanup access to guest frames
arch/s390/kvm/gaccess.c | 158 +++++++++++++++++++++++-----------------
1 file changed, 92 insertions(+), 66 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v2 1/3] KVM: s390: gaccess: Refactor gpa and length calculation 2021-10-28 13:55 [PATCH v2 0/3] KVM: s390: Some gaccess cleanup Janis Schoetterl-Glausch @ 2021-10-28 13:55 ` Janis Schoetterl-Glausch 2021-11-19 8:27 ` Janosch Frank 2021-11-23 12:38 ` Janosch Frank 2021-10-28 13:55 ` [PATCH v2] KVM: s390: gaccess: Refactor access address range check Janis Schoetterl-Glausch ` (2 subsequent siblings) 3 siblings, 2 replies; 13+ messages in thread From: Janis Schoetterl-Glausch @ 2021-10-28 13:55 UTC (permalink / raw) To: Christian Borntraeger, Janosch Frank, Heiko Carstens, Vasily Gorbik Cc: Janis Schoetterl-Glausch, David Hildenbrand, Claudio Imbrenda, Alexander Gordeev, kvm, linux-s390, linux-kernel Improve readability be renaming the length variable and not calculating the offset manually. Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> --- arch/s390/kvm/gaccess.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c index 6af59c59cc1b..0d11cea92603 100644 --- a/arch/s390/kvm/gaccess.c +++ b/arch/s390/kvm/gaccess.c @@ -831,7 +831,8 @@ 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 fragment_len; unsigned long pages_array[2]; unsigned long *pages; int need_ipte_lock; @@ -855,15 +856,15 @@ 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); + gpa = pages[idx] + offset_in_page(ga); + fragment_len = min(PAGE_SIZE - offset_in_page(gpa), len); if (mode == GACC_STORE) - rc = kvm_write_guest(vcpu->kvm, gpa, data, _len); + rc = kvm_write_guest(vcpu->kvm, gpa, data, fragment_len); else - rc = kvm_read_guest(vcpu->kvm, gpa, data, _len); - len -= _len; - ga += _len; - data += _len; + rc = kvm_read_guest(vcpu->kvm, gpa, data, fragment_len); + len -= fragment_len; + ga += fragment_len; + data += fragment_len; } if (need_ipte_lock) ipte_unlock(vcpu); @@ -875,19 +876,20 @@ 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 int fragment_len; + unsigned long gpa; int rc = 0; while (len && !rc) { gpa = kvm_s390_real_to_abs(vcpu, gra); - _len = min(PAGE_SIZE - (gpa & ~PAGE_MASK), len); + fragment_len = min(PAGE_SIZE - offset_in_page(gpa), len); if (mode) - rc = write_guest_abs(vcpu, gpa, data, _len); + rc = write_guest_abs(vcpu, gpa, data, fragment_len); else - rc = read_guest_abs(vcpu, gpa, data, _len); - len -= _len; - gra += _len; - data += _len; + rc = read_guest_abs(vcpu, gpa, data, fragment_len); + len -= fragment_len; + gra += fragment_len; + data += fragment_len; } return rc; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/3] KVM: s390: gaccess: Refactor gpa and length calculation 2021-10-28 13:55 ` [PATCH v2 1/3] KVM: s390: gaccess: Refactor gpa and length calculation Janis Schoetterl-Glausch @ 2021-11-19 8:27 ` Janosch Frank 2021-11-23 12:38 ` Janosch Frank 1 sibling, 0 replies; 13+ messages in thread From: Janosch Frank @ 2021-11-19 8:27 UTC (permalink / raw) To: Janis Schoetterl-Glausch, Christian Borntraeger, Heiko Carstens, Vasily Gorbik Cc: David Hildenbrand, Claudio Imbrenda, Alexander Gordeev, kvm, linux-s390, linux-kernel On 10/28/21 15:55, Janis Schoetterl-Glausch wrote: > Improve readability be renaming the length variable and > not calculating the offset manually. > > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> Reviewed-by: Janosch Frank <frankja@linux.ibm.com> > --- > arch/s390/kvm/gaccess.c | 32 +++++++++++++++++--------------- > 1 file changed, 17 insertions(+), 15 deletions(-) > > diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c > index 6af59c59cc1b..0d11cea92603 100644 > --- a/arch/s390/kvm/gaccess.c > +++ b/arch/s390/kvm/gaccess.c > @@ -831,7 +831,8 @@ 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 fragment_len; > unsigned long pages_array[2]; > unsigned long *pages; > int need_ipte_lock; > @@ -855,15 +856,15 @@ 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); > + gpa = pages[idx] + offset_in_page(ga); > + fragment_len = min(PAGE_SIZE - offset_in_page(gpa), len); > if (mode == GACC_STORE) > - rc = kvm_write_guest(vcpu->kvm, gpa, data, _len); > + rc = kvm_write_guest(vcpu->kvm, gpa, data, fragment_len); > else > - rc = kvm_read_guest(vcpu->kvm, gpa, data, _len); > - len -= _len; > - ga += _len; > - data += _len; > + rc = kvm_read_guest(vcpu->kvm, gpa, data, fragment_len); > + len -= fragment_len; > + ga += fragment_len; > + data += fragment_len; > } > if (need_ipte_lock) > ipte_unlock(vcpu); > @@ -875,19 +876,20 @@ 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 int fragment_len; > + unsigned long gpa; > int rc = 0; > > while (len && !rc) { > gpa = kvm_s390_real_to_abs(vcpu, gra); > - _len = min(PAGE_SIZE - (gpa & ~PAGE_MASK), len); > + fragment_len = min(PAGE_SIZE - offset_in_page(gpa), len); > if (mode) > - rc = write_guest_abs(vcpu, gpa, data, _len); > + rc = write_guest_abs(vcpu, gpa, data, fragment_len); > else > - rc = read_guest_abs(vcpu, gpa, data, _len); > - len -= _len; > - gra += _len; > - data += _len; > + rc = read_guest_abs(vcpu, gpa, data, fragment_len); > + len -= fragment_len; > + gra += fragment_len; > + data += fragment_len; > } > return rc; > } > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/3] KVM: s390: gaccess: Refactor gpa and length calculation 2021-10-28 13:55 ` [PATCH v2 1/3] KVM: s390: gaccess: Refactor gpa and length calculation Janis Schoetterl-Glausch 2021-11-19 8:27 ` Janosch Frank @ 2021-11-23 12:38 ` Janosch Frank 1 sibling, 0 replies; 13+ messages in thread From: Janosch Frank @ 2021-11-23 12:38 UTC (permalink / raw) To: Janis Schoetterl-Glausch, Christian Borntraeger, Heiko Carstens, Vasily Gorbik Cc: David Hildenbrand, Claudio Imbrenda, Alexander Gordeev, kvm, linux-s390, linux-kernel On 10/28/21 15:55, Janis Schoetterl-Glausch wrote: > Improve readability be renaming the length variable and s/be/by/ > not calculating the offset manually. > > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> > --- > arch/s390/kvm/gaccess.c | 32 +++++++++++++++++--------------- > 1 file changed, 17 insertions(+), 15 deletions(-) > > diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c > index 6af59c59cc1b..0d11cea92603 100644 > --- a/arch/s390/kvm/gaccess.c > +++ b/arch/s390/kvm/gaccess.c > @@ -831,7 +831,8 @@ 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 fragment_len; > unsigned long pages_array[2]; > unsigned long *pages; > int need_ipte_lock; > @@ -855,15 +856,15 @@ 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); > + gpa = pages[idx] + offset_in_page(ga); > + fragment_len = min(PAGE_SIZE - offset_in_page(gpa), len); > if (mode == GACC_STORE) > - rc = kvm_write_guest(vcpu->kvm, gpa, data, _len); > + rc = kvm_write_guest(vcpu->kvm, gpa, data, fragment_len); > else > - rc = kvm_read_guest(vcpu->kvm, gpa, data, _len); > - len -= _len; > - ga += _len; > - data += _len; > + rc = kvm_read_guest(vcpu->kvm, gpa, data, fragment_len); > + len -= fragment_len; > + ga += fragment_len; > + data += fragment_len; > } > if (need_ipte_lock) > ipte_unlock(vcpu); > @@ -875,19 +876,20 @@ 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 int fragment_len; > + unsigned long gpa; > int rc = 0; > > while (len && !rc) { > gpa = kvm_s390_real_to_abs(vcpu, gra); > - _len = min(PAGE_SIZE - (gpa & ~PAGE_MASK), len); > + fragment_len = min(PAGE_SIZE - offset_in_page(gpa), len); > if (mode) > - rc = write_guest_abs(vcpu, gpa, data, _len); > + rc = write_guest_abs(vcpu, gpa, data, fragment_len); > else > - rc = read_guest_abs(vcpu, gpa, data, _len); > - len -= _len; > - gra += _len; > - data += _len; > + rc = read_guest_abs(vcpu, gpa, data, fragment_len); > + len -= fragment_len; > + gra += fragment_len; > + data += fragment_len; > } > return rc; > } > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] KVM: s390: gaccess: Refactor access address range check 2021-10-28 13:55 [PATCH v2 0/3] KVM: s390: Some gaccess cleanup Janis Schoetterl-Glausch 2021-10-28 13:55 ` [PATCH v2 1/3] KVM: s390: gaccess: Refactor gpa and length calculation Janis Schoetterl-Glausch @ 2021-10-28 13:55 ` Janis Schoetterl-Glausch 2021-11-19 8:56 ` Janosch Frank 2021-10-28 13:55 ` [PATCH v2 3/3] KVM: s390: gaccess: Cleanup access to guest frames Janis Schoetterl-Glausch 2021-11-23 12:39 ` [PATCH v2 0/3] KVM: s390: Some gaccess cleanup Janosch Frank 3 siblings, 1 reply; 13+ messages in thread From: Janis Schoetterl-Glausch @ 2021-10-28 13:55 UTC (permalink / raw) To: Christian Borntraeger, Janosch Frank, Heiko Carstens, Vasily Gorbik Cc: Janis Schoetterl-Glausch, David Hildenbrand, Claudio Imbrenda, Alexander Gordeev, kvm, linux-s390, linux-kernel Do not round down the first address to the page boundary, just translate it normally, which gives the value we care about in the first place. Given this, translating a single address is just the special case of translating a range spanning a single page. Make the output optional, so the function can be used to just check a range. Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> --- arch/s390/kvm/gaccess.c | 122 +++++++++++++++++++++++----------------- 1 file changed, 69 insertions(+), 53 deletions(-) diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c index 0d11cea92603..7725dd7566ed 100644 --- a/arch/s390/kvm/gaccess.c +++ b/arch/s390/kvm/gaccess.c @@ -794,35 +794,74 @@ static int low_address_protection_enabled(struct kvm_vcpu *vcpu, return 1; } -static int guest_page_range(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, - unsigned long *pages, unsigned long nr_pages, - const union asce asce, enum gacc_mode mode) +/** + * guest_range_to_gpas() - Calculate guest physical addresses of page fragments + * covering a logical range + * @vcpu: virtual cpu + * @ga: guest address, start of range + * @ar: access register + * @gpas: output argument, may be NULL + * @len: length of range in bytes + * @asce: address-space-control element to use for translation + * @mode: access mode + * + * Translate a logical range to a series of guest absolute addresses, + * such that the concatenation of page fragments starting at each gpa make up + * the whole range. + * The translation is performed as if done by the cpu for the given @asce, @ar, + * @mode and state of the @vcpu. + * If the translation causes an exception, its program interruption code is + * returned and the &struct kvm_s390_pgm_info pgm member of @vcpu is modified + * such that a subsequent call to kvm_s390_inject_prog_vcpu() will inject + * a correct exception into the guest. + * The resulting gpas are stored into @gpas, unless it is NULL. + * + * Note: All gpas except the first one start at the beginning of a page. + * When deriving the boundaries of a fragment from a gpa, all but the last + * fragment end at the end of the page. + * + * Return: + * * 0 - success + * * <0 - translation could not be performed, for example if guest + * memory could not be accessed + * * >0 - an access exception occurred. In this case the returned value + * is the program interruption code and the contents of pgm may + * be used to inject an exception into the guest. + */ +static int guest_range_to_gpas(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, + unsigned long *gpas, unsigned long len, + const union asce asce, enum gacc_mode mode) { psw_t *psw = &vcpu->arch.sie_block->gpsw; + unsigned int offset = offset_in_page(ga); + unsigned int fragment_len; int lap_enabled, rc = 0; enum prot_type prot; + unsigned long gpa; lap_enabled = low_address_protection_enabled(vcpu, asce); - while (nr_pages) { + while (min(PAGE_SIZE - offset, len) > 0) { + fragment_len = min(PAGE_SIZE - offset, len); ga = kvm_s390_logical_to_effective(vcpu, ga); if (mode == GACC_STORE && lap_enabled && is_low_address(ga)) return trans_exc(vcpu, PGM_PROTECTION, ga, ar, mode, PROT_TYPE_LA); - ga &= PAGE_MASK; if (psw_bits(*psw).dat) { - rc = guest_translate(vcpu, ga, pages, asce, mode, &prot); + rc = guest_translate(vcpu, ga, &gpa, asce, mode, &prot); if (rc < 0) return rc; } else { - *pages = kvm_s390_real_to_abs(vcpu, ga); - if (kvm_is_error_gpa(vcpu->kvm, *pages)) + gpa = kvm_s390_real_to_abs(vcpu, ga); + if (kvm_is_error_gpa(vcpu->kvm, gpa)) rc = PGM_ADDRESSING; } if (rc) return trans_exc(vcpu, rc, ga, ar, mode, prot); - ga += PAGE_SIZE; - pages++; - nr_pages--; + if (gpas) + *gpas++ = gpa; + offset = 0; + ga += fragment_len; + len -= fragment_len; } return 0; } @@ -831,10 +870,10 @@ 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 nr_pages, gpa, idx; + unsigned long nr_pages, idx; unsigned int fragment_len; - unsigned long pages_array[2]; - unsigned long *pages; + unsigned long gpa_array[2]; + unsigned long *gpas; int need_ipte_lock; union asce asce; int rc; @@ -846,30 +885,28 @@ int access_guest(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, void *data, if (rc) return rc; nr_pages = (((ga & ~PAGE_MASK) + len - 1) >> PAGE_SHIFT) + 1; - pages = pages_array; - if (nr_pages > ARRAY_SIZE(pages_array)) - pages = vmalloc(array_size(nr_pages, sizeof(unsigned long))); - if (!pages) + gpas = gpa_array; + if (nr_pages > ARRAY_SIZE(gpa_array)) + gpas = vmalloc(array_size(nr_pages, sizeof(unsigned long))); + if (!gpas) return -ENOMEM; need_ipte_lock = psw_bits(*psw).dat && !asce.r; if (need_ipte_lock) ipte_lock(vcpu); - rc = guest_page_range(vcpu, ga, ar, pages, nr_pages, asce, mode); + rc = guest_range_to_gpas(vcpu, ga, ar, gpas, len, asce, mode); for (idx = 0; idx < nr_pages && !rc; idx++) { - gpa = pages[idx] + offset_in_page(ga); - fragment_len = min(PAGE_SIZE - offset_in_page(gpa), len); + fragment_len = min(PAGE_SIZE - offset_in_page(gpas[idx]), len); if (mode == GACC_STORE) - rc = kvm_write_guest(vcpu->kvm, gpa, data, fragment_len); + rc = kvm_write_guest(vcpu->kvm, gpas[idx], data, fragment_len); else - rc = kvm_read_guest(vcpu->kvm, gpa, data, fragment_len); + rc = kvm_read_guest(vcpu->kvm, gpas[idx], data, fragment_len); len -= fragment_len; - ga += fragment_len; data += fragment_len; } if (need_ipte_lock) ipte_unlock(vcpu); - if (nr_pages > ARRAY_SIZE(pages_array)) - vfree(pages); + if (nr_pages > ARRAY_SIZE(gpa_array)) + vfree(gpas); return rc; } @@ -911,8 +948,6 @@ int access_guest_real(struct kvm_vcpu *vcpu, unsigned long gra, int guest_translate_address(struct kvm_vcpu *vcpu, unsigned long gva, u8 ar, unsigned long *gpa, enum gacc_mode mode) { - psw_t *psw = &vcpu->arch.sie_block->gpsw; - enum prot_type prot; union asce asce; int rc; @@ -920,23 +955,7 @@ int guest_translate_address(struct kvm_vcpu *vcpu, unsigned long gva, u8 ar, rc = get_vcpu_asce(vcpu, &asce, gva, ar, mode); if (rc) return rc; - if (is_low_address(gva) && low_address_protection_enabled(vcpu, asce)) { - if (mode == GACC_STORE) - return trans_exc(vcpu, PGM_PROTECTION, gva, 0, - mode, PROT_TYPE_LA); - } - - if (psw_bits(*psw).dat && !asce.r) { /* Use DAT? */ - rc = guest_translate(vcpu, gva, gpa, asce, mode, &prot); - if (rc > 0) - return trans_exc(vcpu, rc, gva, 0, mode, prot); - } else { - *gpa = kvm_s390_real_to_abs(vcpu, gva); - if (kvm_is_error_gpa(vcpu->kvm, *gpa)) - return trans_exc(vcpu, rc, gva, PGM_ADDRESSING, mode, 0); - } - - return rc; + return guest_range_to_gpas(vcpu, gva, ar, gpa, 1, asce, mode); } /** @@ -950,17 +969,14 @@ int guest_translate_address(struct kvm_vcpu *vcpu, unsigned long gva, u8 ar, int check_gva_range(struct kvm_vcpu *vcpu, unsigned long gva, u8 ar, unsigned long length, enum gacc_mode mode) { - unsigned long gpa; - unsigned long currlen; + union asce asce; int rc = 0; + rc = get_vcpu_asce(vcpu, &asce, gva, ar, mode); + if (rc) + return rc; ipte_lock(vcpu); - while (length > 0 && !rc) { - currlen = min(length, PAGE_SIZE - (gva % PAGE_SIZE)); - rc = guest_translate_address(vcpu, gva, ar, &gpa, mode); - gva += currlen; - length -= currlen; - } + rc = guest_range_to_gpas(vcpu, gva, ar, NULL, length, asce, mode); ipte_unlock(vcpu); return rc; -- 2.25.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2] KVM: s390: gaccess: Refactor access address range check 2021-10-28 13:55 ` [PATCH v2] KVM: s390: gaccess: Refactor access address range check Janis Schoetterl-Glausch @ 2021-11-19 8:56 ` Janosch Frank 2021-11-19 10:01 ` Janis Schoetterl-Glausch 0 siblings, 1 reply; 13+ messages in thread From: Janosch Frank @ 2021-11-19 8:56 UTC (permalink / raw) To: Janis Schoetterl-Glausch, Christian Borntraeger, Heiko Carstens, Vasily Gorbik Cc: David Hildenbrand, Claudio Imbrenda, Alexander Gordeev, kvm, linux-s390, linux-kernel On 10/28/21 15:55, Janis Schoetterl-Glausch wrote: > Do not round down the first address to the page boundary, just translate > it normally, which gives the value we care about in the first place. > Given this, translating a single address is just the special case of > translating a range spanning a single page. > > Make the output optional, so the function can be used to just check a > range. > > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> Reviewed-by: Janosch Frank <frankja@linux.ibm.com> > --- > arch/s390/kvm/gaccess.c | 122 +++++++++++++++++++++++----------------- > 1 file changed, 69 insertions(+), 53 deletions(-) > > diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c > index 0d11cea92603..7725dd7566ed 100644 > --- a/arch/s390/kvm/gaccess.c > +++ b/arch/s390/kvm/gaccess.c > @@ -794,35 +794,74 @@ static int low_address_protection_enabled(struct kvm_vcpu *vcpu, > return 1; > } > > -static int guest_page_range(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, > - unsigned long *pages, unsigned long nr_pages, > - const union asce asce, enum gacc_mode mode) > +/** > + * guest_range_to_gpas() - Calculate guest physical addresses of page fragments > + * covering a logical range I'd add an empty line here. Apart from that this is a very nice cleanup. > + * @vcpu: virtual cpu > + * @ga: guest address, start of range > + * @ar: access register > + * @gpas: output argument, may be NULL > + * @len: length of range in bytes > + * @asce: address-space-control element to use for translation > + * @mode: access mode > + * > + * Translate a logical range to a series of guest absolute addresses, > + * such that the concatenation of page fragments starting at each gpa make up > + * the whole range. > + * The translation is performed as if done by the cpu for the given @asce, @ar, > + * @mode and state of the @vcpu. > + * If the translation causes an exception, its program interruption code is > + * returned and the &struct kvm_s390_pgm_info pgm member of @vcpu is modified > + * such that a subsequent call to kvm_s390_inject_prog_vcpu() will inject > + * a correct exception into the guest. > + * The resulting gpas are stored into @gpas, unless it is NULL. > + * > + * Note: All gpas except the first one start at the beginning of a page. > + * When deriving the boundaries of a fragment from a gpa, all but the last > + * fragment end at the end of the page. > + * > + * Return: > + * * 0 - success > + * * <0 - translation could not be performed, for example if guest > + * memory could not be accessed > + * * >0 - an access exception occurred. In this case the returned value > + * is the program interruption code and the contents of pgm may > + * be used to inject an exception into the guest. > + */ > +static int guest_range_to_gpas(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, > + unsigned long *gpas, unsigned long len, > + const union asce asce, enum gacc_mode mode) > { > psw_t *psw = &vcpu->arch.sie_block->gpsw; > + unsigned int offset = offset_in_page(ga); > + unsigned int fragment_len; > int lap_enabled, rc = 0; > enum prot_type prot; > + unsigned long gpa; > > lap_enabled = low_address_protection_enabled(vcpu, asce); > - while (nr_pages) { > + while (min(PAGE_SIZE - offset, len) > 0) { > + fragment_len = min(PAGE_SIZE - offset, len); > ga = kvm_s390_logical_to_effective(vcpu, ga); > if (mode == GACC_STORE && lap_enabled && is_low_address(ga)) > return trans_exc(vcpu, PGM_PROTECTION, ga, ar, mode, > PROT_TYPE_LA); > - ga &= PAGE_MASK; > if (psw_bits(*psw).dat) { > - rc = guest_translate(vcpu, ga, pages, asce, mode, &prot); > + rc = guest_translate(vcpu, ga, &gpa, asce, mode, &prot); > if (rc < 0) > return rc; > } else { > - *pages = kvm_s390_real_to_abs(vcpu, ga); > - if (kvm_is_error_gpa(vcpu->kvm, *pages)) > + gpa = kvm_s390_real_to_abs(vcpu, ga); > + if (kvm_is_error_gpa(vcpu->kvm, gpa)) > rc = PGM_ADDRESSING; > } > if (rc) > return trans_exc(vcpu, rc, ga, ar, mode, prot); > - ga += PAGE_SIZE; > - pages++; > - nr_pages--; > + if (gpas) > + *gpas++ = gpa; > + offset = 0; > + ga += fragment_len; > + len -= fragment_len; > } > return 0; > } > @@ -831,10 +870,10 @@ 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 nr_pages, gpa, idx; > + unsigned long nr_pages, idx; > unsigned int fragment_len; > - unsigned long pages_array[2]; > - unsigned long *pages; > + unsigned long gpa_array[2]; > + unsigned long *gpas; > int need_ipte_lock; > union asce asce; > int rc; > @@ -846,30 +885,28 @@ int access_guest(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, void *data, > if (rc) > return rc; > nr_pages = (((ga & ~PAGE_MASK) + len - 1) >> PAGE_SHIFT) + 1; > - pages = pages_array; > - if (nr_pages > ARRAY_SIZE(pages_array)) > - pages = vmalloc(array_size(nr_pages, sizeof(unsigned long))); > - if (!pages) > + gpas = gpa_array; > + if (nr_pages > ARRAY_SIZE(gpa_array)) > + gpas = vmalloc(array_size(nr_pages, sizeof(unsigned long))); > + if (!gpas) > return -ENOMEM; > need_ipte_lock = psw_bits(*psw).dat && !asce.r; > if (need_ipte_lock) > ipte_lock(vcpu); > - rc = guest_page_range(vcpu, ga, ar, pages, nr_pages, asce, mode); > + rc = guest_range_to_gpas(vcpu, ga, ar, gpas, len, asce, mode); > for (idx = 0; idx < nr_pages && !rc; idx++) { > - gpa = pages[idx] + offset_in_page(ga); > - fragment_len = min(PAGE_SIZE - offset_in_page(gpa), len); > + fragment_len = min(PAGE_SIZE - offset_in_page(gpas[idx]), len); > if (mode == GACC_STORE) > - rc = kvm_write_guest(vcpu->kvm, gpa, data, fragment_len); > + rc = kvm_write_guest(vcpu->kvm, gpas[idx], data, fragment_len); > else > - rc = kvm_read_guest(vcpu->kvm, gpa, data, fragment_len); > + rc = kvm_read_guest(vcpu->kvm, gpas[idx], data, fragment_len); > len -= fragment_len; > - ga += fragment_len; > data += fragment_len; > } > if (need_ipte_lock) > ipte_unlock(vcpu); > - if (nr_pages > ARRAY_SIZE(pages_array)) > - vfree(pages); > + if (nr_pages > ARRAY_SIZE(gpa_array)) > + vfree(gpas); > return rc; > } > > @@ -911,8 +948,6 @@ int access_guest_real(struct kvm_vcpu *vcpu, unsigned long gra, > int guest_translate_address(struct kvm_vcpu *vcpu, unsigned long gva, u8 ar, > unsigned long *gpa, enum gacc_mode mode) > { > - psw_t *psw = &vcpu->arch.sie_block->gpsw; > - enum prot_type prot; > union asce asce; > int rc; > > @@ -920,23 +955,7 @@ int guest_translate_address(struct kvm_vcpu *vcpu, unsigned long gva, u8 ar, > rc = get_vcpu_asce(vcpu, &asce, gva, ar, mode); > if (rc) > return rc; > - if (is_low_address(gva) && low_address_protection_enabled(vcpu, asce)) { > - if (mode == GACC_STORE) > - return trans_exc(vcpu, PGM_PROTECTION, gva, 0, > - mode, PROT_TYPE_LA); > - } > - > - if (psw_bits(*psw).dat && !asce.r) { /* Use DAT? */ > - rc = guest_translate(vcpu, gva, gpa, asce, mode, &prot); > - if (rc > 0) > - return trans_exc(vcpu, rc, gva, 0, mode, prot); > - } else { > - *gpa = kvm_s390_real_to_abs(vcpu, gva); > - if (kvm_is_error_gpa(vcpu->kvm, *gpa)) > - return trans_exc(vcpu, rc, gva, PGM_ADDRESSING, mode, 0); > - } > - > - return rc; > + return guest_range_to_gpas(vcpu, gva, ar, gpa, 1, asce, mode); > } > > /** > @@ -950,17 +969,14 @@ int guest_translate_address(struct kvm_vcpu *vcpu, unsigned long gva, u8 ar, > int check_gva_range(struct kvm_vcpu *vcpu, unsigned long gva, u8 ar, > unsigned long length, enum gacc_mode mode) > { > - unsigned long gpa; > - unsigned long currlen; > + union asce asce; > int rc = 0; > > + rc = get_vcpu_asce(vcpu, &asce, gva, ar, mode); > + if (rc) > + return rc; > ipte_lock(vcpu); > - while (length > 0 && !rc) { > - currlen = min(length, PAGE_SIZE - (gva % PAGE_SIZE)); > - rc = guest_translate_address(vcpu, gva, ar, &gpa, mode); > - gva += currlen; > - length -= currlen; > - } > + rc = guest_range_to_gpas(vcpu, gva, ar, NULL, length, asce, mode); > ipte_unlock(vcpu); > > return rc; > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] KVM: s390: gaccess: Refactor access address range check 2021-11-19 8:56 ` Janosch Frank @ 2021-11-19 10:01 ` Janis Schoetterl-Glausch 0 siblings, 0 replies; 13+ messages in thread From: Janis Schoetterl-Glausch @ 2021-11-19 10:01 UTC (permalink / raw) To: Janosch Frank, Janis Schoetterl-Glausch, Christian Borntraeger, Heiko Carstens, Vasily Gorbik Cc: David Hildenbrand, Claudio Imbrenda, Alexander Gordeev, kvm, linux-s390, linux-kernel On 11/19/21 09:56, Janosch Frank wrote: > On 10/28/21 15:55, Janis Schoetterl-Glausch wrote: >> Do not round down the first address to the page boundary, just translate >> it normally, which gives the value we care about in the first place. >> Given this, translating a single address is just the special case of >> translating a range spanning a single page. >> >> Make the output optional, so the function can be used to just check a >> range. >> >> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> > > Reviewed-by: Janosch Frank <frankja@linux.ibm.com> > >> --- >> arch/s390/kvm/gaccess.c | 122 +++++++++++++++++++++++----------------- >> 1 file changed, 69 insertions(+), 53 deletions(-) >> >> diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c >> index 0d11cea92603..7725dd7566ed 100644 >> --- a/arch/s390/kvm/gaccess.c >> +++ b/arch/s390/kvm/gaccess.c >> @@ -794,35 +794,74 @@ static int low_address_protection_enabled(struct kvm_vcpu *vcpu, >> return 1; >> } >> -static int guest_page_range(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, >> - unsigned long *pages, unsigned long nr_pages, >> - const union asce asce, enum gacc_mode mode) >> +/** >> + * guest_range_to_gpas() - Calculate guest physical addresses of page fragments >> + * covering a logical range > > I'd add an empty line here. The guide says not to. https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html : > Function parameters > > Each function argument should be described in order,immediately following the short function description. Do not leave a blank line between the function description and the arguments, nor between the arguments. In this case it's a static function, so not a must, but I'll stick to it anyway. > Apart from that this is a very nice cleanup. >>> + * @vcpu: virtual cpu >> + * @ga: guest address, start of range >> + * @ar: access register >> + * @gpas: output argument, may be NULL >> + * @len: length of range in bytes >> + * @asce: address-space-control element to use for translation >> + * @mode: access mode ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 3/3] KVM: s390: gaccess: Cleanup access to guest frames 2021-10-28 13:55 [PATCH v2 0/3] KVM: s390: Some gaccess cleanup Janis Schoetterl-Glausch 2021-10-28 13:55 ` [PATCH v2 1/3] KVM: s390: gaccess: Refactor gpa and length calculation Janis Schoetterl-Glausch 2021-10-28 13:55 ` [PATCH v2] KVM: s390: gaccess: Refactor access address range check Janis Schoetterl-Glausch @ 2021-10-28 13:55 ` Janis Schoetterl-Glausch 2021-10-28 14:25 ` David Hildenbrand 2021-11-23 12:39 ` [PATCH v2 0/3] KVM: s390: Some gaccess cleanup Janosch Frank 3 siblings, 1 reply; 13+ messages in thread From: Janis Schoetterl-Glausch @ 2021-10-28 13:55 UTC (permalink / raw) To: Christian Borntraeger, Janosch Frank, Heiko Carstens, Vasily Gorbik Cc: Janis Schoetterl-Glausch, David Hildenbrand, Claudio Imbrenda, Alexander Gordeev, kvm, linux-s390, linux-kernel Introduce a helper function for guest frame access. Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> --- arch/s390/kvm/gaccess.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c index f0848c37b003..9a633310b6fe 100644 --- a/arch/s390/kvm/gaccess.c +++ b/arch/s390/kvm/gaccess.c @@ -866,6 +866,20 @@ static int guest_range_to_gpas(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, return 0; } +static int access_guest_page(struct kvm *kvm, enum gacc_mode mode, gpa_t gpa, + void *data, unsigned int len) +{ + const unsigned int offset = offset_in_page(gpa); + const gfn_t gfn = gpa_to_gfn(gpa); + int rc; + + 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); + return rc; +} + int access_guest(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, void *data, unsigned long len, enum gacc_mode mode) { @@ -896,10 +910,7 @@ int access_guest(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, void *data, rc = guest_range_to_gpas(vcpu, ga, ar, gpas, len, asce, mode); for (idx = 0; idx < nr_pages && !rc; idx++) { fragment_len = min(PAGE_SIZE - offset_in_page(gpas[idx]), len); - if (mode == GACC_STORE) - rc = kvm_write_guest(vcpu->kvm, gpas[idx], data, fragment_len); - else - rc = kvm_read_guest(vcpu->kvm, gpas[idx], data, fragment_len); + rc = access_guest_page(vcpu->kvm, mode, gpas[idx], data, fragment_len); len -= fragment_len; data += fragment_len; } @@ -920,10 +931,7 @@ int access_guest_real(struct kvm_vcpu *vcpu, unsigned long gra, while (len && !rc) { gpa = kvm_s390_real_to_abs(vcpu, gra); fragment_len = min(PAGE_SIZE - offset_in_page(gpa), len); - if (mode) - rc = write_guest_abs(vcpu, gpa, data, fragment_len); - else - rc = read_guest_abs(vcpu, gpa, data, fragment_len); + rc = access_guest_page(vcpu->kvm, mode, gpa, data, fragment_len); len -= fragment_len; gra += fragment_len; data += fragment_len; -- 2.25.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/3] KVM: s390: gaccess: Cleanup access to guest frames 2021-10-28 13:55 ` [PATCH v2 3/3] KVM: s390: gaccess: Cleanup access to guest frames Janis Schoetterl-Glausch @ 2021-10-28 14:25 ` David Hildenbrand 2021-10-28 14:48 ` Janis Schoetterl-Glausch 0 siblings, 1 reply; 13+ messages in thread From: David Hildenbrand @ 2021-10-28 14:25 UTC (permalink / raw) To: Janis Schoetterl-Glausch, Christian Borntraeger, Janosch Frank, Heiko Carstens, Vasily Gorbik Cc: Claudio Imbrenda, Alexander Gordeev, kvm, linux-s390, linux-kernel On 28.10.21 15:55, Janis Schoetterl-Glausch wrote: > Introduce a helper function for guest frame access. "guest page access" But I do wonder if you actually want to call it "access_guest_abs" and say "guest absolute access" instead here. Because we're dealing with absolute addresses and the fact that we are accessing it page-wise is just because we have to perform a page-wise translation in the callers (either virtual->absolute or real->absolute). Theoretically, if you know you're across X pages but they are contiguous in absolute address space, nothing speaks against using that function directly across X pages with a single call. > > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> > --- > arch/s390/kvm/gaccess.c | 24 ++++++++++++++++-------- > 1 file changed, 16 insertions(+), 8 deletions(-) > > diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c > index f0848c37b003..9a633310b6fe 100644 > --- a/arch/s390/kvm/gaccess.c > +++ b/arch/s390/kvm/gaccess.c > @@ -866,6 +866,20 @@ static int guest_range_to_gpas(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, > return 0; > } > > +static int access_guest_page(struct kvm *kvm, enum gacc_mode mode, gpa_t gpa, > + void *data, unsigned int len) > +{ > + const unsigned int offset = offset_in_page(gpa); > + const gfn_t gfn = gpa_to_gfn(gpa); > + int rc; > + > + 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); > + return rc; > +} > + > int access_guest(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, void *data, > unsigned long len, enum gacc_mode mode) > { > @@ -896,10 +910,7 @@ int access_guest(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, void *data, > rc = guest_range_to_gpas(vcpu, ga, ar, gpas, len, asce, mode); > for (idx = 0; idx < nr_pages && !rc; idx++) { > fragment_len = min(PAGE_SIZE - offset_in_page(gpas[idx]), len); > - if (mode == GACC_STORE) > - rc = kvm_write_guest(vcpu->kvm, gpas[idx], data, fragment_len); > - else > - rc = kvm_read_guest(vcpu->kvm, gpas[idx], data, fragment_len); > + rc = access_guest_page(vcpu->kvm, mode, gpas[idx], data, fragment_len); > len -= fragment_len; > data += fragment_len; > } > @@ -920,10 +931,7 @@ int access_guest_real(struct kvm_vcpu *vcpu, unsigned long gra, > while (len && !rc) { > gpa = kvm_s390_real_to_abs(vcpu, gra); > fragment_len = min(PAGE_SIZE - offset_in_page(gpa), len); > - if (mode) > - rc = write_guest_abs(vcpu, gpa, data, fragment_len); > - else > - rc = read_guest_abs(vcpu, gpa, data, fragment_len); > + rc = access_guest_page(vcpu->kvm, mode, gpa, data, fragment_len); > len -= fragment_len; > gra += fragment_len; > data += fragment_len; > -- Thanks, David / dhildenb ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/3] KVM: s390: gaccess: Cleanup access to guest frames 2021-10-28 14:25 ` David Hildenbrand @ 2021-10-28 14:48 ` Janis Schoetterl-Glausch 2021-11-19 9:00 ` Janosch Frank 0 siblings, 1 reply; 13+ messages in thread From: Janis Schoetterl-Glausch @ 2021-10-28 14:48 UTC (permalink / raw) To: David Hildenbrand, Janis Schoetterl-Glausch, Christian Borntraeger, Janosch Frank, Heiko Carstens, Vasily Gorbik Cc: Claudio Imbrenda, Alexander Gordeev, kvm, linux-s390, linux-kernel On 10/28/21 16:25, David Hildenbrand wrote: > On 28.10.21 15:55, Janis Schoetterl-Glausch wrote: >> Introduce a helper function for guest frame access. > > "guest page access" Ok. > > But I do wonder if you actually want to call it > > "access_guest_abs" > > and say "guest absolute access" instead here. > > Because we're dealing with absolute addresses and the fact that we are > accessing it page-wise is just because we have to perform a page-wise > translation in the callers (either virtual->absolute or real->absolute). > > Theoretically, if you know you're across X pages but they are contiguous > in absolute address space, nothing speaks against using that function > directly across X pages with a single call. There currently is no point to this, is there? kvm_read/write_guest break the region up into pages anyway, so no reason to try to identify larger continuous chunks. > >> >> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> >> --- >> arch/s390/kvm/gaccess.c | 24 ++++++++++++++++-------- >> 1 file changed, 16 insertions(+), 8 deletions(-) >> >> diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c >> index f0848c37b003..9a633310b6fe 100644 >> --- a/arch/s390/kvm/gaccess.c >> +++ b/arch/s390/kvm/gaccess.c >> @@ -866,6 +866,20 @@ static int guest_range_to_gpas(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, >> return 0; >> } >> >> +static int access_guest_page(struct kvm *kvm, enum gacc_mode mode, gpa_t gpa, >> + void *data, unsigned int len) >> +{ >> + const unsigned int offset = offset_in_page(gpa); >> + const gfn_t gfn = gpa_to_gfn(gpa); >> + int rc; >> + >> + 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); >> + return rc; >> +} >> + >> int access_guest(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, void *data, >> unsigned long len, enum gacc_mode mode) >> { >> @@ -896,10 +910,7 @@ int access_guest(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, void *data, >> rc = guest_range_to_gpas(vcpu, ga, ar, gpas, len, asce, mode); >> for (idx = 0; idx < nr_pages && !rc; idx++) { >> fragment_len = min(PAGE_SIZE - offset_in_page(gpas[idx]), len); >> - if (mode == GACC_STORE) >> - rc = kvm_write_guest(vcpu->kvm, gpas[idx], data, fragment_len); >> - else >> - rc = kvm_read_guest(vcpu->kvm, gpas[idx], data, fragment_len); >> + rc = access_guest_page(vcpu->kvm, mode, gpas[idx], data, fragment_len); >> len -= fragment_len; >> data += fragment_len; >> } >> @@ -920,10 +931,7 @@ int access_guest_real(struct kvm_vcpu *vcpu, unsigned long gra, >> while (len && !rc) { >> gpa = kvm_s390_real_to_abs(vcpu, gra); >> fragment_len = min(PAGE_SIZE - offset_in_page(gpa), len); >> - if (mode) >> - rc = write_guest_abs(vcpu, gpa, data, fragment_len); >> - else >> - rc = read_guest_abs(vcpu, gpa, data, fragment_len); >> + rc = access_guest_page(vcpu->kvm, mode, gpa, data, fragment_len); >> len -= fragment_len; >> gra += fragment_len; >> data += fragment_len; >> > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/3] KVM: s390: gaccess: Cleanup access to guest frames 2021-10-28 14:48 ` Janis Schoetterl-Glausch @ 2021-11-19 9:00 ` Janosch Frank 2021-11-22 11:13 ` David Hildenbrand 0 siblings, 1 reply; 13+ messages in thread From: Janosch Frank @ 2021-11-19 9:00 UTC (permalink / raw) To: Janis Schoetterl-Glausch, David Hildenbrand, Janis Schoetterl-Glausch, Christian Borntraeger, Heiko Carstens, Vasily Gorbik Cc: Claudio Imbrenda, Alexander Gordeev, kvm, linux-s390, linux-kernel On 10/28/21 16:48, Janis Schoetterl-Glausch wrote: > On 10/28/21 16:25, David Hildenbrand wrote: >> On 28.10.21 15:55, Janis Schoetterl-Glausch wrote: >>> Introduce a helper function for guest frame access. >> >> "guest page access" > > Ok. >> >> But I do wonder if you actually want to call it >> >> "access_guest_abs" >> >> and say "guest absolute access" instead here. >> >> Because we're dealing with absolute addresses and the fact that we are >> accessing it page-wise is just because we have to perform a page-wise >> translation in the callers (either virtual->absolute or real->absolute). >> >> Theoretically, if you know you're across X pages but they are contiguous >> in absolute address space, nothing speaks against using that function >> directly across X pages with a single call. > > There currently is no point to this, is there? > kvm_read/write_guest break the region up into pages anyway, > so no reason to try to identify larger continuous chunks. Considering that we call kvm functions that have page in the name and this is directly over the function where it's used I'd leave the naming as it is. @David: How strongly do you feel about this? Reviewed-by: Janosch Frank <frankja@linux.ibm.com> >> >>> >>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> >>> --- >>> arch/s390/kvm/gaccess.c | 24 ++++++++++++++++-------- >>> 1 file changed, 16 insertions(+), 8 deletions(-) >>> >>> diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c >>> index f0848c37b003..9a633310b6fe 100644 >>> --- a/arch/s390/kvm/gaccess.c >>> +++ b/arch/s390/kvm/gaccess.c >>> @@ -866,6 +866,20 @@ static int guest_range_to_gpas(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, >>> return 0; >>> } >>> >>> +static int access_guest_page(struct kvm *kvm, enum gacc_mode mode, gpa_t gpa, >>> + void *data, unsigned int len) >>> +{ >>> + const unsigned int offset = offset_in_page(gpa); >>> + const gfn_t gfn = gpa_to_gfn(gpa); >>> + int rc; >>> + >>> + 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); >>> + return rc; >>> +} >>> + >>> int access_guest(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, void *data, >>> unsigned long len, enum gacc_mode mode) >>> { >>> @@ -896,10 +910,7 @@ int access_guest(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, void *data, >>> rc = guest_range_to_gpas(vcpu, ga, ar, gpas, len, asce, mode); >>> for (idx = 0; idx < nr_pages && !rc; idx++) { >>> fragment_len = min(PAGE_SIZE - offset_in_page(gpas[idx]), len); >>> - if (mode == GACC_STORE) >>> - rc = kvm_write_guest(vcpu->kvm, gpas[idx], data, fragment_len); >>> - else >>> - rc = kvm_read_guest(vcpu->kvm, gpas[idx], data, fragment_len); >>> + rc = access_guest_page(vcpu->kvm, mode, gpas[idx], data, fragment_len); >>> len -= fragment_len; >>> data += fragment_len; >>> } >>> @@ -920,10 +931,7 @@ int access_guest_real(struct kvm_vcpu *vcpu, unsigned long gra, >>> while (len && !rc) { >>> gpa = kvm_s390_real_to_abs(vcpu, gra); >>> fragment_len = min(PAGE_SIZE - offset_in_page(gpa), len); >>> - if (mode) >>> - rc = write_guest_abs(vcpu, gpa, data, fragment_len); >>> - else >>> - rc = read_guest_abs(vcpu, gpa, data, fragment_len); >>> + rc = access_guest_page(vcpu->kvm, mode, gpa, data, fragment_len); >>> len -= fragment_len; >>> gra += fragment_len; >>> data += fragment_len; >>> >> >> > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/3] KVM: s390: gaccess: Cleanup access to guest frames 2021-11-19 9:00 ` Janosch Frank @ 2021-11-22 11:13 ` David Hildenbrand 0 siblings, 0 replies; 13+ messages in thread From: David Hildenbrand @ 2021-11-22 11:13 UTC (permalink / raw) To: Janosch Frank, Janis Schoetterl-Glausch, Janis Schoetterl-Glausch, Christian Borntraeger, Heiko Carstens, Vasily Gorbik Cc: Claudio Imbrenda, Alexander Gordeev, kvm, linux-s390, linux-kernel On 19.11.21 10:00, Janosch Frank wrote: > On 10/28/21 16:48, Janis Schoetterl-Glausch wrote: >> On 10/28/21 16:25, David Hildenbrand wrote: >>> On 28.10.21 15:55, Janis Schoetterl-Glausch wrote: >>>> Introduce a helper function for guest frame access. >>> >>> "guest page access" >> >> Ok. >>> >>> But I do wonder if you actually want to call it >>> >>> "access_guest_abs" >>> >>> and say "guest absolute access" instead here. >>> >>> Because we're dealing with absolute addresses and the fact that we are >>> accessing it page-wise is just because we have to perform a page-wise >>> translation in the callers (either virtual->absolute or real->absolute). >>> >>> Theoretically, if you know you're across X pages but they are contiguous >>> in absolute address space, nothing speaks against using that function >>> directly across X pages with a single call. >> >> There currently is no point to this, is there? >> kvm_read/write_guest break the region up into pages anyway, >> so no reason to try to identify larger continuous chunks. > Right, we're changing the calls from e.g., kvm_write_guest() and write_guest_abs() to kvm_write_guest_page(). As we're not exposing this function via arch/s390/kvm/gaccess.h, I think it's ok. Because for external functions we have nice function names like write_guest_abs(), write_guest_real(), write_guest_lc(), write_guest(), which implicitly state in their name which kind of address they expect. access_guest_page() now accepts an absolute address whereby access_guest() accepts a virtual address. This is for example different to kvm_read_guest() and kvm_read_guest_page(), which expect absolute addresses. But there, the _page functions are not internal helpers. > > @David: How strongly do you feel about this? Not strongly :) -- Thanks, David / dhildenb ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/3] KVM: s390: Some gaccess cleanup 2021-10-28 13:55 [PATCH v2 0/3] KVM: s390: Some gaccess cleanup Janis Schoetterl-Glausch ` (2 preceding siblings ...) 2021-10-28 13:55 ` [PATCH v2 3/3] KVM: s390: gaccess: Cleanup access to guest frames Janis Schoetterl-Glausch @ 2021-11-23 12:39 ` Janosch Frank 3 siblings, 0 replies; 13+ messages in thread From: Janosch Frank @ 2021-11-23 12:39 UTC (permalink / raw) To: Janis Schoetterl-Glausch, Christian Borntraeger, Heiko Carstens, Vasily Gorbik Cc: David Hildenbrand, Claudio Imbrenda, Alexander Gordeev, kvm, linux-s390, linux-kernel On 10/28/21 15:55, Janis Schoetterl-Glausch wrote: > Cleanup s390 guest access code a bit, getting rid of some code > duplication and improving readability. > > v1 -> v2 > separate patch for renamed variable > fragment_len instead of seg > expand comment of guest_range_to_gpas > fix nits > > I did not pick up Janosch's Reviewed-by because of the split patch > and the changed variable name. > > Janis Schoetterl-Glausch (3): > KVM: s390: gaccess: Refactor gpa and length calculation > KVM: s390: gaccess: Refactor access address range check > KVM: s390: gaccess: Cleanup access to guest frames > > arch/s390/kvm/gaccess.c | 158 +++++++++++++++++++++++----------------- > 1 file changed, 92 insertions(+), 66 deletions(-) > Could you please push this to devel so we get some CI coverage? ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-11-23 12:39 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-28 13:55 [PATCH v2 0/3] KVM: s390: Some gaccess cleanup Janis Schoetterl-Glausch 2021-10-28 13:55 ` [PATCH v2 1/3] KVM: s390: gaccess: Refactor gpa and length calculation Janis Schoetterl-Glausch 2021-11-19 8:27 ` Janosch Frank 2021-11-23 12:38 ` Janosch Frank 2021-10-28 13:55 ` [PATCH v2] KVM: s390: gaccess: Refactor access address range check Janis Schoetterl-Glausch 2021-11-19 8:56 ` Janosch Frank 2021-11-19 10:01 ` Janis Schoetterl-Glausch 2021-10-28 13:55 ` [PATCH v2 3/3] KVM: s390: gaccess: Cleanup access to guest frames Janis Schoetterl-Glausch 2021-10-28 14:25 ` David Hildenbrand 2021-10-28 14:48 ` Janis Schoetterl-Glausch 2021-11-19 9:00 ` Janosch Frank 2021-11-22 11:13 ` David Hildenbrand 2021-11-23 12:39 ` [PATCH v2 0/3] KVM: s390: Some gaccess cleanup Janosch Frank
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).