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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.01.2021 20:52, Boris Ostrovsky wrote:
> On 1/22/21 7:51 AM, Jan Beulich wrote:
>> On 20.01.2021 23:49, Boris Ostrovsky wrote:
>>> +
>>> +    /*
>>> +     * 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).
> 
> 
> Yes, that's why I didn't return here. We will be here in !is_guest_msr_access 
> case most likely due to a bug in the emulator so I think we do want to see 
> the error logged.

Why "most likely"?

>>> +    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.
> 
> 
> True. I dropped !is_write from v1 without considering this.
> 
> As far as the conditional --- dropping it too would be a behavior change. 

Albeit an intentional one then? Plus I think I have trouble
seeing what behavior it would be that would change.

>>> --- 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.
> 
> 
> "Sibling" in terms of name (yes, it would be) or something else?

Name and (possible) purpose - a validate hook could want to
make use of this, for example.

>>> +{
>>> +    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?
> 
> Here is what I've got (not an inline but shouldn't make much difference I'd 
> think)
> 
> ffff82d040385960 <x86_emul_guest_msr_access_2>: # your code
> ffff82d040385960:       8b 47 2c                mov    0x2c(%rdi),%eax
> ffff82d040385963:       83 e0 fd                and    $0xfffffffd,%eax
> ffff82d040385966:       3d 30 00 0f 00          cmp    $0xf0030,%eax
> ffff82d04038596b:       0f 94 c0                sete   %al
> ffff82d04038596e:       c3                      retq
> 
> ffff82d04038596f <x86_emul_guest_msr_access_1>: # my code
> ffff82d04038596f:       8b 47 2c                mov    0x2c(%rdi),%eax
> ffff82d040385972:       83 c8 02                or     $0x2,%eax
> ffff82d040385975:       3d 32 00 0f 00          cmp    $0xf0032,%eax
> ffff82d04038597a:       0f 94 c0                sete   %al
> ffff82d04038597d:       c3                      retq
> 
> 
> So it's a wash in terms of generated code.

True, albeit I guess you got "your code" and "my code" the
wrong way round, as I don't expect the compiler to
translate | into "and".

>> 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.)
> 
> 
> Won't we end up in hvm_funcs.msr_write_intercept ops which do call it?

Of course we will - the boolean will very likely need
propagating (a possible alternative being a per-vCPU flag
indicating "in emulator").

Jan