| Message ID | a63de5e687c530849312099ee02007089b67e92f.1655761627.git.ashish.kalra@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) | expand |
( On Mon, Jun 20, 2022 at 5:05 PM Ashish Kalra <Ashish.Kalra@amd.com> wrote: > > From: Brijesh Singh <brijesh.singh@amd.com> > > Provide the APIs for the hypervisor to manage an SEV-SNP guest. The > commands for SEV-SNP is defined in the SEV-SNP firmware specification. > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > drivers/crypto/ccp/sev-dev.c | 24 ++++++++++++ > include/linux/psp-sev.h | 73 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 97 insertions(+) > > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c > index f1173221d0b9..35d76333e120 100644 > --- a/drivers/crypto/ccp/sev-dev.c > +++ b/drivers/crypto/ccp/sev-dev.c > @@ -1205,6 +1205,30 @@ int sev_guest_df_flush(int *error) > } > EXPORT_SYMBOL_GPL(sev_guest_df_flush); > > +int snp_guest_decommission(struct sev_data_snp_decommission *data, int *error) > +{ > + return sev_do_cmd(SEV_CMD_SNP_DECOMMISSION, data, error); > +} > +EXPORT_SYMBOL_GPL(snp_guest_decommission); > + > +int snp_guest_df_flush(int *error) > +{ > + return sev_do_cmd(SEV_CMD_SNP_DF_FLUSH, NULL, error); > +} > +EXPORT_SYMBOL_GPL(snp_guest_df_flush); Why not instead change sev_guest_df_flush() to be SNP aware? That way callers get the right behavior without having to know if SNP is enabled or not. int sev_guest_df_flush(int *error) { if (!psp_master || !psp_master->sev_data) return -EINVAL; if (psp_master->sev_data->snp_inited) return sev_do_cmd(SEV_CMD_SNP_DF_FLUSH, NULL, error); return sev_do_cmd(SEV_CMD_DF_FLUSH, NULL, error); } > +int snp_guest_page_reclaim(struct sev_data_snp_page_reclaim *data, int *error) > +{ > + return sev_do_cmd(SEV_CMD_SNP_PAGE_RECLAIM, data, error); > +} > +EXPORT_SYMBOL_GPL(snp_guest_page_reclaim); > + > +int snp_guest_dbg_decrypt(struct sev_data_snp_dbg *data, int *error) > +{ > + return sev_do_cmd(SEV_CMD_SNP_DBG_DECRYPT, data, error); > +} > +EXPORT_SYMBOL_GPL(snp_guest_dbg_decrypt); > + > static void sev_exit(struct kref *ref) > { > misc_deregister(&misc_dev->misc); > diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h > index ef4d42e8c96e..9f921d221b75 100644 > --- a/include/linux/psp-sev.h > +++ b/include/linux/psp-sev.h > @@ -881,6 +881,64 @@ int sev_guest_df_flush(int *error); > */ > int sev_guest_decommission(struct sev_data_decommission *data, int *error); > > +/** > + * snp_guest_df_flush - perform SNP DF_FLUSH command > + * > + * @sev_ret: sev command return code > + * > + * Returns: > + * 0 if the sev successfully processed the command > + * -%ENODEV if the sev device is not available > + * -%ENOTSUPP if the sev does not support SEV > + * -%ETIMEDOUT if the sev command timed out > + * -%EIO if the sev returned a non-zero return code > + */ > +int snp_guest_df_flush(int *error); > + > +/** > + * snp_guest_decommission - perform SNP_DECOMMISSION command > + * > + * @decommission: sev_data_decommission structure to be processed > + * @sev_ret: sev command return code > + * > + * Returns: > + * 0 if the sev successfully processed the command > + * -%ENODEV if the sev device is not available > + * -%ENOTSUPP if the sev does not support SEV > + * -%ETIMEDOUT if the sev command timed out > + * -%EIO if the sev returned a non-zero return code > + */ > +int snp_guest_decommission(struct sev_data_snp_decommission *data, int *error); > + > +/** > + * snp_guest_page_reclaim - perform SNP_PAGE_RECLAIM command > + * > + * @decommission: sev_snp_page_reclaim structure to be processed > + * @sev_ret: sev command return code > + * > + * Returns: > + * 0 if the sev successfully processed the command > + * -%ENODEV if the sev device is not available > + * -%ENOTSUPP if the sev does not support SEV > + * -%ETIMEDOUT if the sev command timed out > + * -%EIO if the sev returned a non-zero return code > + */ > +int snp_guest_page_reclaim(struct sev_data_snp_page_reclaim *data, int *error); > + > +/** > + * snp_guest_dbg_decrypt - perform SEV SNP_DBG_DECRYPT command > + * > + * @sev_ret: sev command return code > + * > + * Returns: > + * 0 if the sev successfully processed the command > + * -%ENODEV if the sev device is not available > + * -%ENOTSUPP if the sev does not support SEV > + * -%ETIMEDOUT if the sev command timed out > + * -%EIO if the sev returned a non-zero return code > + */ > +int snp_guest_dbg_decrypt(struct sev_data_snp_dbg *data, int *error); > + > void *psp_copy_user_blob(u64 uaddr, u32 len); > > #else /* !CONFIG_CRYPTO_DEV_SP_PSP */ > @@ -908,6 +966,21 @@ sev_issue_cmd_external_user(struct file *filep, unsigned int id, void *data, int > > static inline void *psp_copy_user_blob(u64 __user uaddr, u32 len) { return ERR_PTR(-EINVAL); } > > +static inline int > +snp_guest_decommission(struct sev_data_snp_decommission *data, int *error) { return -ENODEV; } > + > +static inline int snp_guest_df_flush(int *error) { return -ENODEV; } > + > +static inline int snp_guest_page_reclaim(struct sev_data_snp_page_reclaim *data, int *error) > +{ > + return -ENODEV; > +} > + > +static inline int snp_guest_dbg_decrypt(struct sev_data_snp_dbg *data, int *error) > +{ > + return -ENODEV; > +} > + > #endif /* CONFIG_CRYPTO_DEV_SP_PSP */ > > #endif /* __PSP_SEV_H__ */ > -- > 2.25.1 >
[Public] >> +EXPORT_SYMBOL_GPL(snp_guest_decommission); >> + >> +int snp_guest_df_flush(int *error) >> +{ >> + return sev_do_cmd(SEV_CMD_SNP_DF_FLUSH, NULL, error); } >> +EXPORT_SYMBOL_GPL(snp_guest_df_flush); >Why not instead change sev_guest_df_flush() to be SNP aware? That way callers get the right behavior without having to know if SNP is enabled or not. It can be done, and actually both DF_FLUSH commands do exactly the same thing. But as with other API interfaces here, I think it is better to differentiate between snp and sev API interfaces and the callers be aware of which interface they are invoking. Thanks, Ashish
On Tue, Jun 21, 2022 at 03:43:13PM -0600, Peter Gonda wrote: > ( > > On Mon, Jun 20, 2022 at 5:05 PM Ashish Kalra <Ashish.Kalra@amd.com> wrote: > > > > From: Brijesh Singh <brijesh.singh@amd.com> > > > > Provide the APIs for the hypervisor to manage an SEV-SNP guest. The > > commands for SEV-SNP is defined in the SEV-SNP firmware specification. > > > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > > --- > > drivers/crypto/ccp/sev-dev.c | 24 ++++++++++++ > > include/linux/psp-sev.h | 73 ++++++++++++++++++++++++++++++++++++ > > 2 files changed, 97 insertions(+) > > > > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c > > index f1173221d0b9..35d76333e120 100644 > > --- a/drivers/crypto/ccp/sev-dev.c > > +++ b/drivers/crypto/ccp/sev-dev.c > > @@ -1205,6 +1205,30 @@ int sev_guest_df_flush(int *error) > > } > > EXPORT_SYMBOL_GPL(sev_guest_df_flush); > > > > +int snp_guest_decommission(struct sev_data_snp_decommission *data, int *error) > > +{ > > + return sev_do_cmd(SEV_CMD_SNP_DECOMMISSION, data, error); > > +} > > +EXPORT_SYMBOL_GPL(snp_guest_decommission); > > + > > +int snp_guest_df_flush(int *error) > > +{ > > + return sev_do_cmd(SEV_CMD_SNP_DF_FLUSH, NULL, error); > > +} > > +EXPORT_SYMBOL_GPL(snp_guest_df_flush); Nit: undocumented exported functions. Both need kdoc. > > Why not instead change sev_guest_df_flush() to be SNP aware? That way > callers get the right behavior without having to know if SNP is > enabled or not. > > int sev_guest_df_flush(int *error) > { > if (!psp_master || !psp_master->sev_data) > return -EINVAL; > > if (psp_master->sev_data->snp_inited) > return sev_do_cmd(SEV_CMD_SNP_DF_FLUSH, NULL, error); > > return sev_do_cmd(SEV_CMD_DF_FLUSH, NULL, error); > } Because it serves no purpose to fuse them into one, and is only more obfuscated (and also undocumented). Two exported symbols can be traced also separately with ftrace/kprobes. Degrading transparency is not great idea IMHO. BR, Jarkko
On Mon, Jun 20, 2022 at 11:04:45PM +0000, Ashish Kalra wrote: > From: Brijesh Singh <brijesh.singh@amd.com> > > Provide the APIs for the hypervisor to manage an SEV-SNP guest. The > commands for SEV-SNP is defined in the SEV-SNP firmware specification. > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > drivers/crypto/ccp/sev-dev.c | 24 ++++++++++++ > include/linux/psp-sev.h | 73 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 97 insertions(+) > > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c > index f1173221d0b9..35d76333e120 100644 > --- a/drivers/crypto/ccp/sev-dev.c > +++ b/drivers/crypto/ccp/sev-dev.c > @@ -1205,6 +1205,30 @@ int sev_guest_df_flush(int *error) > } > EXPORT_SYMBOL_GPL(sev_guest_df_flush); > > +int snp_guest_decommission(struct sev_data_snp_decommission *data, int *error) > +{ > + return sev_do_cmd(SEV_CMD_SNP_DECOMMISSION, data, error); > +} > +EXPORT_SYMBOL_GPL(snp_guest_decommission); > + > +int snp_guest_df_flush(int *error) > +{ > + return sev_do_cmd(SEV_CMD_SNP_DF_FLUSH, NULL, error); > +} > +EXPORT_SYMBOL_GPL(snp_guest_df_flush); > + > +int snp_guest_page_reclaim(struct sev_data_snp_page_reclaim *data, int *error) > +{ > + return sev_do_cmd(SEV_CMD_SNP_PAGE_RECLAIM, data, error); > +} > +EXPORT_SYMBOL_GPL(snp_guest_page_reclaim); > + > +int snp_guest_dbg_decrypt(struct sev_data_snp_dbg *data, int *error) > +{ > + return sev_do_cmd(SEV_CMD_SNP_DBG_DECRYPT, data, error); > +} > +EXPORT_SYMBOL_GPL(snp_guest_dbg_decrypt); So this mindless repetition is getting annoying. I see ~70 SEV commands. Adding ~70 functions which parrot all the same call to sev_do_cmd() is just insane. I think you should simply export sev_do_cmd() and call it instead. Yes, when it turns out that a command and the preparation to issue it before it starts repeating pretty often, you could do a helper. But adding those silly wrappers doesn't bring anything besides confusion and code bloat. Thx.
[AMD Official Use Only - General] Hello Boris, >> +int snp_guest_decommission(struct sev_data_snp_decommission *data, >> +int *error) { >> + return sev_do_cmd(SEV_CMD_SNP_DECOMMISSION, data, error); } >> +EXPORT_SYMBOL_GPL(snp_guest_decommission); >> + >> +int snp_guest_df_flush(int *error) >> +{ >> + return sev_do_cmd(SEV_CMD_SNP_DF_FLUSH, NULL, error); } >> +EXPORT_SYMBOL_GPL(snp_guest_df_flush); >> + >> +int snp_guest_page_reclaim(struct sev_data_snp_page_reclaim *data, >> +int *error) { >> + return sev_do_cmd(SEV_CMD_SNP_PAGE_RECLAIM, data, error); } >> +EXPORT_SYMBOL_GPL(snp_guest_page_reclaim); >> + >> +int snp_guest_dbg_decrypt(struct sev_data_snp_dbg *data, int *error) >> +{ >> + return sev_do_cmd(SEV_CMD_SNP_DBG_DECRYPT, data, error); } >> +EXPORT_SYMBOL_GPL(snp_guest_dbg_decrypt); >So this mindless repetition is getting annoying. I see ~70 SEV commands. >Adding ~70 functions which parrot all the same call to sev_do_cmd() is just insane. >I think you should simply export sev_do_cmd() and call it instead. >Yes, when it turns out that a command and the preparation to issue it before it starts repeating pretty often, you could do a helper. But adding those silly wrappers doesn't bring anything besides confusion and code bloat. There are actually only 8 functions in total which are simply calling sev_do_cmd(), all other functions calling sev_do_cmd() have a lot of other specific functionality in them. Those are the earlier - sev_platform_status(), sev_guest_deactivate(), sev_guest_decomission() and sev_guest_df_flush(). And the 4 functions added in this patch - snp_guest_decomission(), snp_guest_df_flush(), snp_guest_page_reclaim(), and snp_guest_dbg_decrypt(). Thanks, Ashish
On Mon, Oct 03, 2022 at 02:38:41PM +0000, Kalra, Ashish wrote:
> There are actually only 8 functions
Only 8?
Lemme ask it differently then: what is the point of the wrappers at all?
[AMD Official Use Only - General] >> There are actually only 8 functions >Only 8? >Lemme ask it differently then: what is the point of the wrappers at all? They are basically providing the APIs for the hypervisor to manage a SNP guest. And this is the original commit for the API to manage the SEV guest: commit 200664d5237f3f8cd2a2f9f5c5dea08502336bd1 Author: Brijesh Singh <brijesh.singh@amd.com> Date: Mon Dec 4 10:57:28 2017 -0600 crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support AMD's new Secure Encrypted Virtualization (SEV) feature allows the memory contents of virtual machines to be transparently encrypted with a key unique to the VM. The programming and management of the encryption keys are handled by the AMD Secure Processor (AMD-SP) which exposes the commands for these tasks. The complete spec is available at: http://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf Extend the AMD-SP driver to provide the following support: - an in-kernel API to communicate with the SEV firmware. The API can be used by the hypervisor to create encryption context for a SEV guest. - a userspace IOCTL to manage the platform certificates. Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: "Radim Krčmář" <rkrcmar@redhat.com> Cc: Borislav Petkov <bp@suse.de> Cc: Herbert Xu <herbert@gondor.apana.org.au> Cc: Gary Hook <gary.hook@amd.com> Cc: Tom Lendacky <thomas.lendacky@amd.com> Cc: linux-crypto@vger.kernel.org Cc: kvm@vger.kernel.org Cc: linux-kernel@vger.kernel.org Improvements-by: Borislav Petkov <bp@suse.de> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> Thanks, Ashish
On Mon, Oct 03, 2022 at 05:11:05PM +0000, Kalra, Ashish wrote: > They are basically providing the APIs for the hypervisor to manage a > SNP guest. Yes, I know. But that is not my question. Lemme try again. My previous comment was: "I think you should simply export sev_do_cmd() and call it instead." In this case, the API is a single function - sev_do_cmd() - which the hypervisor calls. So my question still stands: why is it better to have silly wrappers of sev_do_cmd() instead of having the hypervisor call sev_do_cmd() directly?
On Mon, Oct 3, 2022 at 11:45 AM Borislav Petkov <bp@alien8.de> wrote: > > On Mon, Oct 03, 2022 at 05:11:05PM +0000, Kalra, Ashish wrote: > > They are basically providing the APIs for the hypervisor to manage a > > SNP guest. > > Yes, I know. But that is not my question. Lemme try again. > > My previous comment was: > > "I think you should simply export sev_do_cmd() and call it instead." > > In this case, the API is a single function - sev_do_cmd() - which the > hypervisor calls. > > So my question still stands: why is it better to have silly wrappers > of sev_do_cmd() instead of having the hypervisor call sev_do_cmd() > directly? We already have sev_issue_cmd_external_user() exported right? Another option could be to make these wrappers more helpful and less silly. For example callers need to know the PSP command format right now, see sev_guest_decommission(). int sev_guest_decommission(struct sev_data_decommission *data, int *error) Instead of taking @data this function could just take inputs to create sev_data_decommission: int sev_guest_decommission(u32 handle, int *error) > > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette
On Mon, Oct 03, 2022 at 12:01:19PM -0600, Peter Gonda wrote: > We already have sev_issue_cmd_external_user() exported right? > > Another option could be to make these wrappers more helpful and less > silly. For example. My point is, whenever something needs to issue a SEV* fw command, something adds a silly wrapper and this will become unwieldy pretty quickly. If a wrapper is not a dumb one but it actually does preparatory work before issuing the command so that the caller's life is made easy, then yes, by all means. If it is only plain forwarding a call to sev_do_cmd(), then I question the whole point of the exercise...
[AMD Official Use Only - General] >> We already have sev_issue_cmd_external_user() exported right? >> >> Another option could be to make these wrappers more helpful and less >> silly. >For example. >My point is, whenever something needs to issue a SEV* fw command, something adds a silly wrapper and this will become unwieldy pretty quickly. >If a wrapper is not a dumb one but it actually does preparatory work before issuing the command so that the caller's life is made easy, then yes, by all means. >If it is only plain forwarding a call to sev_do_cmd(), then I question the whole point of the exercise... Well, these all were added as APIs to serve as a abstraction for SEV/SNP guest, and probably it is nice to have an abstracted interface, but I have no issues with replacing these simply with calls to sev_do_cmd(). Thanks, Ashish
On Mon, Oct 03, 2022 at 06:43:08PM +0000, Kalra, Ashish wrote:
> probably it is nice to have an abstracted interface,
Why is it "probably nice" to have an abstracted interface?
Is the hypervisor allowed to issue only a subset of the commands?
Do you want to control the arguments the hypervisor is supposed to send
down to the firmware?
There must be a reason why one would do an abstracted interface. Not
just because and probably.
Because from where I'm standing this looks like adding a bunch of random
wrappers without any logic to it.
So, if you wanna have an interface, you should think this through and
design it properly and explain why it is there and how it is supposed to
be used.
Don't get me wrong - a properly designed interface to control what the
HV issues to the firmware is not a bad idea. But it needs to be properly
designed.
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c index f1173221d0b9..35d76333e120 100644 --- a/drivers/crypto/ccp/sev-dev.c +++ b/drivers/crypto/ccp/sev-dev.c @@ -1205,6 +1205,30 @@ int sev_guest_df_flush(int *error) } EXPORT_SYMBOL_GPL(sev_guest_df_flush); +int snp_guest_decommission(struct sev_data_snp_decommission *data, int *error) +{ + return sev_do_cmd(SEV_CMD_SNP_DECOMMISSION, data, error); +} +EXPORT_SYMBOL_GPL(snp_guest_decommission); + +int snp_guest_df_flush(int *error) +{ + return sev_do_cmd(SEV_CMD_SNP_DF_FLUSH, NULL, error); +} +EXPORT_SYMBOL_GPL(snp_guest_df_flush); + +int snp_guest_page_reclaim(struct sev_data_snp_page_reclaim *data, int *error) +{ + return sev_do_cmd(SEV_CMD_SNP_PAGE_RECLAIM, data, error); +} +EXPORT_SYMBOL_GPL(snp_guest_page_reclaim); + +int snp_guest_dbg_decrypt(struct sev_data_snp_dbg *data, int *error) +{ + return sev_do_cmd(SEV_CMD_SNP_DBG_DECRYPT, data, error); +} +EXPORT_SYMBOL_GPL(snp_guest_dbg_decrypt); + static void sev_exit(struct kref *ref) { misc_deregister(&misc_dev->misc); diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h index ef4d42e8c96e..9f921d221b75 100644 --- a/include/linux/psp-sev.h +++ b/include/linux/psp-sev.h @@ -881,6 +881,64 @@ int sev_guest_df_flush(int *error); */ int sev_guest_decommission(struct sev_data_decommission *data, int *error); +/** + * snp_guest_df_flush - perform SNP DF_FLUSH command + * + * @sev_ret: sev command return code + * + * Returns: + * 0 if the sev successfully processed the command + * -%ENODEV if the sev device is not available + * -%ENOTSUPP if the sev does not support SEV + * -%ETIMEDOUT if the sev command timed out + * -%EIO if the sev returned a non-zero return code + */ +int snp_guest_df_flush(int *error); + +/** + * snp_guest_decommission - perform SNP_DECOMMISSION command + * + * @decommission: sev_data_decommission structure to be processed + * @sev_ret: sev command return code + * + * Returns: + * 0 if the sev successfully processed the command + * -%ENODEV if the sev device is not available + * -%ENOTSUPP if the sev does not support SEV + * -%ETIMEDOUT if the sev command timed out + * -%EIO if the sev returned a non-zero return code + */ +int snp_guest_decommission(struct sev_data_snp_decommission *data, int *error); + +/** + * snp_guest_page_reclaim - perform SNP_PAGE_RECLAIM command + * + * @decommission: sev_snp_page_reclaim structure to be processed + * @sev_ret: sev command return code + * + * Returns: + * 0 if the sev successfully processed the command + * -%ENODEV if the sev device is not available + * -%ENOTSUPP if the sev does not support SEV + * -%ETIMEDOUT if the sev command timed out + * -%EIO if the sev returned a non-zero return code + */ +int snp_guest_page_reclaim(struct sev_data_snp_page_reclaim *data, int *error); + +/** + * snp_guest_dbg_decrypt - perform SEV SNP_DBG_DECRYPT command + * + * @sev_ret: sev command return code + * + * Returns: + * 0 if the sev successfully processed the command + * -%ENODEV if the sev device is not available + * -%ENOTSUPP if the sev does not support SEV + * -%ETIMEDOUT if the sev command timed out + * -%EIO if the sev returned a non-zero return code + */ +int snp_guest_dbg_decrypt(struct sev_data_snp_dbg *data, int *error); + void *psp_copy_user_blob(u64 uaddr, u32 len); #else /* !CONFIG_CRYPTO_DEV_SP_PSP */ @@ -908,6 +966,21 @@ sev_issue_cmd_external_user(struct file *filep, unsigned int id, void *data, int static inline void *psp_copy_user_blob(u64 __user uaddr, u32 len) { return ERR_PTR(-EINVAL); } +static inline int +snp_guest_decommission(struct sev_data_snp_decommission *data, int *error) { return -ENODEV; } + +static inline int snp_guest_df_flush(int *error) { return -ENODEV; } + +static inline int snp_guest_page_reclaim(struct sev_data_snp_page_reclaim *data, int *error) +{ + return -ENODEV; +} + +static inline int snp_guest_dbg_decrypt(struct sev_data_snp_dbg *data, int *error) +{ + return -ENODEV; +} + #endif /* CONFIG_CRYPTO_DEV_SP_PSP */ #endif /* __PSP_SEV_H__ */