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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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


>
> But having such a list of ignored MSRs in msr_policy makes the whole
> get/set logic a bit weird, as the user would have to provide a buffer
> in order to get the list of ignored MSRs.


If we go with ranges/lists of ignored MSRs then we will need to have 
ignore_msrs as a rangeset in msr_policy, not as (current) uint8. And 
xen_msr_entry_t will need to have a range as opposed to single index. Or maybe 
I don't understand what you are referring to as get/set logic.


But I would like to make sure we really want to support such ranges, I am a 
little concerned about over-engineering this (especially wrt migration, I think 
it will need marshalling/unmarshalling)


>>> Isn't is possible to convey this data in the xl migration stream
>>> instead of having to pack it with MSRs?
>>
>> I haven't looked at this but again --- the feature itself has nothing to do 
>> with migration. The fact that folding it into policy makes migration of this 
>> information "just work" is just a nice side benefit of this implementation.
> IMO it feels slightly weird that we have to use a MSR (MSR_UNHANDLED)
> to store this option, seems like wasting an MSR index when there's
> really no need for it to be stored in an MSR, as we don't expose it to
> guests.
>
> It would seem more natural for such option to live in arch_domain as a
> rangeset for example.
>
> Maybe introduce a new DOMCTL to set it?
>
> #define XEN_DOMCTL_msr_set_ignore ...
> struct xen_domctl_msr_set_ignore {
>     uint32_t index;
>     uint32_t size;
> };


That will work too but this is adding 2 new domctls (I think we will need a 
"get" too) whereas with policy we use existing interface.


-boris