From: Alejandro Vallejo <alejandro.vallejo@cloud.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Xen-devel <xen-devel@lists.xenproject.org>,
Anthony PERARD <anthony@xenproject.org>,
Juergen Gross <jgross@suse.com>
Subject: Re: [PATCH v2 2/2] tools/xg: Clean up xend-style overrides for CPU policies
Date: Wed, 22 May 2024 16:15:09 +0100 [thread overview]
Message-ID: <974bfbc6-97f6-4afa-978c-562448042d4a@cloud.com> (raw)
In-Reply-To: <Zktl8zRt1kue5vA6@macbook>
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
next prev parent reply other threads:[~2024-05-22 15:15 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-17 16:08 [PATCH v2 0/3] Clean the policy manipulation path in domain creation Alejandro Vallejo
2024-05-17 16:08 ` [PATCH v2 1/2] tools/xg: Streamline cpu policy serialise/deserialise calls Alejandro Vallejo
2024-05-20 13:47 ` Roger Pau Monné
2024-05-22 15:14 ` Alejandro Vallejo
2024-05-17 16:08 ` [PATCH v2 2/2] tools/xg: Clean up xend-style overrides for CPU policies Alejandro Vallejo
2024-05-20 15:02 ` Roger Pau Monné
2024-05-22 15:15 ` Alejandro Vallejo [this message]
2024-05-20 12:45 ` [PATCH v2 0/3] Clean the policy manipulation path in domain creation Roger Pau Monné
2024-05-22 15:34 ` Alejandro Vallejo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=974bfbc6-97f6-4afa-978c-562448042d4a@cloud.com \
--to=alejandro.vallejo@cloud.com \
--cc=anthony@xenproject.org \
--cc=jgross@suse.com \
--cc=roger.pau@citrix.com \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).