|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 1/2] tools/xg: Streamline cpu policy serialise/deserialise calls
On Thu, May 23, 2024 at 10:41:29AM +0100, Alejandro Vallejo wrote:
> The idea is to use xc_cpu_policy_t as a single object containing both the
> serialised and deserialised forms of the policy. Note that we need lengths
> for the arrays, as the serialised policies may be shorter than the array
> capacities.
>
> * Add the serialised lengths to the struct so we can distinguish
> between length and capacity of the serialisation buffers.
> * Remove explicit buffer+lengths in serialise/deserialise calls
> and use the internal buffer inside xc_cpu_policy_t instead.
> * Refactor everything to use the new serialisation functions.
> * Remove redundant serialization calls and avoid allocating dynamic
> memory aside from the policy objects in xen-cpuid. Also minor cleanup
> in the policy print call sites.
>
> No functional change intended.
>
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
Just two comments.
> ---
> v3:
> * Better context scoping in xg_sr_common_x86.
> * Can't be const because write_record() takes non-const.
> * Adjusted line length of xen-cpuid's print_policy.
> * Adjusted error messages in xen-cpuid's print_policy.
> * Reverted removal of overscoped loop indices.
> ---
> tools/include/xenguest.h | 8 ++-
> tools/libs/guest/xg_cpuid_x86.c | 98 ++++++++++++++++++++---------
> tools/libs/guest/xg_private.h | 2 +
> tools/libs/guest/xg_sr_common_x86.c | 56 ++++++-----------
> tools/misc/xen-cpuid.c | 41 ++++--------
> 5 files changed, 106 insertions(+), 99 deletions(-)
>
> diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
> index e01f494b772a..563811cd8dde 100644
> --- a/tools/include/xenguest.h
> +++ b/tools/include/xenguest.h
> @@ -799,14 +799,16 @@ int xc_cpu_policy_set_domain(xc_interface *xch,
> uint32_t domid,
> xc_cpu_policy_t *policy);
>
> /* Manipulate a policy via architectural representations. */
> -int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t *policy,
> - xen_cpuid_leaf_t *leaves, uint32_t *nr_leaves,
> - xen_msr_entry_t *msrs, uint32_t *nr_msrs);
> +int xc_cpu_policy_serialise(xc_interface *xch, xc_cpu_policy_t *policy);
> int xc_cpu_policy_update_cpuid(xc_interface *xch, xc_cpu_policy_t *policy,
> const xen_cpuid_leaf_t *leaves,
> uint32_t nr);
> int xc_cpu_policy_update_msrs(xc_interface *xch, xc_cpu_policy_t *policy,
> const xen_msr_entry_t *msrs, uint32_t nr);
> +int xc_cpu_policy_get_leaves(xc_interface *xch, const xc_cpu_policy_t
> *policy,
> + const xen_cpuid_leaf_t **leaves, uint32_t *nr);
> +int xc_cpu_policy_get_msrs(xc_interface *xch, const xc_cpu_policy_t *policy,
> + const xen_msr_entry_t **msrs, uint32_t *nr);
Maybe it would be helpful to have a comment clarifying that the return
of xc_cpu_policy_get_{leaves,msrs}() is a reference to the content of
the policy, not a copy of it (and hence is tied to the lifetime of
policy, and doesn't require explicit freeing).
>
> /* Compatibility calculations. */
> bool xc_cpu_policy_is_compatible(xc_interface *xch, xc_cpu_policy_t *host,
> diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
> index 4453178100ad..4f4b86b59470 100644
> --- a/tools/libs/guest/xg_cpuid_x86.c
> +++ b/tools/libs/guest/xg_cpuid_x86.c
> @@ -834,14 +834,13 @@ void xc_cpu_policy_destroy(xc_cpu_policy_t *policy)
> }
> }
>
> -static int deserialize_policy(xc_interface *xch, xc_cpu_policy_t *policy,
> - unsigned int nr_leaves, unsigned int
> nr_entries)
> +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,
> - nr_leaves, &err_leaf, &err_subleaf);
> + policy->nr_leaves, &err_leaf,
> &err_subleaf);
> if ( rc )
> {
> if ( err_leaf != -1 )
> @@ -851,7 +850,7 @@ static int deserialize_policy(xc_interface *xch,
> xc_cpu_policy_t *policy,
> }
>
> rc = x86_msr_copy_from_buffer(&policy->policy, policy->msrs,
> - nr_entries, &err_msr);
> + policy->nr_msrs, &err_msr);
> if ( rc )
> {
> if ( err_msr != -1 )
> @@ -878,7 +877,10 @@ int xc_cpu_policy_get_system(xc_interface *xch, unsigned
> int policy_idx,
> return rc;
> }
>
> - rc = deserialize_policy(xch, policy, nr_leaves, nr_msrs);
> + policy->nr_leaves = nr_leaves;
> + policy->nr_msrs = nr_msrs;
> +
> + rc = deserialize_policy(xch, policy);
> if ( rc )
> {
> errno = -rc;
> @@ -903,7 +905,10 @@ int xc_cpu_policy_get_domain(xc_interface *xch, uint32_t
> domid,
> return rc;
> }
>
> - rc = deserialize_policy(xch, policy, nr_leaves, nr_msrs);
> + policy->nr_leaves = nr_leaves;
> + policy->nr_msrs = nr_msrs;
> +
> + rc = deserialize_policy(xch, policy);
> if ( rc )
> {
> errno = -rc;
> @@ -917,17 +922,14 @@ int xc_cpu_policy_set_domain(xc_interface *xch,
> uint32_t domid,
> xc_cpu_policy_t *policy)
> {
> uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
> - unsigned int nr_leaves = ARRAY_SIZE(policy->leaves);
> - unsigned int nr_msrs = ARRAY_SIZE(policy->msrs);
> int rc;
>
> - rc = xc_cpu_policy_serialise(xch, policy, policy->leaves, &nr_leaves,
> - policy->msrs, &nr_msrs);
> + rc = xc_cpu_policy_serialise(xch, policy);
> if ( rc )
> return rc;
>
> - rc = xc_set_domain_cpu_policy(xch, domid, nr_leaves, policy->leaves,
> - nr_msrs, policy->msrs,
> + rc = xc_set_domain_cpu_policy(xch, domid, policy->nr_leaves,
> policy->leaves,
> + policy->nr_msrs, policy->msrs,
> &err_leaf, &err_subleaf, &err_msr);
> if ( rc )
> {
> @@ -942,34 +944,32 @@ int xc_cpu_policy_set_domain(xc_interface *xch,
> uint32_t domid,
> return rc;
> }
>
> -int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t *p,
> - xen_cpuid_leaf_t *leaves, uint32_t *nr_leaves,
> - xen_msr_entry_t *msrs, uint32_t *nr_msrs)
> +int xc_cpu_policy_serialise(xc_interface *xch, xc_cpu_policy_t *p)
> {
> + unsigned int nr_leaves = ARRAY_SIZE(p->leaves);
> + unsigned int nr_msrs = ARRAY_SIZE(p->msrs);
> int rc;
>
> - if ( leaves )
> + rc = x86_cpuid_copy_to_buffer(&p->policy, p->leaves, &nr_leaves);
> + if ( rc )
> {
> - rc = x86_cpuid_copy_to_buffer(&p->policy, leaves, nr_leaves);
> - if ( rc )
> - {
> - ERROR("Failed to serialize CPUID policy");
> - errno = -rc;
> - return -1;
> - }
> + ERROR("Failed to serialize CPUID policy");
> + errno = -rc;
> + return -1;
> }
>
> - if ( msrs )
> + p->nr_leaves = nr_leaves;
Nit: FWIW, I think you could avoid having to introduce local
nr_{leaves,msrs} variables and just use p->nr_{leaves,msrs}? By
setting them to ARRAY_SIZE() at the top of the function and then
letting x86_{cpuid,msr}_copy_to_buffer() adjust as necessary.
Thanks, Roger.
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |