On 21/06/2024 14:40, Michael Roth wrote:
> From: Brijesh Singh <brijesh.singh@xxxxxxx>
>
> Version 2 of GHCB specification added support for the SNP Guest Request
> Message NAE event. The event allows for an SEV-SNP guest to make
> requests to the SEV-SNP firmware through hypervisor using the
> SNP_GUEST_REQUEST API defined in the SEV-SNP firmware specification.
>
> This is used by guests primarily to request attestation reports from
> firmware. There are other request types are available as well, but the
> specifics of what guest requests are being made are opaque to the
> hypervisor, which only serves as a proxy for the guest requests and
> firmware responses.
>
> Implement handling for these events.
>
> Signed-off-by: Brijesh Singh <brijesh.singh@xxxxxxx>
> Co-developed-by: Alexey Kardashevskiy <aik@xxxxxxx>
> Signed-off-by: Alexey Kardashevskiy <aik@xxxxxxx>
> Co-developed-by: Ashish Kalra <ashish.kalra@xxxxxxx>
> Signed-off-by: Ashish Kalra <ashish.kalra@xxxxxxx>
> Reviewed-by: Tom Lendacky <thomas.lendacky@xxxxxxx>
> [mdr: ensure FW command failures are indicated to guest, drop extended
> request handling to be re-written as separate patch, massage commit]
> Signed-off-by: Michael Roth <michael.roth@xxxxxxx>
> Message-ID: <20240501085210.2213060-19-michael.roth@xxxxxxx>
> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> ---
> arch/x86/kvm/svm/sev.c | 69 ++++++++++++++++++++++++++++++++++
> include/uapi/linux/sev-guest.h | 9 +++++
> 2 files changed, 78 insertions(+)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index df8818759698..7338b987cadd 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -19,6 +19,7 @@
> #include <linux/misc_cgroup.h>
> #include <linux/processor.h>
> #include <linux/trace_events.h>
> +#include <uapi/linux/sev-guest.h>
>
> #include <asm/pkru.h>
> #include <asm/trapnr.h>
> @@ -3321,6 +3322,10 @@ static int sev_es_validate_vmgexit(struct vcpu_svm *svm)
> if (!sev_snp_guest(vcpu->kvm) || !kvm_ghcb_sw_scratch_is_valid(svm))
> goto vmgexit_err;
> break;
> + case SVM_VMGEXIT_GUEST_REQUEST:
> + if (!sev_snp_guest(vcpu->kvm))
> + goto vmgexit_err;
> + break;
> default:
> reason = GHCB_ERR_INVALID_EVENT;
> goto vmgexit_err;
> @@ -3939,6 +3944,67 @@ static int sev_snp_ap_creation(struct vcpu_svm *svm)
> return ret;
> }
>
> +static int snp_handle_guest_req(struct vcpu_svm *svm, gpa_t req_gpa, gpa_t resp_gpa)
> +{
> + struct sev_data_snp_guest_request data = {0};
> + struct kvm *kvm = svm->vcpu.kvm;
> + kvm_pfn_t req_pfn, resp_pfn;
> + sev_ret_code fw_err = 0;
> + int ret;
> +
> + if (!sev_snp_guest(kvm) || !PAGE_ALIGNED(req_gpa) || !PAGE_ALIGNED(resp_gpa))
> + return -EINVAL;
> +
> + req_pfn = gfn_to_pfn(kvm, gpa_to_gfn(req_gpa));
> + if (is_error_noslot_pfn(req_pfn))
> + return -EINVAL;
> +
> + resp_pfn = gfn_to_pfn(kvm, gpa_to_gfn(resp_gpa));
> + if (is_error_noslot_pfn(resp_pfn)) {
> + ret = EINVAL;
> + goto release_req;
> + }
> +
> + if (rmp_make_private(resp_pfn, 0, PG_LEVEL_4K, 0, true)) {
> + ret = -EINVAL;
> + kvm_release_pfn_clean(resp_pfn);
> + goto release_req;
> + }
> +
> + data.gctx_paddr = __psp_pa(to_kvm_sev_info(kvm)->snp_context);
> + data.req_paddr = __sme_set(req_pfn << PAGE_SHIFT);
> + data.res_paddr = __sme_set(resp_pfn << PAGE_SHIFT);
> +
> + ret = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &data, &fw_err);
> + if (ret)
> + return ret;
Should this be goto release_req; ?
Does resp_pfn need to be dealt with as well?
> +
> + /*
> + * If reclaim fails then there's a good chance the guest will no longer
> + * be runnable so just let userspace terminate the guest.
> + */5
> + if (snp_page_reclaim(kvm, resp_pfn)) {
> + return -EIO;
Should this be setting ret = -EIO ? Next line is unreachable.
Does resp_pfn need to be dealt with as well?
> + goto release_req;
> + }
> +
> + /*
> + * As per GHCB spec, firmware failures should be communicated back to
> + * the guest via SW_EXITINFO2 rather than be treated as immediately
> + * fatal.
> + */
> + ghcb_set_sw_exit_info_2(svm->sev_es.ghcb,
> + SNP_GUEST_ERR(ret ? SNP_GUEST_VMM_ERR_GENERIC : 0,
> + fw_err));
> +
> + ret = 1; /* resume guest */
> + kvm_release_pfn_dirty(resp_pfn);
> +
> +release_req:
> + kvm_release_pfn_clean(req_pfn);
> + return ret;
> +}
> +
> static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm)
> {
> struct vmcb_control_area *control = &svm->vmcb->control;
> @@ -4213,6 +4279,9 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
>
> ret = 1;
> break;
> + case SVM_VMGEXIT_GUEST_REQUEST:
> + ret = snp_handle_guest_req(svm, control->exit_info_1, control->exit_info_2);
> + break;
> case SVM_VMGEXIT_UNSUPPORTED_EVENT:
> vcpu_unimpl(vcpu,
> "vmgexit: unsupported event - exit_info_1=%#llx, exit_info_2=%#llx\n",
> diff --git a/include/uapi/linux/sev-guest.h b/include/uapi/linux/sev-guest.h
> index 154a87a1eca9..7bd78e258569 100644
> --- a/include/uapi/linux/sev-guest.h
> +++ b/include/uapi/linux/sev-guest.h
> @@ -89,8 +89,17 @@ struct snp_ext_report_req {
> #define SNP_GUEST_FW_ERR_MASK GENMASK_ULL(31, 0)
> #define SNP_GUEST_VMM_ERR_SHIFT 32
> #define SNP_GUEST_VMM_ERR(x) (((u64)x) << SNP_GUEST_VMM_ERR_SHIFT)
> +#define SNP_GUEST_FW_ERR(x) ((x) & SNP_GUEST_FW_ERR_MASK)
> +#define SNP_GUEST_ERR(vmm_err, fw_err) (SNP_GUEST_VMM_ERR(vmm_err) | \
> + SNP_GUEST_FW_ERR(fw_err))
>
> +/*
> + * The GHCB spec only formally defines INVALID_LEN/BUSY VMM errors, but define
> + * a GENERIC error code such that it won't ever conflict with GHCB-defined
> + * errors if any get added in the future.
> + */
> #define SNP_GUEST_VMM_ERR_INVALID_LEN 1
> #define SNP_GUEST_VMM_ERR_BUSY 2
> +#define SNP_GUEST_VMM_ERR_GENERIC BIT(31)
>
> #endif /* __UAPI_LINUX_SEV_GUEST_H_ */
Regards,
Liam
