diff mbox series

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

Message ID 20231003062458.23552-17-xin3.li@intel.com (mailing list archive)
State Superseded
Headers show
Series x86: enable FRED for x86-64 | expand

Commit Message

Li, Xin3 Oct. 3, 2023, 6:24 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 v11:
* Add a new structure fred_cs to denote the FRED flags above CS
  selector as what is done for SS (H. Peter Anvin).

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 | 70 ++++++++++++++++++++++++++++++++---
 1 file changed, 65 insertions(+), 5 deletions(-)

Comments

Borislav Petkov Nov. 28, 2023, 8:51 a.m. UTC | #1 
On Mon, Oct 02, 2023 at 11:24:37PM -0700, 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>

You and hpa need to go through all the patches and figure out who's the
author that's going to land in git.

Because this and others have hpa's SOB first, suggesting he's the
author. However, the mail doesn't start with

From: H. Peter Anvin (Intel) <hpa@zytor.com>

and then git will make *you* the author.

> 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>

...

>  	union {
> -		u64	ssx;	// The full 64-bit data slot containing SS
> -		u16	ss;	// SS selector
> +		/* SS selector */
> +		u16		ss;
> +		/* The extended 64-bit data slot containing SS */
> +		u64		ssx;
> +		/* The FRED SS extension */
> +		struct fred_ss	fred_ss;

Aha, sanity about the right comments has come to your mind in this next
patch. :-P

Just do them right in the previous one.

>  	/*
> -	 * 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.

Btw, I really appreciate the good commenting - thanks for that!
H. Peter Anvin Nov. 28, 2023, 5:18 p.m. UTC | #2 
On November 28, 2023 12:51:22 AM PST, Borislav Petkov <bp@alien8.de> wrote:
>On Mon, Oct 02, 2023 at 11:24:37PM -0700, 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>
>
>You and hpa need to go through all the patches and figure out who's the
>author that's going to land in git.
>
>Because this and others have hpa's SOB first, suggesting he's the
>author. However, the mail doesn't start with
>
>From: H. Peter Anvin (Intel) <hpa@zytor.com>
>
>and then git will make *you* the author.
>
>> 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>
>
>...
>
>>  	union {
>> -		u64	ssx;	// The full 64-bit data slot containing SS
>> -		u16	ss;	// SS selector
>> +		/* SS selector */
>> +		u16		ss;
>> +		/* The extended 64-bit data slot containing SS */
>> +		u64		ssx;
>> +		/* The FRED SS extension */
>> +		struct fred_ss	fred_ss;
>
>Aha, sanity about the right comments has come to your mind in this next
>patch. :-P
>
>Just do them right in the previous one.
>
>>  	/*
>> -	 * 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.
>
>Btw, I really appreciate the good commenting - thanks for that!
>

For Xin, mainly:

Standard practice is:

1. For a patch with relatively small modifications, or where the changes are mainly in comments or the patch message:

Keep the authorship, but put a description of what you have changed in brackets with your username at the bottom of the description, immediately before Signed-off-by:

[ xin: changed foo, bar, baz ]


2. For a patch with major rewrites:

Take authorship on the From: line, but have an Originally-by: tag (rather than a Signed-off-by: by the original author):

Originally-by: Someone Else <someone@elsewhere.dom>


3. For a patch which is fully or nearly fully your own work (a total rewrite, or based on a concept idea rather than actual code), credit the original in the patch comment:

Based on an idea by Someone Else <someone@elsewhere.dom> (optional link to lore.kernel.org).
diff mbox series

Patch

diff --git a/arch/x86/include/asm/ptrace.h b/arch/x86/include/asm/ptrace.h
index f08ea073edd6..5a83fbd9bc0b 100644
--- a/arch/x86/include/asm/ptrace.h
+++ b/arch/x86/include/asm/ptrace.h
@@ -56,6 +56,50 @@  struct pt_regs {
 
 #else /* __i386__ */
 
+struct fred_cs {
+		/* CS selector */
+	u64	cs	: 16,
+		/* Stack level at event time */
+		sl	:  2,
+		/* IBT in WAIT_FOR_ENDBRANCH state */
+		wfe	:  1,
+			: 45;
+};
+
+struct fred_ss {
+		/* SS selector */
+	u64	ss	: 16,
+		/* STI state */
+		sti	:  1,
+		/* Set if syscall, sysenter or INT n */
+		swevent	:  1,
+		/* Event is NMI type */
+		nmi	:  1,
+			: 13,
+		/* Event vector */
+		vector	:  8,
+			:  8,
+		/* Event type */
+		type	:  4,
+			:  4,
+		/* Event was incident to enclave execution */
+		enclave	:  1,
+		/* CPU was in long mode */
+		lm	:  1,
+		/*
+		 * Nested exception during FRED delivery, not set
+		 * for #DF.
+		 */
+		nested	:  1,
+			:  1,
+		/*
+		 * The length of the instruction causing the event.
+		 * Only set for INTO, INT1, INT3, INT n, SYSCALL
+		 * and SYSENTER.  0 otherwise.
+		 */
+		insnlen	:  4;
+};
+
 struct pt_regs {
 	/*
 	 * C ABI says these regs are callee-preserved. They aren't saved on
@@ -85,6 +129,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 +142,30 @@  struct pt_regs {
 	unsigned long ip;
 
 	union {
-		u64	csx;	// The full 64-bit data slot containing CS
-		u16	cs;	// CS selector
+		/* CS selector */
+		u16		cs;
+		/* The extended 64-bit data slot containing CS */
+		u64		csx;
+		/* The FRED CS extension */
+		struct fred_cs	fred_cs;
 	};
 
 	unsigned long flags;
 	unsigned long sp;
 
 	union {
-		u64	ssx;	// The full 64-bit data slot containing SS
-		u16	ss;	// SS selector
+		/* SS selector */
+		u16		ss;
+		/* The extended 64-bit data slot containing SS */
+		u64		ssx;
+		/* The FRED SS extension */
+		struct fred_ss	fred_ss;
 	};
 
 	/*
-	 * 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.
 	 */
 };