[Xen-devel] [Patch v2] x86/traps: improvements to {rd, wr}msr_hypervisor_regs()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

Coverity ID: 1055249 1055250

Coverity was complaining that the switch statments contained dead code in
their default statements.  While this is quite minor, the code flow in
wrmsr_hypervisor_regs() was sufficiently opaque that I felt it approprate to
fix.

Other improvements include:
 * not shadowing the function parameter 'idx'.
 * use of PAGE_SHIFT instead of opencoded numbers.
 * a more descriptive error message for attempting to write invalid indicies
   for hypercall pages.

There is no behavioural change as a result.

Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
CC: Keir Fraser <keir@xxxxxxx>
CC: Jan Beulich <JBeulich@xxxxxxxx>
---
 xen/arch/x86/traps.c |   44 ++++++++++++++++++--------------------------
 1 file changed, 18 insertions(+), 26 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 6c7bd99..8797e56 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -595,55 +595,50 @@ DO_ERROR_NOCODE(TRAP_copro_error,     coprocessor_error)
 DO_ERROR(       TRAP_alignment_check, alignment_check)
 DO_ERROR_NOCODE(TRAP_simd_error,      simd_coprocessor_error)
 
+/*
+ * Returns 0 if not handled, and non-0 for error. (The calling semantics are
+ * in need of some work)
+ */
 int rdmsr_hypervisor_regs(uint32_t idx, uint64_t *val)
 {
     struct domain *d = current->domain;
     /* Optionally shift out of the way of Viridian architectural MSRs. */
     uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
 
-    idx -= base;
-    if ( idx > 0 )
-        return 0;
-
-    switch ( idx )
+    switch ( idx - base )
     {
-    case 0:
+    case 0: /* Write hypercall page.  Reads are invalid. Hand a #GP back. */
     {
         *val = 0;
-        break;
+        return 1;
     }
-    default:
-        BUG();
     }
 
-    return 1;
+    return 0;
 }
 
+/* Returns 1 if handled, 0 if not and -Exx for error. */
 int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val)
 {
     struct domain *d = current->domain;
     /* Optionally shift out of the way of Viridian architectural MSRs. */
     uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
 
-    idx -= base;
-    if ( idx > 0 )
-        return 0;
-
-    switch ( idx )
+    switch ( idx - base )
     {
-    case 0:
+    case 0: /* Write hypercall page */
     {
         void *hypercall_page;
-        unsigned long gmfn = val >> 12;
-        unsigned int idx  = val & 0xfff;
+        unsigned long gmfn = val >> PAGE_SHIFT;
         struct page_info *page;
         p2m_type_t t;
 
-        if ( idx > 0 )
+        if ( val & 0xfff )
         {
+            /* Bottom 12 bits are hypercall page index.  Only 0 is valid. */
             gdprintk(XENLOG_WARNING,
-                     "Out of range index %u to MSR %08x\n",
-                     idx, 0x40000000);
+                     "wrmsr hypercall page index %#x unsupported\n",
+                     val & 0xfff);
             return 0;
         }
 
@@ -671,14 +666,11 @@ int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val)
         unmap_domain_page(hypercall_page);
 
         put_page_and_type(page);
-        break;
+        return 1;
     }
-
-    default:
-        BUG();
     }
 
-    return 1;
+    return 0;
 }
 
 int cpuid_hypervisor_leaves( uint32_t idx, uint32_t sub_idx,
-- 
1.7.10.4


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