On Wed, Aug 10, 2022 at 1:28 PM Tom Lendacky <thomas.lendacky@xxxxxxx> wrote: > > On 8/2/22 13:55, Jacky Li wrote: > > Currently the OS fails the PSP initialization when the file specified at > > 'init_ex_path' does not exist or has invalid content. However the SEV > > spec just requires users to allocate 32KB of 0xFF in the file, which can > > be taken care of by the OS easily. > > > > To improve the robustness during the PSP init, leverage the retry > > mechanism and continue the init process: > > > > During the first INIT_EX call, if the content is invalid or missing, > > continue the process by feeding those contents into PSP instead of > > aborting. PSP will then override it with 32KB 0xFF and return > > SEV_RET_SECURE_DATA_INVALID status code. In the second INIT_EX call, > > this 32KB 0xFF content will then be fed and PSP will write the valid > > data to the file. > > > > In order to do this, it's also needed to skip the sev_read_init_ex_file > > in the second INIT_EX call to prevent the file content from being > > overwritten by the 32KB 0xFF data provided by PSP in the first INIT_EX > > call. > > > > Co-developed-by: Peter Gonda <pgonda@xxxxxxxxxx> > > Signed-off-by: Peter Gonda <pgonda@xxxxxxxxxx> > > Signed-off-by: Jacky Li <jackyli@xxxxxxxxxx> > > Reported-by: Alper Gun <alpergun@xxxxxxxxxx> > > --- > > .../virt/kvm/x86/amd-memory-encryption.rst | 5 ++-- > > drivers/crypto/ccp/sev-dev.c | 29 +++++++++++++------ > > 2 files changed, 22 insertions(+), 12 deletions(-) > > > > diff --git a/Documentation/virt/kvm/x86/amd-memory-encryption.rst b/Documentation/virt/kvm/x86/amd-memory-encryption.rst > > index 2d307811978c..935aaeb97fe6 100644 > > --- a/Documentation/virt/kvm/x86/amd-memory-encryption.rst > > +++ b/Documentation/virt/kvm/x86/amd-memory-encryption.rst > > @@ -89,9 +89,8 @@ context. In a typical workflow, this command should be the first command issued. > > > > The firmware can be initialized either by using its own non-volatile storage or > > the OS can manage the NV storage for the firmware using the module parameter > > -``init_ex_path``. The file specified by ``init_ex_path`` must exist. To create > > -a new NV storage file allocate the file with 32KB bytes of 0xFF as required by > > -the SEV spec. > > +``init_ex_path``. If the file specified by ``init_ex_path`` does not exist or > > +is invalid, the OS will create or override the file with output from PSP. > > > > Returns: 0 on success, -negative on error > > > > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c > > index 799b476fc3e8..5bb2ae250d38 100644 > > --- a/drivers/crypto/ccp/sev-dev.c > > +++ b/drivers/crypto/ccp/sev-dev.c > > @@ -75,6 +75,7 @@ static void *sev_es_tmr; > > */ > > #define NV_LENGTH (32 * 1024) > > static void *sev_init_ex_buffer; > > +static bool nv_file_loaded; > > > > static inline bool sev_version_greater_or_equal(u8 maj, u8 min) > > { > > @@ -211,18 +212,19 @@ static int sev_read_init_ex_file(void) > > if (IS_ERR(fp)) { > > int ret = PTR_ERR(fp); > > > > - dev_err(sev->dev, > > - "SEV: could not open %s for read, error %d\n", > > - init_ex_path, ret); > > + if (ret != -ENOENT) { > > + dev_err(sev->dev, > > + "SEV: could not open %s for read, error %d\n", > > + init_ex_path, ret); > > + } > > Shouldn't there still be some kind of message if the file is missing? A > typo could result in a file being created that the user isn't expecting. > At least indicating that the file will now possibly be created might be > good. Maybe not here, since this is called multiple times, though. > This function will actually only be called once during the psp initialization, I will add an info message here in v2 to indicate the file would be created when missing, thanks! > > return ret; > > } > > > > nread = kernel_read(fp, sev_init_ex_buffer, NV_LENGTH, NULL); > > if (nread != NV_LENGTH) { > > - dev_err(sev->dev, > > - "SEV: failed to read %u bytes to non volatile memory area, ret %ld\n", > > + dev_info(sev->dev, > > + "SEV: could not read %u bytes to non volatile memory area, ret %ld\n", > > NV_LENGTH, nread); > > - return -EIO; > > } > > > > dev_dbg(sev->dev, "SEV: read %ld bytes from NV file\n", nread); > > @@ -417,9 +419,18 @@ static int __sev_init_ex_locked(int *error) > > data.nv_address = __psp_pa(sev_init_ex_buffer); > > data.nv_len = NV_LENGTH; > > > > - ret = sev_read_init_ex_file(); > > - if (ret) > > - return ret; > > + /* > > + * The caller of INIT_EX will retry if it fails. Furthermore, if the > > This is a little confusing since this function, __sev_init_ex_locked(), is > the caller of INIT_EX. Maybe say "The caller of __sev_init_ex_locked() > will retry..." > I'm going to move the sev_read_init_ex_file() to the caller of this function (i.e. __sev_platform_init_locked) in v2 so that it's less confusing. > > + * failure is due to sev_init_ex_buffer being invalid, the PSP will have > > + * properly initialized the buffer already. Therefore, do not initialize > > + * it a second time. > > + */ > > + if (!nv_file_loaded) { > > + ret = sev_read_init_ex_file(); > > + if (ret && ret != -ENOENT) > > + return ret; > > + nv_file_loaded = true; > > This is really meant to show the INIT_EX has been called, right? Maybe > move this and part of the above comment to just before the call to > INIT_EX. That will make this a bit less confusing. > > But, going back to the changes in sev_read_init_ex_file(), couldn't you > just return 0 in the "if (IS_ERR(fp)) {" path if ret == -ENOENT? Then you > don't have to check for it here, too. > > Thanks, > Tom > Thanks Tom, this is a great suggestion. I will move the sev_read_init_ex_file() before the call to the INIT_EX as well as returning 0 for sev_read_init_ex_file when the file is missing in v2. Thanks, Jacky > > + } > > > > if (sev_es_tmr) { > > /*