| Message ID | 20210430155211.3709-4-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libs/guest: new CPUID/MSR interface | expand |
On 30.04.2021 17:52, Roger Pau Monne wrote: > Introduce an interface that returns a specific MSR entry from a cpu > policy in xen_msr_entry_t format. Provide a helper to perform a binary > search against an array of MSR entries. > > This is useful to callers can peek data from the opaque > xc_cpu_policy_t type. > > No caller of the interface introduced on this patch. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > Changes since v1: > - Introduce a helper to perform a binary search of the MSR entries > array. > --- > tools/include/xenctrl.h | 2 ++ > tools/libs/guest/xg_cpuid_x86.c | 42 +++++++++++++++++++++++++++++++++ > 2 files changed, 44 insertions(+) > > diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h > index cbca7209e34..605c632cf30 100644 > --- a/tools/include/xenctrl.h > +++ b/tools/include/xenctrl.h > @@ -2611,6 +2611,8 @@ int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t policy, > int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy, > uint32_t leaf, uint32_t subleaf, > xen_cpuid_leaf_t *out); > +int xc_cpu_policy_get_msr(xc_interface *xch, const xc_cpu_policy_t policy, > + uint32_t msr, xen_msr_entry_t *out); > > int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps); > int xc_get_cpu_featureset(xc_interface *xch, uint32_t index, > diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c > index de27826f415..9e83daca0e6 100644 > --- a/tools/libs/guest/xg_cpuid_x86.c > +++ b/tools/libs/guest/xg_cpuid_x86.c > @@ -850,3 +850,45 @@ int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy, > *out = *tmp; > return 0; > } > + > +static int compare_entries(const void *l, const void *r) > +{ > + const xen_msr_entry_t *lhs = l; > + const xen_msr_entry_t *rhs = r; > + > + if ( lhs->idx == rhs->idx ) > + return 0; > + return lhs->idx < rhs->idx ? -1 : 1; > +} > + > +static xen_msr_entry_t *find_entry(xen_msr_entry_t *entries, > + unsigned int nr_entries, unsigned int index) > +{ > + const xen_msr_entry_t key = { index }; > + > + return bsearch(&key, entries, nr_entries, sizeof(*entries), compare_entries); > +} Isn't "entries" / "entry" a little too generic a name here, considering the CPUID equivalents use "leaves" / "leaf"? (Noticed really while looking at patch 7.) Jan
On Mon, May 03, 2021 at 12:41:29PM +0200, Jan Beulich wrote: > On 30.04.2021 17:52, Roger Pau Monne wrote: > > Introduce an interface that returns a specific MSR entry from a cpu > > policy in xen_msr_entry_t format. Provide a helper to perform a binary > > search against an array of MSR entries. > > > > This is useful to callers can peek data from the opaque > > xc_cpu_policy_t type. > > > > No caller of the interface introduced on this patch. > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > --- > > Changes since v1: > > - Introduce a helper to perform a binary search of the MSR entries > > array. > > --- > > tools/include/xenctrl.h | 2 ++ > > tools/libs/guest/xg_cpuid_x86.c | 42 +++++++++++++++++++++++++++++++++ > > 2 files changed, 44 insertions(+) > > > > diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h > > index cbca7209e34..605c632cf30 100644 > > --- a/tools/include/xenctrl.h > > +++ b/tools/include/xenctrl.h > > @@ -2611,6 +2611,8 @@ int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t policy, > > int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy, > > uint32_t leaf, uint32_t subleaf, > > xen_cpuid_leaf_t *out); > > +int xc_cpu_policy_get_msr(xc_interface *xch, const xc_cpu_policy_t policy, > > + uint32_t msr, xen_msr_entry_t *out); > > > > int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps); > > int xc_get_cpu_featureset(xc_interface *xch, uint32_t index, > > diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c > > index de27826f415..9e83daca0e6 100644 > > --- a/tools/libs/guest/xg_cpuid_x86.c > > +++ b/tools/libs/guest/xg_cpuid_x86.c > > @@ -850,3 +850,45 @@ int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy, > > *out = *tmp; > > return 0; > > } > > + > > +static int compare_entries(const void *l, const void *r) > > +{ > > + const xen_msr_entry_t *lhs = l; > > + const xen_msr_entry_t *rhs = r; > > + > > + if ( lhs->idx == rhs->idx ) > > + return 0; > > + return lhs->idx < rhs->idx ? -1 : 1; > > +} > > + > > +static xen_msr_entry_t *find_entry(xen_msr_entry_t *entries, > > + unsigned int nr_entries, unsigned int index) > > +{ > > + const xen_msr_entry_t key = { index }; > > + > > + return bsearch(&key, entries, nr_entries, sizeof(*entries), compare_entries); > > +} > > Isn't "entries" / "entry" a little too generic a name here, considering > the CPUID equivalents use "leaves" / "leaf"? (Noticed really while looking > at patch 7.) Would you be fine with naming the function find_msr and leaving the rest of the parameters names as-is? Thanks, Roger.
On 04.05.2021 12:56, Roger Pau Monné wrote: > On Mon, May 03, 2021 at 12:41:29PM +0200, Jan Beulich wrote: >> On 30.04.2021 17:52, Roger Pau Monne wrote: >>> --- a/tools/libs/guest/xg_cpuid_x86.c >>> +++ b/tools/libs/guest/xg_cpuid_x86.c >>> @@ -850,3 +850,45 @@ int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy, >>> *out = *tmp; >>> return 0; >>> } >>> + >>> +static int compare_entries(const void *l, const void *r) >>> +{ >>> + const xen_msr_entry_t *lhs = l; >>> + const xen_msr_entry_t *rhs = r; >>> + >>> + if ( lhs->idx == rhs->idx ) >>> + return 0; >>> + return lhs->idx < rhs->idx ? -1 : 1; >>> +} >>> + >>> +static xen_msr_entry_t *find_entry(xen_msr_entry_t *entries, >>> + unsigned int nr_entries, unsigned int index) >>> +{ >>> + const xen_msr_entry_t key = { index }; >>> + >>> + return bsearch(&key, entries, nr_entries, sizeof(*entries), compare_entries); >>> +} >> >> Isn't "entries" / "entry" a little too generic a name here, considering >> the CPUID equivalents use "leaves" / "leaf"? (Noticed really while looking >> at patch 7.) > > Would you be fine with naming the function find_msr and leaving the > rest of the parameters names as-is? Yes. But recall I'm not the maintainer of this code anyway. Jan
On Tue, May 04, 2021 at 01:40:11PM +0200, Jan Beulich wrote: > On 04.05.2021 12:56, Roger Pau Monné wrote: > > On Mon, May 03, 2021 at 12:41:29PM +0200, Jan Beulich wrote: > >> On 30.04.2021 17:52, Roger Pau Monne wrote: > >>> --- a/tools/libs/guest/xg_cpuid_x86.c > >>> +++ b/tools/libs/guest/xg_cpuid_x86.c > >>> @@ -850,3 +850,45 @@ int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy, > >>> *out = *tmp; > >>> return 0; > >>> } > >>> + > >>> +static int compare_entries(const void *l, const void *r) > >>> +{ > >>> + const xen_msr_entry_t *lhs = l; > >>> + const xen_msr_entry_t *rhs = r; > >>> + > >>> + if ( lhs->idx == rhs->idx ) > >>> + return 0; > >>> + return lhs->idx < rhs->idx ? -1 : 1; > >>> +} > >>> + > >>> +static xen_msr_entry_t *find_entry(xen_msr_entry_t *entries, > >>> + unsigned int nr_entries, unsigned int index) > >>> +{ > >>> + const xen_msr_entry_t key = { index }; > >>> + > >>> + return bsearch(&key, entries, nr_entries, sizeof(*entries), compare_entries); > >>> +} > >> > >> Isn't "entries" / "entry" a little too generic a name here, considering > >> the CPUID equivalents use "leaves" / "leaf"? (Noticed really while looking > >> at patch 7.) > > > > Would you be fine with naming the function find_msr and leaving the > > rest of the parameters names as-is? > > Yes. But recall I'm not the maintainer of this code anyway. You cared to provide feedback, and I'm happy to make the change. Thanks, Roger.
On 04/05/2021 12:58, Roger Pau Monné wrote: > On Tue, May 04, 2021 at 01:40:11PM +0200, Jan Beulich wrote: >> On 04.05.2021 12:56, Roger Pau Monné wrote: >>> On Mon, May 03, 2021 at 12:41:29PM +0200, Jan Beulich wrote: >>>> On 30.04.2021 17:52, Roger Pau Monne wrote: >>>>> --- a/tools/libs/guest/xg_cpuid_x86.c >>>>> +++ b/tools/libs/guest/xg_cpuid_x86.c >>>>> @@ -850,3 +850,45 @@ int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy, >>>>> *out = *tmp; >>>>> return 0; >>>>> } >>>>> + >>>>> +static int compare_entries(const void *l, const void *r) >>>>> +{ >>>>> + const xen_msr_entry_t *lhs = l; >>>>> + const xen_msr_entry_t *rhs = r; >>>>> + >>>>> + if ( lhs->idx == rhs->idx ) >>>>> + return 0; >>>>> + return lhs->idx < rhs->idx ? -1 : 1; >>>>> +} >>>>> + >>>>> +static xen_msr_entry_t *find_entry(xen_msr_entry_t *entries, >>>>> + unsigned int nr_entries, unsigned int index) >>>>> +{ >>>>> + const xen_msr_entry_t key = { index }; >>>>> + >>>>> + return bsearch(&key, entries, nr_entries, sizeof(*entries), compare_entries); >>>>> +} >>>> Isn't "entries" / "entry" a little too generic a name here, considering >>>> the CPUID equivalents use "leaves" / "leaf"? (Noticed really while looking >>>> at patch 7.) >>> Would you be fine with naming the function find_msr and leaving the >>> rest of the parameters names as-is? >> Yes. But recall I'm not the maintainer of this code anyway. This file in particular has been entirely within the x86 remit for multiple years now, as have the other cpuid bits in misc/ and libxl. > You cared to provide feedback, and I'm happy to make the change. find_msr() would be a better name. As for entries and nr_entries, suggestions welcome. I couldn't think of anything better for the low level helpers. ~Andrew
On 04.05.2021 19:11, Andrew Cooper wrote: > On 04/05/2021 12:58, Roger Pau Monné wrote: >> On Tue, May 04, 2021 at 01:40:11PM +0200, Jan Beulich wrote: >>> On 04.05.2021 12:56, Roger Pau Monné wrote: >>>> On Mon, May 03, 2021 at 12:41:29PM +0200, Jan Beulich wrote: >>>>> On 30.04.2021 17:52, Roger Pau Monne wrote: >>>>>> --- a/tools/libs/guest/xg_cpuid_x86.c >>>>>> +++ b/tools/libs/guest/xg_cpuid_x86.c >>>>>> @@ -850,3 +850,45 @@ int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy, >>>>>> *out = *tmp; >>>>>> return 0; >>>>>> } >>>>>> + >>>>>> +static int compare_entries(const void *l, const void *r) >>>>>> +{ >>>>>> + const xen_msr_entry_t *lhs = l; >>>>>> + const xen_msr_entry_t *rhs = r; >>>>>> + >>>>>> + if ( lhs->idx == rhs->idx ) >>>>>> + return 0; >>>>>> + return lhs->idx < rhs->idx ? -1 : 1; >>>>>> +} >>>>>> + >>>>>> +static xen_msr_entry_t *find_entry(xen_msr_entry_t *entries, >>>>>> + unsigned int nr_entries, unsigned int index) >>>>>> +{ >>>>>> + const xen_msr_entry_t key = { index }; >>>>>> + >>>>>> + return bsearch(&key, entries, nr_entries, sizeof(*entries), compare_entries); >>>>>> +} >>>>> Isn't "entries" / "entry" a little too generic a name here, considering >>>>> the CPUID equivalents use "leaves" / "leaf"? (Noticed really while looking >>>>> at patch 7.) >>>> Would you be fine with naming the function find_msr and leaving the >>>> rest of the parameters names as-is? >>> Yes. But recall I'm not the maintainer of this code anyway. > > This file in particular has been entirely within the x86 remit for > multiple years now, as have the other cpuid bits in misc/ and libxl. Well, it's somewhere in the middle imo: Its maintainers have kind of permanently delegated to the x86 maintainers. Which still doesn't make it part of "X86 ARCHITECTURE" until such time as the file gets actually named there. Note how e.g. tools/misc/xen-cpuid.c already is. Jan
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h index cbca7209e34..605c632cf30 100644 --- a/tools/include/xenctrl.h +++ b/tools/include/xenctrl.h @@ -2611,6 +2611,8 @@ int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t policy, int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy, uint32_t leaf, uint32_t subleaf, xen_cpuid_leaf_t *out); +int xc_cpu_policy_get_msr(xc_interface *xch, const xc_cpu_policy_t policy, + uint32_t msr, xen_msr_entry_t *out); int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps); int xc_get_cpu_featureset(xc_interface *xch, uint32_t index, diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c index de27826f415..9e83daca0e6 100644 --- a/tools/libs/guest/xg_cpuid_x86.c +++ b/tools/libs/guest/xg_cpuid_x86.c @@ -850,3 +850,45 @@ int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t policy, *out = *tmp; return 0; } + +static int compare_entries(const void *l, const void *r) +{ + const xen_msr_entry_t *lhs = l; + const xen_msr_entry_t *rhs = r; + + if ( lhs->idx == rhs->idx ) + return 0; + return lhs->idx < rhs->idx ? -1 : 1; +} + +static xen_msr_entry_t *find_entry(xen_msr_entry_t *entries, + unsigned int nr_entries, unsigned int index) +{ + const xen_msr_entry_t key = { index }; + + return bsearch(&key, entries, nr_entries, sizeof(*entries), compare_entries); +} + +int xc_cpu_policy_get_msr(xc_interface *xch, const xc_cpu_policy_t policy, + uint32_t msr, xen_msr_entry_t *out) +{ + unsigned int nr_entries = ARRAY_SIZE(policy->entries); + xen_msr_entry_t *tmp; + int rc; + + rc = xc_cpu_policy_serialise(xch, policy, NULL, 0, + policy->entries, &nr_entries); + if ( rc ) + return rc; + + tmp = find_entry(policy->entries, nr_entries, msr); + if ( !tmp ) + { + /* Unable to find a matching MSR. */ + errno = ENOENT; + return -1; + } + + *out = *tmp; + return 0; +}
Introduce an interface that returns a specific MSR entry from a cpu policy in xen_msr_entry_t format. Provide a helper to perform a binary search against an array of MSR entries. This is useful to callers can peek data from the opaque xc_cpu_policy_t type. No caller of the interface introduced on this patch. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v1: - Introduce a helper to perform a binary search of the MSR entries array. --- tools/include/xenctrl.h | 2 ++ tools/libs/guest/xg_cpuid_x86.c | 42 +++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+)