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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 20/03/18 16:49, Tamas K Lengyel wrote:
> On Tue, Mar 20, 2018 at 3:40 AM, Alexandru Isaila
> <aisaila@xxxxxxxxxxxxxxx> wrote:
>> 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"
>>
>> ---
>> Changes since V1:
>>         - Get inst_len from __get_instruction_length()
>>         - Updated __get_instruction_length() for the INSTR_ICEBP
>>           instruction
>>
>> Signed-off-by: Alexandru Isaila <aisaila@xxxxxxxxxxxxxxx>
>> ---
>>  xen/arch/x86/hvm/svm/emulate.c        |  1 +
>>  xen/arch/x86/hvm/svm/svm.c            | 37 
>> +++++++++++++++++++++++++----------
>>  xen/arch/x86/hvm/svm/vmcb.c           |  2 +-
>>  xen/include/asm-x86/hvm/svm/emulate.h |  1 +
>>  xen/include/asm-x86/monitor.h         |  4 ++--
>>  5 files changed, 32 insertions(+), 13 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/svm/emulate.c b/xen/arch/x86/hvm/svm/emulate.c
>> index e1a1581..172369e 100644
>> --- a/xen/arch/x86/hvm/svm/emulate.c
>> +++ b/xen/arch/x86/hvm/svm/emulate.c
>> @@ -80,6 +80,7 @@ static const struct {
>>      [INSTR_RDTSC]   = { X86EMUL_OPC(0x0f, 0x31) },
>>      [INSTR_RDMSR]   = { X86EMUL_OPC(0x0f, 0x32) },
>>      [INSTR_CPUID]   = { X86EMUL_OPC(0x0f, 0xa2) },
>> +    [INSTR_ICEBP]   = { X86EMUL_OPC(   0, 0xf1) },
>>  };
>>
>>  int __get_instruction_length_from_list(struct vcpu *v,
>> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
>> index c34f5b5..d4f2290 100644
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -1109,7 +1109,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);
> Please explain the rationale why this is included under
> "debug_exception" and not "software_breakpoint".

FTR (as I haven't commented on this aspect yet), I think it is important
that however it is classified, it behaves the same on different
hardware.  i.e. monitoring of ICEBP is reported consistently between
Intel and AMD.

Beyond that, I'm not fussed exactly how it is classified. 
Fundamentally, it is a software breakpoint because it is (only) a real
usable instruction, but in reality, the only time you'll ever see it is
test code, malware attempting to exploit hypervisor vulnerabilities, or
when someone really has got an ICE hooked up to a real system (at which
point it gets intercepted in the lower levels of the CPU and disappears
behind our backs).

~Andrew

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