| Message ID | 1611182952-9941-3-git-send-email-boris.ostrovsky@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Permit fault-less access to non-emulated MSRs | expand |
On 20.01.2021 23:49, Boris Ostrovsky wrote: > --- a/xen/arch/x86/msr.c > +++ b/xen/arch/x86/msr.c > @@ -295,7 +295,7 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val) > } > > /* Fallthrough. */ > - case 0x40000200 ... 0x400002ff: > + case 0x40000200 ... 0x400002fe: > ret = guest_rdmsr_xen(v, msr, val); > break; > > @@ -514,7 +514,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val) > } > > /* Fallthrough. */ > - case 0x40000200 ... 0x400002ff: > + case 0x40000200 ... 0x400002fe: > ret = guest_wrmsr_xen(v, msr, val); > break; For both of these, we need some kind of connection to MSR_UNHANDLED. Could be a BUILD_BUG_ON() or an explicit "case MSR_UNHANDLED:" (preventing someone "correcting" the apparent mistake) or yet something else. > --- a/xen/include/xen/lib/x86/msr.h > +++ b/xen/include/xen/lib/x86/msr.h > @@ -2,8 +2,21 @@ > #ifndef XEN_LIB_X86_MSR_H > #define XEN_LIB_X86_MSR_H > > +/* > + * Behavior on accesses to MSRs that are not handled by emulation: > + * 0 = return #GP, warning emitted > + * 1 = read as 0, writes are dropped, no warning > + * 2 = read as 0, writes are dropped, warning emitted > + */ > +#define MSR_UNHANDLED_NEVER 0 > +#define MSR_UNHANDLED_SILENT 1 > +#define MSR_UNHANDLED_VERBOSE 2 > + > +/* MSR that is not explicitly processed by emulation */ > +#define MSR_UNHANDLED 0x400002ff MSR indexes as well as definitions for MSR contents generally live in asm-x86/msr-index.h. I think it would be better for the above to also go there. Additionally the comment on MSR_UNHANDLED should not only say what will not happen for this index, but also what its intended purpose is. > @@ -45,6 +58,8 @@ struct msr_policy > bool taa_no:1; > }; > } arch_caps; > + > + uint8_t ignore_msrs; Add a brief comment along the lines of /* MSR_UNHANDLED_* */ here to make the connection to intended values? Also, Andrew, (I think I did say so before) - I definitely would want your general consent with this model, as what gets altered here is almost all relatively recent contributions by you. Nor would I exclude the approach being controversial. Jan
On 1/22/21 6:51 AM, Jan Beulich wrote: > On 20.01.2021 23:49, Boris Ostrovsky wrote: >> --- a/xen/arch/x86/msr.c >> +++ b/xen/arch/x86/msr.c >> @@ -295,7 +295,7 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val) >> } >> >> /* Fallthrough. */ >> - case 0x40000200 ... 0x400002ff: >> + case 0x40000200 ... 0x400002fe: >> ret = guest_rdmsr_xen(v, msr, val); >> break; >> >> @@ -514,7 +514,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val) >> } >> >> /* Fallthrough. */ >> - case 0x40000200 ... 0x400002ff: >> + case 0x40000200 ... 0x400002fe: >> ret = guest_wrmsr_xen(v, msr, val); >> break; > > For both of these, we need some kind of connection to > MSR_UNHANDLED. Could be a BUILD_BUG_ON() or an explicit > "case MSR_UNHANDLED:" (preventing someone "correcting" the > apparent mistake) or yet something else. I actually meant to add a comment there but forgot. I'll add an explicit 'case'. > >> --- a/xen/include/xen/lib/x86/msr.h >> +++ b/xen/include/xen/lib/x86/msr.h >> @@ -2,8 +2,21 @@ >> #ifndef XEN_LIB_X86_MSR_H >> #define XEN_LIB_X86_MSR_H >> >> +/* >> + * Behavior on accesses to MSRs that are not handled by emulation: >> + * 0 = return #GP, warning emitted >> + * 1 = read as 0, writes are dropped, no warning >> + * 2 = read as 0, writes are dropped, warning emitted >> + */ >> +#define MSR_UNHANDLED_NEVER 0 >> +#define MSR_UNHANDLED_SILENT 1 >> +#define MSR_UNHANDLED_VERBOSE 2 >> + >> +/* MSR that is not explicitly processed by emulation */ >> +#define MSR_UNHANDLED 0x400002ff > > MSR indexes as well as definitions for MSR contents generally > live in asm-x86/msr-index.h. I think it would be better for > the above to also go there. > I didn't put it there because I felt that file is for "real" (including hypervisor range) MSRs. But I can move it to msr-index.h -boris
On 1/22/21 6:51 AM, Jan Beulich wrote: > Also, Andrew, (I think I did say so before) - I definitely > would want your general consent with this model, as what gets > altered here is almost all relatively recent contributions > by you. Nor would I exclude the approach being controversial. > Andrew, ping? -boris
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? Isn't is possible to convey this data in the xl migration stream instead of having to pack it with MSRs? Thanks, Roger.
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? > 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. -boris
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? 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. > > > 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; }; Thanks, Roger.
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
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. Jan
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? It would still need to be stored somewhere in the Xen internal domain structure using a rangeset I think, which could be translated back and forth into this xen_msr_entry_t format. Roger.
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. Andrew - afaict you've been completely silent on this thread so far. What's your view? Jan
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.
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. > It certainly seems better than hijacking an MSR index (MSR_UNHANDLED). Not sure. Jan
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?) Before I do that though --- what was the conclusion on verbosity control? -boris
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
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.
On 2/23/21 11:11 AM, Roger Pau Monné wrote: > On Tue, Feb 23, 2021 at 10:39:48AM -0500, Boris Ostrovsky wrote: > >> 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? Probably. msr_ignore=["verbose=<bool>", "<range>", "range>", ...] -boris > > 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.
On Tue, Feb 23, 2021 at 05:10:16PM +0100, Jan Beulich wrote: > On 23.02.2021 16:39, Boris Ostrovsky wrote: > > 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? As I said on my reply to Boris, I would really like to have a more generic way of doing this kind of per-domain verbosity level, but I don't think it would be fair to ask Boris to implement this. Xen likely needs a way to print issues with MSRs accesses in 4.15 on a per-domain basis, so going for a MSR specific solution would be acceptable given the time frame. I think Boris made a proposal for a solution on another email, let me go look at that. Thanks, Roger.
On Tue, Feb 23, 2021 at 11:40:07AM -0500, Boris Ostrovsky wrote: > > On 2/23/21 11:11 AM, Roger Pau Monné wrote: > > On Tue, Feb 23, 2021 at 10:39:48AM -0500, Boris Ostrovsky wrote: > > > >> 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? > > > Probably. > > > msr_ignore=["verbose=<bool>", "<range>", "range>", ...] I think just adding a new option will be easier to parse in xl rather than placing it in the list of MSR ranges: msr_ignore=["<range>", "range>", ...] msr_ignore_verbose=<boolean> Seems easier to implement IMO. Thanks, Roger.
On 2/23/21 1:02 PM, Roger Pau Monné wrote: > On Tue, Feb 23, 2021 at 11:40:07AM -0500, Boris Ostrovsky wrote: >> On 2/23/21 11:11 AM, Roger Pau Monné wrote: >>> On Tue, Feb 23, 2021 at 10:39:48AM -0500, Boris Ostrovsky wrote: >>> >>>> 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? >> >> Probably. >> >> >> msr_ignore=["verbose=<bool>", "<range>", "range>", ...] > I think just adding a new option will be easier to parse in xl rather > than placing it in the list of MSR ranges: > > msr_ignore=["<range>", "range>", ...] > msr_ignore_verbose=<boolean> > > Seems easier to implement IMO. I haven't looked at what parsing support is available in xl. If I don't find anything useful (and I give up quickly ;-) ) I can use separate options. -boris
diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c index be8e36386250..433d16c80728 100644 --- a/xen/arch/x86/msr.c +++ b/xen/arch/x86/msr.c @@ -295,7 +295,7 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val) } /* Fallthrough. */ - case 0x40000200 ... 0x400002ff: + case 0x40000200 ... 0x400002fe: ret = guest_rdmsr_xen(v, msr, val); break; @@ -514,7 +514,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val) } /* Fallthrough. */ - case 0x40000200 ... 0x400002ff: + case 0x40000200 ... 0x400002fe: ret = guest_wrmsr_xen(v, msr, val); break; diff --git a/xen/include/xen/lib/x86/msr.h b/xen/include/xen/lib/x86/msr.h index 48ba4a59c036..fbbb3b7ba870 100644 --- a/xen/include/xen/lib/x86/msr.h +++ b/xen/include/xen/lib/x86/msr.h @@ -2,8 +2,21 @@ #ifndef XEN_LIB_X86_MSR_H #define XEN_LIB_X86_MSR_H +/* + * Behavior on accesses to MSRs that are not handled by emulation: + * 0 = return #GP, warning emitted + * 1 = read as 0, writes are dropped, no warning + * 2 = read as 0, writes are dropped, warning emitted + */ +#define MSR_UNHANDLED_NEVER 0 +#define MSR_UNHANDLED_SILENT 1 +#define MSR_UNHANDLED_VERBOSE 2 + +/* MSR that is not explicitly processed by emulation */ +#define MSR_UNHANDLED 0x400002ff + /* Maximum number of MSRs written when serialising msr_policy. */ -#define MSR_MAX_SERIALISED_ENTRIES 2 +#define MSR_MAX_SERIALISED_ENTRIES 3 /* MSR policy object for shared per-domain MSRs */ struct msr_policy @@ -45,6 +58,8 @@ struct msr_policy bool taa_no:1; }; } arch_caps; + + uint8_t ignore_msrs; }; #ifdef __XEN__ diff --git a/xen/lib/x86/msr.c b/xen/lib/x86/msr.c index 7d71e92a380a..178203803946 100644 --- a/xen/lib/x86/msr.c +++ b/xen/lib/x86/msr.c @@ -40,6 +40,7 @@ int x86_msr_copy_to_buffer(const struct msr_policy *p, COPY_MSR(MSR_INTEL_PLATFORM_INFO, p->platform_info.raw); COPY_MSR(MSR_ARCH_CAPABILITIES, p->arch_caps.raw); + COPY_MSR(MSR_UNHANDLED, p->ignore_msrs); #undef COPY_MSR @@ -101,6 +102,7 @@ int x86_msr_copy_from_buffer(struct msr_policy *p, case MSR_INTEL_PLATFORM_INFO: ASSIGN(platform_info.raw); break; case MSR_ARCH_CAPABILITIES: ASSIGN(arch_caps.raw); break; + case MSR_UNHANDLED: ASSIGN(ignore_msrs); break; #undef ASSIGN
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. Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> --- Changes in v2: * Use 0x400002ff for MSR_UNHANDLED * Pass ignore_msrs in msr_policy.value xen/arch/x86/msr.c | 4 ++-- xen/include/xen/lib/x86/msr.h | 17 ++++++++++++++++- xen/lib/x86/msr.c | 2 ++ 3 files changed, 20 insertions(+), 3 deletions(-)