| Message ID | 20210323095849.37858-4-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libs/guest: new CPUID/MSR interface | expand |
On 23/03/2021 09:58, Roger Pau Monne wrote: > Introduce an opaque type that is used to store the CPUID and MSRs > policies of a domain. Such type uses the existing cpu_policy structure > to store the data, but doesn't expose the type to the users of the > xenguest library. > > Introduce an allocation (init) and freeing function (destroy) to > manage the type. > > Note the type is not yet used anywhere. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > tools/include/xenctrl.h | 6 ++++++ > tools/libs/guest/xg_cpuid_x86.c | 28 ++++++++++++++++++++++++++++ > 2 files changed, 34 insertions(+) > > diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h > index e91ff92b9b1..ffb3024bfeb 100644 > --- a/tools/include/xenctrl.h > +++ b/tools/include/xenctrl.h > @@ -2590,6 +2590,12 @@ int xc_psr_get_domain_data(xc_interface *xch, uint32_t domid, > int xc_psr_get_hw_info(xc_interface *xch, uint32_t socket, > xc_psr_feat_type type, xc_psr_hw_info *hw_info); > > +typedef struct cpu_policy *xc_cpu_policy_t; > + > +/* Create and free a xc_cpu_policy object. */ > +xc_cpu_policy_t xc_cpu_policy_init(void); > +void xc_cpu_policy_destroy(xc_cpu_policy_t policy); > + > int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps); > int xc_get_cpu_featureset(xc_interface *xch, uint32_t index, > uint32_t *nr_features, uint32_t *featureset); > diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c > index 9846f81e1f1..ade5281c178 100644 > --- a/tools/libs/guest/xg_cpuid_x86.c > +++ b/tools/libs/guest/xg_cpuid_x86.c > @@ -659,3 +659,31 @@ out: > > return rc; > } > + > +xc_cpu_policy_t xc_cpu_policy_init(void) > +{ > + xc_cpu_policy_t policy = calloc(1, sizeof(*policy)); > + > + if ( !policy ) > + return NULL; > + > + policy->cpuid = calloc(1, sizeof(*policy->cpuid)); > + policy->msr = calloc(1, sizeof(*policy->msr)); > + if ( !policy->cpuid || !policy->msr ) > + { > + xc_cpu_policy_destroy(policy); > + return NULL; > + } > + > + return policy; > +} > + > +void xc_cpu_policy_destroy(xc_cpu_policy_t policy) > +{ > + if ( !policy ) > + return; > + > + free(policy->cpuid); > + free(policy->msr); > + free(policy); > +} Looking at the series as a whole, we have a fair quantity of complexity from short-lived dynamic allocations. I suspect that the code would be rather better if we had struct xc_cpu_policy { struct cpuid_policy cpuid; struct msr_policy msr; xen_cpuid_leaf_t leaves[CPUID_MAX_SERIALISED_LEAVES]; xen_msr_entry_t msrs[MSR_MAX_SERIALISED_ENTRIES]; /* Names perhaps subject to improvement */ }; and just made one memory allocation. This is userspace after all, and we're taking about <4k at the moment. All operations with Xen need to bounce through the leaves/msrs encoding (so we're using the space a minimum of twice for any logical operation at the higher level), and several userspace-only operations use them too. ~Andrew
On Wed, Mar 31, 2021 at 09:10:13PM +0100, Andrew Cooper wrote: > On 23/03/2021 09:58, Roger Pau Monne wrote: > > Introduce an opaque type that is used to store the CPUID and MSRs > > policies of a domain. Such type uses the existing cpu_policy structure > > to store the data, but doesn't expose the type to the users of the > > xenguest library. > > > > Introduce an allocation (init) and freeing function (destroy) to > > manage the type. > > > > Note the type is not yet used anywhere. > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > --- > > tools/include/xenctrl.h | 6 ++++++ > > tools/libs/guest/xg_cpuid_x86.c | 28 ++++++++++++++++++++++++++++ > > 2 files changed, 34 insertions(+) > > > > diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h > > index e91ff92b9b1..ffb3024bfeb 100644 > > --- a/tools/include/xenctrl.h > > +++ b/tools/include/xenctrl.h > > @@ -2590,6 +2590,12 @@ int xc_psr_get_domain_data(xc_interface *xch, uint32_t domid, > > int xc_psr_get_hw_info(xc_interface *xch, uint32_t socket, > > xc_psr_feat_type type, xc_psr_hw_info *hw_info); > > > > +typedef struct cpu_policy *xc_cpu_policy_t; > > + > > +/* Create and free a xc_cpu_policy object. */ > > +xc_cpu_policy_t xc_cpu_policy_init(void); > > +void xc_cpu_policy_destroy(xc_cpu_policy_t policy); > > + > > int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps); > > int xc_get_cpu_featureset(xc_interface *xch, uint32_t index, > > uint32_t *nr_features, uint32_t *featureset); > > diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c > > index 9846f81e1f1..ade5281c178 100644 > > --- a/tools/libs/guest/xg_cpuid_x86.c > > +++ b/tools/libs/guest/xg_cpuid_x86.c > > @@ -659,3 +659,31 @@ out: > > > > return rc; > > } > > + > > +xc_cpu_policy_t xc_cpu_policy_init(void) > > +{ > > + xc_cpu_policy_t policy = calloc(1, sizeof(*policy)); > > + > > + if ( !policy ) > > + return NULL; > > + > > + policy->cpuid = calloc(1, sizeof(*policy->cpuid)); > > + policy->msr = calloc(1, sizeof(*policy->msr)); > > + if ( !policy->cpuid || !policy->msr ) > > + { > > + xc_cpu_policy_destroy(policy); > > + return NULL; > > + } > > + > > + return policy; > > +} > > + > > +void xc_cpu_policy_destroy(xc_cpu_policy_t policy) > > +{ > > + if ( !policy ) > > + return; > > + > > + free(policy->cpuid); > > + free(policy->msr); > > + free(policy); > > +} > > Looking at the series as a whole, we have a fair quantity of complexity > from short-lived dynamic allocations. > > I suspect that the code would be rather better if we had > > struct xc_cpu_policy { > struct cpuid_policy cpuid; > struct msr_policy msr; > xen_cpuid_leaf_t leaves[CPUID_MAX_SERIALISED_LEAVES]; > xen_msr_entry_t msrs[MSR_MAX_SERIALISED_ENTRIES]; > /* Names perhaps subject to improvement */ > }; > > and just made one memory allocation. > > This is userspace after all, and we're taking about <4k at the moment. > > All operations with Xen need to bounce through the leaves/msrs encoding > (so we're using the space a minimum of twice for any logical operation > at the higher level), and several userspace-only operations use them too. We would still need to do some allocations for the system policies, but yes, it would prevent some of the short-lived allocations. I didn't care much because it's all user-space, but removing them will likely make the code simpler. Thanks, Roger.
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h index e91ff92b9b1..ffb3024bfeb 100644 --- a/tools/include/xenctrl.h +++ b/tools/include/xenctrl.h @@ -2590,6 +2590,12 @@ int xc_psr_get_domain_data(xc_interface *xch, uint32_t domid, int xc_psr_get_hw_info(xc_interface *xch, uint32_t socket, xc_psr_feat_type type, xc_psr_hw_info *hw_info); +typedef struct cpu_policy *xc_cpu_policy_t; + +/* Create and free a xc_cpu_policy object. */ +xc_cpu_policy_t xc_cpu_policy_init(void); +void xc_cpu_policy_destroy(xc_cpu_policy_t policy); + int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps); int xc_get_cpu_featureset(xc_interface *xch, uint32_t index, uint32_t *nr_features, uint32_t *featureset); diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c index 9846f81e1f1..ade5281c178 100644 --- a/tools/libs/guest/xg_cpuid_x86.c +++ b/tools/libs/guest/xg_cpuid_x86.c @@ -659,3 +659,31 @@ out: return rc; } + +xc_cpu_policy_t xc_cpu_policy_init(void) +{ + xc_cpu_policy_t policy = calloc(1, sizeof(*policy)); + + if ( !policy ) + return NULL; + + policy->cpuid = calloc(1, sizeof(*policy->cpuid)); + policy->msr = calloc(1, sizeof(*policy->msr)); + if ( !policy->cpuid || !policy->msr ) + { + xc_cpu_policy_destroy(policy); + return NULL; + } + + return policy; +} + +void xc_cpu_policy_destroy(xc_cpu_policy_t policy) +{ + if ( !policy ) + return; + + free(policy->cpuid); + free(policy->msr); + free(policy); +}
Introduce an opaque type that is used to store the CPUID and MSRs policies of a domain. Such type uses the existing cpu_policy structure to store the data, but doesn't expose the type to the users of the xenguest library. Introduce an allocation (init) and freeing function (destroy) to manage the type. Note the type is not yet used anywhere. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- tools/include/xenctrl.h | 6 ++++++ tools/libs/guest/xg_cpuid_x86.c | 28 ++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+)