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 <[email protected]>
+ Reviewed-by: Janosch Frank <[email protected]>
## 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 <[email protected]>
+ Reviewed-by: Janosch Frank <[email protected]>
## 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 <[email protected]>
## 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 <[email protected]>
+ Reviewed-by: Janosch Frank <[email protected]>
## 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
Improve readability by renaming the length variable and
not calculating the offset manually.
Signed-off-by: Janis Schoetterl-Glausch <[email protected]>
Reviewed-by: Janosch Frank <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Janosch Frank <[email protected]>
---
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
Introduce a helper function for guest frame access.
Signed-off-by: Janis Schoetterl-Glausch <[email protected]>
Reviewed-by: Janosch Frank <[email protected]>
---
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
On 26.11.21 17:45, Janis Schoetterl-Glausch wrote:
> Introduce a helper function for guest frame access.
>
> Signed-off-by: Janis Schoetterl-Glausch <[email protected]>
> Reviewed-by: Janosch Frank <[email protected]>
> ---
> 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 <[email protected]>
--
Thanks,
David / dhildenb
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 <[email protected]>
> Reviewed-by: Janosch Frank <[email protected]>
> ---
> arch/s390/kvm/gaccess.c | 32 +++++++++++++++++---------------
> 1 file changed, 17 insertions(+), 15 deletions(-)
Reviewed-by: David Hildenbrand <[email protected]>
--
Thanks,
David / dhildenb
On Fri, 26 Nov 2021 17:45:47 +0100
Janis Schoetterl-Glausch <[email protected]> wrote:
> Improve readability by renaming the length variable and
> not calculating the offset manually.
>
> Signed-off-by: Janis Schoetterl-Glausch <[email protected]>
> Reviewed-by: Janosch Frank <[email protected]>
Reviewed-by: Claudio Imbrenda <[email protected]>
> ---
> 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;
> }
On Fri, 26 Nov 2021 17:45:48 +0100
Janis Schoetterl-Glausch <[email protected]> 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 <[email protected]>
> Reviewed-by: Janosch Frank <[email protected]>
Reviewed-by: Claudio Imbrenda <[email protected]>
> ---
> 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;
On Fri, 26 Nov 2021 17:45:49 +0100
Janis Schoetterl-Glausch <[email protected]> wrote:
> Introduce a helper function for guest frame access.
>
> Signed-off-by: Janis Schoetterl-Glausch <[email protected]>
> Reviewed-by: Janosch Frank <[email protected]>
Reviewed-by: Claudio Imbrenda <[email protected]>
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;
On 11/29/21 09:59, Claudio Imbrenda wrote:
> On Fri, 26 Nov 2021 17:45:49 +0100
> Janis Schoetterl-Glausch <[email protected]> wrote:
>
>> Introduce a helper function for guest frame access.
>>
>> Signed-off-by: Janis Schoetterl-Glausch <[email protected]>
>> Reviewed-by: Janosch Frank <[email protected]>
>
> Reviewed-by: Claudio Imbrenda <[email protected]>
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;
>
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 <[email protected]>
> + Reviewed-by: Janosch Frank <[email protected]>
>
> ## 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 <[email protected]>
> + Reviewed-by: Janosch Frank <[email protected]>
>
> ## 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 <[email protected]>
>
> ## 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 <[email protected]>
> + Reviewed-by: Janosch Frank <[email protected]>
>
> ## 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
>