Re: [Patch 2/2] x86/traps: improvements to {rd, wr}msr_hypervisor_regs()

["Jan Beulich" <JBeulich@suse.com>]

>>> On 07.10.13 at 11:48, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -595,55 +595,45 @@ 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 1 if handled, 0 if not and -Exx for error. */

This comment is not in line with all current uses of the function.
Either you fix the comment, or you fix the callers.

>  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 */

This comment looks more confusing that clarifying considering that
we're in "rdmsr_...".

>      {
>          *val = 0;
> -        break;
> +        return 1;
>      }
>      default:
> -        BUG();
> +        return 0;

In a situation like this I think it is better to not have a "default:" at
all, and instead have the "return" at the end of the function deal
with all cases not getting handled inside the switch statement.
But yes, this is a matter of taste.

>      }
> -
> -    return 1;
>  }
>  
> +/* 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 & PAGE_MASK )

Did you mean ~PAGE_MASK? And in the light of this - did you test
the change?

>          {
>              gdprintk(XENLOG_WARNING,
> -                     "Out of range index %u to MSR %08x\n",
> -                     idx, 0x40000000);
> +                     "Expected aligned frame for writing hypercall page\n");

I don't think that's the intention here. Instead the low bits are
specifying the n-th hypercall page, and hence talking about
alignment here seems wrong.

Jan