Message-ID: <20231229213828.x6zivjikri5qfa5e@amd.com>
Date: Fri, 29 Dec 2023 15:38:28 -0600
From: Michael Roth <michael.roth@....com>
To: Borislav Petkov <bp@...en8.de>
CC: <kvm@...r.kernel.org>, <linux-coco@...ts.linux.dev>, <linux-mm@...ck.org>,
	<linux-crypto@...r.kernel.org>, <x86@...nel.org>,
	<linux-kernel@...r.kernel.org>, <tglx@...utronix.de>, <mingo@...hat.com>,
	<jroedel@...e.de>, <thomas.lendacky@....com>, <hpa@...or.com>,
	<ardb@...nel.org>, <pbonzini@...hat.com>, <seanjc@...gle.com>,
	<vkuznets@...hat.com>, <jmattson@...gle.com>, <luto@...nel.org>,
	<dave.hansen@...ux.intel.com>, <slp@...hat.com>, <pgonda@...gle.com>,
	<peterz@...radead.org>, <srinivas.pandruvada@...ux.intel.com>,
	<rientjes@...gle.com>, <dovmurik@...ux.ibm.com>, <tobin@....com>,
	<vbabka@...e.cz>, <kirill@...temov.name>, <ak@...ux.intel.com>,
	<tony.luck@...el.com>, <marcorr@...gle.com>,
	<sathyanarayanan.kuppuswamy@...ux.intel.com>, <alpergun@...gle.com>,
	<jarkko@...nel.org>, <ashish.kalra@....com>, <nikunj.dadhania@....com>,
	<pankaj.gupta@....com>, <liam.merwick@...cle.com>, <zhi.a.wang@...el.com>,
	Brijesh Singh <brijesh.singh@....com>
Subject: Re: [PATCH v10 18/50] crypto: ccp: Handle the legacy SEV command
 when SNP is enabled

On Sat, Dec 09, 2023 at 04:36:56PM +0100, Borislav Petkov wrote:
> > +static int __snp_cmd_buf_copy(int cmd, void *cmd_buf, bool to_fw, int fw_err)
> > +{
> > +	int (*func)(u64 *paddr, u32 len, bool guest, struct snp_host_map *map);
> > +	struct sev_device *sev = psp_master->sev_data;
> > +	bool from_fw = !to_fw;
> > +
> > +	/*
> > +	 * After the command is completed, change the command buffer memory to
> > +	 * hypervisor state.
> > +	 *
> > +	 * The immutable bit is automatically cleared by the firmware, so
> > +	 * no not need to reclaim the page.
> > +	 */
> > +	if (from_fw && sev_legacy_cmd_buf_writable(cmd)) {
> > +		if (snp_reclaim_pages(__pa(cmd_buf), 1, true))
> > +			return -EFAULT;
> > +
> > +		/* No need to go further if firmware failed to execute command. */
> > +		if (fw_err)
> > +			return 0;
> > +	}
> > +
> > +	if (to_fw)
> > +		func = map_firmware_writeable;
> > +	else
> > +		func = unmap_firmware_writeable;
> 
> Eww, ugly and with the macro above even worse. And completely
> unnecessary.
> 
> Define prep_buffer() as a normal function which selects which @func to
> call and then does it. Not like this.

I've rewritten this using a descriptor array to handle buffers for
various command parameters, and switched to allocating bounce buffers
on-demand to avoid some of the init/cleanup coordination. I dont think
any of these are really performance critical and its only for legacy
support, but would be straightforward to add a cache of pre-allocated
buffers later if needed.

I've tried to document/name the helpers so the flow is a bit clearer.

-Mike

> 
> ...
> 
> > +static inline bool need_firmware_copy(int cmd)
> > +{
> > +	struct sev_device *sev = psp_master->sev_data;
> > +
> > +	/* After SNP is INIT'ed, the behavior of legacy SEV command is changed. */
> 
> "initialized"
> 
> > +	return ((cmd < SEV_CMD_SNP_INIT) && sev->snp_initialized) ? true : false;
> 
> redundant ternary conditional:
> 
> 	return cmd < SEV_CMD_SNP_INIT && sev->snp_initialized;
> 
> > +}
> > +
> > +static int snp_aware_copy_to_firmware(int cmd, void *data)
> 
> What does "SNP aware" even mean?
> 
> > +{
> > +	return __snp_cmd_buf_copy(cmd, data, true, 0);
> > +}
> > +
> > +static int snp_aware_copy_from_firmware(int cmd, void *data, int fw_err)
> > +{
> > +	return __snp_cmd_buf_copy(cmd, data, false, fw_err);
> > +}
> > +
> >  static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> >  {
> >  	struct psp_device *psp = psp_master;
> >  	struct sev_device *sev;
> >  	unsigned int phys_lsb, phys_msb;
> >  	unsigned int reg, ret = 0;
> > +	void *cmd_buf;
> >  	int buf_len;
> >  
> >  	if (!psp || !psp->sev_data)
> > @@ -487,12 +770,28 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> >  	 * work for some memory, e.g. vmalloc'd addresses, and @data may not be
> >  	 * physically contiguous.
> >  	 */
> > -	if (data)
> > -		memcpy(sev->cmd_buf, data, buf_len);
> > +	if (data) {
> > +		if (sev->cmd_buf_active > 2)
> 
> What is that silly counter supposed to mean?
> 
> Nested SNP commands?
> 
> > +			return -EBUSY;
> > +
> > +		cmd_buf = sev->cmd_buf_active ? sev->cmd_buf_backup : sev->cmd_buf;
> > +
> > +		memcpy(cmd_buf, data, buf_len);
> > +		sev->cmd_buf_active++;
> > +
> > +		/*
> > +		 * The behavior of the SEV-legacy commands is altered when the
> > +		 * SNP firmware is in the INIT state.
> > +		 */
> > +		if (need_firmware_copy(cmd) && snp_aware_copy_to_firmware(cmd, cmd_buf))
> 
> Move that need_firmware_copy() check inside snp_aware_copy_to_firmware()
> and the other one.
> 
> > +			return -EFAULT;
> > +	} else {
> > +		cmd_buf = sev->cmd_buf;
> > +	}
> >  
> >  	/* Get the physical address of the command buffer */
> > -	phys_lsb = data ? lower_32_bits(__psp_pa(sev->cmd_buf)) : 0;
> > -	phys_msb = data ? upper_32_bits(__psp_pa(sev->cmd_buf)) : 0;
> > +	phys_lsb = data ? lower_32_bits(__psp_pa(cmd_buf)) : 0;
> > +	phys_msb = data ? upper_32_bits(__psp_pa(cmd_buf)) : 0;
> >  
> >  	dev_dbg(sev->dev, "sev command id %#x buffer 0x%08x%08x timeout %us\n",
> >  		cmd, phys_msb, phys_lsb, psp_timeout);
> 
> ...
> 
> > @@ -639,6 +947,14 @@ static int ___sev_platform_init_locked(int *error, bool probe)
> >  	if (probe && !psp_init_on_probe)
> >  		return 0;
> >  
> > +	/*
> > +	 * Allocate the intermediate buffers used for the legacy command handling.
> > +	 */
> > +	if (rc != -ENODEV && alloc_snp_host_map(sev)) {
> 
> Why isn't this
> 
> 	if (!rc && ...)
> 
> > +		dev_notice(sev->dev, "Failed to alloc host map (disabling legacy SEV)\n");
> > +		goto skip_legacy;
> 
> No need for that skip_legacy silly label. Just "return 0" here.
> 
> ...
> 
> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette
> 

Powered by blists - more mailing lists