[Qemu-devel] [PATCH 11/11] kvm, x86: broadcast mce depending on the cpu version

[Qemu-devel] [PATCH 11/11] kvm, x86: broadcast mce depending on the cpu version

[Jin Dongming]

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(-)

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;
     }
-- 
1.7.1.1

[Qemu-devel] Re: [PATCH 11/11] kvm, x86: broadcast mce depending on the cpu version

[Marcelo Tosatti]

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. 

Re: [Qemu-devel] Re: [PATCH 11/11] kvm, x86: broadcast mce depending on the cpu version

[Hidetoshi Seto]

(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

Re: [Qemu-devel] Re: [PATCH 11/11] kvm, x86: broadcast mce depending on the cpu version

[Huang Ying]

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

Re: [Qemu-devel] Re: [PATCH 11/11] kvm, x86: broadcast mce depending on the cpu version

[Marcelo Tosatti]

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.

Re: [Qemu-devel] Re: [PATCH 11/11] kvm, x86: broadcast mce depending on the cpu version

[Hidetoshi Seto]

(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

[Qemu-devel] [PATCH uq/master 1/2] kvm, x86: ignore SRAO only when MCG_SER_P is available

[Hidetoshi Seto]

And restruct this block to call kvm_mce_in_exception() only when it is
required.

Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
---
 target-i386/kvm.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index d940175..98a0505 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -239,12 +239,16 @@ static void kvm_do_inject_x86_mce(void *_data)
     struct kvm_x86_mce_data *data = _data;
     int r;
 
-    /* If there is an MCE excpetion being processed, ignore this SRAO MCE */
-    r = kvm_mce_in_exception(data->env);
-    if (r == -1)
-        fprintf(stderr, "Failed to get MCE status\n");
-    else if (r && !(data->mce->status & MCI_STATUS_AR))
-        return;
+    /* If there is an MCE exception being processed, ignore this SRAO MCE */
+    if ((data->env->mcg_cap & MCG_SER_P) &&
+        !(data->mce->status & MCI_STATUS_AR)) {
+        r = kvm_mce_in_exception(data->env);
+        if (r == -1) {
+            fprintf(stderr, "Failed to get MCE status\n");
+        } else if (r) {
+            return;
+        }
+    }
 
     r = kvm_set_mce(data->env, data->mce);
     if (r < 0) {
-- 
1.7.3.1

[Qemu-devel] [PATCH uq/master 2/2] kvm, x86: broadcast mce depending on the cpu version

[Hidetoshi Seto]

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 smp guests.

Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
---
 target-i386/kvm.c |   28 ++++++++++++++++++++++++----
 1 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 98a0505..e97fbd3 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1631,6 +1631,28 @@ static void hardware_memory_error(void)
     exit(1);
 }
 
+#ifdef KVM_CAP_MCE
+static void kvm_mce_broadcast_rest(CPUState *env)
+{
+    CPUState *cenv;
+    int family, model, cpuver = env->cpuid_version;
+
+    family = (cpuver >> 8) & 0xf;
+    model = ((cpuver >> 12) & 0xf0) + ((cpuver >> 4) & 0xf);
+
+    /* Broadcast MCA signal for processor version 06H_EH and above */
+    if ((family == 6 && model >= 14) || family > 6) {
+        if (cenv == env) {
+            continue;
+        }
+        for (cenv = first_cpu; cenv != NULL; cenv = cenv->next_cpu) {
+            kvm_inject_x86_mce(cenv, 1, MCI_STATUS_VAL | MCI_STATUS_UC,
+                               MCG_STATUS_MCIP | MCG_STATUS_RIPV, 0, 0, 1);
+        }
+    }
+}
+#endif
+
 int kvm_on_sigbus_vcpu(CPUState *env, int code, void *addr)
 {
 #if defined(KVM_CAP_MCE)
@@ -1688,6 +1710,7 @@ int kvm_on_sigbus_vcpu(CPUState *env, int code, void *addr)
             fprintf(stderr, "kvm_set_mce: %s\n", strerror(errno));
             abort();
         }
+        kvm_mce_broadcast_rest(env);
     } else
 #endif
     {
@@ -1726,10 +1749,7 @@ int kvm_on_sigbus(int code, void *addr)
         kvm_inject_x86_mce(first_cpu, 9, status,
                            MCG_STATUS_MCIP | MCG_STATUS_RIPV, paddr,
                            (MCM_ADDR_PHYS << 6) | 0xc, 1);
-        for (cenv = first_cpu->next_cpu; cenv != NULL; cenv = cenv->next_cpu) {
-            kvm_inject_x86_mce(cenv, 1, MCI_STATUS_VAL | MCI_STATUS_UC,
-                               MCG_STATUS_MCIP | MCG_STATUS_RIPV, 0, 0, 1);
-        }
+        kvm_mce_broadcast_rest(first_cpu);
     } else
 #endif
     {
-- 
1.7.3.1