* [PATCH] VMX: fix/adjust trap injection
@ 2015-11-23 12:49 Jan Beulich
2015-11-23 15:01 ` Andrew Cooper
2015-11-24 5:02 ` Tian, Kevin
0 siblings, 2 replies; 5+ messages in thread
From: Jan Beulich @ 2015-11-23 12:49 UTC (permalink / raw)
To: xen-devel; +Cc: Kevin Tian, Jun Nakajima
[-- Attachment #1: Type: text/plain, Size: 3089 bytes --]
In the course of investigating the 4.1.6 backport issue of the XSA-156
patch I realized that #DB injection has always been broken, but with it
now getting always intercepted the problem has got worse: Documentation
clearly states that neither DR7.GD nor DebugCtl.LBR get cleared before
the intercept, so this is something we need to do before reflecting the
intercepted exception.
While adjusting this (and also with 4.1.6's strange use of
X86_EVENTTYPE_SW_EXCEPTION for #DB in mind) I further realized that
the special casing of individual vectors shouldn't be done for
software interrupts (resulting from INT $nn).
And then some code movement: Setting of CR2 for #PF can be done in the
same switch() statement (no need for a separate if()), and reading of
intr_info is better done close the the consumption of the variable
(allowing the compiler to generate better code / use fewer registers
for variables).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1508,16 +1508,7 @@ static void vmx_inject_trap(struct hvm_t
struct vcpu *curr = current;
struct hvm_trap _trap = *trap;
- if ( (_trap.vector == TRAP_page_fault) &&
- (_trap.type == X86_EVENTTYPE_HW_EXCEPTION) )
- curr->arch.hvm_vcpu.guest_cr[2] = _trap.cr2;
-
- if ( nestedhvm_vcpu_in_guestmode(curr) )
- intr_info = vcpu_2_nvmx(curr).intr.intr_info;
- else
- __vmread(VM_ENTRY_INTR_INFO, &intr_info);
-
- switch ( _trap.vector )
+ switch ( _trap.vector | -(_trap.type == X86_EVENTTYPE_SW_INTERRUPT) )
{
case TRAP_debug:
if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
@@ -1525,6 +1516,16 @@ static void vmx_inject_trap(struct hvm_t
__restore_debug_registers(curr);
write_debugreg(6, read_debugreg(6) | DR_STEP);
}
+ if ( !nestedhvm_vcpu_in_guestmode(curr) ||
+ !nvmx_intercepts_exception(curr, TRAP_debug, _trap.error_code) )
+ {
+ unsigned long val;
+
+ __vmread(GUEST_DR7, &val);
+ __vmwrite(GUEST_DR7, val & ~DR_GENERAL_DETECT);
+ __vmread(GUEST_IA32_DEBUGCTL, &val);
+ __vmwrite(GUEST_IA32_DEBUGCTL, val & ~IA32_DEBUGCTLMSR_LBR);
+ }
if ( cpu_has_monitor_trap_flag )
break;
/* fall through */
@@ -1535,8 +1536,19 @@ static void vmx_inject_trap(struct hvm_t
domain_pause_for_debugger();
return;
}
+ break;
+
+ case TRAP_page_fault:
+ ASSERT(_trap.type == X86_EVENTTYPE_HW_EXCEPTION);
+ curr->arch.hvm_vcpu.guest_cr[2] = _trap.cr2;
+ break;
}
+ if ( nestedhvm_vcpu_in_guestmode(curr) )
+ intr_info = vcpu_2_nvmx(curr).intr.intr_info;
+ else
+ __vmread(VM_ENTRY_INTR_INFO, &intr_info);
+
if ( unlikely(intr_info & INTR_INFO_VALID_MASK) &&
(MASK_EXTR(intr_info, INTR_INFO_INTR_TYPE_MASK) ==
X86_EVENTTYPE_HW_EXCEPTION) )
[-- Attachment #2: VMX-adjust-regs-on-#DB.patch --]
[-- Type: text/plain, Size: 3117 bytes --]
VMX: fix/adjust trap injection
In the course of investigating the 4.1.6 backport issue of the XSA-156
patch I realized that #DB injection has always been broken, but with it
now getting always intercepted the problem has got worse: Documentation
clearly states that neither DR7.GD nor DebugCtl.LBR get cleared before
the intercept, so this is something we need to do before reflecting the
intercepted exception.
While adjusting this (and also with 4.1.6's strange use of
X86_EVENTTYPE_SW_EXCEPTION for #DB in mind) I further realized that
the special casing of individual vectors shouldn't be done for
software interrupts (resulting from INT $nn).
And then some code movement: Setting of CR2 for #PF can be done in the
same switch() statement (no need for a separate if()), and reading of
intr_info is better done close the the consumption of the variable
(allowing the compiler to generate better code / use fewer registers
for variables).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1508,16 +1508,7 @@ static void vmx_inject_trap(struct hvm_t
struct vcpu *curr = current;
struct hvm_trap _trap = *trap;
- if ( (_trap.vector == TRAP_page_fault) &&
- (_trap.type == X86_EVENTTYPE_HW_EXCEPTION) )
- curr->arch.hvm_vcpu.guest_cr[2] = _trap.cr2;
-
- if ( nestedhvm_vcpu_in_guestmode(curr) )
- intr_info = vcpu_2_nvmx(curr).intr.intr_info;
- else
- __vmread(VM_ENTRY_INTR_INFO, &intr_info);
-
- switch ( _trap.vector )
+ switch ( _trap.vector | -(_trap.type == X86_EVENTTYPE_SW_INTERRUPT) )
{
case TRAP_debug:
if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
@@ -1525,6 +1516,16 @@ static void vmx_inject_trap(struct hvm_t
__restore_debug_registers(curr);
write_debugreg(6, read_debugreg(6) | DR_STEP);
}
+ if ( !nestedhvm_vcpu_in_guestmode(curr) ||
+ !nvmx_intercepts_exception(curr, TRAP_debug, _trap.error_code) )
+ {
+ unsigned long val;
+
+ __vmread(GUEST_DR7, &val);
+ __vmwrite(GUEST_DR7, val & ~DR_GENERAL_DETECT);
+ __vmread(GUEST_IA32_DEBUGCTL, &val);
+ __vmwrite(GUEST_IA32_DEBUGCTL, val & ~IA32_DEBUGCTLMSR_LBR);
+ }
if ( cpu_has_monitor_trap_flag )
break;
/* fall through */
@@ -1535,8 +1536,19 @@ static void vmx_inject_trap(struct hvm_t
domain_pause_for_debugger();
return;
}
+ break;
+
+ case TRAP_page_fault:
+ ASSERT(_trap.type == X86_EVENTTYPE_HW_EXCEPTION);
+ curr->arch.hvm_vcpu.guest_cr[2] = _trap.cr2;
+ break;
}
+ if ( nestedhvm_vcpu_in_guestmode(curr) )
+ intr_info = vcpu_2_nvmx(curr).intr.intr_info;
+ else
+ __vmread(VM_ENTRY_INTR_INFO, &intr_info);
+
if ( unlikely(intr_info & INTR_INFO_VALID_MASK) &&
(MASK_EXTR(intr_info, INTR_INFO_INTR_TYPE_MASK) ==
X86_EVENTTYPE_HW_EXCEPTION) )
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] VMX: fix/adjust trap injection
2015-11-23 12:49 [PATCH] VMX: fix/adjust trap injection Jan Beulich
@ 2015-11-23 15:01 ` Andrew Cooper
2015-11-23 15:24 ` Jan Beulich
2015-11-24 5:02 ` Tian, Kevin
1 sibling, 1 reply; 5+ messages in thread
From: Andrew Cooper @ 2015-11-23 15:01 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Kevin Tian, Jun Nakajima
On 23/11/15 12:49, Jan Beulich wrote:
> In the course of investigating the 4.1.6 backport issue of the XSA-156
> patch I realized that #DB injection has always been broken, but with it
> now getting always intercepted the problem has got worse: Documentation
> clearly states that neither DR7.GD nor DebugCtl.LBR get cleared before
> the intercept, so this is something we need to do before reflecting the
> intercepted exception.
>
> While adjusting this (and also with 4.1.6's strange use of
> X86_EVENTTYPE_SW_EXCEPTION for #DB in mind) I further realized that
> the special casing of individual vectors shouldn't be done for
> software interrupts (resulting from INT $nn).
>
> And then some code movement: Setting of CR2 for #PF can be done in the
> same switch() statement (no need for a separate if()), and reading of
> intr_info is better done close the the consumption of the variable
> (allowing the compiler to generate better code / use fewer registers
> for variables).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1508,16 +1508,7 @@ static void vmx_inject_trap(struct hvm_t
> struct vcpu *curr = current;
> struct hvm_trap _trap = *trap;
>
> - if ( (_trap.vector == TRAP_page_fault) &&
> - (_trap.type == X86_EVENTTYPE_HW_EXCEPTION) )
> - curr->arch.hvm_vcpu.guest_cr[2] = _trap.cr2;
> -
> - if ( nestedhvm_vcpu_in_guestmode(curr) )
> - intr_info = vcpu_2_nvmx(curr).intr.intr_info;
> - else
> - __vmread(VM_ENTRY_INTR_INFO, &intr_info);
> -
> - switch ( _trap.vector )
> + switch ( _trap.vector | -(_trap.type == X86_EVENTTYPE_SW_INTERRUPT) )
> {
> case TRAP_debug:
> if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
> @@ -1525,6 +1516,16 @@ static void vmx_inject_trap(struct hvm_t
> __restore_debug_registers(curr);
> write_debugreg(6, read_debugreg(6) | DR_STEP);
> }
> + if ( !nestedhvm_vcpu_in_guestmode(curr) ||
> + !nvmx_intercepts_exception(curr, TRAP_debug, _trap.error_code) )
> + {
> + unsigned long val;
> +
> + __vmread(GUEST_DR7, &val);
> + __vmwrite(GUEST_DR7, val & ~DR_GENERAL_DETECT);
> + __vmread(GUEST_IA32_DEBUGCTL, &val);
> + __vmwrite(GUEST_IA32_DEBUGCTL, val & ~IA32_DEBUGCTLMSR_LBR);
Is it worth eliding the __vmwrite's if possible? It will be fewer
VMexits if this Xen is running nested, and ISTR it will avoid slowing
down the vmentry with further consistency checks.
~Andrew
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] VMX: fix/adjust trap injection
2015-11-23 15:01 ` Andrew Cooper
@ 2015-11-23 15:24 ` Jan Beulich
2015-11-24 11:15 ` Andrew Cooper
0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2015-11-23 15:24 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Kevin Tian, Jun Nakajima
>>> On 23.11.15 at 16:01, <andrew.cooper3@citrix.com> wrote:
> On 23/11/15 12:49, Jan Beulich wrote:
>> @@ -1525,6 +1516,16 @@ static void vmx_inject_trap(struct hvm_t
>> __restore_debug_registers(curr);
>> write_debugreg(6, read_debugreg(6) | DR_STEP);
>> }
>> + if ( !nestedhvm_vcpu_in_guestmode(curr) ||
>> + !nvmx_intercepts_exception(curr, TRAP_debug, _trap.error_code) )
>> + {
>> + unsigned long val;
>> +
>> + __vmread(GUEST_DR7, &val);
>> + __vmwrite(GUEST_DR7, val & ~DR_GENERAL_DETECT);
>> + __vmread(GUEST_IA32_DEBUGCTL, &val);
>> + __vmwrite(GUEST_IA32_DEBUGCTL, val & ~IA32_DEBUGCTLMSR_LBR);
>
> Is it worth eliding the __vmwrite's if possible? It will be fewer
> VMexits if this Xen is running nested, and ISTR it will avoid slowing
> down the vmentry with further consistency checks.
I've considered this, but then decided #DB shouldn't really be
raised on any fast paths (or such paths become non-fast anyway
due to the many exceptions). The code certainly is easier to read
without further conditionals.
Jan
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] VMX: fix/adjust trap injection
2015-11-23 15:24 ` Jan Beulich
@ 2015-11-24 11:15 ` Andrew Cooper
0 siblings, 0 replies; 5+ messages in thread
From: Andrew Cooper @ 2015-11-24 11:15 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Kevin Tian, Jun Nakajima
On 23/11/15 15:24, Jan Beulich wrote:
>>>> On 23.11.15 at 16:01, <andrew.cooper3@citrix.com> wrote:
>> On 23/11/15 12:49, Jan Beulich wrote:
>>> @@ -1525,6 +1516,16 @@ static void vmx_inject_trap(struct hvm_t
>>> __restore_debug_registers(curr);
>>> write_debugreg(6, read_debugreg(6) | DR_STEP);
>>> }
>>> + if ( !nestedhvm_vcpu_in_guestmode(curr) ||
>>> + !nvmx_intercepts_exception(curr, TRAP_debug, _trap.error_code) )
>>> + {
>>> + unsigned long val;
>>> +
>>> + __vmread(GUEST_DR7, &val);
>>> + __vmwrite(GUEST_DR7, val & ~DR_GENERAL_DETECT);
>>> + __vmread(GUEST_IA32_DEBUGCTL, &val);
>>> + __vmwrite(GUEST_IA32_DEBUGCTL, val & ~IA32_DEBUGCTLMSR_LBR);
>> Is it worth eliding the __vmwrite's if possible? It will be fewer
>> VMexits if this Xen is running nested, and ISTR it will avoid slowing
>> down the vmentry with further consistency checks.
> I've considered this, but then decided #DB shouldn't really be
> raised on any fast paths (or such paths become non-fast anyway
> due to the many exceptions). The code certainly is easier to read
> without further conditionals.
Ok.
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] VMX: fix/adjust trap injection
2015-11-23 12:49 [PATCH] VMX: fix/adjust trap injection Jan Beulich
2015-11-23 15:01 ` Andrew Cooper
@ 2015-11-24 5:02 ` Tian, Kevin
1 sibling, 0 replies; 5+ messages in thread
From: Tian, Kevin @ 2015-11-24 5:02 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Nakajima, Jun
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Monday, November 23, 2015 8:49 PM
>
> In the course of investigating the 4.1.6 backport issue of the XSA-156
> patch I realized that #DB injection has always been broken, but with it
> now getting always intercepted the problem has got worse: Documentation
> clearly states that neither DR7.GD nor DebugCtl.LBR get cleared before
> the intercept, so this is something we need to do before reflecting the
> intercepted exception.
>
> While adjusting this (and also with 4.1.6's strange use of
> X86_EVENTTYPE_SW_EXCEPTION for #DB in mind) I further realized that
> the special casing of individual vectors shouldn't be done for
> software interrupts (resulting from INT $nn).
>
> And then some code movement: Setting of CR2 for #PF can be done in the
> same switch() statement (no need for a separate if()), and reading of
> intr_info is better done close the the consumption of the variable
> (allowing the compiler to generate better code / use fewer registers
> for variables).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
Acked-by: Kevin Tian <kevin.tian@intel.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-11-24 11:15 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-11-23 12:49 [PATCH] VMX: fix/adjust trap injection Jan Beulich
2015-11-23 15:01 ` Andrew Cooper
2015-11-23 15:24 ` Jan Beulich
2015-11-24 11:15 ` Andrew Cooper
2015-11-24 5:02 ` Tian, Kevin
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.