On 23/05/2024 11:21, Roger Pau Monné wrote:
> 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.vall...@cloud.com>
>
> Acked-by: Roger Pau Monné <roger....@citrix.com>
>
> 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).
Sure.
>
>>
>> /* 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.
The intent was to avoid mutating the policy object in the error cases
during deserialise. Then I adjusted the serialise case to have symmetry.
It's true the preservation is not meaningful in the serialise case
because at that point the serialised form is already corrupted.
I don't mind either way. Seeing how I'm sending one final version with
the comments of patch2 I'll just adjust as you proposed.
Cheers,
Alejandro
