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

[Jan Beulich <jbeulich@suse.com>]

On 25.01.2021 19:42, Boris Ostrovsky wrote:
> On 21-01-25 11:22:08, Jan Beulich wrote:
>> 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"?
> 
> 
> OK, definitely ;-)

Oops - I was thinking the other way around, considering such
to possibly be legitimate. It just so happens that curently
we have no such path.

> But I still think logging these accesses would be helpful.

Because of the above I continue to question this.

>>>>> +    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.
> 
> 
> Currently callers of, say, read_msr() don't expect the argument that they pass in to change. Granted, they shouldn't (and AFAICS don't) look at it but it's a change nonetheless.

Hmm, I'm confused: The purpose of read_msr() is to change the
value pointed at by the passed in argument. And for write_msr()
the users of the hook pass the argument by value, i.e. wouldn't
observe the changed value (it would only possibly be
intermediate layers which might observe the change, but those
ought to not care).

>>>>> --- 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.
> 
> A validate hook? 

Quoting from struct x86_emulate_ops:

    /*
     * validate: Post-decode, pre-emulate hook to allow caller controlled
     * filtering.
     */
    int (*validate)(
        const struct x86_emulate_state *state,
        struct x86_emulate_ctxt *ctxt);

Granted to be directly usable the function would need to have a
"state" parameter. As that's unused, having it have one and
passing NULL in your case might be acceptable. But I also could
see arguments towards this not being a good idea.

>>>> 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").
> 
> 
> Oh, I see what you mean. By per-vcpu flag you mean arch_vcpu field I assume?

Yes, a boolean in one of the arch-specific per-vCPU structs.
Whether that's arch_vcpu or perhaps something HVM specific is
another question.

Jan