[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.