diff mbox series

KVM: s390: vsie: fix crycb virtual vs physical usage

Message ID 20220905084148.234821-1-pmorel@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series KVM: s390: vsie: fix crycb virtual vs physical usage | expand

Commit Message

Pierre Morel Sept. 5, 2022, 8:41 a.m. UTC 
Prepare VSIE for architectural changes where lowmem kernel real and
kernel virtual address are different.

When we get the original crycb from the guest crycb we can use the
phys_to_virt transformation, which will use the host transformations,
but we must use an offset to calculate the guest real address apcb
and give it to read_guest_real().

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 arch/s390/kvm/vsie.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

Comments

David Hildenbrand Sept. 5, 2022, 4:32 p.m. UTC | #1 
On 05.09.22 10:41, Pierre Morel wrote:
> Prepare VSIE for architectural changes where lowmem kernel real and
> kernel virtual address are different.

Bear with me, it used to be

	crycb = (struct kvm_s390_crypto_cb *) (unsigned long)crycb_o;
	apcb_o = (unsigned long) &crycb->apcb0;

and now it's

	apcb_o = crycb_o + offsetof(struct kvm_s390_crypto_cb, apcb0);


So the real issue seems to be

	crycb = (struct kvm_s390_crypto_cb *) (unsigned long)crycb_o;

because crycb_o actually is a guest address and not a host address.


But now I'm confused, because I would have thought that the result 
produced by both code would be identical (I completely agree that the 
new variant is better).

How does this interact with "lowmem kernel real and kernel virtual 
address are different." -- I would have thought that &crycb->apcb0 
doesn't actually access any memory and only performs arithmetical 
operations?

> 
> When we get the original crycb from the guest crycb we can use the
> phys_to_virt transformation, which will use the host transformations,
> but we must use an offset to calculate the guest real address apcb
> and give it to read_guest_real().

Can you elaborate where phys_to_virt() comes into play?

If this is an actual fix (as indicated in the patch subject), should 
this carry a

	Fixes: 56019f9aca22 ("KVM: s390: vsie: Allow CRYCB FORMAT-2")
Pierre Morel Sept. 12, 2022, 8:03 a.m. UTC | #2 
Hi David,

sorry for the delay, just came back from vacation.

On 9/5/22 18:32, David Hildenbrand wrote:
> On 05.09.22 10:41, Pierre Morel wrote:
>> Prepare VSIE for architectural changes where lowmem kernel real and
>> kernel virtual address are different.
> 
> Bear with me, it used to be
> 
>      crycb = (struct kvm_s390_crypto_cb *) (unsigned long)crycb_o;
>      apcb_o = (unsigned long) &crycb->apcb0;
> 
> and now it's
> 
>      apcb_o = crycb_o + offsetof(struct kvm_s390_crypto_cb, apcb0);
> 
> 
> So the real issue seems to be
> 
>      crycb = (struct kvm_s390_crypto_cb *) (unsigned long)crycb_o;
> 
> because crycb_o actually is a guest address and not a host address.

Yes, right this is the real issue, however...

> 
> 
> But now I'm confused, because I would have thought that the result 
> produced by both code would be identical (I completely agree that the 
> new variant is better).
> 
> How does this interact with "lowmem kernel real and kernel virtual 
> address are different." -- I would have thought that &crycb->apcb0 
> doesn't actually access any memory and only performs arithmetical 
> operations?

...you are right and the result is identical.


> 
>>
>> When we get the original crycb from the guest crycb we can use the
>> phys_to_virt transformation, which will use the host transformations,
>> but we must use an offset to calculate the guest real address apcb
>> and give it to read_guest_real().
> 
> Can you elaborate where phys_to_virt() comes into play?

No, it does not have to do with phys_to_virt(), I first started to work 
on the line:
- crycb = (struct kvm_s390_crypto_cb *) (unsigned long)crycb_o;

and did not notice that after the simplifications the result was identical.
So The comment is wrong and this patch is only making the code clearer.

Thanks for your clarifications,

Regards,
Pierre


> 
> If this is an actual fix (as indicated in the patch subject), should 
> this carry a
> 
>      Fixes: 56019f9aca22 ("KVM: s390: vsie: Allow CRYCB FORMAT-2")
>
diff mbox series

Patch

diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
index 94138f8f0c1c..f37851c9b1ab 100644
--- a/arch/s390/kvm/vsie.c
+++ b/arch/s390/kvm/vsie.c
@@ -138,9 +138,12 @@  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_o, struct kvm_s390_apcb1 *apcb_h)
 {
 	struct kvm_s390_apcb0 tmp;
+	unsigned long apcb_o;
+
+	apcb_o = crycb_o + offsetof(struct kvm_s390_crypto_cb, apcb0);
 
 	if (read_guest_real(vcpu, apcb_o, &tmp, sizeof(struct kvm_s390_apcb0)))
 		return -EFAULT;
@@ -157,14 +160,18 @@  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_o: real guest 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_o, unsigned long *apcb_h)
 {
+	unsigned long apcb_o;
+
+	apcb_o = crycb_o + offsetof(struct kvm_s390_crypto_cb, apcb0);
+
 	if (read_guest_real(vcpu, apcb_o, apcb_s,
 			    sizeof(struct kvm_s390_apcb0)))
 		return -EFAULT;
@@ -178,15 +185,19 @@  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_o: real guest 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_o,
 			unsigned long *apcb_h)
 {
+	unsigned long apcb_o;
+
+	apcb_o = crycb_o + offsetof(struct kvm_s390_crypto_cb, apcb1);
+
 	if (read_guest_real(vcpu, apcb_o, apcb_s,
 			    sizeof(struct kvm_s390_apcb1)))
 		return -EFAULT;
@@ -200,7 +211,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_o: real 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.
@@ -215,10 +226,6 @@  static int setup_apcb(struct kvm_vcpu *vcpu, struct kvm_s390_crypto_cb *crycb_s,
 	       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))
@@ -226,18 +233,18 @@  static int setup_apcb(struct kvm_vcpu *vcpu, struct kvm_s390_crypto_cb *crycb_s,
 		if (fmt_h != CRYCB_FORMAT2)
 			return -EINVAL;
 		return setup_apcb11(vcpu, (unsigned long *)&crycb_s->apcb1,
-				    (unsigned long) &crycb->apcb1,
+				    crycb_o,
 				    (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_o,
 					    &crycb_h->apcb1);
 		case CRYCB_FORMAT1:
 			return setup_apcb00(vcpu,
 					    (unsigned long *) &crycb_s->apcb0,
-					    (unsigned long) &crycb->apcb0,
+					    crycb_o,
 					    (unsigned long *) &crycb_h->apcb0);
 		}
 		break;
@@ -248,13 +255,13 @@  static int setup_apcb(struct kvm_vcpu *vcpu, struct kvm_s390_crypto_cb *crycb_s,
 		switch (fmt_h) {
 		case CRYCB_FORMAT2:
 			return setup_apcb10(vcpu, &crycb_s->apcb1,
-					    (unsigned long) &crycb->apcb0,
+					    crycb_o,
 					    &crycb_h->apcb1);
 		case CRYCB_FORMAT1:
 		case CRYCB_FORMAT0:
 			return setup_apcb00(vcpu,
 					    (unsigned long *) &crycb_s->apcb0,
-					    (unsigned long) &crycb->apcb0,
+					    crycb_o,
 					    (unsigned long *) &crycb_h->apcb0);
 		}
 	}