[PATCH v2 0/3] KVM: s390: Some gaccess cleanup

[PATCH v2 0/3] KVM: s390: Some gaccess cleanup

[Janis Schoetterl-Glausch]

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

[PATCH v2 1/3] KVM: s390: gaccess: Refactor gpa and length calculation

[Janis Schoetterl-Glausch]

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

[PATCH v2] KVM: s390: gaccess: Refactor access address range check

[Janis Schoetterl-Glausch]

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

[PATCH v2 3/3] KVM: s390: gaccess: Cleanup access to guest frames

[Janis Schoetterl-Glausch]

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

Re: [PATCH v2 3/3] KVM: s390: gaccess: Cleanup access to guest frames

[David Hildenbrand]

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

Re: [PATCH v2 3/3] KVM: s390: gaccess: Cleanup access to guest frames

[Janis Schoetterl-Glausch]

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

Re: [PATCH v2 1/3] KVM: s390: gaccess: Refactor gpa and length calculation

[Janosch Frank]

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

Re: [PATCH v2] KVM: s390: gaccess: Refactor access address range check

[Janosch Frank]

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

Re: [PATCH v2 3/3] KVM: s390: gaccess: Cleanup access to guest frames

[Janosch Frank]

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

Re: [PATCH v2] KVM: s390: gaccess: Refactor access address range check

[Janis Schoetterl-Glausch]

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

Re: [PATCH v2 3/3] KVM: s390: gaccess: Cleanup access to guest frames

[David Hildenbrand]

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

Re: [PATCH v2 1/3] KVM: s390: gaccess: Refactor gpa and length calculation

[Janosch Frank]

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

Re: [PATCH v2 0/3] KVM: s390: Some gaccess cleanup

[Janosch Frank]

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?