lkml.org 
[lkml]   [2021]   [Sep]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH V6 5/8] x86/hyperv: Add Write/Read MSR registers via ghcb page
    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

    \
     
     \ /
      Last update: 2021-09-30 20:21    [W:3.436 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site