On 04/11/12 22:59, Christoffer Dall wrote:
> Prepare to factor out load/store instruction decoding by using struct
> kvm_decode and operate on a separate copy of struct pt_regs.
I quite like the approach, except for a couple of points below.
> Signed-off-by: Christoffer Dall <c.dall@xxxxxxxxxxxxxxxxxxxxxx>
> ---
> arch/arm/include/asm/kvm_host.h | 12 ++++--
> arch/arm/kvm/emulate.c | 84 ++++++++++++++++++++++++---------------
> arch/arm/kvm/mmio.c | 22 +++++-----
> 3 files changed, 70 insertions(+), 48 deletions(-)
>
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 6c1c6fc..439deb0 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -78,6 +78,13 @@ struct kvm_mmu_memory_cache {
> void *objects[KVM_NR_MEM_OBJS];
> };
>
> +struct kvm_decode {
> + struct pt_regs *regs;
> + unsigned long hxfar;
Can we call this field "far" instead? We don't need to know it codes
from the hypervisor, and it will always be data.
> + unsigned long rt; /* destination register for loads */
> + bool sign_extend; /* for byte/halfword loads */
> +};
> +
> struct kvm_vcpu_arch {
> struct kvm_regs regs;
>
> @@ -115,10 +122,7 @@ struct kvm_vcpu_arch {
> bool pause;
>
> /* IO related fields */
> - struct {
> - bool sign_extend; /* for byte/halfword loads */
> - u32 rd;
> - } mmio;
> + struct kvm_decode mmio_decode;
>
> /* Interrupt related fields */
> u32 irq_lines; /* IRQ and FIQ levels */
> diff --git a/arch/arm/kvm/emulate.c b/arch/arm/kvm/emulate.c
> index ca72be9..7cbdc68 100644
> --- a/arch/arm/kvm/emulate.c
> +++ b/arch/arm/kvm/emulate.c
> @@ -304,7 +304,7 @@ struct arm_instr {
> bool sign_extend;
> bool w;
>
> - bool (*decode)(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
> + bool (*decode)(struct kvm_decode *decode, struct kvm_exit_mmio *mmio,
> unsigned long instr, struct arm_instr *ai);
> };
>
> @@ -380,7 +380,7 @@ u32 shift(u32 value, u8 N, enum SRType type, u8 amount, bool carry_in)
> return value & mask;
> }
>
> -static bool decode_arm_wb(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
> +static bool decode_arm_wb(struct kvm_decode *decode, struct kvm_exit_mmio *mmio,
> unsigned long instr, const struct arm_instr *ai)
> {
> u8 Rt = (instr >> 12) & 0xf;
> @@ -388,7 +388,7 @@ static bool decode_arm_wb(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
> u8 W = (instr >> 21) & 1;
> u8 U = (instr >> 23) & 1;
> u8 P = (instr >> 24) & 1;
> - u32 base_addr = *vcpu_reg(vcpu, Rn);
> + u32 base_addr = decode->regs->uregs[Rn];
Can we please have accessors in some include file? At lease for stuff
that is architecture specific? Something like:
static inline unsigned long *kvm_decode_reg(struct kvm_decode *decode,
int reg)
{
return &decode->regs->uregs[reg];
}
so it is broadly equivalent to vcpu_reg? This would save me a lot of
hassle for arm64.
> u32 offset_addr, offset;
>
> /*
> @@ -403,14 +403,14 @@ static bool decode_arm_wb(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
> return false;
> }
>
> - vcpu->arch.mmio.rd = Rt;
> + decode->rt = Rt;
>
> if (ai->register_form) {
> /* Register operation */
> enum SRType s_type;
> u8 shift_n;
> - bool c_bit = *vcpu_cpsr(vcpu) & PSR_C_BIT;
> - u32 s_reg = *vcpu_reg(vcpu, ai->Rm);
> + bool c_bit = decode->regs->ARM_cpsr & PSR_C_BIT;
static inline unsigned long kvm_decode_cpsr(struct kvm_decode *decode)
{
return decode->regs->ARM_cpsr;
}
> + u32 s_reg = decode->regs->uregs[ai->Rm];
>
> s_type = decode_imm_shift(ai->type, ai->shift_n, &shift_n);
> offset = shift(s_reg, 5, s_type, shift_n, c_bit);
> @@ -424,18 +424,18 @@ static bool decode_arm_wb(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
> offset_addr = base_addr + offset;
> else
> offset_addr = base_addr - offset;
> - *vcpu_reg(vcpu, Rn) = offset_addr;
> + decode->regs->uregs[Rn] = offset_addr;
> return true;
> }
>
> -static bool decode_arm_ls(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
> +static bool decode_arm_ls(struct kvm_decode *decode, struct kvm_exit_mmio *mmio,
> unsigned long instr, struct arm_instr *ai)
> {
> u8 A = (instr >> 25) & 1;
>
> mmio->is_write = ai->w;
> mmio->len = ai->len;
> - vcpu->arch.mmio.sign_extend = false;
> + decode->sign_extend = false;
>
> ai->register_form = A;
> ai->imm = instr & 0xfff;
> @@ -443,15 +443,16 @@ static bool decode_arm_ls(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
> ai->type = (instr >> 5) & 0x3;
> ai->shift_n = (instr >> 7) & 0x1f;
>
> - return decode_arm_wb(vcpu, mmio, instr, ai);
> + return decode_arm_wb(decode, mmio, instr, ai);
> }
>
> -static bool decode_arm_extra(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
> +static bool decode_arm_extra(struct kvm_decode *decode,
> + struct kvm_exit_mmio *mmio,
> unsigned long instr, struct arm_instr *ai)
> {
> mmio->is_write = ai->w;
> mmio->len = ai->len;
> - vcpu->arch.mmio.sign_extend = ai->sign_extend;
> + decode->sign_extend = ai->sign_extend;
>
> ai->register_form = !((instr >> 22) & 1);
> ai->imm = ((instr >> 4) & 0xf0) | (instr & 0xf);
> @@ -459,7 +460,7 @@ static bool decode_arm_extra(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
> ai->type = 0; /* SRType_LSL */
> ai->shift_n = 0;
>
> - return decode_arm_wb(vcpu, mmio, instr, ai);
> + return decode_arm_wb(decode, mmio, instr, ai);
> }
>
> /*
> @@ -526,7 +527,7 @@ static const struct arm_instr arm_instr[] = {
> .sign_extend = true , .decode = decode_arm_extra },
> };
>
> -static bool kvm_decode_arm_ls(struct kvm_vcpu *vcpu, unsigned long instr,
> +static bool kvm_decode_arm_ls(struct kvm_decode *decode, unsigned long instr,
> struct kvm_exit_mmio *mmio)
> {
> int i;
> @@ -535,7 +536,7 @@ static bool kvm_decode_arm_ls(struct kvm_vcpu *vcpu, unsigned long instr,
> const struct arm_instr *ai = &arm_instr[i];
> if ((instr & ai->opc_mask) == ai->opc) {
> struct arm_instr ai_copy = *ai;
> - return ai->decode(vcpu, mmio, instr, &ai_copy);
> + return ai->decode(decode, mmio, instr, &ai_copy);
> }
> }
> return false;
> @@ -557,40 +558,42 @@ struct thumb_instr {
> } t32;
> };
>
> - bool (*decode)(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
> + bool (*decode)(struct kvm_decode *decode, struct kvm_exit_mmio *mmio,
> unsigned long instr, const struct thumb_instr *ti);
> };
>
> -static bool decode_thumb_wb(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
> +static bool decode_thumb_wb(struct kvm_decode *decode,
> + struct kvm_exit_mmio *mmio,
> unsigned long instr)
> {
> bool P = (instr >> 10) & 1;
> bool U = (instr >> 9) & 1;
> u8 imm8 = instr & 0xff;
> - u32 offset_addr = vcpu->arch.hxfar;
> + u32 offset_addr = decode->hxfar;
> u8 Rn = (instr >> 16) & 0xf;
>
> - vcpu->arch.mmio.rd = (instr >> 12) & 0xf;
> + decode->rt = (instr >> 12) & 0xf;
>
> - if (kvm_vcpu_reg_is_pc(vcpu, Rn))
> + if (Rn == 15)
> return false;
>
> /* Handle Writeback */
> if (!P && U)
> - *vcpu_reg(vcpu, Rn) = offset_addr + imm8;
> + decode->regs->uregs[Rn] = offset_addr + imm8;
> else if (!P && !U)
> - *vcpu_reg(vcpu, Rn) = offset_addr - imm8;
> + decode->regs->uregs[Rn] = offset_addr - imm8;
> return true;
> }
>
> -static bool decode_thumb_str(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
> +static bool decode_thumb_str(struct kvm_decode *decode,
> + struct kvm_exit_mmio *mmio,
> unsigned long instr, const struct thumb_instr *ti)
> {
> u8 op1 = (instr >> (16 + 5)) & 0x7;
> u8 op2 = (instr >> 6) & 0x3f;
>
> mmio->is_write = true;
> - vcpu->arch.mmio.sign_extend = false;
> + decode->sign_extend = false;
>
> switch (op1) {
> case 0x0: mmio->len = 1; break;
> @@ -602,13 +605,14 @@ static bool decode_thumb_str(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
>
> if ((op2 & 0x24) == 0x24) {
> /* STRB (immediate, thumb, W=1) */
> - return decode_thumb_wb(vcpu, mmio, instr);
> + return decode_thumb_wb(decode, mmio, instr);
> }
>
> return false;
> }
>
> -static bool decode_thumb_ldr(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
> +static bool decode_thumb_ldr(struct kvm_decode *decode,
> + struct kvm_exit_mmio *mmio,
> unsigned long instr, const struct thumb_instr *ti)
> {
> u8 op1 = (instr >> (16 + 7)) & 0x3;
> @@ -623,15 +627,15 @@ static bool decode_thumb_ldr(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
> }
>
> if (op1 == 0x0)
> - vcpu->arch.mmio.sign_extend = false;
> + decode->sign_extend = false;
> else if (op1 == 0x2 && (ti->t32.op2 & 0x7) != 0x5)
> - vcpu->arch.mmio.sign_extend = true;
> + decode->sign_extend = true;
> else
> return false; /* Only register write-back versions! */
>
> if ((op2 & 0x24) == 0x24) {
> /* LDR{S}X (immediate, thumb, W=1) */
> - return decode_thumb_wb(vcpu, mmio, instr);
> + return decode_thumb_wb(decode, mmio, instr);
> }
>
> return false;
> @@ -663,7 +667,7 @@ static const struct thumb_instr thumb_instr[] = {
>
>
>
> -static bool kvm_decode_thumb_ls(struct kvm_vcpu *vcpu, unsigned long instr,
> +static bool kvm_decode_thumb_ls(struct kvm_decode *decode, unsigned long instr,
> struct kvm_exit_mmio *mmio)
> {
> bool is32 = is_wide_instruction(instr);
> @@ -693,7 +697,7 @@ static bool kvm_decode_thumb_ls(struct kvm_vcpu *vcpu, unsigned long instr,
> continue;
> }
>
> - return ti->decode(vcpu, mmio, instr, &tinstr);
> + return ti->decode(decode, mmio, instr, &tinstr);
> }
>
> return false;
> @@ -720,6 +724,8 @@ int kvm_emulate_mmio_ls(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> {
> bool is_thumb;
> unsigned long instr = 0;
> + struct pt_regs current_regs;
> + struct kvm_decode *decode = &vcpu->arch.mmio_decode;
>
> trace_kvm_mmio_emulate(*vcpu_pc(vcpu), instr, *vcpu_cpsr(vcpu));
>
> @@ -728,14 +734,22 @@ int kvm_emulate_mmio_ls(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> return 1;
>
> mmio->phys_addr = fault_ipa;
> +
> + memcpy(¤t_regs, &vcpu->arch.regs.usr_regs, sizeof(current_regs));
> + current_regs.ARM_sp = *vcpu_reg(vcpu, 13);
> + current_regs.ARM_lr = *vcpu_reg(vcpu, 14);
> +
> + decode->regs = ¤t_regs;
> + decode->hxfar = vcpu->arch.hxfar;
> +
> is_thumb = !!(*vcpu_cpsr(vcpu) & PSR_T_BIT);
> - if (!is_thumb && !kvm_decode_arm_ls(vcpu, instr, mmio)) {
> + if (!is_thumb && !kvm_decode_arm_ls(decode, instr, mmio)) {
> kvm_debug("Unable to decode inst: %#08lx (cpsr: %#08x (T=0)"
> "pc: %#08x)\n",
> instr, *vcpu_cpsr(vcpu), *vcpu_pc(vcpu));
> kvm_inject_dabt(vcpu, vcpu->arch.hxfar);
> return 1;
> - } else if (is_thumb && !kvm_decode_thumb_ls(vcpu, instr, mmio)) {
> + } else if (is_thumb && !kvm_decode_thumb_ls(decode, instr, mmio)) {
> kvm_debug("Unable to decode inst: %#08lx (cpsr: %#08x (T=1)"
> "pc: %#08x)\n",
> instr, *vcpu_cpsr(vcpu), *vcpu_pc(vcpu));
> @@ -743,6 +757,10 @@ int kvm_emulate_mmio_ls(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> return 1;
> }
>
> + memcpy(&vcpu->arch.regs.usr_regs, ¤t_regs, sizeof(current_regs));
> + *vcpu_reg(vcpu, 13) = current_regs.ARM_sp;
> + *vcpu_reg(vcpu, 14) = current_regs.ARM_lr;
> +
> /*
> * The MMIO instruction is emulated and should not be re-executed
> * in the guest.
> diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
> index beb7134..eefbead 100644
> --- a/arch/arm/kvm/mmio.c
> +++ b/arch/arm/kvm/mmio.c
> @@ -37,7 +37,7 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
> int mask;
>
> if (!run->mmio.is_write) {
> - dest = vcpu_reg(vcpu, vcpu->arch.mmio.rd);
> + dest = vcpu_reg(vcpu, vcpu->arch.mmio_decode.rt);
> memset(dest, 0, sizeof(int));
>
> len = run->mmio.len;
> @@ -49,7 +49,7 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
> trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr,
> *((u64 *)run->mmio.data));
>
> - if (vcpu->arch.mmio.sign_extend && len < 4) {
> + if (vcpu->arch.mmio_decode.sign_extend && len < 4) {
> mask = 1U << ((len * 8) - 1);
> *dest = (*dest ^ mask) - mask;
> }
> @@ -61,7 +61,7 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
> static int decode_hsr(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> struct kvm_exit_mmio *mmio)
> {
> - unsigned long rd, len;
> + unsigned long rt, len;
> bool is_write, sign_extend;
>
> if ((vcpu->arch.hsr >> 8) & 1) {
> @@ -93,9 +93,9 @@ static int decode_hsr(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>
> is_write = vcpu->arch.hsr & HSR_WNR;
> sign_extend = vcpu->arch.hsr & HSR_SSE;
> - rd = (vcpu->arch.hsr & HSR_SRT_MASK) >> HSR_SRT_SHIFT;
> + rt = (vcpu->arch.hsr & HSR_SRT_MASK) >> HSR_SRT_SHIFT;
>
> - if (kvm_vcpu_reg_is_pc(vcpu, rd)) {
> + if (kvm_vcpu_reg_is_pc(vcpu, rt)) {
> /* IO memory trying to read/write pc */
> kvm_inject_pabt(vcpu, vcpu->arch.hxfar);
> return 1;
> @@ -104,8 +104,8 @@ static int decode_hsr(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> mmio->is_write = is_write;
> mmio->phys_addr = fault_ipa;
> mmio->len = len;
> - vcpu->arch.mmio.sign_extend = sign_extend;
> - vcpu->arch.mmio.rd = rd;
> + vcpu->arch.mmio_decode.sign_extend = sign_extend;
> + vcpu->arch.mmio_decode.rt = rt;
>
> /*
> * The MMIO instruction is emulated and should not be re-executed
> @@ -119,7 +119,7 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
> phys_addr_t fault_ipa, struct kvm_memory_slot *memslot)
> {
> struct kvm_exit_mmio mmio;
> - unsigned long rd;
> + unsigned long rt;
> int ret;
>
> /*
> @@ -137,14 +137,14 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
> if (ret != 0)
> return ret;
>
> - rd = vcpu->arch.mmio.rd;
> + rt = vcpu->arch.mmio_decode.rt;
> 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, rd) : 0);
> + (mmio.is_write) ? *vcpu_reg(vcpu, rt) : 0);
>
> if (mmio.is_write)
> - memcpy(mmio.data, vcpu_reg(vcpu, rd), mmio.len);
> + memcpy(mmio.data, vcpu_reg(vcpu, rt), mmio.len);
>
> if (vgic_handle_mmio(vcpu, run, &mmio))
> return 1;
Looks OK otherwise.
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
