Introduce a helper function for guest frame access.
Rewrite the calculation of the gpa and the length of the segment
to be more readable.
Signed-off-by: Janis Schoetterl-Glausch <[email protected]>
---
arch/s390/kvm/gaccess.c | 48 +++++++++++++++++++++++++----------------
1 file changed, 29 insertions(+), 19 deletions(-)
diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
index b9f85b2dc053..df83de0843de 100644
--- a/arch/s390/kvm/gaccess.c
+++ b/arch/s390/kvm/gaccess.c
@@ -827,11 +827,26 @@ static int guest_page_range(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar,
return 0;
}
+static int access_guest_frame(struct kvm *kvm, enum gacc_mode mode, gpa_t gpa,
+ void *data, unsigned int len)
+{
+ gfn_t gfn = gpa_to_gfn(gpa);
+ unsigned int offset = offset_in_page(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)
{
psw_t *psw = &vcpu->arch.sie_block->gpsw;
- unsigned long _len, nr_pages, gpa, idx;
+ unsigned long nr_pages, gpa, idx;
+ unsigned int seg;
unsigned long pages_array[2];
unsigned long *pages;
int need_ipte_lock;
@@ -855,15 +870,12 @@ int access_guest(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, void *data,
ipte_lock(vcpu);
rc = guest_page_range(vcpu, ga, ar, pages, nr_pages, asce, mode);
for (idx = 0; idx < nr_pages && !rc; idx++) {
- gpa = *(pages + idx) + (ga & ~PAGE_MASK);
- _len = min(PAGE_SIZE - (gpa & ~PAGE_MASK), len);
- if (mode == GACC_STORE)
- rc = kvm_write_guest(vcpu->kvm, gpa, data, _len);
- else
- rc = kvm_read_guest(vcpu->kvm, gpa, data, _len);
- len -= _len;
- ga += _len;
- data += _len;
+ gpa = pages[idx] + offset_in_page(ga);
+ seg = min(PAGE_SIZE - offset_in_page(gpa), len);
+ rc = access_guest_frame(vcpu->kvm, mode, gpa, data, seg);
+ len -= seg;
+ ga += seg;
+ data += seg;
}
if (need_ipte_lock)
ipte_unlock(vcpu);
@@ -875,19 +887,17 @@ int access_guest(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, void *data,
int access_guest_real(struct kvm_vcpu *vcpu, unsigned long gra,
void *data, unsigned long len, enum gacc_mode mode)
{
- unsigned long _len, gpa;
+ unsigned long gpa;
+ unsigned int seg;
int rc = 0;
while (len && !rc) {
gpa = kvm_s390_real_to_abs(vcpu, gra);
- _len = min(PAGE_SIZE - (gpa & ~PAGE_MASK), len);
- if (mode)
- rc = write_guest_abs(vcpu, gpa, data, _len);
- else
- rc = read_guest_abs(vcpu, gpa, data, _len);
- len -= _len;
- gra += _len;
- data += _len;
+ seg = min(PAGE_SIZE - offset_in_page(gpa), len);
+ rc = access_guest_frame(vcpu->kvm, mode, gpa, data, seg);
+ len -= seg;
+ gra += seg;
+ data += seg;
}
return rc;
}
--
2.25.1
On 8/16/21 5:07 PM, Janis Schoetterl-Glausch wrote:
> Introduce a helper function for guest frame access.
> Rewrite the calculation of the gpa and the length of the segment
> to be more readable.
>
> Signed-off-by: Janis Schoetterl-Glausch <[email protected]>
Reviewed-by: Janosch Frank <[email protected]>
> ---
> arch/s390/kvm/gaccess.c | 48 +++++++++++++++++++++++++----------------
> 1 file changed, 29 insertions(+), 19 deletions(-)
>
> diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
> index b9f85b2dc053..df83de0843de 100644
> --- a/arch/s390/kvm/gaccess.c
> +++ b/arch/s390/kvm/gaccess.c
> @@ -827,11 +827,26 @@ static int guest_page_range(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar,
> return 0;
> }
>
> +static int access_guest_frame(struct kvm *kvm, enum gacc_mode mode, gpa_t gpa,
> + void *data, unsigned int len)
> +{
> + gfn_t gfn = gpa_to_gfn(gpa);
> + unsigned int offset = offset_in_page(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)
> {
> psw_t *psw = &vcpu->arch.sie_block->gpsw;
> - unsigned long _len, nr_pages, gpa, idx;
> + unsigned long nr_pages, gpa, idx;
> + unsigned int seg;
> unsigned long pages_array[2];
> unsigned long *pages;
> int need_ipte_lock;
> @@ -855,15 +870,12 @@ int access_guest(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, void *data,
> ipte_lock(vcpu);
> rc = guest_page_range(vcpu, ga, ar, pages, nr_pages, asce, mode);
> for (idx = 0; idx < nr_pages && !rc; idx++) {
> - gpa = *(pages + idx) + (ga & ~PAGE_MASK);
> - _len = min(PAGE_SIZE - (gpa & ~PAGE_MASK), len);
> - if (mode == GACC_STORE)
> - rc = kvm_write_guest(vcpu->kvm, gpa, data, _len);
> - else
> - rc = kvm_read_guest(vcpu->kvm, gpa, data, _len);
> - len -= _len;
> - ga += _len;
> - data += _len;
> + gpa = pages[idx] + offset_in_page(ga);
> + seg = min(PAGE_SIZE - offset_in_page(gpa), len);
> + rc = access_guest_frame(vcpu->kvm, mode, gpa, data, seg);
> + len -= seg;
> + ga += seg;
> + data += seg;
> }
> if (need_ipte_lock)
> ipte_unlock(vcpu);
> @@ -875,19 +887,17 @@ int access_guest(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, void *data,
> int access_guest_real(struct kvm_vcpu *vcpu, unsigned long gra,
> void *data, unsigned long len, enum gacc_mode mode)
> {
> - unsigned long _len, gpa;
> + unsigned long gpa;
> + unsigned int seg;
> int rc = 0;
>
> while (len && !rc) {
> gpa = kvm_s390_real_to_abs(vcpu, gra);
> - _len = min(PAGE_SIZE - (gpa & ~PAGE_MASK), len);
> - if (mode)
> - rc = write_guest_abs(vcpu, gpa, data, _len);
> - else
> - rc = read_guest_abs(vcpu, gpa, data, _len);
> - len -= _len;
> - gra += _len;
> - data += _len;
> + seg = min(PAGE_SIZE - offset_in_page(gpa), len);
> + rc = access_guest_frame(vcpu->kvm, mode, gpa, data, seg);
> + len -= seg;
> + gra += seg;
> + data += seg;
> }
> return rc;
> }
>
On 16.08.21 17:07, Janis Schoetterl-Glausch wrote:
> Introduce a helper function for guest frame access.
> Rewrite the calculation of the gpa and the length of the segment
> to be more readable.
>
> Signed-off-by: Janis Schoetterl-Glausch <[email protected]>
> ---
> arch/s390/kvm/gaccess.c | 48 +++++++++++++++++++++++++----------------
> 1 file changed, 29 insertions(+), 19 deletions(-)
>
> diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
> index b9f85b2dc053..df83de0843de 100644
> --- a/arch/s390/kvm/gaccess.c
> +++ b/arch/s390/kvm/gaccess.c
> @@ -827,11 +827,26 @@ static int guest_page_range(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar,
> return 0;
> }
>
> +static int access_guest_frame(struct kvm *kvm, enum gacc_mode mode, gpa_t gpa,
> + void *data, unsigned int len)
I know, "frame" is a beautiful term for "page" -- can we just avoid
using it because we're not using it anywhere else here? :)
What's wrong with "access_guest_page()" ?
> +{
> + gfn_t gfn = gpa_to_gfn(gpa);
> + unsigned int offset = offset_in_page(gpa);
> + int rc;
You could turn both const. You might want to consider
reverse-christmas-treeing this.
> +
> + if (mode == GACC_STORE)
> + rc = kvm_write_guest_page(kvm, gfn, data, offset, len);
> + else
> + rc = kvm_read_guest_page(kvm, gfn, data, offset, len);
Personally, I prefer passing in pfn + offset instead of a gpa. Also
avoids having to convert back and forth.
> + return rc;
> +}
> +
> int access_guest(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, void *data,
> unsigned long len, enum gacc_mode mode)
> {
> psw_t *psw = &vcpu->arch.sie_block->gpsw;
> - unsigned long _len, nr_pages, gpa, idx;
> + unsigned long nr_pages, gpa, idx;
> + unsigned int seg;
Dito, reverse christmas tree might be worth keeping.
> unsigned long pages_array[2];
> unsigned long *pages;
> int need_ipte_lock;
> @@ -855,15 +870,12 @@ int access_guest(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, void *data,
> ipte_lock(vcpu);
> rc = guest_page_range(vcpu, ga, ar, pages, nr_pages, asce, mode);
> for (idx = 0; idx < nr_pages && !rc; idx++) {
> - gpa = *(pages + idx) + (ga & ~PAGE_MASK);
> - _len = min(PAGE_SIZE - (gpa & ~PAGE_MASK), len);
> - if (mode == GACC_STORE)
> - rc = kvm_write_guest(vcpu->kvm, gpa, data, _len);
> - else
> - rc = kvm_read_guest(vcpu->kvm, gpa, data, _len);
> - len -= _len;
> - ga += _len;
> - data += _len;
> + gpa = pages[idx] + offset_in_page(ga);
> + seg = min(PAGE_SIZE - offset_in_page(gpa), len);
> + rc = access_guest_frame(vcpu->kvm, mode, gpa, data, seg);
> + len -= seg;
> + ga += seg;
> + data += seg;
> }
> if (need_ipte_lock)
> ipte_unlock(vcpu);
> @@ -875,19 +887,17 @@ int access_guest(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, void *data,
> int access_guest_real(struct kvm_vcpu *vcpu, unsigned long gra,
> void *data, unsigned long len, enum gacc_mode mode)
> {
> - unsigned long _len, gpa;
> + unsigned long gpa;
> + unsigned int seg;
> int rc = 0;
>
> while (len && !rc) {
> gpa = kvm_s390_real_to_abs(vcpu, gra);
> - _len = min(PAGE_SIZE - (gpa & ~PAGE_MASK), len);
> - if (mode)
> - rc = write_guest_abs(vcpu, gpa, data, _len);
> - else
> - rc = read_guest_abs(vcpu, gpa, data, _len);
> - len -= _len;
> - gra += _len;
> - data += _len;
> + seg = min(PAGE_SIZE - offset_in_page(gpa), len);
What does "seg" mean? I certainly know when "len" means -- which is also
what the function eats.
> + rc = access_guest_frame(vcpu->kvm, mode, gpa, data, seg);
> + len -= seg;
> + gra += seg;
> + data += seg;
> }
> return rc;
> }
>
--
Thanks,
David / dhildenb
On 8/18/21 9:54 AM, David Hildenbrand wrote:
> On 16.08.21 17:07, Janis Schoetterl-Glausch wrote:
>> Introduce a helper function for guest frame access.
>> Rewrite the calculation of the gpa and the length of the segment
>> to be more readable.
>>
>> Signed-off-by: Janis Schoetterl-Glausch <[email protected]>
[...]
>> - unsigned long _len, gpa;
>> + unsigned long gpa;
>> + unsigned int seg;
>> int rc = 0;
>>
>> while (len && !rc) {
>> gpa = kvm_s390_real_to_abs(vcpu, gra);
>> - _len = min(PAGE_SIZE - (gpa & ~PAGE_MASK), len);
>> - if (mode)
>> - rc = write_guest_abs(vcpu, gpa, data, _len);
>> - else
>> - rc = read_guest_abs(vcpu, gpa, data, _len);
>> - len -= _len;
>> - gra += _len;
>> - data += _len;
>> + seg = min(PAGE_SIZE - offset_in_page(gpa), len);
>
> What does "seg" mean? I certainly know when "len" means -- which is also
> what the function eats.
What does "_len" mean especially in contrast to "len"?
"seg" is used in the common kvm guest access functions so it's at least
consistent although I share the sentiment that it's not a great name for
the length we access inside the page.
Originally I suggested "len_in_page" if you have a better name I'd
expect we'll both be happy to discuss it :-)
>
>> + rc = access_guest_frame(vcpu->kvm, mode, gpa, data, seg);
>> + len -= seg;
>> + gra += seg;
>> + data += seg;
>> }
>> return rc;
>> }
>>
>
>
On 18.08.21 10:06, Janosch Frank wrote:
> On 8/18/21 9:54 AM, David Hildenbrand wrote:
>> On 16.08.21 17:07, Janis Schoetterl-Glausch wrote:
>>> Introduce a helper function for guest frame access.
>>> Rewrite the calculation of the gpa and the length of the segment
>>> to be more readable.
>>>
>>> Signed-off-by: Janis Schoetterl-Glausch <[email protected]>
> [...]
>>> - unsigned long _len, gpa;
>>> + unsigned long gpa;
>>> + unsigned int seg;
>>> int rc = 0;
>>>
>>> while (len && !rc) {
>>> gpa = kvm_s390_real_to_abs(vcpu, gra);
>>> - _len = min(PAGE_SIZE - (gpa & ~PAGE_MASK), len);
>>> - if (mode)
>>> - rc = write_guest_abs(vcpu, gpa, data, _len);
>>> - else
>>> - rc = read_guest_abs(vcpu, gpa, data, _len);
>>> - len -= _len;
>>> - gra += _len;
>>> - data += _len;
>>> + seg = min(PAGE_SIZE - offset_in_page(gpa), len);
>>
>> What does "seg" mean? I certainly know when "len" means -- which is also
>> what the function eats.
>
> What does "_len" mean especially in contrast to "len"?
>
> "seg" is used in the common kvm guest access functions so it's at least
> consistent although I share the sentiment that it's not a great name for
> the length we access inside the page.
>
> Originally I suggested "len_in_page" if you have a better name I'd
> expect we'll both be happy to discuss it :-)
Similar code I encountered in other places uses "len" vs "cur_len" or
"total_len" vs. "cur_len". I agree that everything is better than "len"
vs. "_len".
I just cannot come up with a proper word for "seg" that would make
sense. "Segment" ? Maybe my uneducated mind is missing some important
English words that just fit perfectly here.
Anyhow, just my 2 cents, you maintain this code :)
--
Thanks,
David / dhildenb
On 8/18/21 10:44 AM, David Hildenbrand wrote:
> On 18.08.21 10:06, Janosch Frank wrote:
>> On 8/18/21 9:54 AM, David Hildenbrand wrote:
>>> On 16.08.21 17:07, Janis Schoetterl-Glausch wrote:
>>>> Introduce a helper function for guest frame access.
>>>> Rewrite the calculation of the gpa and the length of the segment
>>>> to be more readable.
>>>>
>>>> Signed-off-by: Janis Schoetterl-Glausch <[email protected]>
>> [...]
>>>> - unsigned long _len, gpa;
>>>> + unsigned long gpa;
>>>> + unsigned int seg;
>>>> int rc = 0;
>>>> while (len && !rc) {
>>>> gpa = kvm_s390_real_to_abs(vcpu, gra);
>>>> - _len = min(PAGE_SIZE - (gpa & ~PAGE_MASK), len);
>>>> - if (mode)
>>>> - rc = write_guest_abs(vcpu, gpa, data, _len);
>>>> - else
>>>> - rc = read_guest_abs(vcpu, gpa, data, _len);
>>>> - len -= _len;
>>>> - gra += _len;
>>>> - data += _len;
>>>> + seg = min(PAGE_SIZE - offset_in_page(gpa), len);
>>>
>>> What does "seg" mean? I certainly know when "len" means -- which is also
>>> what the function eats.
>>
>> What does "_len" mean especially in contrast to "len"?
>>
>> "seg" is used in the common kvm guest access functions so it's at least
>> consistent although I share the sentiment that it's not a great name for
>> the length we access inside the page.
>>
>> Originally I suggested "len_in_page" if you have a better name I'd
>> expect we'll both be happy to discuss it :-)
>
> Similar code I encountered in other places uses "len" vs "cur_len" or "total_len" vs. "cur_len". I agree that everything is better than "len" vs. "_len".
>
> I just cannot come up with a proper word for "seg" that would make sense. "Segment" ? Maybe my uneducated mind is missing some important English words that just fit perfectly here.
Yes, it's short for segment, kvm_main has a function next_segment to calculate it.
I don't like the naming scheme much either.
>
> Anyhow, just my 2 cents, you maintain this code :)
>
On 8/18/21 9:54 AM, David Hildenbrand wrote:
> On 16.08.21 17:07, Janis Schoetterl-Glausch wrote:
>> Introduce a helper function for guest frame access.
>> Rewrite the calculation of the gpa and the length of the segment
>> to be more readable.
>>
>> Signed-off-by: Janis Schoetterl-Glausch <[email protected]>
>> ---
>> arch/s390/kvm/gaccess.c | 48 +++++++++++++++++++++++++----------------
>> 1 file changed, 29 insertions(+), 19 deletions(-)
>>
>> diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
>> index b9f85b2dc053..df83de0843de 100644
>> --- a/arch/s390/kvm/gaccess.c
>> +++ b/arch/s390/kvm/gaccess.c
>> @@ -827,11 +827,26 @@ static int guest_page_range(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar,
>> return 0;
>> }
>> +static int access_guest_frame(struct kvm *kvm, enum gacc_mode mode, gpa_t gpa,
>> + void *data, unsigned int len)
>
> I know, "frame" is a beautiful term for "page" -- can we just avoid using it because we're not using it anywhere else here? :)
>
> What's wrong with "access_guest_page()" ?
Ok, I'll use page for consistency's sake.
>
>
>> +{
>> + gfn_t gfn = gpa_to_gfn(gpa);
>> + unsigned int offset = offset_in_page(gpa);
>> + int rc;
>
> You could turn both const. You might want to consider reverse-christmas-treeing this.
Ok
>
>> +
>> + if (mode == GACC_STORE)
>> + rc = kvm_write_guest_page(kvm, gfn, data, offset, len);
>> + else
>> + rc = kvm_read_guest_page(kvm, gfn, data, offset, len);
>
> Personally, I prefer passing in pfn + offset instead of a gpa. Also avoids having to convert back and forth.
In access_guest_real we get back the gpa directly from the translation function.
After the next patch the same is true for access_guest.
So using gpas everywhere is nicer.
And if we were to introduce a len_in_page function the offset would not even show up as an intermediary.
>
>> + return rc;
>> +}
>> +
>> int access_guest(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, void *data,
>> unsigned long len, enum gacc_mode mode)
>> {
>> psw_t *psw = &vcpu->arch.sie_block->gpsw;
>> - unsigned long _len, nr_pages, gpa, idx;
>> + unsigned long nr_pages, gpa, idx;
>> + unsigned int seg;
>
> Dito, reverse christmas tree might be worth keeping.
>
>> unsigned long pages_array[2];
>> unsigned long *pages;
>> int need_ipte_lock;
>> @@ -855,15 +870,12 @@ int access_guest(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, void *data,
>> ipte_lock(vcpu);
>> rc = guest_page_range(vcpu, ga, ar, pages, nr_pages, asce, mode);
>> for (idx = 0; idx < nr_pages && !rc; idx++) {
>> - gpa = *(pages + idx) + (ga & ~PAGE_MASK);
>> - _len = min(PAGE_SIZE - (gpa & ~PAGE_MASK), len);
>> - if (mode == GACC_STORE)
>> - rc = kvm_write_guest(vcpu->kvm, gpa, data, _len);
>> - else
>> - rc = kvm_read_guest(vcpu->kvm, gpa, data, _len);
>> - len -= _len;
>> - ga += _len;
>> - data += _len;
>> + gpa = pages[idx] + offset_in_page(ga);
>> + seg = min(PAGE_SIZE - offset_in_page(gpa), len);
>> + rc = access_guest_frame(vcpu->kvm, mode, gpa, data, seg);
>> + len -= seg;
>> + ga += seg;
>> + data += seg;
>> }
>> if (need_ipte_lock)
>> ipte_unlock(vcpu);
>> @@ -875,19 +887,17 @@ int access_guest(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, void *data,
>> int access_guest_real(struct kvm_vcpu *vcpu, unsigned long gra,
>> void *data, unsigned long len, enum gacc_mode mode)
>> {
>> - unsigned long _len, gpa;
>> + unsigned long gpa;
>> + unsigned int seg;
>> int rc = 0;
>> while (len && !rc) {
>> gpa = kvm_s390_real_to_abs(vcpu, gra);
>> - _len = min(PAGE_SIZE - (gpa & ~PAGE_MASK), len);
>> - if (mode)
>> - rc = write_guest_abs(vcpu, gpa, data, _len);
>> - else
>> - rc = read_guest_abs(vcpu, gpa, data, _len);
>> - len -= _len;
>> - gra += _len;
>> - data += _len;
>> + seg = min(PAGE_SIZE - offset_in_page(gpa), len);
>
> What does "seg" mean? I certainly know when "len" means -- which is also what the function eats.
>
>> + rc = access_guest_frame(vcpu->kvm, mode, gpa, data, seg);
>> + len -= seg;
>> + gra += seg;
>> + data += seg;
>> }
>> return rc;
>> }
>>
>
>
On 8/18/21 10:06 AM, Janosch Frank wrote:
> On 8/18/21 9:54 AM, David Hildenbrand wrote:
>> On 16.08.21 17:07, Janis Schoetterl-Glausch wrote:
>>> Introduce a helper function for guest frame access.
>>> Rewrite the calculation of the gpa and the length of the segment
>>> to be more readable.
>>>
>>> Signed-off-by: Janis Schoetterl-Glausch <[email protected]>
> [...]
>>> - unsigned long _len, gpa;
>>> + unsigned long gpa;
>>> + unsigned int seg;
>>> int rc = 0;
>>>
>>> while (len && !rc) {
>>> gpa = kvm_s390_real_to_abs(vcpu, gra);
>>> - _len = min(PAGE_SIZE - (gpa & ~PAGE_MASK), len);
>>> - if (mode)
>>> - rc = write_guest_abs(vcpu, gpa, data, _len);
>>> - else
>>> - rc = read_guest_abs(vcpu, gpa, data, _len);
>>> - len -= _len;
>>> - gra += _len;
>>> - data += _len;
>>> + seg = min(PAGE_SIZE - offset_in_page(gpa), len);
>>
>> What does "seg" mean? I certainly know when "len" means -- which is also
>> what the function eats.
>
> What does "_len" mean especially in contrast to "len"?
>
> "seg" is used in the common kvm guest access functions so it's at least
> consistent although I share the sentiment that it's not a great name for
> the length we access inside the page.
>
> Originally I suggested "len_in_page" if you have a better name I'd
> expect we'll both be happy to discuss it :-)
fragment_len ?
>
>>
>>> + rc = access_guest_frame(vcpu->kvm, mode, gpa, data, seg);
>>> + len -= seg;
>>> + gra += seg;
>>> + data += seg;
>>> }
>>> return rc;
>>> }
>>>
>>
>>
>
On 8/19/21 3:53 PM, Janis Schoetterl-Glausch wrote:
> On 8/18/21 10:06 AM, Janosch Frank wrote:
>> On 8/18/21 9:54 AM, David Hildenbrand wrote:
>>> On 16.08.21 17:07, Janis Schoetterl-Glausch wrote:
>>>> Introduce a helper function for guest frame access.
>>>> Rewrite the calculation of the gpa and the length of the segment
>>>> to be more readable.
>>>>
>>>> Signed-off-by: Janis Schoetterl-Glausch <[email protected]>
>> [...]
>>>> - unsigned long _len, gpa;
>>>> + unsigned long gpa;
>>>> + unsigned int seg;
>>>> int rc = 0;
>>>>
>>>> while (len && !rc) {
>>>> gpa = kvm_s390_real_to_abs(vcpu, gra);
>>>> - _len = min(PAGE_SIZE - (gpa & ~PAGE_MASK), len);
>>>> - if (mode)
>>>> - rc = write_guest_abs(vcpu, gpa, data, _len);
>>>> - else
>>>> - rc = read_guest_abs(vcpu, gpa, data, _len);
>>>> - len -= _len;
>>>> - gra += _len;
>>>> - data += _len;
>>>> + seg = min(PAGE_SIZE - offset_in_page(gpa), len);
>>>
>>> What does "seg" mean? I certainly know when "len" means -- which is also
>>> what the function eats.
>>
>> What does "_len" mean especially in contrast to "len"?
>>
>> "seg" is used in the common kvm guest access functions so it's at least
>> consistent although I share the sentiment that it's not a great name for
>> the length we access inside the page.
>>
>> Originally I suggested "len_in_page" if you have a better name I'd
>> expect we'll both be happy to discuss it :-)
>
> fragment_len ?
Sounds good to me
>>
>>>
>>>> + rc = access_guest_frame(vcpu->kvm, mode, gpa, data, seg);
>>>> + len -= seg;
>>>> + gra += seg;
>>>> + data += seg;
>>>> }
>>>> return rc;
>>>> }
>>>>
>>>
>>>
>>
>