Received-SPF: pass (google.com: domain of linux-crypto+bounces-1679-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Received-SPF: Pass (protection.outlook.com: domain of amd.com designates
 165.204.84.17 as permitted sender) receiver=protection.outlook.com;
 client-ip=165.204.84.17; helo=SATLEXMB04.amd.com; pr=C
Date: Fri, 26 Jan 2024 07:29:02 -0600
From: Michael Roth <michael.roth@amd.com>
To: Borislav Petkov <bp@alien8.de>
CC: <x86@kernel.org>, <kvm@vger.kernel.org>, <linux-coco@lists.linux.dev>,
	<linux-mm@kvack.org>, <linux-crypto@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <tglx@linutronix.de>, <mingo@redhat.com>,
	<jroedel@suse.de>, <thomas.lendacky@amd.com>, <hpa@zytor.com>,
	<ardb@kernel.org>, <pbonzini@redhat.com>, <seanjc@google.com>,
	<vkuznets@redhat.com>, <jmattson@google.com>, <luto@kernel.org>,
	<dave.hansen@linux.intel.com>, <slp@redhat.com>, <pgonda@google.com>,
	<peterz@infradead.org>, <srinivas.pandruvada@linux.intel.com>,
	<rientjes@google.com>, <tobin@ibm.com>, <vbabka@suse.cz>,
	<kirill@shutemov.name>, <ak@linux.intel.com>, <tony.luck@intel.com>,
	<sathyanarayanan.kuppuswamy@linux.intel.com>, <alpergun@google.com>,
	<jarkko@kernel.org>, <ashish.kalra@amd.com>, <nikunj.dadhania@amd.com>,
	<pankaj.gupta@amd.com>, <liam.merwick@oracle.com>
Subject: Re: [PATCH v1 18/26] crypto: ccp: Handle legacy SEV commands when
 SNP is enabled
Message-ID: <20240126132902.p7f6xbg6ru3btsfo@amd.com>
References: <20231230161954.569267-1-michael.roth@amd.com>
 <20231230161954.569267-19-michael.roth@amd.com>
 <20240119171816.GKZaqu2M_1Pu7Q4mBn@fat_crate.local>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240119171816.GKZaqu2M_1Pu7Q4mBn@fat_crate.local>
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Jan 2024 13:39:03.3559
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 1f209bbf-53b1-4e78-1045-08dc1e742917
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=3dd8961f-e488-4e60-8e11-a82d994e183d;Ip=[165.204.84.17];Helo=[SATLEXMB04.amd.com]
X-MS-Exchange-CrossTenant-AuthSource:
	BN1PEPF0000468A.namprd05.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR12MB8948

On Fri, Jan 19, 2024 at 06:18:30PM +0100, Borislav Petkov wrote:
> On Sat, Dec 30, 2023 at 10:19:46AM -0600, Michael Roth wrote:
> > From: Brijesh Singh <brijesh.singh@amd.com>
> > 
> > The behavior of legacy SEV commands is altered when the firmware is
> > initialized for SNP support. In that case, all command buffer memory
> > that may get written to by legacy SEV commands must be marked as
> > firmware-owned in the RMP table prior to issuing the command.
> > 
> > Additionally, when a command buffer contains a system physical address
> > that points to additional buffers that firmware may write to, special
> > handling is needed depending on whether:
> > 
> >   1) the system physical address points to guest memory
> >   2) the system physical address points to host memory
> > 
> > To handle case #1, the pages of these buffers are changed to
> > firmware-owned in the RMP table before issuing the command, and restored
> > to after the command completes.
> > 
> > For case #2, a bounce buffer is used instead of the original address.
> > 
> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> > Co-developed-by: Michael Roth <michael.roth@amd.com>
> > Signed-off-by: Michael Roth <michael.roth@amd.com>
> > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> > ---
> >  drivers/crypto/ccp/sev-dev.c | 421 ++++++++++++++++++++++++++++++++++-
> >  drivers/crypto/ccp/sev-dev.h |   3 +
> >  2 files changed, 414 insertions(+), 10 deletions(-)
> 
> Definitely better, thanks.
> 
> Some cleanups ontop:
> 
> ---
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 8cfb376ca2e7..7681c094c7ff 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -514,18 +514,21 @@ static void *sev_fw_alloc(unsigned long len)
>   * struct cmd_buf_desc - descriptors for managing legacy SEV command address
>   * parameters corresponding to buffers that may be written to by firmware.
>   *
> - * @paddr_ptr: pointer the address parameter in the command buffer, which may
> - *	need to be saved/restored depending on whether a bounce buffer is used.
> - *	Must be NULL if this descriptor is only an end-of-list indicator.
> + * @paddr: address which may need to be saved/restored depending on whether
> + * a bounce buffer is used. Must be NULL if this descriptor is only an
> + * end-of-list indicator.
> + *
>   * @paddr_orig: storage for the original address parameter, which can be used to
> - *	restore the original value in @paddr_ptr in cases where it is replaced
> - *	with the address of a bounce buffer.
> - * @len: length of buffer located at the address originally stored at @paddr_ptr
> + * restore the original value in @paddr in cases where it is replaced with
> + * the address of a bounce buffer.
> + *
> + * @len: length of buffer located at the address originally stored at @paddr
> + *
>   * @guest_owned: true if the address corresponds to guest-owned pages, in which
> - *	case bounce buffers are not needed.
> + * case bounce buffers are not needed.

In v2 I've fixed up the alignments in accordance with
Documentation/doc-guide/kernel-doc.rst, which asks for the starting column of
a multi-line description to be the same as first line, and asked for newlines
to not be inserted in between parameters/members. I've gone back and updated
other occurences in the series as well.

>   */
>  struct cmd_buf_desc {
> -	u64 *paddr_ptr;
> +	u64 paddr;

Unfortunately the logic here doesn't really work with this approach. If
a descriptor needs to be re-mapped to a bounce buffer, the command
buffer that contains the original address of the buffer needs to be
updated to point to it, so that's where the pointer comes into play, so
I've kept this in place for v2, but worked on all your other
suggestions.

Thanks,

Mike

>  	u64 paddr_orig;
>  	u32 len;
>  	bool guest_owned;
> @@ -549,30 +552,30 @@ static void snp_populate_cmd_buf_desc_list(int cmd, void *cmd_buf,
>  	case SEV_CMD_PDH_CERT_EXPORT: {
>  		struct sev_data_pdh_cert_export *data = cmd_buf;
> 
> -		desc_list[0].paddr_ptr = &data->pdh_cert_address;
> +		desc_list[0].paddr = data->pdh_cert_address;
>  		desc_list[0].len = data->pdh_cert_len;
> -		desc_list[1].paddr_ptr = &data->cert_chain_address;
> +		desc_list[1].paddr = data->cert_chain_address;
>  		desc_list[1].len = data->cert_chain_len;
>  		break;
>  	}
>  	case SEV_CMD_GET_ID: {
>  		struct sev_data_get_id *data = cmd_buf;
> 
> -		desc_list[0].paddr_ptr = &data->address;
> +		desc_list[0].paddr = data->address;
>  		desc_list[0].len = data->len;
>  		break;
>  	}
>  	case SEV_CMD_PEK_CSR: {
>  		struct sev_data_pek_csr *data = cmd_buf;
> 
> -		desc_list[0].paddr_ptr = &data->address;
> +		desc_list[0].paddr = data->address;
>  		desc_list[0].len = data->len;
>  		break;
>  	}
>  	case SEV_CMD_LAUNCH_UPDATE_DATA: {
>  		struct sev_data_launch_update_data *data = cmd_buf;
> 
> -		desc_list[0].paddr_ptr = &data->address;
> +		desc_list[0].paddr = data->address;
>  		desc_list[0].len = data->len;
>  		desc_list[0].guest_owned = true;
>  		break;
> @@ -580,7 +583,7 @@ static void snp_populate_cmd_buf_desc_list(int cmd, void *cmd_buf,
>  	case SEV_CMD_LAUNCH_UPDATE_VMSA: {
>  		struct sev_data_launch_update_vmsa *data = cmd_buf;
> 
> -		desc_list[0].paddr_ptr = &data->address;
> +		desc_list[0].paddr = data->address;
>  		desc_list[0].len = data->len;
>  		desc_list[0].guest_owned = true;
>  		break;
> @@ -588,14 +591,14 @@ static void snp_populate_cmd_buf_desc_list(int cmd, void *cmd_buf,
>  	case SEV_CMD_LAUNCH_MEASURE: {
>  		struct sev_data_launch_measure *data = cmd_buf;
> 
> -		desc_list[0].paddr_ptr = &data->address;
> +		desc_list[0].paddr = data->address;
>  		desc_list[0].len = data->len;
>  		break;
>  	}
>  	case SEV_CMD_LAUNCH_UPDATE_SECRET: {
>  		struct sev_data_launch_secret *data = cmd_buf;
> 
> -		desc_list[0].paddr_ptr = &data->guest_address;
> +		desc_list[0].paddr = data->guest_address;
>  		desc_list[0].len = data->guest_len;
>  		desc_list[0].guest_owned = true;
>  		break;
> @@ -603,7 +606,7 @@ static void snp_populate_cmd_buf_desc_list(int cmd, void *cmd_buf,
>  	case SEV_CMD_DBG_DECRYPT: {
>  		struct sev_data_dbg *data = cmd_buf;
> 
> -		desc_list[0].paddr_ptr = &data->dst_addr;
> +		desc_list[0].paddr = data->dst_addr;
>  		desc_list[0].len = data->len;
>  		desc_list[0].guest_owned = true;
>  		break;
> @@ -611,7 +614,7 @@ static void snp_populate_cmd_buf_desc_list(int cmd, void *cmd_buf,
>  	case SEV_CMD_DBG_ENCRYPT: {
>  		struct sev_data_dbg *data = cmd_buf;
> 
> -		desc_list[0].paddr_ptr = &data->dst_addr;
> +		desc_list[0].paddr = data->dst_addr;
>  		desc_list[0].len = data->len;
>  		desc_list[0].guest_owned = true;
>  		break;
> @@ -619,39 +622,39 @@ static void snp_populate_cmd_buf_desc_list(int cmd, void *cmd_buf,
>  	case SEV_CMD_ATTESTATION_REPORT: {
>  		struct sev_data_attestation_report *data = cmd_buf;
> 
> -		desc_list[0].paddr_ptr = &data->address;
> +		desc_list[0].paddr = data->address;
>  		desc_list[0].len = data->len;
>  		break;
>  	}
>  	case SEV_CMD_SEND_START: {
>  		struct sev_data_send_start *data = cmd_buf;
> 
> -		desc_list[0].paddr_ptr = &data->session_address;
> +		desc_list[0].paddr = data->session_address;
>  		desc_list[0].len = data->session_len;
>  		break;
>  	}
>  	case SEV_CMD_SEND_UPDATE_DATA: {
>  		struct sev_data_send_update_data *data = cmd_buf;
> 
> -		desc_list[0].paddr_ptr = &data->hdr_address;
> +		desc_list[0].paddr = data->hdr_address;
>  		desc_list[0].len = data->hdr_len;
> -		desc_list[1].paddr_ptr = &data->trans_address;
> +		desc_list[1].paddr = data->trans_address;
>  		desc_list[1].len = data->trans_len;
>  		break;
>  	}
>  	case SEV_CMD_SEND_UPDATE_VMSA: {
>  		struct sev_data_send_update_vmsa *data = cmd_buf;
> 
> -		desc_list[0].paddr_ptr = &data->hdr_address;
> +		desc_list[0].paddr = data->hdr_address;
>  		desc_list[0].len = data->hdr_len;
> -		desc_list[1].paddr_ptr = &data->trans_address;
> +		desc_list[1].paddr = data->trans_address;
>  		desc_list[1].len = data->trans_len;
>  		break;
>  	}
>  	case SEV_CMD_RECEIVE_UPDATE_DATA: {
>  		struct sev_data_receive_update_data *data = cmd_buf;
> 
> -		desc_list[0].paddr_ptr = &data->guest_address;
> +		desc_list[0].paddr = data->guest_address;
>  		desc_list[0].len = data->guest_len;
>  		desc_list[0].guest_owned = true;
>  		break;
> @@ -659,7 +662,7 @@ static void snp_populate_cmd_buf_desc_list(int cmd, void *cmd_buf,
>  	case SEV_CMD_RECEIVE_UPDATE_VMSA: {
>  		struct sev_data_receive_update_vmsa *data = cmd_buf;
> 
> -		desc_list[0].paddr_ptr = &data->guest_address;
> +		desc_list[0].paddr = data->guest_address;
>  		desc_list[0].len = data->guest_len;
>  		desc_list[0].guest_owned = true;
>  		break;
> @@ -687,16 +690,16 @@ static int snp_map_cmd_buf_desc(struct cmd_buf_desc *desc)
>  			return -ENOMEM;
>  		}
> 
> -		desc->paddr_orig = *desc->paddr_ptr;
> -		*desc->paddr_ptr = __psp_pa(page_to_virt(page));
> +		desc->paddr_orig = desc->paddr;
> +		desc->paddr = __psp_pa(page_to_virt(page));
>  	}
> 
> -	paddr = *desc->paddr_ptr;
> +	paddr = desc->paddr;
>  	npages = PAGE_ALIGN(desc->len) >> PAGE_SHIFT;
> 
>  	/* Transition the buffer to firmware-owned. */
>  	if (rmp_mark_pages_firmware(paddr, npages, true)) {
> -		pr_warn("Failed move pages to firmware-owned state for SEV legacy command.\n");
> +		pr_warn("Error moving pages to firmware-owned state for SEV legacy command.\n");
>  		return -EFAULT;
>  	}
> 
> @@ -705,31 +708,29 @@ static int snp_map_cmd_buf_desc(struct cmd_buf_desc *desc)
> 
>  static int snp_unmap_cmd_buf_desc(struct cmd_buf_desc *desc)
>  {
> -	unsigned long paddr;
>  	unsigned int npages;
> 
>  	if (!desc->len)
>  		return 0;
> 
> -	paddr = *desc->paddr_ptr;
>  	npages = PAGE_ALIGN(desc->len) >> PAGE_SHIFT;
> 
>  	/* Transition the buffers back to hypervisor-owned. */
> -	if (snp_reclaim_pages(paddr, npages, true)) {
> +	if (snp_reclaim_pages(desc->paddr, npages, true)) {
>  		pr_warn("Failed to reclaim firmware-owned pages while issuing SEV legacy command.\n");
>  		return -EFAULT;
>  	}
> 
>  	/* Copy data from bounce buffer and then free it. */
>  	if (!desc->guest_owned) {
> -		void *bounce_buf = __va(__sme_clr(paddr));
> +		void *bounce_buf = __va(__sme_clr(desc->paddr));
>  		void *dst_buf = __va(__sme_clr(desc->paddr_orig));
> 
>  		memcpy(dst_buf, bounce_buf, desc->len);
>  		__free_pages(virt_to_page(bounce_buf), get_order(desc->len));
> 
>  		/* Restore the original address in the command buffer. */
> -		*desc->paddr_ptr = desc->paddr_orig;
> +		desc->paddr = desc->paddr_orig;
>  	}
> 
>  	return 0;
> @@ -737,14 +738,14 @@ static int snp_unmap_cmd_buf_desc(struct cmd_buf_desc *desc)
> 
>  static int snp_map_cmd_buf_desc_list(int cmd, void *cmd_buf, struct cmd_buf_desc *desc_list)
>  {
> -	int i, n;
> +	int i;
> 
>  	snp_populate_cmd_buf_desc_list(cmd, cmd_buf, desc_list);
> 
>  	for (i = 0; i < CMD_BUF_DESC_MAX; i++) {
>  		struct cmd_buf_desc *desc = &desc_list[i];
> 
> -		if (!desc->paddr_ptr)
> +		if (!desc->paddr)
>  			break;
> 
>  		if (snp_map_cmd_buf_desc(desc))
> @@ -754,8 +755,7 @@ static int snp_map_cmd_buf_desc_list(int cmd, void *cmd_buf, struct cmd_buf_desc
>  	return 0;
> 
>  err_unmap:
> -	n = i;
> -	for (i = 0; i < n; i++)
> +	for (i--; i >= 0; i--)
>  		snp_unmap_cmd_buf_desc(&desc_list[i]);
> 
>  	return -EFAULT;
> @@ -768,7 +768,7 @@ static int snp_unmap_cmd_buf_desc_list(struct cmd_buf_desc *desc_list)
>  	for (i = 0; i < CMD_BUF_DESC_MAX; i++) {
>  		struct cmd_buf_desc *desc = &desc_list[i];
> 
> -		if (!desc->paddr_ptr)
> +		if (!desc->paddr)
>  			break;
> 
>  		if (snp_unmap_cmd_buf_desc(desc))
> @@ -799,8 +799,8 @@ static bool sev_cmd_buf_writable(int cmd)
>  	}
>  }
> 
> -/* After SNP is INIT'ed, the behavior of legacy SEV commands is changed. */
> -static bool snp_legacy_handling_needed(int cmd)
> +/* After SNP is initialized, the behavior of legacy SEV commands is changed. */
> +static inline bool snp_legacy_handling_needed(int cmd)
>  {
>  	struct sev_device *sev = psp_master->sev_data;
> 
> @@ -891,7 +891,7 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
>  			sev->cmd_buf_backup_active = true;
>  		} else {
>  			dev_err(sev->dev,
> -				"SEV: too many firmware commands are in-progress, no command buffers available.\n");
> +				"SEV: too many firmware commands in progress, no command buffers available.\n");
>  			return -EBUSY;
>  		}
> 
> @@ -904,7 +904,7 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
>  		ret = snp_prep_cmd_buf(cmd, cmd_buf, desc_list);
>  		if (ret) {
>  			dev_err(sev->dev,
> -				"SEV: failed to prepare buffer for legacy command %#x. Error: %d\n",
> +				"SEV: failed to prepare buffer for legacy command 0x%#x. Error: %d\n",
>  				cmd, ret);
>  			return ret;
>  		}
> 
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette
>