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