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

[Sergii Dmytruk <sergii.dmytruk@xxxxxxxxx>]

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

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.

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

OK, thanks.

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

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

Regards