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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 21-01-26 10:05:47, Jan Beulich wrote:
> 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.

I was imagining a case where we'd add have another "ops->read_msr(_regs.ecx, 
...)"
in the emulator and some values of ecx may not be tested. (Or even passing
an explicit index that is not handled, although that is highly unlikely to
pass code review).

Yes, these cases do not currently exist. 

> 
> > 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.

Only if MSR exists/handled. We add parameters that allow it to be set
to zero for unhandled case but default (which is existing behavior, i.e.
MSR_UNHANDLED_NEVER) would leave the (pointed to) argument unchanged.

>  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).

Yes, this would only be relevant to reads but since I dropped is_write check
the conditional covers both reads and writes.

In any case, this (and the one above) are minor issues and I don't mind
making the 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.
> > 
> > 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 see. Where would we need to call this hook though? We are never directly
emulating MSR access (compared to, for example, CR accesses where
x86_insn_is_cr_access is used). And for PV we already validate it in
emul-priv-op.c():validate().

> 
> >>>> 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.

Currently we only need this for HVM but using it for both will avoid the need to
check guest type. 


-boris