diff mbox

[07/11] kvm, x86: unify sigbus handling, prep

Message ID 4CB6C427.10202@np.css.fujitsu.com
State New
Headers show

Commit Message

Jin Dongming Oct. 14, 2010, 8:49 a.m. UTC 
There are 2 similar functions to handle SIGBUS:
  sigbus_handler(int n, struct qemu_signalfd_siginfo *siginfo,
                 void *ctx)
  kvm_on_sigbus(CPUState *env, siginfo_t *siginfo)

The former is used when main thread receives SIGBUS via signalfd,
while latter is used when vcpu thread receives SIGBUS.
These 2 take different siginfo, but in both case required parameters
are common, the code and the addr in the info.

Restruct functions to take the code and the addr explicitly.

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

Comments

Marcelo Tosatti Oct. 15, 2010, 12:36 a.m. UTC | #1 
On Thu, Oct 14, 2010 at 05:49:43PM +0900, Jin Dongming wrote:
> There are 2 similar functions to handle SIGBUS:
>   sigbus_handler(int n, struct qemu_signalfd_siginfo *siginfo,
>                  void *ctx)
>   kvm_on_sigbus(CPUState *env, siginfo_t *siginfo)
> 
> The former is used when main thread receives SIGBUS via signalfd,
> while latter is used when vcpu thread receives SIGBUS.
> These 2 take different siginfo, but in both case required parameters
> are common, the code and the addr in the info.
> 
> Restruct functions to take the code and the addr explicitly.
> 
> Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
> Tested-by: Jin Dongming <jin.dongming@np.css.fujitsu.com>
> ---
>  qemu-kvm.c |   41 ++++++++++++++++++++---------------------
>  1 files changed, 20 insertions(+), 21 deletions(-)

Don't see the benefit, separate functions are cleaner.
Hidetoshi Seto Oct. 15, 2010, 1:29 a.m. UTC | #2 
(2010/10/15 9:36), Marcelo Tosatti wrote:
> On Thu, Oct 14, 2010 at 05:49:43PM +0900, Jin Dongming wrote:
>> There are 2 similar functions to handle SIGBUS:
>>   sigbus_handler(int n, struct qemu_signalfd_siginfo *siginfo,
>>                  void *ctx)
>>   kvm_on_sigbus(CPUState *env, siginfo_t *siginfo)
>>
>> The former is used when main thread receives SIGBUS via signalfd,
>> while latter is used when vcpu thread receives SIGBUS.
>> These 2 take different siginfo, but in both case required parameters
>> are common, the code and the addr in the info.
>>
>> Restruct functions to take the code and the addr explicitly.
>>
>> Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
>> Tested-by: Jin Dongming <jin.dongming@np.css.fujitsu.com>
>> ---
>>  qemu-kvm.c |   41 ++++++++++++++++++++---------------------
>>  1 files changed, 20 insertions(+), 21 deletions(-)
> 
> Don't see the benefit, separate functions are cleaner.

I think this is good for maintainability.
If you want to fix a bug in this area, you might have to change
2 separate functions in completely same way.
See 6c85786 and a05684e for examples.

Thanks,
H.Seto
Marcelo Tosatti Oct. 15, 2010, 1:46 a.m. UTC | #3 
On Fri, Oct 15, 2010 at 10:29:25AM +0900, Hidetoshi Seto wrote:
> (2010/10/15 9:36), Marcelo Tosatti wrote:
> > On Thu, Oct 14, 2010 at 05:49:43PM +0900, Jin Dongming wrote:
> >> There are 2 similar functions to handle SIGBUS:
> >>   sigbus_handler(int n, struct qemu_signalfd_siginfo *siginfo,
> >>                  void *ctx)
> >>   kvm_on_sigbus(CPUState *env, siginfo_t *siginfo)
> >>
> >> The former is used when main thread receives SIGBUS via signalfd,
> >> while latter is used when vcpu thread receives SIGBUS.
> >> These 2 take different siginfo, but in both case required parameters
> >> are common, the code and the addr in the info.
> >>
> >> Restruct functions to take the code and the addr explicitly.
> >>
> >> Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
> >> Tested-by: Jin Dongming <jin.dongming@np.css.fujitsu.com>
> >> ---
> >>  qemu-kvm.c |   41 ++++++++++++++++++++---------------------
> >>  1 files changed, 20 insertions(+), 21 deletions(-)
> > 
> > Don't see the benefit, separate functions are cleaner.
> 
> I think this is good for maintainability.
> If you want to fix a bug in this area, you might have to change
> 2 separate functions in completely same way.
> See 6c85786 and a05684e for examples.
> 
> Thanks,
> H.Seto

OK, makes sense. Do you mind to rebase your patchset after MCE support is merged in qemu
upstream?
diff mbox

Patch

diff --git a/qemu-kvm.c b/qemu-kvm.c
index 89ae524..b58181a 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -1219,32 +1219,28 @@  static void kvm_mce_inj_srao_broadcast(target_phys_addr_t paddr)
 }
 #endif
 
-static void sigbus_handler(int n, struct qemu_signalfd_siginfo *siginfo,
-                           void *ctx)
+static void kvm_handle_sigbus(int code, void *vaddr)
 {
 #if defined(KVM_CAP_MCE) && defined(TARGET_I386)
-    if ((first_cpu->mcg_cap & MCG_SER_P) && siginfo->ssi_addr
-        && siginfo->ssi_code == BUS_MCEERR_AO) {
-        void *vaddr;
+    if ((first_cpu->mcg_cap & MCG_SER_P) && vaddr && code == BUS_MCEERR_AO) {
         ram_addr_t ram_addr;
         target_phys_addr_t paddr;
 
         /* Hope we are lucky for AO MCE */
-        vaddr = (void *)(intptr_t)siginfo->ssi_addr;
         if (do_qemu_ram_addr_from_host(vaddr, &ram_addr) ||
             !kvm_physical_memory_addr_from_ram(kvm_state, ram_addr, &paddr)) {
             fprintf(stderr, "Hardware memory error for memory used by "
                     "QEMU itself instead of guest system!: %llx\n",
-                    (unsigned long long)siginfo->ssi_addr);
+                    (unsigned long long)vaddr);
             return;
         }
         kvm_mce_inj_srao_broadcast(paddr);
     } else
 #endif
     {
-        if (siginfo->ssi_code == BUS_MCEERR_AO) {
+        if (code == BUS_MCEERR_AO) {
             return;
-        } else if (siginfo->ssi_code == BUS_MCEERR_AR) {
+        } else if (code == BUS_MCEERR_AR) {
             hardware_memory_error();
         } else {
             sigbus_reraise();
@@ -1252,6 +1248,11 @@  static void sigbus_handler(int n, struct qemu_signalfd_siginfo *siginfo,
     }
 }
 
+static void sigbus_handler(int n, struct qemu_signalfd_siginfo *ssi, void *ctx)
+{
+    kvm_handle_sigbus(ssi->ssi_code, (void *)(intptr_t)ssi->ssi_addr);
+}
+
 static void on_vcpu(CPUState *env, void (*func)(void *data), void *data)
 {
     struct qemu_work_item wi;
@@ -1377,36 +1378,34 @@  static void flush_queued_work(CPUState *env)
     pthread_cond_broadcast(&qemu_work_cond);
 }
 
-static void kvm_on_sigbus(CPUState *env, siginfo_t *siginfo)
+static void kvm_on_sigbus(CPUState *env, int code, void *vaddr)
 {
 #if defined(KVM_CAP_MCE) && defined(TARGET_I386)
-    void *vaddr;
     ram_addr_t ram_addr;
     target_phys_addr_t paddr;
 
-    if ((env->mcg_cap & MCG_SER_P) && siginfo->si_addr
-        && (siginfo->si_code == BUS_MCEERR_AR
-            || siginfo->si_code == BUS_MCEERR_AO)) {
+    if ((env->mcg_cap & MCG_SER_P) && vaddr
+        && (code == BUS_MCEERR_AR || code == BUS_MCEERR_AO)) {
 
         /*
          * If there is an MCE excpetion being processed, ignore this SRAO MCE
          */
-        if (siginfo->si_code == BUS_MCEERR_AO && kvm_mce_in_progress(env)) {
+        if (code == BUS_MCEERR_AO && kvm_mce_in_progress(env)) }
             return;
         }
-        vaddr = (void *)siginfo->si_addr;
+
         if (do_qemu_ram_addr_from_host(vaddr, &ram_addr) ||
             !kvm_physical_memory_addr_from_ram(kvm_state, ram_addr, &paddr)) {
             fprintf(stderr, "Hardware memory error for memory used by "
                     "QEMU itself instead of guest system!\n");
             /* Hope we are lucky for AO MCE */
-            if (siginfo->si_code == BUS_MCEERR_AO) {
+            if (code == BUS_MCEERR_AO) {
                 return;
             } else {
                 hardware_memory_error();
             }
         }
-        if (siginfo->si_code == BUS_MCEERR_AR) {
+        if (code == BUS_MCEERR_AR) {
             /* Fake an Intel architectural Data Load SRAR UCR */
             kvm_mce_inj_srar_dataload(env, paddr);
         } else {
@@ -1416,9 +1415,9 @@  static void kvm_on_sigbus(CPUState *env, siginfo_t *siginfo)
     } else
 #endif
     {
-        if (siginfo->si_code == BUS_MCEERR_AO) {
+        if (code == BUS_MCEERR_AO) {
             return;
-        } else if (siginfo->si_code == BUS_MCEERR_AR) {
+        } else if (code == BUS_MCEERR_AR) {
             hardware_memory_error();
         } else {
             sigbus_reraise();
@@ -1455,7 +1454,7 @@  static void kvm_main_loop_wait(CPUState *env, int timeout)
 
         switch (r) {
         case SIGBUS:
-            kvm_on_sigbus(env, &siginfo);
+            kvm_on_sigbus(env, siginfo.si_code, (void *)siginfo.si_addr);
             break;
         default:
             break;