Re: [Xen-devel] [PATCH v4 1/3] x86/msr: Dispatch Xen and Viridian MSRs from guest_{wr, rd}msr()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.09.18 at 14:00, <andrew.cooper3@xxxxxxxxxx> wrote:
> Despite the complicated diff in {svm,vmx}_msr_write_intercept(), it is just
> the 0 case losing one level of indentation, as part of removing the call to
> wrmsr_hypervisor_regs().
> 
> The case blocks in guest_{wr,rd}msr() use raw numbers, partly for consistency
> with the CPUID side of things, but mainly because this is clearer code to
> follow.  In particular, the Xen block may overlap with the Viridian block if
> Viridian is not enabled for the domain, and trying to express this with named
> literals caused more confusion that it solved.

Well, imo it's not much better this way, but also not much worse.

> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -114,9 +114,11 @@ int init_vcpu_msr_policy(struct vcpu *v)
>  
>  int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>  {
> -    const struct cpuid_policy *cp = v->domain->arch.cpuid;
> -    const struct msr_policy *mp = v->domain->arch.msr;
> +    const struct domain *d = v->domain;
> +    const struct cpuid_policy *cp = d->arch.cpuid;
> +    const struct msr_policy *mp = d->arch.msr;
>      const struct vcpu_msrs *msrs = v->arch.msrs;
> +    int ret = X86EMUL_OKAY;
>  
>      switch ( msr )
>      {
> @@ -145,11 +147,25 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, 
> uint64_t *val)
>          *val = msrs->misc_features_enables.raw;
>          break;
>  
> +    case 0x40000000 ... 0x400001ff:
> +        if ( is_viridian_domain(d) )
> +        {
> +            ret = (rdmsr_viridian_regs(msr, val)
> +                   ? X86EMUL_OKAY : X86EMUL_EXCEPTION);
> +            break;
> +        }
> +
> +        /* Fallthrough. */
> +    case 0x40000200 ... 0x400002ff:
> +        ret = (rdmsr_hypervisor_regs(msr, val)
> +               ? X86EMUL_OKAY : X86EMUL_EXCEPTION);
> +        break;
> +
>      default:
>          return X86EMUL_UNHANDLEABLE;

Considering the "return" here, could I talk you into using "return"
for your additions as well, rather than introducing a local variable?
Same for the wrmsr side then. In any event
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan



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