Skip to contentSkip to conversation

x86/MCE: bypass uninitialized vcpu in vMCE injection

PATCH Superseded Kai Huang 3 participants 8 messages May 7, 2014 – May 8, 2014

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Kai Huang

author
#0 lore raw

PATCH 1/1 x86/MCE: bypass uninitialized vcpu in vMCE injection

Dom0 may bring up less number of vCPUs than xen hypervisor actually created for it, and in this case, on Intel platform, vMCE injection to dom0 will fail due to injecting vMCE to uninitialized vcpu, and cause dom0 crash.

Signed-off-by Kai Huang <[email protected]>
---
 xen/arch/x86/cpu/mcheck/vmce.c | 4 ++++
 1 file changed, 4 insertions(+)
xen/arch/x86/cpu/mcheck/vmce.c +4 -0
index c83375e..72fe924 100644
@@ -356,6 +356,10 @@ int inject_vmce(struct domain *d, int vcpu)
         if ( vcpu != VMCE_INJECT_BROADCAST && vcpu != v->vcpu_id )
             continue;
 
+        /* In case of broadcasting, don't inject to uninitialized VCPU */
+        if ( vcpu == VMCE_INJECT_BROADCAST && !v->is_initialised )
+            continue;
+
         if ( (has_hvm_container_domain(d) ||
               guest_has_trap_callback(d, v->vcpu_id, TRAP_machine_check)) &&
              !test_and_set_bool(v->mce_pending) )
Signature · 1 line
1.9.1
#1 lore raw

xen/arch/x86/cpu/mcheck/vmce.c int inject_vmce(struct domain *d, int vcpu) · line 362

Quoted 11 lines
Quoted 1 line
Quoted 1 line · On 07.05.14 at 09:29, <[email protected]> wrote:
xen/arch/x86/cpu/mcheck/vmce.c +4 -0
@@ -356,6 +356,10 @@ int inject_vmce(struct domain *d, int vcpu)
         if ( vcpu != VMCE_INJECT_BROADCAST && vcpu != v->vcpu_id )
             continue;
 
+        /* In case of broadcasting, don't inject to uninitialized VCPU */
+        if ( vcpu == VMCE_INJECT_BROADCAST && !v->is_initialised )
+            continue;
+

Conceptually fine, but mechanically in need of improvement: Please fold the check with the previous one, to avoid checking for VMCE_INJECT_BROADCAST twice.

I'd do this as

        if ( vcpu != VMCE_INJECT_BROADCAST ? vcpu != v->vcpu_id
                                           : !v->is_initialised )
            continue;

but if you prefer some other style (like nested if/else), that would be fine with me too.

Also please don't forget to also Cc the second x86/MCE maintainer.

Jan

author
#2 lore raw
Quoted 15 lines · Jan Beulich

xen/arch/x86/cpu/mcheck/vmce.c int inject_vmce(struct domain *d, int vcpu) · line 362

Quoted 11 lines
Quoted 1 line
Quoted 1 line · On 07.05.14 at 09:29, <[email protected]> wrote:
xen/arch/x86/cpu/mcheck/vmce.c +4 -0
@@ -356,6 +356,10 @@ int inject_vmce(struct domain *d, int vcpu)
          if ( vcpu != VMCE_INJECT_BROADCAST && vcpu != v->vcpu_id )
              continue;
 
+        /* In case of broadcasting, don't inject to uninitialized VCPU */
+        if ( vcpu == VMCE_INJECT_BROADCAST && !v->is_initialised )
+            continue;
+

Conceptually fine, but mechanically in need of improvement: Please fold the check with the previous one, to avoid checking for VMCE_INJECT_BROADCAST twice.

Quoted 5 lines

I'd do this as

         if ( vcpu != VMCE_INJECT_BROADCAST ? vcpu != v->vcpu_id
                                            : !v->is_initialised )
             continue;

Thanks for comments. In this case, is it OK to you to add below comments just before the if statement? I think it's better to keep the comments somewhere.

	/* In case of broadcasting, don't inject to uninitialized VCPU */
	if ( vcpu != VMCE_INJECT_BROADCAST ? vcpu != v->vcpu_id
                                           : !v->is_initialised )
            continue;

Thanks,
-Kai

Quoted 8 lines

but if you prefer some other style (like nested if/else), that would be fine with me too.

Also please don't forget to also Cc the second x86/MCE maintainer.

Jan

#3 lore raw
Quoted 23 lines
Quoted 1 line
Quoted 1 line · On 07.05.14 at 10:27, <[email protected]> wrote:
Quoted 15 lines · Jan Beulich

xen/arch/x86/cpu/mcheck/vmce.c int inject_vmce(struct domain *d, int vcpu) · line 362

Quoted 11 lines
Quoted 1 line
Quoted 1 line · On 07.05.14 at 09:29, <[email protected]> wrote:
xen/arch/x86/cpu/mcheck/vmce.c +4 -0
@@ -356,6 +356,10 @@ int inject_vmce(struct domain *d, int vcpu)
          if ( vcpu != VMCE_INJECT_BROADCAST && vcpu != v->vcpu_id )
              continue;
 
+        /* In case of broadcasting, don't inject to uninitialized VCPU */
+        if ( vcpu == VMCE_INJECT_BROADCAST && !v->is_initialised )
+            continue;
+

Conceptually fine, but mechanically in need of improvement: Please fold the check with the previous one, to avoid checking for VMCE_INJECT_BROADCAST twice.

Quoted 5 lines

I'd do this as

         if ( vcpu != VMCE_INJECT_BROADCAST ? vcpu != v->vcpu_id
                                            : !v->is_initialised )
             continue;
Quoted 9 lines

Thanks for comments. In this case, is it OK to you to add below comments just before the if statement? I think it's better to keep the comments somewhere.

	/* In case of broadcasting, don't inject to uninitialized VCPU */
	if ( vcpu != VMCE_INJECT_BROADCAST ? vcpu != v->vcpu_id
                                            : !v->is_initialised )
             continue;

Yes, of course it's fine to keep the comment (but please fix its style
- it's missing a stop at the end).

Jan

author
#4 lore raw
Quoted 26 lines · Jan Beulich
Quoted 26 lines
Quoted 1 line
Quoted 1 line · On 07.05.14 at 10:27, <[email protected]> wrote:
Quoted 15 lines · Jan Beulich

xen/arch/x86/cpu/mcheck/vmce.c int inject_vmce(struct domain *d, int vcpu) · line 362

Quoted 11 lines
Quoted 1 line
Quoted 1 line · On 07.05.14 at 09:29, <[email protected]> wrote:
xen/arch/x86/cpu/mcheck/vmce.c +4 -0
@@ -356,6 +356,10 @@ int inject_vmce(struct domain *d, int vcpu)
           if ( vcpu != VMCE_INJECT_BROADCAST && vcpu != v->vcpu_id )
               continue;
 
+        /* In case of broadcasting, don't inject to uninitialized VCPU */
+        if ( vcpu == VMCE_INJECT_BROADCAST && !v->is_initialised )
+            continue;
+

Conceptually fine, but mechanically in need of improvement: Please fold the check with the previous one, to avoid checking for VMCE_INJECT_BROADCAST twice.

Quoted 5 lines

I'd do this as

          if ( vcpu != VMCE_INJECT_BROADCAST ? vcpu != v->vcpu_id
                                             : !v->is_initialised )
              continue;

Thanks for comments. In this case, is it OK to you to add below comments just before the if statement? I think it's better to keep the comments somewhere.

Quoted 7 lines
Quoted 5 lines
	/* In case of broadcasting, don't inject to uninitialized VCPU */
	if ( vcpu != VMCE_INJECT_BROADCAST ? vcpu != v->vcpu_id
                                             : !v->is_initialised )
              continue;

Yes, of course it's fine to keep the comment (but please fix its style
- it's missing a stop at the end).

Oh, thanks for noticing that. I'll re-send patch after sanity check on my testing machine.

Thanks,
-Kai

Quoted 3 lines

Jan

#5 lore raw
In reply to Kai Huang
Quoted 14 lines · Kai Huang

Dom0 may bring up less number of vCPUs than xen hypervisor actually created for it, and in this case, on Intel platform, vMCE injection to dom0 will fail due to injecting vMCE to uninitialized vcpu, and cause dom0 crash.

Signed-off-by Kai Huang <[email protected]>
---
 xen/arch/x86/cpu/mcheck/vmce.c | 4 ++++
 1 file changed, 4 insertions(+)
xen/arch/x86/cpu/mcheck/vmce.c +0 -0
index c83375e..72fe924 100644

xen/arch/x86/cpu/mcheck/vmce.c int inject_vmce(struct domain *d, int vcpu) · line 362

Quoted 7 lines
xen/arch/x86/cpu/mcheck/vmce.c +4 -0
@@ -356,6 +356,10 @@ int inject_vmce(struct domain *d, int vcpu)
         if ( vcpu != VMCE_INJECT_BROADCAST && vcpu != v->vcpu_id )
             continue;
 
+        /* In case of broadcasting, don't inject to uninitialized VCPU */
+        if ( vcpu == VMCE_INJECT_BROADCAST && !v->is_initialised )
+            continue;
+

What happens when you inject to exactly one uninitialized VCPU? I think what you want is this:

           /* Don't inject to uninitialized VCPU */
           if ( !v->is_initialised )
               continue;
Quoted 4 lines
xen/arch/x86/cpu/mcheck/vmce.c +0 -0
         if ( (has_hvm_container_domain(d) ||
               guest_has_trap_callback(d, v->vcpu_id, TRAP_machine_check)) &&
              !test_and_set_bool(v->mce_pending) )
author
#6 lore raw
Quoted 25 lines · Christoph
Quoted 14 lines · Kai Huang

Dom0 may bring up less number of vCPUs than xen hypervisor actually created for it, and in this case, on Intel platform, vMCE injection to dom0 will fail due to injecting vMCE to uninitialized vcpu, and cause dom0 crash.

Signed-off-by Kai Huang <[email protected]>
---
  xen/arch/x86/cpu/mcheck/vmce.c | 4 ++++
  1 file changed, 4 insertions(+)
xen/arch/x86/cpu/mcheck/vmce.c +0 -0
index c83375e..72fe924 100644

xen/arch/x86/cpu/mcheck/vmce.c int inject_vmce(struct domain *d, int vcpu) · line 362

Quoted 7 lines
xen/arch/x86/cpu/mcheck/vmce.c +4 -0
@@ -356,6 +356,10 @@ int inject_vmce(struct domain *d, int vcpu)
          if ( vcpu != VMCE_INJECT_BROADCAST && vcpu != v->vcpu_id )
              continue;
 
+        /* In case of broadcasting, don't inject to uninitialized VCPU */
+        if ( vcpu == VMCE_INJECT_BROADCAST && !v->is_initialised )
+            continue;
+

What happens when you inject to exactly one uninitialized VCPU? I think what you want is this:

Quoted 3 lines
            /* Don't inject to uninitialized VCPU */
            if ( !v->is_initialised )
                continue;

Good suggestion indeed. In inject_vmce, the ret is initialized to -ESRCH at beginning. In case of injecting to one particular vcpu, we will simply bypass all loops of "for_each_vcpu" and don't do the injection at all, while the ret remains -ESRCH. Looks it's the right behaviour for AMD case.

Thanks Egger for comments (and sorry that I forgot to CC you at beginning:)). I'll follow your suggestion.

Thanks,
-Kai

Quoted 4 lines
Quoted 4 lines
xen/arch/x86/cpu/mcheck/vmce.c +0 -0
          if ( (has_hvm_container_domain(d) ||
                guest_has_trap_callback(d, v->vcpu_id, TRAP_machine_check)) &&
               !test_and_set_bool(v->mce_pending) )
author
#7 lore raw
Quoted 27 lines · Kai Huang
Quoted 25 lines · Christoph
Quoted 18 lines · Kai Huang

Dom0 may bring up less number of vCPUs than xen hypervisor actually created for it, and in this case, on Intel platform, vMCE injection to dom0 will fail due to injecting vMCE to uninitialized vcpu, and cause dom0 crash.

Signed-off-by Kai Huang <[email protected]>
---
  xen/arch/x86/cpu/mcheck/vmce.c | 4 ++++
  1 file changed, 4 insertions(+)
xen/arch/x86/cpu/mcheck/vmce.c +0 -0
index c83375e..72fe924 100644
@@ -356,6 +356,10 @@ int inject_vmce(struct domain *d, int vcpu)
          if ( vcpu != VMCE_INJECT_BROADCAST && vcpu != v->vcpu_id )
              continue;
  +        /* In case of broadcasting, don't inject to uninitialized

VCPU */

xen/arch/x86/cpu/mcheck/vmce.c int inject_vmce(struct domain *d, int vcpu) · line 362

Quoted 3 lines
xen/arch/x86/cpu/mcheck/vmce.c +3 -0
+        if ( vcpu == VMCE_INJECT_BROADCAST && !v->is_initialised )
+            continue;
+

What happens when you inject to exactly one uninitialized VCPU? I think what you want is this:

Quoted 8 lines
Quoted 3 lines
            /* Don't inject to uninitialized VCPU */
            if ( !v->is_initialised )
                continue;

Good suggestion indeed. In inject_vmce, the ret is initialized to
-ESRCH at beginning. In case of injecting to one particular vcpu, we
will simply bypass all loops of "for_each_vcpu" and don't do the injection at all, while the ret remains -ESRCH. Looks it's the right behaviour for AMD case.

Thanks Egger for comments (and sorry that I forgot to CC you at beginning:)). I'll follow your suggestion.

Hi Egger,

Forgot to say, the reason I didn't do this at beginning is this code will potentially impact vMCE injection behaviour on AMD platform. Considering I don't have AMD machine to test, so I can just do a theoretically analysis here. If you are OK with this, I'll follow your suggestion, Otherwise, I think Jan's suggestion is also good enough.

Thanks,
-Kai

Quoted 14 lines

Thanks,
-Kai

Quoted 5 lines
Quoted 5 lines
          if ( (has_hvm_container_domain(d) ||
                guest_has_trap_callback(d, v->vcpu_id, 
TRAP_machine_check)) &&
               !test_and_set_bool(v->mce_pending) )

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel