Re: [PATCH v2 0/9] Move initializing SEV/SNP functionality to KVM

["Kalra, Ashish" <ashish.kalra@amd.com>]

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@amd.com> wrote:
>>>>>>
>>>>>> From: Ashish Kalra <ashish.kalra@amd.com>
>>>>>
>>>>>> 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.

Thanks,
Ashish