> > 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 (A
> 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 g
> > 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 upd
> >>> + 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.
> > Pe
> > 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 (regs-
> > +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 X8
> 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 into
> > 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 F
> 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
>
> https://w
> > 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 i
> > 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
> > +
>
> >> +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 bec
> >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.
Ri
> > > +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 i
> > 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&id=1da5c9bc119d3a749b519596b93f9b2667e93c4a
> >
> > It will make more sense to rep
> > 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 ne
> > +#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
> cla
> > +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
>
> • B
> > +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 call
> > +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
> > +
24 matches
Mail list logo
