Re: [PATCH v1 18/26] crypto: ccp: Handle legacy SEV commands when SNP is enabled
From: Borislav Petkov
Date: Fri Jan 19 2024 - 12:19:44 EST
On Sat, Dec 30, 2023 at 10:19:46AM -0600, Michael Roth wrote:
> From: Brijesh Singh <brijesh.singh@xxxxxxx>
>
> 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@xxxxxxx>
> Co-developed-by: Michael Roth <michael.roth@xxxxxxx>
> Signed-off-by: Michael Roth <michael.roth@xxxxxxx>
> Signed-off-by: Ashish Kalra <ashish.kalra@xxxxxxx>
> ---
> 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.
*/
struct cmd_buf_desc {
- u64 *paddr_ptr;
+ u64 paddr;
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