On Fri, Nov 12, 2021 at 4:39 PM Brijesh Singh <brijesh.singh@xxxxxxx> wrote: > > > On 11/12/21 10:55 AM, Peter Gonda wrote: > > On Wed, Nov 10, 2021 at 8:32 AM Peter Gonda <pgonda@xxxxxxxxxx> wrote: > >> On Tue, Nov 9, 2021 at 3:20 PM Brijesh Singh <brijesh.singh@xxxxxxx> wrote: > >>> > >>> > >>> On 11/9/21 2:46 PM, Peter Gonda wrote: > >>>> On Tue, Nov 9, 2021 at 1:26 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > >>>>> On Tue, Nov 09, 2021, Peter Gonda wrote: > >>>>>> On Tue, Nov 9, 2021 at 10:21 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > >>>>>>> There's no need for this to be a function pointer, and the duplicate code can be > >>>>>>> consolidated. > >>>>>>> > >>>>>>> static int sev_do_init_locked(int cmd, void *data, int *error) > >>>>>>> { > >>>>>>> if (sev_es_tmr) { > >>>>>>> /* > >>>>>>> * Do not include the encryption mask on the physical > >>>>>>> * address of the TMR (firmware should clear it anyway). > >>>>>>> */ > >>>>>>> data.flags |= SEV_INIT_FLAGS_SEV_ES; > >>>>>>> data.tmr_address = __pa(sev_es_tmr); > >>>>>>> data.tmr_len = SEV_ES_TMR_SIZE; > >>>>>>> } > >>>>>>> return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error); > >>>>>>> } > >>>>>>> > >>>>>>> static int __sev_init_locked(int *error) > >>>>>>> { > >>>>>>> struct sev_data_init data; > >>>>>>> > >>>>>>> memset(&data, 0, sizeof(data)); > >>>>>>> return sev_do_init_locked(cmd, &data, error); > >>>>>>> } > >>>>>>> > >>>>>>> static int __sev_init_ex_locked(int *error) > >>>>>>> { > >>>>>>> struct sev_data_init_ex data; > >>>>>>> > >>>>>>> memset(&data, 0, sizeof(data)); > >>>>>>> data.length = sizeof(data); > >>>>>>> data.nv_address = __psp_pa(sev_init_ex_nv_address); > >>>>>>> data.nv_len = NV_LENGTH; > >>>>>>> return sev_do_init_locked(SEV_CMD_INIT_EX, &data, error); > >>>>>>> } > >>>>>> I am missing how this removes the duplication of the retry code, > >>>>>> parameter checking, and other error checking code.. With what you have > >>>>>> typed out I would assume I still need to function pointer between > >>>>>> __sev_init_ex_locked and __sev_init_locked. Can you please elaborate > >>>>>> here? > >>>>> Hmm. Ah, I got distracted between the original thought, the realization that > >>>>> the two commands used different structs, and typing up the above. > >>>>> > >>>>>> Also is there some reason the function pointer is not acceptable? > >>>>> It's not unacceptable, it would just be nice to avoid, assuming the alternative > >>>>> is cleaner. But I don't think any alternative is cleaner, since as you pointed > >>>>> out the above is a half-baked thought. > >>>> OK I'll leave as is. > >>>> > >>>>>>>> + rc = init_function(error); > >>>>>>>> if (rc && *error == SEV_RET_SECURE_DATA_INVALID) { > >>>>>>>> /* > >>>>>>>> * INIT command returned an integrity check failure > >>>>>>>> @@ -286,8 +423,8 @@ static int __sev_platform_init_locked(int *error) > >>>>>>>> * failed and persistent state has been erased. > >>>>>>>> * Retrying INIT command here should succeed. > >>>>>>>> */ > >>>>>>>> - dev_dbg(sev->dev, "SEV: retrying INIT command"); > >>>>>>>> - rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error); > >>>>>>>> + dev_notice(sev->dev, "SEV: retrying INIT command"); > >>>>>>>> + rc = init_function(error); > >>>>>>> The above comment says "persistent state has been erased", but __sev_do_cmd_locked() > >>>>>>> only writes back to the file if a relevant command was successful, which means > >>>>>>> that rereading the userspace file in __sev_init_ex_locked() will retry INIT_EX > >>>>>>> with the same garbage data. > >>>>>> Ack my mistake, that comment is stale. I will update it so its correct > >>>>>> for the INIT and INIT_EX flows. > >>>>>>> IMO, the behavior should be to read the file on load and then use the kernel buffer > >>>>>>> without ever reloading (unless this is built as a module and is unloaded and reloaded). > >>>>>>> The writeback then becomes opportunistic in the sense that if it fails for some reason, > >>>>>>> the kernel's internal state isn't blasted away. > >>>>>> One issue here is that the file read can fail on load so we use the > >>>>>> late retry to guarantee we can read the file. > >>>>> But why continue loading if reading the file fails on load? > >>>>> > >>>>>> The other point seems like preference. Users may wish to shutdown the PSP FW, > >>>>>> load a new file, and INIT_EX again with that new data. Why should we preclude > >>>>>> them from that functionality? > >>>>> I don't think we should preclude that functionality, but it needs to be explicitly > >>>>> tied to a userspace action, e.g. either on module load or on writing the param to > >>>>> change the path. If the latter is allowed, then it needs to be denied if the PSP > >>>>> is initialized, otherwise the kernel will be in a non-coherent state and AFAICT > >>>>> userspace will have a heck of a time even understanding what state has been used > >>>>> to initialize the PSP. > >>>> If this driver is builtin the filesystem will be unavailable during > >>>> __init. Using the existing retries already built into > >>>> sev_platform_init() also the file to be read once userspace is > >>>> running, meaning the file system is usable. As I tried to explain in > >>>> the commit message. We could remove the sev_platform_init call during > >>>> sev_pci_init since this only actually needs to be initialized when the > >>>> first command requiring it is issues (either reading some keys/certs > >>>> from the PSP or launching an SEV guest). Then userspace in both the > >>>> builtin and module usage would know running one of those commands > >>>> cause the file to be read for PSP usage. Tom any thoughts on this? > >>>> > >>> One thing to note is that if we do the INIT on the first command then > >>> the first guest launch will take a longer. The init command is not > >>> cheap (especially with the SNP, it may take a longer because it has to > >>> do all those RMP setup etc). IIRC, in my early SEV series in I was doing > >>> the INIT during the first command execution and based on the > >>> recommendation moved to do the init on probe. > >>> > >>> Should we add a module param to control whether to do INIT on probe or > >>> delay until the first command ? > >> Thats a good point Brijesh. I've only been testing this with SEV and > >> ES so haven't noticed that long setup time. I like the idea of a > >> module parameter to decide when to INIT, that should satisfy Sean's > >> concern that the user doesn't know when the INIT_EX file would be read > >> and that there is extra retry code (duplicated between sev_pci_init > >> and all the PSP commands). I'll get started on that. > > I need a little guidance on how to proceed with this. Should I have > > the new module parameter 'psp_init_on_probe' just disable PSP init on > > module init if false. Or should it also disable PSP init during > > command flow if it's true? > > > > I was thinking I should just have 'psp_init_on_probe' default to true, > > and if false it stops the PSP init during sev_pci_init(). If I add the > > second change that seems like it changes the ABI. Thoughts? > > > Good point that a module params may break the ABI. How about if we add a > new ioctl that can be used to initialize the SEV_INIT_EX. The ioctl > implementation will be similar to the PLATFORM_RESET; it will shutdown > the firmware then call INIT_EX. A platform provisioning tool may use ioctl. Would just a 'skip_psp_init_on_probe' parameter be simpler. We default to false but if users set it, we can skip that init attempt in sev_pci_init(). The init attempts on all other commands that require the INIT state would then provide users with INIT_EX functionality. They would also know exactly when INIT or INIT_EX would be attempted based on the parameter. Otherwise a new ioctl sounds reasonable. > > -Brijesh >