On Fri, Oct 11, 2013 at 03:35:01PM +0100, 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.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
> ---
> arch/arm/include/asm/kvm_emulate.h | 41 ++++++++++++++++++++
> arch/arm/kvm/mmio.c | 74 ++++++++++++++++++++++++++++++------
> arch/arm64/include/asm/kvm_emulate.h | 48 +++++++++++++++++++++++
> 3 files changed, 152 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;
> + 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..5f65c95 100644
> --- a/arch/arm/kvm/mmio.c
> +++ b/arch/arm/kvm/mmio.c
> @@ -33,28 +33,49 @@
> */
> int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
> {
> - unsigned long *dest;
> unsigned int len;
> int mask;
>
> if (!run->mmio.is_write) {
> - dest = vcpu_reg(vcpu, vcpu->arch.mmio_decode.rt);
> - *dest = 0;
> + unsigned long data = 0;
>
> len = run->mmio.len;
> if (len > sizeof(unsigned long))
> return -EINVAL;
>
> - memcpy(dest, run->mmio.data, len);
> -
> - trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr,
> - *((u64 *)run->mmio.data));
> + switch (run->mmio.len) {
> + case 1:
> + data = run->mmio.data[0];
> + break;
> + case 2: {
> + u16 hword;
> + memcpy(&hword, run->mmio.data, len);
> + data = hword;
> + break;
> + }
> + case 4: {
> + u32 word;
> + memcpy(&word, run->mmio.data, len);
> + data = word;
> + break;
> + }
> + case 8: {
> + u64 dword;
> + memcpy(&dword, run->mmio.data, len);
> + data = dword;
> + break;
> + }
> + }
>
> if (vcpu->arch.mmio_decode.sign_extend &&
> len < sizeof(unsigned long)) {
> mask = 1U << ((len * 8) - 1);
> - *dest = (*dest ^ mask) - mask;
> + data = (data ^ mask) - mask;
> }
> +
> + trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr,
> + data);
> + *vcpu_reg(vcpu, vcpu->arch.mmio_decode.rt) = vcpu_data_host_to_guest(vcpu, data, len);
argh, very long line, can you reuse the "dest" variable and make it two
lines?
> }
>
> return 0;
> @@ -105,6 +126,7 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
> phys_addr_t fault_ipa)
> {
> struct kvm_exit_mmio mmio;
> + unsigned long data;
> unsigned long rt;
> int ret;
>
> @@ -125,13 +147,43 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
> }
>
> rt = vcpu->arch.mmio_decode.rt;
> + data = vcpu_data_guest_to_host(vcpu, *vcpu_reg(vcpu, rt), mmio.len);
> +
> trace_kvm_mmio((mmio.is_write) ? KVM_TRACE_MMIO_WRITE :
> KVM_TRACE_MMIO_READ_UNSATISFIED,
> mmio.len, fault_ipa,
> - (mmio.is_write) ? *vcpu_reg(vcpu, rt) : 0);
> + (mmio.is_write) ? data : 0);
> +
> + if (mmio.is_write) {
> + 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;
> + }
>
can we factor this out to a separate function like mmio_write_data_fill
or something?
> - if (mmio.is_write)
> - memcpy(mmio.data, vcpu_reg(vcpu, rt), mmio.len);
> + memcpy(mmio.data, datap, mmio.len);
> + }
>
> if (vgic_handle_mmio(vcpu, run, &mmio))
> return 1;
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index eec0738..3a7d058 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -177,4 +177,52 @@ static inline u8 kvm_vcpu_trap_get_fault(const struct kvm_vcpu *vcpu)
> return kvm_vcpu_get_hsr(vcpu) & ESR_EL2_FSC_TYPE;
> }
>
> +static inline bool kvm_vcpu_is_be(struct kvm_vcpu *vcpu)
> +{
> + if (vcpu_mode_is_32bit(vcpu))
> + return !!(*vcpu_cpsr(vcpu) & COMPAT_PSR_E_BIT);
> +
> + return !!(vcpu_sys_reg(vcpu, SCTLR_EL1) & (1 << 25));
> +}
> +
> +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;
> + case 2:
> + return be16_to_cpu(data & 0xffff);
> + case 4:
> + return be32_to_cpu(data & ((1UL << 32) - 1));
> + default:
> + return be64_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);
> + case 4:
> + return cpu_to_be32(data & ((1UL << 32) - 1));
> + default:
> + return cpu_to_be64(data);
> + }
> + }
> +
> + return data; /* Leave LE untouched */
> +}
> +
> #endif /* __ARM64_KVM_EMULATE_H__ */
> --
> 1.8.2.3
>
>
Otherwise it looks pretty good,
-Christoffer
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
