MIME-Version: 1.0
References: <20230307023946.14516-1-xin3.li@intel.com> <20230307023946.14516-23-xin3.li@intel.com>
 <CAJhGHyADXz-3PCFS3M_7TJ8qLGJ=4NcV9aBWrpjemuXB_SnMGg@mail.gmail.com> <5D679723-D84F-42F0-AD8A-8BD1A38FB6CD@zytor.com>
In-Reply-To: <5D679723-D84F-42F0-AD8A-8BD1A38FB6CD@zytor.com>
From:   Lai Jiangshan <jiangshanlai@gmail.com>
Date:   Sat, 18 Mar 2023 14:33:30 +0800
Message-ID: <CAJhGHyC0_1xJD2R03-NoRVpMXFTHR4v8CdzyJOZe_k0rdv=NfQ@mail.gmail.com>
Subject: Re: [PATCH v5 22/34] x86/fred: FRED initialization code
To:     "H. Peter Anvin" <hpa@zytor.com>
Cc:     Xin Li <xin3.li@intel.com>, linux-kernel@vger.kernel.org,
        x86@kernel.org, kvm@vger.kernel.org, tglx@linutronix.de,
        mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com,
        peterz@infradead.org, andrew.cooper3@citrix.com, seanjc@google.com,
        pbonzini@redhat.com, ravi.v.shankar@intel.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sat, Mar 18, 2023 at 5:32=E2=80=AFAM H. Peter Anvin <hpa@zytor.com> wrot=
e:
>
> On March 17, 2023 6:35:57 AM PDT, Lai Jiangshan <jiangshanlai@gmail.com> =
wrote:
> >Hello
> >
> >
> >Comments in cpu_init_fred_exceptions() seem scarce for understanding.
> >
> >On Tue, Mar 7, 2023 at 11:07=E2=80=AFAM Xin Li <xin3.li@intel.com> wrote=
:
> >
> >> +/*
> >> + * Initialize FRED on this CPU. This cannot be __init as it is called
> >> + * during CPU hotplug.
> >> + */
> >> +void cpu_init_fred_exceptions(void)
> >> +{
> >> +       wrmsrl(MSR_IA32_FRED_CONFIG,
> >> +              FRED_CONFIG_ENTRYPOINT(fred_entrypoint_user) |
> >> +              FRED_CONFIG_REDZONE(8) | /* Reserve for CALL emulation =
*/
> >> +              FRED_CONFIG_INT_STKLVL(0));
> >
> >What is it about "Reserve for CALL emulation"?
> >
> >I guess it relates to X86_TRAP_BP. In entry_64.S:
> >
> >        .if \vector =3D=3D X86_TRAP_BP
> >                /*
> >                 * If coming from kernel space, create a 6-word gap to a=
llow the
> >                 * int3 handler to emulate a call instruction.
> >                 */
> >
> >> +
> >> +       wrmsrl(MSR_IA32_FRED_STKLVLS,
> >> +              FRED_STKLVL(X86_TRAP_DB,  1) |
> >> +              FRED_STKLVL(X86_TRAP_NMI, 2) |
> >> +              FRED_STKLVL(X86_TRAP_MC,  2) |
> >> +              FRED_STKLVL(X86_TRAP_DF,  3));
> >
> >Why each exception here needs a stack level > 0?
> >Especially for X86_TRAP_DB and X86_TRAP_NMI.
> >
> >Why does or why does not X86_TRAP_VE have a stack level > 0?
> >
> >X86_TRAP_DF is the highest stack level, is it accidental
> >or deliberate?
> >
> >Thanks
> >Lai
> >
>
> Yes, the extra redzone space is there to allow for the call emulation wit=
hout having to adjust the stack frame "manually".
>
> In theory we could enable it only while code patching is in progress, but=
 that would probably just result in stack overflows becoming utterly imposs=
ible to debug as we have to consider the worst case.
>
> The purpose of separate stacks for NMI, #DB and #MC *in the kernel* (reme=
mber that user space faults are always taken on stack level 0) is to avoid =
overflowing the kernel stack. #DB in the kernel would imply the use of a ke=
rnel debugger.

Could you add it to the code, please? I think it can help other reviewers.

If there is no other concrete reason other than overflowing for
assigning NMI and #DB with a stack level > 0, #VE should also
be assigned with a stack level > 0, and #BP too. #VE can happen
anytime and anywhere, so it is subject to overflowing too.

>
>
> #DF is the highest level because a #DF means "something went wrong *while=
 delivering an exception*." The number of cases for which that can happen w=
ith FRED is drastically reduced and basically amount to "the stack you poin=
ted me to is broken."
>
> Thus, you basically always want to change stacks on #DF, which means it s=
hould be at the highest level.