| Message ID | 1610051698-23675-3-git-send-email-boris.ostrovsky@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Permit fault-less access to non-emulated MSRs | expand |
On 07.01.2021 21:34, Boris Ostrovsky wrote: > --- 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: What about ones handled by emulation, when emulation decides to raise #GP and this still causes trouble to a guest? (Slightly orthogonal, so we may want to defer this aspect.) > + * 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 -1 Not sure about this choice: I'd consider an MSR index in the Xen range more safe to use, not the least because this value effectively becomes part of the migration ABI. Or if you use one outside, then maybe a less prominent one than 0xffffffff (I guess -1, irrespective of the missing parentheses, isn't appropriate to use here). > @@ -77,7 +78,7 @@ int x86_msr_copy_from_buffer(struct msr_policy *p, > if ( copy_from_buffer_offset(&data, msrs, i, 1) ) > return -EFAULT; > > - if ( data.flags ) /* .flags MBZ */ > + if ( data.idx != MSR_UNHANDLED && data.flags ) You permit all flags bits set here, but ... > @@ -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: p->ignore_msrs = data.flags & 0xff; break; ... you use only the low 8 ones here. You want to forbid any we don't know of, and even restrict the low two ones to just the three values you define. E.g. for now if ( data.idx != MSR_UNHANDLED ? data.flags : data.flags > MSR_UNHANDLED_VERBOSE ) { rc = -EINVAL; goto err; Otoh I don't see why you need to use flags here - I think you could as well use the MSR value to convey the setting. And if you don't, you'd want to check the value to be zero. Nit: You can shorten line length by omitting the "& 0xff" and reducing the number of blanks ahead of "break;". Jan
On 1/8/21 9:55 AM, Jan Beulich wrote: > On 07.01.2021 21:34, Boris Ostrovsky wrote: >> --- 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: > What about ones handled by emulation, when emulation decides to > raise #GP and this still causes trouble to a guest? (Slightly > orthogonal, so we may want to defer this aspect.) Yes, that's a good point --- in some cases the situation is somewhat similar, e.g. when a new bit shows up in an MSR that until now was considered invalid. > >> + * 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 -1 > Not sure about this choice: I'd consider an MSR index in the Xen > range more safe to use, not the least because this value > effectively becomes part of the migration ABI. Or if you use one > outside, then maybe a less prominent one than 0xffffffff (I > guess -1, irrespective of the missing parentheses, isn't > appropriate to use here). All MSRs in Xen range are currently handled (although most return an exception). I can reserve the last one (0x400002ff) if you feel it's more appropriate. > >> @@ -77,7 +78,7 @@ int x86_msr_copy_from_buffer(struct msr_policy *p, >> if ( copy_from_buffer_offset(&data, msrs, i, 1) ) >> return -EFAULT; >> >> - if ( data.flags ) /* .flags MBZ */ >> + if ( data.idx != MSR_UNHANDLED && data.flags ) > You permit all flags bits set here, but ... > >> @@ -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: p->ignore_msrs = data.flags & 0xff; break; > ... you use only the low 8 ones here. You want to forbid any we > don't know of, and even restrict the low two ones to just the > three values you define. E.g. for now > > if ( data.idx != MSR_UNHANDLED ? data.flags > : data.flags > MSR_UNHANDLED_VERBOSE ) > { > rc = -EINVAL; > goto err; > > Otoh I don't see why you need to use flags here - I think you > could as well use the MSR value to convey the setting. And if > you don't, you'd want to check the value to be zero. Sure, using value will result in a slightly smaller diffstat too. -boris
On 08.01.2021 21:31, boris.ostrovsky@oracle.com wrote: > On 1/8/21 9:55 AM, Jan Beulich wrote: >> On 07.01.2021 21:34, Boris Ostrovsky wrote: >>> + * 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 -1 >> Not sure about this choice: I'd consider an MSR index in the Xen >> range more safe to use, not the least because this value >> effectively becomes part of the migration ABI. Or if you use one >> outside, then maybe a less prominent one than 0xffffffff (I >> guess -1, irrespective of the missing parentheses, isn't >> appropriate to use here). > > > All MSRs in Xen range are currently handled (although most return > an exception). I can reserve the last one (0x400002ff) if you feel > it's more appropriate. I do, yes, but I'd prefer to also have Andrew's general view here. Difficulty is his email delivery issue, so I don't know how soon we could hope for a reply. Jan
diff --git a/xen/include/xen/lib/x86/msr.h b/xen/include/xen/lib/x86/msr.h index 48ba4a59c036..7911ae31eb48 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 -1 + /* 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..cf53768dfa4e 100644 --- a/xen/lib/x86/msr.c +++ b/xen/lib/x86/msr.c @@ -6,11 +6,11 @@ * Copy a single MSR into the provided msr_entry_buffer_t buffer, performing a * boundary check against the buffer size. */ -static int copy_msr_to_buffer(uint32_t idx, uint64_t val, +static int copy_msr_to_buffer(uint32_t idx, uint64_t val, uint32_t flags, msr_entry_buffer_t msrs, uint32_t *curr_entry, const uint32_t nr_entries) { - const xen_msr_entry_t ent = { .idx = idx, .val = val }; + const xen_msr_entry_t ent = { .idx = idx, .val = val, .flags = flags }; if ( *curr_entry == nr_entries ) return -ENOBUFS; @@ -29,17 +29,18 @@ int x86_msr_copy_to_buffer(const struct msr_policy *p, const uint32_t nr_entries = *nr_entries_p; uint32_t curr_entry = 0; -#define COPY_MSR(idx, val) \ +#define COPY_MSR(idx, val, flags) \ ({ \ int ret; \ \ if ( (ret = copy_msr_to_buffer( \ - idx, val, msrs, &curr_entry, nr_entries)) ) \ + idx, val, flags, msrs, &curr_entry, nr_entries)) ) \ return ret; \ }) - COPY_MSR(MSR_INTEL_PLATFORM_INFO, p->platform_info.raw); - COPY_MSR(MSR_ARCH_CAPABILITIES, p->arch_caps.raw); + COPY_MSR(MSR_INTEL_PLATFORM_INFO, p->platform_info.raw, 0); + COPY_MSR(MSR_ARCH_CAPABILITIES, p->arch_caps.raw, 0); + COPY_MSR(MSR_UNHANDLED, 0, p->ignore_msrs); #undef COPY_MSR @@ -77,7 +78,7 @@ int x86_msr_copy_from_buffer(struct msr_policy *p, if ( copy_from_buffer_offset(&data, msrs, i, 1) ) return -EFAULT; - if ( data.flags ) /* .flags MBZ */ + if ( data.idx != MSR_UNHANDLED && data.flags ) { rc = -EINVAL; goto err; @@ -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: p->ignore_msrs = data.flags & 0xff; break; #undef ASSIGN
When toolstack updates MSR policy, this MSR offset (which is an invalid MSR index) is used to indicate hypervisor behavior when a guest accesses an MSR which is not explicitly emulated. Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> --- xen/include/xen/lib/x86/msr.h | 17 ++++++++++++++++- xen/lib/x86/msr.c | 16 +++++++++------- 2 files changed, 25 insertions(+), 8 deletions(-)