Re: [PATCH v3 2/2] tools/xg: Clean up xend-style overrides for CPU policies
On 23/05/2024 11:47, Roger Pau Monné wrote:
>> -static xen_cpuid_leaf_t *find_leaf(
>> -xen_cpuid_leaf_t *leaves, unsigned int nr_leaves,
>> -const struct xc_xend_cpuid *xend)
>> +static xen_cpuid_leaf_t *find_leaf(xc_cpu_policy_t *p,
>> + const struct xc_xend_cpuid *xend)
>> {
>> const xen_cpuid_leaf_t key = { xend->leaf, xend->subleaf };
>>
>> -return bsearch(&key, leaves, nr_leaves, sizeof(*leaves),
>> compare_leaves);
>> +return bsearch(&key, p->leaves, ARRAY_SIZE(p->leaves),
>
> Don't you need to use p->nr_leaves here, as otherwise we could check
> against possibly uninitialized leaves (or leaves with stale data)?
Indeed. Good catch (same on the MSR side).
>> -switch ( p->policy.x86_vendor )
>> +switch ( cur->policy.x86_vendor )
>> {
>> case X86_VENDOR_INTEL:
>> -for ( i = 0; (p->policy.cache.subleaf[i].type &&
>> - i < ARRAY_SIZE(p->policy.cache.raw)); ++i )
>> +for ( i = 0; (cur->policy.cache.subleaf[i].type &&
>> +i < ARRAY_SIZE(cur->policy.cache.raw)); ++i
>> )
>
> Nit: indentation is weird here. I would use:
>
> for ( i = 0; cur->policy.cache.subleaf[i].type &&
> i < ARRAY_SIZE(cur->policy.cache.raw); ++i )
>
> Thanks, Roger.
Sure. Leftover from removing the size_t in v2.
Cheers,
Alejandro
Re: [PATCH v3 2/2] tools/xg: Clean up xend-style overrides for CPU policies
On Thu, May 23, 2024 at 10:41:30AM +0100, Alejandro Vallejo wrote:
> Factor out policy getters/setters from both (CPUID and MSR) policy override
> functions. Additionally, use host policy rather than featureset when
> preparing the cur policy, saving one hypercall and several lines of
> boilerplate.
>
> No functional change intended.
>
> Signed-off-by: Alejandro Vallejo
> ---
> v3:
> * Restored overscoped loop indices
> * Split long line in conditional
> ---
> tools/libs/guest/xg_cpuid_x86.c | 438 ++--
> 1 file changed, 131 insertions(+), 307 deletions(-)
>
> diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
> index 4f4b86b59470..1e631fd46d2f 100644
> --- a/tools/libs/guest/xg_cpuid_x86.c
> +++ b/tools/libs/guest/xg_cpuid_x86.c
> @@ -36,6 +36,34 @@ enum {
> #define bitmaskof(idx) (1u << ((idx) & 31))
> #define featureword_of(idx) ((idx) >> 5)
>
> +static int deserialize_policy(xc_interface *xch, xc_cpu_policy_t *policy)
> +{
> +uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
> +int rc;
> +
> +rc = x86_cpuid_copy_from_buffer(&policy->policy, policy->leaves,
> +policy->nr_leaves, &err_leaf,
> &err_subleaf);
> +if ( rc )
> +{
> +if ( err_leaf != -1 )
> +ERROR("Failed to deserialise CPUID (err leaf %#x, subleaf %#x)
> (%d = %s)",
> + err_leaf, err_subleaf, -rc, strerror(-rc));
> +return rc;
> +}
> +
> +rc = x86_msr_copy_from_buffer(&policy->policy, policy->msrs,
> + policy->nr_msrs, &err_msr);
> +if ( rc )
> +{
> +if ( err_msr != -1 )
> +ERROR("Failed to deserialise MSR (err MSR %#x) (%d = %s)",
> + err_msr, -rc, strerror(-rc));
> +return rc;
> +}
> +
> +return 0;
> +}
> +
> int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps)
> {
> struct xen_sysctl sysctl = {};
> @@ -260,102 +288,37 @@ static int compare_leaves(const void *l, const void *r)
> return 0;
> }
>
> -static xen_cpuid_leaf_t *find_leaf(
> -xen_cpuid_leaf_t *leaves, unsigned int nr_leaves,
> -const struct xc_xend_cpuid *xend)
> +static xen_cpuid_leaf_t *find_leaf(xc_cpu_policy_t *p,
> + const struct xc_xend_cpuid *xend)
> {
> const xen_cpuid_leaf_t key = { xend->leaf, xend->subleaf };
>
> -return bsearch(&key, leaves, nr_leaves, sizeof(*leaves), compare_leaves);
> +return bsearch(&key, p->leaves, ARRAY_SIZE(p->leaves),
Don't you need to use p->nr_leaves here, as otherwise we could check
against possibly uninitialized leaves (or leaves with stale data)?
> + sizeof(*p->leaves), compare_leaves);
> }
>
> -static int xc_cpuid_xend_policy(
> -xc_interface *xch, uint32_t domid, const struct xc_xend_cpuid *xend)
> +static int xc_cpuid_xend_policy(xc_interface *xch, uint32_t domid,
> +const struct xc_xend_cpuid *xend,
> +xc_cpu_policy_t *host,
> +xc_cpu_policy_t *def,
> +xc_cpu_policy_t *cur)
> {
> -int rc;
> -bool hvm;
> -xc_domaininfo_t di;
> -unsigned int nr_leaves, nr_msrs;
> -uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
> -/*
> - * Three full policies. The host, default for the domain type,
> - * and domain current.
> - */
> -xen_cpuid_leaf_t *host = NULL, *def = NULL, *cur = NULL;
> -unsigned int nr_host, nr_def, nr_cur;
> -
> -if ( (rc = xc_domain_getinfo_single(xch, domid, &di)) < 0 )
> -{
> -PERROR("Failed to obtain d%d info", domid);
> -rc = -errno;
> -goto fail;
> -}
> -hvm = di.flags & XEN_DOMINF_hvm_guest;
> -
> -rc = xc_cpu_policy_get_size(xch, &nr_leaves, &nr_msrs);
> -if ( rc )
> -{
> -PERROR("Failed to obtain policy info size");
> -rc = -errno;
> -goto fail;
> -}
> -
> -rc = -ENOMEM;
> -if ( (host = calloc(nr_leaves, sizeof(*host))) == NULL ||
> - (def = calloc(nr_leaves, sizeof(*def))) == NULL ||
> - (cur = calloc(nr_leaves, sizeof(*cur))) == NULL )
> -{
> -ERROR("Unable to allocate memory for %u CPUID leaves", nr_leaves);
> -goto fail;
> -}
> -
> -/* Get the domain's current policy. */
> -nr_msrs = 0;
> -nr_cur = nr_leaves;
> -rc = get_domain_cpu_policy(xch, domid, &nr_cur, cur, &nr_msrs, NULL);
> -if ( rc )
> -{
> -PERROR("Failed to obtain d%d current policy", domid);
> -rc = -errno;
> -goto fail;
> -}
> +if ( !xend )
> +return 0;
>
> -/* Get the domain type's default policy. */
> -nr_msrs = 0;
> -nr_def = nr_leaves;
> -rc = get_system_cpu_policy(xch, hvm ? XEN_SYSCTL_cpu_policy_hvm_default
> -: XEN_SYSCTL_cpu_poli
[PATCH v3 2/2] tools/xg: Clean up xend-style overrides for CPU policies
Factor out policy getters/setters from both (CPUID and MSR) policy override
functions. Additionally, use host policy rather than featureset when
preparing the cur policy, saving one hypercall and several lines of
boilerplate.
No functional change intended.
Signed-off-by: Alejandro Vallejo
---
v3:
* Restored overscoped loop indices
* Split long line in conditional
---
tools/libs/guest/xg_cpuid_x86.c | 438 ++--
1 file changed, 131 insertions(+), 307 deletions(-)
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index 4f4b86b59470..1e631fd46d2f 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -36,6 +36,34 @@ enum {
#define bitmaskof(idx) (1u << ((idx) & 31))
#define featureword_of(idx) ((idx) >> 5)
+static int deserialize_policy(xc_interface *xch, xc_cpu_policy_t *policy)
+{
+uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
+int rc;
+
+rc = x86_cpuid_copy_from_buffer(&policy->policy, policy->leaves,
+policy->nr_leaves, &err_leaf,
&err_subleaf);
+if ( rc )
+{
+if ( err_leaf != -1 )
+ERROR("Failed to deserialise CPUID (err leaf %#x, subleaf %#x) (%d
= %s)",
+ err_leaf, err_subleaf, -rc, strerror(-rc));
+return rc;
+}
+
+rc = x86_msr_copy_from_buffer(&policy->policy, policy->msrs,
+ policy->nr_msrs, &err_msr);
+if ( rc )
+{
+if ( err_msr != -1 )
+ERROR("Failed to deserialise MSR (err MSR %#x) (%d = %s)",
+ err_msr, -rc, strerror(-rc));
+return rc;
+}
+
+return 0;
+}
+
int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps)
{
struct xen_sysctl sysctl = {};
@@ -260,102 +288,37 @@ static int compare_leaves(const void *l, const void *r)
return 0;
}
-static xen_cpuid_leaf_t *find_leaf(
-xen_cpuid_leaf_t *leaves, unsigned int nr_leaves,
-const struct xc_xend_cpuid *xend)
+static xen_cpuid_leaf_t *find_leaf(xc_cpu_policy_t *p,
+ const struct xc_xend_cpuid *xend)
{
const xen_cpuid_leaf_t key = { xend->leaf, xend->subleaf };
-return bsearch(&key, leaves, nr_leaves, sizeof(*leaves), compare_leaves);
+return bsearch(&key, p->leaves, ARRAY_SIZE(p->leaves),
+ sizeof(*p->leaves), compare_leaves);
}
-static int xc_cpuid_xend_policy(
-xc_interface *xch, uint32_t domid, const struct xc_xend_cpuid *xend)
+static int xc_cpuid_xend_policy(xc_interface *xch, uint32_t domid,
+const struct xc_xend_cpuid *xend,
+xc_cpu_policy_t *host,
+xc_cpu_policy_t *def,
+xc_cpu_policy_t *cur)
{
-int rc;
-bool hvm;
-xc_domaininfo_t di;
-unsigned int nr_leaves, nr_msrs;
-uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
-/*
- * Three full policies. The host, default for the domain type,
- * and domain current.
- */
-xen_cpuid_leaf_t *host = NULL, *def = NULL, *cur = NULL;
-unsigned int nr_host, nr_def, nr_cur;
-
-if ( (rc = xc_domain_getinfo_single(xch, domid, &di)) < 0 )
-{
-PERROR("Failed to obtain d%d info", domid);
-rc = -errno;
-goto fail;
-}
-hvm = di.flags & XEN_DOMINF_hvm_guest;
-
-rc = xc_cpu_policy_get_size(xch, &nr_leaves, &nr_msrs);
-if ( rc )
-{
-PERROR("Failed to obtain policy info size");
-rc = -errno;
-goto fail;
-}
-
-rc = -ENOMEM;
-if ( (host = calloc(nr_leaves, sizeof(*host))) == NULL ||
- (def = calloc(nr_leaves, sizeof(*def))) == NULL ||
- (cur = calloc(nr_leaves, sizeof(*cur))) == NULL )
-{
-ERROR("Unable to allocate memory for %u CPUID leaves", nr_leaves);
-goto fail;
-}
-
-/* Get the domain's current policy. */
-nr_msrs = 0;
-nr_cur = nr_leaves;
-rc = get_domain_cpu_policy(xch, domid, &nr_cur, cur, &nr_msrs, NULL);
-if ( rc )
-{
-PERROR("Failed to obtain d%d current policy", domid);
-rc = -errno;
-goto fail;
-}
+if ( !xend )
+return 0;
-/* Get the domain type's default policy. */
-nr_msrs = 0;
-nr_def = nr_leaves;
-rc = get_system_cpu_policy(xch, hvm ? XEN_SYSCTL_cpu_policy_hvm_default
-: XEN_SYSCTL_cpu_policy_pv_default,
- &nr_def, def, &nr_msrs, NULL);
-if ( rc )
-{
-PERROR("Failed to obtain %s def policy", hvm ? "hvm" : "pv");
-rc = -errno;
-goto fail;
-}
+if ( !host || !def || !cur )
+return -EINVAL;
-/* Get the host policy. */
-nr_msrs = 0;
-nr_host = nr_leaves;
-rc = get_system_cpu_policy(xch, XEN_SYSCTL_cpu_policy_host,
- &nr_host, host, &nr_msrs,
