Re: [PATCH v10 17/50] crypto: ccp: Handle the legacy TMR allocation when SNP is enabled

[Michael Roth]

On Fri, Dec 08, 2023 at 02:05:20PM +0100, Borislav Petkov wrote:
> On Mon, Oct 16, 2023 at 08:27:46AM -0500, Michael Roth wrote:
> > From: Brijesh Singh <brijesh.singh@xxxxxxx>
> > 
> > The behavior and requirement for the SEV-legacy command is altered when
> > the SNP firmware is in the INIT state. See SEV-SNP firmware specification
> > for more details.
> > 
> > Allocate the Trusted Memory Region (TMR) as a 2mb sized/aligned region
> > when SNP is enabled to satisfy new requirements for the SNP. Continue
> 
> s/the //
> 
> > allocating a 1mb region for !SNP configuration.
> > 
> > While at it, provide API that can be used by others to allocate a page
> 
> "...an API... ... to allocate a firmware page."
> 
> Simple.
> 
> > that can be used by the firmware.
> 
> > The immediate user for this API will be the KVM driver.
> 
> Delete that sentence.
> 
> > The KVM driver to need to allocate a firmware context
> 
> "The KVM driver needs to allocate ...
> 
> > page during the guest creation. The context page need to be updated
> 
> "needs"
> 
> > by the firmware. See the SEV-SNP specification for further details.
> > 
> > Co-developed-by: Ashish Kalra <ashish.kalra@xxxxxxx>
> > Signed-off-by: Ashish Kalra <ashish.kalra@xxxxxxx>
> > Signed-off-by: Brijesh Singh <brijesh.singh@xxxxxxx>
> > [mdr: use struct sev_data_snp_page_reclaim instead of passing paddr
> >       directly to SEV_CMD_SNP_PAGE_RECLAIM]
> > Signed-off-by: Michael Roth <michael.roth@xxxxxxx>
> > ---
> >  drivers/crypto/ccp/sev-dev.c | 151 ++++++++++++++++++++++++++++++++---
> >  include/linux/psp-sev.h      |   9 +++
> >  2 files changed, 151 insertions(+), 9 deletions(-)
> > 
> > +static int rmp_mark_pages_firmware(unsigned long paddr, unsigned int npages, bool locked)
> > +{
> > +	/* Cbit maybe set in the paddr */
> > +	unsigned long pfn = __sme_clr(paddr) >> PAGE_SHIFT;
> > +	int rc, n = 0, i;
> 
> That n looks like it can be replaced by i.

Indeed, and for snp_reclaim_pages() too by the looks of it. Will fix that up,
along with all the other suggestions.

> > +
> > +void *snp_alloc_firmware_page(gfp_t gfp_mask)
> > +{
> > +	struct page *page;
> > +
> > +	page = __snp_alloc_firmware_pages(gfp_mask, 0, false);
> > +
> > +	return page ? page_address(page) : NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(snp_alloc_firmware_page);
> > +
> > +static void __snp_free_firmware_pages(struct page *page, int order, bool locked)a
> 
> This @locked too is always false. It becomes true later in
> 
> Subject: [PATCH v10 50/50] crypto: ccp: Add panic notifier for SEV/SNP firmware shutdown on kdump
> 
> which talks about some panic notifier running in atomic context. But
> then you can't take locks in atomic context.

In that case, the lock isn't actually taken. locked==true is basically
used to tell the code to not to try to acquire the lock, but the caller
is relying on the fact that all the other CPUs are stopped at that point
so there's no need to protect against multiple concurrent firmware
commands being issued.

> 
> Looks like this whole dance around the locked thing needs a cleanup.

There's another case that will be introduced in the next version of this
series (likely right after this patch) to handle a bug where the buffer used
to access INIT_EX non-volatile data needs to be transitioned to
firmware-owned beforehand. In that case, the CCP cleanup path introduces
another caller of __snp_free_firmware_pages() where locked==true. Maybe this
can be revisited in that context.

Thanks,

Mike


> 
> ...
> 
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette