RE: [PATCH v13 26/35] x86/fred: FRED entry/exit and dispatch code

["H. Peter Anvin" <hpa@zytor.com>]

On December 6, 2023 11:19:26 AM PST, "Li, Xin3" <xin3.li@intel.com> wrote:
>> >>> +	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.
>> > Per FRED spec 5.0, section 7.3 Software Interrupts and Related Instructions:
>> > INT n (opcode CD followed by an immediate byte): There are 256 such
>> > software interrupt instructions, one for each value n of the immediate
>> > byte (0–255).
>> >
>> > And appendix B Event Stack Levels:
>> > If the event is an execution of INT n (opcode CD n for 8-bit value n),
>> > the event stack level is 0. The event type is 4 (software interrupt)
>> > and the vector is n.
>> >
>> > So int $0x4 and int $0x3 (use asm(".byte 0xCD, 0x03")) get here.
>> >
>> > But into (0xCE) and int3 (0xCC) do use event type SWEXC.
>> >
>> > BTW, into is NOT allowed in 64-bit mode but "int $0x4" is allowed.
>> 
>> There is certainly fun to be had with CD 03 and CD 04 byte patterns, but if you
>> meant to mean those here, then the comments are wrong.
>> 
>> Vectors 3 and 4 are installed with DPL3 because that is necessary to make CC and
>> CE function in userspace.  It also suggests that the SWINT vs SWEXC distinction
>> was retrofitted to architecture after the 286, because exceptions don't check DPL
>> and ICEBP delivers #DB from userspace even when Vector 1 has a DPL of 0.
>> 
>> While CC is for most cases indistinguishable from CD 03, CE behaves entirely
>> differently to CD 04.  CD 04 doesn't #UD in 64bit mode, and will trigger
>> exc_overflow() irrespective of the state of EFLAGS.OF.
>> 
>> 
>> The SDM goes out of it's way to say not to use the CD 03 byte pattern (and it
>> does take effort to emit this byte pattern - e.g. GAS will silently translate "int $3"
>> to "int3"), and there's no plausible way software is using CD 04 in place of CE.
>> 
>> So why do we care about containing to make mistakes of the IDT era work in a
>> FRED world?
>
>First, I agree with you because it makes things simple and neat.
>
>However, the latest SDM and FRED spec 5.0 both doesn't disallow it, so it
>becomes an OS implementation choice.
>
>> 
>> Is there anything (other than perhaps the selftests) which would even notice?
>
>I'm just conservative :)
>
>If a user app can do it with IDT, we should still allow it when FRED is
>enabled.  But if all key stakeholders don't care whatever gets broken
>due to the change and agree to change it.
>
>> >>> +		instrumentation_end();
>> >>> +		irqentry_exit(regs, state);
>> >>> +	} else {
>> >>> +		common_interrupt(regs, vector);
>> >>> +	}
>> >>> +}
>> >>> +
>> >>> +static noinstr void fred_exception(struct pt_regs *regs, unsigned
>> >>> +long error_code) {
>> >>> +	/* Optimize for #PF. That's the only exception which matters
>> >>> +performance
>> >> wise */
>> >>> +	if (likely(regs->fred_ss.vector == X86_TRAP_PF)) {
>> >>> +		exc_page_fault(regs, error_code);
>> >>> +		return;
>> >>> +	}
>> >>> +
>> >>> +	switch (regs->fred_ss.vector) {
>> >>> +	case X86_TRAP_DE: return exc_divide_error(regs);
>> >>> +	case X86_TRAP_DB: return fred_exc_debug(regs);
>> >>> +	case X86_TRAP_BP: return exc_int3(regs);
>> >>> +	case X86_TRAP_OF: return exc_overflow(regs);
>> >> Depending on what you want to do with BP/OF vs fred_intx(), this may
>> >> need adjusting.
>> >>
>> >> If you are cross-checking type and vector, then these should be
>> >> rejected for not being of type HWEXC.
>> > You're right, the event type needs to be SWEXC for into and int3.
>> >
>> > However, would it be overkilling?  Assuming hardware and VMM are sane.
>> 
>> You either care about cross checking, or not.  Right now, this patch is a mix of the
>> two approaches.
>> 
>> 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.
>
>Thanks!
>    Xin
>

The intent is to not break userspace even if userspace does something fundamentally stupid.