On Thu, Sep 30, 2021 at 09:05:41AM -0400, Tianyu Lan wrote:
> diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
> index 9f90f460a28c..dd7f37de640b 100644
> --- a/arch/x86/kernel/sev-shared.c
> +++ b/arch/x86/kernel/sev-shared.c
> @@ -94,10 +94,9 @@ static void vc_finish_insn(struct es_em_ctxt *ctxt)
> ctxt->regs->ip += ctxt->insn.length;
> }
>
> -static enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb,
> - struct es_em_ctxt *ctxt,
> - u64 exit_code, u64 exit_info_1,
> - u64 exit_info_2)
> +enum es_result sev_es_ghcb_hv_call_simple(struct ghcb *ghcb,
> + u64 exit_code, u64 exit_info_1,
> + u64 exit_info_2)
Align arguments on the opening brace.
Also, there's nothing "simple" about it - what you've carved out does
the actual HV call and the trailing part is verifying the HV info. So
that function should be called
__sev_es_ghcb_hv_call()
and the outer one without the "__".
> {
> enum es_result ret;
>
> @@ -109,29 +108,45 @@ static enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb,
> ghcb_set_sw_exit_info_1(ghcb, exit_info_1);
> ghcb_set_sw_exit_info_2(ghcb, exit_info_2);
>
> - sev_es_wr_ghcb_msr(__pa(ghcb));
> VMGEXIT();
>
> - if ((ghcb->save.sw_exit_info_1 & 0xffffffff) == 1) {
> - u64 info = ghcb->save.sw_exit_info_2;
> - unsigned long v;
> -
> - info = ghcb->save.sw_exit_info_2;
> - v = info & SVM_EVTINJ_VEC_MASK;
> -
> - /* Check if exception information from hypervisor is sane. */
> - if ((info & SVM_EVTINJ_VALID) &&
> - ((v == X86_TRAP_GP) || (v == X86_TRAP_UD)) &&
> - ((info & SVM_EVTINJ_TYPE_MASK) == SVM_EVTINJ_TYPE_EXEPT)) {
> - ctxt->fi.vector = v;
> - if (info & SVM_EVTINJ_VALID_ERR)
> - ctxt->fi.error_code = info >> 32;
> - ret = ES_EXCEPTION;
> - } else {
> - ret = ES_VMM_ERROR;
> - }
> - } else {
> + if ((ghcb->save.sw_exit_info_1 & 0xffffffff) == 1)
> + ret = ES_VMM_ERROR;
> + else
> ret = ES_OK;
> +
> + return ret;
> +}
> +
> +static enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb,
> + struct es_em_ctxt *ctxt,
> + u64 exit_code, u64 exit_info_1,
> + u64 exit_info_2)
Align arguments on the opening brace.
> +{
> + unsigned long v;
> + enum es_result ret;
> + u64 info;
> +
> + sev_es_wr_ghcb_msr(__pa(ghcb));
> +
> + ret = sev_es_ghcb_hv_call_simple(ghcb, exit_code, exit_info_1,
> + exit_info_2);
> + if (ret == ES_OK)
> + return ret;
> +
> + info = ghcb->save.sw_exit_info_2;
> + v = info & SVM_EVTINJ_VEC_MASK;
> +
> + /* Check if exception information from hypervisor is sane. */
> + if ((info & SVM_EVTINJ_VALID) &&
> + ((v == X86_TRAP_GP) || (v == X86_TRAP_UD)) &&
> + ((info & SVM_EVTINJ_TYPE_MASK) == SVM_EVTINJ_TYPE_EXEPT)) {
> + ctxt->fi.vector = v;
> + if (info & SVM_EVTINJ_VALID_ERR)
> + ctxt->fi.error_code = info >> 32;
> + ret = ES_EXCEPTION;
> + } else {
> + ret = ES_VMM_ERROR;
Why do you need to assign ES_VMM_ERROR here again when you return it
above?
IOW, that else branch is not really needed.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
