Re: [PATCH v10 14/50] crypto: ccp: Add support to initialize the AMD-SP for SEV-SNP

[Borislav Petkov]

On Mon, Dec 11, 2023 at 03:11:17PM -0600, Kalra, Ashish wrote:
> What we need is a mechanism to do legacy SEV/SEV-ES INIT only if a
> SEV/SEV-ES guest is being launched, hence, we want an additional parameter
> added to sev_platform_init() exported interface so that kvm_amd module can
> call this interface during guest launch and indicate if SNP/legacy guest is
> being launched.
> 
> That's the reason we want to add the probe parameter to
> sev_platform_init().

That's not what your original patch does and nowhere in the whole
patchset do I see this new requirement for KVM to be able to control the
probing.

The probe param is added to ___sev_platform_init_locked() which is
called by this new sev_platform_init_on_probe() thing to signal that
whatever calls this, it wants the probing.

And "whatever" is sev_pci_init() which is called from the bowels of the
secure processor drivers. Suffice it to say, this is some sort of an
init path.

So, it wants to init SNP stuff which is unconditional during driver init
- not when KVM starts guests - and probe too on driver init time, *iff*
that psp_init_on_probe thing is set. Which looks suspicious to me:

  "Add psp_init_on_probe module parameter that allows for skipping the
  PSP's SEV platform initialization during module init. User may decouple
  module init from PSP init due to use of the INIT_EX support in upcoming
  patch which allows for users to save PSP's internal state to file."

>From b64fa5fc9f44 ("crypto: ccp - Add psp_init_on_probe module
parameter").

And reading about INIT_EX, "This command loads the SEV related
persistent data from user-supplied data and initializes the platform
context."

So it sounds like HV vendor wants to supply something itself. But then
looking at init_ex_path and open_file_as_root() makes me cringe.
I would've never done it this way: we have request_firmware* etc helpers
for loading blobs from userspace which are widely used. But then reading 

  3d725965f836 ("crypto: ccp - Add SEV_INIT_EX support")

that increases the cringe factor even more because that also wants to
*write* into that file. Maybe there were good reasons to do it this way
- it is still yucky for my taste tho...

But I digress - whatever you want to do, the right approach is to split
the functionality:

SNP init
legacy SEV init

and to call them from a wrapper function around it which determines
which ones need to get called depending on that delayed probe thing.

Lumping everything together and handing a silly bool downwards is
already turning into a mess.

Now, looking at sev_guest_init() which calls sev_platform_init() and if
you want to pass back'n'forth more information than just that &error
pointer, then you can define your own struct sev_platform_init_info or
so which you preset before calling sev_platform_init() and pass in
a pointer to it.

And in it you can stick &error, bool probe or whatever else you need to
control what the platform needs to do upon init. And if you need to
extend that in the future, you can add new struct members and so on.

HTH.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette