Re: [PATCH 3/4] kvm, emulator: Rename VendorSpecific flag

Re: [PATCH 3/4] kvm, emulator: Rename VendorSpecific flag

[Gleb Natapov]

On Thu, Jun 20, 2013 at 11:30:45AM +0200, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> Call it EmulateOnUD which is exactly what we're trying to do with
> vendor-specific instructions.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
>  arch/x86/kvm/emulate.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index d9aa9f8772f0..7872d9871fb7 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -126,7 +126,7 @@
>  #define Mov         (1<<20)
>  /* Misc flags */
>  #define Prot        (1<<21) /* instruction generates #UD if not in prot-mode */
> -#define VendorSpecific (1<<22) /* Vendor specific instruction */
> +#define EmulateOnUD (1<<22) /* Emulate if unsupported by the host */
>  #define NoAccess    (1<<23) /* Don't access memory (lea/invlpg/verr etc) */
>  #define Op3264      (1<<24) /* Operand is 64b in long mode, 32b otherwise */
>  #define Undefined   (1<<25) /* No Such Instruction */
> @@ -3688,7 +3688,7 @@ static const struct opcode group7_rm1[] = {
>  
>  static const struct opcode group7_rm3[] = {
>  	DIP(SrcNone | Prot | Priv,		vmrun,		check_svme_pa),
> -	II(SrcNone  | Prot | VendorSpecific,	em_vmmcall,	vmmcall),
> +	II(SrcNone  | Prot | EmulateOnUD,	em_vmmcall,	vmmcall),
>  	DIP(SrcNone | Prot | Priv,		vmload,		check_svme_pa),
>  	DIP(SrcNone | Prot | Priv,		vmsave,		check_svme_pa),
>  	DIP(SrcNone | Prot | Priv,		stgi,		check_svme),
> @@ -3773,7 +3773,7 @@ static const struct group_dual group7 = { {
>  	II(SrcMem16 | Mov | Priv,		em_lmsw, lmsw),
>  	II(SrcMem | ByteOp | Priv | NoAccess,	em_invlpg, invlpg),
>  }, {
> -	I(SrcNone | Priv | VendorSpecific,	em_vmcall),
> +	I(SrcNone | Priv | EmulateOnUD,	em_vmcall),
>  	EXT(0, group7_rm1),
>  	N, EXT(0, group7_rm3),
>  	II(SrcNone | DstMem | Mov,		em_smsw, smsw), N,
> @@ -3995,7 +3995,7 @@ static const struct opcode opcode_table[256] = {
>  static const struct opcode twobyte_table[256] = {
>  	/* 0x00 - 0x0F */
>  	G(0, group6), GD(0, &group7), N, N,
> -	N, I(ImplicitOps | VendorSpecific, em_syscall),
> +	N, I(ImplicitOps | EmulateOnUD, em_syscall),
>  	II(ImplicitOps | Priv, em_clts, clts), N,
>  	DI(ImplicitOps | Priv, invd), DI(ImplicitOps | Priv, wbinvd), N, N,
>  	N, D(ImplicitOps | ModRM), N, N,
> @@ -4015,8 +4015,8 @@ static const struct opcode twobyte_table[256] = {
>  	IIP(ImplicitOps, em_rdtsc, rdtsc, check_rdtsc),
>  	II(ImplicitOps | Priv, em_rdmsr, rdmsr),
>  	IIP(ImplicitOps, em_rdpmc, rdpmc, check_rdpmc),
> -	I(ImplicitOps | VendorSpecific, em_sysenter),
> -	I(ImplicitOps | Priv | VendorSpecific, em_sysexit),
> +	I(ImplicitOps | EmulateOnUD, em_sysenter),
> +	I(ImplicitOps | Priv | EmulateOnUD, em_sysexit),
>  	N, N,
>  	N, N, N, N, N, N, N, N,
>  	/* 0x40 - 0x4F */
> @@ -4435,7 +4435,7 @@ done_prefixes:
>  	if (ctxt->d == 0 || (ctxt->d & NotImpl))
>  		return EMULATION_FAILED;
>  
> -	if (!(ctxt->d & VendorSpecific) && ctxt->only_vendor_specific_insn)
> +	if (!(ctxt->d & EmulateOnUD) && ctxt->only_vendor_specific_insn)
Lets rename only_vendor_specific_insn to something like ->ud too.

>  		return EMULATION_FAILED;
>  
>  	if (mode == X86EMUL_MODE_PROT64 && (ctxt->d & Stack))
> -- 
> 1.8.3

--
			Gleb.

Re: [PATCH 3/4] kvm, emulator: Rename VendorSpecific flag

[Borislav Petkov]

On Tue, Jun 25, 2013 at 02:10:20PM +0300, Gleb Natapov wrote:
> > -	if (!(ctxt->d & VendorSpecific) && ctxt->only_vendor_specific_insn)
> > +	if (!(ctxt->d & EmulateOnUD) && ctxt->only_vendor_specific_insn)
> Lets rename only_vendor_specific_insn to something like ->ud too.

So this thing is set only when either svm or vmx encounter an #UD and go
and emulate the instruction.

I guess this is for the case where we actually do want to inject the #UD
into the guest and not emulate the instruction.

Btw, it is only checked in x86_decode_insn so we could just as well hand
down the emulation_type from the caller x86_emulate_instruction and kill
->only_vendor_specific_insn completely like so:

	if (!(ctxt->d & EmulateOnUD) && (emul_type & EMULTYPE_TRAP_UD))

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 3/4] kvm, emulator: Rename VendorSpecific flag

[Gleb Natapov]

On Wed, Jun 26, 2013 at 04:11:59PM +0200, Borislav Petkov wrote:
> On Tue, Jun 25, 2013 at 02:10:20PM +0300, Gleb Natapov wrote:
> > > -	if (!(ctxt->d & VendorSpecific) && ctxt->only_vendor_specific_insn)
> > > +	if (!(ctxt->d & EmulateOnUD) && ctxt->only_vendor_specific_insn)
> > Lets rename only_vendor_specific_insn to something like ->ud too.
> 
> So this thing is set only when either svm or vmx encounter an #UD and go
> and emulate the instruction.
> 
> I guess this is for the case where we actually do want to inject the #UD
> into the guest and not emulate the instruction.
> 
> Btw, it is only checked in x86_decode_insn so we could just as well hand
> down the emulation_type from the caller x86_emulate_instruction and kill
> ->only_vendor_specific_insn completely like so:
> 
> 	if (!(ctxt->d & EmulateOnUD) && (emul_type & EMULTYPE_TRAP_UD))
> 
EMULTYPE_ values are external to emulator.c and control how x86.c
invokes the emulator. I prefer not to change kvm<->emulator interface
just to get rid of one ctxt field.

--
			Gleb.