Messages in this thread | | | Subject | Re: [PATCH V3 4/4] crypto: ccp - Add SEV_INIT_EX support | From | Brijesh Singh <> | Date | Tue, 9 Nov 2021 16:19:57 -0600 |
| |
On 11/9/21 2:46 PM, Peter Gonda wrote: > On Tue, Nov 9, 2021 at 1:26 PM Sean Christopherson <seanjc@google.com> wrote: >> >> On Tue, Nov 09, 2021, Peter Gonda wrote: >>> On Tue, Nov 9, 2021 at 10:21 AM Sean Christopherson <seanjc@google.com> 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 ?
-Brijesh
| |