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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: xen-devel-bounces@xxxxxxxxxxxxx [mailto:xen-devel-
> bounces@xxxxxxxxxxxxx] On Behalf Of Andrew Cooper
> Sent: 07 October 2013 13:01
> To: Xen-devel
> Cc: Andrew Cooper; Keir (Xen.org); Jan Beulich
> Subject: [Xen-devel] [Patch v3] x86/traps: improvements to {rd,
> wr}msr_hypervisor_regs()
> 
> 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..3bb62f0 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 )

If you're not going to use 12, then shouldn't you use PAGE_SIZE-1 rather than 
0xfff?

>          {
> +            /* Bottom 12 bits are hypercall page index.  Only 0 is valid. */

And modify this comment accordingly?

  Paul

>              gdprintk(XENLOG_WARNING,
> -                     "Out of range index %u to MSR %08x\n",
> -                     idx, 0x40000000);
> +                     "wrmsr hypercall page index %#lx 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

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