| Message ID | 20240823132137.336874-7-aik@amd.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Secure VFIO, TDISP, SEV TIO | expand |
On Fri, 23 Aug 2024 23:21:20 +1000 Alexey Kardashevskiy <aik@amd.com> wrote: > The PSP advertises the SEV-TIO support via the FEATURE_INFO command > support of which is advertised via SNP_PLATFORM_STATUS. > > Add FEATURE_INFO and use it to detect the TIO support in the PSP. > If present, enable TIO in the SNP_INIT_EX call. > > While at this, add new bits to sev_data_snp_init_ex() from SEV-SNP 1.55. > > Note that this tests the PSP firmware support but not if the feature > is enabled in the BIOS. > > While at this, add new sev_data_snp_shutdown_ex::x86_snp_shutdown > > Signed-off-by: Alexey Kardashevskiy <aik@amd.com> I was curious so had a read. Some minor comments inline. Jonathan > --- > include/linux/psp-sev.h | 31 ++++++++- > include/uapi/linux/psp-sev.h | 4 +- > drivers/crypto/ccp/sev-dev.c | 73 ++++++++++++++++++++ > 3 files changed, 104 insertions(+), 4 deletions(-) > > diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h > index 52d5ee101d3a..1d63044f66be 100644 > --- a/include/linux/psp-sev.h > +++ b/include/linux/psp-sev.h > @@ -107,6 +107,7 @@ enum sev_cmd { > SEV_CMD_SNP_DOWNLOAD_FIRMWARE_EX = 0x0CA, > SEV_CMD_SNP_COMMIT = 0x0CB, > SEV_CMD_SNP_VLEK_LOAD = 0x0CD, > + SEV_CMD_SNP_FEATURE_INFO = 0x0CE, > > SEV_CMD_MAX, > }; > @@ -584,6 +585,25 @@ struct sev_data_snp_addr { > u64 address; /* In/Out */ > } __packed; > > +/** > + * struct sev_data_snp_feature_info - SEV_CMD_SNP_FEATURE_INFO command params > + * > + * @len: length of this struct > + * @ecx_in: subfunction index of CPUID Fn8000_0024 > + * @feature_info_paddr: physical address of a page with sev_snp_feature_info > + */ Comment seems to have drifted away from the structure. > +#define SNP_FEATURE_FN8000_0024_EBX_X00_SEVTIO 1 > + > +struct sev_snp_feature_info { > + u32 eax, ebx, ecx, edx; /* Out */ > +} __packed; > + > +struct sev_data_snp_feature_info { > + u32 length; /* In */ > + u32 ecx_in; /* In */ > + u64 feature_info_paddr; /* In */ > +} __packed; > + > /** > @@ -787,7 +811,8 @@ struct sev_data_range_list { > struct sev_data_snp_shutdown_ex { > u32 len; > u32 iommu_snp_shutdown:1; > - u32 rsvd1:31; > + u32 x86_snp_shutdown:1; Has docs that want updating I think. > + u32 rsvd1:30; > } __packed; > > /** > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c > index f6eafde584d9..a49fe54b8dd8 100644 > --- a/drivers/crypto/ccp/sev-dev.c > +++ b/drivers/crypto/ccp/sev-dev.c > @@ -223,6 +223,7 @@ static int sev_cmd_buffer_len(int cmd) > +static int snp_feature_info_locked(struct sev_device *sev, u32 ecx, > + struct sev_snp_feature_info *fi, int *psp_ret) > +{ > + struct sev_data_snp_feature_info buf = { > + .length = sizeof(buf), > + .ecx_in = ecx, > + }; > + struct page *status_page; > + void *data; > + int ret; > + > + status_page = alloc_page(GFP_KERNEL_ACCOUNT); > + if (!status_page) > + return -ENOMEM; > + > + data = page_address(status_page); > + > + if (sev->snp_initialized && rmp_mark_pages_firmware(__pa(data), 1, true)) { > + ret = -EFAULT; > + goto cleanup; > + } > + > + buf.feature_info_paddr = __psp_pa(data); > + ret = __sev_do_cmd_locked(SEV_CMD_SNP_FEATURE_INFO, &buf, psp_ret); > + > + if (sev->snp_initialized && snp_reclaim_pages(__pa(data), 1, true)) > + ret = -EFAULT; goto cleanup } memcpy(fi, data, sizeof(*fi)); > + > + if (!ret) > + memcpy(fi, data, sizeof(*fi)); rather than this is more consistent and hence easier to review. > + > +cleanup: > + __free_pages(status_page, 0); free_page(status_page); Maybe worth a DEFINE_FREE() to let you do early returns and make this even nicer to read. > + return ret; > +} > + > +static int snp_get_feature_info(struct sev_device *sev, u32 ecx, struct sev_snp_feature_info *fi) > +{ > + struct sev_user_data_snp_status status = { 0 }; > + int psp_ret = 0, ret; > + > + ret = snp_platform_status_locked(sev, &status, &psp_ret); > + if (ret) > + return ret; > + if (ret != SEV_RET_SUCCESS) won't get here as ret definitely == 0 given you checked it was just above. > + return -EFAULT; > + if (!status.feature_info) > + return -ENOENT; > + > + ret = snp_feature_info_locked(sev, ecx, fi, &psp_ret); > + if (ret) > + return ret; > + if (ret != SEV_RET_SUCCESS) > + return -EFAULT; and another. return snp_feature_info_locked(... > + > + return 0; > +} > + > +static bool sev_tio_present(struct sev_device *sev) > +{ > + struct sev_snp_feature_info fi = { 0 }; > + bool present; > + > + if (snp_get_feature_info(sev, 0, &fi)) > + return false; > + > + present = (fi.ebx & SNP_FEATURE_FN8000_0024_EBX_X00_SEVTIO) != 0; > + dev_info(sev->dev, "SEV-TIO support is %s\n", present ? "present" : "not present"); Probably too noisy for final driver but fine for RFC I guess. > + return present; > +} > + > static int __sev_snp_init_locked(int *error) > { > struct psp_device *psp = psp_master; > @@ -1189,6 +1261,7 @@ static int __sev_snp_init_locked(int *error) > data.init_rmp = 1; > data.list_paddr_en = 1; > data.list_paddr = __psp_pa(snp_range_list); > + data.tio_en = sev_tio_present(sev); > cmd = SEV_CMD_SNP_INIT_EX; > } else { > cmd = SEV_CMD_SNP_INIT;
Alexey Kardashevskiy wrote: > The PSP advertises the SEV-TIO support via the FEATURE_INFO command > support of which is advertised via SNP_PLATFORM_STATUS. > > Add FEATURE_INFO and use it to detect the TIO support in the PSP. > If present, enable TIO in the SNP_INIT_EX call. > > While at this, add new bits to sev_data_snp_init_ex() from SEV-SNP 1.55. > > Note that this tests the PSP firmware support but not if the feature > is enabled in the BIOS. > > While at this, add new sev_data_snp_shutdown_ex::x86_snp_shutdown > > Signed-off-by: Alexey Kardashevskiy <aik@amd.com> > --- > include/linux/psp-sev.h | 31 ++++++++- > include/uapi/linux/psp-sev.h | 4 +- > drivers/crypto/ccp/sev-dev.c | 73 ++++++++++++++++++++ > 3 files changed, 104 insertions(+), 4 deletions(-) Taking a peek to familiarize myself with that is required for TIO enabling in the PSP driver... > > diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h > index 52d5ee101d3a..1d63044f66be 100644 > --- a/include/linux/psp-sev.h > +++ b/include/linux/psp-sev.h > @@ -107,6 +107,7 @@ enum sev_cmd { > SEV_CMD_SNP_DOWNLOAD_FIRMWARE_EX = 0x0CA, > SEV_CMD_SNP_COMMIT = 0x0CB, > SEV_CMD_SNP_VLEK_LOAD = 0x0CD, > + SEV_CMD_SNP_FEATURE_INFO = 0x0CE, > > SEV_CMD_MAX, > }; > @@ -584,6 +585,25 @@ struct sev_data_snp_addr { > u64 address; /* In/Out */ > } __packed; > > +/** > + * struct sev_data_snp_feature_info - SEV_CMD_SNP_FEATURE_INFO command params > + * > + * @len: length of this struct > + * @ecx_in: subfunction index of CPUID Fn8000_0024 > + * @feature_info_paddr: physical address of a page with sev_snp_feature_info > + */ > +#define SNP_FEATURE_FN8000_0024_EBX_X00_SEVTIO 1 > + > +struct sev_snp_feature_info { > + u32 eax, ebx, ecx, edx; /* Out */ > +} __packed; > + > +struct sev_data_snp_feature_info { > + u32 length; /* In */ > + u32 ecx_in; /* In */ > + u64 feature_info_paddr; /* In */ > +} __packed; Why use CPU register names in C structures? I would hope the spec renames these parameters to something meaninful? > + > /** > * struct sev_data_snp_launch_start - SNP_LAUNCH_START command params > * > @@ -745,10 +765,14 @@ struct sev_data_snp_guest_request { Would be nice to have direct pointer to the spec and spec chapter documented for these command structure fields. > struct sev_data_snp_init_ex { > u32 init_rmp:1; > u32 list_paddr_en:1; > - u32 rsvd:30; > + u32 rapl_dis:1; > + u32 ciphertext_hiding_en:1; > + u32 tio_en:1; > + u32 rsvd:27; > u32 rsvd1; > u64 list_paddr; > - u8 rsvd2[48]; > + u16 max_snp_asid; > + u8 rsvd2[46]; > } __packed; > > /** > @@ -787,7 +811,8 @@ struct sev_data_range_list { > struct sev_data_snp_shutdown_ex { > u32 len; > u32 iommu_snp_shutdown:1; > - u32 rsvd1:31; > + u32 x86_snp_shutdown:1; > + u32 rsvd1:30; > } __packed; > > /** [..] > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c > index f6eafde584d9..a49fe54b8dd8 100644 > --- a/drivers/crypto/ccp/sev-dev.c > +++ b/drivers/crypto/ccp/sev-dev.c > @@ -223,6 +223,7 @@ static int sev_cmd_buffer_len(int cmd) > case SEV_CMD_SNP_GUEST_REQUEST: return sizeof(struct sev_data_snp_guest_request); > case SEV_CMD_SNP_CONFIG: return sizeof(struct sev_user_data_snp_config); > case SEV_CMD_SNP_COMMIT: return sizeof(struct sev_data_snp_commit); > + case SEV_CMD_SNP_FEATURE_INFO: return sizeof(struct sev_data_snp_feature_info); > default: return 0; > } > > @@ -1125,6 +1126,77 @@ static int snp_platform_status_locked(struct sev_device *sev, > return ret; > } > > +static int snp_feature_info_locked(struct sev_device *sev, u32 ecx, > + struct sev_snp_feature_info *fi, int *psp_ret) > +{ > + struct sev_data_snp_feature_info buf = { > + .length = sizeof(buf), > + .ecx_in = ecx, > + }; > + struct page *status_page; > + void *data; > + int ret; > + > + status_page = alloc_page(GFP_KERNEL_ACCOUNT); > + if (!status_page) > + return -ENOMEM; > + > + data = page_address(status_page); > + > + if (sev->snp_initialized && rmp_mark_pages_firmware(__pa(data), 1, true)) { > + ret = -EFAULT; > + goto cleanup; Jonathan already mentioned this, but "goto cleanup" is so 2022. > + } > + > + buf.feature_info_paddr = __psp_pa(data); > + ret = __sev_do_cmd_locked(SEV_CMD_SNP_FEATURE_INFO, &buf, psp_ret); > + > + if (sev->snp_initialized && snp_reclaim_pages(__pa(data), 1, true)) > + ret = -EFAULT; > + > + if (!ret) > + memcpy(fi, data, sizeof(*fi)); > + > +cleanup: > + __free_pages(status_page, 0); > + return ret; > +} > + > +static int snp_get_feature_info(struct sev_device *sev, u32 ecx, struct sev_snp_feature_info *fi) Why not make this bool... > +{ > + struct sev_user_data_snp_status status = { 0 }; > + int psp_ret = 0, ret; > + > + ret = snp_platform_status_locked(sev, &status, &psp_ret); > + if (ret) > + return ret; > + if (ret != SEV_RET_SUCCESS) > + return -EFAULT; > + if (!status.feature_info) > + return -ENOENT; > + > + ret = snp_feature_info_locked(sev, ecx, fi, &psp_ret); > + if (ret) > + return ret; > + if (ret != SEV_RET_SUCCESS) > + return -EFAULT; > + > + return 0; > +} > + > +static bool sev_tio_present(struct sev_device *sev) > +{ > + struct sev_snp_feature_info fi = { 0 }; > + bool present; > + > + if (snp_get_feature_info(sev, 0, &fi)) ...since the caller does not care? > + return false; > + > + present = (fi.ebx & SNP_FEATURE_FN8000_0024_EBX_X00_SEVTIO) != 0; > + dev_info(sev->dev, "SEV-TIO support is %s\n", present ? "present" : "not present"); > + return present; > +} > + > static int __sev_snp_init_locked(int *error) > { > struct psp_device *psp = psp_master; > @@ -1189,6 +1261,7 @@ static int __sev_snp_init_locked(int *error) > data.init_rmp = 1; > data.list_paddr_en = 1; > data.list_paddr = __psp_pa(snp_range_list); > + data.tio_en = sev_tio_present(sev); Where does this get saved for follow-on code to consume that TIO is active?
On 4/9/24 07:27, Dan Williams wrote: > Alexey Kardashevskiy wrote: >> The PSP advertises the SEV-TIO support via the FEATURE_INFO command >> support of which is advertised via SNP_PLATFORM_STATUS. >> >> Add FEATURE_INFO and use it to detect the TIO support in the PSP. >> If present, enable TIO in the SNP_INIT_EX call. >> >> While at this, add new bits to sev_data_snp_init_ex() from SEV-SNP 1.55. >> >> Note that this tests the PSP firmware support but not if the feature >> is enabled in the BIOS. >> >> While at this, add new sev_data_snp_shutdown_ex::x86_snp_shutdown >> >> Signed-off-by: Alexey Kardashevskiy <aik@amd.com> >> --- >> include/linux/psp-sev.h | 31 ++++++++- >> include/uapi/linux/psp-sev.h | 4 +- >> drivers/crypto/ccp/sev-dev.c | 73 ++++++++++++++++++++ >> 3 files changed, 104 insertions(+), 4 deletions(-) > > Taking a peek to familiarize myself with that is required for TIO > enabling in the PSP driver... > >> >> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h >> index 52d5ee101d3a..1d63044f66be 100644 >> --- a/include/linux/psp-sev.h >> +++ b/include/linux/psp-sev.h >> @@ -107,6 +107,7 @@ enum sev_cmd { >> SEV_CMD_SNP_DOWNLOAD_FIRMWARE_EX = 0x0CA, >> SEV_CMD_SNP_COMMIT = 0x0CB, >> SEV_CMD_SNP_VLEK_LOAD = 0x0CD, >> + SEV_CMD_SNP_FEATURE_INFO = 0x0CE, >> >> SEV_CMD_MAX, >> }; >> @@ -584,6 +585,25 @@ struct sev_data_snp_addr { >> u64 address; /* In/Out */ >> } __packed; >> >> +/** >> + * struct sev_data_snp_feature_info - SEV_CMD_SNP_FEATURE_INFO command params >> + * >> + * @len: length of this struct >> + * @ecx_in: subfunction index of CPUID Fn8000_0024 >> + * @feature_info_paddr: physical address of a page with sev_snp_feature_info >> + */ >> +#define SNP_FEATURE_FN8000_0024_EBX_X00_SEVTIO 1 >> + >> +struct sev_snp_feature_info { >> + u32 eax, ebx, ecx, edx; /* Out */ >> +} __packed; >> + >> +struct sev_data_snp_feature_info { >> + u32 length; /* In */ >> + u32 ecx_in; /* In */ >> + u64 feature_info_paddr; /* In */ >> +} __packed; > > Why use CPU register names in C structures? I would hope the spec > renames these parameters to something meaninful? This mimics the CPUID instruction and (my guess) x86 people are used to "CPUID's ECX" == "Subfunction index". The spec (the one I mention below) calls it precisely "ECX_IN". >> + >> /** >> * struct sev_data_snp_launch_start - SNP_LAUNCH_START command params >> * >> @@ -745,10 +765,14 @@ struct sev_data_snp_guest_request { > > Would be nice to have direct pointer to the spec and spec chapter > documented for these command structure fields. For every command? Seems overkill. Any good example? Although the file could have mentioned in the header that SNP_xxx are from "SEV Secure Nested Paging Firmware ABI Specification" which google easily finds, and search on that pdf for "SNP_INIT_EX" finds the structure layout. Using the exact chapter numbers/titles means they cannot change, or someone has to track the changes. > >> struct sev_data_snp_init_ex { >> u32 init_rmp:1; >> u32 list_paddr_en:1; >> - u32 rsvd:30; >> + u32 rapl_dis:1; >> + u32 ciphertext_hiding_en:1; >> + u32 tio_en:1; >> + u32 rsvd:27; >> u32 rsvd1; >> u64 list_paddr; >> - u8 rsvd2[48]; >> + u16 max_snp_asid; >> + u8 rsvd2[46]; >> } __packed; >> >> /** >> @@ -787,7 +811,8 @@ struct sev_data_range_list { >> struct sev_data_snp_shutdown_ex { >> u32 len; >> u32 iommu_snp_shutdown:1; >> - u32 rsvd1:31; >> + u32 x86_snp_shutdown:1; >> + u32 rsvd1:30; >> } __packed; >> >> /** > [..] >> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c >> index f6eafde584d9..a49fe54b8dd8 100644 >> --- a/drivers/crypto/ccp/sev-dev.c >> +++ b/drivers/crypto/ccp/sev-dev.c >> @@ -223,6 +223,7 @@ static int sev_cmd_buffer_len(int cmd) >> case SEV_CMD_SNP_GUEST_REQUEST: return sizeof(struct sev_data_snp_guest_request); >> case SEV_CMD_SNP_CONFIG: return sizeof(struct sev_user_data_snp_config); >> case SEV_CMD_SNP_COMMIT: return sizeof(struct sev_data_snp_commit); >> + case SEV_CMD_SNP_FEATURE_INFO: return sizeof(struct sev_data_snp_feature_info); >> default: return 0; >> } >> >> @@ -1125,6 +1126,77 @@ static int snp_platform_status_locked(struct sev_device *sev, >> return ret; >> } >> >> +static int snp_feature_info_locked(struct sev_device *sev, u32 ecx, >> + struct sev_snp_feature_info *fi, int *psp_ret) >> +{ >> + struct sev_data_snp_feature_info buf = { >> + .length = sizeof(buf), >> + .ecx_in = ecx, >> + }; >> + struct page *status_page; >> + void *data; >> + int ret; >> + >> + status_page = alloc_page(GFP_KERNEL_ACCOUNT); >> + if (!status_page) >> + return -ENOMEM; >> + >> + data = page_address(status_page); >> + >> + if (sev->snp_initialized && rmp_mark_pages_firmware(__pa(data), 1, true)) { >> + ret = -EFAULT; >> + goto cleanup; > > Jonathan already mentioned this, but "goto cleanup" is so 2022. This requires DEFINE_FREE() which yet another place to look at. When I Then, no_free_ptr() just hurts to read (cold breath of c++). It is not needed here but unavoidably will be in other places when I start using __free(kfree). But alright, I'll switch. > >> + } >> + >> + buf.feature_info_paddr = __psp_pa(data); >> + ret = __sev_do_cmd_locked(SEV_CMD_SNP_FEATURE_INFO, &buf, psp_ret); >> + >> + if (sev->snp_initialized && snp_reclaim_pages(__pa(data), 1, true)) >> + ret = -EFAULT; >> + >> + if (!ret) >> + memcpy(fi, data, sizeof(*fi)); >> + >> +cleanup: >> + __free_pages(status_page, 0); >> + return ret; >> +} >> + >> +static int snp_get_feature_info(struct sev_device *sev, u32 ecx, struct sev_snp_feature_info *fi) > > Why not make this bool... > >> +{ >> + struct sev_user_data_snp_status status = { 0 }; >> + int psp_ret = 0, ret; >> + >> + ret = snp_platform_status_locked(sev, &status, &psp_ret); >> + if (ret) >> + return ret; >> + if (ret != SEV_RET_SUCCESS) >> + return -EFAULT; >> + if (!status.feature_info) >> + return -ENOENT; >> + >> + ret = snp_feature_info_locked(sev, ecx, fi, &psp_ret); >> + if (ret) >> + return ret; >> + if (ret != SEV_RET_SUCCESS) >> + return -EFAULT; >> + >> + return 0; >> +} >> + >> +static bool sev_tio_present(struct sev_device *sev) >> +{ >> + struct sev_snp_feature_info fi = { 0 }; >> + bool present; >> + >> + if (snp_get_feature_info(sev, 0, &fi)) > > ...since the caller does not care? sev_tio_present() does not but other users of snp_get_feature_info() (one is coming sooner that TIO) might, WIP. >> + return false; >> + >> + present = (fi.ebx & SNP_FEATURE_FN8000_0024_EBX_X00_SEVTIO) != 0; >> + dev_info(sev->dev, "SEV-TIO support is %s\n", present ? "present" : "not present"); >> + return present; >> +} >> + >> static int __sev_snp_init_locked(int *error) >> { >> struct psp_device *psp = psp_master; >> @@ -1189,6 +1261,7 @@ static int __sev_snp_init_locked(int *error) >> data.init_rmp = 1; >> data.list_paddr_en = 1; >> data.list_paddr = __psp_pa(snp_range_list); >> + data.tio_en = sev_tio_present(sev); > > Where does this get saved for follow-on code to consume that TIO is > active? Oh. It is not saved, whether TIO is actually active is determined by the result of calling PSP's TIO_STATUS (which I should skip if tio_en == false in the first place). Thanks,
Alexey Kardashevskiy wrote: > > > On 4/9/24 07:27, Dan Williams wrote: > > Alexey Kardashevskiy wrote: > >> The PSP advertises the SEV-TIO support via the FEATURE_INFO command > >> support of which is advertised via SNP_PLATFORM_STATUS. > >> > >> Add FEATURE_INFO and use it to detect the TIO support in the PSP. > >> If present, enable TIO in the SNP_INIT_EX call. > >> > >> While at this, add new bits to sev_data_snp_init_ex() from SEV-SNP 1.55. > >> > >> Note that this tests the PSP firmware support but not if the feature > >> is enabled in the BIOS. > >> > >> While at this, add new sev_data_snp_shutdown_ex::x86_snp_shutdown > >> > >> Signed-off-by: Alexey Kardashevskiy <aik@amd.com> [..] > > Why use CPU register names in C structures? I would hope the spec > > renames these parameters to something meaninful? > > This mimics the CPUID instruction and (my guess) x86 people are used to > "CPUID's ECX" == "Subfunction index". The spec (the one I mention below) > calls it precisely "ECX_IN". Oh, I never would have guessed that "snp feature info" mimicked CPUID, but then again, no one has ever accused me of being an "x86 people". > >> /** > >> * struct sev_data_snp_launch_start - SNP_LAUNCH_START command params > >> * > >> @@ -745,10 +765,14 @@ struct sev_data_snp_guest_request { > > > > Would be nice to have direct pointer to the spec and spec chapter > > documented for these command structure fields. > > For every command? Seems overkill. Any good example? > > Although the file could have mentioned in the header that SNP_xxx are > from "SEV Secure Nested Paging Firmware ABI Specification" which google > easily finds, and search on that pdf for "SNP_INIT_EX" finds the > structure layout. Using the exact chapter numbers/titles means they > cannot change, or someone has to track the changes. No need to go overboard, but you can grep for: "PCIe\ r\[0-9\]" ...or: "CXL\ \[12\]" ...for some examples. Yes, these references can bit rot, but that can also be good information "the last time this definition was touched was in vN and vN+1 introduced some changes." [..] > >> +static int snp_get_feature_info(struct sev_device *sev, u32 ecx, struct sev_snp_feature_info *fi) > > > > Why not make this bool... > > > >> +{ > >> + struct sev_user_data_snp_status status = { 0 }; > >> + int psp_ret = 0, ret; > >> + > >> + ret = snp_platform_status_locked(sev, &status, &psp_ret); > >> + if (ret) > >> + return ret; > >> + if (ret != SEV_RET_SUCCESS) > >> + return -EFAULT; > >> + if (!status.feature_info) > >> + return -ENOENT; > >> + > >> + ret = snp_feature_info_locked(sev, ecx, fi, &psp_ret); > >> + if (ret) > >> + return ret; > >> + if (ret != SEV_RET_SUCCESS) > >> + return -EFAULT; > >> + > >> + return 0; > >> +} > >> + > >> +static bool sev_tio_present(struct sev_device *sev) > >> +{ > >> + struct sev_snp_feature_info fi = { 0 }; > >> + bool present; > >> + > >> + if (snp_get_feature_info(sev, 0, &fi)) > > > > ...since the caller does not care? > > sev_tio_present() does not but other users of snp_get_feature_info() > (one is coming sooner that TIO) might, WIP. ...not a huge deal, but it definitely looked odd to see so much care to return distinct error codes only to throw away the distinction.
diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h index 52d5ee101d3a..1d63044f66be 100644 --- a/include/linux/psp-sev.h +++ b/include/linux/psp-sev.h @@ -107,6 +107,7 @@ enum sev_cmd { SEV_CMD_SNP_DOWNLOAD_FIRMWARE_EX = 0x0CA, SEV_CMD_SNP_COMMIT = 0x0CB, SEV_CMD_SNP_VLEK_LOAD = 0x0CD, + SEV_CMD_SNP_FEATURE_INFO = 0x0CE, SEV_CMD_MAX, }; @@ -584,6 +585,25 @@ struct sev_data_snp_addr { u64 address; /* In/Out */ } __packed; +/** + * struct sev_data_snp_feature_info - SEV_CMD_SNP_FEATURE_INFO command params + * + * @len: length of this struct + * @ecx_in: subfunction index of CPUID Fn8000_0024 + * @feature_info_paddr: physical address of a page with sev_snp_feature_info + */ +#define SNP_FEATURE_FN8000_0024_EBX_X00_SEVTIO 1 + +struct sev_snp_feature_info { + u32 eax, ebx, ecx, edx; /* Out */ +} __packed; + +struct sev_data_snp_feature_info { + u32 length; /* In */ + u32 ecx_in; /* In */ + u64 feature_info_paddr; /* In */ +} __packed; + /** * struct sev_data_snp_launch_start - SNP_LAUNCH_START command params * @@ -745,10 +765,14 @@ struct sev_data_snp_guest_request { struct sev_data_snp_init_ex { u32 init_rmp:1; u32 list_paddr_en:1; - u32 rsvd:30; + u32 rapl_dis:1; + u32 ciphertext_hiding_en:1; + u32 tio_en:1; + u32 rsvd:27; u32 rsvd1; u64 list_paddr; - u8 rsvd2[48]; + u16 max_snp_asid; + u8 rsvd2[46]; } __packed; /** @@ -787,7 +811,8 @@ struct sev_data_range_list { struct sev_data_snp_shutdown_ex { u32 len; u32 iommu_snp_shutdown:1; - u32 rsvd1:31; + u32 x86_snp_shutdown:1; + u32 rsvd1:30; } __packed; /** diff --git a/include/uapi/linux/psp-sev.h b/include/uapi/linux/psp-sev.h index 7d2e10e3cdd5..28ee2a03c2b9 100644 --- a/include/uapi/linux/psp-sev.h +++ b/include/uapi/linux/psp-sev.h @@ -214,6 +214,7 @@ struct sev_user_data_get_id2 { * @mask_chip_id: whether chip id is present in attestation reports or not * @mask_chip_key: whether attestation reports are signed or not * @vlek_en: VLEK (Version Loaded Endorsement Key) hashstick is loaded + * @feature_info: Indicates that the SNP_FEATURE_INFO command is available * @rsvd1: reserved * @guest_count: the number of guest currently managed by the firmware * @current_tcb_version: current TCB version @@ -229,7 +230,8 @@ struct sev_user_data_snp_status { __u32 mask_chip_id:1; /* Out */ __u32 mask_chip_key:1; /* Out */ __u32 vlek_en:1; /* Out */ - __u32 rsvd1:29; + __u32 feature_info:1; /* Out */ + __u32 rsvd1:28; __u32 guest_count; /* Out */ __u64 current_tcb_version; /* Out */ __u64 reported_tcb_version; /* Out */ diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c index f6eafde584d9..a49fe54b8dd8 100644 --- a/drivers/crypto/ccp/sev-dev.c +++ b/drivers/crypto/ccp/sev-dev.c @@ -223,6 +223,7 @@ static int sev_cmd_buffer_len(int cmd) case SEV_CMD_SNP_GUEST_REQUEST: return sizeof(struct sev_data_snp_guest_request); case SEV_CMD_SNP_CONFIG: return sizeof(struct sev_user_data_snp_config); case SEV_CMD_SNP_COMMIT: return sizeof(struct sev_data_snp_commit); + case SEV_CMD_SNP_FEATURE_INFO: return sizeof(struct sev_data_snp_feature_info); default: return 0; } @@ -1125,6 +1126,77 @@ static int snp_platform_status_locked(struct sev_device *sev, return ret; } +static int snp_feature_info_locked(struct sev_device *sev, u32 ecx, + struct sev_snp_feature_info *fi, int *psp_ret) +{ + struct sev_data_snp_feature_info buf = { + .length = sizeof(buf), + .ecx_in = ecx, + }; + struct page *status_page; + void *data; + int ret; + + status_page = alloc_page(GFP_KERNEL_ACCOUNT); + if (!status_page) + return -ENOMEM; + + data = page_address(status_page); + + if (sev->snp_initialized && rmp_mark_pages_firmware(__pa(data), 1, true)) { + ret = -EFAULT; + goto cleanup; + } + + buf.feature_info_paddr = __psp_pa(data); + ret = __sev_do_cmd_locked(SEV_CMD_SNP_FEATURE_INFO, &buf, psp_ret); + + if (sev->snp_initialized && snp_reclaim_pages(__pa(data), 1, true)) + ret = -EFAULT; + + if (!ret) + memcpy(fi, data, sizeof(*fi)); + +cleanup: + __free_pages(status_page, 0); + return ret; +} + +static int snp_get_feature_info(struct sev_device *sev, u32 ecx, struct sev_snp_feature_info *fi) +{ + struct sev_user_data_snp_status status = { 0 }; + int psp_ret = 0, ret; + + ret = snp_platform_status_locked(sev, &status, &psp_ret); + if (ret) + return ret; + if (ret != SEV_RET_SUCCESS) + return -EFAULT; + if (!status.feature_info) + return -ENOENT; + + ret = snp_feature_info_locked(sev, ecx, fi, &psp_ret); + if (ret) + return ret; + if (ret != SEV_RET_SUCCESS) + return -EFAULT; + + return 0; +} + +static bool sev_tio_present(struct sev_device *sev) +{ + struct sev_snp_feature_info fi = { 0 }; + bool present; + + if (snp_get_feature_info(sev, 0, &fi)) + return false; + + present = (fi.ebx & SNP_FEATURE_FN8000_0024_EBX_X00_SEVTIO) != 0; + dev_info(sev->dev, "SEV-TIO support is %s\n", present ? "present" : "not present"); + return present; +} + static int __sev_snp_init_locked(int *error) { struct psp_device *psp = psp_master; @@ -1189,6 +1261,7 @@ static int __sev_snp_init_locked(int *error) data.init_rmp = 1; data.list_paddr_en = 1; data.list_paddr = __psp_pa(snp_range_list); + data.tio_en = sev_tio_present(sev); cmd = SEV_CMD_SNP_INIT_EX; } else { cmd = SEV_CMD_SNP_INIT;
The PSP advertises the SEV-TIO support via the FEATURE_INFO command support of which is advertised via SNP_PLATFORM_STATUS. Add FEATURE_INFO and use it to detect the TIO support in the PSP. If present, enable TIO in the SNP_INIT_EX call. While at this, add new bits to sev_data_snp_init_ex() from SEV-SNP 1.55. Note that this tests the PSP firmware support but not if the feature is enabled in the BIOS. While at this, add new sev_data_snp_shutdown_ex::x86_snp_shutdown Signed-off-by: Alexey Kardashevskiy <aik@amd.com> --- include/linux/psp-sev.h | 31 ++++++++- include/uapi/linux/psp-sev.h | 4 +- drivers/crypto/ccp/sev-dev.c | 73 ++++++++++++++++++++ 3 files changed, 104 insertions(+), 4 deletions(-)