Re: [PATCH 12/16] x86/amd: Probe for NX support and re-activate if possible

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.01.2026 18:53, Andrew Cooper wrote:
> An AMD Ryzen system has been found with a firmware option to disable NX.  Like
> we do on Intel systems, try to detect and undo this stupidity.
> 
> Link: https://xcp-ng.org/forum/post/80714
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Julian Vetter <julian.vetter@xxxxxxxxxx>
> CC: Teddy Astie <teddy.astie@xxxxxxxxxx>
> 
> Somewhat RFC.  I don't particularly like the double handling of
> MSR_K8_EXT_FEATURE_MASK in this function, but I can't find any way of making
> the logic legible while trying to dedup the MSR accesses.

Looks reasonable to me as is. If later someone has a neat idea for 
de-duplication,
it can always be done incrementally.

> @@ -639,6 +640,40 @@ static void amd_early_adjust_cpuid_extd(void)
>              printk(XENLOG_INFO "CPU: Re-enabling disabled Topology 
> Extensions Support\n");
>          }
>      }
> +
> +    /*
> +     * Probe for NX support if it appears to be unavailable.  All 64-bit
> +     * capable AMD CPUs support it, but some firmwares have an option to hide
> +     * it in CPUID, apparently for "feature parity" with Intel platforms.
> +     *
> +     * Unlike Intel, there's no active indication that this has been done.  A
> +     * conversation with AMD suggests that if we can set EFER.NXE, then NX
> +     * will work.  Use this as a heuristic to probe for support, coping with
> +     * the fact that a hypervisor might have really disabled and blocked NX,
> +     * and not emulate the mask MSRs either.
> +     */
> +    if ( !boot_cpu_has(X86_FEATURE_NX) )
> +    {
> +        uint64_t *this_efer = &this_cpu(efer);
> +
> +        if ( wrmsr_safe(MSR_EFER, *this_efer | EFER_NXE) == 0 )

Would you mind using ! here, just like you do ...

> +        {
> +            *this_efer |= EFER_NXE;
> +
> +            if ( !rdmsr_safe(MSR_K8_EXT_FEATURE_MASK, &val) )

... here?

> +            {
> +                val |= 1ULL << cpufeat_bit(X86_FEATURE_NX);
> +                wrmsr_safe(MSR_K8_EXT_FEATURE_MASK, val);
> +                val = rdmsr(MSR_K8_EXT_FEATURE_MASK);
> +                if ( val & (1ULL << cpufeat_bit(X86_FEATURE_NX)) )
> +                {
> +                    __set_bit(X86_FEATURE_NX, c->x86_capability);

Again this shouldn't be needed, as identify_cpu() takes care. Unless in this
case it matters for the time in between?

Jan