diff mbox

[11/11] kvm, x86: broadcast mce depending on the cpu version

Message ID 4CB6C580.1090804@np.css.fujitsu.com
State New
Headers show

Commit Message

Jin Dongming Oct. 14, 2010, 8:55 a.m. UTC 
There is no reason why SRAO event received by the main thread
is the only one that being broadcasted.

According to the x86 ASDM vol.3A 15.10.4.1,
MCE signal is broadcast on processor version 06H_EH or later.

This change is required to handle SRAR in the guest.

Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
Tested-by: Jin Dongming <jin.dongming@np.css.fujitsu.com>
---
 qemu-kvm.c |   63 +++++++++++++++++++++++++++++------------------------------
 1 files changed, 31 insertions(+), 32 deletions(-)

Comments

Marcelo Tosatti Oct. 15, 2010, 1:06 a.m. UTC | #1 
On Thu, Oct 14, 2010 at 05:55:28PM +0900, Jin Dongming wrote:
> There is no reason why SRAO event received by the main thread
> is the only one that being broadcasted.
> 
> According to the x86 ASDM vol.3A 15.10.4.1,
> MCE signal is broadcast on processor version 06H_EH or later.
> 
> This change is required to handle SRAR in the guest.
> 
> Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
> Tested-by: Jin Dongming <jin.dongming@np.css.fujitsu.com>
> ---
>  qemu-kvm.c |   63 +++++++++++++++++++++++++++++------------------------------
>  1 files changed, 31 insertions(+), 32 deletions(-)

Why is this necessary? _AO SIGBUS should be sent to all vcpu threads and
main thread.

Please separate bug fixes from cleanups. Very nice, thanks.
Hidetoshi Seto Oct. 15, 2010, 1:52 a.m. UTC | #2 
(2010/10/15 10:06), Marcelo Tosatti wrote:
> On Thu, Oct 14, 2010 at 05:55:28PM +0900, Jin Dongming wrote:
>> There is no reason why SRAO event received by the main thread
>> is the only one that being broadcasted.
>>
>> According to the x86 ASDM vol.3A 15.10.4.1,
>> MCE signal is broadcast on processor version 06H_EH or later.
>>
>> This change is required to handle SRAR in the guest.
>>
>> Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
>> Tested-by: Jin Dongming <jin.dongming@np.css.fujitsu.com>
>> ---
>>  qemu-kvm.c |   63 +++++++++++++++++++++++++++++------------------------------
>>  1 files changed, 31 insertions(+), 32 deletions(-)
> 
> Why is this necessary? _AO SIGBUS should be sent to all vcpu threads and
> main thread.

Humm? If you are right, vcpu threads will receive same SRAO event twice,
one is that received by itself and another is that received by main thread
and forwarded by the broadcast.

My understanding is (Jin, please correct me if something wrong):
 - _AO SIGBUS is sent to main thread only, and then SRAO event is
   broadcasted to all vcpu threads.
 - _AR SIGBUS is sent to a vcpu thread that tried to touch the
   unmapped poisoned page, and SRAR event is posted to the vcpu.

One problem here is that SRAR is not broadcasted.
The guest might observe the event differently, like "some cpus
don't enter machine check."

> Please separate bug fixes from cleanups. Very nice, thanks. 

Maybe this set is considered as 10 cleanups + 1 fix.
I think this fix will be complicated one without preceding cleanups.


Thanks,
H.Seto
Huang, Ying Oct. 15, 2010, 4:56 a.m. UTC | #3 
On Fri, 2010-10-15 at 09:52 +0800, Hidetoshi Seto wrote:
> (2010/10/15 10:06), Marcelo Tosatti wrote:
> > On Thu, Oct 14, 2010 at 05:55:28PM +0900, Jin Dongming wrote:
> >> There is no reason why SRAO event received by the main thread
> >> is the only one that being broadcasted.
> >>
> >> According to the x86 ASDM vol.3A 15.10.4.1,
> >> MCE signal is broadcast on processor version 06H_EH or later.
> >>
> >> This change is required to handle SRAR in the guest.
> >>
> >> Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
> >> Tested-by: Jin Dongming <jin.dongming@np.css.fujitsu.com>
> >> ---
> >>  qemu-kvm.c |   63 +++++++++++++++++++++++++++++------------------------------
> >>  1 files changed, 31 insertions(+), 32 deletions(-)
> > 
> > Why is this necessary? _AO SIGBUS should be sent to all vcpu threads and
> > main thread.
> 
> Humm? If you are right, vcpu threads will receive same SRAO event twice,
> one is that received by itself and another is that received by main thread
> and forwarded by the broadcast.
> 
> My understanding is (Jin, please correct me if something wrong):
>  - _AO SIGBUS is sent to main thread only, and then SRAO event is
>    broadcasted to all vcpu threads.

Yes. It is.

>  - _AR SIGBUS is sent to a vcpu thread that tried to touch the
>    unmapped poisoned page, and SRAR event is posted to the vcpu.
> 
> One problem here is that SRAR is not broadcasted.
> The guest might observe the event differently, like "some cpus
> don't enter machine check."

Yes. SRAR "Broadcast" follows spec better.

Best Regards,
Huang Ying
Marcelo Tosatti Oct. 15, 2010, 1:30 p.m. UTC | #4 
On Fri, Oct 15, 2010 at 10:52:05AM +0900, Hidetoshi Seto wrote:
> (2010/10/15 10:06), Marcelo Tosatti wrote:
> > On Thu, Oct 14, 2010 at 05:55:28PM +0900, Jin Dongming wrote:
> >> There is no reason why SRAO event received by the main thread
> >> is the only one that being broadcasted.
> >>
> >> According to the x86 ASDM vol.3A 15.10.4.1,
> >> MCE signal is broadcast on processor version 06H_EH or later.
> >>
> >> This change is required to handle SRAR in the guest.
> >>
> >> Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
> >> Tested-by: Jin Dongming <jin.dongming@np.css.fujitsu.com>
> >> ---
> >>  qemu-kvm.c |   63 +++++++++++++++++++++++++++++------------------------------
> >>  1 files changed, 31 insertions(+), 32 deletions(-)
> > 
> > Why is this necessary? _AO SIGBUS should be sent to all vcpu threads and
> > main thread.
> 
> Humm? If you are right, vcpu threads will receive same SRAO event twice,
> one is that received by itself and another is that received by main thread
> and forwarded by the broadcast.
> 
> My understanding is (Jin, please correct me if something wrong):
>  - _AO SIGBUS is sent to main thread only, and then SRAO event is
>    broadcasted to all vcpu threads.
>  - _AR SIGBUS is sent to a vcpu thread that tried to touch the
>    unmapped poisoned page, and SRAR event is posted to the vcpu.
> 
> One problem here is that SRAR is not broadcasted.
> The guest might observe the event differently, like "some cpus
> don't enter machine check."

Right.

> > Please separate bug fixes from cleanups. Very nice, thanks. 
> 
> Maybe this set is considered as 10 cleanups + 1 fix.
> I think this fix will be complicated one without preceding cleanups.

Why? All you need is to broadcast from vcpu context.

Please do a minimal fix separately so it can be backported, and the
cleanups can be done later once its merged upstream.

Thanks.
Hidetoshi Seto Oct. 19, 2010, 1:59 a.m. UTC | #5 
(2010/10/15 22:30), Marcelo Tosatti wrote:
> On Fri, Oct 15, 2010 at 10:52:05AM +0900, Hidetoshi Seto wrote:
>> (2010/10/15 10:06), Marcelo Tosatti wrote:
>>> On Thu, Oct 14, 2010 at 05:55:28PM +0900, Jin Dongming wrote:
>>>> There is no reason why SRAO event received by the main thread
>>>> is the only one that being broadcasted.
>>>>
>>>> According to the x86 ASDM vol.3A 15.10.4.1,
>>>> MCE signal is broadcast on processor version 06H_EH or later.
>>>>
>>>> This change is required to handle SRAR in the guest.
>>>>
>>>> Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
>>>> Tested-by: Jin Dongming <jin.dongming@np.css.fujitsu.com>
>>>> ---
>>>>  qemu-kvm.c |   63 +++++++++++++++++++++++++++++------------------------------
>>>>  1 files changed, 31 insertions(+), 32 deletions(-)
>>>
>>> Why is this necessary? _AO SIGBUS should be sent to all vcpu threads and
>>> main thread.
>>
>> Humm? If you are right, vcpu threads will receive same SRAO event twice,
>> one is that received by itself and another is that received by main thread
>> and forwarded by the broadcast.
>>
>> My understanding is (Jin, please correct me if something wrong):
>>  - _AO SIGBUS is sent to main thread only, and then SRAO event is
>>    broadcasted to all vcpu threads.
>>  - _AR SIGBUS is sent to a vcpu thread that tried to touch the
>>    unmapped poisoned page, and SRAR event is posted to the vcpu.
>>
>> One problem here is that SRAR is not broadcasted.
>> The guest might observe the event differently, like "some cpus
>> don't enter machine check."
> 
> Right.
> 
>>> Please separate bug fixes from cleanups. Very nice, thanks. 
>>
>> Maybe this set is considered as 10 cleanups + 1 fix.
>> I think this fix will be complicated one without preceding cleanups.
> 
> Why? All you need is to broadcast from vcpu context.

No, it is not correct. What I really need is reliable QEMU and
maintainable source codes with open community.

Anyway, since I found it could be simpler than what I expected,
I rebased  2 "functional change" pieces in this set to today's
uq/master.

But these are not tested on the tree yet since I could not build
the uq/master due to many warnings on it (even without my fixes).

> Please do a minimal fix separately so it can be backported, and the
> cleanups can be done later once its merged upstream.

When it will be merged?


Thanks,
H.Seto
diff mbox

Patch

diff --git a/qemu-kvm.c b/qemu-kvm.c
index d2b2459..846f0b6 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -1149,6 +1149,34 @@  static int kvm_mce_in_progress(CPUState *env)
     return !!(msr_mcg_status.data & MCG_STATUS_MCIP);
 }
 
+static void kvm_mce_inj_broadcast(CPUState *env, struct kvm_x86_mce *mce)
+{
+    struct kvm_x86_mce mce_sub = {
+        .bank = 1,
+        .status = MCI_STATUS_VAL | MCI_STATUS_UC,
+        .mcg_status = MCG_STATUS_MCIP | MCG_STATUS_RIPV,
+        .addr = 0,
+        .misc = 0,
+    };
+    CPUState *cenv;
+    int family, model, cpuver = env->cpuid_version;
+
+    family = (cpuver >> 8) & 0xf;
+    model = ((cpuver >> 12) & 0xf0) + ((cpuver >> 4) & 0xf);
+
+    kvm_inject_x86_mce_on(env, mce, 1);
+
+    /* Broadcast MCA signal for processor version 06H_EH and above */
+    if ((family == 6 && model >= 14) || family > 6) {
+        for (cenv = first_cpu; cenv != NULL; cenv = cenv->next_cpu) {
+            if (cenv == env) {
+                continue;
+            }
+            kvm_inject_x86_mce_on(cenv, &mce_sub, 1);
+        }
+    }
+}
+
 static void kvm_do_set_mce(CPUState *env, struct kvm_x86_mce *mce,
                            int abort_on_error)
 {
@@ -1175,7 +1203,7 @@  static void kvm_mce_inj_srar_dataload(CPUState *env, target_phys_addr_t paddr)
         .misc = (MCM_ADDR_PHYS << 6) | 0xc,
     };
 
-    kvm_do_set_mce(env, &mce, 1);
+    kvm_mce_inj_broadcast(env, &mce);
 }
 
 static void kvm_mce_inj_srao_memscrub(CPUState *env, target_phys_addr_t paddr)
@@ -1190,32 +1218,7 @@  static void kvm_mce_inj_srao_memscrub(CPUState *env, target_phys_addr_t paddr)
         .misc = (MCM_ADDR_PHYS << 6) | 0xc,
     };
 
-    kvm_do_set_mce(env, &mce, 1);
-}
-
-static void kvm_mce_inj_srao_broadcast(target_phys_addr_t paddr)
-{
-    struct kvm_x86_mce mce_srao_memscrub = {
-        .bank = 9,
-        .status = MCI_STATUS_VAL | MCI_STATUS_UC | MCI_STATUS_EN
-                  | MCI_STATUS_MISCV | MCI_STATUS_ADDRV | MCI_STATUS_S
-                  | 0xc0,
-        .mcg_status = MCG_STATUS_MCIP | MCG_STATUS_RIPV,
-        .addr = paddr,
-        .misc = (MCM_ADDR_PHYS << 6) | 0xc,
-    };
-    struct kvm_x86_mce mce_dummy = {
-        .bank = 1,
-        .status = MCI_STATUS_VAL | MCI_STATUS_UC,
-        .mcg_status = MCG_STATUS_MCIP | MCG_STATUS_RIPV,
-        .addr = 0,
-        .misc = 0,
-    };
-    CPUState *cenv;
-
-    kvm_inject_x86_mce_on(first_cpu, &mce_srao_memscrub, 1);
-    for (cenv = first_cpu->next_cpu; cenv != NULL; cenv = cenv->next_cpu)
-        kvm_inject_x86_mce_on(cenv, &mce_dummy, 1);
+    kvm_mce_inj_broadcast(env, &mce);
 }
 #endif
 
@@ -1255,11 +1258,7 @@  static void kvm_handle_sigbus(CPUState *env, int code, void *vaddr)
             kvm_mce_inj_srar_dataload(target_env, paddr);
         } else {
             /* Fake an Intel architectural Memory scrubbing UCR */
-            if (env) {
-                kvm_mce_inj_srao_memscrub(target_env, paddr);
-            } else {
-                kvm_mce_inj_srao_broadcast(paddr);
-            }
+            kvm_mce_inj_srao_memscrub(target_env, paddr);
         }
         return;
     }