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

[Brijesh Singh <brijesh.ksingh@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6846 bytes --]

On Tue, Feb 1, 2022 at 2:17 PM Peter Gonda <pgonda@google.com> wrote:

> 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?
>
>
As per the GHCB spec, we will always have a non-zero error code. So, yes, I
can drop the if() check.

FYI, somehow this email did not show up on my @amd.com so I did not address
it in v10.

thanks

[-- Attachment #2: Type: text/html, Size: 8805 bytes --]