Re: [PATCH v9 3/6] x86/sev: Disable CPU hotplug while SNP is active

[K Prateek Nayak <kprateek.nayak@amd.com>]

Hello Ashish,

On 6/26/2026 8:08 AM, Kalra, Ashish wrote:
>> Looking at snp_prepare(), we have an early-bailout for
>>
>>     rdmsrq(MSR_AMD64_SYSCFG, val);
>>     if (val & MSR_AMD64_SYSCFG_SNP_EN)
>>          return;
>>
>> Does executing SHUTDOWN command lead to the firmware clearing SNP_EN in
>> SYSCFG on all CPUS?
> 
> Yes, in case of X86_SNP_SHUTDOWN (available if firmware supports X86SnpShutdown feature)
> SNP is disabled on all cores by clearing SYSCFG[SNPEn] bit.
> 
> If X86_SNP_SHUTDOWN is set to 1, the firmware clears the SYSCFG[SNPEn] bit in each core. 
> 
> But, in case of legacy SNP shutdown, SNP_EN bit is not cleared and so SNP remains enabled.

Ah! That was the bit I was missing. Thanks a ton for clarifying.

> 
>>
>> If SNP_EN remains set (and Linux can't clear it since it is
>> "Write-1-only" bit), then a subsequent snp_prepare() will skip setting
>> SYSCFG if it sees SNP_EN on local CPU.
>>
>> It can so happen that we enable hotlpug at shutdown, CPUs come online
>> without setting SNP_EN in SYSCFG, subsequent snp_prepare() runs on a CPU
>> where SNP_EN is still set and skips configuring it for the CPUs that
>> don't have it set, and we'll be in a pickle still.
>>
>> The comment above that bailout saying "this can happen in case of kexec
>> boot" makes me believe that SNP_EN remains set until a full system
>> reset.
>>
>> The only safe way to do this is to ensure all possible CPUs are online
>> during snp_prepare() and do snp_enable() regardless of whether local CPU
>> has SNP_EN or not.
>>
>> Am I missing something?
>>
> 
> The piece that makes the early bailout safe is the disable this patch adds:
> hotplug is disabled while SNP is active, so the online set can't change under an
> active SNP. snp_prepare() already requires online == present, so at a successful
> init every present CPU gets SNP_EN,

How is this enforced? AFAICT, on_each_cpu(snp_enable) will only covers
the online CPUs and there could be CPUs that have been offlined before
that right?

> and because hotplug is then disabled none
> can leave or rejoin without it. So whenever the bailout is hit with SNP active,
> every online CPU already has SNP_EN:
> 
>   - kexec: SNP_EN is already set on all CPUs by the previous kernel.

There is a catch here: you can have offline CPUs during the previous boot
(say you have maxcpus=8 in your cmdline), and then you kexec with a different
kernel / cmdline that brings online a bunch more CPUs.

SNP_EN will only be set for a subset of then with the legacy SNP_INIT and
if snp_prepare() runs on those legacy CPUs, you still skip setting it for
the ones that don't have SNP_EN set.

Is that case covered somehow or is it a non-issue?

>   - re-init while SNP is still active (e.g. after a legacy SNP_SHUTDOWN that
>   leaves SNP_EN set): hotplug was disabled the whole time, so the online set is
>   unchanged and all of them still have SNP_EN.
> 
> The only way a CPU can be online without SNP_EN is when SNP is not active --
> i.e. after an SNP_INIT failure, where this patch re-enables hotplug. That is
> deliberately the same as the behavior before this support existed (hotplug was
> never disabled then), and it is benign: SNP_EN only gates RMP checks, the RMP
> itself is initialized by SNP_INIT, so on a failed init the RMP is all-zeroes --
> every entry is in the default HV-owned state, no page is assigned, no check ever blocks
> and snp_initialized stays false, so no SNP guest can be created.
> Nothing is enforced and nothing is protected.
> 
> So I've kept snp_prepare()'s existing bailout / snp_enable() behavior unchanged;
> what this patch adds is disabling hotplug while SNP is active, which is what
> actually closes the window (a CPU coming online without SNP_EN while SNP is
> live). That window -- and the SNP_EN-stays-set-on-failure situation -- already
> exist in today's code, this patch constrains the dangerous (active) case and
> otherwise matches current behavior.

Ack! Just that one small bit up above bothers me but other than that,
doing it in snp_prepare() should be good.

This is all new to me so thanks a ton for answering my queries.

> 
> (On the v9 placement specifically: I'm moving the disable into snp_prepare()
> ahead of SNP_EN in the next version; in v9 it sits after SNP_INIT, which leaves
> the window you originally pointed out.)
-- 
Thanks and Regards,
Prateek