|
[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 |