Re: [Xen-devel] [PATCH v4] hvm/svm: Implement Debug events

[Jan Beulich]

>>> On 22.03.18 at 11:46,  wrote:
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -172,6 +172,24 @@ static void svm_enable_msr_interception(struct domain 
> *d, uint32_t msr)
>  svm_intercept_msr(v, msr, MSR_INTERCEPT_WRITE);
>  }
>  
> +static void svm_set_icebp_interception(struct domain *d, bool enable)
> +{
> +struct vcpu *v;

While I agree that the hook's parameter would better not be a
pointer to const, the local variable here surely should be.

> @@ -2656,9 +2663,28 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>  HVMTRACE_0D(SMI);
>  break;
>  
> +case VMEXIT_ICEBP:
>  case VMEXIT_EXCEPTION_DB:
>  if ( !v->domain->debugger_attached )
> -hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
> +{
> +int rc;
> +unsigned int trap_type = exit_reason == VMEXIT_ICEBP ?
> +X86_EVENTTYPE_PRI_SW_EXCEPTION : X86_EVENTTYPE_HW_EXCEPTION;
> +
> +inst_len = 0;
> +
> +if ( trap_type >= X86_EVENTTYPE_SW_INTERRUPT )
> +inst_len = __get_instruction_length(v, INSTR_ICEBP);

>= (other == ) implies more than a single type is covered. How
does that fit with passing the unique INSTR_ICEBP to the function?
I don't see the point anyway to set the type to one of two
possible values and then compare against a third. Things would
likely quite a bit more obvious if you had an if/else pair and did
both type and insn len assignments separately for each case.

> @@ -581,6 +596,16 @@ static inline bool_t hvm_enable_msr_interception(struct 
> domain *d, uint32_t msr)
>  return 0;
>  }
>  
> +static inline bool hvm_set_icebp_interception(struct domain *d, bool enable)
> +{
> +if( hvm_funcs.set_icebp_interception )

Contrary to what your revision log says, there's still a style issue
here plus ...

> +{
> +hvm_funcs.set_icebp_interception(d, enable);
> +return 1;

... true here and ...

> +}
> +return 0;

... false here.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v4] hvm/svm: Implement Debug events

[Alexandru Isaila]

At this moment the Debug events for the AMD architecture are not
forwarded to the monitor layer.

This patch adds the Debug event to the common capabilities, adds
the VMEXIT_ICEBP then forwards the event to the monitor layer.

Chapter 2: SVM Processor and Platform Extensions: "Note: A vector 1
exception generated by the single byte INT1
instruction (also known as ICEBP) does not trigger the #DB
intercept. Software should use the dedicated ICEBP
intercept to intercept ICEBP"

Signed-off-by: Alexandru Isaila 

---
Changes since V3:
- Merge disable/enable hooks into set_icebp_interception
- Address style comments.
---
 xen/arch/x86/hvm/svm/emulate.c|  1 +
 xen/arch/x86/hvm/svm/svm.c| 60 +--
 xen/arch/x86/monitor.c|  3 ++
 xen/include/asm-x86/hvm/hvm.h | 25 +++
 xen/include/asm-x86/hvm/svm/emulate.h |  1 +
 xen/include/asm-x86/monitor.h |  4 +--
 6 files changed, 76 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/emulate.c b/xen/arch/x86/hvm/svm/emulate.c
index e1a1581..535674e 100644
--- a/xen/arch/x86/hvm/svm/emulate.c
+++ b/xen/arch/x86/hvm/svm/emulate.c
@@ -65,6 +65,7 @@ static const struct {
 } 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) },
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index c34f5b5..affd8da 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -172,6 +172,24 @@ static void svm_enable_msr_interception(struct domain *d, 
uint32_t msr)
 svm_intercept_msr(v, msr, MSR_INTERCEPT_WRITE);
 }
 
+static void svm_set_icebp_interception(struct domain *d, bool enable)
+{
+struct vcpu *v;
+
+for_each_vcpu ( d, v )
+{
+struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
+uint32_t intercepts = vmcb_get_general2_intercepts(vmcb);
+
+if ( enable )
+intercepts |= GENERAL2_INTERCEPT_ICEBP;
+else
+intercepts &= ~GENERAL2_INTERCEPT_ICEBP;
+
+vmcb_set_general2_intercepts(vmcb, intercepts);
+}
+}
+
 static void svm_save_dr(struct vcpu *v)
 {
 struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
@@ -1109,7 +1127,8 @@ static void noreturn svm_do_resume(struct vcpu *v)
 {
 struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
 bool debug_state = (v->domain->debugger_attached ||
-v->domain->arch.monitor.software_breakpoint_enabled);
+v->domain->arch.monitor.software_breakpoint_enabled ||
+v->domain->arch.monitor.debug_exception_enabled);
 bool_t vcpu_guestmode = 0;
 struct vlapic *vlapic = vcpu_vlapic(v);
 
@@ -2438,19 +2457,6 @@ static bool svm_get_pending_event(struct vcpu *v, struct 
x86_event *info)
 return true;
 }
 
-static void svm_propagate_intr(struct vcpu *v, unsigned long insn_len)
-{
-struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
-struct x86_event event = {
-.vector = vmcb->eventinj.fields.type,
-.type = vmcb->eventinj.fields.type,
-.error_code = vmcb->exitinfo1,
-};
-
-event.insn_len = insn_len;
-hvm_inject_event();
-}
-
 static struct hvm_function_table __initdata svm_function_table = {
 .name = "SVM",
 .cpu_up_prepare   = svm_cpu_up_prepare,
@@ -2490,6 +2496,7 @@ static struct hvm_function_table __initdata 
svm_function_table = {
 .msr_read_intercept   = svm_msr_read_intercept,
 .msr_write_intercept  = svm_msr_write_intercept,
 .enable_msr_interception = svm_enable_msr_interception,
+.set_icebp_interception = svm_set_icebp_interception,
 .set_rdtsc_exiting= svm_set_rdtsc_exiting,
 .set_descriptor_access_exiting = svm_set_descriptor_access_exiting,
 .get_insn_bytes   = svm_get_insn_bytes,
@@ -2656,9 +2663,28 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
 HVMTRACE_0D(SMI);
 break;
 
+case VMEXIT_ICEBP:
 case VMEXIT_EXCEPTION_DB:
 if ( !v->domain->debugger_attached )
-hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
+{
+int rc;
+unsigned int trap_type = exit_reason == VMEXIT_ICEBP ?
+X86_EVENTTYPE_PRI_SW_EXCEPTION : X86_EVENTTYPE_HW_EXCEPTION;
+
+inst_len = 0;
+
+if ( trap_type >= X86_EVENTTYPE_SW_INTERRUPT )
+inst_len = __get_instruction_length(v, INSTR_ICEBP);
+
+rc = hvm_monitor_debug(regs->rip,
+   HVM_MONITOR_DEBUG_EXCEPTION,
+   trap_type, inst_len);
+if ( rc < 0 )
+