On Mon, Oct 15, 2012 at 11:19 AM, Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
> The use of VCPU_*_MODE in register manipulation code is not really
> necessary, and possibly costly (table lookup doesn't come for free).
the reason why the table is there is that previous reviewers felt the
series of branches with a lot of room for misprediction wold be
harmfully slow and therefore suggested the lookup table. I'm curious,
do you feel otherwise?
>
> Just use the normal *_MODE defines and some case statements. Also,
> merge USR and SYS registers, which are architecturally indentical.
otherwise this looks ok.
I will find time to apply and test this probably at the end of the week.
(note the subject of this patch is unreasonably long)
-Christoffer
>
> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
> ---
> arch/arm/include/asm/kvm_emulate.h | 17 +-----
> arch/arm/kvm/emulate.c | 114 ++++++++++++++-----------------------
> 2 files changed, 47 insertions(+), 84 deletions(-)
>
> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
> index 1a7f3dd..6a82fcc 100644
> --- a/arch/arm/include/asm/kvm_emulate.h
> +++ b/arch/arm/include/asm/kvm_emulate.h
> @@ -22,8 +22,6 @@
> #include <linux/kvm_host.h>
> #include <asm/kvm_asm.h>
>
> -u32 *vcpu_reg_mode(struct kvm_vcpu *vcpu, u8 reg_num, u32 cpsr);
> -u32 *vcpu_spsr_mode(struct kvm_vcpu *vcpu, u32 cpsr);
> /*
> * The in-kernel MMIO emulation code wants to use a copy of run->mmio,
> * which is an anonymous type. Use our own type instead.
> @@ -45,6 +43,9 @@ static inline void kvm_prepare_mmio(struct kvm_run *run,
> run->exit_reason = KVM_EXIT_MMIO;
> }
>
> +u32 *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
> +u32 *vcpu_spsr(struct kvm_vcpu *vcpu);
> +
> int kvm_handle_wfi(struct kvm_vcpu *vcpu, struct kvm_run *run);
> int kvm_emulate_mmio_ls(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> unsigned long instr, struct kvm_exit_mmio *mmio);
> @@ -53,12 +54,6 @@ void kvm_inject_undefined(struct kvm_vcpu *vcpu);
> void kvm_inject_dabt(struct kvm_vcpu *vcpu, unsigned long addr);
> void kvm_inject_pabt(struct kvm_vcpu *vcpu, unsigned long addr);
>
> -/* Get vcpu register for current mode */
> -static inline u32 *vcpu_reg(struct kvm_vcpu *vcpu, unsigned long reg_num)
> -{
> - return vcpu_reg_mode(vcpu, reg_num, vcpu->arch.regs.usr_regs.ARM_cpsr);
> -}
> -
> static inline u32 *vcpu_pc(struct kvm_vcpu *vcpu)
> {
> return (u32 *)&vcpu->arch.regs.usr_regs.ARM_pc;
> @@ -69,12 +64,6 @@ static inline u32 *vcpu_cpsr(struct kvm_vcpu *vcpu)
> return (u32 *)&vcpu->arch.regs.usr_regs.ARM_cpsr;
> }
>
> -/* Get vcpu SPSR for current mode */
> -static inline u32 *vcpu_spsr(struct kvm_vcpu *vcpu)
> -{
> - return vcpu_spsr_mode(vcpu, vcpu->arch.regs.usr_regs.ARM_cpsr);
> -}
> -
> static inline bool mode_has_spsr(struct kvm_vcpu *vcpu)
> {
> unsigned long cpsr_mode = vcpu->arch.regs.usr_regs.ARM_cpsr & MODE_MASK;
> diff --git a/arch/arm/kvm/emulate.c b/arch/arm/kvm/emulate.c
> index de6189e..995c59f 100644
> --- a/arch/arm/kvm/emulate.c
> +++ b/arch/arm/kvm/emulate.c
> @@ -24,13 +24,23 @@
>
> #include "trace.h"
>
> -#define VCPU_NR_MODES 7
> +#define VCPU_NR_MODES 6
> #define REG_OFFSET(_reg) \
> (offsetof(struct kvm_regs, _reg) / sizeof(u32))
>
> #define USR_REG_OFFSET(_num) REG_OFFSET(usr_regs.uregs[_num])
>
> static const unsigned long vcpu_reg_offsets[VCPU_NR_MODES][16] = {
> + /* USR/SYS Registers */
> + {
> + USR_REG_OFFSET(0), USR_REG_OFFSET(1), USR_REG_OFFSET(2),
> + USR_REG_OFFSET(3), USR_REG_OFFSET(4), USR_REG_OFFSET(5),
> + USR_REG_OFFSET(6), USR_REG_OFFSET(7), USR_REG_OFFSET(8),
> + USR_REG_OFFSET(9), USR_REG_OFFSET(10), USR_REG_OFFSET(11),
> + USR_REG_OFFSET(12), USR_REG_OFFSET(13), USR_REG_OFFSET(14),
> + REG_OFFSET(usr_regs.ARM_pc)
> + },
> +
> /* FIQ Registers */
> {
> USR_REG_OFFSET(0), USR_REG_OFFSET(1), USR_REG_OFFSET(2),
> @@ -93,93 +103,57 @@ static const unsigned long vcpu_reg_offsets[VCPU_NR_MODES][16] = {
> REG_OFFSET(und_regs[1]), /* r14 */
> REG_OFFSET(usr_regs.ARM_pc)
> },
> -
> - /* USR Registers */
> - {
> - USR_REG_OFFSET(0), USR_REG_OFFSET(1), USR_REG_OFFSET(2),
> - USR_REG_OFFSET(3), USR_REG_OFFSET(4), USR_REG_OFFSET(5),
> - USR_REG_OFFSET(6), USR_REG_OFFSET(7), USR_REG_OFFSET(8),
> - USR_REG_OFFSET(9), USR_REG_OFFSET(10), USR_REG_OFFSET(11),
> - USR_REG_OFFSET(12), USR_REG_OFFSET(13), USR_REG_OFFSET(14),
> - REG_OFFSET(usr_regs.ARM_pc)
> - },
> -
> - /* SYS Registers */
> - {
> - USR_REG_OFFSET(0), USR_REG_OFFSET(1), USR_REG_OFFSET(2),
> - USR_REG_OFFSET(3), USR_REG_OFFSET(4), USR_REG_OFFSET(5),
> - USR_REG_OFFSET(6), USR_REG_OFFSET(7), USR_REG_OFFSET(8),
> - USR_REG_OFFSET(9), USR_REG_OFFSET(10), USR_REG_OFFSET(11),
> - USR_REG_OFFSET(12), USR_REG_OFFSET(13), USR_REG_OFFSET(14),
> - REG_OFFSET(usr_regs.ARM_pc)
> - },
> -};
> -
> -/*
> - * Modes used for short-hand mode determinition in the world-switch code and
> - * in emulation code.
> - *
> - * Note: These indices do NOT correspond to the value of the CPSR mode bits!
> - */
> -enum vcpu_mode {
> - VCPU_FIQ_MODE = 0,
> - VCPU_IRQ_MODE,
> - VCPU_SVC_MODE,
> - VCPU_ABT_MODE,
> - VCPU_UND_MODE,
> - VCPU_USR_MODE,
> - VCPU_SYS_MODE
> -};
> -
> -static const u8 modes_table[32] = {
> - 0xf, 0xf, 0xf, 0xf, 0xf, 0xf, 0xf, 0xf,
> - 0xf, 0xf, 0xf, 0xf, 0xf, 0xf, 0xf, 0xf,
> - VCPU_USR_MODE, /* 0x0 */
> - VCPU_FIQ_MODE, /* 0x1 */
> - VCPU_IRQ_MODE, /* 0x2 */
> - VCPU_SVC_MODE, /* 0x3 */
> - 0xf, 0xf, 0xf,
> - VCPU_ABT_MODE, /* 0x7 */
> - 0xf, 0xf, 0xf,
> - VCPU_UND_MODE, /* 0xb */
> - 0xf, 0xf, 0xf,
> - VCPU_SYS_MODE /* 0xf */
> -};
> -
> -static enum vcpu_mode vcpu_mode(u32 cpsr)
> -{
> - u8 mode = modes_table[cpsr & 0x1f];
> - BUG_ON(mode == 0xf);
> - return mode;
> };
>
> /*
> - * Return a pointer to the register number valid in the specified mode of
> + * Return a pointer to the register number valid in the current mode of
> * the virtual CPU.
> */
> -u32 *vcpu_reg_mode(struct kvm_vcpu *vcpu, u8 reg_num, u32 cpsr)
> +u32 *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num)
> {
> - unsigned long mode = vcpu_mode(cpsr);
> u32 *reg_array = (u32 *)&vcpu->arch.regs;
> + u32 mode = *vcpu_cpsr(vcpu) & MODE_MASK;
> +
> + switch (mode) {
> + case USR_MODE...SVC_MODE:
> + mode &= ~MODE32_BIT; /* 0 ... 3 */
> + break;
> +
> + case ABT_MODE:
> + mode = 4;
> + break;
> +
> + case UND_MODE:
> + mode = 5;
> + break;
> +
> + case SYSTEM_MODE:
> + mode = 0;
> + break;
> +
> + default:
> + BUG();
> + }
>
> return reg_array + vcpu_reg_offsets[mode][reg_num];
> }
>
> /*
> - * Return the SPSR for the specified mode of the virtual CPU.
> + * Return the SPSR for the current mode of the virtual CPU.
> */
> -u32 *vcpu_spsr_mode(struct kvm_vcpu *vcpu, u32 cpsr)
> +u32 *vcpu_spsr(struct kvm_vcpu *vcpu)
> {
> - switch (vcpu_mode(cpsr)) {
> - case VCPU_SVC_MODE:
> + u32 mode = *vcpu_cpsr(vcpu) & MODE_MASK;
> + switch (mode) {
> + case SVC_MODE:
> return &vcpu->arch.regs.svc_regs[2];
> - case VCPU_ABT_MODE:
> + case ABT_MODE:
> return &vcpu->arch.regs.abt_regs[2];
> - case VCPU_UND_MODE:
> + case UND_MODE:
> return &vcpu->arch.regs.und_regs[2];
> - case VCPU_IRQ_MODE:
> + case IRQ_MODE:
> return &vcpu->arch.regs.irq_regs[2];
> - case VCPU_FIQ_MODE:
> + case FIQ_MODE:
> return &vcpu->arch.regs.fiq_regs[7];
> default:
> BUG();
> --
> 1.7.12
>
>
>
> _______________________________________________
> kvmarm mailing list
> kvmarm@xxxxxxxxxxxxxxxxxxxxx
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
