Re: [PATCH v2 2/2] tools/xg: Clean up xend-style overrides for CPU policies
On 20/05/2024 16:02, Roger Pau Monné wrote:
>> -
>> static int xc_msr_policy(xc_interface *xch, domid_t domid,
>> - const struct xc_msr *msr)
>> + const struct xc_msr *msr,
>> + xc_cpu_policy_t *host,
>> + xc_cpu_policy_t *def,
>
> host and def should likely be const?
I tried, but I can't. All policies go through find_msr(), which takes a
non-const policy, and must be non-const because it's also used for the
cur policy.
I did the next best thing (I think) by const-ifying the result of
find_msr inside the loop for host and def. Same thing on the cpuid function.
>> -if ( rc )
>> -{
>> -PERROR("Failed to obtain host policy");
>> -rc = -errno;
>> -goto out;
>> -}
>> +if ( !msrs )
>
> Does this build? Where is 'msrs' defined in this context? The
> function parameter is 'msr' AFAICT.
Ugh. I fixed that while adjusting it for testing within XenServer and
then neglected to make the change in the actual for-upstream patches.
You're right.
>
>> +return 0;
>
> Should we also check for host, def, cur != NULL also?
It's already done by the caller, but can do out of paranoia; returning
-EINVAL.
>> @@ -583,14 +436,16 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t
>> domid, bool restore,
>> int rc;
>> bool hvm;
>> xc_domaininfo_t di;
>> -struct xc_cpu_policy *p = xc_cpu_policy_init();
>> -unsigned int i, nr_leaves = ARRAY_SIZE(p->leaves), nr_msrs = 0;
>> -uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
>> -uint32_t host_featureset[FEATURESET_NR_ENTRIES] = {};
>> -uint32_t len = ARRAY_SIZE(host_featureset);
>>
>> -if ( !p )
>> -return -ENOMEM;
>> +struct xc_cpu_policy *host = xc_cpu_policy_init();
>> +struct xc_cpu_policy *def = xc_cpu_policy_init();
>
> I would be helpful to have some kind of mechanism to allocate + init a
> policy at the same time, so that the resulting object could be made
> const here. (Not that you need to do it in this patch).
That would seem sensible, but we'd also need a way to clone it to avoid
repeating hypercalls when they aren't required. I had a patch that did
that, but was quite complicated for other reasons. I might get back to
it at some point now that per-vCPU policies don't seem to be required.
>> @@ -695,24 +542,24 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t
>> domid, bool restore,
>> !(dfs = x86_cpu_policy_lookup_deep_deps(b)) )
>> continue;
>>
>> -for ( i = 0; i < ARRAY_SIZE(disabled_features); ++i )
>> +for ( size_t i = 0; i < ARRAY_SIZE(disabled_features); ++i )
>
> All this loop index type changes could be done as a separate patch,
> you are not even touching the surrounding lines. It adds a lot of
> churn to this patch for no reason IMO.
I got carried away. Let me revert that. I still want to get rid of all
those overscoped indices, but this is not the patch for it.
>> @@ -772,49 +619,45 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t
>> domid, bool restore,
>> * apic_id_size values greater than 7. Limit the value to
>> * 7 for now.
>> */
>> -if ( p->policy.extd.nc < 0x7f )
>> +if ( cur->policy.extd.nc < 0x7f )
>> {
>> -if ( p->policy.extd.apic_id_size != 0 &&
>> p->policy.extd.apic_id_size < 0x7 )
>> -p->policy.extd.apic_id_size++;
>> +if ( cur->policy.extd.apic_id_size != 0 &&
>> cur->policy.extd.apic_id_size < 0x7 )
>
> I would split the line while there, it's overly long.
Ack
>
> Thanks, Roger.
Cheers,
Alejandro
Re: [PATCH v2 2/2] tools/xg: Clean up xend-style overrides for CPU policies
On Fri, May 17, 2024 at 05:08:35PM +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
> ---
> v2:
> * Cosmetic change to comment (// into /**/).
> * Added missing null pointer check to MSR override function.
> ---
> tools/libs/guest/xg_cpuid_x86.c | 445 ++--
> 1 file changed, 130 insertions(+), 315 deletions(-)
>
> diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
> index 4f4b86b59470..74bca0e65b69 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,34 @@ 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;
> -}
> -
> -/* 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
[PATCH v2 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
---
v2:
* Cosmetic change to comment (// into /**/).
* Added missing null pointer check to MSR override function.
---
tools/libs/guest/xg_cpuid_x86.c | 445 ++--
1 file changed, 130 insertions(+), 315 deletions(-)
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index 4f4b86b59470..74bca0e65b69 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,34 @@ 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;
-}
-
-/* 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;
-}
-
-/* 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, NULL);
-if ( rc )
-{
-PERROR("Failed to o
