From: Alejandro Vallejo <alejandro.vallejo@cloud.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Andrew Cooper" <andrew.cooper3@citrix.com>,
"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
"Anthony PERARD" <anthony.perard@citrix.com>,
Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 5/6] xen/x86: Derive topologically correct x2APIC IDs from the policy
Date: Thu, 2 May 2024 15:44:26 +0100 [thread overview]
Message-ID: <adcc0c16-6f02-454e-8a2c-ae00a6cbebd2@cloud.com> (raw)
In-Reply-To: <4a280eab-81f7-4a87-b531-3633311a4c4a@suse.com>
On 02/05/2024 07:57, Jan Beulich wrote:
> On 02.05.2024 08:55, Jan Beulich wrote:
>> On 01.05.2024 18:35, Alejandro Vallejo wrote:
>>> Hi,
>>>
>>> On 26/03/2024 16:41, Jan Beulich wrote:
>>>> On 09.01.2024 16:38, Alejandro Vallejo wrote:
>>>>> --- a/xen/lib/x86/policy.c
>>>>> +++ b/xen/lib/x86/policy.c
>>>>> @@ -2,15 +2,78 @@
>>>>>
>>>>> #include <xen/lib/x86/cpu-policy.h>
>>>>>
>>>>> -uint32_t x86_x2apic_id_from_vcpu_id(const struct cpu_policy *p, uint32_t vcpu_id)
>>>>> +static uint32_t parts_per_higher_scoped_level(const struct cpu_policy *p, size_t lvl)
>>>>> {
>>>>> /*
>>>>> - * TODO: Derive x2APIC ID from the topology information inside `p`
>>>>> - * rather than from vCPU ID. This bodge is a temporary measure
>>>>> - * until all infra is in place to retrieve or derive the initial
>>>>> - * x2APIC ID from migrated domains.
>>>>> + * `nr_logical` reported by Intel is the number of THREADS contained in
>>>>> + * the next topological scope. For example, assuming a system with 2
>>>>> + * threads/core and 3 cores/module in a fully symmetric topology,
>>>>> + * `nr_logical` at the core level will report 6. Because it's reporting
>>>>> + * the number of threads in a module.
>>>>> + *
>>>>> + * On AMD/Hygon, nr_logical is already normalized by the higher scoped
>>>>> + * level (cores/complex, etc) so we can return it as-is.
>>>>> */
>>>>> - return vcpu_id * 2;
>>>>> + if ( p->x86_vendor != X86_VENDOR_INTEL || !lvl )
>>>>> + return p->topo.subleaf[lvl].nr_logical;
>>>>
>>>> Is "!= Intel" really appropriate here? I'd rather see this being "AMD || Hygon".
>>>
>>> Sure, I don't particularly mind, but why? As far as we know only Intel
>>> has this interpretation for the part counts. I definitely haven't seen
>>> any non-Intel CPUID dump in which the part count is the total number of
>>> threads (Centaur/Zhaoxin are not multithreaded, and don't expose leaves
>>> 1f or e26, as far as I could see).
>>
>> Because of x86'es origin and perhaps other historical aspects, cloning
>> Intel behavior is far more likely.
That claim doesn't hold very well seeing how...
>> The fact that Hygon matches AMD is
>> simply because they took AMD's design wholesale.
... this statement contradicts it. We can't predict which new vendor (if
any) will be cloned/mimicked next, so that's not a very plausible reason
to prioritise a specific vendor in conditionals.
It remains to be seen what a Zhaoxin actually looks like, because I
couldn't get ahold of a complete cpuid dump.
>
> Perhaps: See how many dead ends AMD have created, i.e. stuff they proudly
> introduced into the architecture, but then gave up again (presumably for
> diverging too far from Intel, and hence lacking long term acceptance):
> 3DNow!, LWP, and XOP just to name those that come to mind right away.
>
> Jan
I can't say I agree on the cause; Regardless I'd rather not discuss the
relative merits of vendors with regards to backwards compatibility, as
that's besides the point. The point is whether there's a credible
technical reason to prefer this...
if ( !(a & (B | C)) )
foo();
... to this...
if ( a == A )
foo();
..., as is the case in patch6.
I argue there's not, and in fact legibility-wise the latter is very
clearly superior.
There's also a compelling reason to keep the check coherent on both
generators to avoid bad surprises down the line.
Cheers,
Alejandro
next prev parent reply other threads:[~2024-05-02 14:44 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-09 15:38 [PATCH 0/6] x86: Expose consistent topology to guests Alejandro Vallejo
2024-01-09 15:38 ` [PATCH 1/6] xen/x86: Add initial x2APIC ID to the per-vLAPIC save area Alejandro Vallejo
2024-03-19 16:20 ` Roger Pau Monné
2024-03-20 9:33 ` Roger Pau Monné
2024-03-25 16:56 ` Alejandro Vallejo
2024-03-25 15:44 ` Alejandro Vallejo
2024-03-25 16:45 ` Jan Beulich
2024-03-25 18:00 ` Alejandro Vallejo
2024-03-26 7:16 ` Jan Beulich
2024-01-09 15:38 ` [PATCH 2/6] tools/xc: Add xc_cpu_policy to the public xenctrl.h header Alejandro Vallejo
2024-03-19 17:55 ` Roger Pau Monné
2024-03-25 18:19 ` Alejandro Vallejo
2024-01-09 15:38 ` [PATCH 3/6] xen/x86: Refactor xen/lib/x86 so it can be linked in hvmloader Alejandro Vallejo
2024-03-25 16:55 ` Jan Beulich
2024-01-09 15:38 ` [PATCH 4/6] tools/hvmloader: Use cpu_policy to determine APIC IDs Alejandro Vallejo
2024-03-20 8:52 ` Roger Pau Monné
2024-03-25 17:00 ` Jan Beulich
2024-01-09 15:38 ` [PATCH 5/6] xen/x86: Derive topologically correct x2APIC IDs from the policy Alejandro Vallejo
2024-03-20 10:15 ` Roger Pau Monné
2024-05-01 16:05 ` Alejandro Vallejo
2024-03-26 16:41 ` Jan Beulich
2024-05-01 16:35 ` Alejandro Vallejo
2024-05-02 6:55 ` Jan Beulich
2024-05-02 6:57 ` Jan Beulich
2024-05-02 14:44 ` Alejandro Vallejo [this message]
2024-01-09 15:38 ` [PATCH 6/6] xen/x86: Add topology generator Alejandro Vallejo
2024-01-10 14:16 ` Alejandro Vallejo
2024-01-10 14:28 ` Jan Beulich
2024-03-20 10:29 ` Roger Pau Monné
2024-03-26 17:02 ` Jan Beulich
2024-05-01 17:06 ` Alejandro Vallejo
2024-05-02 7:13 ` Jan Beulich
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=adcc0c16-6f02-454e-8a2c-ae00a6cbebd2@cloud.com \
--to=alejandro.vallejo@cloud.com \
--cc=andrew.cooper3@citrix.com \
--cc=anthony.perard@citrix.com \
--cc=jbeulich@suse.com \
--cc=roger.pau@citrix.com \
--cc=wl@xen.org \
--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).