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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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 certainly seems better
than hijacking an MSR index (MSR_UNHANDLED).

Thanks, Roger.