linux-s390.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/3] KVM: s390: Some gaccess cleanup
@ 2021-11-26 16:45 Janis Schoetterl-Glausch
  2021-11-26 16:45 ` [PATCH v3 1/3] KVM: s390: gaccess: Refactor gpa and length calculation Janis Schoetterl-Glausch
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Janis Schoetterl-Glausch @ 2021-11-26 16:45 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.

v2 -> v3
	minor changes only
		typo fixes
		whitespace
		line reordering
		picked up Reviewed-by's

v1 -> v2
	separate patch for renamed variable
		fragment_len instead of seg
	expand comment of guest_range_to_gpas
	fix nits

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 pages

 arch/s390/kvm/gaccess.c | 158 +++++++++++++++++++++++-----------------
 1 file changed, 92 insertions(+), 66 deletions(-)

Range-diff against v2:
1:  60d050210198 ! 1:  e5d7d2d7a4da KVM: s390: gaccess: Refactor gpa and length calculation
    @@ Metadata
      ## Commit message ##
         KVM: s390: gaccess: Refactor gpa and length calculation
     
    -    Improve readability be renaming the length variable and
    +    Improve readability by 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 ##
     @@ arch/s390/kvm/gaccess.c: int access_guest(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, void *data,
    @@ arch/s390/kvm/gaccess.c: int access_guest(struct kvm_vcpu *vcpu, unsigned long g
      	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 int fragment_len;
      	unsigned long *pages;
      	int need_ipte_lock;
    + 	union asce asce;
     @@ arch/s390/kvm/gaccess.c: 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);
2:  7080846c8c07 ! 2:  91cadb42cbbc KVM: s390: gaccess: Refactor access address range check
    @@ Commit message
         range.
     
         Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
    +    Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
     
      ## arch/s390/kvm/gaccess.c ##
     @@ arch/s390/kvm/gaccess.c: static int low_address_protection_enabled(struct kvm_vcpu *vcpu,
    @@ arch/s390/kvm/gaccess.c: static int low_address_protection_enabled(struct kvm_vc
     + * 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.
    ++ * Note: All fragments 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.
     + *
    @@ arch/s390/kvm/gaccess.c: int access_guest(struct kvm_vcpu *vcpu, unsigned long g
      {
      	psw_t *psw = &vcpu->arch.sie_block->gpsw;
     -	unsigned long nr_pages, gpa, idx;
    +-	unsigned long pages_array[2];
     +	unsigned long nr_pages, idx;
    ++	unsigned long gpa_array[2];
      	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;
3:  c991cbdbfbd5 ! 3:  f5000a22efcd KVM: s390: gaccess: Cleanup access to guest frames
    @@ Metadata
     Author: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
     
      ## Commit message ##
    -    KVM: s390: gaccess: Cleanup access to guest frames
    +    KVM: s390: gaccess: Cleanup access to guest pages
     
         Introduce a helper function for guest frame access.
     
         Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
    +    Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
     
      ## arch/s390/kvm/gaccess.c ##
     @@ arch/s390/kvm/gaccess.c: static int guest_range_to_gpas(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar,
    @@ arch/s390/kvm/gaccess.c: static int guest_range_to_gpas(struct kvm_vcpu *vcpu, u
      }
      
     +static int access_guest_page(struct kvm *kvm, enum gacc_mode mode, gpa_t gpa,
    -+			      void *data, unsigned int len)
    ++			     void *data, unsigned int len)
     +{
     +	const unsigned int offset = offset_in_page(gpa);
     +	const gfn_t gfn = gpa_to_gfn(gpa);

base-commit: d25f27432f80a800a3592db128254c8140bd71bf
-- 
2.32.0


^ permalink raw reply	[flat|nested] 11+ messages in thread
* [PATCH v3 1/3] KVM: s390: gaccess: Refactor gpa and length calculation
  2021-11-26 16:45 [PATCH v3 0/3] KVM: s390: Some gaccess cleanup Janis Schoetterl-Glausch
@ 2021-11-26 16:45 ` Janis Schoetterl-Glausch
  2021-11-26 17:10   ` David Hildenbrand
  2021-11-29  8:55   ` Claudio Imbrenda
  2021-11-26 16:45 ` [PATCH v3 2/3] KVM: s390: gaccess: Refactor access address range check Janis Schoetterl-Glausch
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 11+ messages in thread
From: Janis Schoetterl-Glausch @ 2021-11-26 16:45 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 by 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..45966fbba182 100644
--- a/arch/s390/kvm/gaccess.c
+++ b/arch/s390/kvm/gaccess.c
@@ -831,8 +831,9 @@ 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 long pages_array[2];
+	unsigned int fragment_len;
 	unsigned long *pages;
 	int need_ipte_lock;
 	union asce asce;
@@ -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.32.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [PATCH v3 2/3] KVM: s390: gaccess: Refactor access address range check
  2021-11-26 16:45 [PATCH v3 0/3] KVM: s390: Some gaccess cleanup Janis Schoetterl-Glausch
  2021-11-26 16:45 ` [PATCH v3 1/3] KVM: s390: gaccess: Refactor gpa and length calculation Janis Schoetterl-Glausch
@ 2021-11-26 16:45 ` Janis Schoetterl-Glausch
  2021-11-29  9:13   ` Claudio Imbrenda
  2021-11-26 16:45 ` [PATCH v3 3/3] KVM: s390: gaccess: Cleanup access to guest pages Janis Schoetterl-Glausch
  2021-12-07  9:27 ` [PATCH v3 0/3] KVM: s390: Some gaccess cleanup Janosch Frank
  3 siblings, 1 reply; 11+ messages in thread
From: Janis Schoetterl-Glausch @ 2021-11-26 16:45 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>
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 45966fbba182..c09659609d68 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 fragments 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 pages_array[2];
+	unsigned long nr_pages, idx;
+	unsigned long gpa_array[2];
 	unsigned int fragment_len;
-	unsigned long *pages;
+	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.32.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [PATCH v3 3/3] KVM: s390: gaccess: Cleanup access to guest pages
  2021-11-26 16:45 [PATCH v3 0/3] KVM: s390: Some gaccess cleanup Janis Schoetterl-Glausch
  2021-11-26 16:45 ` [PATCH v3 1/3] KVM: s390: gaccess: Refactor gpa and length calculation Janis Schoetterl-Glausch
  2021-11-26 16:45 ` [PATCH v3 2/3] KVM: s390: gaccess: Refactor access address range check Janis Schoetterl-Glausch
@ 2021-11-26 16:45 ` Janis Schoetterl-Glausch
  2021-11-26 17:09   ` David Hildenbrand
  2021-11-29  8:59   ` Claudio Imbrenda
  2021-12-07  9:27 ` [PATCH v3 0/3] KVM: s390: Some gaccess cleanup Janosch Frank
  3 siblings, 2 replies; 11+ messages in thread
From: Janis Schoetterl-Glausch @ 2021-11-26 16:45 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>
Reviewed-by: Janosch Frank <frankja@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 c09659609d68..9193f0de40b1 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.32.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread
* Re: [PATCH v3 3/3] KVM: s390: gaccess: Cleanup access to guest pages
  2021-11-26 16:45 ` [PATCH v3 3/3] KVM: s390: gaccess: Cleanup access to guest pages Janis Schoetterl-Glausch
@ 2021-11-26 17:09   ` David Hildenbrand
  2021-11-29  8:59   ` Claudio Imbrenda
  1 sibling, 0 replies; 11+ messages in thread
From: David Hildenbrand @ 2021-11-26 17:09 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 26.11.21 17:45, Janis Schoetterl-Glausch wrote:
> Introduce a helper function for guest frame access.
> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> Reviewed-by: Janosch Frank <frankja@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 c09659609d68..9193f0de40b1 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;
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb


^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/3] KVM: s390: gaccess: Refactor gpa and length calculation
  2021-11-26 16:45 ` [PATCH v3 1/3] KVM: s390: gaccess: Refactor gpa and length calculation Janis Schoetterl-Glausch
@ 2021-11-26 17:10   ` David Hildenbrand
  2021-11-29  8:55   ` Claudio Imbrenda
  1 sibling, 0 replies; 11+ messages in thread
From: David Hildenbrand @ 2021-11-26 17:10 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 26.11.21 17:45, Janis Schoetterl-Glausch wrote:
> Improve readability by 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(-)

Reviewed-by: David Hildenbrand <david@redhat.com>


-- 
Thanks,

David / dhildenb


^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/3] KVM: s390: gaccess: Refactor gpa and length calculation
  2021-11-26 16:45 ` [PATCH v3 1/3] KVM: s390: gaccess: Refactor gpa and length calculation Janis Schoetterl-Glausch
  2021-11-26 17:10   ` David Hildenbrand
@ 2021-11-29  8:55   ` Claudio Imbrenda
  1 sibling, 0 replies; 11+ messages in thread
From: Claudio Imbrenda @ 2021-11-29  8:55 UTC (permalink / raw)
  To: Janis Schoetterl-Glausch
  Cc: Christian Borntraeger, Janosch Frank, Heiko Carstens,
	Vasily Gorbik, David Hildenbrand, Alexander Gordeev, kvm,
	linux-s390, linux-kernel

On Fri, 26 Nov 2021 17:45:47 +0100
Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

> Improve readability by 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>

Reviewed-by: Claudio Imbrenda <imbrenda@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..45966fbba182 100644
> --- a/arch/s390/kvm/gaccess.c
> +++ b/arch/s390/kvm/gaccess.c
> @@ -831,8 +831,9 @@ 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 long pages_array[2];
> +	unsigned int fragment_len;
>  	unsigned long *pages;
>  	int need_ipte_lock;
>  	union asce asce;
> @@ -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] 11+ messages in thread
* Re: [PATCH v3 3/3] KVM: s390: gaccess: Cleanup access to guest pages
  2021-11-26 16:45 ` [PATCH v3 3/3] KVM: s390: gaccess: Cleanup access to guest pages Janis Schoetterl-Glausch
  2021-11-26 17:09   ` David Hildenbrand
@ 2021-11-29  8:59   ` Claudio Imbrenda
  2021-11-29  9:17     ` Janis Schoetterl-Glausch
  1 sibling, 1 reply; 11+ messages in thread
From: Claudio Imbrenda @ 2021-11-29  8:59 UTC (permalink / raw)
  To: Janis Schoetterl-Glausch
  Cc: Christian Borntraeger, Janosch Frank, Heiko Carstens,
	Vasily Gorbik, David Hildenbrand, Alexander Gordeev, kvm,
	linux-s390, linux-kernel

On Fri, 26 Nov 2021 17:45:49 +0100
Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

> Introduce a helper function for guest frame access.
> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

see a small nit below

> ---
>  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 c09659609d68..9193f0de40b1 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);

why not just return ?

(but don't bother with a v4, it's ok anyway)

> +	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] 11+ messages in thread
* Re: [PATCH v3 2/3] KVM: s390: gaccess: Refactor access address range check
  2021-11-26 16:45 ` [PATCH v3 2/3] KVM: s390: gaccess: Refactor access address range check Janis Schoetterl-Glausch
@ 2021-11-29  9:13   ` Claudio Imbrenda
  0 siblings, 0 replies; 11+ messages in thread
From: Claudio Imbrenda @ 2021-11-29  9:13 UTC (permalink / raw)
  To: Janis Schoetterl-Glausch
  Cc: Christian Borntraeger, Janosch Frank, Heiko Carstens,
	Vasily Gorbik, David Hildenbrand, Alexander Gordeev, kvm,
	linux-s390, linux-kernel

On Fri, 26 Nov 2021 17:45:48 +0100
Janis Schoetterl-Glausch <scgl@linux.ibm.com> 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>

Reviewed-by: Claudio Imbrenda <imbrenda@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 45966fbba182..c09659609d68 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 fragments 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 pages_array[2];
> +	unsigned long nr_pages, idx;
> +	unsigned long gpa_array[2];
>  	unsigned int fragment_len;
> -	unsigned long *pages;
> +	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] 11+ messages in thread
* Re: [PATCH v3 3/3] KVM: s390: gaccess: Cleanup access to guest pages
  2021-11-29  8:59   ` Claudio Imbrenda
@ 2021-11-29  9:17     ` Janis Schoetterl-Glausch
  0 siblings, 0 replies; 11+ messages in thread
From: Janis Schoetterl-Glausch @ 2021-11-29  9:17 UTC (permalink / raw)
  To: Claudio Imbrenda, Janis Schoetterl-Glausch
  Cc: Christian Borntraeger, Janosch Frank, Heiko Carstens,
	Vasily Gorbik, David Hildenbrand, Alexander Gordeev, kvm,
	linux-s390, linux-kernel

On 11/29/21 09:59, Claudio Imbrenda wrote:
> On Fri, 26 Nov 2021 17:45:49 +0100
> Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
> 
>> Introduce a helper function for guest frame access.
>>
>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
> 
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

Thanks.
> 
> see a small nit below
> 
>> ---
>>  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 c09659609d68..9193f0de40b1 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);
> 
> why not just return ?

No clue, maybe I wanted to look at the rc while debugging something?
> 
> (but don't bother with a v4, it's ok anyway)
> 
>> +	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] 11+ messages in thread
* Re: [PATCH v3 0/3] KVM: s390: Some gaccess cleanup
  2021-11-26 16:45 [PATCH v3 0/3] KVM: s390: Some gaccess cleanup Janis Schoetterl-Glausch
                   ` (2 preceding siblings ...)
  2021-11-26 16:45 ` [PATCH v3 3/3] KVM: s390: gaccess: Cleanup access to guest pages Janis Schoetterl-Glausch
@ 2021-12-07  9:27 ` Janosch Frank
  3 siblings, 0 replies; 11+ messages in thread
From: Janosch Frank @ 2021-12-07  9: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 11/26/21 17:45, Janis Schoetterl-Glausch wrote:
> Cleanup s390 guest access code a bit, getting rid of some code
> duplication and improving readability.
> 
> v2 -> v3
> 	minor changes only
> 		typo fixes
> 		whitespace
> 		line reordering
> 		picked up Reviewed-by's
> 
> v1 -> v2
> 	separate patch for renamed variable
> 		fragment_len instead of seg
> 	expand comment of guest_range_to_gpas
> 	fix nits

Thanks, picked

> 
> 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 pages
> 
>   arch/s390/kvm/gaccess.c | 158 +++++++++++++++++++++++-----------------
>   1 file changed, 92 insertions(+), 66 deletions(-)
> 
> Range-diff against v2:
> 1:  60d050210198 ! 1:  e5d7d2d7a4da KVM: s390: gaccess: Refactor gpa and length calculation
>      @@ Metadata
>        ## Commit message ##
>           KVM: s390: gaccess: Refactor gpa and length calculation
>       
>      -    Improve readability be renaming the length variable and
>      +    Improve readability by 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 ##
>       @@ arch/s390/kvm/gaccess.c: int access_guest(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, void *data,
>      @@ arch/s390/kvm/gaccess.c: int access_guest(struct kvm_vcpu *vcpu, unsigned long g
>        	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 int fragment_len;
>        	unsigned long *pages;
>        	int need_ipte_lock;
>      + 	union asce asce;
>       @@ arch/s390/kvm/gaccess.c: 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);
> 2:  7080846c8c07 ! 2:  91cadb42cbbc KVM: s390: gaccess: Refactor access address range check
>      @@ Commit message
>           range.
>       
>           Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>      +    Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>       
>        ## arch/s390/kvm/gaccess.c ##
>       @@ arch/s390/kvm/gaccess.c: static int low_address_protection_enabled(struct kvm_vcpu *vcpu,
>      @@ arch/s390/kvm/gaccess.c: static int low_address_protection_enabled(struct kvm_vc
>       + * 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.
>      ++ * Note: All fragments 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.
>       + *
>      @@ arch/s390/kvm/gaccess.c: int access_guest(struct kvm_vcpu *vcpu, unsigned long g
>        {
>        	psw_t *psw = &vcpu->arch.sie_block->gpsw;
>       -	unsigned long nr_pages, gpa, idx;
>      +-	unsigned long pages_array[2];
>       +	unsigned long nr_pages, idx;
>      ++	unsigned long gpa_array[2];
>        	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;
> 3:  c991cbdbfbd5 ! 3:  f5000a22efcd KVM: s390: gaccess: Cleanup access to guest frames
>      @@ Metadata
>       Author: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>       
>        ## Commit message ##
>      -    KVM: s390: gaccess: Cleanup access to guest frames
>      +    KVM: s390: gaccess: Cleanup access to guest pages
>       
>           Introduce a helper function for guest frame access.
>       
>           Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>      +    Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>       
>        ## arch/s390/kvm/gaccess.c ##
>       @@ arch/s390/kvm/gaccess.c: static int guest_range_to_gpas(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar,
>      @@ arch/s390/kvm/gaccess.c: static int guest_range_to_gpas(struct kvm_vcpu *vcpu, u
>        }
>        
>       +static int access_guest_page(struct kvm *kvm, enum gacc_mode mode, gpa_t gpa,
>      -+			      void *data, unsigned int len)
>      ++			     void *data, unsigned int len)
>       +{
>       +	const unsigned int offset = offset_in_page(gpa);
>       +	const gfn_t gfn = gpa_to_gfn(gpa);
> 
> base-commit: d25f27432f80a800a3592db128254c8140bd71bf
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-12-07  9:27 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-26 16:45 [PATCH v3 0/3] KVM: s390: Some gaccess cleanup Janis Schoetterl-Glausch
2021-11-26 16:45 ` [PATCH v3 1/3] KVM: s390: gaccess: Refactor gpa and length calculation Janis Schoetterl-Glausch
2021-11-26 17:10   ` David Hildenbrand
2021-11-29  8:55   ` Claudio Imbrenda
2021-11-26 16:45 ` [PATCH v3 2/3] KVM: s390: gaccess: Refactor access address range check Janis Schoetterl-Glausch
2021-11-29  9:13   ` Claudio Imbrenda
2021-11-26 16:45 ` [PATCH v3 3/3] KVM: s390: gaccess: Cleanup access to guest pages Janis Schoetterl-Glausch
2021-11-26 17:09   ` David Hildenbrand
2021-11-29  8:59   ` Claudio Imbrenda
2021-11-29  9:17     ` Janis Schoetterl-Glausch
2021-12-07  9:27 ` [PATCH v3 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).