|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v4 1/2] tools/xg: Streamline cpu policy serialise/deserialise calls
On 31/05/2024 00:59, Andrew Cooper wrote:
> On 29/05/2024 3:30 pm, Alejandro Vallejo wrote:
>> diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
>> index e01f494b772a..85d56f26537b 100644
>> --- a/tools/include/xenguest.h
>> +++ b/tools/include/xenguest.h
>> @@ -799,15 +799,23 @@ 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);
>>
>> +/*
>> + * Accessors for the serialised forms of the policy. The outputs are
>> pointers
>> + * into the policy object and not fresh allocations, so their lifetimes are
>> tied
>> + * to the policy object itself.
>
> This is far more complicated. See below.
>
>> + */
>> +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,
>> xc_cpu_policy_t *guest);
>> diff --git a/tools/libs/guest/xg_cpuid_x86.c
>> b/tools/libs/guest/xg_cpuid_x86.c
>> index 4453178100ad..6cab5c60bb41 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 )
>
> Urgh - this is a mess.
That's a fair assessment, yes :)
> (Not your fault, but we really need to think
> twice before continuing.)>
> xc_cpu_policy_serialise() is an exported function and, prior to this
> series, used by external entities to get at the content inside the
> opaque object.>
> deserialize_policy() (Clearly not written by me - Roger?) is a local
> helper. Also it looks wonky in the next patch, although I think that's
> just code movement to avoid a forward declaration?
Correct.
>
> By the end of the series, xc_cpu_policy_serialise() isn't used
> externally, but it's still exported.
Because it's external and I didn't want to break the ABI should it be
used somewhere downstream. Happy to change that though.
>
> But, besides the visibility, there's a second difference...
>
>
>> @@ -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);
>
> ... they're asymmetric as to whether the caller or the callee preloads
> policy->nr_*.>
> Both of these need rationalising, one way or another.
>
I'm not sure I follow. Neither the why nor the how.
nr_* assignments are not arbitrary. Deserialising doesn't involve
modifying the number of leaves/MSRs. The cases in which they are set are:
1. New policy is loaded from the hypervisor
* Must be reset to maximum values before hypercall and set to the
outputs of the hypercall after.
2. Policy is serialised
* Number of leaves could increase/decrease
Case (1) must be handled in the hypercall wrappers themselves because
the policy is effectively an output object as a whole then. On case (2)
they must be set by the serialiser itself, as that's the one that knows
the end result.
Could you elaborate in what you mean?
>
> But, there's a related problem.
>
> Previously there was only one canonical form (the deserialised form),
> and anything operating on state was responsible for getting it back to
> being the deserialised form.
That's true. But this tension exists regardless of this patch. Some
parts of the code want to operate on raw data and others on structured
data; and then others on featuresets because 3 forms are better than 2.
The roundtrip through featuresets already breaks apart the idea of a
"canonical" form. Truth is, users of this framework operate on a "I know
what form I'm operating on and what form I must restore it to", if not
by design then by accident.
I don't have a good argument about the fragility of the whole thing
besides it being silly, convoluted and visually noisy to dynamically
allocate a fixed-sized array in order to deserialize things. Plus it's
yet another source of errors when callers have to keep track of their
own dynamic buffers.
>
> Now, there are two forms which are coexist side by side. The buffer
> exposed by get_{cpuid,msr}() is only good until the next operation which
> uses what were (previously) the internal staging buffer(s).
>
> And that makes it a fragile and error prone interface.
3, including the featuresets. And note that dynamically allocating
buffers outside the policy object is very error prone as well. It's a
matter of where we want to have fragility.
Unrelated for what you mean, but I'll put my Rust hat for this and start
preaching; This is the sort of thing Rust's borrow checker can be abused
to avoid lifetime related pitfalls.
Can you think of another way that doesn't involve a copyout?
>> 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,32 +944,26 @@ 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)
>> {
>> int rc;
>> + p->nr_leaves = ARRAY_SIZE(p->leaves);
>> + p->nr_msrs = ARRAY_SIZE(p->msrs);
>>
>> - if ( leaves )
>> + rc = x86_cpuid_copy_to_buffer(&p->policy, p->leaves, &p->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 )
>> + rc = x86_msr_copy_to_buffer(&p->policy, p->msrs, &p->nr_msrs);
>> + if ( rc )
>> {
>> - rc = x86_msr_copy_to_buffer(&p->policy, msrs, nr_msrs);
>> - if ( rc )
>> - {
>> - ERROR("Failed to serialize MSR policy");
>> - errno = -rc;
>> - return -1;
>> - }
>> + ERROR("Failed to serialize MSR policy");
>> + errno = -rc;
>> + return -1;
>> }
>>
>> errno = 0;
>> @@ -1012,6 +1008,42 @@ int xc_cpu_policy_update_msrs(xc_interface *xch,
>> xc_cpu_policy_t *policy,
>> return rc;
>> }
>>
>> +int xc_cpu_policy_get_leaves(xc_interface *xch,
>> + const xc_cpu_policy_t *policy,
>> + const xen_cpuid_leaf_t **leaves,
>> + uint32_t *nr)
>> +{
>> + if ( !policy )
>> + {
>> + ERROR("Failed to fetch CPUID leaves from policy object");
>> + errno = -EINVAL;
>> + return -1;
>> + }
>
> This check isn't useful, and it's making the interface inconsistent.
> There's no case ever where a NULL policy is meaningful, except for the
> very initial failure to allocate, and there it's the return value not an
> input parameter.>
> More importantly however, the error message is misleading as a consequence.
Pretty sure I've been asked before to validate trivial preconditions so
I'm guessing various maintainers have conflicting opinions on how
aggresively to validate?
TL;DR: Sure, happy to rip that out.
>
>> +
>> + *leaves = policy->leaves;
>> + *nr = policy->nr_leaves;
>> +
>> + return 0;
>> +}
>> +
>> +int xc_cpu_policy_get_msrs(xc_interface *xch,
>> + const xc_cpu_policy_t *policy,
>> + const xen_msr_entry_t **msrs,
>> + uint32_t *nr)
>> +{
>> + if ( !policy )
>> + {
>> + ERROR("Failed to fetch MSRs from policy object");
>> + errno = -EINVAL;
>> + return -1;
>> + }
>> +
>> + *msrs = policy->msrs;
>> + *nr = policy->nr_msrs;
>> +
>> + return 0;
>> +}
>> +
>> bool xc_cpu_policy_is_compatible(xc_interface *xch, xc_cpu_policy_t *host,
>> xc_cpu_policy_t *guest)
>> {
>> diff --git a/tools/libs/guest/xg_private.h b/tools/libs/guest/xg_private.h
>> index d73947094f2e..a65dae818f3d 100644
>> --- a/tools/libs/guest/xg_private.h
>> +++ b/tools/libs/guest/xg_private.h
>> @@ -177,6 +177,8 @@ struct xc_cpu_policy {
>> struct cpu_policy policy;
>> xen_cpuid_leaf_t leaves[CPUID_MAX_SERIALISED_LEAVES];
>> xen_msr_entry_t msrs[MSR_MAX_SERIALISED_ENTRIES];
>> + uint32_t nr_leaves;
>> + uint32_t nr_msrs;
>
> These need a comment explaining how they're used, and sadly they have no
> relationship to the lengths of the array. There's a corner case where
> they can end up larger.
The may end up smaller, but they must absolutely never end up larger. If
such a corner case exists, please elaborate because it's a bug I'd like
to have fixed.
>
>> };
>> #endif /* x86 */
>>
>> diff --git a/tools/libs/guest/xg_sr_common_x86.c
>> b/tools/libs/guest/xg_sr_common_x86.c
>> index 563b4f016877..a0d67c3211c6 100644
>> --- a/tools/libs/guest/xg_sr_common_x86.c
>> +++ b/tools/libs/guest/xg_sr_common_x86.c
>> @@ -1,4 +1,5 @@
>> #include "xg_sr_common_x86.h"
>> +#include "xg_sr_stream_format.h"
>
> I'm pretty sure this shouldn't be necessary. Is it?
Indeed. Leftover from previous version
>
>>
>> int write_x86_tsc_info(struct xc_sr_context *ctx)
>> {
>> @@ -45,54 +46,39 @@ int handle_x86_tsc_info(struct xc_sr_context *ctx,
>> struct xc_sr_record *rec)
>> int write_x86_cpu_policy_records(struct xc_sr_context *ctx)
>> {
>> xc_interface *xch = ctx->xch;
>> - struct xc_sr_record cpuid = { .type = REC_TYPE_X86_CPUID_POLICY, };
>> - struct xc_sr_record msrs = { .type = REC_TYPE_X86_MSR_POLICY, };
>> - uint32_t nr_leaves = 0, nr_msrs = 0;
>> - xc_cpu_policy_t *policy = NULL;
>> + xc_cpu_policy_t *policy = xc_cpu_policy_init();
>> int rc;
>>
>> - if ( xc_cpu_policy_get_size(xch, &nr_leaves, &nr_msrs) < 0 )
>> - {
>> - PERROR("Unable to get CPU Policy size");
>> - return -1;
>> - }
>> -
>> - cpuid.data = malloc(nr_leaves * sizeof(xen_cpuid_leaf_t));
>> - msrs.data = malloc(nr_msrs * sizeof(xen_msr_entry_t));
>> - policy = xc_cpu_policy_init();
>> - if ( !cpuid.data || !msrs.data || !policy )
>> - {
>> - ERROR("Cannot allocate memory for CPU Policy");
>> - rc = -1;
>> - goto out;
>> - }
>> -
>> - if ( xc_cpu_policy_get_domain(xch, ctx->domid, policy) )
>> + if ( !policy || xc_cpu_policy_get_domain(xch, ctx->domid, policy) )
>> {
>> PERROR("Unable to get d%d CPU Policy", ctx->domid);
>> rc = -1;
>> goto out;
>> }
>> - if ( xc_cpu_policy_serialise(xch, policy, cpuid.data, &nr_leaves,
>> - msrs.data, &nr_msrs) )
>> - {
>> - PERROR("Unable to serialize d%d CPU Policy", ctx->domid);
>> - rc = -1;
>> - goto out;
>> - }
>
> Wow, the old code here was especially daft.
Can confirm.
>
> We're having Xen serialise the policy, copying (double buffering) into
> the policy object then desensitising. And vs the old copy, we've got
> rid of the re-serialise into yet another buffer.
>
> But we should still be using a plain XEN_DOMCTL_get_cpu_policy here.
> Literally all we want to do is take the array(s) Xen gave us and feed
> them straight into the fd.
>
> deserialising is already a reasonably expensive operation (every
> individual leaf coordinate needs re-range checking), and is only ever
> going to get worse.
I wouldn't go that far. It's definitely on the "don't do it every other
operation", but it's a tiny fraction of the (current) cost of a single
hypercall.
>
> It will probably help to split the changes to
> write_x86_cpu_policy_records() out into a separate patch. It's more
> clear cut and also addresses one of the local vs external issues
> discussed above.
>
>
>>
>> - cpuid.length = nr_leaves * sizeof(xen_cpuid_leaf_t);
>> - if ( cpuid.length )
>> +
>> + if ( policy->nr_leaves )
>> {
>> - rc = write_record(ctx, &cpuid);
>> + struct xc_sr_record record = {
>> + .type = REC_TYPE_X86_CPUID_POLICY,
>> + .data = policy->leaves,
>> + .length = policy->nr_leaves * sizeof(*policy->leaves),
>> + };
>> +
>> + rc = write_record(ctx, &record);
>
> Please keep this name being cpuid. It's more helpful when grepping, and
> it also shrinks the diff.
Ack
>
>> if ( rc )
>> goto out;
>> }
>>
>> - msrs.length = nr_msrs * sizeof(xen_msr_entry_t);
>> - if ( msrs.length )
>> + if ( policy->nr_msrs )
>> {
>> - rc = write_record(ctx, &msrs);
>> + struct xc_sr_record record = {
>> + .type = REC_TYPE_X86_MSR_POLICY,
>> + .data = policy->msrs,
>> + .length = policy->nr_msrs * sizeof(*policy->msrs),
>> + };
>> +
>> + rc = write_record(ctx, &record);
>> if ( rc )
>> goto out;
>> }
>> @@ -100,8 +86,6 @@ int write_x86_cpu_policy_records(struct xc_sr_context
>> *ctx)
>> rc = 0;
>>
>> out:
>> - free(cpuid.data);
>> - free(msrs.data);
>> xc_cpu_policy_destroy(policy);
>>
>> return rc;
>> diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
>> index 4c4593528dfe..488f43378406 100644
>> --- a/tools/misc/xen-cpuid.c
>> +++ b/tools/misc/xen-cpuid.c
>> @@ -156,12 +156,18 @@ static void dump_info(xc_interface *xch, bool detail)
>>
>> free(fs);
>> }
>> -
>
> Stray (deleted) whitespace.
Ack
>
>> -static void print_policy(const char *name,
>> - xen_cpuid_leaf_t *leaves, uint32_t nr_leaves,
>> - xen_msr_entry_t *msrs, uint32_t nr_msrs)
>> +static void print_policy(xc_interface *xch, const char *name,
>> + const xc_cpu_policy_t *policy)
>> {
>> unsigned int l;
>> + const xen_cpuid_leaf_t *leaves;
>> + const xen_msr_entry_t *msrs;
>> + uint32_t nr_leaves, nr_msrs;
>> +
>> + if ( xc_cpu_policy_get_leaves(xch, policy, &leaves, &nr_leaves) )
>> + err(1, "xc_cpu_policy_get_leaves()");
>> + if ( xc_cpu_policy_get_msrs(xch, policy, &msrs, &nr_msrs) )
>> + err(1, "xc_cpu_policy_get_msrs()");
>
> Not an issue with here per say, but to drive home the main problem.
>
> This doesn't return the current leaves/msrs. It gives you whatever's
> stale in the staging buffer, which happens to be ok in xen-cpuid because
> it only ever reads a policy...
>
> ~Andrew
It's not stale if the deserialized policy is not dirty. The alternative
is a copyout (what happened before) which was way more brittle and
involved more hypercalls.
As I mentioned before, I you can think of a better scheme, I'm happy to
consider it.
Cheers,
Alejandro
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |