All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Jan Beulich" <JBeulich@suse.com>
To: xen-devel <xen-devel@lists.xenproject.org>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: [PATCH v2 2/3] drop pointless and add useful default cases
Date: Mon, 04 Jul 2016 05:55:44 -0600	[thread overview]
Message-ID: <577A6AE002000078000FAD08@prv-mh.provo.novell.com> (raw)
In-Reply-To: <577A69E502000078000FACED@prv-mh.provo.novell.com>

[-- Attachment #1: Type: text/plain, Size: 3144 bytes --]

There's no point in having default cases when all possible values have
respective case statements, or when there's just a "break" statement.

Otoh the two main switch() statements better get default cases added,
just to cover the case of someone altering one of the two lookup arrays
without suitably changing these switch statements.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: use ASSERT_UNREACHABLE() in favor of BUG(), and log a message once.

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1379,7 +1379,6 @@ decode_segment(uint8_t modrm_reg)
     case 3: return x86_seg_ds;
     case 4: return x86_seg_fs;
     case 5: return x86_seg_gs;
-    default: break;
     }
     return decode_segment_failed;
 }
@@ -1503,6 +1502,19 @@ int x86emul_unhandleable_rw(
     return X86EMUL_UNHANDLEABLE;
 }
 
+static void internal_error(const char *which, uint8_t byte,
+                           const struct cpu_user_regs *regs)
+{
+#ifdef __XEN__
+    static bool_t logged;
+
+    if ( !test_and_set_bool(logged) )
+        gprintk(XENLOG_ERR, "Internal error: %s/%02x [%04x:%08lx]\n",
+                which, byte, regs->cs, regs->eip);
+#endif
+    ASSERT_UNREACHABLE();
+}
+
 int
 x86_emulate(
     struct x86_emulate_ctxt *ctxt,
@@ -2996,8 +3008,6 @@ x86_emulate(
             case 7: /* fdivr */
                 emulate_fpu_insn_memsrc("fdivrs", src.val);
                 break;
-            default:
-                goto cannot_emulate;
             }
         }
         break;
@@ -3128,8 +3138,6 @@ x86_emulate(
             case 7: /* fidivr m32i */
                 emulate_fpu_insn_memsrc("fidivrl", src.val);
                 break;
-            default:
-                goto cannot_emulate;
             }
         }
         break;
@@ -3352,8 +3360,6 @@ x86_emulate(
             case 7: /* fidivr m16i */
                 emulate_fpu_insn_memsrc("fidivrs", src.val);
                 break;
-            default:
-                goto cannot_emulate;
             }
         }
         break;
@@ -3431,8 +3437,6 @@ x86_emulate(
                 dst.type = OP_MEM;
                 emulate_fpu_insn_memdst("fistpll", dst.val);
                 break;
-            default:
-                goto cannot_emulate;
             }
         }
         break;
@@ -3750,8 +3754,6 @@ x86_emulate(
             }
             break;
         }
-        default:
-            goto cannot_emulate;
         }
         break;
 
@@ -3845,10 +3847,12 @@ x86_emulate(
             goto push;
         case 7:
             generate_exception_if(1, EXC_UD, -1);
-        default:
-            goto cannot_emulate;
         }
         break;
+
+    default:
+        internal_error("primary", b, ctxt->regs);
+        goto cannot_emulate;
     }
 
  writeback:
@@ -4815,6 +4819,10 @@ x86_emulate(
             break;
         }
         break;
+
+    default:
+        internal_error("secondary", b, ctxt->regs);
+        goto cannot_emulate;
     }
     goto writeback;
 




[-- Attachment #2: x86emul-default-cases.patch --]
[-- Type: text/plain, Size: 3194 bytes --]

x86emul: drop pointless and add useful default cases

There's no point in having default cases when all possible values have
respective case statements, or when there's just a "break" statement.

Otoh the two main switch() statements better get default cases added,
just to cover the case of someone altering one of the two lookup arrays
without suitably changing these switch statements.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: use ASSERT_UNREACHABLE() in favor of BUG(), and log a message once.

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1379,7 +1379,6 @@ decode_segment(uint8_t modrm_reg)
     case 3: return x86_seg_ds;
     case 4: return x86_seg_fs;
     case 5: return x86_seg_gs;
-    default: break;
     }
     return decode_segment_failed;
 }
@@ -1503,6 +1502,19 @@ int x86emul_unhandleable_rw(
     return X86EMUL_UNHANDLEABLE;
 }
 
+static void internal_error(const char *which, uint8_t byte,
+                           const struct cpu_user_regs *regs)
+{
+#ifdef __XEN__
+    static bool_t logged;
+
+    if ( !test_and_set_bool(logged) )
+        gprintk(XENLOG_ERR, "Internal error: %s/%02x [%04x:%08lx]\n",
+                which, byte, regs->cs, regs->eip);
+#endif
+    ASSERT_UNREACHABLE();
+}
+
 int
 x86_emulate(
     struct x86_emulate_ctxt *ctxt,
@@ -2996,8 +3008,6 @@ x86_emulate(
             case 7: /* fdivr */
                 emulate_fpu_insn_memsrc("fdivrs", src.val);
                 break;
-            default:
-                goto cannot_emulate;
             }
         }
         break;
@@ -3128,8 +3138,6 @@ x86_emulate(
             case 7: /* fidivr m32i */
                 emulate_fpu_insn_memsrc("fidivrl", src.val);
                 break;
-            default:
-                goto cannot_emulate;
             }
         }
         break;
@@ -3352,8 +3360,6 @@ x86_emulate(
             case 7: /* fidivr m16i */
                 emulate_fpu_insn_memsrc("fidivrs", src.val);
                 break;
-            default:
-                goto cannot_emulate;
             }
         }
         break;
@@ -3431,8 +3437,6 @@ x86_emulate(
                 dst.type = OP_MEM;
                 emulate_fpu_insn_memdst("fistpll", dst.val);
                 break;
-            default:
-                goto cannot_emulate;
             }
         }
         break;
@@ -3750,8 +3754,6 @@ x86_emulate(
             }
             break;
         }
-        default:
-            goto cannot_emulate;
         }
         break;
 
@@ -3845,10 +3847,12 @@ x86_emulate(
             goto push;
         case 7:
             generate_exception_if(1, EXC_UD, -1);
-        default:
-            goto cannot_emulate;
         }
         break;
+
+    default:
+        internal_error("primary", b, ctxt->regs);
+        goto cannot_emulate;
     }
 
  writeback:
@@ -4815,6 +4819,10 @@ x86_emulate(
             break;
         }
         break;
+
+    default:
+        internal_error("secondary", b, ctxt->regs);
+        goto cannot_emulate;
     }
     goto writeback;
 

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

  parent reply	other threads:[~2016-07-04 11:55 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-07-04 11:51 [PATCH v2 0/3] x86: instruction emulator adjustments Jan Beulich
2016-07-04 11:55 ` [PATCH v2 1/3] use consistent exit mechanism Jan Beulich
2016-07-04 12:54   ` Andrew Cooper
2016-07-04 11:55 ` Jan Beulich [this message]
2016-07-04 12:55   ` [PATCH v2 2/3] drop pointless and add useful default cases Andrew Cooper
2016-07-04 11:56 ` [PATCH v2 3/3] x86emul: fold local variables Jan Beulich

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=577A6AE002000078000FAD08@prv-mh.provo.novell.com \
    --to=jbeulich@suse.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.