[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH for-4.12 v4 2/3] x86/svm: Drop enum instruction_index and simplify svm_get_insn_len()
On 1/31/19 12:24 PM, Andrew Cooper wrote: > Passing a 32-bit integer index into an array with entries containing less than > 32 bits of data is wasteful, and creates an unnecessary error condition of > passing an out-of-range index. > > The width of the X86EMUL_OPC() encoding is currently 20 bits for the > instructions used, which leaves room for a modrm byte. Drop opc_tab[] > entirely, and encode the expected opcode/modrm information directly. > > Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> Acked-by: Brian Woods <brian.woods@xxxxxxx> > --- > CC: Jan Beulich <JBeulich@xxxxxxxx> > CC: Wei Liu <wei.liu2@xxxxxxxxxx> > CC: Roger Pau Monné <roger.pau@xxxxxxxxxx> > CC: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> > CC: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx> > CC: Brian Woods <brian.woods@xxxxxxx> > CC: Juergen Gross <jgross@xxxxxxxx> > > The internals of X86EMUL_OPC() mean that we can't actually check for overflows > with BUILD_BUG_ON(), but if the opcode encoding does changes and overflow, > then the resulting fallout will be very obvious in debug builds of Xen. > > v3: > * New > v4: > * Drop MODRM(), use Octal instead. > --- > xen/arch/x86/hvm/svm/emulate.c | 51 +++++++-------------------------- > xen/include/asm-x86/hvm/svm/emulate.h | 53 > +++++++++++++++++++---------------- > 2 files changed, 39 insertions(+), 65 deletions(-) > > diff --git a/xen/arch/x86/hvm/svm/emulate.c b/xen/arch/x86/hvm/svm/emulate.c > index 7799908..fb0d823 100644 > --- a/xen/arch/x86/hvm/svm/emulate.c > +++ b/xen/arch/x86/hvm/svm/emulate.c > @@ -54,36 +54,6 @@ static unsigned long svm_nextrip_insn_length(struct vcpu > *v) > return vmcb->nextrip - vmcb->rip; > } > > -static const struct { > - unsigned int opcode; > - struct { > - unsigned int rm:3; > - unsigned int reg:3; > - unsigned int mod:2; > -#define MODRM(mod, reg, rm) { rm, reg, mod } > - } modrm; > -} opc_tab[INSTR_MAX_COUNT] = { > - [INSTR_PAUSE] = { X86EMUL_OPC_F3(0, 0x90) }, > - [INSTR_INT3] = { X86EMUL_OPC( 0, 0xcc) }, > - [INSTR_ICEBP] = { X86EMUL_OPC( 0, 0xf1) }, > - [INSTR_HLT] = { X86EMUL_OPC( 0, 0xf4) }, > - [INSTR_XSETBV] = { X86EMUL_OPC(0x0f, 0x01), MODRM(3, 2, 1) }, > - [INSTR_VMRUN] = { X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 0) }, > - [INSTR_VMCALL] = { X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 1) }, > - [INSTR_VMLOAD] = { X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 2) }, > - [INSTR_VMSAVE] = { X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 3) }, > - [INSTR_STGI] = { X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 4) }, > - [INSTR_CLGI] = { X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 5) }, > - [INSTR_INVLPGA] = { X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 7) }, > - [INSTR_RDTSCP] = { X86EMUL_OPC(0x0f, 0x01), MODRM(3, 7, 1) }, > - [INSTR_INVD] = { X86EMUL_OPC(0x0f, 0x08) }, > - [INSTR_WBINVD] = { X86EMUL_OPC(0x0f, 0x09) }, > - [INSTR_WRMSR] = { X86EMUL_OPC(0x0f, 0x30) }, > - [INSTR_RDTSC] = { X86EMUL_OPC(0x0f, 0x31) }, > - [INSTR_RDMSR] = { X86EMUL_OPC(0x0f, 0x32) }, > - [INSTR_CPUID] = { X86EMUL_OPC(0x0f, 0xa2) }, > -}; > - > /* > * First-gen SVM didn't have the NextRIP feature, meaning that when we take > a > * fault-style vmexit, we have to decode the instruction stream to calculate > @@ -93,12 +63,13 @@ static const struct { > * hardware reported instruction length (if available) with the result from > * x86_decode_insn(). > */ > -unsigned int svm_get_insn_len(struct vcpu *v, enum instruction_index insn) > +unsigned int svm_get_insn_len(struct vcpu *v, unsigned int instr_enc) > { > struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb; > struct hvm_emulate_ctxt ctxt; > struct x86_emulate_state *state; > unsigned long nrip_len, emul_len; > + unsigned int instr_opcode, instr_modrm; > unsigned int modrm_rm, modrm_reg; > int modrm_mod; > > @@ -131,20 +102,18 @@ unsigned int svm_get_insn_len(struct vcpu *v, enum > instruction_index insn) > } > #endif > > - if ( insn >= ARRAY_SIZE(opc_tab) ) > - { > - ASSERT_UNREACHABLE(); > - return 0; > - } > + /* Extract components from instr_enc. */ > + instr_modrm = instr_enc & 0xff; > + instr_opcode = instr_enc >> 8; > > - if ( opc_tab[insn].opcode == ctxt.ctxt.opcode ) > + if ( instr_opcode == ctxt.ctxt.opcode ) > { > - if ( !opc_tab[insn].modrm.mod ) > + if ( !instr_modrm ) > return emul_len; > > - if ( modrm_mod == opc_tab[insn].modrm.mod && > - (modrm_rm & 7) == opc_tab[insn].modrm.rm && > - (modrm_reg & 7) == opc_tab[insn].modrm.reg ) > + if ( modrm_mod == MASK_EXTR(instr_modrm, 0300) && > + (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) && > + (modrm_rm & 7) == MASK_EXTR(instr_modrm, 0007) ) > return emul_len; > } > > diff --git a/xen/include/asm-x86/hvm/svm/emulate.h > b/xen/include/asm-x86/hvm/svm/emulate.h > index 82359ec..9af1006 100644 > --- a/xen/include/asm-x86/hvm/svm/emulate.h > +++ b/xen/include/asm-x86/hvm/svm/emulate.h > @@ -19,33 +19,38 @@ > #ifndef __ASM_X86_HVM_SVM_EMULATE_H__ > #define __ASM_X86_HVM_SVM_EMULATE_H__ > > -/* Enumerate some standard instructions that we support */ > -enum instruction_index { > - INSTR_INVD, > - INSTR_WBINVD, > - INSTR_CPUID, > - INSTR_RDMSR, > - INSTR_WRMSR, > - INSTR_VMCALL, > - INSTR_HLT, > - INSTR_INT3, > - INSTR_RDTSC, > - INSTR_RDTSCP, > - INSTR_PAUSE, > - INSTR_XSETBV, > - INSTR_VMRUN, > - INSTR_VMLOAD, > - INSTR_VMSAVE, > - INSTR_STGI, > - INSTR_CLGI, > - INSTR_INVLPGA, > - INSTR_ICEBP, > - INSTR_MAX_COUNT /* Must be last - Number of instructions supported */ > -}; > +/* > + * Encoding for svm_get_insn_len(). We take X86EMUL_OPC() for the main > + * opcode, shifted left to make room for the ModRM byte. > + * > + * The Grp7 instructions have their ModRM byte expressed in octal for easier > + * cross referencing with the opcode extension table. > + */ > +#define INSTR_ENC(opc, modrm) (((opc) << 8) | (modrm)) > + > +#define INSTR_PAUSE INSTR_ENC(X86EMUL_OPC_F3(0, 0x90), 0) > +#define INSTR_INT3 INSTR_ENC(X86EMUL_OPC( 0, 0xcc), 0) > +#define INSTR_ICEBP INSTR_ENC(X86EMUL_OPC( 0, 0xf1), 0) > +#define INSTR_HLT INSTR_ENC(X86EMUL_OPC( 0, 0xf4), 0) > +#define INSTR_XSETBV INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0321) > +#define INSTR_VMRUN INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0330) > +#define INSTR_VMCALL INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0331) > +#define INSTR_VMLOAD INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0332) > +#define INSTR_VMSAVE INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0333) > +#define INSTR_STGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0334) > +#define INSTR_CLGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0335) > +#define INSTR_INVLPGA INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0337) > +#define INSTR_RDTSCP INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0371) > +#define INSTR_INVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x08), 0) > +#define INSTR_WBINVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x09), 0) > +#define INSTR_WRMSR INSTR_ENC(X86EMUL_OPC(0x0f, 0x30), 0) > +#define INSTR_RDTSC INSTR_ENC(X86EMUL_OPC(0x0f, 0x31), 0) > +#define INSTR_RDMSR INSTR_ENC(X86EMUL_OPC(0x0f, 0x32), 0) > +#define INSTR_CPUID INSTR_ENC(X86EMUL_OPC(0x0f, 0xa2), 0) > > struct vcpu; > > -unsigned int svm_get_insn_len(struct vcpu *v, enum instruction_index instr); > +unsigned int svm_get_insn_len(struct vcpu *v, unsigned int instr_enc); > > #endif /* __ASM_X86_HVM_SVM_EMULATE_H__ */ > > _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |