| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ef8f247e-a685-4097-aaff-fa8fcafa91c5@amd.com>
Date: Thu, 2 Jan 2025 08:09:11 -0600
From: "Paluri, PavanKumar" <papaluri@....com>
To: Melody Wang <huibo.wang@....com>, Sean Christopherson <seanjc@...gle.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, KVM <kvm@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>, Tom Lendacky <thomas.lendacky@....com>,
Dhaval Giani <dhaval.giani@....com>,
"Paluri, PavanKumar (Pavan Kumar)" <pavankumar.paluri@....com>
Subject: Re: [PATCH v4 2/2] KVM: SVM: Provide helpers to set the error code
On 12/30/2024 2:36 PM, Melody Wang wrote:
> Provide helpers to set the error code when converting VMGEXIT SW_EXITINFO1 and
> SW_EXITINFO2 codes from plain numbers to proper defines. Add comments for
> better code readability.
>
> No functionality changed.
>
> Suggested-by: Sean Christopherson <seanjc@...gle.com>
> Signed-off-by: Melody Wang <huibo.wang@....com>
> ---
> arch/x86/kvm/svm/sev.c | 36 +++++++++++++++++++-----------------
> arch/x86/kvm/svm/svm.c | 6 +-----
> arch/x86/kvm/svm/svm.h | 29 +++++++++++++++++++++++++++++
> 3 files changed, 49 insertions(+), 22 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 59a0d8292f87..6e2a0f0b4753 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -3420,8 +3420,7 @@ static int sev_es_validate_vmgexit(struct vcpu_svm *svm)
> dump_ghcb(svm);
> }
>
> - ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, GHCB_HV_RESP_MALFORMED_INPUT);
> - ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, reason);
> + svm_vmgexit_bad_input(svm, reason);
>
> /* Resume the guest to "return" the error code. */
> return 1;
> @@ -3564,8 +3563,7 @@ static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
> return 0;
>
> e_scratch:
> - ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, GHCB_HV_RESP_MALFORMED_INPUT);
> - ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, GHCB_ERR_INVALID_SCRATCH_AREA);
> + svm_vmgexit_bad_input(svm, GHCB_ERR_INVALID_SCRATCH_AREA);
>
> return 1;
> }
> @@ -3658,7 +3656,14 @@ static void snp_complete_psc(struct vcpu_svm *svm, u64 psc_ret)
> svm->sev_es.psc_inflight = 0;
> svm->sev_es.psc_idx = 0;
> svm->sev_es.psc_2m = false;
> - ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, psc_ret);
> +
> + /*
> + * A value of zero in SW_EXITINFO1 does not guarantee that
> + * all operations have completed or completed successfully.
> + * PSC requests always get a "no action" response, with a
> + * PSC-specific return code in SW_EXITINFO2.
> + */
I feel you should make this more clearer by mentioning "no action"
corresponds to SW_EXITINFO1.
> + svm_vmgexit_no_action(svm, psc_ret);
> }
>
> static void __snp_complete_one_psc(struct vcpu_svm *svm)
> @@ -4055,7 +4060,7 @@ static int snp_handle_guest_req(struct vcpu_svm *svm, gpa_t req_gpa, gpa_t resp_
> goto out_unlock;
> }
>
> - ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, SNP_GUEST_ERR(0, fw_err));
> + svm_vmgexit_no_action(svm, SNP_GUEST_ERR(0, fw_err));
>
You missed out on Sean's suggestion to add the following comment.
/*
* No action is requested from KVM, even if the request failed due to a
* firmware error.
*/
I feel this comment is necessary since you are calling
svm_vmgexit_no_action() that sheds light on SW_EXITINFO1.
Thanks,
Pavan
> ret = 1; /* resume guest */
>
> @@ -4111,8 +4116,7 @@ static int snp_handle_ext_guest_req(struct vcpu_svm *svm, gpa_t req_gpa, gpa_t r
> return snp_handle_guest_req(svm, req_gpa, resp_gpa);
>
> request_invalid:
> - ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, GHCB_HV_RESP_MALFORMED_INPUT);
> - ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, GHCB_ERR_INVALID_INPUT);
> + svm_vmgexit_bad_input(svm, GHCB_ERR_INVALID_INPUT);
> return 1; /* resume guest */
> }
>
> @@ -4304,8 +4308,7 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
> if (ret)
> return ret;
>
> - ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, GHCB_HV_RESP_NO_ACTION);
> - ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, 0);
> + svm_vmgexit_success(svm, 0);
>
> exit_code = kvm_ghcb_get_sw_exit_code(control);
> switch (exit_code) {
> @@ -4349,20 +4352,20 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
> break;
> case 1:
> /* Get AP jump table address */
> - ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, sev->ap_jump_table);
> + svm_vmgexit_success(svm, sev->ap_jump_table);
> break;
> default:
> pr_err("svm: vmgexit: unsupported AP jump table request - exit_info_1=%#llx\n",
> control->exit_info_1);
> - ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, GHCB_HV_RESP_MALFORMED_INPUT);
> - ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, GHCB_ERR_INVALID_INPUT);
> + svm_vmgexit_bad_input(svm, GHCB_ERR_INVALID_INPUT);
> }
>
> ret = 1;
> break;
> }
> case SVM_VMGEXIT_HV_FEATURES:
> - ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, GHCB_HV_FT_SUPPORTED);
> + /* Get hypervisor supported features */
> + svm_vmgexit_success(svm, GHCB_HV_FT_SUPPORTED);
>
> ret = 1;
> break;
> @@ -4384,8 +4387,7 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
> case SVM_VMGEXIT_AP_CREATION:
> ret = sev_snp_ap_creation(svm);
> if (ret) {
> - ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, GHCB_HV_RESP_MALFORMED_INPUT);
> - ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, GHCB_ERR_INVALID_INPUT);
> + svm_vmgexit_bad_input(svm, GHCB_ERR_INVALID_INPUT);
> }
>
> ret = 1;
> @@ -4622,7 +4624,7 @@ void sev_vcpu_deliver_sipi_vector(struct kvm_vcpu *vcpu, u8 vector)
> * Return from an AP Reset Hold VMGEXIT, where the guest will
> * set the CS and RIP. Set SW_EXIT_INFO_2 to a non-zero value.
> */
> - ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, 1);
> + svm_vmgexit_success(svm, 1);
> break;
> case AP_RESET_HOLD_MSR_PROTO:
> /*
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 0de2bf132056..f8c5c78b917e 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -2977,11 +2977,7 @@ static int svm_complete_emulated_msr(struct kvm_vcpu *vcpu, int err)
> if (!err || !sev_es_guest(vcpu->kvm) || WARN_ON_ONCE(!svm->sev_es.ghcb))
> return kvm_complete_insn_gp(vcpu, err);
>
> - ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, GHCB_HV_RESP_ISSUE_EXCEPTION);
> - ghcb_set_sw_exit_info_2(svm->sev_es.ghcb,
> - X86_TRAP_GP |
> - SVM_EVTINJ_TYPE_EXEPT |
> - SVM_EVTINJ_VALID);
> + svm_vmgexit_inject_exception(svm, X86_TRAP_GP);
> return 1;
> }
>
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 43fa6a16eb19..78c8b5fb2bdc 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -588,6 +588,35 @@ static inline bool is_vnmi_enabled(struct vcpu_svm *svm)
> return false;
> }
>
> +static inline void svm_vmgexit_set_return_code(struct vcpu_svm *svm,
> + u64 response, u64 data)
> +{
> + ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, response);
> + ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, data);
> +}
> +
> +static inline void svm_vmgexit_inject_exception(struct vcpu_svm *svm, u8 vector)
> +{
> + u64 data = SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_EXEPT | vector;
> +
> + svm_vmgexit_set_return_code(svm, GHCB_HV_RESP_ISSUE_EXCEPTION, data);
> +}
> +
> +static inline void svm_vmgexit_bad_input(struct vcpu_svm *svm, u64 suberror)
> +{
> + svm_vmgexit_set_return_code(svm, GHCB_HV_RESP_MALFORMED_INPUT, suberror);
> +}
> +
> +static inline void svm_vmgexit_success(struct vcpu_svm *svm, u64 data)
> +{
> + svm_vmgexit_set_return_code(svm, GHCB_HV_RESP_NO_ACTION, data);
> +}
> +
> +static inline void svm_vmgexit_no_action(struct vcpu_svm *svm, u64 data)
> +{
> + svm_vmgexit_set_return_code(svm, GHCB_HV_RESP_NO_ACTION, data);
> +}
> +
> /* svm.c */
> #define MSR_INVALID 0xffffffffU
>
Powered by blists - more mailing lists