Re: [PATCH 01/16] x86/cpu: Fix boot time cache flushing

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.01.2026 18:53, Andrew Cooper wrote:
> On 27/01/2026 11:35 am, Jan Beulich wrote:
>> On 27.01.2026 12:08, Andrew Cooper wrote:
>>> On 27/01/2026 10:37 am, Jan Beulich wrote:
>>>> On 26.01.2026 18:53, Andrew Cooper wrote:
>>>>> --- a/xen/arch/x86/cpu/common.c
>>>>> +++ b/xen/arch/x86/cpu/common.c
>>>>> @@ -319,8 +319,6 @@ void __init early_cpu_init(bool verbose)
>>>>>   uint64_t val;
>>>>>   u32 eax, ebx, ecx, edx;
>>>>>  
>>>>> - c->x86_cache_alignment = 32;
>>>>> -
>>>>>   /* Get vendor name */
>>>>>   cpuid(0x00000000, &c->cpuid_level, &ebx, &ecx, &edx);
>>>>>   *(u32 *)&c->x86_vendor_id[0] = ebx;
>>>>> @@ -352,6 +350,7 @@ void __init early_cpu_init(bool verbose)
>>>>>   if (edx & cpufeat_mask(X86_FEATURE_CLFLUSH)) {
>>>>>           unsigned int size = ((ebx >> 8) & 0xff) * 8;
>>>>>  
>>>>> +         c->x86_clflush_size = size;
>>>>>           c->x86_cache_alignment = size;
>>>> With this change, can't the writing of the field in generic_identify()
>>>> go away? CPU_DATA_INIT() in particular doesn't invalidate it.
>>> No, it can't.  The value needs setting up on every AP, right now at least.
>> Are you sure? APs inherit part of the BSP's data (initialize_cpu_data()),
>> and reset_cpuinfo() doesn't clear ->x86_clflush_size afaics.
> 
> Every time I look at that, it gets more insane.
> 
> For every CPU, initialize_cpu_data() clobbers boot_cpu_data, *then*
> copies the result into cpu_data[] array.
> 
> This cannot possibly be correct.  Why on earth did I ack it?

I wonder what you're looking at. My initialize_cpu_data() has

    struct cpuinfo_x86 c = boot_cpu_data;

which means a copy is being made, the address of which is then handed
to reset_cpuinfo().

>>>> Tangentially, "cpuid=no-clflush" didn't have any effect on any of this so
>>>> far, and also isn't going to have with the changes you make.
>>> The line immediately out of context above will applies the clear cap
>>> mask, so will cause cpuid=no-clflush to take effect.
>> This concerns me. With your change, "cpuid=no-clflush" will lead to an
>> unconditional panic() then.
> 
> So will no-cmpxchg8b.

Which doesn't make the situation any better. (I think you mean no-cmpxchg16b
though?)

>> Whereas previously, with cleared_caps[] being
>> applied by identify_cpu() only after generic_identify() has already
>> evaluated the CLFLUSH bit, there was no effect at all.
> 
> That wasn't no effect.  The effect (upon request of an impossible thing)
> would be that part of Xen would have ignored the request and functioned,
> but another part of Xen would have propagated that to guests, which will
> probably have equally rude things to say.

Well, I thought it was clear from context that I meant "no effect for Xen
itself". As to guests - as long as they're properly checking CPUID bits
and refrain from using insns which CPUID says aren't available, I don't
see why they should get upset.

When knowing one may run virtualized, the concept of "I know one feature
(e.g. LM) implies another (e.g. CLFLUSH)" is flawed. Any combination of
features can be surfaced, so long as true dependencies between them are
respected. IOW I disagree with "cpuid=no-clflush" requesting an impossible
thing. "cpuid=no-lm", otoh, does for a 64-bit target environment.

>> I don't think this panic()ing is desirable, but as an absolute minimum this
>> (drastic) change in behavior would want calling out in the description.
>>
>> Further, if the panic() was to stay, there's no point having cpu_has_clflush
>> evaluate to anything other than constant true anymore.
> 
> I'm not overly interested in users complaining about a panic() if they
> ask for an impossible thing.  Better that than the prior behaviour we had.
> 
> Talking of other impossible things, cpuid=no-$foo does nothing for
> FPU/DE/PSE/PGE or MMX which are the features hard wired to 1 already,
> and with 0 users in the tree.

Indeed, and while there is that "Currently accepted:" section in the doc,
I can't help thinking that even for the speculation control aspect that
it explicitly names it has already gone stale. Yes, in the past we said
we'd mean to not support use of arbitrary forms of this option, yet

"Unless otherwise noted, options only have any effect in their negative form,
 to hide the named feature(s).  Ignoring a feature using this mechanism will
 cause Xen not to use the feature, nor offer them as usable to guests."

to me really says otherwise. Even if intended to be thus restricted, it
would then feel rather odd that we implement support for an option with
hundreds of sub-options, out of which only a handful are supposed to be
possibly used.

On concrete example where a presently not explicitly permitted form
could be useful to people is "no-rdseed" on AMD hardware affected by
one of the two known issues (patches sadly still only pending). This
viable mitigation would be unsupported by your implied interpretation.

Jan