Re: [PATCH] x86/msr: Unify the real {rd,wr}msr() paths in guest_{rd,wr}msr()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.07.2020 12:55, Andrew Cooper wrote:
> Both the read and write side have commonalities which can be abstracted away.
> This also allows for additional safety in release builds, and slightly more
> helpful diagnostics in debug builds.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> I'm not a massive fan of the global scope want_rdmsr_safe boolean, but I can't
> think of a reasonable way to fix it without starting to use other
> flexibiltiies offered to us by C99.

I can't seem to be able to guess what C99 feature(s) you mean.
If there are any that would help, why not use them?

> @@ -204,10 +205,9 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t 
> *val)
>           */
>          if ( !(cp->x86_vendor & (X86_VENDOR_INTEL | X86_VENDOR_AMD)) ||
>               !(boot_cpu_data.x86_vendor &
> -               (X86_VENDOR_INTEL | X86_VENDOR_AMD)) ||
> -             rdmsr_safe(MSR_AMD_PATCHLEVEL, *val) )
> +               (X86_VENDOR_INTEL | X86_VENDOR_AMD)) )
>              goto gp_fault;
> -        break;
> +        goto read_from_hw_safe;

Above from here is a read from MSR_IA32_PLATFORM_ID - any reason
it doesn't also get folded?

> @@ -278,7 +278,7 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t 
> *val)
>           */
>  #ifdef CONFIG_HVM
>          if ( v == current && is_hvm_domain(d) && v->arch.hvm.flag_dr_dirty )
> -            rdmsrl(msr, *val);
> +            goto read_from_hw;

In the write path you also abstract out the check for v being current.
Wouldn't this better be abstracted out here, too, as reading an actual
MSR when not current isn't generally very helpful?

> @@ -493,8 +506,8 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t 
> val)
>                                 ? 0 : (msr - MSR_AMD64_DR1_ADDRESS_MASK + 1),
>                                 ARRAY_SIZE(msrs->dr_mask))] = val;
>  
> -        if ( v == curr && (curr->arch.dr7 & DR7_ACTIVE_MASK) )
> -            wrmsrl(msr, val);
> +        if ( curr->arch.dr7 & DR7_ACTIVE_MASK )
> +            goto maybe_write_to_hw;
>          break;

I have to admit that I'd find it more logical if v was now used
here instead of curr.

> @@ -509,6 +522,23 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t 
> val)
>  
>      return ret;
>  
> + maybe_write_to_hw:
> +    /*
> +     * All paths potentially updating a value in hardware need to check
> +     * whether the call is in current context or not, so the logic is
> +     * implemented here.  Remote context need do nothing more.
> +     */
> +    if ( v != curr || !wrmsr_safe(msr, val) )
> +        return X86EMUL_OKAY;
> +
> +    /*
> +     * Paths which end up here took a #GP fault in wrmsr_safe().  Something 
> is
> +     * broken with the logic above, so make it obvious in debug builds, and
> +     * fail safe by handing #GP back to the guests in release builds.
> +     */
> +    gprintk(XENLOG_ERR, "Bad wrmsr %#x val %016"PRIx64"\n", msr, val);

Didn't you indicate more than once that you dislike mixing 0x-
prefixed and non-prefixed hex values in a single message?
(Personally I'd simple drop the #, but I expect you to prefer it
the other way around.)

Also both here and in the read path I'm unconvinced of the
"by handing #GP back" wording: When v != curr, no #GP fault can
typically be handed anywhere. And even when v == curr it's still
up to the caller to decide what to do. IOW how about "by
suggesting to hand back #GP" or some such?

Jan