* [PATCH v3 0/3] x86/svm: Improvements to SVM instruction length handling
@ 2018-12-31 11:37 Andrew Cooper
2018-12-31 11:37 ` [PATCH v3 1/3] x86/svm: Remove list functionality from __get_instruction_length_* infrastructure Andrew Cooper
` (3 more replies)
0 siblings, 4 replies; 16+ messages in thread
From: Andrew Cooper @ 2018-12-31 11:37 UTC (permalink / raw)
To: Xen-devel
Cc: Wei Liu, Suravee Suthikulpanit, Andrew Cooper, Paul Durrant,
Jan Beulich, Boris Ostrovsky, Brian Woods, Roger Pau Monné
The main bugfix in v2 of this series has now been committed, leaving just the
cleanup remaining. See patches for details.
Andrew Cooper (3):
x86/svm: Remove list functionality from __get_instruction_length_* infrastructure
x86/svm: Drop enum instruction_index and simplify svm_get_insn_len()
x86/svm: Improve diagnostics when svm_get_insn_len() fails
xen/arch/x86/hvm/svm/emulate.c | 116 +++++++++++-----------------------
xen/arch/x86/hvm/svm/nestedsvm.c | 9 +--
xen/arch/x86/hvm/svm/svm.c | 39 ++++++------
xen/include/asm-x86/hvm/svm/emulate.h | 58 ++++++++---------
4 files changed, 88 insertions(+), 134 deletions(-)
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 1/3] x86/svm: Remove list functionality from __get_instruction_length_* infrastructure
2018-12-31 11:37 [PATCH v3 0/3] x86/svm: Improvements to SVM instruction length handling Andrew Cooper
@ 2018-12-31 11:37 ` Andrew Cooper
2019-01-31 16:42 ` Woods, Brian
2018-12-31 11:37 ` [PATCH v3 2/3] x86/svm: Drop enum instruction_index and simplify svm_get_insn_len() Andrew Cooper
` (2 subsequent siblings)
3 siblings, 1 reply; 16+ messages in thread
From: Andrew Cooper @ 2018-12-31 11:37 UTC (permalink / raw)
To: Xen-devel
Cc: Wei Liu, Andrew Cooper, Suravee Suthikulpanit, Boris Ostrovsky,
Brian Woods, Roger Pau Monné
The existing __get_instruction_length_from_list() has a single user
which uses the list functionality. That user however should be looking
specifically for INVD or WBINVD, as reported by the vmexit exit reason.
Modify svm_vmexit_do_invalidate_cache() to ask for the correct
instruction, and drop all list functionality from the helper.
Take the opportunity to rename it to svm_get_insn_len(), and drop the
IOIO length handling whch has never been used.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Brian Woods <brian.woods@amd.com>
v2:
* New
v3:
* Deduplicate the calls to svm_nextrip_insn_length()
---
xen/arch/x86/hvm/svm/emulate.c | 76 +++++++++++++++++------------------
xen/arch/x86/hvm/svm/nestedsvm.c | 9 +++--
xen/arch/x86/hvm/svm/svm.c | 39 +++++++++---------
xen/include/asm-x86/hvm/svm/emulate.h | 9 +----
4 files changed, 61 insertions(+), 72 deletions(-)
diff --git a/xen/arch/x86/hvm/svm/emulate.c b/xen/arch/x86/hvm/svm/emulate.c
index 4abeab8..7799908 100644
--- a/xen/arch/x86/hvm/svm/emulate.c
+++ b/xen/arch/x86/hvm/svm/emulate.c
@@ -84,28 +84,31 @@ static const struct {
[INSTR_CPUID] = { X86EMUL_OPC(0x0f, 0xa2) },
};
-int __get_instruction_length_from_list(struct vcpu *v,
- const enum instruction_index *list, unsigned int list_count)
+/*
+ * 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
+ * how many bytes to move %rip forwards by.
+ *
+ * To double check the implementation, in debug builds, always compare the
+ * 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)
{
struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
struct hvm_emulate_ctxt ctxt;
struct x86_emulate_state *state;
- unsigned long inst_len, j;
+ unsigned long nrip_len, emul_len;
unsigned int modrm_rm, modrm_reg;
int modrm_mod;
- /*
- * In debug builds, always use x86_decode_insn() and compare with
- * hardware.
- */
-#ifdef NDEBUG
- if ( (inst_len = svm_nextrip_insn_length(v)) > MAX_INST_LEN )
- gprintk(XENLOG_WARNING, "NRip reported inst_len %lu\n", inst_len);
- else if ( inst_len != 0 )
- return inst_len;
+ nrip_len = svm_nextrip_insn_length(v);
- if ( vmcb->exitcode == VMEXIT_IOIO )
- return vmcb->exitinfo2 - vmcb->rip;
+#ifdef NDEBUG
+ if ( nrip_len > MAX_INST_LEN )
+ gprintk(XENLOG_WARNING, "NRip reported inst_len %lu\n", nrip_len);
+ else if ( nrip_len != 0 )
+ return nrip_len;
#endif
ASSERT(v == current);
@@ -115,41 +118,34 @@ int __get_instruction_length_from_list(struct vcpu *v,
if ( IS_ERR_OR_NULL(state) )
return 0;
- inst_len = x86_insn_length(state, &ctxt.ctxt);
+ emul_len = x86_insn_length(state, &ctxt.ctxt);
modrm_mod = x86_insn_modrm(state, &modrm_rm, &modrm_reg);
x86_emulate_free_state(state);
+
#ifndef NDEBUG
- if ( vmcb->exitcode == VMEXIT_IOIO )
- j = vmcb->exitinfo2 - vmcb->rip;
- else
- j = svm_nextrip_insn_length(v);
- if ( j && j != inst_len )
+ if ( nrip_len && nrip_len != emul_len )
{
gprintk(XENLOG_WARNING, "insn-len[%02x]=%lu (exp %lu)\n",
- ctxt.ctxt.opcode, inst_len, j);
- return j;
+ ctxt.ctxt.opcode, nrip_len, emul_len);
+ return nrip_len;
}
#endif
- for ( j = 0; j < list_count; j++ )
+ if ( insn >= ARRAY_SIZE(opc_tab) )
{
- unsigned int instr = list[j];
-
- if ( instr >= ARRAY_SIZE(opc_tab) )
- {
- ASSERT_UNREACHABLE();
- break;
- }
- if ( opc_tab[instr].opcode == ctxt.ctxt.opcode )
- {
- if ( !opc_tab[instr].modrm.mod )
- return inst_len;
-
- if ( modrm_mod == opc_tab[instr].modrm.mod &&
- (modrm_rm & 7) == opc_tab[instr].modrm.rm &&
- (modrm_reg & 7) == opc_tab[instr].modrm.reg )
- return inst_len;
- }
+ ASSERT_UNREACHABLE();
+ return 0;
+ }
+
+ if ( opc_tab[insn].opcode == ctxt.ctxt.opcode )
+ {
+ if ( !opc_tab[insn].modrm.mod )
+ 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 )
+ return emul_len;
}
gdprintk(XENLOG_WARNING,
diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
index 9660202..35c1a04 100644
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -743,8 +743,9 @@ nsvm_vcpu_vmrun(struct vcpu *v, struct cpu_user_regs *regs)
struct nestedvcpu *nv = &vcpu_nestedhvm(v);
struct nestedsvm *svm = &vcpu_nestedsvm(v);
- inst_len = __get_instruction_length(v, INSTR_VMRUN);
- if (inst_len == 0) {
+ inst_len = svm_get_insn_len(v, INSTR_VMRUN);
+ if ( inst_len == 0 )
+ {
svm->ns_vmexit.exitcode = VMEXIT_SHUTDOWN;
return -1;
}
@@ -1616,7 +1617,7 @@ void svm_vmexit_do_stgi(struct cpu_user_regs *regs, struct vcpu *v)
return;
}
- if ( (inst_len = __get_instruction_length(v, INSTR_STGI)) == 0 )
+ if ( (inst_len = svm_get_insn_len(v, INSTR_STGI)) == 0 )
return;
nestedsvm_vcpu_stgi(v);
@@ -1637,7 +1638,7 @@ void svm_vmexit_do_clgi(struct cpu_user_regs *regs, struct vcpu *v)
return;
}
- if ( (inst_len = __get_instruction_length(v, INSTR_CLGI)) == 0 )
+ if ( (inst_len = svm_get_insn_len(v, INSTR_CLGI)) == 0 )
return;
nestedsvm_vcpu_clgi(v);
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 954822c..2584b90 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2244,8 +2244,8 @@ static void svm_do_msr_access(struct cpu_user_regs *regs)
{
struct vcpu *curr = current;
bool rdmsr = curr->arch.hvm.svm.vmcb->exitinfo1 == 0;
- int rc, inst_len = __get_instruction_length(
- curr, rdmsr ? INSTR_RDMSR : INSTR_WRMSR);
+ int rc, inst_len = svm_get_insn_len(curr, rdmsr ? INSTR_RDMSR
+ : INSTR_WRMSR);
if ( inst_len == 0 )
return;
@@ -2272,7 +2272,7 @@ static void svm_vmexit_do_hlt(struct vmcb_struct *vmcb,
{
unsigned int inst_len;
- if ( (inst_len = __get_instruction_length(current, INSTR_HLT)) == 0 )
+ if ( (inst_len = svm_get_insn_len(current, INSTR_HLT)) == 0 )
return;
__update_guest_eip(regs, inst_len);
@@ -2283,7 +2283,6 @@ static void svm_vmexit_do_rdtsc(struct cpu_user_regs *regs, bool rdtscp)
{
struct vcpu *curr = current;
const struct domain *currd = curr->domain;
- enum instruction_index insn = rdtscp ? INSTR_RDTSCP : INSTR_RDTSC;
unsigned int inst_len;
if ( rdtscp && !currd->arch.cpuid->extd.rdtscp )
@@ -2292,7 +2291,8 @@ static void svm_vmexit_do_rdtsc(struct cpu_user_regs *regs, bool rdtscp)
return;
}
- if ( (inst_len = __get_instruction_length(curr, insn)) == 0 )
+ if ( (inst_len = svm_get_insn_len(curr, rdtscp ? INSTR_RDTSCP
+ : INSTR_RDTSC)) == 0 )
return;
__update_guest_eip(regs, inst_len);
@@ -2307,7 +2307,7 @@ static void svm_vmexit_do_pause(struct cpu_user_regs *regs)
{
unsigned int inst_len;
- if ( (inst_len = __get_instruction_length(current, INSTR_PAUSE)) == 0 )
+ if ( (inst_len = svm_get_insn_len(current, INSTR_PAUSE)) == 0 )
return;
__update_guest_eip(regs, inst_len);
@@ -2374,7 +2374,7 @@ svm_vmexit_do_vmload(struct vmcb_struct *vmcb,
unsigned int inst_len;
struct page_info *page;
- if ( (inst_len = __get_instruction_length(v, INSTR_VMLOAD)) == 0 )
+ if ( (inst_len = svm_get_insn_len(v, INSTR_VMLOAD)) == 0 )
return;
if ( !nsvm_efer_svm_enabled(v) )
@@ -2409,7 +2409,7 @@ svm_vmexit_do_vmsave(struct vmcb_struct *vmcb,
unsigned int inst_len;
struct page_info *page;
- if ( (inst_len = __get_instruction_length(v, INSTR_VMSAVE)) == 0 )
+ if ( (inst_len = svm_get_insn_len(v, INSTR_VMSAVE)) == 0 )
return;
if ( !nsvm_efer_svm_enabled(v) )
@@ -2477,13 +2477,12 @@ static void svm_wbinvd_intercept(void)
flush_all(FLUSH_CACHE);
}
-static void svm_vmexit_do_invalidate_cache(struct cpu_user_regs *regs)
+static void svm_vmexit_do_invalidate_cache(struct cpu_user_regs *regs,
+ bool invld)
{
- static const enum instruction_index list[] = { INSTR_INVD, INSTR_WBINVD };
- int inst_len;
+ unsigned int inst_len = svm_get_insn_len(current, invld ? INSTR_INVD
+ : INSTR_WBINVD);
- inst_len = __get_instruction_length_from_list(
- current, list, ARRAY_SIZE(list));
if ( inst_len == 0 )
return;
@@ -2758,7 +2757,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
else
{
trap_type = X86_EVENTTYPE_PRI_SW_EXCEPTION;
- inst_len = __get_instruction_length(v, INSTR_ICEBP);
+ inst_len = svm_get_insn_len(v, INSTR_ICEBP);
}
rc = hvm_monitor_debug(regs->rip,
@@ -2775,7 +2774,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
break;
case VMEXIT_EXCEPTION_BP:
- inst_len = __get_instruction_length(v, INSTR_INT3);
+ inst_len = svm_get_insn_len(v, INSTR_INT3);
if ( inst_len == 0 )
break;
@@ -2866,7 +2865,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
case VMEXIT_INVD:
case VMEXIT_WBINVD:
- svm_vmexit_do_invalidate_cache(regs);
+ svm_vmexit_do_invalidate_cache(regs, exit_reason == VMEXIT_INVD);
break;
case VMEXIT_TASK_SWITCH: {
@@ -2895,7 +2894,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
case VMEXIT_CPUID:
{
- unsigned int inst_len = __get_instruction_length(v, INSTR_CPUID);
+ unsigned int inst_len = svm_get_insn_len(v, INSTR_CPUID);
int rc = 0;
if ( inst_len == 0 )
@@ -2951,14 +2950,14 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
break;
}
- if ( (inst_len = __get_instruction_length(v, INSTR_INVLPGA)) == 0 )
+ if ( (inst_len = svm_get_insn_len(v, INSTR_INVLPGA)) == 0 )
break;
svm_invlpga_intercept(v, regs->rax, regs->ecx);
__update_guest_eip(regs, inst_len);
break;
case VMEXIT_VMMCALL:
- if ( (inst_len = __get_instruction_length(v, INSTR_VMCALL)) == 0 )
+ if ( (inst_len = svm_get_insn_len(v, INSTR_VMCALL)) == 0 )
break;
BUG_ON(vcpu_guestmode);
HVMTRACE_1D(VMMCALL, regs->eax);
@@ -3012,7 +3011,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
case VMEXIT_XSETBV:
if ( vmcb_get_cpl(vmcb) )
hvm_inject_hw_exception(TRAP_gp_fault, 0);
- else if ( (inst_len = __get_instruction_length(v, INSTR_XSETBV)) &&
+ else if ( (inst_len = svm_get_insn_len(v, INSTR_XSETBV)) &&
hvm_handle_xsetbv(regs->ecx, msr_fold(regs)) == X86EMUL_OKAY )
__update_guest_eip(regs, inst_len);
break;
diff --git a/xen/include/asm-x86/hvm/svm/emulate.h b/xen/include/asm-x86/hvm/svm/emulate.h
index ca92abb..82359ec 100644
--- a/xen/include/asm-x86/hvm/svm/emulate.h
+++ b/xen/include/asm-x86/hvm/svm/emulate.h
@@ -45,14 +45,7 @@ enum instruction_index {
struct vcpu;
-int __get_instruction_length_from_list(
- struct vcpu *, const enum instruction_index *, unsigned int list_count);
-
-static inline int __get_instruction_length(
- struct vcpu *v, enum instruction_index instr)
-{
- return __get_instruction_length_from_list(v, &instr, 1);
-}
+unsigned int svm_get_insn_len(struct vcpu *v, enum instruction_index instr);
#endif /* __ASM_X86_HVM_SVM_EMULATE_H__ */
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v3 2/3] x86/svm: Drop enum instruction_index and simplify svm_get_insn_len()
2018-12-31 11:37 [PATCH v3 0/3] x86/svm: Improvements to SVM instruction length handling Andrew Cooper
2018-12-31 11:37 ` [PATCH v3 1/3] x86/svm: Remove list functionality from __get_instruction_length_* infrastructure Andrew Cooper
@ 2018-12-31 11:37 ` Andrew Cooper
2018-12-31 11:57 ` Andrew Cooper
` (2 more replies)
2018-12-31 11:37 ` [PATCH v3 3/3] x86/svm: Improve diagnostics when svm_get_insn_len() fails Andrew Cooper
2019-01-31 16:56 ` [PATCH v3 0/3] x86/svm: Improvements to SVM instruction length handling Andrew Cooper
3 siblings, 3 replies; 16+ messages in thread
From: Andrew Cooper @ 2018-12-31 11:37 UTC (permalink / raw)
To: Xen-devel
Cc: Wei Liu, Jan Beulich, Andrew Cooper, Suravee Suthikulpanit,
Boris Ostrovsky, Brian Woods, Roger Pau Monné
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 at most 24 bits, 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@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Brian Woods <brian.woods@amd.com>
v3:
* New
---
xen/arch/x86/hvm/svm/emulate.c | 51 +++++++----------------------------
xen/include/asm-x86/hvm/svm/emulate.h | 51 ++++++++++++++++++-----------------
2 files changed, 37 insertions(+), 65 deletions(-)
diff --git a/xen/arch/x86/hvm/svm/emulate.c b/xen/arch/x86/hvm/svm/emulate.c
index 7799908..827cfc8 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, 0xc0) &&
+ (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0x38) &&
+ (modrm_rm & 7) == MASK_EXTR(instr_modrm, 0x07) )
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..aa64ec3 100644
--- a/xen/include/asm-x86/hvm/svm/emulate.h
+++ b/xen/include/asm-x86/hvm/svm/emulate.h
@@ -19,33 +19,36 @@
#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.
+ */
+#define INSTR_ENC(opc, modrm) (((unsigned int)(opc) << 8) | (modrm))
+#define MODRM(mod, reg, rm) (((mod) << 6) | ((reg) << 3) | rm)
+
+#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), MODRM(3, 2, 1))
+#define INSTR_VMRUN INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 0))
+#define INSTR_VMCALL INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 1))
+#define INSTR_VMLOAD INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 2))
+#define INSTR_VMSAVE INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 3))
+#define INSTR_STGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 4))
+#define INSTR_CLGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 5))
+#define INSTR_INVLPGA INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 7))
+#define INSTR_RDTSCP INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), MODRM(3, 7, 1))
+#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__ */
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v3 3/3] x86/svm: Improve diagnostics when svm_get_insn_len() fails
2018-12-31 11:37 [PATCH v3 0/3] x86/svm: Improvements to SVM instruction length handling Andrew Cooper
2018-12-31 11:37 ` [PATCH v3 1/3] x86/svm: Remove list functionality from __get_instruction_length_* infrastructure Andrew Cooper
2018-12-31 11:37 ` [PATCH v3 2/3] x86/svm: Drop enum instruction_index and simplify svm_get_insn_len() Andrew Cooper
@ 2018-12-31 11:37 ` Andrew Cooper
2019-01-03 9:01 ` Paul Durrant
2019-01-31 16:56 ` [PATCH v3 0/3] x86/svm: Improvements to SVM instruction length handling Andrew Cooper
3 siblings, 1 reply; 16+ messages in thread
From: Andrew Cooper @ 2018-12-31 11:37 UTC (permalink / raw)
To: Xen-devel
Cc: Wei Liu, Suravee Suthikulpanit, Andrew Cooper, Paul Durrant,
Jan Beulich, Boris Ostrovsky, Brian Woods, Roger Pau Monné
Sadly, a lone:
(XEN) emulate.c:156:d2v0 svm_get_insn_len: Mismatch between expected and actual instruction: eip = fffff804564139c0
on the console is of no use trying to identify what went wrong. Dump as much
state as we can to help identify what went wrong.
(XEN) Insn mismatch: Expected opcode 0xf0031, modrm 0, got nrip_len 3, emul_len 3
(XEN) SVM Insn len emulation failed (1): d1v0 64bit @ 0008:0010475f -> 0f 01 f9 0f 31 5b 31 ff 31 c0 e9 c2 db ff ff 00
Drop the debug-only early exit if the sources of length disagree, because the
only effect it has it to avoid the more detailed analysis of what went wrong.
Reported-by: Paul Durrant <paul.durrant@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Brian Woods <brian.woods@amd.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Paul Durrant <paul.durrant@citrix.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Brian Woods <brian.woods@amd.com>
v2:
* Drop anonymous union
* Rebase
v3:
* Rework yet again, over the removal of enum instruction_index
---
xen/arch/x86/hvm/svm/emulate.c | 19 ++++++-------------
1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/xen/arch/x86/hvm/svm/emulate.c b/xen/arch/x86/hvm/svm/emulate.c
index 827cfc8..4000087 100644
--- a/xen/arch/x86/hvm/svm/emulate.c
+++ b/xen/arch/x86/hvm/svm/emulate.c
@@ -65,7 +65,6 @@ static unsigned long svm_nextrip_insn_length(struct vcpu *v)
*/
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;
@@ -93,15 +92,6 @@ unsigned int svm_get_insn_len(struct vcpu *v, unsigned int instr_enc)
modrm_mod = x86_insn_modrm(state, &modrm_rm, &modrm_reg);
x86_emulate_free_state(state);
-#ifndef NDEBUG
- if ( nrip_len && nrip_len != emul_len )
- {
- gprintk(XENLOG_WARNING, "insn-len[%02x]=%lu (exp %lu)\n",
- ctxt.ctxt.opcode, nrip_len, emul_len);
- return nrip_len;
- }
-#endif
-
/* Extract components from instr_enc. */
instr_modrm = instr_enc & 0xff;
instr_opcode = instr_enc >> 8;
@@ -117,9 +107,12 @@ unsigned int svm_get_insn_len(struct vcpu *v, unsigned int instr_enc)
return emul_len;
}
- gdprintk(XENLOG_WARNING,
- "%s: Mismatch between expected and actual instruction: "
- "eip = %lx\n", __func__, (unsigned long)vmcb->rip);
+ printk(XENLOG_G_WARNING
+ "Insn mismatch: Expected opcode %#x, modrm %#x, got nrip_len %lu, emul_len %lu\n",
+ instr_opcode, instr_modrm, nrip_len, emul_len);
+ hvm_dump_emulation_state(XENLOG_G_WARNING, "SVM Insn len",
+ &ctxt, X86EMUL_UNHANDLEABLE);
+
hvm_inject_hw_exception(TRAP_gp_fault, 0);
return 0;
}
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v3 2/3] x86/svm: Drop enum instruction_index and simplify svm_get_insn_len()
2018-12-31 11:37 ` [PATCH v3 2/3] x86/svm: Drop enum instruction_index and simplify svm_get_insn_len() Andrew Cooper
@ 2018-12-31 11:57 ` Andrew Cooper
2019-01-04 10:30 ` Jan Beulich
2019-01-07 10:30 ` Jan Beulich
2019-01-31 18:24 ` [PATCH for-4.12 v4 " Andrew Cooper
2 siblings, 1 reply; 16+ messages in thread
From: Andrew Cooper @ 2018-12-31 11:57 UTC (permalink / raw)
To: Xen-devel
Cc: Wei Liu, Jan Beulich, Suravee Suthikulpanit, Boris Ostrovsky,
Brian Woods, Roger Pau Monné
[-- Attachment #1.1: Type: text/plain, Size: 2070 bytes --]
On 31/12/2018 11:37, Andrew Cooper wrote:
> +/*
> + * Encoding for svm_get_insn_len(). We take X86EMUL_OPC() for the main
> + * opcode, shifted left to make room for the ModRM byte.
> + */
> +#define INSTR_ENC(opc, modrm) (((unsigned int)(opc) << 8) | (modrm))
> +#define MODRM(mod, reg, rm) (((mod) << 6) | ((reg) << 3) | rm)
> +
> +#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), MODRM(3, 2, 1))
> +#define INSTR_VMRUN INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 0))
> +#define INSTR_VMCALL INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 1))
> +#define INSTR_VMLOAD INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 2))
> +#define INSTR_VMSAVE INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 3))
> +#define INSTR_STGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 4))
> +#define INSTR_CLGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 5))
> +#define INSTR_INVLPGA INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 7))
> +#define INSTR_RDTSCP INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), MODRM(3, 7, 1))
I'm still tempted to drop the MODRM() macro, and use octal notation
#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)
Seeing as this is a far more logical way to read x86 instructions.
~Andrew
[-- Attachment #1.2: Type: text/html, Size: 2495 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 3/3] x86/svm: Improve diagnostics when svm_get_insn_len() fails
2018-12-31 11:37 ` [PATCH v3 3/3] x86/svm: Improve diagnostics when svm_get_insn_len() fails Andrew Cooper
@ 2019-01-03 9:01 ` Paul Durrant
0 siblings, 0 replies; 16+ messages in thread
From: Paul Durrant @ 2019-01-03 9:01 UTC (permalink / raw)
To: Xen-devel
Cc: Wei Liu, Jan Beulich, Andrew Cooper, Suravee Suthikulpanit,
Boris Ostrovsky, Brian Woods, Roger Pau Monne
> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: 31 December 2018 11:38
> To: Xen-devel <xen-devel@lists.xen.org>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
> <JBeulich@suse.com>; Wei Liu <wei.liu2@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Boris
> Ostrovsky <boris.ostrovsky@oracle.com>; Suravee Suthikulpanit
> <suravee.suthikulpanit@amd.com>; Brian Woods <brian.woods@amd.com>
> Subject: [PATCH v3 3/3] x86/svm: Improve diagnostics when
> svm_get_insn_len() fails
>
> Sadly, a lone:
>
> (XEN) emulate.c:156:d2v0 svm_get_insn_len: Mismatch between expected and
> actual instruction: eip = fffff804564139c0
>
> on the console is of no use trying to identify what went wrong. Dump as
> much
> state as we can to help identify what went wrong.
>
> (XEN) Insn mismatch: Expected opcode 0xf0031, modrm 0, got nrip_len 3,
> emul_len 3
> (XEN) SVM Insn len emulation failed (1): d1v0 64bit @ 0008:0010475f ->
> 0f 01 f9 0f 31 5b 31 ff 31 c0 e9 c2 db ff ff 00
>
> Drop the debug-only early exit if the sources of length disagree, because
> the
> only effect it has it to avoid the more detailed analysis of what went
> wrong.
>
> Reported-by: Paul Durrant <paul.durrant@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Acked-by: Brian Woods <brian.woods@amd.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Brian Woods <brian.woods@amd.com>
>
> v2:
> * Drop anonymous union
> * Rebase
> v3:
> * Rework yet again, over the removal of enum instruction_index
> ---
> xen/arch/x86/hvm/svm/emulate.c | 19 ++++++-------------
> 1 file changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/svm/emulate.c
> b/xen/arch/x86/hvm/svm/emulate.c
> index 827cfc8..4000087 100644
> --- a/xen/arch/x86/hvm/svm/emulate.c
> +++ b/xen/arch/x86/hvm/svm/emulate.c
> @@ -65,7 +65,6 @@ static unsigned long svm_nextrip_insn_length(struct vcpu
> *v)
> */
> 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;
> @@ -93,15 +92,6 @@ unsigned int svm_get_insn_len(struct vcpu *v, unsigned
> int instr_enc)
> modrm_mod = x86_insn_modrm(state, &modrm_rm, &modrm_reg);
> x86_emulate_free_state(state);
>
> -#ifndef NDEBUG
> - if ( nrip_len && nrip_len != emul_len )
> - {
> - gprintk(XENLOG_WARNING, "insn-len[%02x]=%lu (exp %lu)\n",
> - ctxt.ctxt.opcode, nrip_len, emul_len);
> - return nrip_len;
> - }
> -#endif
> -
> /* Extract components from instr_enc. */
> instr_modrm = instr_enc & 0xff;
> instr_opcode = instr_enc >> 8;
> @@ -117,9 +107,12 @@ unsigned int svm_get_insn_len(struct vcpu *v,
> unsigned int instr_enc)
> return emul_len;
> }
>
> - gdprintk(XENLOG_WARNING,
> - "%s: Mismatch between expected and actual instruction: "
> - "eip = %lx\n", __func__, (unsigned long)vmcb->rip);
> + printk(XENLOG_G_WARNING
> + "Insn mismatch: Expected opcode %#x, modrm %#x, got nrip_len
> %lu, emul_len %lu\n",
> + instr_opcode, instr_modrm, nrip_len, emul_len);
> + hvm_dump_emulation_state(XENLOG_G_WARNING, "SVM Insn len",
> + &ctxt, X86EMUL_UNHANDLEABLE);
> +
> hvm_inject_hw_exception(TRAP_gp_fault, 0);
> return 0;
> }
> --
> 2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 2/3] x86/svm: Drop enum instruction_index and simplify svm_get_insn_len()
2018-12-31 11:57 ` Andrew Cooper
@ 2019-01-04 10:30 ` Jan Beulich
0 siblings, 0 replies; 16+ messages in thread
From: Jan Beulich @ 2019-01-04 10:30 UTC (permalink / raw)
To: Andrew Cooper
Cc: Wei Liu, Xen-devel, Suravee Suthikulpanit, Boris Ostrovsky,
Brian Woods, Roger Pau Monne
>>> On 31.12.18 at 12:57, <andrew.cooper3@citrix.com> wrote:
> On 31/12/2018 11:37, Andrew Cooper wrote:
>> +/*
>> + * Encoding for svm_get_insn_len(). We take X86EMUL_OPC() for the main
>> + * opcode, shifted left to make room for the ModRM byte.
>> + */
>> +#define INSTR_ENC(opc, modrm) (((unsigned int)(opc) << 8) | (modrm))
>> +#define MODRM(mod, reg, rm) (((mod) << 6) | ((reg) << 3) | rm)
>> +
>> +#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), MODRM(3, 2, 1))
>> +#define INSTR_VMRUN INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 0))
>> +#define INSTR_VMCALL INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 1))
>> +#define INSTR_VMLOAD INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 2))
>> +#define INSTR_VMSAVE INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 3))
>> +#define INSTR_STGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 4))
>> +#define INSTR_CLGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 5))
>> +#define INSTR_INVLPGA INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), MODRM(3, 3, 7))
>> +#define INSTR_RDTSCP INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), MODRM(3, 7, 1))
>
> I'm still tempted to drop the MODRM() macro, and use octal notation
>
> #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)
>
> Seeing as this is a far more logical way to read x86 instructions.
Fine with me, fwiw (with a comment making clear what this stands for).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 2/3] x86/svm: Drop enum instruction_index and simplify svm_get_insn_len()
2018-12-31 11:37 ` [PATCH v3 2/3] x86/svm: Drop enum instruction_index and simplify svm_get_insn_len() Andrew Cooper
2018-12-31 11:57 ` Andrew Cooper
@ 2019-01-07 10:30 ` Jan Beulich
2019-01-31 18:07 ` Andrew Cooper
2019-01-31 18:24 ` [PATCH for-4.12 v4 " Andrew Cooper
2 siblings, 1 reply; 16+ messages in thread
From: Jan Beulich @ 2019-01-07 10:30 UTC (permalink / raw)
To: Andrew Cooper
Cc: Wei Liu, Xen-devel, Suravee Suthikulpanit, Boris Ostrovsky,
Brian Woods, Roger Pau Monne
>>> On 31.12.18 at 12:37, <andrew.cooper3@citrix.com> 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 at most 24 bits, which leaves room
> for a modrm byte.
That's true for the 0x0f-prefix-space insns (and it's just 20 bits in that
case), but going this route we'd paint ourselves into a corner if we'd
ever have to add 0x0f38-, 0x0f3a-, or 0x8f0?-prefix-space insns.
Furthermore someone adjusting the encoding layout in x86_emulate.h
is very unlikely to notice breakage here until trying the resulting
binary - I strongly think some BUILD_BUG_ON() should be added to
make this apparent at build time.
> --- a/xen/include/asm-x86/hvm/svm/emulate.h
> +++ b/xen/include/asm-x86/hvm/svm/emulate.h
> @@ -19,33 +19,36 @@
> #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.
> + */
> +#define INSTR_ENC(opc, modrm) (((unsigned int)(opc) << 8) | (modrm))
I can't seem to figure what good the cast does.
> +#define MODRM(mod, reg, rm) (((mod) << 6) | ((reg) << 3) | rm)
"rm" also wants to be parenthesized (or neither "mod" nor "reg" would
need to be).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/3] x86/svm: Remove list functionality from __get_instruction_length_* infrastructure
2018-12-31 11:37 ` [PATCH v3 1/3] x86/svm: Remove list functionality from __get_instruction_length_* infrastructure Andrew Cooper
@ 2019-01-31 16:42 ` Woods, Brian
0 siblings, 0 replies; 16+ messages in thread
From: Woods, Brian @ 2019-01-31 16:42 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel
Cc: Boris Ostrovsky, Wei Liu, Suthikulpanit, Suravee, Roger Pau Monné
On 12/31/18 5:37 AM, Andrew Cooper wrote:
> The existing __get_instruction_length_from_list() has a single user
> which uses the list functionality. That user however should be looking
> specifically for INVD or WBINVD, as reported by the vmexit exit reason.
>
> Modify svm_vmexit_do_invalidate_cache() to ask for the correct
> instruction, and drop all list functionality from the helper.
>
> Take the opportunity to rename it to svm_get_insn_len(), and drop the
> IOIO length handling whch has never been used.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Brian Woods <brian.woods@amd.com>
> ---
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Brian Woods <brian.woods@amd.com>
>
> v2:
> * New
> v3:
> * Deduplicate the calls to svm_nextrip_insn_length()
> ---
> xen/arch/x86/hvm/svm/emulate.c | 76 +++++++++++++++++------------------
> xen/arch/x86/hvm/svm/nestedsvm.c | 9 +++--
> xen/arch/x86/hvm/svm/svm.c | 39 +++++++++---------
> xen/include/asm-x86/hvm/svm/emulate.h | 9 +----
> 4 files changed, 61 insertions(+), 72 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/svm/emulate.c b/xen/arch/x86/hvm/svm/emulate.c
> index 4abeab8..7799908 100644
> --- a/xen/arch/x86/hvm/svm/emulate.c
> +++ b/xen/arch/x86/hvm/svm/emulate.c
> @@ -84,28 +84,31 @@ static const struct {
> [INSTR_CPUID] = { X86EMUL_OPC(0x0f, 0xa2) },
> };
>
> -int __get_instruction_length_from_list(struct vcpu *v,
> - const enum instruction_index *list, unsigned int list_count)
> +/*
> + * 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
> + * how many bytes to move %rip forwards by.
> + *
> + * To double check the implementation, in debug builds, always compare the
> + * 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)
> {
> struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
> struct hvm_emulate_ctxt ctxt;
> struct x86_emulate_state *state;
> - unsigned long inst_len, j;
> + unsigned long nrip_len, emul_len;
> unsigned int modrm_rm, modrm_reg;
> int modrm_mod;
>
> - /*
> - * In debug builds, always use x86_decode_insn() and compare with
> - * hardware.
> - */
> -#ifdef NDEBUG
> - if ( (inst_len = svm_nextrip_insn_length(v)) > MAX_INST_LEN )
> - gprintk(XENLOG_WARNING, "NRip reported inst_len %lu\n", inst_len);
> - else if ( inst_len != 0 )
> - return inst_len;
> + nrip_len = svm_nextrip_insn_length(v);
>
> - if ( vmcb->exitcode == VMEXIT_IOIO )
> - return vmcb->exitinfo2 - vmcb->rip;
> +#ifdef NDEBUG
> + if ( nrip_len > MAX_INST_LEN )
> + gprintk(XENLOG_WARNING, "NRip reported inst_len %lu\n", nrip_len);
> + else if ( nrip_len != 0 )
> + return nrip_len;
> #endif
>
> ASSERT(v == current);
> @@ -115,41 +118,34 @@ int __get_instruction_length_from_list(struct vcpu *v,
> if ( IS_ERR_OR_NULL(state) )
> return 0;
>
> - inst_len = x86_insn_length(state, &ctxt.ctxt);
> + emul_len = x86_insn_length(state, &ctxt.ctxt);
> modrm_mod = x86_insn_modrm(state, &modrm_rm, &modrm_reg);
> x86_emulate_free_state(state);
> +
> #ifndef NDEBUG
> - if ( vmcb->exitcode == VMEXIT_IOIO )
> - j = vmcb->exitinfo2 - vmcb->rip;
> - else
> - j = svm_nextrip_insn_length(v);
> - if ( j && j != inst_len )
> + if ( nrip_len && nrip_len != emul_len )
> {
> gprintk(XENLOG_WARNING, "insn-len[%02x]=%lu (exp %lu)\n",
> - ctxt.ctxt.opcode, inst_len, j);
> - return j;
> + ctxt.ctxt.opcode, nrip_len, emul_len);
> + return nrip_len;
> }
> #endif
>
> - for ( j = 0; j < list_count; j++ )
> + if ( insn >= ARRAY_SIZE(opc_tab) )
> {
> - unsigned int instr = list[j];
> -
> - if ( instr >= ARRAY_SIZE(opc_tab) )
> - {
> - ASSERT_UNREACHABLE();
> - break;
> - }
> - if ( opc_tab[instr].opcode == ctxt.ctxt.opcode )
> - {
> - if ( !opc_tab[instr].modrm.mod )
> - return inst_len;
> -
> - if ( modrm_mod == opc_tab[instr].modrm.mod &&
> - (modrm_rm & 7) == opc_tab[instr].modrm.rm &&
> - (modrm_reg & 7) == opc_tab[instr].modrm.reg )
> - return inst_len;
> - }
> + ASSERT_UNREACHABLE();
> + return 0;
> + }
> +
> + if ( opc_tab[insn].opcode == ctxt.ctxt.opcode )
> + {
> + if ( !opc_tab[insn].modrm.mod )
> + 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 )
> + return emul_len;
> }
>
> gdprintk(XENLOG_WARNING,
> diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
> index 9660202..35c1a04 100644
> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
> @@ -743,8 +743,9 @@ nsvm_vcpu_vmrun(struct vcpu *v, struct cpu_user_regs *regs)
> struct nestedvcpu *nv = &vcpu_nestedhvm(v);
> struct nestedsvm *svm = &vcpu_nestedsvm(v);
>
> - inst_len = __get_instruction_length(v, INSTR_VMRUN);
> - if (inst_len == 0) {
> + inst_len = svm_get_insn_len(v, INSTR_VMRUN);
> + if ( inst_len == 0 )
> + {
> svm->ns_vmexit.exitcode = VMEXIT_SHUTDOWN;
> return -1;
> }
> @@ -1616,7 +1617,7 @@ void svm_vmexit_do_stgi(struct cpu_user_regs *regs, struct vcpu *v)
> return;
> }
>
> - if ( (inst_len = __get_instruction_length(v, INSTR_STGI)) == 0 )
> + if ( (inst_len = svm_get_insn_len(v, INSTR_STGI)) == 0 )
> return;
>
> nestedsvm_vcpu_stgi(v);
> @@ -1637,7 +1638,7 @@ void svm_vmexit_do_clgi(struct cpu_user_regs *regs, struct vcpu *v)
> return;
> }
>
> - if ( (inst_len = __get_instruction_length(v, INSTR_CLGI)) == 0 )
> + if ( (inst_len = svm_get_insn_len(v, INSTR_CLGI)) == 0 )
> return;
>
> nestedsvm_vcpu_clgi(v);
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 954822c..2584b90 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2244,8 +2244,8 @@ static void svm_do_msr_access(struct cpu_user_regs *regs)
> {
> struct vcpu *curr = current;
> bool rdmsr = curr->arch.hvm.svm.vmcb->exitinfo1 == 0;
> - int rc, inst_len = __get_instruction_length(
> - curr, rdmsr ? INSTR_RDMSR : INSTR_WRMSR);
> + int rc, inst_len = svm_get_insn_len(curr, rdmsr ? INSTR_RDMSR
> + : INSTR_WRMSR);
>
> if ( inst_len == 0 )
> return;
> @@ -2272,7 +2272,7 @@ static void svm_vmexit_do_hlt(struct vmcb_struct *vmcb,
> {
> unsigned int inst_len;
>
> - if ( (inst_len = __get_instruction_length(current, INSTR_HLT)) == 0 )
> + if ( (inst_len = svm_get_insn_len(current, INSTR_HLT)) == 0 )
> return;
> __update_guest_eip(regs, inst_len);
>
> @@ -2283,7 +2283,6 @@ static void svm_vmexit_do_rdtsc(struct cpu_user_regs *regs, bool rdtscp)
> {
> struct vcpu *curr = current;
> const struct domain *currd = curr->domain;
> - enum instruction_index insn = rdtscp ? INSTR_RDTSCP : INSTR_RDTSC;
> unsigned int inst_len;
>
> if ( rdtscp && !currd->arch.cpuid->extd.rdtscp )
> @@ -2292,7 +2291,8 @@ static void svm_vmexit_do_rdtsc(struct cpu_user_regs *regs, bool rdtscp)
> return;
> }
>
> - if ( (inst_len = __get_instruction_length(curr, insn)) == 0 )
> + if ( (inst_len = svm_get_insn_len(curr, rdtscp ? INSTR_RDTSCP
> + : INSTR_RDTSC)) == 0 )
> return;
>
> __update_guest_eip(regs, inst_len);
> @@ -2307,7 +2307,7 @@ static void svm_vmexit_do_pause(struct cpu_user_regs *regs)
> {
> unsigned int inst_len;
>
> - if ( (inst_len = __get_instruction_length(current, INSTR_PAUSE)) == 0 )
> + if ( (inst_len = svm_get_insn_len(current, INSTR_PAUSE)) == 0 )
> return;
> __update_guest_eip(regs, inst_len);
>
> @@ -2374,7 +2374,7 @@ svm_vmexit_do_vmload(struct vmcb_struct *vmcb,
> unsigned int inst_len;
> struct page_info *page;
>
> - if ( (inst_len = __get_instruction_length(v, INSTR_VMLOAD)) == 0 )
> + if ( (inst_len = svm_get_insn_len(v, INSTR_VMLOAD)) == 0 )
> return;
>
> if ( !nsvm_efer_svm_enabled(v) )
> @@ -2409,7 +2409,7 @@ svm_vmexit_do_vmsave(struct vmcb_struct *vmcb,
> unsigned int inst_len;
> struct page_info *page;
>
> - if ( (inst_len = __get_instruction_length(v, INSTR_VMSAVE)) == 0 )
> + if ( (inst_len = svm_get_insn_len(v, INSTR_VMSAVE)) == 0 )
> return;
>
> if ( !nsvm_efer_svm_enabled(v) )
> @@ -2477,13 +2477,12 @@ static void svm_wbinvd_intercept(void)
> flush_all(FLUSH_CACHE);
> }
>
> -static void svm_vmexit_do_invalidate_cache(struct cpu_user_regs *regs)
> +static void svm_vmexit_do_invalidate_cache(struct cpu_user_regs *regs,
> + bool invld)
> {
> - static const enum instruction_index list[] = { INSTR_INVD, INSTR_WBINVD };
> - int inst_len;
> + unsigned int inst_len = svm_get_insn_len(current, invld ? INSTR_INVD
> + : INSTR_WBINVD);
>
> - inst_len = __get_instruction_length_from_list(
> - current, list, ARRAY_SIZE(list));
> if ( inst_len == 0 )
> return;
>
> @@ -2758,7 +2757,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
> else
> {
> trap_type = X86_EVENTTYPE_PRI_SW_EXCEPTION;
> - inst_len = __get_instruction_length(v, INSTR_ICEBP);
> + inst_len = svm_get_insn_len(v, INSTR_ICEBP);
> }
>
> rc = hvm_monitor_debug(regs->rip,
> @@ -2775,7 +2774,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
> break;
>
> case VMEXIT_EXCEPTION_BP:
> - inst_len = __get_instruction_length(v, INSTR_INT3);
> + inst_len = svm_get_insn_len(v, INSTR_INT3);
>
> if ( inst_len == 0 )
> break;
> @@ -2866,7 +2865,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>
> case VMEXIT_INVD:
> case VMEXIT_WBINVD:
> - svm_vmexit_do_invalidate_cache(regs);
> + svm_vmexit_do_invalidate_cache(regs, exit_reason == VMEXIT_INVD);
> break;
>
> case VMEXIT_TASK_SWITCH: {
> @@ -2895,7 +2894,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>
> case VMEXIT_CPUID:
> {
> - unsigned int inst_len = __get_instruction_length(v, INSTR_CPUID);
> + unsigned int inst_len = svm_get_insn_len(v, INSTR_CPUID);
> int rc = 0;
>
> if ( inst_len == 0 )
> @@ -2951,14 +2950,14 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
> hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
> break;
> }
> - if ( (inst_len = __get_instruction_length(v, INSTR_INVLPGA)) == 0 )
> + if ( (inst_len = svm_get_insn_len(v, INSTR_INVLPGA)) == 0 )
> break;
> svm_invlpga_intercept(v, regs->rax, regs->ecx);
> __update_guest_eip(regs, inst_len);
> break;
>
> case VMEXIT_VMMCALL:
> - if ( (inst_len = __get_instruction_length(v, INSTR_VMCALL)) == 0 )
> + if ( (inst_len = svm_get_insn_len(v, INSTR_VMCALL)) == 0 )
> break;
> BUG_ON(vcpu_guestmode);
> HVMTRACE_1D(VMMCALL, regs->eax);
> @@ -3012,7 +3011,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
> case VMEXIT_XSETBV:
> if ( vmcb_get_cpl(vmcb) )
> hvm_inject_hw_exception(TRAP_gp_fault, 0);
> - else if ( (inst_len = __get_instruction_length(v, INSTR_XSETBV)) &&
> + else if ( (inst_len = svm_get_insn_len(v, INSTR_XSETBV)) &&
> hvm_handle_xsetbv(regs->ecx, msr_fold(regs)) == X86EMUL_OKAY )
> __update_guest_eip(regs, inst_len);
> break;
> diff --git a/xen/include/asm-x86/hvm/svm/emulate.h b/xen/include/asm-x86/hvm/svm/emulate.h
> index ca92abb..82359ec 100644
> --- a/xen/include/asm-x86/hvm/svm/emulate.h
> +++ b/xen/include/asm-x86/hvm/svm/emulate.h
> @@ -45,14 +45,7 @@ enum instruction_index {
>
> struct vcpu;
>
> -int __get_instruction_length_from_list(
> - struct vcpu *, const enum instruction_index *, unsigned int list_count);
> -
> -static inline int __get_instruction_length(
> - struct vcpu *v, enum instruction_index instr)
> -{
> - return __get_instruction_length_from_list(v, &instr, 1);
> -}
> +unsigned int svm_get_insn_len(struct vcpu *v, enum instruction_index instr);
>
> #endif /* __ASM_X86_HVM_SVM_EMULATE_H__ */
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 0/3] x86/svm: Improvements to SVM instruction length handling
2018-12-31 11:37 [PATCH v3 0/3] x86/svm: Improvements to SVM instruction length handling Andrew Cooper
` (2 preceding siblings ...)
2018-12-31 11:37 ` [PATCH v3 3/3] x86/svm: Improve diagnostics when svm_get_insn_len() fails Andrew Cooper
@ 2019-01-31 16:56 ` Andrew Cooper
2019-02-01 6:05 ` Juergen Gross
3 siblings, 1 reply; 16+ messages in thread
From: Andrew Cooper @ 2019-01-31 16:56 UTC (permalink / raw)
To: Xen-devel; +Cc: Juergen Gross
On 31/12/2018 11:37, Andrew Cooper wrote:
> The main bugfix in v2 of this series has now been committed, leaving just the
> cleanup remaining. See patches for details.
>
> Andrew Cooper (3):
> x86/svm: Remove list functionality from __get_instruction_length_* infrastructure
> x86/svm: Drop enum instruction_index and simplify svm_get_insn_len()
> x86/svm: Improve diagnostics when svm_get_insn_len() fails
>
> xen/arch/x86/hvm/svm/emulate.c | 116 +++++++++++-----------------------
> xen/arch/x86/hvm/svm/nestedsvm.c | 9 +--
> xen/arch/x86/hvm/svm/svm.c | 39 ++++++------
> xen/include/asm-x86/hvm/svm/emulate.h | 58 ++++++++---------
> 4 files changed, 88 insertions(+), 134 deletions(-)
Now that I've got maintainer acks, could I get a view to 4.12 release
ack please? This improves diagnostics for a real issue we discovered
during the 4.12 development cycle.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 2/3] x86/svm: Drop enum instruction_index and simplify svm_get_insn_len()
2019-01-07 10:30 ` Jan Beulich
@ 2019-01-31 18:07 ` Andrew Cooper
2019-02-01 7:49 ` Jan Beulich
0 siblings, 1 reply; 16+ messages in thread
From: Andrew Cooper @ 2019-01-31 18:07 UTC (permalink / raw)
To: Jan Beulich
Cc: Wei Liu, Xen-devel, Suravee Suthikulpanit, Boris Ostrovsky,
Brian Woods, Roger Pau Monne
On 07/01/2019 10:30, Jan Beulich wrote:
>>>> On 31.12.18 at 12:37, <andrew.cooper3@citrix.com> 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 at most 24 bits, which leaves room
>> for a modrm byte.
> That's true for the 0x0f-prefix-space insns (and it's just 20 bits in that
> case), but going this route we'd paint ourselves into a corner if we'd
> ever have to add 0x0f38-, 0x0f3a-, or 0x8f0?-prefix-space insns.
We only need constants here for instructions which have intercepts. I
doubt any SIMD instructions are going to enter that category, but we can
always reconsider the encoding scheme (probably to unsigned long) if
this becomes an issue.
> Furthermore someone adjusting the encoding layout in x86_emulate.h
> is very unlikely to notice breakage here until trying the resulting
> binary - I strongly think some BUILD_BUG_ON() should be added to
> make this apparent at build time.
It turns out that BUILD_BUG_ON() doesn't work, because the macro
truncates at 32 bits due to types.
Using
#define INSTR_PAUSE INSTR_ENC(X86EMUL_OPC_F3(0l, 0x90), 0)
i.e. using a long constant for for the ext field does end up triggering
-Woverflow when I artificially set a high bit inside X86EMUL_OPC()
However, this isn't viable protection, as internal changes in
X86EMUL_OPC() would render it useless.
Given that a build time check is proving complicated, and that encoding
errors will be blindingly obvious in debug builds (as the diagnostics
will trigger and we'll hand #GP to the guest), and that it is unlikely
that we're going to vastly change the encoding scheme, I think it will
be fine to go without a build-time check.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH for-4.12 v4 2/3] x86/svm: Drop enum instruction_index and simplify svm_get_insn_len()
2018-12-31 11:37 ` [PATCH v3 2/3] x86/svm: Drop enum instruction_index and simplify svm_get_insn_len() Andrew Cooper
2018-12-31 11:57 ` Andrew Cooper
2019-01-07 10:30 ` Jan Beulich
@ 2019-01-31 18:24 ` Andrew Cooper
2019-01-31 18:56 ` Woods, Brian
2019-02-01 9:31 ` Jan Beulich
2 siblings, 2 replies; 16+ messages in thread
From: Andrew Cooper @ 2019-01-31 18:24 UTC (permalink / raw)
To: Xen-devel
Cc: Juergen Gross, Wei Liu, Jan Beulich, Andrew Cooper,
Suravee Suthikulpanit, Boris Ostrovsky, Brian Woods,
Roger Pau Monné
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@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Brian Woods <brian.woods@amd.com>
CC: Juergen Gross <jgross@suse.com>
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__ */
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH for-4.12 v4 2/3] x86/svm: Drop enum instruction_index and simplify svm_get_insn_len()
2019-01-31 18:24 ` [PATCH for-4.12 v4 " Andrew Cooper
@ 2019-01-31 18:56 ` Woods, Brian
2019-02-01 9:31 ` Jan Beulich
1 sibling, 0 replies; 16+ messages in thread
From: Woods, Brian @ 2019-01-31 18:56 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel
Cc: Juergen Gross, Wei Liu, Jan Beulich, Suthikulpanit, Suravee,
Boris Ostrovsky, Roger Pau Monné
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@citrix.com>
Acked-by: Brian Woods <brian.woods@amd.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Brian Woods <brian.woods@amd.com>
> CC: Juergen Gross <jgross@suse.com>
>
> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 0/3] x86/svm: Improvements to SVM instruction length handling
2019-01-31 16:56 ` [PATCH v3 0/3] x86/svm: Improvements to SVM instruction length handling Andrew Cooper
@ 2019-02-01 6:05 ` Juergen Gross
0 siblings, 0 replies; 16+ messages in thread
From: Juergen Gross @ 2019-02-01 6:05 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel
On 31/01/2019 17:56, Andrew Cooper wrote:
> On 31/12/2018 11:37, Andrew Cooper wrote:
>> The main bugfix in v2 of this series has now been committed, leaving just the
>> cleanup remaining. See patches for details.
>>
>> Andrew Cooper (3):
>> x86/svm: Remove list functionality from __get_instruction_length_* infrastructure
>> x86/svm: Drop enum instruction_index and simplify svm_get_insn_len()
>> x86/svm: Improve diagnostics when svm_get_insn_len() fails
>>
>> xen/arch/x86/hvm/svm/emulate.c | 116 +++++++++++-----------------------
>> xen/arch/x86/hvm/svm/nestedsvm.c | 9 +--
>> xen/arch/x86/hvm/svm/svm.c | 39 ++++++------
>> xen/include/asm-x86/hvm/svm/emulate.h | 58 ++++++++---------
>> 4 files changed, 88 insertions(+), 134 deletions(-)
>
> Now that I've got maintainer acks, could I get a view to 4.12 release
> ack please? This improves diagnostics for a real issue we discovered
> during the 4.12 development cycle.
For the series:
Release-acked-by: Juergen Gross <jgross@suse.com>
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 2/3] x86/svm: Drop enum instruction_index and simplify svm_get_insn_len()
2019-01-31 18:07 ` Andrew Cooper
@ 2019-02-01 7:49 ` Jan Beulich
0 siblings, 0 replies; 16+ messages in thread
From: Jan Beulich @ 2019-02-01 7:49 UTC (permalink / raw)
To: Andrew Cooper
Cc: Wei Liu, Xen-devel, Suravee Suthikulpanit, Boris Ostrovsky,
Brian Woods, Roger Pau Monne
>>> On 31.01.19 at 19:07, <andrew.cooper3@citrix.com> wrote:
> On 07/01/2019 10:30, Jan Beulich wrote:
>>>>> On 31.12.18 at 12:37, <andrew.cooper3@citrix.com> 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 at most 24 bits, which leaves room
>>> for a modrm byte.
>> That's true for the 0x0f-prefix-space insns (and it's just 20 bits in that
>> case), but going this route we'd paint ourselves into a corner if we'd
>> ever have to add 0x0f38-, 0x0f3a-, or 0x8f0?-prefix-space insns.
>
> We only need constants here for instructions which have intercepts. I
> doubt any SIMD instructions are going to enter that category, but we can
> always reconsider the encoding scheme (probably to unsigned long) if
> this becomes an issue.
Okay then. Nevetheless one reference point regarding this: Recall
that not all insns in these two opcode spaces are SIMD. And as an
aside recall also that insns like {LD,ST}MXCSR are only half-way SIMD,
but have VEX encoded counterparts. I'm also not sure whether you'd
call {,F}X{SAVE,RSTOR} SIMD insns, yet XSAVES does have an
intercept (of course without exceeding the 24-bit encoding scheme).
>> Furthermore someone adjusting the encoding layout in x86_emulate.h
>> is very unlikely to notice breakage here until trying the resulting
>> binary - I strongly think some BUILD_BUG_ON() should be added to
>> make this apparent at build time.
>
> It turns out that BUILD_BUG_ON() doesn't work, because the macro
> truncates at 32 bits due to types.
>
> Using
>
> #define INSTR_PAUSE INSTR_ENC(X86EMUL_OPC_F3(0l, 0x90), 0)
>
> i.e. using a long constant for for the ext field does end up triggering
> -Woverflow when I artificially set a high bit inside X86EMUL_OPC()
> However, this isn't viable protection, as internal changes in
> X86EMUL_OPC() would render it useless.
>
>
> Given that a build time check is proving complicated, and that encoding
> errors will be blindingly obvious in debug builds (as the diagnostics
> will trigger and we'll hand #GP to the guest), and that it is unlikely
> that we're going to vastly change the encoding scheme, I think it will
> be fine to go without a build-time check.
Well, okay then.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH for-4.12 v4 2/3] x86/svm: Drop enum instruction_index and simplify svm_get_insn_len()
2019-01-31 18:24 ` [PATCH for-4.12 v4 " Andrew Cooper
2019-01-31 18:56 ` Woods, Brian
@ 2019-02-01 9:31 ` Jan Beulich
1 sibling, 0 replies; 16+ messages in thread
From: Jan Beulich @ 2019-02-01 9:31 UTC (permalink / raw)
To: Andrew Cooper
Cc: Juergen Gross, Wei Liu, Xen-devel, Suravee Suthikulpanit,
Boris Ostrovsky, Brian Woods, Roger Pau Monne
>>> On 31.01.19 at 19:24, <andrew.cooper3@citrix.com> 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@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2019-02-01 9:31 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-31 11:37 [PATCH v3 0/3] x86/svm: Improvements to SVM instruction length handling Andrew Cooper
2018-12-31 11:37 ` [PATCH v3 1/3] x86/svm: Remove list functionality from __get_instruction_length_* infrastructure Andrew Cooper
2019-01-31 16:42 ` Woods, Brian
2018-12-31 11:37 ` [PATCH v3 2/3] x86/svm: Drop enum instruction_index and simplify svm_get_insn_len() Andrew Cooper
2018-12-31 11:57 ` Andrew Cooper
2019-01-04 10:30 ` Jan Beulich
2019-01-07 10:30 ` Jan Beulich
2019-01-31 18:07 ` Andrew Cooper
2019-02-01 7:49 ` Jan Beulich
2019-01-31 18:24 ` [PATCH for-4.12 v4 " Andrew Cooper
2019-01-31 18:56 ` Woods, Brian
2019-02-01 9:31 ` Jan Beulich
2018-12-31 11:37 ` [PATCH v3 3/3] x86/svm: Improve diagnostics when svm_get_insn_len() fails Andrew Cooper
2019-01-03 9:01 ` Paul Durrant
2019-01-31 16:56 ` [PATCH v3 0/3] x86/svm: Improvements to SVM instruction length handling Andrew Cooper
2019-02-01 6:05 ` Juergen Gross
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.