public inbox for linux-riscv@lists.infradead.org
 help / color / mirror / Atom feed
* updates for instruction trapping and ptrace
@ 2024-11-13  9:17 Ben Dooks
  2024-11-13  9:17 ` [PATCH 1/3] riscv: ptrace: add regs_set_register() Ben Dooks
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Ben Dooks @ 2024-11-13  9:17 UTC (permalink / raw)
  To: linux-riscv; +Cc: palmer, ajones, alex

After looking at handling the RDCYCLE as RDTIME, the following
patches are probably still useful (even if the actual in-kernel
fixup of RDCYCLE isn't).



_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [PATCH 1/3] riscv: ptrace: add regs_set_register()
  2024-11-13  9:17 updates for instruction trapping and ptrace Ben Dooks
@ 2024-11-13  9:17 ` Ben Dooks
  2024-11-13 10:12   ` Andrew Jones
  2024-11-13  9:17 ` [PATCH 2/3] riscv: traps: make insn fetch common in unknown instruction Ben Dooks
  2024-11-13  9:17 ` [PATCH 3/3] riscv: insn: add RV_EXTRACT_FUNCT3() Ben Dooks
  2 siblings, 1 reply; 10+ messages in thread
From: Ben Dooks @ 2024-11-13  9:17 UTC (permalink / raw)
  To: linux-riscv; +Cc: palmer, ajones, alex, Ben Dooks

Since we have regs_get_register() and we could use the set counterpart
for things like fixing up traps, add regs_set_register() to set a pt_regs
value from offset.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 arch/riscv/include/asm/ptrace.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/riscv/include/asm/ptrace.h b/arch/riscv/include/asm/ptrace.h
index b5b0adcc85c1..66fc1795141d 100644
--- a/arch/riscv/include/asm/ptrace.h
+++ b/arch/riscv/include/asm/ptrace.h
@@ -143,6 +143,26 @@ static inline unsigned long regs_get_register(struct pt_regs *regs,
 	return *(unsigned long *)((unsigned long)regs + offset);
 }
 
+/**
+ * regs_set_register() - set register value from its offset
+ * @regs:	pt_regs from which register value is gotten
+ * @offset:	offset of the register.
+ * @to:		value to set register to
+ *
+ * regs_set_register sets the value @to to a register whose offset from @regs.
+ * The @offset is the offset of the register in struct pt_regs.
+ * If @offset is bigger than MAX_REG_OFFSET, this will ignore the write.
+ */
+static inline void regs_set_register(struct pt_regs *regs,
+				     unsigned int offset,
+				     unsigned long to)
+{
+	if (unlikely(offset > MAX_REG_OFFSET))
+		return;
+
+	*(unsigned long *)((unsigned long)regs + offset) = to;
+}
+
 /**
  * regs_get_kernel_argument() - get Nth function argument in kernel
  * @regs:       pt_regs of that context
-- 
2.37.2.352.g3c44437643


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH 2/3] riscv: traps: make insn fetch common in unknown instruction
  2024-11-13  9:17 updates for instruction trapping and ptrace Ben Dooks
  2024-11-13  9:17 ` [PATCH 1/3] riscv: ptrace: add regs_set_register() Ben Dooks
@ 2024-11-13  9:17 ` Ben Dooks
  2024-11-13  9:21   ` Ben Dooks
  2024-11-13  9:17 ` [PATCH 3/3] riscv: insn: add RV_EXTRACT_FUNCT3() Ben Dooks
  2 siblings, 1 reply; 10+ messages in thread
From: Ben Dooks @ 2024-11-13  9:17 UTC (permalink / raw)
  To: linux-riscv; +Cc: palmer, ajones, alex, Ben Dooks

Add the trapped instruction (insn) as the second argument to
riscv_v_first_use_handler() from the trap handler so when we
add more handlers we can do the fetch of the instruction just
once.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
- fixed wording of patch from rfc
---
 arch/riscv/include/asm/vector.h |  4 ++--
 arch/riscv/kernel/traps.c       | 11 ++++++++++-
 arch/riscv/kernel/vector.c      | 11 +----------
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h
index be7d309cca8a..c9f0b02cd975 100644
--- a/arch/riscv/include/asm/vector.h
+++ b/arch/riscv/include/asm/vector.h
@@ -21,7 +21,7 @@
 
 extern unsigned long riscv_v_vsize;
 int riscv_v_setup_vsize(void);
-bool riscv_v_first_use_handler(struct pt_regs *regs);
+bool riscv_v_first_use_handler(struct pt_regs *regs, u32 insn);
 void kernel_vector_begin(void);
 void kernel_vector_end(void);
 void get_cpu_vector_context(void);
@@ -268,7 +268,7 @@ struct pt_regs;
 
 static inline int riscv_v_setup_vsize(void) { return -EOPNOTSUPP; }
 static __always_inline bool has_vector(void) { return false; }
-static inline bool riscv_v_first_use_handler(struct pt_regs *regs) { return false; }
+static inline bool riscv_v_first_use_handler(struct pt_regs *regs, u32 insn) { return false; }
 static inline bool riscv_v_vstate_query(struct pt_regs *regs) { return false; }
 static inline bool riscv_v_vstate_ctrl_user_allowed(void) { return false; }
 #define riscv_v_vsize (0)
diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
index 51ebfd23e007..1c3fab272fd1 100644
--- a/arch/riscv/kernel/traps.c
+++ b/arch/riscv/kernel/traps.c
@@ -172,11 +172,20 @@ asmlinkage __visible __trap_section void do_trap_insn_illegal(struct pt_regs *re
 	bool handled;
 
 	if (user_mode(regs)) {
+		u32 __user *epc = (u32 __user *)regs->epc;
+		u32 insn = (u32)regs->badaddr;
+
 		irqentry_enter_from_user_mode(regs);
 
 		local_irq_enable();
 
-		handled = riscv_v_first_use_handler(regs);
+		if (!insn) {
+			if (__get_user(insn, epc)) {
+				/* todo */
+			}
+		}
+
+		handled = riscv_v_first_use_handler(regs, insn);
 
 		local_irq_disable();
 
diff --git a/arch/riscv/kernel/vector.c b/arch/riscv/kernel/vector.c
index 682b3feee451..b852648cb8d5 100644
--- a/arch/riscv/kernel/vector.c
+++ b/arch/riscv/kernel/vector.c
@@ -168,11 +168,8 @@ bool riscv_v_vstate_ctrl_user_allowed(void)
 }
 EXPORT_SYMBOL_GPL(riscv_v_vstate_ctrl_user_allowed);
 
-bool riscv_v_first_use_handler(struct pt_regs *regs)
+bool riscv_v_first_use_handler(struct pt_regs *regs, u32 insn)
 {
-	u32 __user *epc = (u32 __user *)regs->epc;
-	u32 insn = (u32)regs->badaddr;
-
 	if (!has_vector())
 		return false;
 
@@ -184,12 +181,6 @@ bool riscv_v_first_use_handler(struct pt_regs *regs)
 	if (riscv_v_vstate_query(regs))
 		return false;
 
-	/* Get the instruction */
-	if (!insn) {
-		if (__get_user(insn, epc))
-			return false;
-	}
-
 	/* Filter out non-V instructions */
 	if (!insn_is_vector(insn))
 		return false;
-- 
2.37.2.352.g3c44437643


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH 3/3] riscv: insn: add RV_EXTRACT_FUNCT3()
  2024-11-13  9:17 updates for instruction trapping and ptrace Ben Dooks
  2024-11-13  9:17 ` [PATCH 1/3] riscv: ptrace: add regs_set_register() Ben Dooks
  2024-11-13  9:17 ` [PATCH 2/3] riscv: traps: make insn fetch common in unknown instruction Ben Dooks
@ 2024-11-13  9:17 ` Ben Dooks
  2024-11-13 10:17   ` Andrew Jones
  2 siblings, 1 reply; 10+ messages in thread
From: Ben Dooks @ 2024-11-13  9:17 UTC (permalink / raw)
  To: linux-riscv; +Cc: palmer, ajones, alex, Ben Dooks

Add extraction for the func3 field of most instructions
for use with anyone who needs it.

Note, added this for decoding of CSR accesses for
work we did looking at the RDCYCLE v RDTIME calls.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 arch/riscv/include/asm/insn.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/riscv/include/asm/insn.h b/arch/riscv/include/asm/insn.h
index 09fde95a5e8f..c67f44ff2066 100644
--- a/arch/riscv/include/asm/insn.h
+++ b/arch/riscv/include/asm/insn.h
@@ -299,6 +299,10 @@ static __always_inline bool riscv_insn_is_c_jalr(u32 code)
 	({typeof(x) x_ = (x); \
 	(RV_X(x_, RVG_RD_OPOFF, RVG_RD_MASK)); })
 
+#define RV_EXTRACT_FUNCT3(x) \
+	({typeof(x) x_ = (x); \
+	(RV_X(x_, RV_INSN_FUNCT3_OPOFF, RV_INSN_FUNCT3_MASK)); })
+
 #define RV_EXTRACT_UTYPE_IMM(x) \
 	({typeof(x) x_ = (x); \
 	(RV_X(x_, RV_U_IMM_31_12_OPOFF, RV_U_IMM_31_12_MASK)); })
-- 
2.37.2.352.g3c44437643


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] riscv: traps: make insn fetch common in unknown instruction
  2024-11-13  9:17 ` [PATCH 2/3] riscv: traps: make insn fetch common in unknown instruction Ben Dooks
@ 2024-11-13  9:21   ` Ben Dooks
  2024-11-13  9:31     ` Ben Dooks
  0 siblings, 1 reply; 10+ messages in thread
From: Ben Dooks @ 2024-11-13  9:21 UTC (permalink / raw)
  To: linux-riscv; +Cc: palmer, ajones, alex

On 13/11/2024 09:17, Ben Dooks wrote:
> Add the trapped instruction (insn) as the second argument to
> riscv_v_first_use_handler() from the trap handler so when we
> add more handlers we can do the fetch of the instruction just
> once.
> 
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
> - fixed wording of patch from rfc
> ---
>   arch/riscv/include/asm/vector.h |  4 ++--
>   arch/riscv/kernel/traps.c       | 11 ++++++++++-
>   arch/riscv/kernel/vector.c      | 11 +----------
>   3 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h
> index be7d309cca8a..c9f0b02cd975 100644
> --- a/arch/riscv/include/asm/vector.h
> +++ b/arch/riscv/include/asm/vector.h
> @@ -21,7 +21,7 @@
>   
>   extern unsigned long riscv_v_vsize;
>   int riscv_v_setup_vsize(void);
> -bool riscv_v_first_use_handler(struct pt_regs *regs);
> +bool riscv_v_first_use_handler(struct pt_regs *regs, u32 insn);
>   void kernel_vector_begin(void);
>   void kernel_vector_end(void);
>   void get_cpu_vector_context(void);
> @@ -268,7 +268,7 @@ struct pt_regs;
>   
>   static inline int riscv_v_setup_vsize(void) { return -EOPNOTSUPP; }
>   static __always_inline bool has_vector(void) { return false; }
> -static inline bool riscv_v_first_use_handler(struct pt_regs *regs) { return false; }
> +static inline bool riscv_v_first_use_handler(struct pt_regs *regs, u32 insn) { return false; }
>   static inline bool riscv_v_vstate_query(struct pt_regs *regs) { return false; }
>   static inline bool riscv_v_vstate_ctrl_user_allowed(void) { return false; }
>   #define riscv_v_vsize (0)
> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
> index 51ebfd23e007..1c3fab272fd1 100644
> --- a/arch/riscv/kernel/traps.c
> +++ b/arch/riscv/kernel/traps.c
> @@ -172,11 +172,20 @@ asmlinkage __visible __trap_section void do_trap_insn_illegal(struct pt_regs *re
>   	bool handled;
>   
>   	if (user_mode(regs)) {
> +		u32 __user *epc = (u32 __user *)regs->epc;
> +		u32 insn = (u32)regs->badaddr;
> +
>   		irqentry_enter_from_user_mode(regs);
>   
>   		local_irq_enable();
>   
> -		handled = riscv_v_first_use_handler(regs);
> +		if (!insn) {
> +			if (__get_user(insn, epc)) {
> +				/* todo */
> +			}

grr, of course as soon as it is sent I notice the /todo/ here

Not sure if we should print something if __get_user() fails for
what /should/ be an good instruction addrss.... should probably
at-least bail out of do_trap_insn_illegal() if not also print a
warning.

> +		}
> +
> +		handled = riscv_v_first_use_handler(regs, insn);
>   
>   		local_irq_disable();
>   
> diff --git a/arch/riscv/kernel/vector.c b/arch/riscv/kernel/vector.c
> index 682b3feee451..b852648cb8d5 100644
> --- a/arch/riscv/kernel/vector.c
> +++ b/arch/riscv/kernel/vector.c
> @@ -168,11 +168,8 @@ bool riscv_v_vstate_ctrl_user_allowed(void)
>   }
>   EXPORT_SYMBOL_GPL(riscv_v_vstate_ctrl_user_allowed);
>   
> -bool riscv_v_first_use_handler(struct pt_regs *regs)
> +bool riscv_v_first_use_handler(struct pt_regs *regs, u32 insn)
>   {
> -	u32 __user *epc = (u32 __user *)regs->epc;
> -	u32 insn = (u32)regs->badaddr;
> -
>   	if (!has_vector())
>   		return false;
>   
> @@ -184,12 +181,6 @@ bool riscv_v_first_use_handler(struct pt_regs *regs)
>   	if (riscv_v_vstate_query(regs))
>   		return false;
>   
> -	/* Get the instruction */
> -	if (!insn) {
> -		if (__get_user(insn, epc))
> -			return false;
> -	}
> -
>   	/* Filter out non-V instructions */
>   	if (!insn_is_vector(insn))
>   		return false;


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] riscv: traps: make insn fetch common in unknown instruction
  2024-11-13  9:21   ` Ben Dooks
@ 2024-11-13  9:31     ` Ben Dooks
  0 siblings, 0 replies; 10+ messages in thread
From: Ben Dooks @ 2024-11-13  9:31 UTC (permalink / raw)
  To: linux-riscv; +Cc: palmer, ajones, alex

On 13/11/2024 09:21, Ben Dooks wrote:
> On 13/11/2024 09:17, Ben Dooks wrote:
>> Add the trapped instruction (insn) as the second argument to
>> riscv_v_first_use_handler() from the trap handler so when we
>> add more handlers we can do the fetch of the instruction just
>> once.
>>
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>> ---
>> - fixed wording of patch from rfc
>> ---
>>   arch/riscv/include/asm/vector.h |  4 ++--
>>   arch/riscv/kernel/traps.c       | 11 ++++++++++-
>>   arch/riscv/kernel/vector.c      | 11 +----------
>>   3 files changed, 13 insertions(+), 13 deletions(-)
>>
>> diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/ 
>> vector.h
>> index be7d309cca8a..c9f0b02cd975 100644
>> --- a/arch/riscv/include/asm/vector.h
>> +++ b/arch/riscv/include/asm/vector.h
>> @@ -21,7 +21,7 @@
>>   extern unsigned long riscv_v_vsize;
>>   int riscv_v_setup_vsize(void);
>> -bool riscv_v_first_use_handler(struct pt_regs *regs);
>> +bool riscv_v_first_use_handler(struct pt_regs *regs, u32 insn);
>>   void kernel_vector_begin(void);
>>   void kernel_vector_end(void);
>>   void get_cpu_vector_context(void);
>> @@ -268,7 +268,7 @@ struct pt_regs;
>>   static inline int riscv_v_setup_vsize(void) { return -EOPNOTSUPP; }
>>   static __always_inline bool has_vector(void) { return false; }
>> -static inline bool riscv_v_first_use_handler(struct pt_regs *regs) 
>> { return false; }
>> +static inline bool riscv_v_first_use_handler(struct pt_regs *regs, 
>> u32 insn) { return false; }
>>   static inline bool riscv_v_vstate_query(struct pt_regs *regs) 
>> { return false; }
>>   static inline bool riscv_v_vstate_ctrl_user_allowed(void) { return 
>> false; }
>>   #define riscv_v_vsize (0)
>> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
>> index 51ebfd23e007..1c3fab272fd1 100644
>> --- a/arch/riscv/kernel/traps.c
>> +++ b/arch/riscv/kernel/traps.c
>> @@ -172,11 +172,20 @@ asmlinkage __visible __trap_section void 
>> do_trap_insn_illegal(struct pt_regs *re
>>       bool handled;
>>       if (user_mode(regs)) {
>> +        u32 __user *epc = (u32 __user *)regs->epc;
>> +        u32 insn = (u32)regs->badaddr;
>> +
>>           irqentry_enter_from_user_mode(regs);
>>           local_irq_enable();
>> -        handled = riscv_v_first_use_handler(regs);
>> +        if (!insn) {
>> +            if (__get_user(insn, epc)) {
>> +                /* todo */
>> +            }
> 
> grr, of course as soon as it is sent I notice the /todo/ here
> 
> Not sure if we should print something if __get_user() fails for
> what /should/ be an good instruction addrss.... should probably
> at-least bail out of do_trap_insn_illegal() if not also print a
> warning.
>

i think doing:

	if (___get_user(insn, epr)) {
		handled = false;
		insn = 0;
	}

then just checking for "if (insn)" before hand handlers.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] riscv: ptrace: add regs_set_register()
  2024-11-13  9:17 ` [PATCH 1/3] riscv: ptrace: add regs_set_register() Ben Dooks
@ 2024-11-13 10:12   ` Andrew Jones
  2024-11-13 10:19     ` Ben Dooks
  0 siblings, 1 reply; 10+ messages in thread
From: Andrew Jones @ 2024-11-13 10:12 UTC (permalink / raw)
  To: Ben Dooks; +Cc: linux-riscv, palmer, alex

On Wed, Nov 13, 2024 at 09:17:01AM +0000, Ben Dooks wrote:
> Since we have regs_get_register() and we could use the set counterpart
> for things like fixing up traps, add regs_set_register() to set a pt_regs
> value from offset.
> 
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
>  arch/riscv/include/asm/ptrace.h | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/ptrace.h b/arch/riscv/include/asm/ptrace.h
> index b5b0adcc85c1..66fc1795141d 100644
> --- a/arch/riscv/include/asm/ptrace.h
> +++ b/arch/riscv/include/asm/ptrace.h
> @@ -143,6 +143,26 @@ static inline unsigned long regs_get_register(struct pt_regs *regs,
>  	return *(unsigned long *)((unsigned long)regs + offset);
>  }
>  
> +/**
> + * regs_set_register() - set register value from its offset

s/from/at/

> + * @regs:	pt_regs from which register value is gotten

s/gotten/set/

> + * @offset:	offset of the register.
> + * @to:		value to set register to

Can we name this 'value' instead of 'to'?

> + *
> + * regs_set_register sets the value @to to a register whose offset from @regs.
> + * The @offset is the offset of the register in struct pt_regs.
> + * If @offset is bigger than MAX_REG_OFFSET, this will ignore the write.
> + */
> +static inline void regs_set_register(struct pt_regs *regs,
> +				     unsigned int offset,
> +				     unsigned long to)
> +{
> +	if (unlikely(offset > MAX_REG_OFFSET))
> +		return;
> +
> +	*(unsigned long *)((unsigned long)regs + offset) = to;
> +}
> +
>  /**
>   * regs_get_kernel_argument() - get Nth function argument in kernel
>   * @regs:       pt_regs of that context
> -- 
> 2.37.2.352.g3c44437643
>

Thanks,
drew

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] riscv: insn: add RV_EXTRACT_FUNCT3()
  2024-11-13  9:17 ` [PATCH 3/3] riscv: insn: add RV_EXTRACT_FUNCT3() Ben Dooks
@ 2024-11-13 10:17   ` Andrew Jones
  2024-11-13 10:35     ` Ben Dooks
  0 siblings, 1 reply; 10+ messages in thread
From: Andrew Jones @ 2024-11-13 10:17 UTC (permalink / raw)
  To: Ben Dooks; +Cc: linux-riscv, palmer, alex

On Wed, Nov 13, 2024 at 09:17:03AM +0000, Ben Dooks wrote:
> Add extraction for the func3 field of most instructions
> for use with anyone who needs it.
> 
> Note, added this for decoding of CSR accesses for
> work we did looking at the RDCYCLE v RDTIME calls.
> 
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
>  arch/riscv/include/asm/insn.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/insn.h b/arch/riscv/include/asm/insn.h
> index 09fde95a5e8f..c67f44ff2066 100644
> --- a/arch/riscv/include/asm/insn.h
> +++ b/arch/riscv/include/asm/insn.h
> @@ -299,6 +299,10 @@ static __always_inline bool riscv_insn_is_c_jalr(u32 code)
>  	({typeof(x) x_ = (x); \
>  	(RV_X(x_, RVG_RD_OPOFF, RVG_RD_MASK)); })
>  
> +#define RV_EXTRACT_FUNCT3(x) \
> +	({typeof(x) x_ = (x); \
> +	(RV_X(x_, RV_INSN_FUNCT3_OPOFF, RV_INSN_FUNCT3_MASK)); })
> +
>  #define RV_EXTRACT_UTYPE_IMM(x) \
>  	({typeof(x) x_ = (x); \
>  	(RV_X(x_, RV_U_IMM_31_12_OPOFF, RV_U_IMM_31_12_MASK)); })
> -- 
> 2.37.2.352.g3c44437643
>

It doesn't hurt to have an unused macro and there's a reasonable chance
there will be a user someday, but I'd typically wait for a user before
adding stuff. Anyway,

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Thanks,
drew

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] riscv: ptrace: add regs_set_register()
  2024-11-13 10:12   ` Andrew Jones
@ 2024-11-13 10:19     ` Ben Dooks
  0 siblings, 0 replies; 10+ messages in thread
From: Ben Dooks @ 2024-11-13 10:19 UTC (permalink / raw)
  To: Andrew Jones; +Cc: linux-riscv, palmer, alex

On 13/11/2024 10:12, Andrew Jones wrote:
> On Wed, Nov 13, 2024 at 09:17:01AM +0000, Ben Dooks wrote:
>> Since we have regs_get_register() and we could use the set counterpart
>> for things like fixing up traps, add regs_set_register() to set a pt_regs
>> value from offset.
>>
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>> ---
>>   arch/riscv/include/asm/ptrace.h | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/arch/riscv/include/asm/ptrace.h b/arch/riscv/include/asm/ptrace.h
>> index b5b0adcc85c1..66fc1795141d 100644
>> --- a/arch/riscv/include/asm/ptrace.h
>> +++ b/arch/riscv/include/asm/ptrace.h
>> @@ -143,6 +143,26 @@ static inline unsigned long regs_get_register(struct pt_regs *regs,
>>   	return *(unsigned long *)((unsigned long)regs + offset);
>>   }
>>   
>> +/**
>> + * regs_set_register() - set register value from its offset
> 
> s/from/at/
> 
>> + * @regs:	pt_regs from which register value is gotten
> 
> s/gotten/set/
> 
>> + * @offset:	offset of the register.
>> + * @to:		value to set register to
> 
> Can we name this 'value' instead of 'to'?

Thank you, I think these are all good comments.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] riscv: insn: add RV_EXTRACT_FUNCT3()
  2024-11-13 10:17   ` Andrew Jones
@ 2024-11-13 10:35     ` Ben Dooks
  0 siblings, 0 replies; 10+ messages in thread
From: Ben Dooks @ 2024-11-13 10:35 UTC (permalink / raw)
  To: Andrew Jones; +Cc: linux-riscv, palmer, alex

On 13/11/2024 10:17, Andrew Jones wrote:
> On Wed, Nov 13, 2024 at 09:17:03AM +0000, Ben Dooks wrote:
>> Add extraction for the func3 field of most instructions
>> for use with anyone who needs it.
>>
>> Note, added this for decoding of CSR accesses for
>> work we did looking at the RDCYCLE v RDTIME calls.
>>
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>> ---
>>   arch/riscv/include/asm/insn.h | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/riscv/include/asm/insn.h b/arch/riscv/include/asm/insn.h
>> index 09fde95a5e8f..c67f44ff2066 100644
>> --- a/arch/riscv/include/asm/insn.h
>> +++ b/arch/riscv/include/asm/insn.h
>> @@ -299,6 +299,10 @@ static __always_inline bool riscv_insn_is_c_jalr(u32 code)
>>   	({typeof(x) x_ = (x); \
>>   	(RV_X(x_, RVG_RD_OPOFF, RVG_RD_MASK)); })
>>   
>> +#define RV_EXTRACT_FUNCT3(x) \
>> +	({typeof(x) x_ = (x); \
>> +	(RV_X(x_, RV_INSN_FUNCT3_OPOFF, RV_INSN_FUNCT3_MASK)); })
>> +
>>   #define RV_EXTRACT_UTYPE_IMM(x) \
>>   	({typeof(x) x_ = (x); \
>>   	(RV_X(x_, RV_U_IMM_31_12_OPOFF, RV_U_IMM_31_12_MASK)); })
>> -- 
>> 2.37.2.352.g3c44437643
>>
> 
> It doesn't hurt to have an unused macro and there's a reasonable chance
> there will be a user someday, but I'd typically wait for a user before
> adding stuff. Anyway,
> 
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

I am wondering if we add a check for RDCYCLE and print a warning
even if we didn't try an fixup.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-11-13 10:37 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-13  9:17 updates for instruction trapping and ptrace Ben Dooks
2024-11-13  9:17 ` [PATCH 1/3] riscv: ptrace: add regs_set_register() Ben Dooks
2024-11-13 10:12   ` Andrew Jones
2024-11-13 10:19     ` Ben Dooks
2024-11-13  9:17 ` [PATCH 2/3] riscv: traps: make insn fetch common in unknown instruction Ben Dooks
2024-11-13  9:21   ` Ben Dooks
2024-11-13  9:31     ` Ben Dooks
2024-11-13  9:17 ` [PATCH 3/3] riscv: insn: add RV_EXTRACT_FUNCT3() Ben Dooks
2024-11-13 10:17   ` Andrew Jones
2024-11-13 10:35     ` Ben Dooks

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox