Re: [PATCH] KVM: arm64: Only sign-extend MMIO up to register width

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxx>
Sent: 13 December 2019 10:56
To: Marc Zyngier <maz@xxxxxxxxxx>
Cc: kvmarm@xxxxxxxxxxxxxxxxxxxxx <kvmarm@xxxxxxxxxxxxxxxxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx <linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>; Djordje Kovacevic <Djordje.Kovacevic@xxxxxxx>; James Morse <James.Morse@xxxxxxx>; Julien Thierry <julien.thierry.kdev@xxxxxxxxx>; Suzuki Poulose <Suzuki.Poulose@xxxxxxx>
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@xxxxxxx>
>
> 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 = "" ^ mask) - mask;
> >              }
> >
> > +           if (!vcpu->arch.mmio_decode.sixty_four)
> > +                   data = "" & 0xffffffff;
> > +
> >              trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr,
> >                             &data);
> >              data = "" 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.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[Index of Archives]     [Linux KVM]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux