Re: [PATCH v3 1/2] tools/xg: Streamline cpu policy serialise/deserialise calls
On Fri, May 24, 2024 at 11:32:50AM +0100, Alejandro Vallejo wrote:
> On 23/05/2024 11:21, Roger Pau Monné wrote:
> > On Thu, May 23, 2024 at 10:41:29AM +0100, Alejandro Vallejo wrote:
> >> -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 currently unavoidable for the policy to be likely mutated even in
case of error, as x86_{cpuid,msr}_copy_to_buffer() are two separate
operations, and hence the first succeeding but the second failing will
already result in the policy being mutated on error.
> 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.
I'm fine either way (hence why prefix it with "nit:") albeit I have a
preference for not introducing the local variables if they are not
needed.
Thanks, Roger.
Re: [PATCH v3 1/2] tools/xg: Streamline cpu policy serialise/deserialise calls
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
>
> Acked-by: Roger Pau Monné
>
> 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_l
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
Acked-by: Roger Pau Monné
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
[PATCH v3 1/2] tools/xg: Streamline cpu policy serialise/deserialise calls
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
---
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);
/* 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);
-unsig
