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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.08.2026 15:09, Sergii Dmytruk wrote:
> From: Michał Żygowski <michal.zygowski@xxxxxxxxx>
> 
> 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 ...

Irrespective of the above also a few comments on the patch itself.

> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -617,6 +617,21 @@ void amd_process_freq(const struct cpuinfo_x86 *c,
>               *low_mhz = amd_parse_freq(c->family, lo);
>  }
>  
> +void amd_log_skinit(const struct cpuinfo_x86 *c)
> +{
> +    /*
> +     * Run only on BSP and not during resume to report the capability only 
> once.
> +     */
> +    if ( system_state == SYS_STATE_resume || smp_processor_id() )
> +        return;

If this is BSP-on-boot only, the function really wants to be __init. For that,
...

> +    printk("CPU: SKINIT capability ");
> +    if ( !test_bit(X86_FEATURE_SKINIT, &boot_cpu_data.x86_capability) )
> +        printk("not supported\n");
> +    else
> +        printk("supported\n");
> +}
> +
>  void cf_check early_init_amd(struct cpuinfo_x86 *c)
>  {
>       if (c == &boot_cpu_data)

... use this condition ...

> @@ -1325,6 +1340,7 @@ static void cf_check init_amd(struct cpuinfo_x86 *c)
>               setup_force_cpu_cap(X86_FEATURE_XEN_REP_MOVSB);
>  
>       amd_log_freq(c);
> +     amd_log_skinit(c);

... at the call site (and of course also the other one). Same for the Intel
code, obviously.

> @@ -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.)

> +    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.

Jan