* RE: [PATCH] Don't enable irq for machine check vmexit
@ 2010-02-01 9:18 Jiang, Yunhong
2010-02-01 11:14 ` Keir Fraser
2010-02-04 9:26 ` Jiang, Yunhong
0 siblings, 2 replies; 14+ messages in thread
From: Jiang, Yunhong @ 2010-02-01 9:18 UTC (permalink / raw)
To: Jiang, Yunhong, Keir Fraser, Tim.Deegan@citrix.com
Cc: xen-devel@lists.xensource.com
[-- Attachment #1: Type: text/plain, Size: 2658 bytes --]
Sorry forgot the patch.
--jyh
diff -r 857d7b2dd8c7 xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c Fri Jan 29 08:59:46 2010 +0000
+++ b/xen/arch/x86/hvm/vmx/vmx.c Sun Jan 31 18:40:34 2010 +0800
@@ -2153,6 +2153,7 @@ static void vmx_failed_vmentry(unsigned
printk("caused by machine check.\n");
HVMTRACE_0D(MCE);
do_machine_check(regs);
+ local_irq_enable();
break;
default:
printk("reason not known yet!");
@@ -2243,6 +2244,23 @@ err:
err:
vmx_inject_hw_exception(TRAP_gp_fault, 0);
return -1;
+}
+
+int vmx_mce_exit(int exit_reason)
+{
+ if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY &&
+ (uint16_t)exit_reason == EXIT_REASON_MCE_DURING_VMENTRY) )
+ return 1;
+ else if (unlikely(exit_reason == EXIT_REASON_EXCEPTION_NMI))
+ {
+ uint32_t vector;
+
+ vector = __vmread(VM_EXIT_INTR_INFO) & INTR_INFO_VECTOR_MASK;
+ if (vector == TRAP_machine_check)
+ return 1;
+ }
+
+ return 0;
}
asmlinkage void vmx_vmexit_handler(struct cpu_user_regs *regs)
@@ -2273,7 +2291,8 @@ asmlinkage void vmx_vmexit_handler(struc
vmx_do_extint(regs);
/* Now enable interrupts so it's safe to take locks. */
- local_irq_enable();
+ if ( !(vmx_mce_exit(exit_reason)) )
+ local_irq_enable();
if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) )
return vmx_failed_vmentry(exit_reason, regs);
@@ -2433,6 +2452,7 @@ asmlinkage void vmx_vmexit_handler(struc
case TRAP_machine_check:
HVMTRACE_0D(MCE);
do_machine_check(regs);
+ local_irq_enable();
break;
case TRAP_invalid_op:
vmx_vmexit_ud_intercept(regs);
>-----Original Message-----
>From: Jiang, Yunhong
>Sent: Monday, February 01, 2010 4:48 PM
>To: Keir Fraser; Tim.Deegan@citrix.com
>Cc: xen-devel@lists.xensource.com
>Subject: [PATCH] Don't enable irq for machine check vmexit
>
>We should not enable irq for machine check VMExit
>
>In changeset 18658:824892134573, IRQ is enabled during VMExit except external
>interrupt. The exception should apply for machine check also, because :
>a) The mce_logout_lock should be held in irq_disabled context.
>b) The machine check event should be handled as quickly as possible, enable irq will
>increase the period greatly.
>
>Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com>
>
>This is in hotspot code path, I try to use unlikely, hope to reduce the performance
>impact
>
>Thanks
>Yunhong Jiang
[-- Attachment #2: hvm_vmexit.patch --]
[-- Type: application/octet-stream, Size: 2183 bytes --]
We should not enable irq for machine check VMExit
In changeset 18658:824892134573, IRQ is enabled during VMExit except external interrupt. The exception should apply for machine check also, because :
a) The mce_logout_lock should be hold in irq_disabled context.
b) The machine check event should be handled as quickly as possible, enable irq will increase the period greatly.
Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com>
diff -r 857d7b2dd8c7 xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c Fri Jan 29 08:59:46 2010 +0000
+++ b/xen/arch/x86/hvm/vmx/vmx.c Sun Jan 31 18:40:34 2010 +0800
@@ -2153,6 +2153,7 @@ static void vmx_failed_vmentry(unsigned
printk("caused by machine check.\n");
HVMTRACE_0D(MCE);
do_machine_check(regs);
+ local_irq_enable();
break;
default:
printk("reason not known yet!");
@@ -2243,6 +2244,23 @@ err:
err:
vmx_inject_hw_exception(TRAP_gp_fault, 0);
return -1;
+}
+
+int vmx_mce_exit(int exit_reason)
+{
+ if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY &&
+ (uint16_t)exit_reason == EXIT_REASON_MCE_DURING_VMENTRY) )
+ return 1;
+ else if (unlikely(exit_reason == EXIT_REASON_EXCEPTION_NMI))
+ {
+ uint32_t vector;
+
+ vector = __vmread(VM_EXIT_INTR_INFO) & INTR_INFO_VECTOR_MASK;
+ if (vector == TRAP_machine_check)
+ return 1;
+ }
+
+ return 0;
}
asmlinkage void vmx_vmexit_handler(struct cpu_user_regs *regs)
@@ -2273,7 +2291,8 @@ asmlinkage void vmx_vmexit_handler(struc
vmx_do_extint(regs);
/* Now enable interrupts so it's safe to take locks. */
- local_irq_enable();
+ if ( !(vmx_mce_exit(exit_reason)) )
+ local_irq_enable();
if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) )
return vmx_failed_vmentry(exit_reason, regs);
@@ -2433,6 +2452,7 @@ asmlinkage void vmx_vmexit_handler(struc
case TRAP_machine_check:
HVMTRACE_0D(MCE);
do_machine_check(regs);
+ local_irq_enable();
break;
case TRAP_invalid_op:
vmx_vmexit_ud_intercept(regs);
[-- Attachment #3: Type: text/plain, Size: 138 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] Don't enable irq for machine check vmexit
2010-02-01 9:18 [PATCH] Don't enable irq for machine check vmexit Jiang, Yunhong
@ 2010-02-01 11:14 ` Keir Fraser
2010-02-04 9:26 ` Jiang, Yunhong
1 sibling, 0 replies; 14+ messages in thread
From: Keir Fraser @ 2010-02-01 11:14 UTC (permalink / raw)
To: Jiang, Yunhong, Tim Deegan; +Cc: xen-devel@lists.xensource.com
Take a look at changeset 18658: local_irq_enable() was made unconditional in
the vmexit handler function for a good reason!
If you make it conditional on !mce, then in the mce case you can crash in
any later function that acquires a spinlock: maybe_deassert_evtchn_irq(),
for example. You will crash because of taking a irq-unsafe spinlock with
irqs disabled. Kind of the opposite way round to the mce_logout_lock: so
your patch would fix acquisition of that lock but break others.
I'm afraid your only option is to hoist all the mce handling up to the same
place we handle extints. Take c/s 18658 as your template for what to do.
-- Keir
On 01/02/2010 09:18, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:
> Sorry forgot the patch.
>
> --jyh
>
> diff -r 857d7b2dd8c7 xen/arch/x86/hvm/vmx/vmx.c
> --- a/xen/arch/x86/hvm/vmx/vmx.c Fri Jan 29 08:59:46 2010 +0000
> +++ b/xen/arch/x86/hvm/vmx/vmx.c Sun Jan 31 18:40:34 2010 +0800
> @@ -2153,6 +2153,7 @@ static void vmx_failed_vmentry(unsigned
> printk("caused by machine check.\n");
> HVMTRACE_0D(MCE);
> do_machine_check(regs);
> + local_irq_enable();
> break;
> default:
> printk("reason not known yet!");
> @@ -2243,6 +2244,23 @@ err:
> err:
> vmx_inject_hw_exception(TRAP_gp_fault, 0);
> return -1;
> +}
> +
> +int vmx_mce_exit(int exit_reason)
> +{
> + if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY &&
> + (uint16_t)exit_reason == EXIT_REASON_MCE_DURING_VMENTRY) )
> + return 1;
> + else if (unlikely(exit_reason == EXIT_REASON_EXCEPTION_NMI))
> + {
> + uint32_t vector;
> +
> + vector = __vmread(VM_EXIT_INTR_INFO) & INTR_INFO_VECTOR_MASK;
> + if (vector == TRAP_machine_check)
> + return 1;
> + }
> +
> + return 0;
> }
>
> asmlinkage void vmx_vmexit_handler(struct cpu_user_regs *regs)
> @@ -2273,7 +2291,8 @@ asmlinkage void vmx_vmexit_handler(struc
> vmx_do_extint(regs);
>
> /* Now enable interrupts so it's safe to take locks. */
> - local_irq_enable();
> + if ( !(vmx_mce_exit(exit_reason)) )
> + local_irq_enable();
>
> if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) )
> return vmx_failed_vmentry(exit_reason, regs);
> @@ -2433,6 +2452,7 @@ asmlinkage void vmx_vmexit_handler(struc
> case TRAP_machine_check:
> HVMTRACE_0D(MCE);
> do_machine_check(regs);
> + local_irq_enable();
> break;
> case TRAP_invalid_op:
> vmx_vmexit_ud_intercept(regs);
>
>
>
>> -----Original Message-----
>> From: Jiang, Yunhong
>> Sent: Monday, February 01, 2010 4:48 PM
>> To: Keir Fraser; Tim.Deegan@citrix.com
>> Cc: xen-devel@lists.xensource.com
>> Subject: [PATCH] Don't enable irq for machine check vmexit
>>
>> We should not enable irq for machine check VMExit
>>
>> In changeset 18658:824892134573, IRQ is enabled during VMExit except external
>> interrupt. The exception should apply for machine check also, because :
>> a) The mce_logout_lock should be held in irq_disabled context.
>> b) The machine check event should be handled as quickly as possible, enable
>> irq will
>> increase the period greatly.
>>
>> Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com>
>>
>> This is in hotspot code path, I try to use unlikely, hope to reduce the
>> performance
>> impact
>>
>> Thanks
>> Yunhong Jiang
>
^ permalink raw reply [flat|nested] 14+ messages in thread* RE: [PATCH] Don't enable irq for machine check vmexit
2010-02-01 9:18 [PATCH] Don't enable irq for machine check vmexit Jiang, Yunhong
2010-02-01 11:14 ` Keir Fraser
@ 2010-02-04 9:26 ` Jiang, Yunhong
2010-02-04 10:03 ` Keir Fraser
1 sibling, 1 reply; 14+ messages in thread
From: Jiang, Yunhong @ 2010-02-04 9:26 UTC (permalink / raw)
To: Jiang, Yunhong, Keir Fraser, Tim.Deegan@citrix.com
Cc: xen-devel@lists.xensource.com
Keir, any comments to this patch?
Thanks
Yunhong Jiang
>-----Original Message-----
>From: Jiang, Yunhong
>Sent: Monday, February 01, 2010 5:18 PM
>To: Jiang, Yunhong; Keir Fraser; Tim.Deegan@citrix.com
>Cc: xen-devel@lists.xensource.com
>Subject: RE: [PATCH] Don't enable irq for machine check vmexit
>
>Sorry forgot the patch.
>
>--jyh
>
>diff -r 857d7b2dd8c7 xen/arch/x86/hvm/vmx/vmx.c
>--- a/xen/arch/x86/hvm/vmx/vmx.c Fri Jan 29 08:59:46 2010 +0000
>+++ b/xen/arch/x86/hvm/vmx/vmx.c Sun Jan 31 18:40:34 2010 +0800
>@@ -2153,6 +2153,7 @@ static void vmx_failed_vmentry(unsigned
> printk("caused by machine check.\n");
> HVMTRACE_0D(MCE);
> do_machine_check(regs);
>+ local_irq_enable();
> break;
> default:
> printk("reason not known yet!");
>@@ -2243,6 +2244,23 @@ err:
> err:
> vmx_inject_hw_exception(TRAP_gp_fault, 0);
> return -1;
>+}
>+
>+int vmx_mce_exit(int exit_reason)
>+{
>+ if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY &&
>+ (uint16_t)exit_reason == EXIT_REASON_MCE_DURING_VMENTRY) )
>+ return 1;
>+ else if (unlikely(exit_reason == EXIT_REASON_EXCEPTION_NMI))
>+ {
>+ uint32_t vector;
>+
>+ vector = __vmread(VM_EXIT_INTR_INFO) & INTR_INFO_VECTOR_MASK;
>+ if (vector == TRAP_machine_check)
>+ return 1;
>+ }
>+
>+ return 0;
> }
>
> asmlinkage void vmx_vmexit_handler(struct cpu_user_regs *regs)
>@@ -2273,7 +2291,8 @@ asmlinkage void vmx_vmexit_handler(struc
> vmx_do_extint(regs);
>
> /* Now enable interrupts so it's safe to take locks. */
>- local_irq_enable();
>+ if ( !(vmx_mce_exit(exit_reason)) )
>+ local_irq_enable();
>
> if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) )
> return vmx_failed_vmentry(exit_reason, regs);
>@@ -2433,6 +2452,7 @@ asmlinkage void vmx_vmexit_handler(struc
> case TRAP_machine_check:
> HVMTRACE_0D(MCE);
> do_machine_check(regs);
>+ local_irq_enable();
> break;
> case TRAP_invalid_op:
> vmx_vmexit_ud_intercept(regs);
>
>
>
>>-----Original Message-----
>>From: Jiang, Yunhong
>>Sent: Monday, February 01, 2010 4:48 PM
>>To: Keir Fraser; Tim.Deegan@citrix.com
>>Cc: xen-devel@lists.xensource.com
>>Subject: [PATCH] Don't enable irq for machine check vmexit
>>
>>We should not enable irq for machine check VMExit
>>
>>In changeset 18658:824892134573, IRQ is enabled during VMExit except external
>>interrupt. The exception should apply for machine check also, because :
>>a) The mce_logout_lock should be held in irq_disabled context.
>>b) The machine check event should be handled as quickly as possible, enable irq will
>>increase the period greatly.
>>
>>Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com>
>>
>>This is in hotspot code path, I try to use unlikely, hope to reduce the performance
>>impact
>>
>>Thanks
>>Yunhong Jiang
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] Don't enable irq for machine check vmexit
2010-02-04 9:26 ` Jiang, Yunhong
@ 2010-02-04 10:03 ` Keir Fraser
2010-02-04 12:25 ` Jiang, Yunhong
0 siblings, 1 reply; 14+ messages in thread
From: Keir Fraser @ 2010-02-04 10:03 UTC (permalink / raw)
To: Jiang, Yunhong, Tim Deegan; +Cc: xen-devel@lists.xensource.com
Yeah, I already replied to you on Monday. What I said was:
"""
Take a look at changeset 18658: local_irq_enable() was made unconditional in
the vmexit handler function for a good reason!
If you make it conditional on !mce, then in the mce case you can crash in
any later function that acquires a spinlock: maybe_deassert_evtchn_irq(),
for example. You will crash because of taking a irq-unsafe spinlock with
irqs disabled. Kind of the opposite way round to the mce_logout_lock: so
your patch would fix acquisition of that lock but break others.
I'm afraid your only option is to hoist all the mce handling up to the same
place we handle extints. Take c/s 18658 as your template for what to do.
"""
-- Keir
On 04/02/2010 09:26, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:
> Keir, any comments to this patch?
>
> Thanks
> Yunhong Jiang
>
>> -----Original Message-----
>> From: Jiang, Yunhong
>> Sent: Monday, February 01, 2010 5:18 PM
>> To: Jiang, Yunhong; Keir Fraser; Tim.Deegan@citrix.com
>> Cc: xen-devel@lists.xensource.com
>> Subject: RE: [PATCH] Don't enable irq for machine check vmexit
>>
>> Sorry forgot the patch.
>>
>> --jyh
>>
>> diff -r 857d7b2dd8c7 xen/arch/x86/hvm/vmx/vmx.c
>> --- a/xen/arch/x86/hvm/vmx/vmx.c Fri Jan 29 08:59:46 2010 +0000
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c Sun Jan 31 18:40:34 2010 +0800
>> @@ -2153,6 +2153,7 @@ static void vmx_failed_vmentry(unsigned
>> printk("caused by machine check.\n");
>> HVMTRACE_0D(MCE);
>> do_machine_check(regs);
>> + local_irq_enable();
>> break;
>> default:
>> printk("reason not known yet!");
>> @@ -2243,6 +2244,23 @@ err:
>> err:
>> vmx_inject_hw_exception(TRAP_gp_fault, 0);
>> return -1;
>> +}
>> +
>> +int vmx_mce_exit(int exit_reason)
>> +{
>> + if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY &&
>> + (uint16_t)exit_reason == EXIT_REASON_MCE_DURING_VMENTRY) )
>> + return 1;
>> + else if (unlikely(exit_reason == EXIT_REASON_EXCEPTION_NMI))
>> + {
>> + uint32_t vector;
>> +
>> + vector = __vmread(VM_EXIT_INTR_INFO) & INTR_INFO_VECTOR_MASK;
>> + if (vector == TRAP_machine_check)
>> + return 1;
>> + }
>> +
>> + return 0;
>> }
>>
>> asmlinkage void vmx_vmexit_handler(struct cpu_user_regs *regs)
>> @@ -2273,7 +2291,8 @@ asmlinkage void vmx_vmexit_handler(struc
>> vmx_do_extint(regs);
>>
>> /* Now enable interrupts so it's safe to take locks. */
>> - local_irq_enable();
>> + if ( !(vmx_mce_exit(exit_reason)) )
>> + local_irq_enable();
>>
>> if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) )
>> return vmx_failed_vmentry(exit_reason, regs);
>> @@ -2433,6 +2452,7 @@ asmlinkage void vmx_vmexit_handler(struc
>> case TRAP_machine_check:
>> HVMTRACE_0D(MCE);
>> do_machine_check(regs);
>> + local_irq_enable();
>> break;
>> case TRAP_invalid_op:
>> vmx_vmexit_ud_intercept(regs);
>>
>>
>>
>>> -----Original Message-----
>>> From: Jiang, Yunhong
>>> Sent: Monday, February 01, 2010 4:48 PM
>>> To: Keir Fraser; Tim.Deegan@citrix.com
>>> Cc: xen-devel@lists.xensource.com
>>> Subject: [PATCH] Don't enable irq for machine check vmexit
>>>
>>> We should not enable irq for machine check VMExit
>>>
>>> In changeset 18658:824892134573, IRQ is enabled during VMExit except
>>> external
>>> interrupt. The exception should apply for machine check also, because :
>>> a) The mce_logout_lock should be held in irq_disabled context.
>>> b) The machine check event should be handled as quickly as possible, enable
>>> irq will
>>> increase the period greatly.
>>>
>>> Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com>
>>>
>>> This is in hotspot code path, I try to use unlikely, hope to reduce the
>>> performance
>>> impact
>>>
>>> Thanks
>>> Yunhong Jiang
>
^ permalink raw reply [flat|nested] 14+ messages in thread* RE: [PATCH] Don't enable irq for machine check vmexit
2010-02-04 10:03 ` Keir Fraser
@ 2010-02-04 12:25 ` Jiang, Yunhong
2010-02-04 14:21 ` Tim Deegan
0 siblings, 1 reply; 14+ messages in thread
From: Jiang, Yunhong @ 2010-02-04 12:25 UTC (permalink / raw)
To: Keir Fraser, Tim Deegan; +Cc: xen-devel@lists.xensource.com
>-----Original Message-----
>From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
>Sent: Thursday, February 04, 2010 6:04 PM
>To: Jiang, Yunhong; Tim Deegan
>Cc: xen-devel@lists.xensource.com
>Subject: Re: [PATCH] Don't enable irq for machine check vmexit
>
>Yeah, I already replied to you on Monday. What I said was:
Sorry, seems I didn't got your original replay.
>
>"""
>Take a look at changeset 18658: local_irq_enable() was made unconditional in
>the vmexit handler function for a good reason!
>
>If you make it conditional on !mce, then in the mce case you can crash in
>any later function that acquires a spinlock: maybe_deassert_evtchn_irq(),
>for example. You will crash because of taking a irq-unsafe spinlock with
>irqs disabled. Kind of the opposite way round to the mce_logout_lock: so
>your patch would fix acquisition of that lock but break others.
In mce case, we can't take any lock that wil be shared with non-mce context at all, because MCE can happen in any context.
The mce_logout_lock is the only spin_lock AFAIK that is used in mce handler, and that lock is stated as " mce_logout_lock should only be used in the trap handler". That should apply to any spin_lock that will be used in MCE handler in future.
Yes, the mcheck_cmn_handler() takes vcpu_schedule_lock_irq(v), but that common handler is now only used in AMD platform, and I assume will be replaced after xen 4.0.
For the functoin that be called after the machine check handler, I enable the interrupt like following code, which is same as current logic, and I think that make sense.
@@ -2449,6 +2468,7 @@ asmlinkage void vmx_vmexit_handler(struc
case TRAP_machine_check:
HVMTRACE_0D(MCE);
do_machine_check(regs);
+ local_irq_enable();
break;
case TRAP_invalid_op:
vmx_vmexit_ud_intercept(regs);
>
>I'm afraid your only option is to hoist all the mce handling up to the same
>place we handle extints. Take c/s 18658 as your template for what to do.
Maybe I missed anything, but I didn't find the difference with the external interrupt case. Because I can't distinguish if a VMExit caused by MCE simply through a exit_reason, I wrap it through vmx_mce_exit().
>"""
>
> -- Keir
>
>On 04/02/2010 09:26, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:
>
>> Keir, any comments to this patch?
>>
>> Thanks
>> Yunhong Jiang
>>
>>> -----Original Message-----
>>> From: Jiang, Yunhong
>>> Sent: Monday, February 01, 2010 5:18 PM
>>> To: Jiang, Yunhong; Keir Fraser; Tim.Deegan@citrix.com
>>> Cc: xen-devel@lists.xensource.com
>>> Subject: RE: [PATCH] Don't enable irq for machine check vmexit
>>>
>>> Sorry forgot the patch.
>>>
>>> --jyh
>>>
>>> diff -r 857d7b2dd8c7 xen/arch/x86/hvm/vmx/vmx.c
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c Fri Jan 29 08:59:46 2010 +0000
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c Sun Jan 31 18:40:34 2010 +0800
>>> @@ -2153,6 +2153,7 @@ static void vmx_failed_vmentry(unsigned
>>> printk("caused by machine check.\n");
>>> HVMTRACE_0D(MCE);
>>> do_machine_check(regs);
>>> + local_irq_enable();
>>> break;
>>> default:
>>> printk("reason not known yet!");
>>> @@ -2243,6 +2244,23 @@ err:
>>> err:
>>> vmx_inject_hw_exception(TRAP_gp_fault, 0);
>>> return -1;
>>> +}
>>> +
>>> +int vmx_mce_exit(int exit_reason)
>>> +{
>>> + if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY &&
>>> + (uint16_t)exit_reason == EXIT_REASON_MCE_DURING_VMENTRY) )
>>> + return 1;
>>> + else if (unlikely(exit_reason == EXIT_REASON_EXCEPTION_NMI))
>>> + {
>>> + uint32_t vector;
>>> +
>>> + vector = __vmread(VM_EXIT_INTR_INFO) &
>INTR_INFO_VECTOR_MASK;
>>> + if (vector == TRAP_machine_check)
>>> + return 1;
>>> + }
>>> +
>>> + return 0;
>>> }
>>>
>>> asmlinkage void vmx_vmexit_handler(struct cpu_user_regs *regs)
>>> @@ -2273,7 +2291,8 @@ asmlinkage void vmx_vmexit_handler(struc
>>> vmx_do_extint(regs);
>>>
>>> /* Now enable interrupts so it's safe to take locks. */
>>> - local_irq_enable();
>>> + if ( !(vmx_mce_exit(exit_reason)) )
>>> + local_irq_enable();
>>>
>>> if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) )
>>> return vmx_failed_vmentry(exit_reason, regs);
>>> @@ -2433,6 +2452,7 @@ asmlinkage void vmx_vmexit_handler(struc
>>> case TRAP_machine_check:
>>> HVMTRACE_0D(MCE);
>>> do_machine_check(regs);
>>> + local_irq_enable();
>>> break;
>>> case TRAP_invalid_op:
>>> vmx_vmexit_ud_intercept(regs);
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Jiang, Yunhong
>>>> Sent: Monday, February 01, 2010 4:48 PM
>>>> To: Keir Fraser; Tim.Deegan@citrix.com
>>>> Cc: xen-devel@lists.xensource.com
>>>> Subject: [PATCH] Don't enable irq for machine check vmexit
>>>>
>>>> We should not enable irq for machine check VMExit
>>>>
>>>> In changeset 18658:824892134573, IRQ is enabled during VMExit except
>>>> external
>>>> interrupt. The exception should apply for machine check also, because :
>>>> a) The mce_logout_lock should be held in irq_disabled context.
>>>> b) The machine check event should be handled as quickly as possible, enable
>>>> irq will
>>>> increase the period greatly.
>>>>
>>>> Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com>
>>>>
>>>> This is in hotspot code path, I try to use unlikely, hope to reduce the
>>>> performance
>>>> impact
>>>>
>>>> Thanks
>>>> Yunhong Jiang
>>
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] Don't enable irq for machine check vmexit
2010-02-04 12:25 ` Jiang, Yunhong
@ 2010-02-04 14:21 ` Tim Deegan
2010-02-04 14:35 ` Jiang, Yunhong
2010-02-05 10:22 ` Jiang, Yunhong
0 siblings, 2 replies; 14+ messages in thread
From: Tim Deegan @ 2010-02-04 14:21 UTC (permalink / raw)
To: Jiang, Yunhong; +Cc: xen-devel@lists.xensource.com, Keir Fraser
At 12:25 +0000 on 04 Feb (1265286343), Jiang, Yunhong wrote:
> For the functoin that be called after the machine check handler, I
> enable the interrupt like following code, which is same as current
> logic, and I think that make sense.
You can't leave interrupts disabled in the main body of the VMEXIT
handler. It's fragile, and more importantly it reintroduces the bug
that 18658 fixed: you end up with spinlocks that are taken both with
interrupts enabled and with interrupts disabled, leading to a deadlock.
If you need to run the MCE handler before interrupts are re-enabled,
then it must be moved to the top of the vmexit handler, right beside
where EXTINTs are hadled.
Cheers,
Tim.
> @@ -2449,6 +2468,7 @@ asmlinkage void vmx_vmexit_handler(struc
> case TRAP_machine_check:
> HVMTRACE_0D(MCE);
> do_machine_check(regs);
> + local_irq_enable();
> break;
> case TRAP_invalid_op:
> vmx_vmexit_ud_intercept(regs);
>
> >
> >I'm afraid your only option is to hoist all the mce handling up to the same
> >place we handle extints. Take c/s 18658 as your template for what to do.
>
> Maybe I missed anything, but I didn't find the difference with the external interrupt case. Because I can't distinguish if a VMExit caused by MCE simply through a exit_reason, I wrap it through vmx_mce_exit().
> >"""
> >
> > -- Keir
> >
> >On 04/02/2010 09:26, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:
> >
> >> Keir, any comments to this patch?
> >>
> >> Thanks
> >> Yunhong Jiang
> >>
> >>> -----Original Message-----
> >>> From: Jiang, Yunhong
> >>> Sent: Monday, February 01, 2010 5:18 PM
> >>> To: Jiang, Yunhong; Keir Fraser; Tim.Deegan@citrix.com
> >>> Cc: xen-devel@lists.xensource.com
> >>> Subject: RE: [PATCH] Don't enable irq for machine check vmexit
> >>>
> >>> Sorry forgot the patch.
> >>>
> >>> --jyh
> >>>
> >>> diff -r 857d7b2dd8c7 xen/arch/x86/hvm/vmx/vmx.c
> >>> --- a/xen/arch/x86/hvm/vmx/vmx.c Fri Jan 29 08:59:46 2010 +0000
> >>> +++ b/xen/arch/x86/hvm/vmx/vmx.c Sun Jan 31 18:40:34 2010 +0800
> >>> @@ -2153,6 +2153,7 @@ static void vmx_failed_vmentry(unsigned
> >>> printk("caused by machine check.\n");
> >>> HVMTRACE_0D(MCE);
> >>> do_machine_check(regs);
> >>> + local_irq_enable();
> >>> break;
> >>> default:
> >>> printk("reason not known yet!");
> >>> @@ -2243,6 +2244,23 @@ err:
> >>> err:
> >>> vmx_inject_hw_exception(TRAP_gp_fault, 0);
> >>> return -1;
> >>> +}
> >>> +
> >>> +int vmx_mce_exit(int exit_reason)
> >>> +{
> >>> + if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY &&
> >>> + (uint16_t)exit_reason == EXIT_REASON_MCE_DURING_VMENTRY) )
> >>> + return 1;
> >>> + else if (unlikely(exit_reason == EXIT_REASON_EXCEPTION_NMI))
> >>> + {
> >>> + uint32_t vector;
> >>> +
> >>> + vector = __vmread(VM_EXIT_INTR_INFO) &
> >INTR_INFO_VECTOR_MASK;
> >>> + if (vector == TRAP_machine_check)
> >>> + return 1;
> >>> + }
> >>> +
> >>> + return 0;
> >>> }
> >>>
> >>> asmlinkage void vmx_vmexit_handler(struct cpu_user_regs *regs)
> >>> @@ -2273,7 +2291,8 @@ asmlinkage void vmx_vmexit_handler(struc
> >>> vmx_do_extint(regs);
> >>>
> >>> /* Now enable interrupts so it's safe to take locks. */
> >>> - local_irq_enable();
> >>> + if ( !(vmx_mce_exit(exit_reason)) )
> >>> + local_irq_enable();
> >>>
> >>> if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) )
> >>> return vmx_failed_vmentry(exit_reason, regs);
> >>> @@ -2433,6 +2452,7 @@ asmlinkage void vmx_vmexit_handler(struc
> >>> case TRAP_machine_check:
> >>> HVMTRACE_0D(MCE);
> >>> do_machine_check(regs);
> >>> + local_irq_enable();
> >>> break;
> >>> case TRAP_invalid_op:
> >>> vmx_vmexit_ud_intercept(regs);
> >>>
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Jiang, Yunhong
> >>>> Sent: Monday, February 01, 2010 4:48 PM
> >>>> To: Keir Fraser; Tim.Deegan@citrix.com
> >>>> Cc: xen-devel@lists.xensource.com
> >>>> Subject: [PATCH] Don't enable irq for machine check vmexit
> >>>>
> >>>> We should not enable irq for machine check VMExit
> >>>>
> >>>> In changeset 18658:824892134573, IRQ is enabled during VMExit except
> >>>> external
> >>>> interrupt. The exception should apply for machine check also, because :
> >>>> a) The mce_logout_lock should be held in irq_disabled context.
> >>>> b) The machine check event should be handled as quickly as possible, enable
> >>>> irq will
> >>>> increase the period greatly.
> >>>>
> >>>> Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com>
> >>>>
> >>>> This is in hotspot code path, I try to use unlikely, hope to reduce the
> >>>> performance
> >>>> impact
> >>>>
> >>>> Thanks
> >>>> Yunhong Jiang
> >>
> >
>
--
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Citrix Systems (R&D) Ltd.
[Company #02300071, SL9 0DZ, UK.]
^ permalink raw reply [flat|nested] 14+ messages in thread* RE: [PATCH] Don't enable irq for machine check vmexit
2010-02-04 14:21 ` Tim Deegan
@ 2010-02-04 14:35 ` Jiang, Yunhong
2010-02-05 10:22 ` Jiang, Yunhong
1 sibling, 0 replies; 14+ messages in thread
From: Jiang, Yunhong @ 2010-02-04 14:35 UTC (permalink / raw)
To: Tim Deegan; +Cc: xen-devel@lists.xensource.com, Keir Fraser
I got the issue. Thanks!
I will re-do the patch tomorrow.
Thanks
Yunhong Jiang
>-----Original Message-----
>From: Tim Deegan [mailto:Tim.Deegan@citrix.com]
>Sent: Thursday, February 04, 2010 10:22 PM
>To: Jiang, Yunhong
>Cc: Keir Fraser; xen-devel@lists.xensource.com
>Subject: Re: [PATCH] Don't enable irq for machine check vmexit
>
>At 12:25 +0000 on 04 Feb (1265286343), Jiang, Yunhong wrote:
>
>> For the functoin that be called after the machine check handler, I
>> enable the interrupt like following code, which is same as current
>> logic, and I think that make sense.
>
>You can't leave interrupts disabled in the main body of the VMEXIT
>handler. It's fragile, and more importantly it reintroduces the bug
>that 18658 fixed: you end up with spinlocks that are taken both with
>interrupts enabled and with interrupts disabled, leading to a deadlock.
>
>If you need to run the MCE handler before interrupts are re-enabled,
>then it must be moved to the top of the vmexit handler, right beside
>where EXTINTs are hadled.
>
>Cheers,
>
>Tim.
>
>> @@ -2449,6 +2468,7 @@ asmlinkage void vmx_vmexit_handler(struc
>> case TRAP_machine_check:
>> HVMTRACE_0D(MCE);
>> do_machine_check(regs);
>> + local_irq_enable();
>> break;
>> case TRAP_invalid_op:
>> vmx_vmexit_ud_intercept(regs);
>>
>> >
>> >I'm afraid your only option is to hoist all the mce handling up to the same
>> >place we handle extints. Take c/s 18658 as your template for what to do.
>>
>> Maybe I missed anything, but I didn't find the difference with the external interrupt
>case. Because I can't distinguish if a VMExit caused by MCE simply through a
>exit_reason, I wrap it through vmx_mce_exit().
>> >"""
>> >
>> > -- Keir
>> >
>> >On 04/02/2010 09:26, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:
>> >
>> >> Keir, any comments to this patch?
>> >>
>> >> Thanks
>> >> Yunhong Jiang
>> >>
>> >>> -----Original Message-----
>> >>> From: Jiang, Yunhong
>> >>> Sent: Monday, February 01, 2010 5:18 PM
>> >>> To: Jiang, Yunhong; Keir Fraser; Tim.Deegan@citrix.com
>> >>> Cc: xen-devel@lists.xensource.com
>> >>> Subject: RE: [PATCH] Don't enable irq for machine check vmexit
>> >>>
>> >>> Sorry forgot the patch.
>> >>>
>> >>> --jyh
>> >>>
>> >>> diff -r 857d7b2dd8c7 xen/arch/x86/hvm/vmx/vmx.c
>> >>> --- a/xen/arch/x86/hvm/vmx/vmx.c Fri Jan 29 08:59:46 2010 +0000
>> >>> +++ b/xen/arch/x86/hvm/vmx/vmx.c Sun Jan 31 18:40:34 2010 +0800
>> >>> @@ -2153,6 +2153,7 @@ static void vmx_failed_vmentry(unsigned
>> >>> printk("caused by machine check.\n");
>> >>> HVMTRACE_0D(MCE);
>> >>> do_machine_check(regs);
>> >>> + local_irq_enable();
>> >>> break;
>> >>> default:
>> >>> printk("reason not known yet!");
>> >>> @@ -2243,6 +2244,23 @@ err:
>> >>> err:
>> >>> vmx_inject_hw_exception(TRAP_gp_fault, 0);
>> >>> return -1;
>> >>> +}
>> >>> +
>> >>> +int vmx_mce_exit(int exit_reason)
>> >>> +{
>> >>> + if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY &&
>> >>> + (uint16_t)exit_reason ==
>EXIT_REASON_MCE_DURING_VMENTRY) )
>> >>> + return 1;
>> >>> + else if (unlikely(exit_reason == EXIT_REASON_EXCEPTION_NMI))
>> >>> + {
>> >>> + uint32_t vector;
>> >>> +
>> >>> + vector = __vmread(VM_EXIT_INTR_INFO) &
>> >INTR_INFO_VECTOR_MASK;
>> >>> + if (vector == TRAP_machine_check)
>> >>> + return 1;
>> >>> + }
>> >>> +
>> >>> + return 0;
>> >>> }
>> >>>
>> >>> asmlinkage void vmx_vmexit_handler(struct cpu_user_regs *regs)
>> >>> @@ -2273,7 +2291,8 @@ asmlinkage void vmx_vmexit_handler(struc
>> >>> vmx_do_extint(regs);
>> >>>
>> >>> /* Now enable interrupts so it's safe to take locks. */
>> >>> - local_irq_enable();
>> >>> + if ( !(vmx_mce_exit(exit_reason)) )
>> >>> + local_irq_enable();
>> >>>
>> >>> if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) )
>> >>> return vmx_failed_vmentry(exit_reason, regs);
>> >>> @@ -2433,6 +2452,7 @@ asmlinkage void vmx_vmexit_handler(struc
>> >>> case TRAP_machine_check:
>> >>> HVMTRACE_0D(MCE);
>> >>> do_machine_check(regs);
>> >>> + local_irq_enable();
>> >>> break;
>> >>> case TRAP_invalid_op:
>> >>> vmx_vmexit_ud_intercept(regs);
>> >>>
>> >>>
>> >>>
>> >>>> -----Original Message-----
>> >>>> From: Jiang, Yunhong
>> >>>> Sent: Monday, February 01, 2010 4:48 PM
>> >>>> To: Keir Fraser; Tim.Deegan@citrix.com
>> >>>> Cc: xen-devel@lists.xensource.com
>> >>>> Subject: [PATCH] Don't enable irq for machine check vmexit
>> >>>>
>> >>>> We should not enable irq for machine check VMExit
>> >>>>
>> >>>> In changeset 18658:824892134573, IRQ is enabled during VMExit except
>> >>>> external
>> >>>> interrupt. The exception should apply for machine check also, because :
>> >>>> a) The mce_logout_lock should be held in irq_disabled context.
>> >>>> b) The machine check event should be handled as quickly as possible, enable
>> >>>> irq will
>> >>>> increase the period greatly.
>> >>>>
>> >>>> Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com>
>> >>>>
>> >>>> This is in hotspot code path, I try to use unlikely, hope to reduce the
>> >>>> performance
>> >>>> impact
>> >>>>
>> >>>> Thanks
>> >>>> Yunhong Jiang
>> >>
>> >
>>
>
>--
>Tim Deegan <Tim.Deegan@citrix.com>
>Principal Software Engineer, Citrix Systems (R&D) Ltd.
>[Company #02300071, SL9 0DZ, UK.]
^ permalink raw reply [flat|nested] 14+ messages in thread* RE: [PATCH] Don't enable irq for machine check vmexit
2010-02-04 14:21 ` Tim Deegan
2010-02-04 14:35 ` Jiang, Yunhong
@ 2010-02-05 10:22 ` Jiang, Yunhong
2010-02-05 10:42 ` Tim Deegan
2010-02-05 12:53 ` Keir Fraser
1 sibling, 2 replies; 14+ messages in thread
From: Jiang, Yunhong @ 2010-02-05 10:22 UTC (permalink / raw)
To: Tim Deegan; +Cc: xen-devel@lists.xensource.com, Keir Fraser
[-- Attachment #1: Type: text/plain, Size: 2763 bytes --]
Keir/Tim, here is attached patch. Please have a look onit.
Thanks
Yunhong Jiang
# HG changeset patch
# User Yunhong Jiang <yunhong.jiang@intel.com>
# Date 1265363638 -28800
# Node ID 01b2ce3f2cc95dd2e9c6defe2cee8a892e867187
# Parent 7b751b0e6f1bc7485b0718e634ed7cb9ce9ab68c
We should not enable irq for machine check VMExit
In changeset 18658:824892134573, IRQ is enabled during VMExit except external interrupt. The exception should apply for machine check also, because :
a) The mce_logout_lock should be held in irq_disabled context.
b) The machine check event should be handled as quickly as possible, enable irq will increase the period greatly.
Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com>
diff -r 7b751b0e6f1b -r 01b2ce3f2cc9 xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c Thu Feb 04 19:40:19 2010 +0000
+++ b/xen/arch/x86/hvm/vmx/vmx.c Fri Feb 05 17:53:58 2010 +0800
@@ -2168,7 +2168,7 @@ static void vmx_failed_vmentry(unsigned
case EXIT_REASON_MCE_DURING_VMENTRY:
printk("caused by machine check.\n");
HVMTRACE_0D(MCE);
- do_machine_check(regs);
+ /* Handled already */
break;
default:
printk("reason not known yet!");
@@ -2259,6 +2259,23 @@ err:
err:
vmx_inject_hw_exception(TRAP_gp_fault, 0);
return -1;
+}
+
+int vmx_mce_exit(int exit_reason)
+{
+ if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY &&
+ (uint16_t)exit_reason == EXIT_REASON_MCE_DURING_VMENTRY) )
+ return 1;
+ else if (unlikely(exit_reason == EXIT_REASON_EXCEPTION_NMI))
+ {
+ uint32_t vector;
+
+ vector = __vmread(VM_EXIT_INTR_INFO) & INTR_INFO_VECTOR_MASK;
+ if (vector == TRAP_machine_check)
+ return 1;
+ }
+
+ return 0;
}
asmlinkage void vmx_vmexit_handler(struct cpu_user_regs *regs)
@@ -2287,6 +2304,9 @@ asmlinkage void vmx_vmexit_handler(struc
/* Handle the interrupt we missed before allowing any more in. */
if ( exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT )
vmx_do_extint(regs);
+
+ if ( vmx_mce_exit(exit_reason) )
+ do_machine_check(regs);
/* Now enable interrupts so it's safe to take locks. */
local_irq_enable();
@@ -2447,8 +2467,9 @@ asmlinkage void vmx_vmexit_handler(struc
self_nmi(); /* Real NMI, vector 2: normal processing. */
break;
case TRAP_machine_check:
+ dprintk(XENLOG_INFO, "VMexit for machine check\n");
HVMTRACE_0D(MCE);
- do_machine_check(regs);
+ /* Handled already */
break;
case TRAP_invalid_op:
vmx_vmexit_ud_intercept(regs);
[-- Attachment #2: vmexit_mca_irq.patch --]
[-- Type: application/octet-stream, Size: 2602 bytes --]
# HG changeset patch
# User Yunhong Jiang <yunhong.jiang@intel.com>
# Date 1265363638 -28800
# Node ID 01b2ce3f2cc95dd2e9c6defe2cee8a892e867187
# Parent 7b751b0e6f1bc7485b0718e634ed7cb9ce9ab68c
We should not enable irq for machine check VMExit
In changeset 18658:824892134573, IRQ is enabled during VMExit except external interrupt. The exception should apply for machine check also, because :
a) The mce_logout_lock should be held in irq_disabled context.
b) The machine check event should be handled as quickly as possible, enable irq will increase the period greatly.
Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com>
diff -r 7b751b0e6f1b -r 01b2ce3f2cc9 xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c Thu Feb 04 19:40:19 2010 +0000
+++ b/xen/arch/x86/hvm/vmx/vmx.c Fri Feb 05 17:53:58 2010 +0800
@@ -2168,7 +2168,7 @@ static void vmx_failed_vmentry(unsigned
case EXIT_REASON_MCE_DURING_VMENTRY:
printk("caused by machine check.\n");
HVMTRACE_0D(MCE);
- do_machine_check(regs);
+ /* Handled already */
break;
default:
printk("reason not known yet!");
@@ -2259,6 +2259,23 @@ err:
err:
vmx_inject_hw_exception(TRAP_gp_fault, 0);
return -1;
+}
+
+int vmx_mce_exit(int exit_reason)
+{
+ if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY &&
+ (uint16_t)exit_reason == EXIT_REASON_MCE_DURING_VMENTRY) )
+ return 1;
+ else if (unlikely(exit_reason == EXIT_REASON_EXCEPTION_NMI))
+ {
+ uint32_t vector;
+
+ vector = __vmread(VM_EXIT_INTR_INFO) & INTR_INFO_VECTOR_MASK;
+ if (vector == TRAP_machine_check)
+ return 1;
+ }
+
+ return 0;
}
asmlinkage void vmx_vmexit_handler(struct cpu_user_regs *regs)
@@ -2287,6 +2304,9 @@ asmlinkage void vmx_vmexit_handler(struc
/* Handle the interrupt we missed before allowing any more in. */
if ( exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT )
vmx_do_extint(regs);
+
+ if ( vmx_mce_exit(exit_reason) )
+ do_machine_check(regs);
/* Now enable interrupts so it's safe to take locks. */
local_irq_enable();
@@ -2447,8 +2467,9 @@ asmlinkage void vmx_vmexit_handler(struc
self_nmi(); /* Real NMI, vector 2: normal processing. */
break;
case TRAP_machine_check:
+ dprintk(XENLOG_INFO, "VMexit for machine check\n");
HVMTRACE_0D(MCE);
- do_machine_check(regs);
+ /* Handled already */
break;
case TRAP_invalid_op:
vmx_vmexit_ud_intercept(regs);
[-- Attachment #3: Type: text/plain, Size: 138 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: RE: [PATCH] Don't enable irq for machine check vmexit
2010-02-05 10:22 ` Jiang, Yunhong
@ 2010-02-05 10:42 ` Tim Deegan
2010-02-05 14:37 ` Jiang, Yunhong
2010-02-05 12:53 ` Keir Fraser
1 sibling, 1 reply; 14+ messages in thread
From: Tim Deegan @ 2010-02-05 10:42 UTC (permalink / raw)
To: Jiang, Yunhong; +Cc: Keir, xen-devel@lists.xensource.com, Fraser
At 10:22 +0000 on 05 Feb (1265365365), Jiang, Yunhong wrote:
> Keir/Tim, here is attached patch. Please have a look onit.
Yep, that looks better. Not sure that the dprintk is useful, though.
Cheers,
Tim.
> Thanks
> Yunhong Jiang
>
> # HG changeset patch
> # User Yunhong Jiang <yunhong.jiang@intel.com>
> # Date 1265363638 -28800
> # Node ID 01b2ce3f2cc95dd2e9c6defe2cee8a892e867187
> # Parent 7b751b0e6f1bc7485b0718e634ed7cb9ce9ab68c
> We should not enable irq for machine check VMExit
>
> In changeset 18658:824892134573, IRQ is enabled during VMExit except external interrupt. The exception should apply for machine check also, because :
> a) The mce_logout_lock should be held in irq_disabled context.
> b) The machine check event should be handled as quickly as possible, enable irq will increase the period greatly.
>
> Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com>
>
> diff -r 7b751b0e6f1b -r 01b2ce3f2cc9 xen/arch/x86/hvm/vmx/vmx.c
> --- a/xen/arch/x86/hvm/vmx/vmx.c Thu Feb 04 19:40:19 2010 +0000
> +++ b/xen/arch/x86/hvm/vmx/vmx.c Fri Feb 05 17:53:58 2010 +0800
> @@ -2168,7 +2168,7 @@ static void vmx_failed_vmentry(unsigned
> case EXIT_REASON_MCE_DURING_VMENTRY:
> printk("caused by machine check.\n");
> HVMTRACE_0D(MCE);
> - do_machine_check(regs);
> + /* Handled already */
> break;
> default:
> printk("reason not known yet!");
> @@ -2259,6 +2259,23 @@ err:
> err:
> vmx_inject_hw_exception(TRAP_gp_fault, 0);
> return -1;
> +}
> +
> +int vmx_mce_exit(int exit_reason)
> +{
> + if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY &&
> + (uint16_t)exit_reason == EXIT_REASON_MCE_DURING_VMENTRY) )
> + return 1;
> + else if (unlikely(exit_reason == EXIT_REASON_EXCEPTION_NMI))
> + {
> + uint32_t vector;
> +
> + vector = __vmread(VM_EXIT_INTR_INFO) & INTR_INFO_VECTOR_MASK;
> + if (vector == TRAP_machine_check)
> + return 1;
> + }
> +
> + return 0;
> }
>
> asmlinkage void vmx_vmexit_handler(struct cpu_user_regs *regs)
> @@ -2287,6 +2304,9 @@ asmlinkage void vmx_vmexit_handler(struc
> /* Handle the interrupt we missed before allowing any more in. */
> if ( exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT )
> vmx_do_extint(regs);
> +
> + if ( vmx_mce_exit(exit_reason) )
> + do_machine_check(regs);
>
> /* Now enable interrupts so it's safe to take locks. */
> local_irq_enable();
> @@ -2447,8 +2467,9 @@ asmlinkage void vmx_vmexit_handler(struc
> self_nmi(); /* Real NMI, vector 2: normal processing. */
> break;
> case TRAP_machine_check:
> + dprintk(XENLOG_INFO, "VMexit for machine check\n");
> HVMTRACE_0D(MCE);
> - do_machine_check(regs);
> + /* Handled already */
> break;
> case TRAP_invalid_op:
> vmx_vmexit_ud_intercept(regs);
>
>
Content-Description: ATT00001.txt
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
--
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Citrix Systems (R&D) Ltd.
[Company #02300071, SL9 0DZ, UK.]
^ permalink raw reply [flat|nested] 14+ messages in thread* RE: RE: [PATCH] Don't enable irq for machine check vmexit
2010-02-05 10:42 ` Tim Deegan
@ 2010-02-05 14:37 ` Jiang, Yunhong
0 siblings, 0 replies; 14+ messages in thread
From: Jiang, Yunhong @ 2010-02-05 14:37 UTC (permalink / raw)
To: Tim Deegan; +Cc: Keir, xen-devel@lists.xensource.com, Fraser
Tim, thanks for review. I add printk here because that is something not always happen.
--jyh
>-----Original Message-----
>From: Tim Deegan [mailto:Tim.Deegan@citrix.com]
>Sent: Friday, February 05, 2010 6:42 PM
>To: Jiang, Yunhong
>Cc: xen-devel@lists.xensource.com; Keir Fraser
>Subject: Re: [Xen-devel] RE: [PATCH] Don't enable irq for machine check vmexit
>
>At 10:22 +0000 on 05 Feb (1265365365), Jiang, Yunhong wrote:
>> Keir/Tim, here is attached patch. Please have a look onit.
>
>Yep, that looks better. Not sure that the dprintk is useful, though.
>
>Cheers,
>
>Tim.
>
>> Thanks
>> Yunhong Jiang
>>
>> # HG changeset patch
>> # User Yunhong Jiang <yunhong.jiang@intel.com>
>> # Date 1265363638 -28800
>> # Node ID 01b2ce3f2cc95dd2e9c6defe2cee8a892e867187
>> # Parent 7b751b0e6f1bc7485b0718e634ed7cb9ce9ab68c
>> We should not enable irq for machine check VMExit
>>
>> In changeset 18658:824892134573, IRQ is enabled during VMExit except external
>interrupt. The exception should apply for machine check also, because :
>> a) The mce_logout_lock should be held in irq_disabled context.
>> b) The machine check event should be handled as quickly as possible, enable irq will
>increase the period greatly.
>>
>> Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com>
>>
>> diff -r 7b751b0e6f1b -r 01b2ce3f2cc9 xen/arch/x86/hvm/vmx/vmx.c
>> --- a/xen/arch/x86/hvm/vmx/vmx.c Thu Feb 04 19:40:19 2010 +0000
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c Fri Feb 05 17:53:58 2010 +0800
>> @@ -2168,7 +2168,7 @@ static void vmx_failed_vmentry(unsigned
>> case EXIT_REASON_MCE_DURING_VMENTRY:
>> printk("caused by machine check.\n");
>> HVMTRACE_0D(MCE);
>> - do_machine_check(regs);
>> + /* Handled already */
>> break;
>> default:
>> printk("reason not known yet!");
>> @@ -2259,6 +2259,23 @@ err:
>> err:
>> vmx_inject_hw_exception(TRAP_gp_fault, 0);
>> return -1;
>> +}
>> +
>> +int vmx_mce_exit(int exit_reason)
>> +{
>> + if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY &&
>> + (uint16_t)exit_reason == EXIT_REASON_MCE_DURING_VMENTRY) )
>> + return 1;
>> + else if (unlikely(exit_reason == EXIT_REASON_EXCEPTION_NMI))
>> + {
>> + uint32_t vector;
>> +
>> + vector = __vmread(VM_EXIT_INTR_INFO) &
>INTR_INFO_VECTOR_MASK;
>> + if (vector == TRAP_machine_check)
>> + return 1;
>> + }
>> +
>> + return 0;
>> }
>>
>> asmlinkage void vmx_vmexit_handler(struct cpu_user_regs *regs)
>> @@ -2287,6 +2304,9 @@ asmlinkage void vmx_vmexit_handler(struc
>> /* Handle the interrupt we missed before allowing any more in. */
>> if ( exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT )
>> vmx_do_extint(regs);
>> +
>> + if ( vmx_mce_exit(exit_reason) )
>> + do_machine_check(regs);
>>
>> /* Now enable interrupts so it's safe to take locks. */
>> local_irq_enable();
>> @@ -2447,8 +2467,9 @@ asmlinkage void vmx_vmexit_handler(struc
>> self_nmi(); /* Real NMI, vector 2: normal processing. */
>> break;
>> case TRAP_machine_check:
>> + dprintk(XENLOG_INFO, "VMexit for machine check\n");
>> HVMTRACE_0D(MCE);
>> - do_machine_check(regs);
>> + /* Handled already */
>> break;
>> case TRAP_invalid_op:
>> vmx_vmexit_ud_intercept(regs);
>>
>>
>
>
>Content-Description: ATT00001.txt
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
>
>
>--
>Tim Deegan <Tim.Deegan@citrix.com>
>Principal Software Engineer, Citrix Systems (R&D) Ltd.
>[Company #02300071, SL9 0DZ, UK.]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] Don't enable irq for machine check vmexit
2010-02-05 10:22 ` Jiang, Yunhong
2010-02-05 10:42 ` Tim Deegan
@ 2010-02-05 12:53 ` Keir Fraser
2010-02-05 14:36 ` Jiang, Yunhong
1 sibling, 1 reply; 14+ messages in thread
From: Keir Fraser @ 2010-02-05 12:53 UTC (permalink / raw)
To: Jiang, Yunhong, Tim Deegan; +Cc: xen-devel@lists.xensource.com
[-- Attachment #1: Type: text/plain, Size: 3372 bytes --]
How about the attached alternative, which avoids repeated reads of
VM_INTR_INFO? Also I'm not sure whether checking for
VM_EXIT_REASONS_FAILED_VMENTRY is useful, so I removed it. After all,
EXIT_REASON_MCE_DURING_VMENTRY should imply it anyway.
Perhaps we should mask exit_reason down to 16 bits right at the top of the
vmexit handler? That would save us if new flags are added to
exit_reason[30:16], also.
Anyway, let me know what you think.
-- Keir
On 05/02/2010 10:22, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:
> Keir/Tim, here is attached patch. Please have a look onit.
>
> Thanks
> Yunhong Jiang
>
> # HG changeset patch
> # User Yunhong Jiang <yunhong.jiang@intel.com>
> # Date 1265363638 -28800
> # Node ID 01b2ce3f2cc95dd2e9c6defe2cee8a892e867187
> # Parent 7b751b0e6f1bc7485b0718e634ed7cb9ce9ab68c
> We should not enable irq for machine check VMExit
>
> In changeset 18658:824892134573, IRQ is enabled during VMExit except external
> interrupt. The exception should apply for machine check also, because :
> a) The mce_logout_lock should be held in irq_disabled context.
> b) The machine check event should be handled as quickly as possible, enable
> irq will increase the period greatly.
>
> Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com>
>
> diff -r 7b751b0e6f1b -r 01b2ce3f2cc9 xen/arch/x86/hvm/vmx/vmx.c
> --- a/xen/arch/x86/hvm/vmx/vmx.c Thu Feb 04 19:40:19 2010 +0000
> +++ b/xen/arch/x86/hvm/vmx/vmx.c Fri Feb 05 17:53:58 2010 +0800
> @@ -2168,7 +2168,7 @@ static void vmx_failed_vmentry(unsigned
> case EXIT_REASON_MCE_DURING_VMENTRY:
> printk("caused by machine check.\n");
> HVMTRACE_0D(MCE);
> - do_machine_check(regs);
> + /* Handled already */
> break;
> default:
> printk("reason not known yet!");
> @@ -2259,6 +2259,23 @@ err:
> err:
> vmx_inject_hw_exception(TRAP_gp_fault, 0);
> return -1;
> +}
> +
> +int vmx_mce_exit(int exit_reason)
> +{
> + if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY &&
> + (uint16_t)exit_reason == EXIT_REASON_MCE_DURING_VMENTRY) )
> + return 1;
> + else if (unlikely(exit_reason == EXIT_REASON_EXCEPTION_NMI))
> + {
> + uint32_t vector;
> +
> + vector = __vmread(VM_EXIT_INTR_INFO) & INTR_INFO_VECTOR_MASK;
> + if (vector == TRAP_machine_check)
> + return 1;
> + }
> +
> + return 0;
> }
>
> asmlinkage void vmx_vmexit_handler(struct cpu_user_regs *regs)
> @@ -2287,6 +2304,9 @@ asmlinkage void vmx_vmexit_handler(struc
> /* Handle the interrupt we missed before allowing any more in. */
> if ( exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT )
> vmx_do_extint(regs);
> +
> + if ( vmx_mce_exit(exit_reason) )
> + do_machine_check(regs);
>
> /* Now enable interrupts so it's safe to take locks. */
> local_irq_enable();
> @@ -2447,8 +2467,9 @@ asmlinkage void vmx_vmexit_handler(struc
> self_nmi(); /* Real NMI, vector 2: normal processing. */
> break;
> case TRAP_machine_check:
> + dprintk(XENLOG_INFO, "VMexit for machine check\n");
> HVMTRACE_0D(MCE);
> - do_machine_check(regs);
> + /* Handled already */
> break;
> case TRAP_invalid_op:
> vmx_vmexit_ud_intercept(regs);
>
>
[-- Attachment #2: 00-mce --]
[-- Type: application/octet-stream, Size: 2868 bytes --]
diff -r 989014ce7b4a xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c Fri Feb 05 10:37:24 2010 +0000
+++ b/xen/arch/x86/hvm/vmx/vmx.c Fri Feb 05 12:48:39 2010 +0000
@@ -2168,7 +2168,7 @@
case EXIT_REASON_MCE_DURING_VMENTRY:
printk("caused by machine check.\n");
HVMTRACE_0D(MCE);
- do_machine_check(regs);
+ /* Already handled. */
break;
default:
printk("reason not known yet!");
@@ -2263,7 +2263,7 @@
asmlinkage void vmx_vmexit_handler(struct cpu_user_regs *regs)
{
- unsigned int exit_reason, idtv_info;
+ unsigned int exit_reason, idtv_info, intr_info = 0, vector = 0;
unsigned long exit_qualification, inst_len = 0;
struct vcpu *v = current;
@@ -2285,8 +2285,22 @@
perfc_incra(vmexits, exit_reason);
/* Handle the interrupt we missed before allowing any more in. */
- if ( exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT )
+ switch ( (uint16_t)exit_reason )
+ {
+ case EXIT_REASON_EXTERNAL_INTERRUPT:
vmx_do_extint(regs);
+ break;
+ case EXIT_REASON_EXCEPTION_NMI:
+ intr_info = __vmread(VM_EXIT_INTR_INFO);
+ BUG_ON(!(intr_info & INTR_INFO_VALID_MASK));
+ vector = intr_info & INTR_INFO_VECTOR_MASK;
+ if ( vector == TRAP_machine_check )
+ do_machine_check(regs);
+ break;
+ case EXIT_REASON_MCE_DURING_VMENTRY:
+ do_machine_check(regs);
+ break;
+ }
/* Now enable interrupts so it's safe to take locks. */
local_irq_enable();
@@ -2296,8 +2310,6 @@
if ( v->arch.hvm_vmx.vmx_realmode )
{
- unsigned int vector;
-
/* Put RFLAGS back the way the guest wants it */
regs->eflags &= ~(X86_EFLAGS_VM | X86_EFLAGS_IOPL);
regs->eflags |= (v->arch.hvm_vmx.vm86_saved_eflags & X86_EFLAGS_IOPL);
@@ -2307,7 +2319,6 @@
switch ( exit_reason )
{
case EXIT_REASON_EXCEPTION_NMI:
- vector = __vmread(VM_EXIT_INTR_INFO) & INTR_INFO_VECTOR_MASK;
if ( vector != TRAP_page_fault
&& vector != TRAP_nmi
&& vector != TRAP_machine_check )
@@ -2366,12 +2377,6 @@
* (1) We can get an exception (e.g. #PG) in the guest, or
* (2) NMI
*/
- unsigned int intr_info, vector;
-
- intr_info = __vmread(VM_EXIT_INTR_INFO);
- BUG_ON(!(intr_info & INTR_INFO_VALID_MASK));
-
- vector = intr_info & INTR_INFO_VECTOR_MASK;
/*
* Re-set the NMI shadow if vmexit caused by a guest IRET fault (see 3B
@@ -2448,7 +2453,7 @@
break;
case TRAP_machine_check:
HVMTRACE_0D(MCE);
- do_machine_check(regs);
+ /* Already handled above. */
break;
case TRAP_invalid_op:
vmx_vmexit_ud_intercept(regs);
[-- Attachment #3: Type: text/plain, Size: 138 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread* RE: [PATCH] Don't enable irq for machine check vmexit
2010-02-05 12:53 ` Keir Fraser
@ 2010-02-05 14:36 ` Jiang, Yunhong
2010-02-05 14:59 ` Keir Fraser
0 siblings, 1 reply; 14+ messages in thread
From: Jiang, Yunhong @ 2010-02-05 14:36 UTC (permalink / raw)
To: Keir Fraser, Tim Deegan; +Cc: xen-devel@lists.xensource.com
>-----Original Message-----
>From: xen-devel-bounces@lists.xensource.com
>[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Keir Fraser
>Sent: Friday, February 05, 2010 8:54 PM
>To: Jiang, Yunhong; Tim Deegan
>Cc: xen-devel@lists.xensource.com
>Subject: [Xen-devel] Re: [PATCH] Don't enable irq for machine check vmexit
>
>How about the attached alternative, which avoids repeated reads of
>VM_INTR_INFO? Also I'm not sure whether checking for
>VM_EXIT_REASONS_FAILED_VMENTRY is useful, so I removed it. After all,
>EXIT_REASON_MCE_DURING_VMENTRY should imply it anyway.
Thanks for your patch. Yes, it is much better to avoid the repeated read.
I'm not sure if it is ok if we don't check the VM_EXIT_REASONS_FAILED_VMENTRY. Checking the SDM and seems it is ok. In fact, I didn't find effective method to test this VMEntry MCE failed case, althgouh I can test MCE VMExit with EXIT_REASON_EXCEPTION_NMI case easily. (I will try to find a method to test this VMEntry failure case next week, maybe poison the VMCS range can trigger it, I'm not sure).
Or maybe code like following? It will increase a "&" operation for each exit, but that should make us safe.
- if ( exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT )
+ switch ( exit_reason & (VMX_EXIT_REASONS_FAILED_VMENTRY | 0xFFFF))
+ {
+ case EXIT_REASON_EXTERNAL_INTERRUPT:
vmx_do_extint(regs);
+ break;
....
+ case EXIT_REASON_MCE_DURING_VMENTRY| VMX_EXIT_REASONS_FAILED_VMENTRY: ---> (or define a new macro for it)
+ do_machine_check(regs);
+ break
>
>Perhaps we should mask exit_reason down to 16 bits right at the top of the
>vmexit handler? That would save us if new flags are added to
>exit_reason[30:16], also.
Or at least mask off the bits 27:16?
Thanks
Yunhong Jiang
>
>Anyway, let me know what you think.
>
> -- Keir
>
>On 05/02/2010 10:22, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:
>
>> Keir/Tim, here is attached patch. Please have a look onit.
>>
>> Thanks
>> Yunhong Jiang
>>
>> # HG changeset patch
>> # User Yunhong Jiang <yunhong.jiang@intel.com>
>> # Date 1265363638 -28800
>> # Node ID 01b2ce3f2cc95dd2e9c6defe2cee8a892e867187
>> # Parent 7b751b0e6f1bc7485b0718e634ed7cb9ce9ab68c
>> We should not enable irq for machine check VMExit
>>
>> In changeset 18658:824892134573, IRQ is enabled during VMExit except external
>> interrupt. The exception should apply for machine check also, because :
>> a) The mce_logout_lock should be held in irq_disabled context.
>> b) The machine check event should be handled as quickly as possible, enable
>> irq will increase the period greatly.
>>
>> Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com>
>>
>> diff -r 7b751b0e6f1b -r 01b2ce3f2cc9 xen/arch/x86/hvm/vmx/vmx.c
>> --- a/xen/arch/x86/hvm/vmx/vmx.c Thu Feb 04 19:40:19 2010 +0000
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c Fri Feb 05 17:53:58 2010 +0800
>> @@ -2168,7 +2168,7 @@ static void vmx_failed_vmentry(unsigned
>> case EXIT_REASON_MCE_DURING_VMENTRY:
>> printk("caused by machine check.\n");
>> HVMTRACE_0D(MCE);
>> - do_machine_check(regs);
>> + /* Handled already */
>> break;
>> default:
>> printk("reason not known yet!");
>> @@ -2259,6 +2259,23 @@ err:
>> err:
>> vmx_inject_hw_exception(TRAP_gp_fault, 0);
>> return -1;
>> +}
>> +
>> +int vmx_mce_exit(int exit_reason)
>> +{
>> + if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY &&
>> + (uint16_t)exit_reason == EXIT_REASON_MCE_DURING_VMENTRY) )
>> + return 1;
>> + else if (unlikely(exit_reason == EXIT_REASON_EXCEPTION_NMI))
>> + {
>> + uint32_t vector;
>> +
>> + vector = __vmread(VM_EXIT_INTR_INFO) &
>INTR_INFO_VECTOR_MASK;
>> + if (vector == TRAP_machine_check)
>> + return 1;
>> + }
>> +
>> + return 0;
>> }
>>
>> asmlinkage void vmx_vmexit_handler(struct cpu_user_regs *regs)
>> @@ -2287,6 +2304,9 @@ asmlinkage void vmx_vmexit_handler(struc
>> /* Handle the interrupt we missed before allowing any more in. */
>> if ( exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT )
>> vmx_do_extint(regs);
>> +
>> + if ( vmx_mce_exit(exit_reason) )
>> + do_machine_check(regs);
>>
>> /* Now enable interrupts so it's safe to take locks. */
>> local_irq_enable();
>> @@ -2447,8 +2467,9 @@ asmlinkage void vmx_vmexit_handler(struc
>> self_nmi(); /* Real NMI, vector 2: normal processing. */
>> break;
>> case TRAP_machine_check:
>> + dprintk(XENLOG_INFO, "VMexit for machine check\n");
>> HVMTRACE_0D(MCE);
>> - do_machine_check(regs);
>> + /* Handled already */
>> break;
>> case TRAP_invalid_op:
>> vmx_vmexit_ud_intercept(regs);
>>
>>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: RE: [PATCH] Don't enable irq for machine check vmexit
2010-02-05 14:36 ` Jiang, Yunhong
@ 2010-02-05 14:59 ` Keir Fraser
2010-02-07 4:25 ` Jiang, Yunhong
0 siblings, 1 reply; 14+ messages in thread
From: Keir Fraser @ 2010-02-05 14:59 UTC (permalink / raw)
To: Jiang, Yunhong, Tim Deegan; +Cc: xen-devel@lists.xensource.com
On 05/02/2010 14:36, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:
>> How about the attached alternative, which avoids repeated reads of
>> VM_INTR_INFO? Also I'm not sure whether checking for
>> VM_EXIT_REASONS_FAILED_VMENTRY is useful, so I removed it. After all,
>> EXIT_REASON_MCE_DURING_VMENTRY should imply it anyway.
>
> Thanks for your patch. Yes, it is much better to avoid the repeated read.
>
> I'm not sure if it is ok if we don't check the VM_EXIT_REASONS_FAILED_VMENTRY.
> Checking the SDM and seems it is ok. In fact, I didn't find effective method
> to test this VMEntry MCE failed case, althgouh I can test MCE VMExit with
> EXIT_REASON_EXCEPTION_NMI case easily. (I will try to find a method to test
> this VMEntry failure case next week, maybe poison the VMCS range can trigger
> it, I'm not sure).
Well, we don't seem to know what bit 31 is for. Or, at least, we don't know
how it should affect our behaviour in the vmexit handler. So looking at it
does seem a bit pointless.
-- Keir
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: RE: [PATCH] Don't enable irq for machine check vmexit
2010-02-05 14:59 ` Keir Fraser
@ 2010-02-07 4:25 ` Jiang, Yunhong
0 siblings, 0 replies; 14+ messages in thread
From: Jiang, Yunhong @ 2010-02-07 4:25 UTC (permalink / raw)
To: Keir Fraser, Tim Deegan; +Cc: xen-devel@lists.xensource.com
Yes, agree and this patch is ok for me. Thanks very much.
--jyh
>-----Original Message-----
>From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
>Sent: Friday, February 05, 2010 10:59 PM
>To: Jiang, Yunhong; Tim Deegan
>Cc: xen-devel@lists.xensource.com
>Subject: Re: [Xen-devel] RE: [PATCH] Don't enable irq for machine check vmexit
>
>On 05/02/2010 14:36, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:
>
>>> How about the attached alternative, which avoids repeated reads of
>>> VM_INTR_INFO? Also I'm not sure whether checking for
>>> VM_EXIT_REASONS_FAILED_VMENTRY is useful, so I removed it. After all,
>>> EXIT_REASON_MCE_DURING_VMENTRY should imply it anyway.
>>
>> Thanks for your patch. Yes, it is much better to avoid the repeated read.
>>
>> I'm not sure if it is ok if we don't check the VM_EXIT_REASONS_FAILED_VMENTRY.
>> Checking the SDM and seems it is ok. In fact, I didn't find effective method
>> to test this VMEntry MCE failed case, althgouh I can test MCE VMExit with
>> EXIT_REASON_EXCEPTION_NMI case easily. (I will try to find a method to test
>> this VMEntry failure case next week, maybe poison the VMCS range can trigger
>> it, I'm not sure).
>
>Well, we don't seem to know what bit 31 is for. Or, at least, we don't know
>how it should affect our behaviour in the vmexit handler. So looking at it
>does seem a bit pointless.
>
> -- Keir
>
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2010-02-07 4:25 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-02-01 9:18 [PATCH] Don't enable irq for machine check vmexit Jiang, Yunhong
2010-02-01 11:14 ` Keir Fraser
2010-02-04 9:26 ` Jiang, Yunhong
2010-02-04 10:03 ` Keir Fraser
2010-02-04 12:25 ` Jiang, Yunhong
2010-02-04 14:21 ` Tim Deegan
2010-02-04 14:35 ` Jiang, Yunhong
2010-02-05 10:22 ` Jiang, Yunhong
2010-02-05 10:42 ` Tim Deegan
2010-02-05 14:37 ` Jiang, Yunhong
2010-02-05 12:53 ` Keir Fraser
2010-02-05 14:36 ` Jiang, Yunhong
2010-02-05 14:59 ` Keir Fraser
2010-02-07 4:25 ` Jiang, Yunhong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).