On 07/11/13 17:42, Christoffer Dall wrote: > On 7 November 2013 09:18, Marc Zyngier <marc.zyngier@xxxxxxx> wrote: >> On 07/11/13 05:10, Christoffer Dall wrote: >>> On Tue, Nov 05, 2013 at 06:58:14PM +0000, Marc Zyngier wrote: >>>> Do the necessary byteswap when host and guest have different >>>> views of the universe. Actually, the only case we need to take >>>> care of is when the guest is BE. All the other cases are naturally >>>> handled. >>>> >>>> Also be careful about endianness when the data is being memcopy-ed >>>> from/to the run buffer. >>>> >>>> Cc: Christoffer Dall <christoffer.dall@xxxxxxxxxx> >>>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx> >>>> --- >>>> arch/arm/include/asm/kvm_emulate.h | 41 +++++++++++++++++ >>>> arch/arm/kvm/mmio.c | 87 +++++++++++++++++++++++++++++++----- >>>> arch/arm64/include/asm/kvm_emulate.h | 48 ++++++++++++++++++++ >>>> 3 files changed, 165 insertions(+), 11 deletions(-) >>>> >>>> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h >>>> index a464e8d..8a6be05 100644 >>>> --- a/arch/arm/include/asm/kvm_emulate.h >>>> +++ b/arch/arm/include/asm/kvm_emulate.h >>>> @@ -157,4 +157,45 @@ static inline u32 kvm_vcpu_hvc_get_imm(struct kvm_vcpu *vcpu) >>>> return kvm_vcpu_get_hsr(vcpu) & HSR_HVC_IMM_MASK; >>>> } >>>> >>>> +static inline bool kvm_vcpu_is_be(struct kvm_vcpu *vcpu) >>>> +{ >>>> + return !!(*vcpu_cpsr(vcpu) & PSR_E_BIT); >>>> +} >>>> + >>>> +static inline unsigned long vcpu_data_guest_to_host(struct kvm_vcpu *vcpu, >>>> + unsigned long data, >>>> + unsigned int len) >>>> +{ >>>> + if (kvm_vcpu_is_be(vcpu)) { >>>> + switch (len) { >>>> + case 1: >>>> + return data & 0xff; >>> >>> why are these masks necessary again? For a writeb there should be no >>> byteswapping on the guest side right? >> >> They are not strictly mandatory, but I feel a lot more confident >> trimming what we found in the register to the bare minimum. >> > > hmm, yeah, I guess a strb would trim anything above the first 8 bits > anyhow. But I believe this is done through your typed assignments of > mmio_write_data and mmio_write_read. But ok, I can live with a bit of > OCD here, as long as I know the reason. > >>>> + case 2: >>>> + return be16_to_cpu(data & 0xffff); >>>> + default: >>>> + return be32_to_cpu(data); >>>> + } >>>> + } >>>> + >>>> + return data; /* Leave LE untouched */ >>>> +} >>>> + >>>> +static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu, >>>> + unsigned long data, >>>> + unsigned int len) >>>> +{ >>>> + if (kvm_vcpu_is_be(vcpu)) { >>>> + switch (len) { >>>> + case 1: >>>> + return data & 0xff; >>>> + case 2: >>>> + return cpu_to_be16(data & 0xffff); >>>> + default: >>>> + return cpu_to_be32(data); >>>> + } >>>> + } >>>> + >>>> + return data; /* Leave LE untouched */ >>>> +} >>>> + >>>> #endif /* __ARM_KVM_EMULATE_H__ */ >>>> diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c >>>> index 0c25d94..54105f1b 100644 >>>> --- a/arch/arm/kvm/mmio.c >>>> +++ b/arch/arm/kvm/mmio.c >>>> @@ -23,6 +23,69 @@ >>>> >>>> #include "trace.h" >>>> >>>> +static void mmio_write_data(struct kvm_exit_mmio *mmio, unsigned long data) >>>> +{ >>>> + void *datap = NULL; >>>> + union { >>>> + u8 byte; >>>> + u16 hword; >>>> + u32 word; >>>> + u64 dword; >>>> + } tmp; >>>> + >>>> + switch (mmio->len) { >>>> + case 1: >>>> + tmp.byte = data; >>>> + datap = &tmp.byte; >>>> + break; >>>> + case 2: >>>> + tmp.hword = data; >>>> + datap = &tmp.hword; >>>> + break; >>>> + case 4: >>>> + tmp.word = data; >>>> + datap = &tmp.word; >>>> + break; >>>> + case 8: >>>> + tmp.dword = data; >>>> + datap = &tmp.dword; >>>> + break; >>>> + } >>>> + >>>> + memcpy(mmio->data, datap, mmio->len); >>>> +} >>>> + >>>> +static unsigned long mmio_read_data(struct kvm_run *run) >>>> +{ >>>> + unsigned long data = 0; >>>> + unsigned int len = run->mmio.len; >>>> + union { >>>> + u16 hword; >>>> + u32 word; >>>> + u64 dword; >>>> + } tmp; >>>> + >>>> + switch (len) { >>>> + case 1: >>>> + data = run->mmio.data[0]; >>>> + break; >>>> + case 2: >>>> + memcpy(&tmp.hword, run->mmio.data, len); >>>> + data = tmp.hword; >>>> + break; >>>> + case 4: >>>> + memcpy(&tmp.word, run->mmio.data, len); >>>> + data = tmp.word; >>>> + break; >>>> + case 8: >>>> + memcpy(&tmp.dword, run->mmio.data, len); >>>> + data = tmp.dword; >>>> + break; >>>> + } >>>> + >>>> + return data; >>>> +} >>>> + >>> >>> don't we have similarly named functions for the vgic where we just >>> assume accesses are always 32 bits? Could we reuse? >> >> Hmmm. The VGIC also deals with masks and stuff, and I'm not sure that's >> worth the complexity. I'll rename this for the time being, and look at >> unifying the two. >> > > If I'm not mistaken the mmio_{read,write} in the vgic code is > basically just casting the data pointer to a u32* and assigning it a > value. Yup. I can probably hand the data pointer directly (as we deal with a mixture of kvm_run and kvm_exit_mmio structs). The mask stuff is only a by-product of the length of the access, and it can be easily cleaned up. M. -- Jazz is not dead. It just smells funny... _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm