Re: [PATCH v2 2/4] x86: Introduce MSR_UNHANDLED

["Roger Pau Monné" <roger.pau@citrix.com>]

On Tue, Feb 23, 2021 at 10:39:48AM -0500, Boris Ostrovsky wrote:
> 
> On 2/23/21 8:23 AM, Jan Beulich wrote:
> > On 23.02.2021 13:17, Roger Pau Monné wrote:
> >> On Tue, Feb 23, 2021 at 11:15:31AM +0100, Jan Beulich wrote:
> >>> On 23.02.2021 10:34, Roger Pau Monné wrote:
> >>>> On Tue, Feb 23, 2021 at 08:57:00AM +0100, Jan Beulich wrote:
> >>>>> On 22.02.2021 22:19, Boris Ostrovsky wrote:
> >>>>>> On 2/22/21 6:08 AM, Roger Pau Monné wrote:
> >>>>>>> On Fri, Feb 19, 2021 at 09:56:32AM -0500, Boris Ostrovsky wrote:
> >>>>>>>> On 2/18/21 5:51 AM, Roger Pau Monné wrote:
> >>>>>>>>> On Wed, Jan 20, 2021 at 05:49:10PM -0500, Boris Ostrovsky wrote:
> >>>>>>>>>> When toolstack updates MSR policy, this MSR offset (which is the last
> >>>>>>>>>> index in the hypervisor MSR range) is used to indicate hypervisor
> >>>>>>>>>> behavior when guest accesses an MSR which is not explicitly emulated.
> >>>>>>>>> It's kind of weird to use an MSR to store this. I assume this is done
> >>>>>>>>> for migration reasons?
> >>>>>>>> Not really. It just seemed to me that MSR policy is the logical place to do that. Because it *is* a policy of how to deal with such accesses, isn't it?
> >>>>>>> I agree that using the msr_policy seems like the most suitable place
> >>>>>>> to convey this information between the toolstack and Xen. I wonder if
> >>>>>>> it would be fine to have fields in msr_policy that don't directly
> >>>>>>> translate into an MSR value?
> >>>>>>
> >>>>>> We have xen_msr_entry_t.flags that we can use when passing policy array back and forth. Then we can ignore xen_msr_entry_t.idx for that entry (although in earlier version of this series Jan preferred to use idx and leave flags alone).
> >>>>> Which, just to clarify, was not the least because of the flags
> >>>>> field being per-entry, i.e. per MSR, while here we want a global
> >>>>> indicator.
> >>>> We could exploit a bit the xen_msr_entry_t structure and use it
> >>>> like:
> >>>>
> >>>> typedef struct xen_msr_entry {
> >>>>     uint32_t idx;
> >>>> #define XEN_MSR_IGNORE (1u << 0)
> >>>>     uint32_t flags;
> >>>>     uint64_t val;
> >>>> } xen_msr_entry_t;
> >>>>
> >>>> Then use the idx and val fields to signal the start and size of the
> >>>> range to ignore accesses to when XEN_MSR_IGNORE is set in the flags
> >>>> field?
> >>>>
> >>>> xen_msr_entry_t = {
> >>>>     .idx = 0,
> >>>>     .val = 0xffffffff,
> >>>>     .flags = XEN_MSR_IGNORE,
> >>>> };
> >>>>
> >>>> Would be equivalent to ignoring accesses to the whole MSR range.
> >>>>
> >>>> This would allow selectively selecting which MSRs to ignore, while
> >>>> not wasting a MSR itself to convey the information?
> >>> Hmm, yes, the added flexibility would be nice from an abstract pov
> >>> (not sure how relevant it would be to Solaris'es issue). But my
> >>> dislike of using a flag which is meaningless in ordinary entries
> >>> remains, as was voiced against Boris'es original version.
> >> I understand the flags field is meaningless for regular MSRs, but I
> >> don't see why it would be an issue to start using it for this specific
> >> case of registering ranges of ignored MSRs.
> > It's not an "issue", it is - as said - my dislike. However, the way
> > it is in your proposal it is perhaps indeed not as bad as in Boris'es
> > original one: The flag now designates the purpose of the entry, and
> > the other two fields still have a meaning. Hence I was wrong to state
> > that it's "meaningless" - it now is required to be clear for
> > "ordinary" entries.
> >
> > In principle there could then also be multiple such entries / ranges.
> 
> 
> TBH I am not sold on usefulness of multiple ranges but if both of you feel it can be handy I'll do that, using Roger's proposal above. (Would it make sense to make val a union with, say, size or should the context be clear from flag's value?)

Doing an union with { val, size } would be fine for me, I'm also fine
with not doing it though.

> 
> Before I do that though --- what was the conclusion on verbosity control?

Ideally I would like to find a way to have a more generic interface to
change verbosity level on a per-guest basis, but I haven't looked at
all about how to do that, neither I think it would be acceptable to
put that burden on you.

Maybe we could introduce another flag to set whether ignored MSRs
should be logged, as that would be easier to implement?

I think in that case we could enforce that either all ranges have the
flag set or not, in order to prevent ending up tracking two different
ranges of ignored MSRs, one that triggers a message and another one
that doesn't.

Thanks, Roger.