diff mbox series

[v10,16/38] x86/ptrace: Add FRED additional information to the pt_regs structure

Message ID 20230914044805.301390-17-xin3.li@intel.com
State New
Headers
Series x86: enable FRED for x86-64 |

Commit Message

Li, Xin3 Sept. 14, 2023, 4:47 a.m. UTC 
  FRED defines additional information in the upper 48 bits of cs/ss
fields. Therefore add the information definitions into the pt_regs
structure.

Specially introduce a new structure fred_ss to denote the FRED flags
above SS selector, which avoids FRED_SSX_ macros and makes the code
simpler and easier to read.

Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Tested-by: Shan Kang <shan.kang@intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Xin Li <xin3.li@intel.com>
---

Changes since v9:
* Introduce a new structure fred_ss to denote the FRED flags above SS
  selector, which avoids FRED_SSX_ macros and makes the code simpler
  and easier to read (Thomas Gleixner).
* Use type u64 to define FRED bit fields instead of type unsigned int
  (Thomas Gleixner).

Changes since v8:
* Reflect stack frame definition changes from FRED spec 3.0 to 5.0.
* Use __packed instead of __attribute__((__packed__)) (Borislav Petkov).
* Put all comments above the members, like the rest of the file does
  (Borislav Petkov).

Changes since v3:
* Rename csl/ssl of the pt_regs structure to csx/ssx (x for extended)
  (Andrew Cooper).
---
 arch/x86/include/asm/ptrace.h | 51 +++++++++++++++++++++++++++++++----
 1 file changed, 46 insertions(+), 5 deletions(-)

Comments

Nikolay Borisov Sept. 20, 2023, 12:57 p.m. UTC | #1 
On 14.09.23 г. 7:47 ч., Xin Li wrote:
> FRED defines additional information in the upper 48 bits of cs/ss
> fields. Therefore add the information definitions into the pt_regs
> structure.
> 
> Specially introduce a new structure fred_ss to denote the FRED flags
> above SS selector, which avoids FRED_SSX_ macros and makes the code
> simpler and easier to read.
> 
> Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
> Tested-by: Shan Kang <shan.kang@intel.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Xin Li <xin3.li@intel.com>
> ---
> 
> Changes since v9:
> * Introduce a new structure fred_ss to denote the FRED flags above SS
>    selector, which avoids FRED_SSX_ macros and makes the code simpler
>    and easier to read (Thomas Gleixner).
> * Use type u64 to define FRED bit fields instead of type unsigned int
>    (Thomas Gleixner).
> 
> Changes since v8:
> * Reflect stack frame definition changes from FRED spec 3.0 to 5.0.
> * Use __packed instead of __attribute__((__packed__)) (Borislav Petkov).
> * Put all comments above the members, like the rest of the file does
>    (Borislav Petkov).
> 
> Changes since v3:
> * Rename csl/ssl of the pt_regs structure to csx/ssx (x for extended)
>    (Andrew Cooper).
> ---
>   arch/x86/include/asm/ptrace.h | 51 +++++++++++++++++++++++++++++++----
>   1 file changed, 46 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/include/asm/ptrace.h b/arch/x86/include/asm/ptrace.h
> index f08ea073edd6..5786c8ca5f4c 100644
> --- a/arch/x86/include/asm/ptrace.h
> +++ b/arch/x86/include/asm/ptrace.h
> @@ -56,6 +56,25 @@ struct pt_regs {
>   
>   #else /* __i386__ */
>   
> +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

• Bits 31:16 are not currently defined and will be zero until they are. < - MISSING hole?


> +		sti	:  1,	// STI state < -
> +		swevent	:  1,	// Set if syscall, sysenter or INT n
> +		nmi	:  1,	// Event is NMI type
> +			: 13,
> +		vector	:  8,	// Event vector
> +			:  8,
> +		type	:  4,	// Event type
> +			:  4,
> +		enclave	:  1,	// Event was incident to enclave execution
> +		lm	:  1,	// CPU was in long mode
> +		nested	:  1,	// Nested exception during FRED delivery
> +				// not set for #DF
> +			:  1,
> +		insnlen	:  4;	// The length of the instruction causing the event
> +				// Only set for INT0, INT1, INT3, INT n, SYSCALL
> +};				// and SYSENTER. 0 otherwise.
> +

<Snip>
Li, Xin3 Sept. 20, 2023, 5:23 p.m. UTC | #2 
> > +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
> 
> • Bits 31:16 are not currently defined and will be zero until they are.

Where did you download the FRED 5.0 spec from?

Mine says bit 16 is sti, bit 17 for sw initiated events and bit 18 is NMI.

I guess you have FRED 3.0 spec, no?

>  < - MISSING > hole?
> 
> > +		sti	:  1,	// STI state < -
> > +		swevent	:  1,	// Set if syscall, sysenter or INT n
> > +		nmi	:  1,	// Event is NMI type
> > +			: 13,
Nikolay Borisov Sept. 21, 2023, 6:07 a.m. UTC | #3 
On 20.09.23 г. 20:23 ч., Li, Xin3 wrote:
>>> +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
>>
>> • Bits 31:16 are not currently defined and will be zero until they are.
> 
> Where did you download the FRED 5.0 spec from?
> 
> Mine says bit 16 is sti, bit 17 for sw initiated events and bit 18 is NMI.
> 
> 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.
> 
>>   < - MISSING > hole?
>>
>>> +		sti	:  1,	// STI state < -
>>> +		swevent	:  1,	// Set if syscall, sysenter or INT n
>>> +		nmi	:  1,	// Event is NMI type
>>> +			: 13,
>
Li, Xin3 Sept. 21, 2023, 6:24 a.m. UTC | #4 
> > 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!
diff mbox series

Patch

diff --git a/arch/x86/include/asm/ptrace.h b/arch/x86/include/asm/ptrace.h
index f08ea073edd6..5786c8ca5f4c 100644
--- a/arch/x86/include/asm/ptrace.h
+++ b/arch/x86/include/asm/ptrace.h
@@ -56,6 +56,25 @@  struct pt_regs {
 
 #else /* __i386__ */
 
+struct fred_ss {
+	u64	ss	: 16,	// SS selector
+		sti	:  1,	// STI state
+		swevent	:  1,	// Set if syscall, sysenter or INT n
+		nmi	:  1,	// Event is NMI type
+			: 13,
+		vector	:  8,	// Event vector
+			:  8,
+		type	:  4,	// Event type
+			:  4,
+		enclave	:  1,	// Event was incident to enclave execution
+		lm	:  1,	// CPU was in long mode
+		nested	:  1,	// Nested exception during FRED delivery
+				// not set for #DF
+			:  1,
+		insnlen	:  4;	// The length of the instruction causing the event
+				// Only set for INT0, INT1, INT3, INT n, SYSCALL
+};				// and SYSENTER. 0 otherwise.
+
 struct pt_regs {
 	/*
 	 * C ABI says these regs are callee-preserved. They aren't saved on
@@ -85,6 +104,12 @@  struct pt_regs {
 	 * - the syscall number (syscall, sysenter, int80)
 	 * - error_code stored by the CPU on traps and exceptions
 	 * - the interrupt number for device interrupts
+	 *
+	 * A FRED stack frame starts here:
+	 *   1) It _always_ includes an error code;
+	 *
+	 *   2) The return frame for ERET[US] starts here, but
+	 *	the content of orig_ax is ignored.
 	 */
 	unsigned long orig_ax;
 
@@ -92,20 +117,36 @@  struct pt_regs {
 	unsigned long ip;
 
 	union {
-		u64	csx;	// The full 64-bit data slot containing CS
-		u16	cs;	// CS selector
+		u64	csx;		// The full data for FRED
+		/*
+		 * The 'cs' member should be defined as a 16-bit bit-field
+		 * along with the 'sl' and 'wfe' members, which however
+		 * breaks compiling REG_OFFSET_NAME(cs), because compilers
+		 * disallow calculating the address of a bit-field.
+		 *
+		 * Therefore 'cs" is defined as an individual member with
+		 * type u16.
+		 */
+		u16	cs;		// CS selector
+		u64		: 16,
+			sl	:  2,	// Stack level at event time
+			wfe	:  1,	// IBT is in WAIT_FOR_BRANCH_STATE
+				: 45;
 	};
 
 	unsigned long flags;
 	unsigned long sp;
 
 	union {
-		u64	ssx;	// The full 64-bit data slot containing SS
-		u16	ss;	// SS selector
+		u64		ssx;		// The full data for FRED
+		u16		ss;		// SS selector
+		struct fred_ss	fred_ss;	// The fred extension
 	};
 
 	/*
-	 * Top of stack on IDT systems.
+	 * Top of stack on IDT systems, while FRED systems have extra fields
+	 * defined above for storing exception related information, e.g. CR2 or
+	 * DR6.
 	 */
 };