Re: [PATCH v4 02/23] x86/cpu: report SMX, TXT and SKINIT capabilities

[Jan Beulich <jbeulich@suse.com>]

On 20.08.2026 15:14, Sergii Dmytruk wrote:
> On Tue, Aug 18, 2026 at 02:08:48PM +0200, Jan Beulich wrote:
>> On 02.08.2026 15:09, Sergii Dmytruk wrote:
>>> From: Michał Żygowski <michal.zygowski@3mdeb.com>
>>>
>>> Report TXT capabilities so that dom0 can query the Intel TXT or AMD
>>> SKINIT support information using xl dmesg.
>>
>> Hmm. I first meant to ask: In how far is this extra logging useful,
>> especially as long as we don't use the features just yet? And only then
>> I noticed that I must have paid too little attention here already in v3.
>> Querying through "xl dmesg" is entirely unreliable. Sooner or later the
>> boot messages will scroll off of the ring buffer. Making this a
>> query-able interface also would mean we can't alter any of the messages,
>> should the want/need arise.
>>
>> For AMD the situation is easy: It's part of the featureset / CPU policy
>> exposed via sysctl. The same is true for SMX on Intel, but the further
>> GETSEC output requires some other means to communicate. I wonder whether
>> making this part of the CPU policy would make sense, or whether to
>> introduce a Dom0-only hypervisor-CPUID bit for it, or whether yet
>> something else would be best here. Likely Andrew will have had thoughts
>> on this long before ...
> 
> I'm not aware of anything relying on this output.  It's just for making
> this information more accessible to users that may be wondering if DRTM
> has a chance of working (e.g., if hardware supports it and firmware is
> properly configured).  The wording may be unfortunate (and needs a fix
> anyway), I can change it to
> 
>     Report DRTM-related capabilities to enable checking for them in dom0
>     using `xl dmesg`.  This targets debug and diagnostic use cases.
> 
> if that helps.

Yes, please. Provided we need this separate output at all. Furthermore,
if we need it, wouldn't it better be adjacent with other extended VT-x /
SVM features?

>>> @@ -620,6 +625,49 @@ static void init_intel_perf(struct cpuinfo_x86 *c)
>>>      }
>>>  }
>>>
>>> +/*
>>> + * Print out the SMX and TXT capabilties, so that dom0 can determine if the
>>> + * system is DRTM-capable.
>>> + */
>>> +static void intel_log_smx_txt(void)
>>> +{
>>> +    unsigned long cr4_val, getsec_caps;
>>> +
>>> +    /*
>>> +     * Run only on BSP and not during resume to report the capability only once.
>>> +     */
>>> +    if ( system_state == SYS_STATE_resume || smp_processor_id() )
>>> +        return;
>>> +
>>> +    printk("CPU: SMX capability ");
>>> +    if ( !test_bit(X86_FEATURE_SMX, &boot_cpu_data.x86_capability) )
>>> +    {
>>> +        printk("not supported\n");
>>> +        return;
>>> +    }
>>> +    printk("supported\n");
>>> +
>>> +    /* Can't run GETSEC without VMX and SMX */
>>> +    if ( !test_bit(X86_FEATURE_VMX, &boot_cpu_data.x86_capability) )
>>> +        return;
>>> +
>>> +    cr4_val = read_cr4();
>>> +    if ( !(cr4_val & X86_CR4_SMXE) )
>>> +        write_cr4(cr4_val | X86_CR4_SMXE);
>>> +
>>> +    asm volatile ("getsec\n"
>>> +        : "=a" (getsec_caps)
>>> +        : "a" (GETSEC_CAPABILITIES), "b" (0) :);
>>
>> Nit (style): Bad indentation, missing blanks, unnecessary \n, and stray colon.
>> Overall:
>>
>>     asm volatile ( "getsec"
>>                    : "=a" (getsec_caps)
>>                    : "a" (GETSEC_CAPABILITIES), "b" (0) );
>>
>> I further question the need for volatile here. (Like for we have for CPUID, we
>> anyway may want to gain a getsec() wrapper for GETSEC.)
> 
> I think `volatile` was added just because it doesn't hurt, rather than
> because it's necessary, so it can be dropped.  Can add a wrapper, but
> there is only one use so far and a generic wrapper will have to use
> 64-bit parameters (`GETSEC[EXITAC]` sets RBX).

Well, if it'll remain just one use, maybe indeed too early for having a
wrapper.

>>> +    if ( !(cr4_val & X86_CR4_SMXE) )
>>> +        write_cr4(cr4_val & ~X86_CR4_SMXE);
>>
>> The clearing of SMXE here is pointless, as the if() already guarantees the bit
>> to be clear.
> 
> This statement restores the value stored in `cr4_val` (see above).
> Maybe should name the variable `old_cr4_val` or `orig_cr4_val`.

Naming wasn't my point here. My point was that masking off a bit that's
already off is pretty clearly useless code.

Jan