On Thu, Nov 21, 2024, Ashish Kalra wrote:
> On 11/21/2024 11:42 AM, Sean Christopherson wrote:
> > On Thu, Nov 21, 2024, Tom Lendacky wrote:
> >> On 11/21/24 10:56, Sean Christopherson wrote:
> >>> On Thu, Nov 21, 2024, Ashish Kalra wrote:
> >>> Actually, IMO, the behavior of _sev_platform_init_locked() and pretty much all of
> >>> the APIs that invoke it are flawed, and make all of this way more confusing and
> >>> convoluted than it needs to be.
> >>>
> >>> IIUC, SNP initialization is forced during probe purely because SNP can't be
> >>> initialized if VMs are running. But the only in-tree user of SEV-XXX functionality
> >>> is KVM, and KVM depends on whatever this driver is called. So forcing SNP
> >>> initialization because a hypervisor could be running legacy VMs make no sense.
> >>> Just require KVM to initialize SEV functionality if KVM wants to use SEV+.
> >>
> >> When we say legacy VMs, that also means non-SEV VMs. So you can't have any
> >> VM running within a VMRUN instruction.
> >
> > Yeah, I know. But if KVM initializes the PSP SEV stuff when KVM is loaded, then
> > KVM can't possibly be running VMs of any kind.
> >
> >> Or...
> >>
> >>>
> >>> /*
> >>> * Legacy guests cannot be running while SNP_INIT(_EX) is executing,
> >>> * so perform SEV-SNP initialization at probe time.
> >>> */
> >>> rc = __sev_snp_init_locked(&args->error);
> >>>
> >>> Rather than automatically init SEV+ functionality, can we instead do something
> >>> like the (half-baked pseudo-patch) below? I.e. delete all paths that implicitly
> >>> init the PSP, and force KVM to explicitly initialize the PSP if KVM wants to use
> >>> SEV+. Then we can put the CipherText and SNP ASID params in KVM.
> >>
> >> ... do you mean at module load time (based on the module parameters)? Or
> >> when the first SEV VM is run? I would think the latter, as the parameters
> >> are all true by default. If the latter, that would present a problem of
> >> having to ensure no VMs are active while performing the SNP_INIT.
> >
> > kvm-amd.ko load time.
>
> Ok, so kvm module load will init SEV+ if indicated by it's module parameters.
>
> But, there are additional concerns here.
>
> SNP will still have to be initialized first, because SNP_INIT will fail if
> SEV INIT has been done.
>
> Additionally, to support SEV firmware hotloading (DLFW_EX), SEV can't be
> initialized.
>
> So probably, we will have to retain some PSP style SEV+ initialization here,
> SNP_INIT is always done first and then SEV INIT is skipped if explicitly
> specified by a module param. This allows SEV firmware hotloading to be
> supported.
>
> But, then with SEV firmware hotload support how do we do SEV INIT without
> unloading and reloading KVM module ?
So the above says:
SEV_CMD_SNP_INIT{_ES} cannot be executed if SEV_CMD_INIT{_EX} has been executed.
but the existing comment in _sev_platform_init_locked() says:
/*
* Legacy guests cannot be running while SNP_INIT(_EX) is executing,
* so perform SEV-SNP initialization at probe time.
*/
Which one is correct? I don't think it matters in the end, just trying to wrap my
head around everything.
And IIUC, SEV_CMD_SNP_INIT{_EX} can be executed before firmware hotload, but
SEV_CMD_INIT{_EX} cannot. Is that correct? Because if firmware hotload can't
be done while SEV VMs are _active_, then that's a very different situation.
> This can reuse the current support (in KVM) to do SEV INIT implicitly when
> the first SEV VM is run: sev_guest_init() -> sev_platform_init()
I don't love the implicit behavior, but assuming hotloading firmware can't be done
after SEV_CMD_INIT{_EX}, that does seem like the least awful solution.
To summarize, if the above assumptions hold:
1. Initialize SNP when kvm-amd.ko is loaded.
2. Define CipherTextHiding and ASID params kvm-amd.ko.
3. Initialize SEV+ at first use.
Just to triple check: that will allow firmware hotloading even if kvm-amd.ko is
built-in, correct? I.e. doesn't requires deferring kvm-amd.ko load until after
firmware hotloading.
