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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.02.2021 16:39, 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.

As indicated I really think an opinion from Andrew would be helpful.

> (Would it make sense to make val a union with, say, size or should
> the context be clear from flag's value?)

I'd recommend against this, unless you were to also name the upper
half of the 64-bit field so you can check that part to be zero.
Plus unions are generally not nice to be introduced into public
headers for already existing types.

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

Not sure - afaict the conclusion was that we still don't really
agree. Roger?

Jan