On Thu, Dec 19, 2024 at 2:04 PM Kalra, Ashish <ashish.kalra@xxxxxxx> wrote:
>
>
>
> On 12/18/2024 7:11 PM, Kalra, Ashish wrote:
> >
> > On 12/18/2024 1:10 PM, Sean Christopherson wrote:
> >> On Tue, Dec 17, 2024, Ashish Kalra wrote:
> >>> On 12/17/2024 3:37 PM, Sean Christopherson wrote:
> >>>> On Tue, Dec 17, 2024, Ashish Kalra wrote:
> >>>>> On 12/17/2024 10:00 AM, Dionna Amalie Glaze wrote:
> >>>>>> On Mon, Dec 16, 2024 at 3:57 PM Ashish Kalra <Ashish.Kalra@xxxxxxx> wrote:
> >>>>>>>
> >>>>>>> From: Ashish Kalra <ashish.kalra@xxxxxxx>
> >>>>>>
> >>>>>>> The on-demand SEV initialization support requires a fix in QEMU to
> >>>>>>> remove check for SEV initialization to be done prior to launching
> >>>>>>> SEV/SEV-ES VMs.
> >>>>>>> NOTE: With the above fix for QEMU, older QEMU versions will be broken
> >>>>>>> with respect to launching SEV/SEV-ES VMs with the newer kernel/KVM as
> >>>>>>> older QEMU versions require SEV initialization to be done before
> >>>>>>> launching SEV/SEV-ES VMs.
> >>>>>>>
> >>>>>>
> >>>>>> I don't think this is okay. I think you need to introduce a KVM
> >>>>>> capability to switch over to the new way of initializing SEV VMs and
> >>>>>> deprecate the old way so it doesn't need to be supported for any new
> >>>>>> additions to the interface.
> >>>>>>
> >>>>>
> >>>>> But that means KVM will need to support both mechanisms of doing SEV
> >>>>> initialization - during KVM module load time and the deferred/lazy
> >>>>> (on-demand) SEV INIT during VM launch.
> >>>>
> >>>> What's the QEMU change? Dionna is right, we can't break userspace, but maybe
> >>>> there's an alternative to supporting both models.
> >>>
> >>> Here is the QEMU fix : (makes a SEV PLATFORM STATUS firmware call via PSP
> >>> driver ioctl to check if SEV is in INIT state)
> >>>
> >>> diff --git a/target/i386/sev.c b/target/i386/sev.c
> >>> index 1a4eb1ada6..4fa8665395 100644
> >>> --- a/target/i386/sev.c
> >>> +++ b/target/i386/sev.c
> >>> @@ -1503,15 +1503,6 @@ static int sev_common_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
> >>> }
> >>> }
> >>>
> >>> - if (sev_es_enabled() && !sev_snp_enabled()) {
> >>> - if (!(status.flags & SEV_STATUS_FLAGS_CONFIG_ES)) {
> >>> - error_setg(errp, "%s: guest policy requires SEV-ES, but "
> >>> - "host SEV-ES support unavailable",
> >>> - __func__);
> >>> - return -1;
> >>> - }
> >>> - }
> >>
> >> Aside from breaking userspace, removing a sanity check is not a "fix".
> >
> > Actually this sanity check is not really required, if SEV INIT is not done before
> > launching a SEV/SEV-ES VM, then LAUNCH_START will fail with invalid platform state
> > error as below:
> >
> > ...
> > qemu-system-x86_64: sev_launch_start: LAUNCH_START ret=1 fw_error=1 'Platform state is invalid'
> > ...
> >
> > So we can safely remove this check without causing a SEV/SEV-ES VM to blow up or something.
> >
> >>
> >> Can't we simply have the kernel do __sev_platform_init_locked() on-demand for
> >> SEV_PLATFORM_STATUS? The goal with lazy initialization is defer initialization
> >> until it's necessary so that userspace can do firmware updates. And it's quite
> >> clearly necessary in this case, so...
> >
> > I don't think we want to do that, probably want to return "raw" status back to userspace,
> > if SEV INIT has not been done we probably need to return back that status, otherwise
> > it may break some other userspace tool.
> >
> > Now, looking at this qemu check we will always have issues launching SEV/SEV-ES VMs
> > with SEV INIT on demand as this check enforces SEV INIT to be done before launching
> > the VMs. And then this causes issues with SEV firmware hotloading as the check
> > enforces SEV INIT before launching VMs and once SEV INIT is done we can't do
> > firmware hotloading.
> >
> > But, i believe there is another alternative approach :
> >
> > - PSP driver can call SEV Shutdown right before calling DLFW_EX and then do
> > a SEV INIT after successful DLFW_EX, in other words, we wrap DLFW_EX with
> > SEV_SHUTDOWN prior to it and SEV INIT post it. This approach will also allow
> > us to do both SNP and SEV INIT at KVM module load time, there is no need to
> > do SEV INIT lazily or on demand before SEV/SEV-ES VM launch.
> >
> > This approach should work without any changes in qemu and also allow
> > SEV firmware hotloading without having any concerns about SEV INIT state.
> >
>
> And to add here that SEV Shutdown will succeed with active SEV and SNP guests.
>
> SEV Shutdown (internally) marks all SEV asids as invalid and decommission all
> SEV guests and does not affect SNP guests.
>
> So any active SEV guests will be implicitly shutdown and SNP guests will not be
> affected after SEV Shutdown right before doing SEV firmware hotloading and
> calling DLFW_EX command.
>
Please don't implicitly shut down VMs. At least have a safe and unsafe
option for dlfw_ex where the default is to not destroy active
workloads.
That's why the 2022 patch series for Intel SGX EUPDATESVN on microcode
hotload was shot down.
It's very rude to destroy running workloads because a system update
was scheduled.
> It should be fine to expect that there are no active SEV guests or any active
> SEV guests will be shutdown as part of SEV firmware hotloading while keeping
> SNP guests running.
>
> Thanks,
> Ashish
--
-Dionna Glaze, PhD, CISSP, CCSP (she/her)
