Re: [PATCH v3 1/1] KVM: s390: vsie: clarifications on setting the APCB

Janosch Frank <frankja@xxxxxxxxxxxxx> · Thu, 16 Feb 2023 14:41:22 +0100

On 2/14/23 13:28, Pierre Morel wrote:
The APCB is part of the CRYCB.
The calculation of the APCB origin can be done by adding
the APCB offset to the CRYCB origin.

Current code makes confusing transformations, converting
the CRYCB origin to a pointer to calculate the APCB origin.

Let's make things simpler and keep the CRYCB origin to make
these calculations.

Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxx>
Reviewed-by: Claudio Imbrenda <imbrenda@xxxxxxxxxxxxx>

LGTM:
Acked-by: Janosch Frank <frankja@xxxxxxxxxxxxx>

---
  arch/s390/kvm/vsie.c | 50 +++++++++++++++++++++++++-------------------
  1 file changed, 29 insertions(+), 21 deletions(-)

diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
index b6a0219e470a..8d6b765abf29 100644
--- a/arch/s390/kvm/vsie.c
+++ b/arch/s390/kvm/vsie.c
@@ -138,11 +138,15 @@ static int prepare_cpuflags(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
  }
  /* Copy to APCB FORMAT1 from APCB FORMAT0 */
  static int setup_apcb10(struct kvm_vcpu *vcpu, struct kvm_s390_apcb1 *apcb_s,
-			unsigned long apcb_o, struct kvm_s390_apcb1 *apcb_h)
+			unsigned long crycb_gpa, struct kvm_s390_apcb1 *apcb_h)
  {
  	struct kvm_s390_apcb0 tmp;
+	unsigned long apcb_gpa;
  
-	if (read_guest_real(vcpu, apcb_o, &tmp, sizeof(struct kvm_s390_apcb0)))
+	apcb_gpa = crycb_gpa + offsetof(struct kvm_s390_crypto_cb, apcb0);
+
+	if (read_guest_real(vcpu, apcb_gpa, &tmp,
+			    sizeof(struct kvm_s390_apcb0)))
  		return -EFAULT;
  
  	apcb_s->apm[0] = apcb_h->apm[0] & tmp.apm[0];
@@ -157,15 +161,19 @@ static int setup_apcb10(struct kvm_vcpu *vcpu, struct kvm_s390_apcb1 *apcb_s,
   * setup_apcb00 - Copy to APCB FORMAT0 from APCB FORMAT0
   * @vcpu: pointer to the virtual CPU
   * @apcb_s: pointer to start of apcb in the shadow crycb
- * @apcb_o: pointer to start of original apcb in the guest2
+ * @crycb_gpa: guest physical address to start of original guest crycb
   * @apcb_h: pointer to start of apcb in the guest1
   *
   * Returns 0 and -EFAULT on error reading guest apcb
   */
  static int setup_apcb00(struct kvm_vcpu *vcpu, unsigned long *apcb_s,
-			unsigned long apcb_o, unsigned long *apcb_h)
+			unsigned long crycb_gpa, unsigned long *apcb_h)
  {
-	if (read_guest_real(vcpu, apcb_o, apcb_s,
+	unsigned long apcb_gpa;
+
+	apcb_gpa = crycb_gpa + offsetof(struct kvm_s390_crypto_cb, apcb0);
+
+	if (read_guest_real(vcpu, apcb_gpa, apcb_s,
  			    sizeof(struct kvm_s390_apcb0)))
  		return -EFAULT;
  
@@ -178,16 +186,20 @@ static int setup_apcb00(struct kvm_vcpu *vcpu, unsigned long *apcb_s,
   * setup_apcb11 - Copy the FORMAT1 APCB from the guest to the shadow CRYCB
   * @vcpu: pointer to the virtual CPU
   * @apcb_s: pointer to start of apcb in the shadow crycb
- * @apcb_o: pointer to start of original guest apcb
+ * @crycb_gpa: guest physical address to start of original guest crycb
   * @apcb_h: pointer to start of apcb in the host
   *
   * Returns 0 and -EFAULT on error reading guest apcb
   */
  static int setup_apcb11(struct kvm_vcpu *vcpu, unsigned long *apcb_s,
-			unsigned long apcb_o,
+			unsigned long crycb_gpa,
  			unsigned long *apcb_h)
  {
-	if (read_guest_real(vcpu, apcb_o, apcb_s,
+	unsigned long apcb_gpa;
+
+	apcb_gpa = crycb_gpa + offsetof(struct kvm_s390_crypto_cb, apcb1);
+
+	if (read_guest_real(vcpu, apcb_gpa, apcb_s,
  			    sizeof(struct kvm_s390_apcb1)))
  		return -EFAULT;
  
@@ -200,7 +212,7 @@ static int setup_apcb11(struct kvm_vcpu *vcpu, unsigned long *apcb_s,
   * setup_apcb - Create a shadow copy of the apcb.
   * @vcpu: pointer to the virtual CPU
   * @crycb_s: pointer to shadow crycb
- * @crycb_o: pointer to original guest crycb
+ * @crycb_gpa: guest physical address of original guest crycb
   * @crycb_h: pointer to the host crycb
   * @fmt_o: format of the original guest crycb.
   * @fmt_h: format of the host crycb.
@@ -211,50 +223,46 @@ static int setup_apcb11(struct kvm_vcpu *vcpu, unsigned long *apcb_s,
   * Return 0 or an error number if the guest and host crycb are incompatible.
   */
  static int setup_apcb(struct kvm_vcpu *vcpu, struct kvm_s390_crypto_cb *crycb_s,
-	       const u32 crycb_o,
+	       const u32 crycb_gpa,
  	       struct kvm_s390_crypto_cb *crycb_h,
  	       int fmt_o, int fmt_h)
  {
-	struct kvm_s390_crypto_cb *crycb;
-
-	crycb = (struct kvm_s390_crypto_cb *) (unsigned long)crycb_o;
-
  	switch (fmt_o) {
  	case CRYCB_FORMAT2:
-		if ((crycb_o & PAGE_MASK) != ((crycb_o + 256) & PAGE_MASK))
+		if ((crycb_gpa & PAGE_MASK) != ((crycb_gpa + 256) & PAGE_MASK))
  			return -EACCES;
  		if (fmt_h != CRYCB_FORMAT2)
  			return -EINVAL;
  		return setup_apcb11(vcpu, (unsigned long *)&crycb_s->apcb1,
-				    (unsigned long) &crycb->apcb1,
+				    crycb_gpa,
  				    (unsigned long *)&crycb_h->apcb1);
  	case CRYCB_FORMAT1:
  		switch (fmt_h) {
  		case CRYCB_FORMAT2:
  			return setup_apcb10(vcpu, &crycb_s->apcb1,
-					    (unsigned long) &crycb->apcb0,
+					    crycb_gpa,
  					    &crycb_h->apcb1);
  		case CRYCB_FORMAT1:
  			return setup_apcb00(vcpu,
  					    (unsigned long *) &crycb_s->apcb0,
-					    (unsigned long) &crycb->apcb0,
+					    crycb_gpa,
  					    (unsigned long *) &crycb_h->apcb0);
  		}
  		break;
  	case CRYCB_FORMAT0:
-		if ((crycb_o & PAGE_MASK) != ((crycb_o + 32) & PAGE_MASK))
+		if ((crycb_gpa & PAGE_MASK) != ((crycb_gpa + 32) & PAGE_MASK))
  			return -EACCES;
  
  		switch (fmt_h) {
  		case CRYCB_FORMAT2:
  			return setup_apcb10(vcpu, &crycb_s->apcb1,
-					    (unsigned long) &crycb->apcb0,
+					    crycb_gpa,
  					    &crycb_h->apcb1);
  		case CRYCB_FORMAT1:
  		case CRYCB_FORMAT0:
  			return setup_apcb00(vcpu,
  					    (unsigned long *) &crycb_s->apcb0,
-					    (unsigned long) &crycb->apcb0,
+					    crycb_gpa,
  					    (unsigned long *) &crycb_h->apcb0);
  		}
  	}







