Re: [PATCH v9 39/43] x86/sev: Provide support for SNP guest request NAEs

[Peter Gonda <pgonda@google.com>]

On Fri, Jan 28, 2022 at 10:19 AM Brijesh Singh <brijesh.singh@amd.com> wrote:
>
> Version 2 of GHCB specification provides SNP_GUEST_REQUEST and
> SNP_EXT_GUEST_REQUEST NAE that can be used by the SNP guest to communicate
> with the PSP.
>
> While at it, add a snp_issue_guest_request() helper that will be used by
> driver or other subsystem to issue the request to PSP.
>
> See SEV-SNP firmware and GHCB spec for more details.
>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  arch/x86/include/asm/sev-common.h |  3 ++
>  arch/x86/include/asm/sev.h        | 14 ++++++++
>  arch/x86/include/uapi/asm/svm.h   |  4 +++
>  arch/x86/kernel/sev.c             | 55 +++++++++++++++++++++++++++++++
>  4 files changed, 76 insertions(+)
>
> diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
> index cd769984e929..442614879dad 100644
> --- a/arch/x86/include/asm/sev-common.h
> +++ b/arch/x86/include/asm/sev-common.h
> @@ -128,6 +128,9 @@ struct snp_psc_desc {
>         struct psc_entry entries[VMGEXIT_PSC_MAX_ENTRY];
>  } __packed;
>
> +/* Guest message request error code */
> +#define SNP_GUEST_REQ_INVALID_LEN      BIT_ULL(32)
> +
>  #define GHCB_MSR_TERM_REQ              0x100
>  #define GHCB_MSR_TERM_REASON_SET_POS   12
>  #define GHCB_MSR_TERM_REASON_SET_MASK  0xf
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index 219abb4590f2..9830ee1d6ef0 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -87,6 +87,14 @@ extern bool handle_vc_boot_ghcb(struct pt_regs *regs);
>
>  #define RMPADJUST_VMSA_PAGE_BIT                BIT(16)
>
> +/* SNP Guest message request */
> +struct snp_req_data {
> +       unsigned long req_gpa;
> +       unsigned long resp_gpa;
> +       unsigned long data_gpa;
> +       unsigned int data_npages;
> +};
> +
>  #ifdef CONFIG_AMD_MEM_ENCRYPT
>  extern struct static_key_false sev_es_enable_key;
>  extern void __sev_es_ist_enter(struct pt_regs *regs);
> @@ -154,6 +162,7 @@ void snp_set_memory_private(unsigned long vaddr, unsigned int npages);
>  void snp_set_wakeup_secondary_cpu(void);
>  bool snp_init(struct boot_params *bp);
>  void snp_abort(void);
> +int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsigned long *fw_err);
>  #else
>  static inline void sev_es_ist_enter(struct pt_regs *regs) { }
>  static inline void sev_es_ist_exit(void) { }
> @@ -173,6 +182,11 @@ static inline void snp_set_memory_private(unsigned long vaddr, unsigned int npag
>  static inline void snp_set_wakeup_secondary_cpu(void) { }
>  static inline bool snp_init(struct boot_params *bp) { return false; }
>  static inline void snp_abort(void) { }
> +static inline int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input,
> +                                         unsigned long *fw_err)
> +{
> +       return -ENOTTY;
> +}
>  #endif
>
>  #endif
> diff --git a/arch/x86/include/uapi/asm/svm.h b/arch/x86/include/uapi/asm/svm.h
> index 8b4c57baec52..5b8bc2b65a5e 100644
> --- a/arch/x86/include/uapi/asm/svm.h
> +++ b/arch/x86/include/uapi/asm/svm.h
> @@ -109,6 +109,8 @@
>  #define SVM_VMGEXIT_SET_AP_JUMP_TABLE          0
>  #define SVM_VMGEXIT_GET_AP_JUMP_TABLE          1
>  #define SVM_VMGEXIT_PSC                                0x80000010
> +#define SVM_VMGEXIT_GUEST_REQUEST              0x80000011
> +#define SVM_VMGEXIT_EXT_GUEST_REQUEST          0x80000012
>  #define SVM_VMGEXIT_AP_CREATION                        0x80000013
>  #define SVM_VMGEXIT_AP_CREATE_ON_INIT          0
>  #define SVM_VMGEXIT_AP_CREATE                  1
> @@ -225,6 +227,8 @@
>         { SVM_VMGEXIT_AP_HLT_LOOP,      "vmgexit_ap_hlt_loop" }, \
>         { SVM_VMGEXIT_AP_JUMP_TABLE,    "vmgexit_ap_jump_table" }, \
>         { SVM_VMGEXIT_PSC,      "vmgexit_page_state_change" }, \
> +       { SVM_VMGEXIT_GUEST_REQUEST,            "vmgexit_guest_request" }, \
> +       { SVM_VMGEXIT_EXT_GUEST_REQUEST,        "vmgexit_ext_guest_request" }, \
>         { SVM_VMGEXIT_AP_CREATION,      "vmgexit_ap_creation" }, \
>         { SVM_VMGEXIT_HV_FEATURES,      "vmgexit_hypervisor_feature" }, \
>         { SVM_EXIT_ERR,         "invalid_guest_state" }
> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
> index cb97200bfda7..1d3ac83226fc 100644
> --- a/arch/x86/kernel/sev.c
> +++ b/arch/x86/kernel/sev.c
> @@ -2122,3 +2122,58 @@ static int __init snp_check_cpuid_table(void)
>  }
>
>  arch_initcall(snp_check_cpuid_table);
> +
> +int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsigned long *fw_err)
> +{
> +       struct ghcb_state state;
> +       struct es_em_ctxt ctxt;
> +       unsigned long flags;
> +       struct ghcb *ghcb;
> +       int ret;
> +
> +       if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
> +               return -ENODEV;
> +
> +       /*
> +        * __sev_get_ghcb() needs to run with IRQs disabled because it is using
> +        * a per-CPU GHCB.
> +        */
> +       local_irq_save(flags);
> +
> +       ghcb = __sev_get_ghcb(&state);
> +       if (!ghcb) {
> +               ret = -EIO;
> +               goto e_restore_irq;
> +       }
> +
> +       vc_ghcb_invalidate(ghcb);
> +
> +       if (exit_code == SVM_VMGEXIT_EXT_GUEST_REQUEST) {
> +               ghcb_set_rax(ghcb, input->data_gpa);
> +               ghcb_set_rbx(ghcb, input->data_npages);
> +       }
> +
> +       ret = sev_es_ghcb_hv_call(ghcb, true, &ctxt, exit_code, input->req_gpa, input->resp_gpa);
> +       if (ret)
> +               goto e_put;
> +
> +       if (ghcb->save.sw_exit_info_2) {
> +               /* Number of expected pages are returned in RBX */
> +               if (exit_code == SVM_VMGEXIT_EXT_GUEST_REQUEST &&
> +                   ghcb->save.sw_exit_info_2 == SNP_GUEST_REQ_INVALID_LEN)
> +                       input->data_npages = ghcb_get_rbx(ghcb);
> +
> +               if (fw_err)
> +                       *fw_err = ghcb->save.sw_exit_info_2;

In the PSP driver we've had a bit of discussion around the fw_err and
the return code and that it would be preferable to have fw_err be a
required parameter. And then we can easily make sure fw_err is always
non-zero when the return code is non-zero. Thoughts about doing the
same inside the guest?


> +
> +               ret = -EIO;
> +       }
> +
> +e_put:
> +       __sev_put_ghcb(&state);
> +e_restore_irq:
> +       local_irq_restore(flags);
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(snp_issue_guest_request);
> --
> 2.25.1
>