All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] KVM: arm64: Only sign-extend MMIO up to register width
@ 2019-12-12 19:50 ` Christoffer Dall
  0 siblings, 0 replies; 9+ messages in thread
From: Christoffer Dall @ 2019-12-12 19:50 UTC (permalink / raw)
  To: kvmarm; +Cc: Marc Zyngier, Djordje.Kovacevic, linux-arm-kernel

On AArch64 you can do a sign-extended load to either a 32-bit or 64-bit
register, and we should only sign extend the register up to the width of
the register as specified in the operation (by using the 32-bit Wn or
64-bit Xn register specifier).

As it turns out, the architecture provides this decoding information in
the SF ("Sixty-Four" -- how cute...) bit.

Let's take advantage of this with the usual 32-bit/64-bit header file
dance and do the right thing on AArch64 hosts.

Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
---
 arch/arm/include/asm/kvm_emulate.h   | 5 +++++
 arch/arm/include/asm/kvm_mmio.h      | 2 ++
 arch/arm64/include/asm/kvm_emulate.h | 5 +++++
 arch/arm64/include/asm/kvm_mmio.h    | 6 ++----
 virt/kvm/arm/mmio.c                  | 8 +++++++-
 5 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
index 9b118516d2db..fe55d8737a11 100644
--- a/arch/arm/include/asm/kvm_emulate.h
+++ b/arch/arm/include/asm/kvm_emulate.h
@@ -182,6 +182,11 @@ static inline bool kvm_vcpu_dabt_issext(struct kvm_vcpu *vcpu)
 	return kvm_vcpu_get_hsr(vcpu) & HSR_SSE;
 }
 
+static inline bool kvm_vcpu_dabt_issf(const struct kvm_vcpu *vcpu)
+{
+	return false;
+}
+
 static inline int kvm_vcpu_dabt_get_rd(struct kvm_vcpu *vcpu)
 {
 	return (kvm_vcpu_get_hsr(vcpu) & HSR_SRT_MASK) >> HSR_SRT_SHIFT;
diff --git a/arch/arm/include/asm/kvm_mmio.h b/arch/arm/include/asm/kvm_mmio.h
index 7c0eddb0adb2..32fbf82e3ebc 100644
--- a/arch/arm/include/asm/kvm_mmio.h
+++ b/arch/arm/include/asm/kvm_mmio.h
@@ -14,6 +14,8 @@
 struct kvm_decode {
 	unsigned long rt;
 	bool sign_extend;
+	/* Not used on 32-bit arm */
+	bool sixty_four;
 };
 
 void kvm_mmio_write_buf(void *buf, unsigned int len, unsigned long data);
diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index 5efe5ca8fecf..f407b6bdad2e 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -283,6 +283,11 @@ static inline bool kvm_vcpu_dabt_issext(const struct kvm_vcpu *vcpu)
 	return !!(kvm_vcpu_get_hsr(vcpu) & ESR_ELx_SSE);
 }
 
+static inline bool kvm_vcpu_dabt_issf(const struct kvm_vcpu *vcpu)
+{
+	return !!(kvm_vcpu_get_hsr(vcpu) & ESR_ELx_SF);
+}
+
 static inline int kvm_vcpu_dabt_get_rd(const struct kvm_vcpu *vcpu)
 {
 	return (kvm_vcpu_get_hsr(vcpu) & ESR_ELx_SRT_MASK) >> ESR_ELx_SRT_SHIFT;
diff --git a/arch/arm64/include/asm/kvm_mmio.h b/arch/arm64/include/asm/kvm_mmio.h
index 02b5c48fd467..b204501a0c39 100644
--- a/arch/arm64/include/asm/kvm_mmio.h
+++ b/arch/arm64/include/asm/kvm_mmio.h
@@ -10,13 +10,11 @@
 #include <linux/kvm_host.h>
 #include <asm/kvm_arm.h>
 
-/*
- * This is annoying. The mmio code requires this, even if we don't
- * need any decoding. To be fixed.
- */
 struct kvm_decode {
 	unsigned long rt;
 	bool sign_extend;
+	/* Witdth of the register accessed by the faulting instruction is 64-bits */
+	bool sixty_four;
 };
 
 void kvm_mmio_write_buf(void *buf, unsigned int len, unsigned long data);
diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c
index 70d3b449692c..e62454b2e529 100644
--- a/virt/kvm/arm/mmio.c
+++ b/virt/kvm/arm/mmio.c
@@ -83,7 +83,7 @@ unsigned long kvm_mmio_read_buf(const void *buf, unsigned int len)
 int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
 {
 	unsigned long data;
-	unsigned int len;
+	unsigned int len, regsize;
 	int mask;
 
 	/* Detect an already handled MMIO return */
@@ -105,6 +105,9 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
 			data = (data ^ mask) - mask;
 		}
 
+		if (!vcpu->arch.mmio_decode.sixty_four)
+			data = data & 0xffffffff;
+
 		trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr,
 			       &data);
 		data = vcpu_data_host_to_guest(vcpu, data, len);
@@ -125,6 +128,7 @@ static int decode_hsr(struct kvm_vcpu *vcpu, bool *is_write, int *len)
 	unsigned long rt;
 	int access_size;
 	bool sign_extend;
+	bool sixty_four;
 
 	if (kvm_vcpu_dabt_iss1tw(vcpu)) {
 		/* page table accesses IO mem: tell guest to fix its TTBR */
@@ -138,11 +142,13 @@ static int decode_hsr(struct kvm_vcpu *vcpu, bool *is_write, int *len)
 
 	*is_write = kvm_vcpu_dabt_iswrite(vcpu);
 	sign_extend = kvm_vcpu_dabt_issext(vcpu);
+	sixty_four = kvm_vcpu_dabt_issf(vcpu);
 	rt = kvm_vcpu_dabt_get_rd(vcpu);
 
 	*len = access_size;
 	vcpu->arch.mmio_decode.sign_extend = sign_extend;
 	vcpu->arch.mmio_decode.rt = rt;
+	vcpu->arch.mmio_decode.sixty_four = sixty_four;
 
 	return 0;
 }
-- 
2.18.0

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH] KVM: arm64: Only sign-extend MMIO up to register width
@ 2019-12-12 19:50 ` Christoffer Dall
  0 siblings, 0 replies; 9+ messages in thread
From: Christoffer Dall @ 2019-12-12 19:50 UTC (permalink / raw)
  To: kvmarm
  Cc: Suzuki K Poulose, Marc Zyngier, Christoffer Dall,
	Djordje.Kovacevic, James Morse, linux-arm-kernel, Julien Thierry

On AArch64 you can do a sign-extended load to either a 32-bit or 64-bit
register, and we should only sign extend the register up to the width of
the register as specified in the operation (by using the 32-bit Wn or
64-bit Xn register specifier).

As it turns out, the architecture provides this decoding information in
the SF ("Sixty-Four" -- how cute...) bit.

Let's take advantage of this with the usual 32-bit/64-bit header file
dance and do the right thing on AArch64 hosts.

Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
---
 arch/arm/include/asm/kvm_emulate.h   | 5 +++++
 arch/arm/include/asm/kvm_mmio.h      | 2 ++
 arch/arm64/include/asm/kvm_emulate.h | 5 +++++
 arch/arm64/include/asm/kvm_mmio.h    | 6 ++----
 virt/kvm/arm/mmio.c                  | 8 +++++++-
 5 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
index 9b118516d2db..fe55d8737a11 100644
--- a/arch/arm/include/asm/kvm_emulate.h
+++ b/arch/arm/include/asm/kvm_emulate.h
@@ -182,6 +182,11 @@ static inline bool kvm_vcpu_dabt_issext(struct kvm_vcpu *vcpu)
 	return kvm_vcpu_get_hsr(vcpu) & HSR_SSE;
 }
 
+static inline bool kvm_vcpu_dabt_issf(const struct kvm_vcpu *vcpu)
+{
+	return false;
+}
+
 static inline int kvm_vcpu_dabt_get_rd(struct kvm_vcpu *vcpu)
 {
 	return (kvm_vcpu_get_hsr(vcpu) & HSR_SRT_MASK) >> HSR_SRT_SHIFT;
diff --git a/arch/arm/include/asm/kvm_mmio.h b/arch/arm/include/asm/kvm_mmio.h
index 7c0eddb0adb2..32fbf82e3ebc 100644
--- a/arch/arm/include/asm/kvm_mmio.h
+++ b/arch/arm/include/asm/kvm_mmio.h
@@ -14,6 +14,8 @@
 struct kvm_decode {
 	unsigned long rt;
 	bool sign_extend;
+	/* Not used on 32-bit arm */
+	bool sixty_four;
 };
 
 void kvm_mmio_write_buf(void *buf, unsigned int len, unsigned long data);
diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index 5efe5ca8fecf..f407b6bdad2e 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -283,6 +283,11 @@ static inline bool kvm_vcpu_dabt_issext(const struct kvm_vcpu *vcpu)
 	return !!(kvm_vcpu_get_hsr(vcpu) & ESR_ELx_SSE);
 }
 
+static inline bool kvm_vcpu_dabt_issf(const struct kvm_vcpu *vcpu)
+{
+	return !!(kvm_vcpu_get_hsr(vcpu) & ESR_ELx_SF);
+}
+
 static inline int kvm_vcpu_dabt_get_rd(const struct kvm_vcpu *vcpu)
 {
 	return (kvm_vcpu_get_hsr(vcpu) & ESR_ELx_SRT_MASK) >> ESR_ELx_SRT_SHIFT;
diff --git a/arch/arm64/include/asm/kvm_mmio.h b/arch/arm64/include/asm/kvm_mmio.h
index 02b5c48fd467..b204501a0c39 100644
--- a/arch/arm64/include/asm/kvm_mmio.h
+++ b/arch/arm64/include/asm/kvm_mmio.h
@@ -10,13 +10,11 @@
 #include <linux/kvm_host.h>
 #include <asm/kvm_arm.h>
 
-/*
- * This is annoying. The mmio code requires this, even if we don't
- * need any decoding. To be fixed.
- */
 struct kvm_decode {
 	unsigned long rt;
 	bool sign_extend;
+	/* Witdth of the register accessed by the faulting instruction is 64-bits */
+	bool sixty_four;
 };
 
 void kvm_mmio_write_buf(void *buf, unsigned int len, unsigned long data);
diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c
index 70d3b449692c..e62454b2e529 100644
--- a/virt/kvm/arm/mmio.c
+++ b/virt/kvm/arm/mmio.c
@@ -83,7 +83,7 @@ unsigned long kvm_mmio_read_buf(const void *buf, unsigned int len)
 int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
 {
 	unsigned long data;
-	unsigned int len;
+	unsigned int len, regsize;
 	int mask;
 
 	/* Detect an already handled MMIO return */
@@ -105,6 +105,9 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
 			data = (data ^ mask) - mask;
 		}
 
+		if (!vcpu->arch.mmio_decode.sixty_four)
+			data = data & 0xffffffff;
+
 		trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr,
 			       &data);
 		data = vcpu_data_host_to_guest(vcpu, data, len);
@@ -125,6 +128,7 @@ static int decode_hsr(struct kvm_vcpu *vcpu, bool *is_write, int *len)
 	unsigned long rt;
 	int access_size;
 	bool sign_extend;
+	bool sixty_four;
 
 	if (kvm_vcpu_dabt_iss1tw(vcpu)) {
 		/* page table accesses IO mem: tell guest to fix its TTBR */
@@ -138,11 +142,13 @@ static int decode_hsr(struct kvm_vcpu *vcpu, bool *is_write, int *len)
 
 	*is_write = kvm_vcpu_dabt_iswrite(vcpu);
 	sign_extend = kvm_vcpu_dabt_issext(vcpu);
+	sixty_four = kvm_vcpu_dabt_issf(vcpu);
 	rt = kvm_vcpu_dabt_get_rd(vcpu);
 
 	*len = access_size;
 	vcpu->arch.mmio_decode.sign_extend = sign_extend;
 	vcpu->arch.mmio_decode.rt = rt;
+	vcpu->arch.mmio_decode.sixty_four = sixty_four;
 
 	return 0;
 }
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] KVM: arm64: Only sign-extend MMIO up to register width
  2019-12-12 19:50 ` Christoffer Dall
@ 2019-12-13 10:12   ` Marc Zyngier
  -1 siblings, 0 replies; 9+ messages in thread
From: Marc Zyngier @ 2019-12-13 10:12 UTC (permalink / raw)
  To: Christoffer Dall; +Cc: djordje.kovacevic, linux-arm-kernel, kvmarm

On 2019-12-12 19:50, Christoffer Dall wrote:
> On AArch64 you can do a sign-extended load to either a 32-bit or 
> 64-bit
> register, and we should only sign extend the register up to the width 
> of
> the register as specified in the operation (by using the 32-bit Wn or
> 64-bit Xn register specifier).

Nice catch. It's only been there for... Oh wait! ;-)

>
> As it turns out, the architecture provides this decoding information 
> in
> the SF ("Sixty-Four" -- how cute...) bit.
>
> Let's take advantage of this with the usual 32-bit/64-bit header file
> dance and do the right thing on AArch64 hosts.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>

Cc: stable?

> ---
>  arch/arm/include/asm/kvm_emulate.h   | 5 +++++
>  arch/arm/include/asm/kvm_mmio.h      | 2 ++
>  arch/arm64/include/asm/kvm_emulate.h | 5 +++++
>  arch/arm64/include/asm/kvm_mmio.h    | 6 ++----
>  virt/kvm/arm/mmio.c                  | 8 +++++++-
>  5 files changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/include/asm/kvm_emulate.h
> b/arch/arm/include/asm/kvm_emulate.h
> index 9b118516d2db..fe55d8737a11 100644
> --- a/arch/arm/include/asm/kvm_emulate.h
> +++ b/arch/arm/include/asm/kvm_emulate.h
> @@ -182,6 +182,11 @@ static inline bool kvm_vcpu_dabt_issext(struct
> kvm_vcpu *vcpu)
>  	return kvm_vcpu_get_hsr(vcpu) & HSR_SSE;
>  }
>
> +static inline bool kvm_vcpu_dabt_issf(const struct kvm_vcpu *vcpu)
> +{
> +	return false;
> +}
> +
>  static inline int kvm_vcpu_dabt_get_rd(struct kvm_vcpu *vcpu)
>  {
>  	return (kvm_vcpu_get_hsr(vcpu) & HSR_SRT_MASK) >> HSR_SRT_SHIFT;
> diff --git a/arch/arm/include/asm/kvm_mmio.h
> b/arch/arm/include/asm/kvm_mmio.h
> index 7c0eddb0adb2..32fbf82e3ebc 100644
> --- a/arch/arm/include/asm/kvm_mmio.h
> +++ b/arch/arm/include/asm/kvm_mmio.h
> @@ -14,6 +14,8 @@
>  struct kvm_decode {
>  	unsigned long rt;
>  	bool sign_extend;
> +	/* Not used on 32-bit arm */
> +	bool sixty_four;
>  };
>
>  void kvm_mmio_write_buf(void *buf, unsigned int len, unsigned long 
> data);
> diff --git a/arch/arm64/include/asm/kvm_emulate.h
> b/arch/arm64/include/asm/kvm_emulate.h
> index 5efe5ca8fecf..f407b6bdad2e 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -283,6 +283,11 @@ static inline bool kvm_vcpu_dabt_issext(const
> struct kvm_vcpu *vcpu)
>  	return !!(kvm_vcpu_get_hsr(vcpu) & ESR_ELx_SSE);
>  }
>
> +static inline bool kvm_vcpu_dabt_issf(const struct kvm_vcpu *vcpu)
> +{
> +	return !!(kvm_vcpu_get_hsr(vcpu) & ESR_ELx_SF);
> +}
> +
>  static inline int kvm_vcpu_dabt_get_rd(const struct kvm_vcpu *vcpu)
>  {
>  	return (kvm_vcpu_get_hsr(vcpu) & ESR_ELx_SRT_MASK) >> 
> ESR_ELx_SRT_SHIFT;
> diff --git a/arch/arm64/include/asm/kvm_mmio.h
> b/arch/arm64/include/asm/kvm_mmio.h
> index 02b5c48fd467..b204501a0c39 100644
> --- a/arch/arm64/include/asm/kvm_mmio.h
> +++ b/arch/arm64/include/asm/kvm_mmio.h
> @@ -10,13 +10,11 @@
>  #include <linux/kvm_host.h>
>  #include <asm/kvm_arm.h>
>
> -/*
> - * This is annoying. The mmio code requires this, even if we don't
> - * need any decoding. To be fixed.
> - */
>  struct kvm_decode {
>  	unsigned long rt;
>  	bool sign_extend;
> +	/* Witdth of the register accessed by the faulting instruction is
> 64-bits */
> +	bool sixty_four;
>  };
>
>  void kvm_mmio_write_buf(void *buf, unsigned int len, unsigned long 
> data);
> diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c
> index 70d3b449692c..e62454b2e529 100644
> --- a/virt/kvm/arm/mmio.c
> +++ b/virt/kvm/arm/mmio.c
> @@ -83,7 +83,7 @@ unsigned long kvm_mmio_read_buf(const void *buf,
> unsigned int len)
>  int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run 
> *run)
>  {
>  	unsigned long data;
> -	unsigned int len;
> +	unsigned int len, regsize;

Unused variable?

>  	int mask;
>
>  	/* Detect an already handled MMIO return */
> @@ -105,6 +105,9 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu,
> struct kvm_run *run)
>  			data = (data ^ mask) - mask;
>  		}
>
> +		if (!vcpu->arch.mmio_decode.sixty_four)
> +			data = data & 0xffffffff;
> +
>  		trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr,
>  			       &data);
>  		data = vcpu_data_host_to_guest(vcpu, data, len);
> @@ -125,6 +128,7 @@ static int decode_hsr(struct kvm_vcpu *vcpu, bool
> *is_write, int *len)
>  	unsigned long rt;
>  	int access_size;
>  	bool sign_extend;
> +	bool sixty_four;
>
>  	if (kvm_vcpu_dabt_iss1tw(vcpu)) {
>  		/* page table accesses IO mem: tell guest to fix its TTBR */
> @@ -138,11 +142,13 @@ static int decode_hsr(struct kvm_vcpu *vcpu,
> bool *is_write, int *len)
>
>  	*is_write = kvm_vcpu_dabt_iswrite(vcpu);
>  	sign_extend = kvm_vcpu_dabt_issext(vcpu);
> +	sixty_four = kvm_vcpu_dabt_issf(vcpu);
>  	rt = kvm_vcpu_dabt_get_rd(vcpu);
>
>  	*len = access_size;
>  	vcpu->arch.mmio_decode.sign_extend = sign_extend;
>  	vcpu->arch.mmio_decode.rt = rt;
> +	vcpu->arch.mmio_decode.sixty_four = sixty_four;
>
>  	return 0;
>  }

I can't remember why we keep this mmio_decode structure as part of
the vcpu. It isn't like it is going to change anytime soon (userspace
cannot change the saved ESR_EL2)...

Anyway, your patch is in keeping with the current shape of the code.
If you're OK with, it, I'll apply it with the above nits addressed.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] KVM: arm64: Only sign-extend MMIO up to register width
@ 2019-12-13 10:12   ` Marc Zyngier
  0 siblings, 0 replies; 9+ messages in thread
From: Marc Zyngier @ 2019-12-13 10:12 UTC (permalink / raw)
  To: Christoffer Dall
  Cc: Suzuki K Poulose, djordje.kovacevic, James Morse,
	linux-arm-kernel, kvmarm, Julien Thierry

On 2019-12-12 19:50, Christoffer Dall wrote:
> On AArch64 you can do a sign-extended load to either a 32-bit or 
> 64-bit
> register, and we should only sign extend the register up to the width 
> of
> the register as specified in the operation (by using the 32-bit Wn or
> 64-bit Xn register specifier).

Nice catch. It's only been there for... Oh wait! ;-)

>
> As it turns out, the architecture provides this decoding information 
> in
> the SF ("Sixty-Four" -- how cute...) bit.
>
> Let's take advantage of this with the usual 32-bit/64-bit header file
> dance and do the right thing on AArch64 hosts.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>

Cc: stable?

> ---
>  arch/arm/include/asm/kvm_emulate.h   | 5 +++++
>  arch/arm/include/asm/kvm_mmio.h      | 2 ++
>  arch/arm64/include/asm/kvm_emulate.h | 5 +++++
>  arch/arm64/include/asm/kvm_mmio.h    | 6 ++----
>  virt/kvm/arm/mmio.c                  | 8 +++++++-
>  5 files changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/include/asm/kvm_emulate.h
> b/arch/arm/include/asm/kvm_emulate.h
> index 9b118516d2db..fe55d8737a11 100644
> --- a/arch/arm/include/asm/kvm_emulate.h
> +++ b/arch/arm/include/asm/kvm_emulate.h
> @@ -182,6 +182,11 @@ static inline bool kvm_vcpu_dabt_issext(struct
> kvm_vcpu *vcpu)
>  	return kvm_vcpu_get_hsr(vcpu) & HSR_SSE;
>  }
>
> +static inline bool kvm_vcpu_dabt_issf(const struct kvm_vcpu *vcpu)
> +{
> +	return false;
> +}
> +
>  static inline int kvm_vcpu_dabt_get_rd(struct kvm_vcpu *vcpu)
>  {
>  	return (kvm_vcpu_get_hsr(vcpu) & HSR_SRT_MASK) >> HSR_SRT_SHIFT;
> diff --git a/arch/arm/include/asm/kvm_mmio.h
> b/arch/arm/include/asm/kvm_mmio.h
> index 7c0eddb0adb2..32fbf82e3ebc 100644
> --- a/arch/arm/include/asm/kvm_mmio.h
> +++ b/arch/arm/include/asm/kvm_mmio.h
> @@ -14,6 +14,8 @@
>  struct kvm_decode {
>  	unsigned long rt;
>  	bool sign_extend;
> +	/* Not used on 32-bit arm */
> +	bool sixty_four;
>  };
>
>  void kvm_mmio_write_buf(void *buf, unsigned int len, unsigned long 
> data);
> diff --git a/arch/arm64/include/asm/kvm_emulate.h
> b/arch/arm64/include/asm/kvm_emulate.h
> index 5efe5ca8fecf..f407b6bdad2e 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -283,6 +283,11 @@ static inline bool kvm_vcpu_dabt_issext(const
> struct kvm_vcpu *vcpu)
>  	return !!(kvm_vcpu_get_hsr(vcpu) & ESR_ELx_SSE);
>  }
>
> +static inline bool kvm_vcpu_dabt_issf(const struct kvm_vcpu *vcpu)
> +{
> +	return !!(kvm_vcpu_get_hsr(vcpu) & ESR_ELx_SF);
> +}
> +
>  static inline int kvm_vcpu_dabt_get_rd(const struct kvm_vcpu *vcpu)
>  {
>  	return (kvm_vcpu_get_hsr(vcpu) & ESR_ELx_SRT_MASK) >> 
> ESR_ELx_SRT_SHIFT;
> diff --git a/arch/arm64/include/asm/kvm_mmio.h
> b/arch/arm64/include/asm/kvm_mmio.h
> index 02b5c48fd467..b204501a0c39 100644
> --- a/arch/arm64/include/asm/kvm_mmio.h
> +++ b/arch/arm64/include/asm/kvm_mmio.h
> @@ -10,13 +10,11 @@
>  #include <linux/kvm_host.h>
>  #include <asm/kvm_arm.h>
>
> -/*
> - * This is annoying. The mmio code requires this, even if we don't
> - * need any decoding. To be fixed.
> - */
>  struct kvm_decode {
>  	unsigned long rt;
>  	bool sign_extend;
> +	/* Witdth of the register accessed by the faulting instruction is
> 64-bits */
> +	bool sixty_four;
>  };
>
>  void kvm_mmio_write_buf(void *buf, unsigned int len, unsigned long 
> data);
> diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c
> index 70d3b449692c..e62454b2e529 100644
> --- a/virt/kvm/arm/mmio.c
> +++ b/virt/kvm/arm/mmio.c
> @@ -83,7 +83,7 @@ unsigned long kvm_mmio_read_buf(const void *buf,
> unsigned int len)
>  int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run 
> *run)
>  {
>  	unsigned long data;
> -	unsigned int len;
> +	unsigned int len, regsize;

Unused variable?

>  	int mask;
>
>  	/* Detect an already handled MMIO return */
> @@ -105,6 +105,9 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu,
> struct kvm_run *run)
>  			data = (data ^ mask) - mask;
>  		}
>
> +		if (!vcpu->arch.mmio_decode.sixty_four)
> +			data = data & 0xffffffff;
> +
>  		trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr,
>  			       &data);
>  		data = vcpu_data_host_to_guest(vcpu, data, len);
> @@ -125,6 +128,7 @@ static int decode_hsr(struct kvm_vcpu *vcpu, bool
> *is_write, int *len)
>  	unsigned long rt;
>  	int access_size;
>  	bool sign_extend;
> +	bool sixty_four;
>
>  	if (kvm_vcpu_dabt_iss1tw(vcpu)) {
>  		/* page table accesses IO mem: tell guest to fix its TTBR */
> @@ -138,11 +142,13 @@ static int decode_hsr(struct kvm_vcpu *vcpu,
> bool *is_write, int *len)
>
>  	*is_write = kvm_vcpu_dabt_iswrite(vcpu);
>  	sign_extend = kvm_vcpu_dabt_issext(vcpu);
> +	sixty_four = kvm_vcpu_dabt_issf(vcpu);
>  	rt = kvm_vcpu_dabt_get_rd(vcpu);
>
>  	*len = access_size;
>  	vcpu->arch.mmio_decode.sign_extend = sign_extend;
>  	vcpu->arch.mmio_decode.rt = rt;
> +	vcpu->arch.mmio_decode.sixty_four = sixty_four;
>
>  	return 0;
>  }

I can't remember why we keep this mmio_decode structure as part of
the vcpu. It isn't like it is going to change anytime soon (userspace
cannot change the saved ESR_EL2)...

Anyway, your patch is in keeping with the current shape of the code.
If you're OK with, it, I'll apply it with the above nits addressed.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] KVM: arm64: Only sign-extend MMIO up to register width
  2019-12-13 10:12   ` Marc Zyngier
@ 2019-12-13 10:56     ` Christoffer Dall
  -1 siblings, 0 replies; 9+ messages in thread
From: Christoffer Dall @ 2019-12-13 10:56 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: djordje.kovacevic, linux-arm-kernel, kvmarm

On Fri, Dec 13, 2019 at 10:12:19AM +0000, Marc Zyngier wrote:
> On 2019-12-12 19:50, Christoffer Dall wrote:
> > On AArch64 you can do a sign-extended load to either a 32-bit or 64-bit
> > register, and we should only sign extend the register up to the width of
> > the register as specified in the operation (by using the 32-bit Wn or
> > 64-bit Xn register specifier).
> 
> Nice catch. It's only been there for... Oh wait! ;-)
> 
> > 
> > As it turns out, the architecture provides this decoding information in
> > the SF ("Sixty-Four" -- how cute...) bit.
> > 
> > Let's take advantage of this with the usual 32-bit/64-bit header file
> > dance and do the right thing on AArch64 hosts.
> > 
> > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> 
> Cc: stable?
> 

Yes, good idea.

> > ---
> >  arch/arm/include/asm/kvm_emulate.h   | 5 +++++
> >  arch/arm/include/asm/kvm_mmio.h      | 2 ++
> >  arch/arm64/include/asm/kvm_emulate.h | 5 +++++
> >  arch/arm64/include/asm/kvm_mmio.h    | 6 ++----
> >  virt/kvm/arm/mmio.c                  | 8 +++++++-
> >  5 files changed, 21 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/arm/include/asm/kvm_emulate.h
> > b/arch/arm/include/asm/kvm_emulate.h
> > index 9b118516d2db..fe55d8737a11 100644
> > --- a/arch/arm/include/asm/kvm_emulate.h
> > +++ b/arch/arm/include/asm/kvm_emulate.h
> > @@ -182,6 +182,11 @@ static inline bool kvm_vcpu_dabt_issext(struct
> > kvm_vcpu *vcpu)
> >  	return kvm_vcpu_get_hsr(vcpu) & HSR_SSE;
> >  }
> > 
> > +static inline bool kvm_vcpu_dabt_issf(const struct kvm_vcpu *vcpu)
> > +{
> > +	return false;
> > +}
> > +
> >  static inline int kvm_vcpu_dabt_get_rd(struct kvm_vcpu *vcpu)
> >  {
> >  	return (kvm_vcpu_get_hsr(vcpu) & HSR_SRT_MASK) >> HSR_SRT_SHIFT;
> > diff --git a/arch/arm/include/asm/kvm_mmio.h
> > b/arch/arm/include/asm/kvm_mmio.h
> > index 7c0eddb0adb2..32fbf82e3ebc 100644
> > --- a/arch/arm/include/asm/kvm_mmio.h
> > +++ b/arch/arm/include/asm/kvm_mmio.h
> > @@ -14,6 +14,8 @@
> >  struct kvm_decode {
> >  	unsigned long rt;
> >  	bool sign_extend;
> > +	/* Not used on 32-bit arm */
> > +	bool sixty_four;
> >  };
> > 
> >  void kvm_mmio_write_buf(void *buf, unsigned int len, unsigned long
> > data);
> > diff --git a/arch/arm64/include/asm/kvm_emulate.h
> > b/arch/arm64/include/asm/kvm_emulate.h
> > index 5efe5ca8fecf..f407b6bdad2e 100644
> > --- a/arch/arm64/include/asm/kvm_emulate.h
> > +++ b/arch/arm64/include/asm/kvm_emulate.h
> > @@ -283,6 +283,11 @@ static inline bool kvm_vcpu_dabt_issext(const
> > struct kvm_vcpu *vcpu)
> >  	return !!(kvm_vcpu_get_hsr(vcpu) & ESR_ELx_SSE);
> >  }
> > 
> > +static inline bool kvm_vcpu_dabt_issf(const struct kvm_vcpu *vcpu)
> > +{
> > +	return !!(kvm_vcpu_get_hsr(vcpu) & ESR_ELx_SF);
> > +}
> > +
> >  static inline int kvm_vcpu_dabt_get_rd(const struct kvm_vcpu *vcpu)
> >  {
> >  	return (kvm_vcpu_get_hsr(vcpu) & ESR_ELx_SRT_MASK) >>
> > ESR_ELx_SRT_SHIFT;
> > diff --git a/arch/arm64/include/asm/kvm_mmio.h
> > b/arch/arm64/include/asm/kvm_mmio.h
> > index 02b5c48fd467..b204501a0c39 100644
> > --- a/arch/arm64/include/asm/kvm_mmio.h
> > +++ b/arch/arm64/include/asm/kvm_mmio.h
> > @@ -10,13 +10,11 @@
> >  #include <linux/kvm_host.h>
> >  #include <asm/kvm_arm.h>
> > 
> > -/*
> > - * This is annoying. The mmio code requires this, even if we don't
> > - * need any decoding. To be fixed.
> > - */
> >  struct kvm_decode {
> >  	unsigned long rt;
> >  	bool sign_extend;
> > +	/* Witdth of the register accessed by the faulting instruction is
> > 64-bits */
> > +	bool sixty_four;
> >  };
> > 
> >  void kvm_mmio_write_buf(void *buf, unsigned int len, unsigned long
> > data);
> > diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c
> > index 70d3b449692c..e62454b2e529 100644
> > --- a/virt/kvm/arm/mmio.c
> > +++ b/virt/kvm/arm/mmio.c
> > @@ -83,7 +83,7 @@ unsigned long kvm_mmio_read_buf(const void *buf,
> > unsigned int len)
> >  int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
> >  {
> >  	unsigned long data;
> > -	unsigned int len;
> > +	unsigned int len, regsize;
> 
> Unused variable?
> 

Ah, yes, whoops.  Guess which unstaged change I still have in my tree...

> >  	int mask;
> > 
> >  	/* Detect an already handled MMIO return */
> > @@ -105,6 +105,9 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu,
> > struct kvm_run *run)
> >  			data = (data ^ mask) - mask;
> >  		}
> > 
> > +		if (!vcpu->arch.mmio_decode.sixty_four)
> > +			data = data & 0xffffffff;
> > +
> >  		trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr,
> >  			       &data);
> >  		data = vcpu_data_host_to_guest(vcpu, data, len);
> > @@ -125,6 +128,7 @@ static int decode_hsr(struct kvm_vcpu *vcpu, bool
> > *is_write, int *len)
> >  	unsigned long rt;
> >  	int access_size;
> >  	bool sign_extend;
> > +	bool sixty_four;
> > 
> >  	if (kvm_vcpu_dabt_iss1tw(vcpu)) {
> >  		/* page table accesses IO mem: tell guest to fix its TTBR */
> > @@ -138,11 +142,13 @@ static int decode_hsr(struct kvm_vcpu *vcpu,
> > bool *is_write, int *len)
> > 
> >  	*is_write = kvm_vcpu_dabt_iswrite(vcpu);
> >  	sign_extend = kvm_vcpu_dabt_issext(vcpu);
> > +	sixty_four = kvm_vcpu_dabt_issf(vcpu);
> >  	rt = kvm_vcpu_dabt_get_rd(vcpu);
> > 
> >  	*len = access_size;
> >  	vcpu->arch.mmio_decode.sign_extend = sign_extend;
> >  	vcpu->arch.mmio_decode.rt = rt;
> > +	vcpu->arch.mmio_decode.sixty_four = sixty_four;
> > 
> >  	return 0;
> >  }
> 
> I can't remember why we keep this mmio_decode structure as part of
> the vcpu. It isn't like it is going to change anytime soon (userspace
> cannot change the saved ESR_EL2)...

I think that was just an uninformed design decision on my part and it
could be reworked to operate on the ESR_EL2 directly or just take the
information from userspace (which we already rely on for read vs.
write).

> 
> Anyway, your patch is in keeping with the current shape of the code.
> If you're OK with, it, I'll apply it with the above nits addressed.
> 

Absolutely, I decided not to rework the mmio_decode stuff, and leave
that for some later day.


Thanks!

    Christoffer
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] KVM: arm64: Only sign-extend MMIO up to register width
@ 2019-12-13 10:56     ` Christoffer Dall
  0 siblings, 0 replies; 9+ messages in thread
From: Christoffer Dall @ 2019-12-13 10:56 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Suzuki K Poulose, djordje.kovacevic, James Morse,
	linux-arm-kernel, kvmarm, Julien Thierry

On Fri, Dec 13, 2019 at 10:12:19AM +0000, Marc Zyngier wrote:
> On 2019-12-12 19:50, Christoffer Dall wrote:
> > On AArch64 you can do a sign-extended load to either a 32-bit or 64-bit
> > register, and we should only sign extend the register up to the width of
> > the register as specified in the operation (by using the 32-bit Wn or
> > 64-bit Xn register specifier).
> 
> Nice catch. It's only been there for... Oh wait! ;-)
> 
> > 
> > As it turns out, the architecture provides this decoding information in
> > the SF ("Sixty-Four" -- how cute...) bit.
> > 
> > Let's take advantage of this with the usual 32-bit/64-bit header file
> > dance and do the right thing on AArch64 hosts.
> > 
> > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> 
> Cc: stable?
> 

Yes, good idea.

> > ---
> >  arch/arm/include/asm/kvm_emulate.h   | 5 +++++
> >  arch/arm/include/asm/kvm_mmio.h      | 2 ++
> >  arch/arm64/include/asm/kvm_emulate.h | 5 +++++
> >  arch/arm64/include/asm/kvm_mmio.h    | 6 ++----
> >  virt/kvm/arm/mmio.c                  | 8 +++++++-
> >  5 files changed, 21 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/arm/include/asm/kvm_emulate.h
> > b/arch/arm/include/asm/kvm_emulate.h
> > index 9b118516d2db..fe55d8737a11 100644
> > --- a/arch/arm/include/asm/kvm_emulate.h
> > +++ b/arch/arm/include/asm/kvm_emulate.h
> > @@ -182,6 +182,11 @@ static inline bool kvm_vcpu_dabt_issext(struct
> > kvm_vcpu *vcpu)
> >  	return kvm_vcpu_get_hsr(vcpu) & HSR_SSE;
> >  }
> > 
> > +static inline bool kvm_vcpu_dabt_issf(const struct kvm_vcpu *vcpu)
> > +{
> > +	return false;
> > +}
> > +
> >  static inline int kvm_vcpu_dabt_get_rd(struct kvm_vcpu *vcpu)
> >  {
> >  	return (kvm_vcpu_get_hsr(vcpu) & HSR_SRT_MASK) >> HSR_SRT_SHIFT;
> > diff --git a/arch/arm/include/asm/kvm_mmio.h
> > b/arch/arm/include/asm/kvm_mmio.h
> > index 7c0eddb0adb2..32fbf82e3ebc 100644
> > --- a/arch/arm/include/asm/kvm_mmio.h
> > +++ b/arch/arm/include/asm/kvm_mmio.h
> > @@ -14,6 +14,8 @@
> >  struct kvm_decode {
> >  	unsigned long rt;
> >  	bool sign_extend;
> > +	/* Not used on 32-bit arm */
> > +	bool sixty_four;
> >  };
> > 
> >  void kvm_mmio_write_buf(void *buf, unsigned int len, unsigned long
> > data);
> > diff --git a/arch/arm64/include/asm/kvm_emulate.h
> > b/arch/arm64/include/asm/kvm_emulate.h
> > index 5efe5ca8fecf..f407b6bdad2e 100644
> > --- a/arch/arm64/include/asm/kvm_emulate.h
> > +++ b/arch/arm64/include/asm/kvm_emulate.h
> > @@ -283,6 +283,11 @@ static inline bool kvm_vcpu_dabt_issext(const
> > struct kvm_vcpu *vcpu)
> >  	return !!(kvm_vcpu_get_hsr(vcpu) & ESR_ELx_SSE);
> >  }
> > 
> > +static inline bool kvm_vcpu_dabt_issf(const struct kvm_vcpu *vcpu)
> > +{
> > +	return !!(kvm_vcpu_get_hsr(vcpu) & ESR_ELx_SF);
> > +}
> > +
> >  static inline int kvm_vcpu_dabt_get_rd(const struct kvm_vcpu *vcpu)
> >  {
> >  	return (kvm_vcpu_get_hsr(vcpu) & ESR_ELx_SRT_MASK) >>
> > ESR_ELx_SRT_SHIFT;
> > diff --git a/arch/arm64/include/asm/kvm_mmio.h
> > b/arch/arm64/include/asm/kvm_mmio.h
> > index 02b5c48fd467..b204501a0c39 100644
> > --- a/arch/arm64/include/asm/kvm_mmio.h
> > +++ b/arch/arm64/include/asm/kvm_mmio.h
> > @@ -10,13 +10,11 @@
> >  #include <linux/kvm_host.h>
> >  #include <asm/kvm_arm.h>
> > 
> > -/*
> > - * This is annoying. The mmio code requires this, even if we don't
> > - * need any decoding. To be fixed.
> > - */
> >  struct kvm_decode {
> >  	unsigned long rt;
> >  	bool sign_extend;
> > +	/* Witdth of the register accessed by the faulting instruction is
> > 64-bits */
> > +	bool sixty_four;
> >  };
> > 
> >  void kvm_mmio_write_buf(void *buf, unsigned int len, unsigned long
> > data);
> > diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c
> > index 70d3b449692c..e62454b2e529 100644
> > --- a/virt/kvm/arm/mmio.c
> > +++ b/virt/kvm/arm/mmio.c
> > @@ -83,7 +83,7 @@ unsigned long kvm_mmio_read_buf(const void *buf,
> > unsigned int len)
> >  int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
> >  {
> >  	unsigned long data;
> > -	unsigned int len;
> > +	unsigned int len, regsize;
> 
> Unused variable?
> 

Ah, yes, whoops.  Guess which unstaged change I still have in my tree...

> >  	int mask;
> > 
> >  	/* Detect an already handled MMIO return */
> > @@ -105,6 +105,9 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu,
> > struct kvm_run *run)
> >  			data = (data ^ mask) - mask;
> >  		}
> > 
> > +		if (!vcpu->arch.mmio_decode.sixty_four)
> > +			data = data & 0xffffffff;
> > +
> >  		trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr,
> >  			       &data);
> >  		data = vcpu_data_host_to_guest(vcpu, data, len);
> > @@ -125,6 +128,7 @@ static int decode_hsr(struct kvm_vcpu *vcpu, bool
> > *is_write, int *len)
> >  	unsigned long rt;
> >  	int access_size;
> >  	bool sign_extend;
> > +	bool sixty_four;
> > 
> >  	if (kvm_vcpu_dabt_iss1tw(vcpu)) {
> >  		/* page table accesses IO mem: tell guest to fix its TTBR */
> > @@ -138,11 +142,13 @@ static int decode_hsr(struct kvm_vcpu *vcpu,
> > bool *is_write, int *len)
> > 
> >  	*is_write = kvm_vcpu_dabt_iswrite(vcpu);
> >  	sign_extend = kvm_vcpu_dabt_issext(vcpu);
> > +	sixty_four = kvm_vcpu_dabt_issf(vcpu);
> >  	rt = kvm_vcpu_dabt_get_rd(vcpu);
> > 
> >  	*len = access_size;
> >  	vcpu->arch.mmio_decode.sign_extend = sign_extend;
> >  	vcpu->arch.mmio_decode.rt = rt;
> > +	vcpu->arch.mmio_decode.sixty_four = sixty_four;
> > 
> >  	return 0;
> >  }
> 
> I can't remember why we keep this mmio_decode structure as part of
> the vcpu. It isn't like it is going to change anytime soon (userspace
> cannot change the saved ESR_EL2)...

I think that was just an uninformed design decision on my part and it
could be reworked to operate on the ESR_EL2 directly or just take the
information from userspace (which we already rely on for read vs.
write).

> 
> Anyway, your patch is in keeping with the current shape of the code.
> If you're OK with, it, I'll apply it with the above nits addressed.
> 

Absolutely, I decided not to rework the mmio_decode stuff, and leave
that for some later day.


Thanks!

    Christoffer

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] KVM: arm64: Only sign-extend MMIO up to register width
  2019-12-13 10:56     ` Christoffer Dall
  (?)
@ 2019-12-13 14:05     ` Djordje Kovacevic
  2019-12-13 14:24         ` Christoffer Dall
  -1 siblings, 1 reply; 9+ messages in thread
From: Djordje Kovacevic @ 2019-12-13 14:05 UTC (permalink / raw)
  To: Christoffer Dall, Marc Zyngier
  Cc: Alexander Shirshikov, linux-arm-kernel, kvmarm


[-- Attachment #1.1: Type: text/plain, Size: 8283 bytes --]

Hi Christoffer,
I have run some test payload to get the exact behavior of all nine LDR[S][W|H|B] [Xt|Wt] instructions. Here it is:

 # instruction       sas  sse  sf   Xt contents
===========================================================================
 1 LDR    Xt, ...     3    0    1   b[63:0] = MEM[63:0]
 2 LDR    Wt, ...     2    0    0   b[63:32]='0..0' b[31:0] = MEM[31:0]
 3 LDRH   Wt, ...     1    0    0   b[63:16]='0..0' b[15:0] = MEM[15:0]
 4 LDRB   Wt, ...     0    0    0   b[63:8] ='0..0' b[7:0]  = MEM[7:0]

 5 LDRSW  Xt, ...     2    1    1   b[63:32] = MEM[31] b[31:0] = MEM[31:0]
 6 LDRSH  Xt, ...     1    1    1   b[63:16] = MEM[15] b[15:0] = MEM[15:0]
 7 LDRSH  Wt, ...     1    1    0   b[63:32] = '0..0' b[31:16] = MEM[15] b[15:0] = MEM[15:0]
 8 LDRSB  Xt, ...     0    1    1   b[63:8] = MEM[7] b[7:0] = MEM[7:0]
 9 LDRSB  Wt, ...     0    1    0   b[63:32] = '0..0' b[31:8] = MEM[7] b[7:0] = MEM[7:0]

Any surprises?

DjK

________________________________
From: Christoffer Dall <christoffer.dall@arm.com>
Sent: 13 December 2019 10:56
To: Marc Zyngier <maz@kernel.org>
Cc: kvmarm@lists.cs.columbia.edu <kvmarm@lists.cs.columbia.edu>; linux-arm-kernel@lists.infradead.org <linux-arm-kernel@lists.infradead.org>; Djordje Kovacevic <Djordje.Kovacevic@arm.com>; James Morse <James.Morse@arm.com>; Julien Thierry <julien.thierry.kdev@gmail.com>; Suzuki Poulose <Suzuki.Poulose@arm.com>
Subject: Re: [PATCH] KVM: arm64: Only sign-extend MMIO up to register width

On Fri, Dec 13, 2019 at 10:12:19AM +0000, Marc Zyngier wrote:
> On 2019-12-12 19:50, Christoffer Dall wrote:
> > On AArch64 you can do a sign-extended load to either a 32-bit or 64-bit
> > register, and we should only sign extend the register up to the width of
> > the register as specified in the operation (by using the 32-bit Wn or
> > 64-bit Xn register specifier).
>
> Nice catch. It's only been there for... Oh wait! ;-)
>
> >
> > As it turns out, the architecture provides this decoding information in
> > the SF ("Sixty-Four" -- how cute...) bit.
> >
> > Let's take advantage of this with the usual 32-bit/64-bit header file
> > dance and do the right thing on AArch64 hosts.
> >
> > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
>
> Cc: stable?
>

Yes, good idea.

> > ---
> >  arch/arm/include/asm/kvm_emulate.h   | 5 +++++
> >  arch/arm/include/asm/kvm_mmio.h      | 2 ++
> >  arch/arm64/include/asm/kvm_emulate.h | 5 +++++
> >  arch/arm64/include/asm/kvm_mmio.h    | 6 ++----
> >  virt/kvm/arm/mmio.c                  | 8 +++++++-
> >  5 files changed, 21 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/arm/include/asm/kvm_emulate.h
> > b/arch/arm/include/asm/kvm_emulate.h
> > index 9b118516d2db..fe55d8737a11 100644
> > --- a/arch/arm/include/asm/kvm_emulate.h
> > +++ b/arch/arm/include/asm/kvm_emulate.h
> > @@ -182,6 +182,11 @@ static inline bool kvm_vcpu_dabt_issext(struct
> > kvm_vcpu *vcpu)
> >      return kvm_vcpu_get_hsr(vcpu) & HSR_SSE;
> >  }
> >
> > +static inline bool kvm_vcpu_dabt_issf(const struct kvm_vcpu *vcpu)
> > +{
> > +   return false;
> > +}
> > +
> >  static inline int kvm_vcpu_dabt_get_rd(struct kvm_vcpu *vcpu)
> >  {
> >      return (kvm_vcpu_get_hsr(vcpu) & HSR_SRT_MASK) >> HSR_SRT_SHIFT;
> > diff --git a/arch/arm/include/asm/kvm_mmio.h
> > b/arch/arm/include/asm/kvm_mmio.h
> > index 7c0eddb0adb2..32fbf82e3ebc 100644
> > --- a/arch/arm/include/asm/kvm_mmio.h
> > +++ b/arch/arm/include/asm/kvm_mmio.h
> > @@ -14,6 +14,8 @@
> >  struct kvm_decode {
> >      unsigned long rt;
> >      bool sign_extend;
> > +   /* Not used on 32-bit arm */
> > +   bool sixty_four;
> >  };
> >
> >  void kvm_mmio_write_buf(void *buf, unsigned int len, unsigned long
> > data);
> > diff --git a/arch/arm64/include/asm/kvm_emulate.h
> > b/arch/arm64/include/asm/kvm_emulate.h
> > index 5efe5ca8fecf..f407b6bdad2e 100644
> > --- a/arch/arm64/include/asm/kvm_emulate.h
> > +++ b/arch/arm64/include/asm/kvm_emulate.h
> > @@ -283,6 +283,11 @@ static inline bool kvm_vcpu_dabt_issext(const
> > struct kvm_vcpu *vcpu)
> >      return !!(kvm_vcpu_get_hsr(vcpu) & ESR_ELx_SSE);
> >  }
> >
> > +static inline bool kvm_vcpu_dabt_issf(const struct kvm_vcpu *vcpu)
> > +{
> > +   return !!(kvm_vcpu_get_hsr(vcpu) & ESR_ELx_SF);
> > +}
> > +
> >  static inline int kvm_vcpu_dabt_get_rd(const struct kvm_vcpu *vcpu)
> >  {
> >      return (kvm_vcpu_get_hsr(vcpu) & ESR_ELx_SRT_MASK) >>
> > ESR_ELx_SRT_SHIFT;
> > diff --git a/arch/arm64/include/asm/kvm_mmio.h
> > b/arch/arm64/include/asm/kvm_mmio.h
> > index 02b5c48fd467..b204501a0c39 100644
> > --- a/arch/arm64/include/asm/kvm_mmio.h
> > +++ b/arch/arm64/include/asm/kvm_mmio.h
> > @@ -10,13 +10,11 @@
> >  #include <linux/kvm_host.h>
> >  #include <asm/kvm_arm.h>
> >
> > -/*
> > - * This is annoying. The mmio code requires this, even if we don't
> > - * need any decoding. To be fixed.
> > - */
> >  struct kvm_decode {
> >      unsigned long rt;
> >      bool sign_extend;
> > +   /* Witdth of the register accessed by the faulting instruction is
> > 64-bits */
> > +   bool sixty_four;
> >  };
> >
> >  void kvm_mmio_write_buf(void *buf, unsigned int len, unsigned long
> > data);
> > diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c
> > index 70d3b449692c..e62454b2e529 100644
> > --- a/virt/kvm/arm/mmio.c
> > +++ b/virt/kvm/arm/mmio.c
> > @@ -83,7 +83,7 @@ unsigned long kvm_mmio_read_buf(const void *buf,
> > unsigned int len)
> >  int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
> >  {
> >      unsigned long data;
> > -   unsigned int len;
> > +   unsigned int len, regsize;
>
> Unused variable?
>

Ah, yes, whoops.  Guess which unstaged change I still have in my tree...

> >      int mask;
> >
> >      /* Detect an already handled MMIO return */
> > @@ -105,6 +105,9 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu,
> > struct kvm_run *run)
> >                      data = (data ^ mask) - mask;
> >              }
> >
> > +           if (!vcpu->arch.mmio_decode.sixty_four)
> > +                   data = data & 0xffffffff;
> > +
> >              trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr,
> >                             &data);
> >              data = vcpu_data_host_to_guest(vcpu, data, len);
> > @@ -125,6 +128,7 @@ static int decode_hsr(struct kvm_vcpu *vcpu, bool
> > *is_write, int *len)
> >      unsigned long rt;
> >      int access_size;
> >      bool sign_extend;
> > +   bool sixty_four;
> >
> >      if (kvm_vcpu_dabt_iss1tw(vcpu)) {
> >              /* page table accesses IO mem: tell guest to fix its TTBR */
> > @@ -138,11 +142,13 @@ static int decode_hsr(struct kvm_vcpu *vcpu,
> > bool *is_write, int *len)
> >
> >      *is_write = kvm_vcpu_dabt_iswrite(vcpu);
> >      sign_extend = kvm_vcpu_dabt_issext(vcpu);
> > +   sixty_four = kvm_vcpu_dabt_issf(vcpu);
> >      rt = kvm_vcpu_dabt_get_rd(vcpu);
> >
> >      *len = access_size;
> >      vcpu->arch.mmio_decode.sign_extend = sign_extend;
> >      vcpu->arch.mmio_decode.rt = rt;
> > +   vcpu->arch.mmio_decode.sixty_four = sixty_four;
> >
> >      return 0;
> >  }
>
> I can't remember why we keep this mmio_decode structure as part of
> the vcpu. It isn't like it is going to change anytime soon (userspace
> cannot change the saved ESR_EL2)...

I think that was just an uninformed design decision on my part and it
could be reworked to operate on the ESR_EL2 directly or just take the
information from userspace (which we already rely on for read vs.
write).

>
> Anyway, your patch is in keeping with the current shape of the code.
> If you're OK with, it, I'll apply it with the above nits addressed.
>

Absolutely, I decided not to rework the mmio_decode stuff, and leave
that for some later day.


Thanks!

    Christoffer
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[-- Attachment #1.2: Type: text/html, Size: 14901 bytes --]

[-- Attachment #2: Type: text/plain, Size: 151 bytes --]

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] KVM: arm64: Only sign-extend MMIO up to register width
  2019-12-13 14:05     ` Djordje Kovacevic
@ 2019-12-13 14:24         ` Christoffer Dall
  0 siblings, 0 replies; 9+ messages in thread
From: Christoffer Dall @ 2019-12-13 14:24 UTC (permalink / raw)
  To: Djordje Kovacevic
  Cc: Marc Zyngier, Alexander Shirshikov, linux-arm-kernel, kvmarm

On Fri, Dec 13, 2019 at 03:05:19PM +0100, Djordje Kovacevic wrote:
>    Hi Christoffer,
> 
>    I have run some test payload to get the exact behavior of all nine
>    LDR[S][W|H|B] [Xt|Wt] instructions. Here it is:
> 
>     # instruction       sas  sse  sf   Xt contents
>    =======================================================================
>    ====
>     1 LDR    Xt, ...     3    0    1   b[63:0] = MEM[63:0]
>     2 LDR    Wt, ...     2    0    0   b[63:32]='0..0' b[31:0] = MEM[31:0]
>     3 LDRH   Wt, ...     1    0    0   b[63:16]='0..0' b[15:0] = MEM[15:0]
>     4 LDRB   Wt, ...     0    0    0   b[63:8] ='0..0' b[7:0]  = MEM[7:0]
>     5 LDRSW  Xt, ...     2    1    1   b[63:32] = MEM[31] b[31:0] =
>    MEM[31:0]
>     6 LDRSH  Xt, ...     1    1    1   b[63:16] = MEM[15] b[15:0] =
>    MEM[15:0]
>     7 LDRSH  Wt, ...     1    1    0   b[63:32] = '0..0' b[31:16] =
>    MEM[15] b[15:0] = MEM[15:0]
>     8 LDRSB  Xt, ...     0    1    1   b[63:8] = MEM[7] b[7:0] = MEM[7:0]
>     9 LDRSB  Wt, ...     0    1    0   b[63:32] = '0..0' b[31:8] = MEM[7]
>    b[7:0] = MEM[7:0]
> 
>    Any surprises?

No, this looks as I expected it to.

Thanks for the test.

    Christoffer

>      __________________________________________________________________
> 
>    From: Christoffer Dall <christoffer.dall@arm.com>
>    Sent: 13 December 2019 10:56
>    To: Marc Zyngier <maz@kernel.org>
>    Cc: kvmarm@lists.cs.columbia.edu <kvmarm@lists.cs.columbia.edu>;
>    linux-arm-kernel@lists.infradead.org
>    <linux-arm-kernel@lists.infradead.org>; Djordje Kovacevic
>    <Djordje.Kovacevic@arm.com>; James Morse <James.Morse@arm.com>; Julien
>    Thierry <julien.thierry.kdev@gmail.com>; Suzuki Poulose
>    <Suzuki.Poulose@arm.com>
>    Subject: Re: [PATCH] KVM: arm64: Only sign-extend MMIO up to register
>    width
> 
>    On Fri, Dec 13, 2019 at 10:12:19AM +0000, Marc Zyngier wrote:
>    > On 2019-12-12 19:50, Christoffer Dall wrote:
>    > > On AArch64 you can do a sign-extended load to either a 32-bit or
>    64-bit
>    > > register, and we should only sign extend the register up to the
>    width of
>    > > the register as specified in the operation (by using the 32-bit Wn
>    or
>    > > 64-bit Xn register specifier).
>    >
>    > Nice catch. It's only been there for... Oh wait! ;-)
>    >
>    > >
>    > > As it turns out, the architecture provides this decoding
>    information in
>    > > the SF ("Sixty-Four" -- how cute...) bit.
>    > >
>    > > Let's take advantage of this with the usual 32-bit/64-bit header
>    file
>    > > dance and do the right thing on AArch64 hosts.
>    > >
>    > > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
>    >
>    > Cc: stable?
>    >
>    Yes, good idea.
>    > > ---
>    > >  arch/arm/include/asm/kvm_emulate.h   | 5 +++++
>    > >  arch/arm/include/asm/kvm_mmio.h      | 2 ++
>    > >  arch/arm64/include/asm/kvm_emulate.h | 5 +++++
>    > >  arch/arm64/include/asm/kvm_mmio.h    | 6 ++----
>    > >  virt/kvm/arm/mmio.c                  | 8 +++++++-
>    > >  5 files changed, 21 insertions(+), 5 deletions(-)
>    > >
>    > > diff --git a/arch/arm/include/asm/kvm_emulate.h
>    > > b/arch/arm/include/asm/kvm_emulate.h
>    > > index 9b118516d2db..fe55d8737a11 100644
>    > > --- a/arch/arm/include/asm/kvm_emulate.h
>    > > +++ b/arch/arm/include/asm/kvm_emulate.h
>    > > @@ -182,6 +182,11 @@ static inline bool kvm_vcpu_dabt_issext(struct
>    > > kvm_vcpu *vcpu)
>    > >      return kvm_vcpu_get_hsr(vcpu) & HSR_SSE;
>    > >  }
>    > >
>    > > +static inline bool kvm_vcpu_dabt_issf(const struct kvm_vcpu *vcpu)
>    > > +{
>    > > +   return false;
>    > > +}
>    > > +
>    > >  static inline int kvm_vcpu_dabt_get_rd(struct kvm_vcpu *vcpu)
>    > >  {
>    > >      return (kvm_vcpu_get_hsr(vcpu) & HSR_SRT_MASK) >>
>    HSR_SRT_SHIFT;
>    > > diff --git a/arch/arm/include/asm/kvm_mmio.h
>    > > b/arch/arm/include/asm/kvm_mmio.h
>    > > index 7c0eddb0adb2..32fbf82e3ebc 100644
>    > > --- a/arch/arm/include/asm/kvm_mmio.h
>    > > +++ b/arch/arm/include/asm/kvm_mmio.h
>    > > @@ -14,6 +14,8 @@
>    > >  struct kvm_decode {
>    > >      unsigned long rt;
>    > >      bool sign_extend;
>    > > +   /* Not used on 32-bit arm */
>    > > +   bool sixty_four;
>    > >  };
>    > >
>    > >  void kvm_mmio_write_buf(void *buf, unsigned int len, unsigned long
>    > > data);
>    > > diff --git a/arch/arm64/include/asm/kvm_emulate.h
>    > > b/arch/arm64/include/asm/kvm_emulate.h
>    > > index 5efe5ca8fecf..f407b6bdad2e 100644
>    > > --- a/arch/arm64/include/asm/kvm_emulate.h
>    > > +++ b/arch/arm64/include/asm/kvm_emulate.h
>    > > @@ -283,6 +283,11 @@ static inline bool kvm_vcpu_dabt_issext(const
>    > > struct kvm_vcpu *vcpu)
>    > >      return !!(kvm_vcpu_get_hsr(vcpu) & ESR_ELx_SSE);
>    > >  }
>    > >
>    > > +static inline bool kvm_vcpu_dabt_issf(const struct kvm_vcpu *vcpu)
>    > > +{
>    > > +   return !!(kvm_vcpu_get_hsr(vcpu) & ESR_ELx_SF);
>    > > +}
>    > > +
>    > >  static inline int kvm_vcpu_dabt_get_rd(const struct kvm_vcpu
>    *vcpu)
>    > >  {
>    > >      return (kvm_vcpu_get_hsr(vcpu) & ESR_ELx_SRT_MASK) >>
>    > > ESR_ELx_SRT_SHIFT;
>    > > diff --git a/arch/arm64/include/asm/kvm_mmio.h
>    > > b/arch/arm64/include/asm/kvm_mmio.h
>    > > index 02b5c48fd467..b204501a0c39 100644
>    > > --- a/arch/arm64/include/asm/kvm_mmio.h
>    > > +++ b/arch/arm64/include/asm/kvm_mmio.h
>    > > @@ -10,13 +10,11 @@
>    > >  #include <linux/kvm_host.h>
>    > >  #include <asm/kvm_arm.h>
>    > >
>    > > -/*
>    > > - * This is annoying. The mmio code requires this, even if we don't
>    > > - * need any decoding. To be fixed.
>    > > - */
>    > >  struct kvm_decode {
>    > >      unsigned long rt;
>    > >      bool sign_extend;
>    > > +   /* Witdth of the register accessed by the faulting instruction
>    is
>    > > 64-bits */
>    > > +   bool sixty_four;
>    > >  };
>    > >
>    > >  void kvm_mmio_write_buf(void *buf, unsigned int len, unsigned long
>    > > data);
>    > > diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c
>    > > index 70d3b449692c..e62454b2e529 100644
>    > > --- a/virt/kvm/arm/mmio.c
>    > > +++ b/virt/kvm/arm/mmio.c
>    > > @@ -83,7 +83,7 @@ unsigned long kvm_mmio_read_buf(const void *buf,
>    > > unsigned int len)
>    > >  int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run
>    *run)
>    > >  {
>    > >      unsigned long data;
>    > > -   unsigned int len;
>    > > +   unsigned int len, regsize;
>    >
>    > Unused variable?
>    >
>    Ah, yes, whoops.  Guess which unstaged change I still have in my
>    tree...
>    > >      int mask;
>    > >
>    > >      /* Detect an already handled MMIO return */
>    > > @@ -105,6 +105,9 @@ int kvm_handle_mmio_return(struct kvm_vcpu
>    *vcpu,
>    > > struct kvm_run *run)
>    > >                      data = (data ^ mask) - mask;
>    > >              }
>    > >
>    > > +           if (!vcpu->arch.mmio_decode.sixty_four)
>    > > +                   data = data & 0xffffffff;
>    > > +
>    > >              trace_kvm_mmio(KVM_TRACE_MMIO_READ, len,
>    run->mmio.phys_addr,
>    > >                             &data);
>    > >              data = vcpu_data_host_to_guest(vcpu, data, len);
>    > > @@ -125,6 +128,7 @@ static int decode_hsr(struct kvm_vcpu *vcpu,
>    bool
>    > > *is_write, int *len)
>    > >      unsigned long rt;
>    > >      int access_size;
>    > >      bool sign_extend;
>    > > +   bool sixty_four;
>    > >
>    > >      if (kvm_vcpu_dabt_iss1tw(vcpu)) {
>    > >              /* page table accesses IO mem: tell guest to fix its
>    TTBR */
>    > > @@ -138,11 +142,13 @@ static int decode_hsr(struct kvm_vcpu *vcpu,
>    > > bool *is_write, int *len)
>    > >
>    > >      *is_write = kvm_vcpu_dabt_iswrite(vcpu);
>    > >      sign_extend = kvm_vcpu_dabt_issext(vcpu);
>    > > +   sixty_four = kvm_vcpu_dabt_issf(vcpu);
>    > >      rt = kvm_vcpu_dabt_get_rd(vcpu);
>    > >
>    > >      *len = access_size;
>    > >      vcpu->arch.mmio_decode.sign_extend = sign_extend;
>    > >      vcpu->arch.mmio_decode.rt = rt;
>    > > +   vcpu->arch.mmio_decode.sixty_four = sixty_four;
>    > >
>    > >      return 0;
>    > >  }
>    >
>    > I can't remember why we keep this mmio_decode structure as part of
>    > the vcpu. It isn't like it is going to change anytime soon (userspace
>    > cannot change the saved ESR_EL2)...
>    I think that was just an uninformed design decision on my part and it
>    could be reworked to operate on the ESR_EL2 directly or just take the
>    information from userspace (which we already rely on for read vs.
>    write).
>    >
>    > Anyway, your patch is in keeping with the current shape of the code.
>    > If you're OK with, it, I'll apply it with the above nits addressed.
>    >
>    Absolutely, I decided not to rework the mmio_decode stuff, and leave
>    that for some later day.
>    Thanks!
>        Christoffer
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] KVM: arm64: Only sign-extend MMIO up to register width
@ 2019-12-13 14:24         ` Christoffer Dall
  0 siblings, 0 replies; 9+ messages in thread
From: Christoffer Dall @ 2019-12-13 14:24 UTC (permalink / raw)
  To: Djordje Kovacevic
  Cc: Suzuki Poulose, Marc Zyngier, Alexander Shirshikov, James Morse,
	linux-arm-kernel, kvmarm, Julien Thierry

On Fri, Dec 13, 2019 at 03:05:19PM +0100, Djordje Kovacevic wrote:
>    Hi Christoffer,
> 
>    I have run some test payload to get the exact behavior of all nine
>    LDR[S][W|H|B] [Xt|Wt] instructions. Here it is:
> 
>     # instruction       sas  sse  sf   Xt contents
>    =======================================================================
>    ====
>     1 LDR    Xt, ...     3    0    1   b[63:0] = MEM[63:0]
>     2 LDR    Wt, ...     2    0    0   b[63:32]='0..0' b[31:0] = MEM[31:0]
>     3 LDRH   Wt, ...     1    0    0   b[63:16]='0..0' b[15:0] = MEM[15:0]
>     4 LDRB   Wt, ...     0    0    0   b[63:8] ='0..0' b[7:0]  = MEM[7:0]
>     5 LDRSW  Xt, ...     2    1    1   b[63:32] = MEM[31] b[31:0] =
>    MEM[31:0]
>     6 LDRSH  Xt, ...     1    1    1   b[63:16] = MEM[15] b[15:0] =
>    MEM[15:0]
>     7 LDRSH  Wt, ...     1    1    0   b[63:32] = '0..0' b[31:16] =
>    MEM[15] b[15:0] = MEM[15:0]
>     8 LDRSB  Xt, ...     0    1    1   b[63:8] = MEM[7] b[7:0] = MEM[7:0]
>     9 LDRSB  Wt, ...     0    1    0   b[63:32] = '0..0' b[31:8] = MEM[7]
>    b[7:0] = MEM[7:0]
> 
>    Any surprises?

No, this looks as I expected it to.

Thanks for the test.

    Christoffer

>      __________________________________________________________________
> 
>    From: Christoffer Dall <christoffer.dall@arm.com>
>    Sent: 13 December 2019 10:56
>    To: Marc Zyngier <maz@kernel.org>
>    Cc: kvmarm@lists.cs.columbia.edu <kvmarm@lists.cs.columbia.edu>;
>    linux-arm-kernel@lists.infradead.org
>    <linux-arm-kernel@lists.infradead.org>; Djordje Kovacevic
>    <Djordje.Kovacevic@arm.com>; James Morse <James.Morse@arm.com>; Julien
>    Thierry <julien.thierry.kdev@gmail.com>; Suzuki Poulose
>    <Suzuki.Poulose@arm.com>
>    Subject: Re: [PATCH] KVM: arm64: Only sign-extend MMIO up to register
>    width
> 
>    On Fri, Dec 13, 2019 at 10:12:19AM +0000, Marc Zyngier wrote:
>    > On 2019-12-12 19:50, Christoffer Dall wrote:
>    > > On AArch64 you can do a sign-extended load to either a 32-bit or
>    64-bit
>    > > register, and we should only sign extend the register up to the
>    width of
>    > > the register as specified in the operation (by using the 32-bit Wn
>    or
>    > > 64-bit Xn register specifier).
>    >
>    > Nice catch. It's only been there for... Oh wait! ;-)
>    >
>    > >
>    > > As it turns out, the architecture provides this decoding
>    information in
>    > > the SF ("Sixty-Four" -- how cute...) bit.
>    > >
>    > > Let's take advantage of this with the usual 32-bit/64-bit header
>    file
>    > > dance and do the right thing on AArch64 hosts.
>    > >
>    > > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
>    >
>    > Cc: stable?
>    >
>    Yes, good idea.
>    > > ---
>    > >  arch/arm/include/asm/kvm_emulate.h   | 5 +++++
>    > >  arch/arm/include/asm/kvm_mmio.h      | 2 ++
>    > >  arch/arm64/include/asm/kvm_emulate.h | 5 +++++
>    > >  arch/arm64/include/asm/kvm_mmio.h    | 6 ++----
>    > >  virt/kvm/arm/mmio.c                  | 8 +++++++-
>    > >  5 files changed, 21 insertions(+), 5 deletions(-)
>    > >
>    > > diff --git a/arch/arm/include/asm/kvm_emulate.h
>    > > b/arch/arm/include/asm/kvm_emulate.h
>    > > index 9b118516d2db..fe55d8737a11 100644
>    > > --- a/arch/arm/include/asm/kvm_emulate.h
>    > > +++ b/arch/arm/include/asm/kvm_emulate.h
>    > > @@ -182,6 +182,11 @@ static inline bool kvm_vcpu_dabt_issext(struct
>    > > kvm_vcpu *vcpu)
>    > >      return kvm_vcpu_get_hsr(vcpu) & HSR_SSE;
>    > >  }
>    > >
>    > > +static inline bool kvm_vcpu_dabt_issf(const struct kvm_vcpu *vcpu)
>    > > +{
>    > > +   return false;
>    > > +}
>    > > +
>    > >  static inline int kvm_vcpu_dabt_get_rd(struct kvm_vcpu *vcpu)
>    > >  {
>    > >      return (kvm_vcpu_get_hsr(vcpu) & HSR_SRT_MASK) >>
>    HSR_SRT_SHIFT;
>    > > diff --git a/arch/arm/include/asm/kvm_mmio.h
>    > > b/arch/arm/include/asm/kvm_mmio.h
>    > > index 7c0eddb0adb2..32fbf82e3ebc 100644
>    > > --- a/arch/arm/include/asm/kvm_mmio.h
>    > > +++ b/arch/arm/include/asm/kvm_mmio.h
>    > > @@ -14,6 +14,8 @@
>    > >  struct kvm_decode {
>    > >      unsigned long rt;
>    > >      bool sign_extend;
>    > > +   /* Not used on 32-bit arm */
>    > > +   bool sixty_four;
>    > >  };
>    > >
>    > >  void kvm_mmio_write_buf(void *buf, unsigned int len, unsigned long
>    > > data);
>    > > diff --git a/arch/arm64/include/asm/kvm_emulate.h
>    > > b/arch/arm64/include/asm/kvm_emulate.h
>    > > index 5efe5ca8fecf..f407b6bdad2e 100644
>    > > --- a/arch/arm64/include/asm/kvm_emulate.h
>    > > +++ b/arch/arm64/include/asm/kvm_emulate.h
>    > > @@ -283,6 +283,11 @@ static inline bool kvm_vcpu_dabt_issext(const
>    > > struct kvm_vcpu *vcpu)
>    > >      return !!(kvm_vcpu_get_hsr(vcpu) & ESR_ELx_SSE);
>    > >  }
>    > >
>    > > +static inline bool kvm_vcpu_dabt_issf(const struct kvm_vcpu *vcpu)
>    > > +{
>    > > +   return !!(kvm_vcpu_get_hsr(vcpu) & ESR_ELx_SF);
>    > > +}
>    > > +
>    > >  static inline int kvm_vcpu_dabt_get_rd(const struct kvm_vcpu
>    *vcpu)
>    > >  {
>    > >      return (kvm_vcpu_get_hsr(vcpu) & ESR_ELx_SRT_MASK) >>
>    > > ESR_ELx_SRT_SHIFT;
>    > > diff --git a/arch/arm64/include/asm/kvm_mmio.h
>    > > b/arch/arm64/include/asm/kvm_mmio.h
>    > > index 02b5c48fd467..b204501a0c39 100644
>    > > --- a/arch/arm64/include/asm/kvm_mmio.h
>    > > +++ b/arch/arm64/include/asm/kvm_mmio.h
>    > > @@ -10,13 +10,11 @@
>    > >  #include <linux/kvm_host.h>
>    > >  #include <asm/kvm_arm.h>
>    > >
>    > > -/*
>    > > - * This is annoying. The mmio code requires this, even if we don't
>    > > - * need any decoding. To be fixed.
>    > > - */
>    > >  struct kvm_decode {
>    > >      unsigned long rt;
>    > >      bool sign_extend;
>    > > +   /* Witdth of the register accessed by the faulting instruction
>    is
>    > > 64-bits */
>    > > +   bool sixty_four;
>    > >  };
>    > >
>    > >  void kvm_mmio_write_buf(void *buf, unsigned int len, unsigned long
>    > > data);
>    > > diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c
>    > > index 70d3b449692c..e62454b2e529 100644
>    > > --- a/virt/kvm/arm/mmio.c
>    > > +++ b/virt/kvm/arm/mmio.c
>    > > @@ -83,7 +83,7 @@ unsigned long kvm_mmio_read_buf(const void *buf,
>    > > unsigned int len)
>    > >  int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run
>    *run)
>    > >  {
>    > >      unsigned long data;
>    > > -   unsigned int len;
>    > > +   unsigned int len, regsize;
>    >
>    > Unused variable?
>    >
>    Ah, yes, whoops.  Guess which unstaged change I still have in my
>    tree...
>    > >      int mask;
>    > >
>    > >      /* Detect an already handled MMIO return */
>    > > @@ -105,6 +105,9 @@ int kvm_handle_mmio_return(struct kvm_vcpu
>    *vcpu,
>    > > struct kvm_run *run)
>    > >                      data = (data ^ mask) - mask;
>    > >              }
>    > >
>    > > +           if (!vcpu->arch.mmio_decode.sixty_four)
>    > > +                   data = data & 0xffffffff;
>    > > +
>    > >              trace_kvm_mmio(KVM_TRACE_MMIO_READ, len,
>    run->mmio.phys_addr,
>    > >                             &data);
>    > >              data = vcpu_data_host_to_guest(vcpu, data, len);
>    > > @@ -125,6 +128,7 @@ static int decode_hsr(struct kvm_vcpu *vcpu,
>    bool
>    > > *is_write, int *len)
>    > >      unsigned long rt;
>    > >      int access_size;
>    > >      bool sign_extend;
>    > > +   bool sixty_four;
>    > >
>    > >      if (kvm_vcpu_dabt_iss1tw(vcpu)) {
>    > >              /* page table accesses IO mem: tell guest to fix its
>    TTBR */
>    > > @@ -138,11 +142,13 @@ static int decode_hsr(struct kvm_vcpu *vcpu,
>    > > bool *is_write, int *len)
>    > >
>    > >      *is_write = kvm_vcpu_dabt_iswrite(vcpu);
>    > >      sign_extend = kvm_vcpu_dabt_issext(vcpu);
>    > > +   sixty_four = kvm_vcpu_dabt_issf(vcpu);
>    > >      rt = kvm_vcpu_dabt_get_rd(vcpu);
>    > >
>    > >      *len = access_size;
>    > >      vcpu->arch.mmio_decode.sign_extend = sign_extend;
>    > >      vcpu->arch.mmio_decode.rt = rt;
>    > > +   vcpu->arch.mmio_decode.sixty_four = sixty_four;
>    > >
>    > >      return 0;
>    > >  }
>    >
>    > I can't remember why we keep this mmio_decode structure as part of
>    > the vcpu. It isn't like it is going to change anytime soon (userspace
>    > cannot change the saved ESR_EL2)...
>    I think that was just an uninformed design decision on my part and it
>    could be reworked to operate on the ESR_EL2 directly or just take the
>    information from userspace (which we already rely on for read vs.
>    write).
>    >
>    > Anyway, your patch is in keeping with the current shape of the code.
>    > If you're OK with, it, I'll apply it with the above nits addressed.
>    >
>    Absolutely, I decided not to rework the mmio_decode stuff, and leave
>    that for some later day.
>    Thanks!
>        Christoffer

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-12-13 22:24 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-12 19:50 [PATCH] KVM: arm64: Only sign-extend MMIO up to register width Christoffer Dall
2019-12-12 19:50 ` Christoffer Dall
2019-12-13 10:12 ` Marc Zyngier
2019-12-13 10:12   ` Marc Zyngier
2019-12-13 10:56   ` Christoffer Dall
2019-12-13 10:56     ` Christoffer Dall
2019-12-13 14:05     ` Djordje Kovacevic
2019-12-13 14:24       ` Christoffer Dall
2019-12-13 14:24         ` Christoffer Dall

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.