> > Subject: Re: [PATCH v13 01/35] x86/cpufeatures,opcode,msr: Add the
> WRMSRNS instruction support
>
> Or simply "x86/fred: Add ... "
Do I need to send an updated patch?
Or just leave it to the maintainer who is going to take care of it?
>
> Other than that,
>
> Acked-by: Borislav Petkov
> So we have recently discovered an overlooked interaction with VT-x.
> Immediately before VMENTER and after VMEXIT, CR2 is live with the
> *guest* CR2. Regardless of if the guest uses FRED or not, this is guest
> state and SHOULD NOT be corrupted. Furthermore, host state MUST NOT leak
> into the
> > In my opinion, cross-checking is the better approach, because it means that
> > violations of the assumptions get noticed more quickly, and hopefully by
> > whomever is working on the new feature which alters the assumptions.
>
> Yeah, I can make the change.
Hi Andrew,
Following is the
> >>> + case X86_TRAP_OF:
> >>> + exc_overflow(regs);
> >>> + return;
> >>> +
> >>> + /* INT3 */
> >>> + case X86_TRAP_BP:
> >>> + exc_int3(regs);
> >>> + return;
> >> ... neither OF nor BP will ever enter fred_intx() because they're
> >> type SWEXC not SWINT.
> >
> > diff --git a/arch/x86/entry/entry_fred.c b/arch/x86/entry/entry_fred.c
> > new file mode 100644 index ..215883e90f94
> > --- /dev/null
> > +++ b/arch/x86/entry/entry_fred.c
> > @@ -0,0 +1,230 @@
> > ...
> > +static noinstr void fred_intx(struct pt_regs *regs) {
> > + switch
> > +static noinstr void fred_intx(struct pt_regs *regs) {
> > + switch (regs->fred_ss.vector) {
> > + /* INT0 */
>
> INTO (for overflow), not INT-zero. However...
>
> > + case X86_TRAP_OF:
> > + exc_overflow(regs);
> > + return;
> > +
> > + /* INT3 */
> > + case
> Add the opcode used by WRMSRNS, which is the non-serializing version of
> WRMSR and may replace it to improve performance, to the x86 opcode map.
>
> Tested-by: Shan Kang
> Signed-off-by: Xin Li
> Acked-by: Masami Hiramatsu (Google)
Hi Masami,
Boris prefers to merge the first 3 patches
> > FRED and IDT can share most of the definitions and declarations so
> > that in the majority of cases the actual handler implementation is the
> > same.
> >
> > The differences are the exceptions where FRED stores exception related
> > information on the stack and the sysvec implementations as
> and if you had to be precise, the code should do:
>
> if (cpu_feature_enabled(X86_FEATURE_FRED)) {
> if (cpu_feature_enabled(X86_FEATURE_WRMSRNS))
> wrmsrns(MSR_IA32_FRED_RSP0, (unsigned
> long)task_stack_page(task) + THREAD_SIZE);
> else
> Then, further down in the patchset, it says:
>
> + if (cpu_feature_enabled(X86_FEATURE_FRED)) {
> + /* WRMSRNS is a baseline feature for FRED. */
>
> but WRMSRNS is not mentioned in the FRED spec "Document Number:
> 346446-005US, Revision: 5.0" which, according to
>
>
> > diff --git a/Documentation/arch/x86/x86_64/fred.rst
> > b/Documentation/arch/x86/x86_64/fred.rst
> > new file mode 100644
> > index ..9f57e7b91f7e
> > --- /dev/null
> > +++ b/Documentation/arch/x86/x86_64/fred.rst
> > @@ -0,0 +1,96 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > This patch set enables the Intel flexible return and event delivery
> > (FRED) architecture for x86-64.
> >
>
>
> Which tree is this based on now?
>
It was based on the tip master on the day I sent the v12 patch set, i.e.,
Monday night in the US west coast.
> I tried running 'b4 diff' but
> >> +EVENT_TYPE_PRIV_SWEXC 5 // INT1 #define EVENT_TYPE_SWEXC 6
> >> +// INTO, INT3
> >
> > nit: This turned into INTO (Oh) rather than INT0( zero) in v11
>
> Yes, v11 corrected a bug in v10.
>
> The INTO instruction is "INT on Overflow". No zero involved.
>
> INT3 is thusly named
> >Yes, this is a noop, just a cleanup patch w/o functionality change.
> >
> It just seems to be completely redundant. We can just drop it, no? If we
> aren't going
> to explicitly clobber the registers there is no harm in setting them up for
> IDT
> unconditionally.
If no objection, I can make
> >diff --git a/arch/x86/kernel/cpu/common.c
> >b/arch/x86/kernel/cpu/common.c index 20bbedbf6dcb..2ee4e7b597a3 100644
> >--- a/arch/x86/kernel/cpu/common.c
> >+++ b/arch/x86/kernel/cpu/common.c
> >@@ -2071,10 +2071,8 @@ static void wrmsrl_cstar(unsigned long val)
> > wrmsrl(MSR_CSTAR,
> > I notice there are several call sites using the safe version w/o
> > checking the return value, should the unsafe version be a better
> > choice in such cases?
>
> Depends. The safe version does not emit a warning on fail. So if the
> callsite truly does not care about the error it's fine.
> > > +static __always_inline void __wrmsrns(u32 msr, u32 low, u32 high)
> >
> > Shouldn't this be named wrmsrns_safe since it has exception handling,
> > similar
> to
> > the current wrmsrl_safe.
> >
>
> Both safe and unsafe versions have exception handling, while the safe
> version returns an
> > Since future kernels will support boottime toggling of whether 32bit
> > syscall interface should be enabled or not as per:
> > https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=
> > x86/entry=1da5c9bc119d3a749b519596b93f9b2667e93c4a
> >
> > It will make more sense to
> > I guess you have FRED 3.0 spec, no?
> Doh you are right, I was looking at the wrong version of the document
> sorry for
> the noise.
Actually I appreciate your review so much!
> > + /*
> > +* Don't check the FRED stack level, the call stack leading to this
> > +* helper is effectively constant and shallow (relatively speaking).
>
> It's more that we don't need to protect from reentrancy. The external
> interrupt uses stack level 0 so no adjustment would be
> > +#ifdef __x86_64__
> > +#define X86_CR4_FRED_BIT 32 /* enable FRED kernel entry */
> > +#define X86_CR4_FRED _BITUL(X86_CR4_FRED_BIT)
>
> nit: s/BITUL/BITULL I guess if __x86_64__ is defined then we are
> guaranteed that unsigned long will be a 64 bit, but for the sake of
>
> > +struct fred_ss {
> > + u64 ss : 16, // SS selector
>
> Is this structure conformant to the return state as described in FRED 5.0?
>
> — The stack segment of the interrupted context, 64 bits formatted as follows:
>
> • Bits 15:0 contain the SS selector. < - WE HAVE THIS
>
> •
> > +static __always_inline void __wrmsrns(u32 msr, u32 low, u32 high)
>
> Shouldn't this be named wrmsrns_safe since it has exception handling, similar
> to
> the current wrmsrl_safe.
>
Both safe and unsafe versions have exception handling, while the safe
version returns an integer to its
> > +static inline void fred_syscall_init(void) {
> > + /*
> > +* Per FRED spec 5.0, FRED uses the ring 3 FRED entrypoint for SYSCALL
> > +* and SYSENTER, and ERETU is the only legit instruction to return to
> > +* ring 3, as a result there is _no_ need to setup the SYSCALL and
> > +
> > The IDT event delivery of X86_TRAP_DF, X86_TRAP_TS, X86_TRAP_NP,
> > X86_TRAP_SS, X86_TRAP_GP, X86_TRAP_AC and X86_TRAP_CP pushes an error
> > code into the orig_ax member of the pt_regs structure, and the error
> > code is passed as the second argument of their C-handlers, although
> > the
> > On 04/08/2023 8:57 am, Xin Li wrote:
> >> I haven't checked Xen implications with this change, i.e., does Xen
> >> hypervisor need to adjust how it passes arguments to PV guests?
> >
> > This is an internal detail of how Linux handles data on it's stacks,
> > isn't it?
> >
> > The Xen code in
> On 04/08/2023 8:57 am, Xin Li wrote:
> > I haven't checked Xen implications with this change, i.e., does Xen
> > hypervisor need to adjust how it passes arguments to PV guests?
>
> This is an internal detail of how Linux handles data on it's stacks, isn't it?
Yes, it is completely internal to
> > Jumping way back to providing a wrapper for FRED, if we do that, then
> > there's no need to include calling.h, and the weird wrinkle about the
> > ERET target kinda goes away too. E.g. provide this in
> > arch/x86/entry/entry_64_fred.S
> >
> > .section .text, "ax"
> >
> > /* Note, this
> > +#include "../../entry/calling.h"
>
> Rather than do the low level PUSH_REGS and POP_REGS, I vote to have core code
> expose a FRED-specific wrapper for invoking external_interrupt(). More below.
Nice idea!
> > + /*
> > +* A FRED stack frame has extra 16 bytes of information pushed
> > I also believe there is a kernel.org service for sending patch series,
> > but i'm not sure I remember the details.
>
> https://b4.docs.kernel.org/en/latest/contributor/send.html
It says:
The kernel.org endpoint can only be used for kernel.org-hosted projects.
If there are no recognized
> > Resend because the mail system failed to deliver some messages yesterday.
>
> The one from yesterday came in 6 thread groups: 0-25, 26, 27, 28, 29, 30-36,
> while the one from today comes in 2 thread groups: 0-26, 27-36. Which I
> suppose one can count as an improvement :/
Sigh, sorry for
> > Are you talking about that you only got a subset of this patch set?
>
> No, I'm saying I don't want to waste a bunch of time tracking down exactly
> which
> commit a 36 patch series is based on. E.g. I just refreshed tip/master and
> still
> get:
>
> Applying: x86/idtentry: Incorporate
> > This patch set enables the Intel flexible return and event delivery
> > (FRED) architecture for x86-64.
>
> ...
>
> > --
> > 2.34.1
>
> What is this based on?
The tip tree master branch.
> FYI, you're using a version of git that will (mostly)
> automatically generate the based, e.g. I do
> > Add instruction opcodes used by FRED ERETU/ERETS to x86-opcode-map.
> >
> > Opcode numbers are per FRED spec v5.0.
> >
> > Signed-off-by: H. Peter Anvin (Intel)
> > Tested-by: Shan Kang
> > Signed-off-by: Xin Li
>
> This looks good to me. (ERETS has the opcode F2 0F 01 CA, ERETU has the
34 matches
Mail list logo
