| Message ID | 20210507110422.24608-4-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | libs/guest: new CPUID/MSR interface | expand |
On 07.05.2021 13:04, Roger Pau Monne wrote: > @@ -91,6 +91,21 @@ int x86_msr_copy_from_buffer(struct msr_policy *policy, > const msr_entry_buffer_t msrs, uint32_t nr_entries, > uint32_t *err_msr); > > +/** > + * Get a MSR entry from a policy object. > + * > + * @param policy The msr_policy object. > + * @param idx The index. > + * @returns a pointer to the requested leaf or NULL in case of error. > + * > + * Do not call this function directly and instead use x86_msr_get_entry that > + * will deal with both const and non-const policies returning a pointer with > + * constness matching that of the input. > + */ > +const uint64_t *_x86_msr_get_entry(const struct msr_policy *policy, > + uint32_t idx); > +#define x86_msr_get_entry(p, i) \ > + ((__typeof__(&(p)->platform_info.raw))_x86_msr_get_entry(p, i)) > #endif /* !XEN_LIB_X86_MSR_H */ Just two nits: I think it would be nice to retain a blank line ahead of the #endif. And here as well as in the CPUID counterpart you introduce, strictly speaking, name space violations (via the leading underscore). I realize I'm not really liked for pointing such out, but it may be more relevant here than in pure hypervisor code, as this library code is supposed to be usable also in userland. Jan
On Mon, May 17, 2021 at 05:43:33PM +0200, Jan Beulich wrote: > On 07.05.2021 13:04, Roger Pau Monne wrote: > > @@ -91,6 +91,21 @@ int x86_msr_copy_from_buffer(struct msr_policy *policy, > > const msr_entry_buffer_t msrs, uint32_t nr_entries, > > uint32_t *err_msr); > > > > +/** > > + * Get a MSR entry from a policy object. > > + * > > + * @param policy The msr_policy object. > > + * @param idx The index. > > + * @returns a pointer to the requested leaf or NULL in case of error. > > + * > > + * Do not call this function directly and instead use x86_msr_get_entry that > > + * will deal with both const and non-const policies returning a pointer with > > + * constness matching that of the input. > > + */ > > +const uint64_t *_x86_msr_get_entry(const struct msr_policy *policy, > > + uint32_t idx); > > +#define x86_msr_get_entry(p, i) \ > > + ((__typeof__(&(p)->platform_info.raw))_x86_msr_get_entry(p, i)) > > #endif /* !XEN_LIB_X86_MSR_H */ > > Just two nits: I think it would be nice to retain a blank line ahead of > the #endif. And here as well as in the CPUID counterpart you introduce, > strictly speaking, name space violations (via the leading underscore). I guess another option would be to name the function x86_msr_get_entry_const, and keep the x86_msr_get_entry macro as-is. Does that seem better? Thanks, Roger.
On 18.05.2021 12:50, Roger Pau Monné wrote: > On Mon, May 17, 2021 at 05:43:33PM +0200, Jan Beulich wrote: >> On 07.05.2021 13:04, Roger Pau Monne wrote: >>> @@ -91,6 +91,21 @@ int x86_msr_copy_from_buffer(struct msr_policy *policy, >>> const msr_entry_buffer_t msrs, uint32_t nr_entries, >>> uint32_t *err_msr); >>> >>> +/** >>> + * Get a MSR entry from a policy object. >>> + * >>> + * @param policy The msr_policy object. >>> + * @param idx The index. >>> + * @returns a pointer to the requested leaf or NULL in case of error. >>> + * >>> + * Do not call this function directly and instead use x86_msr_get_entry that >>> + * will deal with both const and non-const policies returning a pointer with >>> + * constness matching that of the input. >>> + */ >>> +const uint64_t *_x86_msr_get_entry(const struct msr_policy *policy, >>> + uint32_t idx); >>> +#define x86_msr_get_entry(p, i) \ >>> + ((__typeof__(&(p)->platform_info.raw))_x86_msr_get_entry(p, i)) >>> #endif /* !XEN_LIB_X86_MSR_H */ >> >> Just two nits: I think it would be nice to retain a blank line ahead of >> the #endif. And here as well as in the CPUID counterpart you introduce, >> strictly speaking, name space violations (via the leading underscore). > > I guess another option would be to name the function > x86_msr_get_entry_const, and keep the x86_msr_get_entry macro as-is. > > Does that seem better? This would be fine with me, as would be a trailing underscore or a double underscore after e.g. the first name component. Jan
diff --git a/tools/tests/cpu-policy/test-cpu-policy.c b/tools/tests/cpu-policy/test-cpu-policy.c index 81de9720c8d..854883fbb39 100644 --- a/tools/tests/cpu-policy/test-cpu-policy.c +++ b/tools/tests/cpu-policy/test-cpu-policy.c @@ -389,16 +389,6 @@ static void test_msr_deserialise_failure(void) .msr = { .idx = 0xce, .flags = 1 }, .rc = -EINVAL, }, - { - .name = "truncated val", - .msr = { .idx = 0xce, .val = ~0ull }, - .rc = -EOVERFLOW, - }, - { - .name = "truncated val", - .msr = { .idx = 0x10a, .val = ~0ull }, - .rc = -EOVERFLOW, - }, }; printf("Testing MSR deserialise failure:\n"); @@ -744,6 +734,43 @@ static void test_cpuid_get_leaf_failure(void) } } +static void test_msr_get_entry(void) +{ + static const struct test { + const char *name; + unsigned int idx; + bool success; + } tests[] = { + { + .name = "bad msr index", + .idx = -1, + }, + { + .name = "good msr index", + .idx = 0xce, + .success = true, + }, + }; + const struct msr_policy pc; + const uint64_t *ec; + struct msr_policy p; + uint64_t *e; + + /* Constness build test. */ + ec = x86_msr_get_entry(&pc, 0); + e = x86_msr_get_entry(&p, 0); + + printf("Testing MSR get leaf:\n"); + + for ( size_t i = 0; i < ARRAY_SIZE(tests); ++i ) + { + const struct test *t = &tests[i]; + + if ( !!x86_msr_get_entry(&pc, t->idx) != t->success ) + fail(" Test %s failed\n", t->name); + } +} + static void test_is_compatible_success(void) { static struct test { @@ -864,6 +891,7 @@ int main(int argc, char **argv) test_msr_serialise_success(); test_msr_deserialise_failure(); + test_msr_get_entry(); test_is_compatible_success(); test_is_compatible_failure(); diff --git a/xen/include/xen/lib/x86/msr.h b/xen/include/xen/lib/x86/msr.h index 48ba4a59c03..9d5bcfad886 100644 --- a/xen/include/xen/lib/x86/msr.h +++ b/xen/include/xen/lib/x86/msr.h @@ -17,7 +17,7 @@ struct msr_policy * is dependent on real hardware support. */ union { - uint32_t raw; + uint64_t raw; struct { uint32_t :31; bool cpuid_faulting:1; @@ -32,7 +32,7 @@ struct msr_policy * fixed in hardware. */ union { - uint32_t raw; + uint64_t raw; struct { bool rdcl_no:1; bool ibrs_all:1; @@ -91,6 +91,21 @@ int x86_msr_copy_from_buffer(struct msr_policy *policy, const msr_entry_buffer_t msrs, uint32_t nr_entries, uint32_t *err_msr); +/** + * Get a MSR entry from a policy object. + * + * @param policy The msr_policy object. + * @param idx The index. + * @returns a pointer to the requested leaf or NULL in case of error. + * + * Do not call this function directly and instead use x86_msr_get_entry that + * will deal with both const and non-const policies returning a pointer with + * constness matching that of the input. + */ +const uint64_t *_x86_msr_get_entry(const struct msr_policy *policy, + uint32_t idx); +#define x86_msr_get_entry(p, i) \ + ((__typeof__(&(p)->platform_info.raw))_x86_msr_get_entry(p, i)) #endif /* !XEN_LIB_X86_MSR_H */ /* diff --git a/xen/lib/x86/msr.c b/xen/lib/x86/msr.c index 7d71e92a380..4b5e3553e34 100644 --- a/xen/lib/x86/msr.c +++ b/xen/lib/x86/msr.c @@ -74,6 +74,8 @@ int x86_msr_copy_from_buffer(struct msr_policy *p, for ( i = 0; i < nr_entries; i++ ) { + uint64_t *val; + if ( copy_from_buffer_offset(&data, msrs, i, 1) ) return -EFAULT; @@ -83,31 +85,13 @@ int x86_msr_copy_from_buffer(struct msr_policy *p, goto err; } - switch ( data.idx ) + val = x86_msr_get_entry(p, data.idx); + if ( !val ) { - /* - * Assign data.val to p->field, checking for truncation if the - * backing storage for field is smaller than uint64_t - */ -#define ASSIGN(field) \ -({ \ - if ( (typeof(p->field))data.val != data.val ) \ - { \ - rc = -EOVERFLOW; \ - goto err; \ - } \ - p->field = data.val; \ -}) - - case MSR_INTEL_PLATFORM_INFO: ASSIGN(platform_info.raw); break; - case MSR_ARCH_CAPABILITIES: ASSIGN(arch_caps.raw); break; - -#undef ASSIGN - - default: rc = -ERANGE; goto err; } + *val = data.val; } return 0; @@ -119,6 +103,21 @@ int x86_msr_copy_from_buffer(struct msr_policy *p, return rc; } +const uint64_t *_x86_msr_get_entry(const struct msr_policy *policy, + uint32_t idx) +{ + switch ( idx ) + { + case MSR_INTEL_PLATFORM_INFO: + return &policy->platform_info.raw; + + case MSR_ARCH_CAPABILITIES: + return &policy->arch_caps.raw; + } + + return NULL; +} + /* * Local variables: * mode: C
Use such helper in order to replace the code in x86_msr_copy_from_buffer. Note the introduced helper should not be directly called and instead x86_msr_get_entry should be used that will properly deal with const and non-const inputs. Note this requires making the raw fields uint64_t so that it can accommodate the maximum size of MSRs values, and in turn removing the truncation tests. Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v3: - New in this version. --- tools/tests/cpu-policy/test-cpu-policy.c | 48 +++++++++++++++++++----- xen/include/xen/lib/x86/msr.h | 19 +++++++++- xen/lib/x86/msr.c | 41 ++++++++++---------- 3 files changed, 75 insertions(+), 33 deletions(-)