Re: [Xen-devel] [PATCH v2 2/5] x86: Handle the Xen MSRs via the new guest_{rd, wr}msr() infrastructure

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.03.18 at 19:58, <andrew.cooper3@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -185,6 +185,10 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, 
> uint64_t *val)
>          }
>  
>          /* Fallthrough. */
> +    case MSR_XEN_ALT_START ... MSR_XEN_ALT_START + NR_XEN_MSRS - 1:

To account for what I've said on patch 1, perhaps this better would
be

    case MSR_HYPERVISOR_START + NR_VIRIDIAN_MSRS - 1 ... MSR_XEN_ALT_START + 
NR_XEN_MSRS - 1:

to produce consistent results regardless of the value of
NR_VIRIDIAN_MSRS (which I suppose is somewhere specified)?

> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -777,31 +777,29 @@ static void do_trap(struct cpu_user_regs *regs)
>            trapnr, trapstr(trapnr), regs->error_code);
>  }
>  
> -/* Returns 0 if not handled, and non-0 for success. */
> -int rdmsr_hypervisor_regs(uint32_t idx, uint64_t *val)
> +int guest_rdmsr_xen(const struct vcpu *v, uint32_t idx, uint64_t *val)
>  {
> -    struct domain *d = current->domain;
> +    const struct domain *d = v->domain;
>      /* Optionally shift out of the way of Viridian architectural MSRs. */
> -    uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
> +    uint32_t base = (is_viridian_domain(d)
> +                     ? MSR_XEN_ALT_START : MSR_HYPERVISOR_START);

Places like this is where it is really helpful to have the constants.

>      switch ( idx - base )
>      {
>      case 0: /* Write hypercall page MSR.  Read as zero. */
> -    {
>          *val = 0;
> -        return 1;
> -    }
> +        return X86EMUL_OKAY;
>      }
>  
> -    return 0;
> +    return X86EMUL_EXCEPTION;
>  }
>  
> -/* Returns 1 if handled, 0 if not and -Exx for error. */
> -int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val)
> +int guest_wrmsr_xen(struct vcpu *v, uint32_t idx, uint64_t val)
>  {
> -    struct domain *d = current->domain;
> +    struct domain *d = v->domain;
>      /* Optionally shift out of the way of Viridian architectural MSRs. */
> -    uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
> +    uint32_t base = (is_viridian_domain(d)
> +                     ? MSR_XEN_ALT_START : MSR_HYPERVISOR_START);
>  
>      switch ( idx - base )
>      {
> @@ -818,7 +816,7 @@ int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val)
>              gdprintk(XENLOG_WARNING,
>                       "wrmsr hypercall page index %#x unsupported\n",
>                       page_index);
> -            return 0;
> +            goto gp_fault;
>          }
>  
>          page = get_page_from_gfn(d, gmfn, &t, P2M_ALLOC);
> @@ -831,13 +829,13 @@ int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val)
>              if ( p2m_is_paging(t) )
>              {
>                  p2m_mem_paging_populate(d, gmfn);
> -                return -ERESTART;
> +                return X86EMUL_RETRY;
>              }
>  
>              gdprintk(XENLOG_WARNING,
>                       "Bad GMFN %lx (MFN %lx) to MSR %08x\n",
>                       gmfn, page ? page_to_mfn(page) : -1UL, base);
> -            return 0;
> +            goto gp_fault;
>          }
>  
>          hypercall_page = __map_domain_page(page);
> @@ -845,11 +843,12 @@ int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val)
>          unmap_domain_page(hypercall_page);
>  
>          put_page_and_type(page);
> -        return 1;
> +        return X86EMUL_OKAY;
>      }
>      }
>  
> -    return 0;
> + gp_fault:
> +    return X86EMUL_EXCEPTION;
>  }

I'll make one more attempt here: Can I talk you into avoiding goto-s
in cases like this?

> --- a/xen/include/asm-x86/msr-index.h
> +++ b/xen/include/asm-x86/msr-index.h
> @@ -543,5 +543,7 @@
>  /* Hypervisor leaves in the 0x4xxxxxxx range. */
>  #define MSR_HYPERVISOR_START            0x40000000
>  #define NR_VIRIDIAN_MSRS                0x00000200
> +#define MSR_XEN_ALT_START               0x40000200
> +#define NR_XEN_MSRS                     0x00000100

Where is this count coming from? I don't think it's part of the public
interface, but if there was such an upper bound I think it should be.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel