Re: [PATCH v2 3/4] x86: Allow non-faulting accesses to non-emulated MSRs if policy permits this

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.01.2021 23:49, Boris Ostrovsky wrote:
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -164,6 +164,34 @@ int init_vcpu_msr_policy(struct vcpu *v)
>      return 0;
>  }
>  
> +/* Returns true if policy requires #GP to the guest. */
> +bool guest_unhandled_msr(const struct vcpu *v, uint32_t msr, uint64_t *val,
> +                         bool is_write, bool is_guest_msr_access)

Would you mind dropping the "_msr" infix from the last
parameter's name? We're anyway aware we're talking about MSR
accesses here, from the function name.

As a nit - while I'm okay with the uint64_t *, I think the
uint32_t would better be unsigned int - see ./CODING_STYLE.

Finally, this being a policy function and policy being per-
domain here, I think the first parameter wants to be const
struct domain *.

> +{
> +    u8 ignore_msrs = v->domain->arch.msr->ignore_msrs;

uint8_t please or, as per above, even better unsigned int.

> +
> +    /*
> +     * Accesses to unimplemented MSRs as part of emulation of instructions
> +     * other than guest's RDMSR/WRMSR should never succeed.
> +     */
> +    if ( !is_guest_msr_access )
> +        ignore_msrs = MSR_UNHANDLED_NEVER;

Wouldn't you better "return true" here? Such accesses also
shouldn't be logged imo (albeit I agree that's a change from
current behavior).

> +    if ( unlikely(ignore_msrs != MSR_UNHANDLED_NEVER) )
> +        *val = 0;

I don't understand the conditional here, even more so with
the respective changelog entry. In any event you don't
want to clobber the value ahead of ...

> +    if ( likely(ignore_msrs != MSR_UNHANDLED_SILENT) )
> +    {
> +        if ( is_write )
> +            gdprintk(XENLOG_WARNING, "WRMSR 0x%08x val 0x%016"PRIx64
> +                    " unimplemented\n", msr, *val);

... logging it.

> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -850,4 +850,10 @@ static inline void x86_emul_reset_event(struct 
> x86_emulate_ctxt *ctxt)
>      ctxt->event = (struct x86_event){};
>  }
>  
> +static inline bool x86_emul_guest_msr_access(struct x86_emulate_ctxt *ctxt)

The parameter wants to be pointer-to-const. In addition I wonder
whether this wouldn't better be a sibling to
x86_insn_is_cr_access() (without a "state" parameter, which
would be unused and unavailable to the callers), which may end
up finding further uses down the road.

> +{
> +    return ctxt->opcode == X86EMUL_OPC(0x0f, 0x32) ||  /* RDMSR */
> +           ctxt->opcode == X86EMUL_OPC(0x0f, 0x30);    /* WRMSR */
> +}

Personally I'd prefer if this was a single comparison:

    return (ctxt->opcode | 2) == X86EMUL_OPC(0x0f, 0x32);

But maybe nowadays' compilers are capable of this
transformation?

I notice you use this function only from PV priv-op emulation.
What about the call paths through hvmemul_{read,write}_msr()?
(It's also questionable whether the write paths need this -
the only MSR written outside of WRMSR emulation is
MSR_SHADOW_GS_BASE, which can't possibly reach the "unhandled"
logic anywhere. But maybe better to be future proof here in
case new MSR writes appear in the emulator, down the road.)

Jan