Re: [PATCH v2 3/3] x86/sev: Add SEV-SNP CipherTextHiding support

[Kalra, Ashish]

Hello Sean,

On 12/9/2024 7:30 PM, Sean Christopherson wrote:
> On Fri, Dec 06, 2024, Ashish Kalra wrote:
>> On 12/6/2024 4:30 PM, Sean Christopherson wrote:
>>>> 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.
>>
>> Yes, the above summary is correct except for (3).
> 
> Heh, that wasn't a statement of fast, it was a suggestion for a possible
> implementation.
> 
>> The initial set of patches will initialize SNP and SEV both at kvm-amd.ko module load,
>> similar to PSP module load/probe time.
> 
> Why?  If SEV+ is initialized at kvm-amd.ko load, doesn't that prevent firmware
> hotloading?

Yes it does, i was thinking of fixing it as part of a series on top of these patches
to support SEV firmware hotloading.

> 
>> For backward compatibility, the PSP module parameter psp_init_on_probe will still be
>> supported, i believe it is used for INIT_EX support.
> 
> Again, why?  If the only use of psp_init_on_probe is to _disable_ that behavior,
> and we make the code never init-on-probe, then the param is unnecessary, no?

Yes, it makes sense to remove this param.

> 
>>> 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.
>>
>> Yes, this should work, for supporting firmware hotloading, the PSP driver's
>> psp_init_on_probe parameter will need to be set to false, which will ensure
>> that SEV INIT is not done during SEV/SNP platform initialization at KVM module
>> probe time and instead it will be done implicitly at first SEV/SEV-ES VM launch.
> 
> Please no.  I really, really don't want gunk like this in KVM:
> 
> 	init_args.probe = false;
> 	ret = sev_platform_init(&init_args);
> 
> That's inscrutable without a verbose comment, and all kinds of ugly.  Why can't
> we simply separate SNP initialization from SEV+ initialization?

Yes we can do that, by default KVM module load time will only do SNP initialization,
and then we will do SEV initialization if a SEV VM is being launched.

This will remove the probe parameter from init_args above, but will need to add another
parameter like VM type to specify if SNP or SEV initialization is to be performed with
the sev_platform_init() call.

Will work on re-posting my series with SNP initialization separated from SEV
initialization.

Thanks,
Ashish