Re: [PATCH v2 05/30] KVM: Provide callback to get/set control registers in emulator ops.

[Andre Przywara <andre.przywara@amd.com>]

Gleb Natapov wrote:
> Use this callback instead of directly call kvm function. Also rename
> realmode_(set|get)_cr to emulator_(set|get)_cr since function has nothing
> to do with real mode.
Do you mind removing the static before emulator_{set,get}_cr and marking 
it EXPORT_SYMBOL? Then one could use it in vmx.c (and soon in svm.c ;-) 
while handling MOV-CR intercepts. Currently most of the code is actually 
duplicated.

Also, shouldn't mk_cr_64() not be called mask_cr_64() for better 
readability?

Regards,
Andre.

> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> ---
>  arch/x86/include/asm/kvm_emulate.h |    3 +-
>  arch/x86/include/asm/kvm_host.h    |    2 -
>  arch/x86/kvm/emulate.c             |    7 +-
>  arch/x86/kvm/x86.c                 |  114 ++++++++++++++++++------------------
>  4 files changed, 63 insertions(+), 63 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
> index 2666d7a..0c5caa4 100644
> --- a/arch/x86/include/asm/kvm_emulate.h
> +++ b/arch/x86/include/asm/kvm_emulate.h
> @@ -108,7 +108,8 @@ struct x86_emulate_ops {
>  				const void *new,
>  				unsigned int bytes,
>  				struct kvm_vcpu *vcpu);
> -
> +	ulong (*get_cr)(int cr, struct kvm_vcpu *vcpu);
> +	void (*set_cr)(int cr, ulong val, struct kvm_vcpu *vcpu);
>  };
>  
>  /* Type, address-of, and value of an instruction's operand. */
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 3b178d8..e8e108a 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -585,8 +585,6 @@ void realmode_lidt(struct kvm_vcpu *vcpu, u16 size, unsigned long address);
>  void realmode_lmsw(struct kvm_vcpu *vcpu, unsigned long msw,
>  		   unsigned long *rflags);
>  
> -unsigned long realmode_get_cr(struct kvm_vcpu *vcpu, int cr);
> -void realmode_set_cr(struct kvm_vcpu *vcpu, int cr, unsigned long value);
>  void kvm_enable_efer_bits(u64);
>  int kvm_get_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *data);
>  int kvm_set_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data);
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 91450b5..5b060e4 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -2483,7 +2483,7 @@ twobyte_insn:
>  			break;
>  		case 4: /* smsw */
>  			c->dst.bytes = 2;
> -			c->dst.val = realmode_get_cr(ctxt->vcpu, 0);
> +			c->dst.val = ops->get_cr(0, ctxt->vcpu);
>  			break;
>  		case 6: /* lmsw */
>  			realmode_lmsw(ctxt->vcpu, (u16)c->src.val,
> @@ -2519,8 +2519,7 @@ twobyte_insn:
>  	case 0x20: /* mov cr, reg */
>  		if (c->modrm_mod != 3)
>  			goto cannot_emulate;
> -		c->regs[c->modrm_rm] =
> -				realmode_get_cr(ctxt->vcpu, c->modrm_reg);
> +		c->regs[c->modrm_rm] = ops->get_cr(c->modrm_reg, ctxt->vcpu);
>  		c->dst.type = OP_NONE;	/* no writeback */
>  		break;
>  	case 0x21: /* mov from dr to reg */
> @@ -2534,7 +2533,7 @@ twobyte_insn:
>  	case 0x22: /* mov reg, cr */
>  		if (c->modrm_mod != 3)
>  			goto cannot_emulate;
> -		realmode_set_cr(ctxt->vcpu, c->modrm_reg, c->modrm_val);
> +		ops->set_cr(c->modrm_reg, c->modrm_val, ctxt->vcpu);
>  		c->dst.type = OP_NONE;
>  		break;
>  	case 0x23: /* mov from reg to dr */
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index a1e671a..bf714df 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3370,12 +3370,70 @@ void kvm_report_emulation_failure(struct kvm_vcpu *vcpu, const char *context)
>  }
>  EXPORT_SYMBOL_GPL(kvm_report_emulation_failure);
>  
> +static u64 mk_cr_64(u64 curr_cr, u32 new_val)
> +{
> +	return (curr_cr & ~((1ULL << 32) - 1)) | new_val;
> +}
> +
> +static unsigned long emulator_get_cr(int cr, struct kvm_vcpu *vcpu)
> +{
> +	unsigned long value;
> +
> +	switch (cr) {
> +	case 0:
> +		value = kvm_read_cr0(vcpu);
> +		break;
> +	case 2:
> +		value = vcpu->arch.cr2;
> +		break;
> +	case 3:
> +		value = vcpu->arch.cr3;
> +		break;
> +	case 4:
> +		value = kvm_read_cr4(vcpu);
> +		break;
> +	case 8:
> +		value = kvm_get_cr8(vcpu);
> +		break;
> +	default:
> +		vcpu_printf(vcpu, "%s: unexpected cr %u\n", __func__, cr);
> +		return 0;
> +	}
> +
> +	return value;
> +}
> +
> +static void emulator_set_cr(int cr, unsigned long val, struct kvm_vcpu *vcpu)
> +{
> +	switch (cr) {
> +	case 0:
> +		kvm_set_cr0(vcpu, mk_cr_64(kvm_read_cr0(vcpu), val));
> +		break;
> +	case 2:
> +		vcpu->arch.cr2 = val;
> +		break;
> +	case 3:
> +		kvm_set_cr3(vcpu, val);
> +		break;
> +	case 4:
> +		kvm_set_cr4(vcpu, mk_cr_64(kvm_read_cr4(vcpu), val));
> +		break;
> +	case 8:
> +		kvm_set_cr8(vcpu, val & 0xfUL);
> +		break;
> +	default:
> +		vcpu_printf(vcpu, "%s: unexpected cr %u\n", __func__, cr);
> +	}
> +}
> +
>  static struct x86_emulate_ops emulate_ops = {
>  	.read_std            = kvm_read_guest_virt_system,
>  	.fetch               = kvm_fetch_guest_virt,
>  	.read_emulated       = emulator_read_emulated,
>  	.write_emulated      = emulator_write_emulated,
>  	.cmpxchg_emulated    = emulator_cmpxchg_emulated,
> +	.get_cr              = emulator_get_cr,
> +	.set_cr              = emulator_set_cr,
>  };
>  
>  static void cache_all_regs(struct kvm_vcpu *vcpu)
> @@ -3973,11 +4031,6 @@ int kvm_fix_hypercall(struct kvm_vcpu *vcpu)
>  	return emulator_write_emulated(rip, instruction, 3, vcpu);
>  }
>  
> -static u64 mk_cr_64(u64 curr_cr, u32 new_val)
> -{
> -	return (curr_cr & ~((1ULL << 32) - 1)) | new_val;
> -}
> -
>  void realmode_lgdt(struct kvm_vcpu *vcpu, u16 limit, unsigned long base)
>  {
>  	struct desc_ptr dt = { limit, base };
> @@ -3999,57 +4052,6 @@ void realmode_lmsw(struct kvm_vcpu *vcpu, unsigned long msw,
>  	*rflags = kvm_get_rflags(vcpu);
>  }
>  
> -unsigned long realmode_get_cr(struct kvm_vcpu *vcpu, int cr)
> -{
> -	unsigned long value;
> -
> -	switch (cr) {
> -	case 0:
> -		value = kvm_read_cr0(vcpu);
> -		break;
> -	case 2:
> -		value = vcpu->arch.cr2;
> -		break;
> -	case 3:
> -		value = vcpu->arch.cr3;
> -		break;
> -	case 4:
> -		value = kvm_read_cr4(vcpu);
> -		break;
> -	case 8:
> -		value = kvm_get_cr8(vcpu);
> -		break;
> -	default:
> -		vcpu_printf(vcpu, "%s: unexpected cr %u\n", __func__, cr);
> -		return 0;
> -	}
> -
> -	return value;
> -}
> -
> -void realmode_set_cr(struct kvm_vcpu *vcpu, int cr, unsigned long val)
> -{
> -	switch (cr) {
> -	case 0:
> -		kvm_set_cr0(vcpu, mk_cr_64(kvm_read_cr0(vcpu), val));
> -		break;
> -	case 2:
> -		vcpu->arch.cr2 = val;
> -		break;
> -	case 3:
> -		kvm_set_cr3(vcpu, val);
> -		break;
> -	case 4:
> -		kvm_set_cr4(vcpu, mk_cr_64(kvm_read_cr4(vcpu), val));
> -		break;
> -	case 8:
> -		kvm_set_cr8(vcpu, val & 0xfUL);
> -		break;
> -	default:
> -		vcpu_printf(vcpu, "%s: unexpected cr %u\n", __func__, cr);
> -	}
> -}
> -
>  static int move_to_next_stateful_cpuid_entry(struct kvm_vcpu *vcpu, int i)
>  {
>  	struct kvm_cpuid_entry2 *e = &vcpu->arch.cpuid_entries[i];


-- 
Andre Przywara
AMD-OSRC (Dresden)
Tel: x29712