[PATCH] x86/MCE: bypass uninitialized vcpu in vMCE injection

[PATCH] x86/MCE: bypass uninitialized vcpu in vMCE injection

[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 <kai.huang@linux.intel.com>
---
 xen/arch/x86/cpu/mcheck/vmce.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/arch/x86/cpu/mcheck/vmce.c b/xen/arch/x86/cpu/mcheck/vmce.c
index c83375e..72fe924 100644
--- a/xen/arch/x86/cpu/mcheck/vmce.c
+++ b/xen/arch/x86/cpu/mcheck/vmce.c
@@ -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) )
-- 
1.9.1

Re: [PATCH] x86/MCE: bypass uninitialized vcpu in vMCE injection

[Jan Beulich]

>>> On 07.05.14 at 09:29, <kai.huang@linux.intel.com> wrote:
> --- a/xen/arch/x86/cpu/mcheck/vmce.c
> +++ b/xen/arch/x86/cpu/mcheck/vmce.c
> @@ -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

Re: [PATCH] x86/MCE: bypass uninitialized vcpu in vMCE injection

[Kai Huang]

On 05/07/2014 04:23 PM, Jan Beulich wrote:
>>>> On 07.05.14 at 09:29, <kai.huang@linux.intel.com> wrote:
>> --- a/xen/arch/x86/cpu/mcheck/vmce.c
>> +++ b/xen/arch/x86/cpu/mcheck/vmce.c
>> @@ -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;
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
>
> 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
>

Re: [PATCH] x86/MCE: bypass uninitialized vcpu in vMCE injection

[Kai Huang]

On 05/07/2014 04:38 PM, Jan Beulich wrote:
>>>> On 07.05.14 at 10:27, <kai.huang@linux.intel.com> wrote:
>> On 05/07/2014 04:23 PM, Jan Beulich wrote:
>>>>>> On 07.05.14 at 09:29, <kai.huang@linux.intel.com> wrote:
>>>> --- a/xen/arch/x86/cpu/mcheck/vmce.c
>>>> +++ b/xen/arch/x86/cpu/mcheck/vmce.c
>>>> @@ -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;
>> 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).
Oh, thanks for noticing that. I'll re-send patch after sanity check on 
my testing machine.

Thanks,
-Kai
>
> Jan
>

Re: [PATCH] x86/MCE: bypass uninitialized vcpu in vMCE injection

[Jan Beulich]

>>> On 07.05.14 at 10:27, <kai.huang@linux.intel.com> wrote:

> On 05/07/2014 04:23 PM, Jan Beulich wrote:
>>>>> On 07.05.14 at 09:29, <kai.huang@linux.intel.com> wrote:
>>> --- a/xen/arch/x86/cpu/mcheck/vmce.c
>>> +++ b/xen/arch/x86/cpu/mcheck/vmce.c
>>> @@ -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;
> 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

Re: [PATCH] x86/MCE: bypass uninitialized vcpu in vMCE injection

[Egger, Christoph]

On 07.05.14 09:29, Kai Huang wrote:
> 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 <kai.huang@linux.intel.com>
> ---
>  xen/arch/x86/cpu/mcheck/vmce.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/xen/arch/x86/cpu/mcheck/vmce.c b/xen/arch/x86/cpu/mcheck/vmce.c
> index c83375e..72fe924 100644
> --- a/xen/arch/x86/cpu/mcheck/vmce.c
> +++ b/xen/arch/x86/cpu/mcheck/vmce.c
> @@ -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;

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

Re: [PATCH] x86/MCE: bypass uninitialized vcpu in vMCE injection

[Kai Huang]

On 05/07/2014 09:25 PM, Egger, Christoph wrote:
> On 07.05.14 09:29, Kai Huang wrote:
>> 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 <kai.huang@linux.intel.com>
>> ---
>>   xen/arch/x86/cpu/mcheck/vmce.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/xen/arch/x86/cpu/mcheck/vmce.c b/xen/arch/x86/cpu/mcheck/vmce.c
>> index c83375e..72fe924 100644
>> --- a/xen/arch/x86/cpu/mcheck/vmce.c
>> +++ b/xen/arch/x86/cpu/mcheck/vmce.c
>> @@ -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;
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
>>           if ( (has_hvm_container_domain(d) ||
>>                 guest_has_trap_callback(d, v->vcpu_id, TRAP_machine_check)) &&
>>                !test_and_set_bool(v->mce_pending) )
>>

Re: [PATCH] x86/MCE: bypass uninitialized vcpu in vMCE injection

[Kai Huang]

On 05/08/2014 09:12 AM, Kai Huang wrote:
>
> On 05/07/2014 09:25 PM, Egger, Christoph wrote:
>> On 07.05.14 09:29, Kai Huang wrote:
>>> 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 <kai.huang@linux.intel.com>
>>> ---
>>>   xen/arch/x86/cpu/mcheck/vmce.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/xen/arch/x86/cpu/mcheck/vmce.c 
>>> b/xen/arch/x86/cpu/mcheck/vmce.c
>>> index c83375e..72fe924 100644
>>> --- a/xen/arch/x86/cpu/mcheck/vmce.c
>>> +++ b/xen/arch/x86/cpu/mcheck/vmce.c
>>> @@ -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;
> 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
>
> Thanks,
> -Kai
>>>           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
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel