[PATCH v3 0/6] Add FIELD_MODIFY() helper

[PATCH v3 0/6] Add FIELD_MODIFY() helper

[Luo Jie]

Add the helper FIELD_MODIFY() to the FIELD_XXX family of bitfield
macros. It is functionally similar as xxx_replace_bits(), but adds
the compile time checking to catch incorrect parameter type errors.

This series also converts the four instances of opencoded FIELD_MODIFY()
that are found in the core kernel files, to instead use the new
FIELD_MODIFY() macro. This is achieved with Coccinelle, by adding
the script field_modify.cocci.

The changes are validated on IPQ9574 SoC which uses ARM64 architecture.

Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
---
Changes in v3:
- Correct the order of header files included.
- Add the Coccinelle script field_modify.cocci..
- Convert the opencoded FIELD_MODIFY() variants inside arm64 directory,
  identified by field_modify.cocci.
- Link to v2: https://lore.kernel.org/all/20250410131048.2054791-1-quic_luoj@quicinc.com/

Changes in v2:
- Update the documented example for FIELD_MODIFY().
- Improve the commit message to describe the need for the change.
- Link to v1: https://lore.kernel.org/all/20250318071526.1836194-1-quic_luoj@quicinc.com/

---
Luo Jie (6):
      bitfield: Add FIELD_MODIFY() helper
      coccinelle: misc: Add field_modify script
      arm64: tlb: Convert the opencoded field modify
      arm64: nvhe: Convert the opencoded field modify
      arm64: kvm: Convert the opencoded field modify
      arm64: mm: Convert the opencoded field modify

 arch/arm64/include/asm/tlbflush.h          |  3 +--
 arch/arm64/kvm/hyp/include/nvhe/memory.h   |  3 +--
 arch/arm64/kvm/vgic/vgic-mmio-v3.c         |  6 ++----
 arch/arm64/mm/context.c                    |  3 +--
 include/linux/bitfield.h                   | 21 +++++++++++++++++++--
 scripts/coccinelle/misc/field_modify.cocci | 24 ++++++++++++++++++++++++
 6 files changed, 48 insertions(+), 12 deletions(-)
---
base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8
change-id: 20250411-field_modify-83b58db99025

Best regards,
-- 
Luo Jie <quic_luoj@quicinc.com>

[PATCH v3 1/6] bitfield: Add FIELD_MODIFY() helper

[Luo Jie]

Add a helper for replacing the contents of bitfield in memory
with the specified value.

Even though a helper xxx_replace_bits() is available, it is not
well documented, and only reports errors at the run time, which
will not be helpful to catch possible overflow errors due to
incorrect parameter types used.

Add the helper FIELD_MODIFY() to the FIELD_XXX family of bitfield
macros. It is functionally similar as xxx_replace_bits(), and in
addition adds the compile time type checking.

FIELD_MODIFY(REG_FIELD_C, &reg, c) is the wrapper of the code below.
reg &= ~REG_FIELD_C;
reg |= FIELD_PREP(REG_FIELD_C, c);

Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
---
 include/linux/bitfield.h | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index 63928f173223..2eaefa76f759 100644
--- a/include/linux/bitfield.h
+++ b/include/linux/bitfield.h
@@ -8,6 +8,7 @@
 #define _LINUX_BITFIELD_H
 
 #include <linux/build_bug.h>
+#include <linux/typecheck.h>
 #include <asm/byteorder.h>
 
 /*
@@ -38,8 +39,7 @@
  *	  FIELD_PREP(REG_FIELD_D, 0x40);
  *
  * Modify:
- *  reg &= ~REG_FIELD_C;
- *  reg |= FIELD_PREP(REG_FIELD_C, c);
+ *  FIELD_MODIFY(REG_FIELD_C, &reg, c);
  */
 
 #define __bf_shf(x) (__builtin_ffsll(x) - 1)
@@ -156,6 +156,23 @@
 		(typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask));	\
 	})
 
+/**
+ * FIELD_MODIFY() - modify a bitfield element
+ * @_mask: shifted mask defining the field's length and position
+ * @_reg_p: pointer to the memory that should be updated
+ * @_val: value to store in the bitfield
+ *
+ * FIELD_MODIFY() modifies the set of bits in @_reg_p specified by @_mask,
+ * by replacing them with the bitfield value passed in as @_val.
+ */
+#define FIELD_MODIFY(_mask, _reg_p, _val)				\
+	({								\
+		typecheck_pointer(_reg_p);				\
+		__BF_FIELD_CHECK(_mask, *(_reg_p), _val, "FIELD_MODIFY: ");		\
+		*(_reg_p) &= ~(_mask);							\
+		*(_reg_p) |= (((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask));	\
+	})
+
 extern void __compiletime_error("value doesn't fit into mask")
 __field_overflow(void);
 extern void __compiletime_error("bad bitfield mask")

-- 
2.34.1

[PATCH v3 2/6] coccinelle: misc: Add field_modify script

[Luo Jie]

This script finds and suggests conversions of opencoded field
modify patterns with the wrapper FIELD_MODIFY() API defined in
include/linux/bitfield.h for better readability.

Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
---
 scripts/coccinelle/misc/field_modify.cocci | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/scripts/coccinelle/misc/field_modify.cocci b/scripts/coccinelle/misc/field_modify.cocci
new file mode 100644
index 000000000000..9022c1b23291
--- /dev/null
+++ b/scripts/coccinelle/misc/field_modify.cocci
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Replaced below code with the wrapper FIELD_MODIFY(MASK, &reg, val)
+/// - reg &= ~MASK;
+/// - reg |= FIELD_PREP(MASK, val);
+//
+// Confidence: High
+// Author: Luo Jie <quic_luoj@quicinc.com>
+// Copyright (C) 2025 Qualcomm Innovation Center, Inc.
+// URL: https://coccinelle.gitlabpages.inria.fr/website
+// Keywords: FIELD_PREP, FIELD_MODIFY
+// Options: --include-headers
+
+virtual patch
+
+@depends on patch@
+identifier reg, val;
+constant mask;
+symbol FIELD_PREP, FIELD_MODIFY;
+@@
+
+- reg &= ~mask;
+- reg |= FIELD_PREP(mask, val);
++ FIELD_MODIFY(mask, &reg, val);

-- 
2.34.1

[PATCH v3 3/6] arm64: tlb: Convert the opencoded field modify

[Luo Jie]

Replaced below code with the wrapper FIELD_MODIFY(MASK, &reg, val)
- reg &= ~MASK;
- reg |= FIELD_PREP(MASK, val);
The semantic patch that makes this change is available
in scripts/coccinelle/misc/field_modify.cocci.

More information about semantic patching is available at
https://coccinelle.gitlabpages.inria.fr/website

Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
---
 arch/arm64/include/asm/tlbflush.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/tlbflush.h b/arch/arm64/include/asm/tlbflush.h
index eba1a98657f1..0d250ef4161c 100644
--- a/arch/arm64/include/asm/tlbflush.h
+++ b/arch/arm64/include/asm/tlbflush.h
@@ -112,8 +112,7 @@ static inline unsigned long get_trans_granule(void)
 	    level >= 0 && level <= 3) {					\
 		u64 ttl = level & 3;					\
 		ttl |= get_trans_granule() << 2;			\
-		arg &= ~TLBI_TTL_MASK;					\
-		arg |= FIELD_PREP(TLBI_TTL_MASK, ttl);			\
+		FIELD_MODIFY(TLBI_TTL_MASK, &arg, ttl);			\
 	}								\
 									\
 	__tlbi(op, arg);						\

-- 
2.34.1

[PATCH v3 4/6] arm64: nvhe: Convert the opencoded field modify

[Luo Jie]

Replaced below code with the wrapper FIELD_MODIFY(MASK, &reg, val)
- reg &= ~MASK;
- reg |= FIELD_PREP(MASK, val);
The semantic patch that makes this change is available
in scripts/coccinelle/misc/field_modify.cocci.

More information about semantic patching is available at
https://coccinelle.gitlabpages.inria.fr/website

Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
---
 arch/arm64/kvm/hyp/include/nvhe/memory.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/hyp/include/nvhe/memory.h b/arch/arm64/kvm/hyp/include/nvhe/memory.h
index 34233d586060..b2af748964d0 100644
--- a/arch/arm64/kvm/hyp/include/nvhe/memory.h
+++ b/arch/arm64/kvm/hyp/include/nvhe/memory.h
@@ -30,8 +30,7 @@ enum pkvm_page_state {
 static inline enum kvm_pgtable_prot pkvm_mkstate(enum kvm_pgtable_prot prot,
 						 enum pkvm_page_state state)
 {
-	prot &= ~PKVM_PAGE_STATE_PROT_MASK;
-	prot |= FIELD_PREP(PKVM_PAGE_STATE_PROT_MASK, state);
+	FIELD_MODIFY(PKVM_PAGE_STATE_PROT_MASK, &prot, state);
 	return prot;
 }
 

-- 
2.34.1

[PATCH v3 5/6] arm64: kvm: Convert the opencoded field modify

[Luo Jie]

Replaced below code with the wrapper FIELD_MODIFY(MASK, &reg, val)
- reg &= ~MASK;
- reg |= FIELD_PREP(MASK, val);
The semantic patch that makes this change is available
in scripts/coccinelle/misc/field_modify.cocci.

More information about semantic patching is available at
https://coccinelle.gitlabpages.inria.fr/website

Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
---
 arch/arm64/kvm/vgic/vgic-mmio-v3.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index ae4c0593d114..946db5b3500f 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -123,10 +123,8 @@ static void vgic_mmio_write_v3_misc(struct kvm_vcpu *vcpu,
 			val &= ~GICD_CTLR_nASSGIreq;
 
 		/* Dist stays enabled? nASSGIreq is RO */
-		if (was_enabled && dist->enabled) {
-			val &= ~GICD_CTLR_nASSGIreq;
-			val |= FIELD_PREP(GICD_CTLR_nASSGIreq, is_hwsgi);
-		}
+		if (was_enabled && dist->enabled)
+			FIELD_MODIFY(GICD_CTLR_nASSGIreq, &val, is_hwsgi);
 
 		/* Switching HW SGIs? */
 		dist->nassgireq = val & GICD_CTLR_nASSGIreq;

-- 
2.34.1

[PATCH v3 6/6] arm64: mm: Convert the opencoded field modify

[Luo Jie]

Replaced below code with the wrapper FIELD_MODIFY(MASK, &reg, val)
- reg &= ~MASK;
- reg |= FIELD_PREP(MASK, val);
The semantic patch that makes this change is available
in scripts/coccinelle/misc/field_modify.cocci.

More information about semantic patching is available at
https://coccinelle.gitlabpages.inria.fr/website

Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
---
 arch/arm64/mm/context.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm64/mm/context.c b/arch/arm64/mm/context.c
index b2ac06246327..4fbac8e74149 100644
--- a/arch/arm64/mm/context.c
+++ b/arch/arm64/mm/context.c
@@ -361,8 +361,7 @@ void cpu_do_switch_mm(phys_addr_t pgd_phys, struct mm_struct *mm)
 		ttbr0 |= FIELD_PREP(TTBR_ASID_MASK, asid);
 
 	/* Set ASID in TTBR1 since TCR.A1 is set */
-	ttbr1 &= ~TTBR_ASID_MASK;
-	ttbr1 |= FIELD_PREP(TTBR_ASID_MASK, asid);
+	FIELD_MODIFY(TTBR_ASID_MASK, &ttbr1, asid);
 
 	cpu_set_reserved_ttbr0_nosync();
 	write_sysreg(ttbr1, ttbr1_el1);

-- 
2.34.1

Re: [PATCH v3 0/6] Add FIELD_MODIFY() helper

[Marc Zyngier]

On Thu, 17 Apr 2025 11:47:07 +0100,
Luo Jie <quic_luoj@quicinc.com> wrote:
> 
> Add the helper FIELD_MODIFY() to the FIELD_XXX family of bitfield
> macros. It is functionally similar as xxx_replace_bits(), but adds
> the compile time checking to catch incorrect parameter type errors.
> 
> This series also converts the four instances of opencoded FIELD_MODIFY()
> that are found in the core kernel files, to instead use the new
> FIELD_MODIFY() macro. This is achieved with Coccinelle, by adding
> the script field_modify.cocci.
> 
> The changes are validated on IPQ9574 SoC which uses ARM64 architecture.

We already have the *_replace_bits() functions (see
include/linux/bitfield.h).

Why do we need extra helpers?

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v3 4/6] arm64: nvhe: Convert the opencoded field modify

[Marc Zyngier]

On Thu, 17 Apr 2025 11:47:11 +0100,
Luo Jie <quic_luoj@quicinc.com> wrote:
> 
> Replaced below code with the wrapper FIELD_MODIFY(MASK, &reg, val)
> - reg &= ~MASK;
> - reg |= FIELD_PREP(MASK, val);
> The semantic patch that makes this change is available
> in scripts/coccinelle/misc/field_modify.cocci.
> 
> More information about semantic patching is available at
> https://coccinelle.gitlabpages.inria.fr/website
> 
> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> ---
>  arch/arm64/kvm/hyp/include/nvhe/memory.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/include/nvhe/memory.h b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> index 34233d586060..b2af748964d0 100644
> --- a/arch/arm64/kvm/hyp/include/nvhe/memory.h
> +++ b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> @@ -30,8 +30,7 @@ enum pkvm_page_state {
>  static inline enum kvm_pgtable_prot pkvm_mkstate(enum kvm_pgtable_prot prot,
>  						 enum pkvm_page_state state)
>  {
> -	prot &= ~PKVM_PAGE_STATE_PROT_MASK;
> -	prot |= FIELD_PREP(PKVM_PAGE_STATE_PROT_MASK, state);
> +	FIELD_MODIFY(PKVM_PAGE_STATE_PROT_MASK, &prot, state);
>  	return prot;
>  }

Following up on my suggestion to *not* add anything new, this patch
could be written as:

diff --git a/arch/arm64/kvm/hyp/include/nvhe/memory.h b/arch/arm64/kvm/hyp/include/nvhe/memory.h
index 34233d5860607..08cb6ba0e0716 100644
--- a/arch/arm64/kvm/hyp/include/nvhe/memory.h
+++ b/arch/arm64/kvm/hyp/include/nvhe/memory.h
@@ -30,9 +30,8 @@ enum pkvm_page_state {
 static inline enum kvm_pgtable_prot pkvm_mkstate(enum kvm_pgtable_prot prot,
 						 enum pkvm_page_state state)
 {
-	prot &= ~PKVM_PAGE_STATE_PROT_MASK;
-	prot |= FIELD_PREP(PKVM_PAGE_STATE_PROT_MASK, state);
-	return prot;
+	u64 p = prot;
+	return u64_replace_bits(p, state, PKVM_PAGE_STATE_PROT_MASK);
 }
 
 static inline enum pkvm_page_state pkvm_getstate(enum kvm_pgtable_prot prot)

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v3 0/6] Add FIELD_MODIFY() helper

[Andrew Lunn]

On Thu, Apr 17, 2025 at 12:10:54PM +0100, Marc Zyngier wrote:
> On Thu, 17 Apr 2025 11:47:07 +0100,
> Luo Jie <quic_luoj@quicinc.com> wrote:
> > 
> > Add the helper FIELD_MODIFY() to the FIELD_XXX family of bitfield
> > macros. It is functionally similar as xxx_replace_bits(), but adds
> > the compile time checking to catch incorrect parameter type errors.
> > 
> > This series also converts the four instances of opencoded FIELD_MODIFY()
> > that are found in the core kernel files, to instead use the new
> > FIELD_MODIFY() macro. This is achieved with Coccinelle, by adding
> > the script field_modify.cocci.
> > 
> > The changes are validated on IPQ9574 SoC which uses ARM64 architecture.
> 
> We already have the *_replace_bits() functions (see
> include/linux/bitfield.h).
> 
> Why do we need extra helpers?

If you look at bitfield.h, the *_replace_bits() seem to be
undocumented internal macro magic, not something you are expected to
use. What you are expected to use in that file is however well
documented. The macro magic also means that cross referencing tools
don't find them.

I think this is a useful additional to bitfield.h.

	Andrew

Re: [PATCH v3 0/6] Add FIELD_MODIFY() helper

[Marc Zyngier]

On Thu, 17 Apr 2025 18:22:29 +0100,
Andrew Lunn <andrew@lunn.ch> wrote:
> 
> On Thu, Apr 17, 2025 at 12:10:54PM +0100, Marc Zyngier wrote:
> > On Thu, 17 Apr 2025 11:47:07 +0100,
> > Luo Jie <quic_luoj@quicinc.com> wrote:
> > > 
> > > Add the helper FIELD_MODIFY() to the FIELD_XXX family of bitfield
> > > macros. It is functionally similar as xxx_replace_bits(), but adds
> > > the compile time checking to catch incorrect parameter type errors.
> > > 
> > > This series also converts the four instances of opencoded FIELD_MODIFY()
> > > that are found in the core kernel files, to instead use the new
> > > FIELD_MODIFY() macro. This is achieved with Coccinelle, by adding
> > > the script field_modify.cocci.
> > > 
> > > The changes are validated on IPQ9574 SoC which uses ARM64 architecture.
> > 
> > We already have the *_replace_bits() functions (see
> > include/linux/bitfield.h).
> > 
> > Why do we need extra helpers?
> 
> If you look at bitfield.h, the *_replace_bits() seem to be
> undocumented internal macro magic, not something you are expected to
> use. What you are expected to use in that file is however well
> documented. The macro magic also means that cross referencing tools
> don't find them.

$ git grep _replace_bits|  wc -l
1514

I think a bunch of people have found them, tooling notwithstanding.

As for the documentation, the commit message in 00b0c9b82663ac would
be advantageously promoted to full-fledged kernel-doc.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v3 3/6] arm64: tlb: Convert the opencoded field modify

[Russell King (Oracle)]

On Thu, Apr 17, 2025 at 06:47:10PM +0800, Luo Jie wrote:
> Replaced below code with the wrapper FIELD_MODIFY(MASK, &reg, val)
> - reg &= ~MASK;
> - reg |= FIELD_PREP(MASK, val);
> The semantic patch that makes this change is available
> in scripts/coccinelle/misc/field_modify.cocci.
> 
> More information about semantic patching is available at
> https://coccinelle.gitlabpages.inria.fr/website
> 
> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> ---
>  arch/arm64/include/asm/tlbflush.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/tlbflush.h b/arch/arm64/include/asm/tlbflush.h
> index eba1a98657f1..0d250ef4161c 100644
> --- a/arch/arm64/include/asm/tlbflush.h
> +++ b/arch/arm64/include/asm/tlbflush.h
> @@ -112,8 +112,7 @@ static inline unsigned long get_trans_granule(void)
>  	    level >= 0 && level <= 3) {					\
>  		u64 ttl = level & 3;					\
>  		ttl |= get_trans_granule() << 2;			\
> -		arg &= ~TLBI_TTL_MASK;					\
> -		arg |= FIELD_PREP(TLBI_TTL_MASK, ttl);			\
> +		FIELD_MODIFY(TLBI_TTL_MASK, &arg, ttl);			\

This could equally be:
		arg = u64_replace_bits(arg, ttl, TLBI_TTL_MASK);

which already exists, so doesn't require a new macro to be introduced.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v3 0/6] Add FIELD_MODIFY() helper

[Yury Norov]

On Thu, Apr 17, 2025 at 06:45:12PM +0100, Marc Zyngier wrote:
> On Thu, 17 Apr 2025 18:22:29 +0100,
> Andrew Lunn <andrew@lunn.ch> wrote:
> > 
> > On Thu, Apr 17, 2025 at 12:10:54PM +0100, Marc Zyngier wrote:
> > > On Thu, 17 Apr 2025 11:47:07 +0100,
> > > Luo Jie <quic_luoj@quicinc.com> wrote:
> > > > 
> > > > Add the helper FIELD_MODIFY() to the FIELD_XXX family of bitfield
> > > > macros. It is functionally similar as xxx_replace_bits(), but adds
> > > > the compile time checking to catch incorrect parameter type errors.
> > > > 
> > > > This series also converts the four instances of opencoded FIELD_MODIFY()
> > > > that are found in the core kernel files, to instead use the new
> > > > FIELD_MODIFY() macro. This is achieved with Coccinelle, by adding
> > > > the script field_modify.cocci.
> > > > 
> > > > The changes are validated on IPQ9574 SoC which uses ARM64 architecture.
> > > 
> > > We already have the *_replace_bits() functions (see
> > > include/linux/bitfield.h).
> > > 
> > > Why do we need extra helpers?
> > 
> > If you look at bitfield.h, the *_replace_bits() seem to be
> > undocumented internal macro magic, not something you are expected to
> > use. What you are expected to use in that file is however well
> > documented. The macro magic also means that cross referencing tools
> > don't find them.
> 
> $ git grep _replace_bits|  wc -l
> 1514

FIELD_PREP() only is used 10 times more.
 
> I think a bunch of people have found them, tooling notwithstanding.
> 
> As for the documentation, the commit message in 00b0c9b82663ac would
> be advantageously promoted to full-fledged kernel-doc.

The FIELD_MODIFY() and uxx_replace_bits() are simply different things.

FIELD_MODIFY() employs __BF_FIELD_CHECK(), which allows strict
parameters checking at compile time. And people like it. See
recent fixed-size GENMASK() series:

https://patchwork.kernel.org/comment/26283604/

The _replace_bits() functions return fixed-width values, and intended
for: "manipulating bitfields both in host- and fixed-endian", as the
very first line in the commit message says.

Those using _replace_bits() for something else abuse the API, and
should switch to FIELD_MODIFY().

Re: [PATCH v3 4/6] arm64: nvhe: Convert the opencoded field modify

[Yury Norov]

On Thu, Apr 17, 2025 at 12:23:10PM +0100, Marc Zyngier wrote:
> On Thu, 17 Apr 2025 11:47:11 +0100,
> Luo Jie <quic_luoj@quicinc.com> wrote:
> > 
> > Replaced below code with the wrapper FIELD_MODIFY(MASK, &reg, val)
> > - reg &= ~MASK;
> > - reg |= FIELD_PREP(MASK, val);
> > The semantic patch that makes this change is available
> > in scripts/coccinelle/misc/field_modify.cocci.
> > 
> > More information about semantic patching is available at
> > https://coccinelle.gitlabpages.inria.fr/website
> > 
> > Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> > ---
> >  arch/arm64/kvm/hyp/include/nvhe/memory.h | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/hyp/include/nvhe/memory.h b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > index 34233d586060..b2af748964d0 100644
> > --- a/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > +++ b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > @@ -30,8 +30,7 @@ enum pkvm_page_state {
> >  static inline enum kvm_pgtable_prot pkvm_mkstate(enum kvm_pgtable_prot prot,
> >  						 enum pkvm_page_state state)
> >  {
> > -	prot &= ~PKVM_PAGE_STATE_PROT_MASK;
> > -	prot |= FIELD_PREP(PKVM_PAGE_STATE_PROT_MASK, state);
> > +	FIELD_MODIFY(PKVM_PAGE_STATE_PROT_MASK, &prot, state);
> >  	return prot;
> >  }
> 
> Following up on my suggestion to *not* add anything new, this patch
> could be written as:
> 
> diff --git a/arch/arm64/kvm/hyp/include/nvhe/memory.h b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> index 34233d5860607..08cb6ba0e0716 100644
> --- a/arch/arm64/kvm/hyp/include/nvhe/memory.h
> +++ b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> @@ -30,9 +30,8 @@ enum pkvm_page_state {
>  static inline enum kvm_pgtable_prot pkvm_mkstate(enum kvm_pgtable_prot prot,
>  						 enum pkvm_page_state state)
>  {
> -	prot &= ~PKVM_PAGE_STATE_PROT_MASK;
> -	prot |= FIELD_PREP(PKVM_PAGE_STATE_PROT_MASK, state);
> -	return prot;
> +	u64 p = prot;
> +	return u64_replace_bits(p, state, PKVM_PAGE_STATE_PROT_MASK);
>  }

This is a great example where u64_replace_bit() should NOT be used. 

Thanks,
Yury

Re: [PATCH v3 4/6] arm64: nvhe: Convert the opencoded field modify

[Marc Zyngier]

On Fri, 18 Apr 2025 16:14:48 +0100,
Yury Norov <yury.norov@gmail.com> wrote:
> 
> On Thu, Apr 17, 2025 at 12:23:10PM +0100, Marc Zyngier wrote:
> > On Thu, 17 Apr 2025 11:47:11 +0100,
> > Luo Jie <quic_luoj@quicinc.com> wrote:
> > > 
> > > Replaced below code with the wrapper FIELD_MODIFY(MASK, &reg, val)
> > > - reg &= ~MASK;
> > > - reg |= FIELD_PREP(MASK, val);
> > > The semantic patch that makes this change is available
> > > in scripts/coccinelle/misc/field_modify.cocci.
> > > 
> > > More information about semantic patching is available at
> > > https://coccinelle.gitlabpages.inria.fr/website
> > > 
> > > Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> > > ---
> > >  arch/arm64/kvm/hyp/include/nvhe/memory.h | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kvm/hyp/include/nvhe/memory.h b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > > index 34233d586060..b2af748964d0 100644
> > > --- a/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > > +++ b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > > @@ -30,8 +30,7 @@ enum pkvm_page_state {
> > >  static inline enum kvm_pgtable_prot pkvm_mkstate(enum kvm_pgtable_prot prot,
> > >  						 enum pkvm_page_state state)
> > >  {
> > > -	prot &= ~PKVM_PAGE_STATE_PROT_MASK;
> > > -	prot |= FIELD_PREP(PKVM_PAGE_STATE_PROT_MASK, state);
> > > +	FIELD_MODIFY(PKVM_PAGE_STATE_PROT_MASK, &prot, state);
> > >  	return prot;
> > >  }
> > 
> > Following up on my suggestion to *not* add anything new, this patch
> > could be written as:
> > 
> > diff --git a/arch/arm64/kvm/hyp/include/nvhe/memory.h b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > index 34233d5860607..08cb6ba0e0716 100644
> > --- a/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > +++ b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > @@ -30,9 +30,8 @@ enum pkvm_page_state {
> >  static inline enum kvm_pgtable_prot pkvm_mkstate(enum kvm_pgtable_prot prot,
> >  						 enum pkvm_page_state state)
> >  {
> > -	prot &= ~PKVM_PAGE_STATE_PROT_MASK;
> > -	prot |= FIELD_PREP(PKVM_PAGE_STATE_PROT_MASK, state);
> > -	return prot;
> > +	u64 p = prot;
> > +	return u64_replace_bits(p, state, PKVM_PAGE_STATE_PROT_MASK);
> >  }
> 
> This is a great example where u64_replace_bit() should NOT be used. 

Well, I'll like well it enough.

	M.

-- 
Jazz isn't dead. It just smells funny.

Re: [PATCH v3 0/6] Add FIELD_MODIFY() helper

[Marc Zyngier]

On Fri, 18 Apr 2025 16:08:38 +0100,
Yury Norov <yury.norov@gmail.com> wrote:
> 
> On Thu, Apr 17, 2025 at 06:45:12PM +0100, Marc Zyngier wrote:
> > On Thu, 17 Apr 2025 18:22:29 +0100,
> > Andrew Lunn <andrew@lunn.ch> wrote:
> > > 
> > > On Thu, Apr 17, 2025 at 12:10:54PM +0100, Marc Zyngier wrote:
> > > > On Thu, 17 Apr 2025 11:47:07 +0100,
> > > > Luo Jie <quic_luoj@quicinc.com> wrote:
> > > > > 
> > > > > Add the helper FIELD_MODIFY() to the FIELD_XXX family of bitfield
> > > > > macros. It is functionally similar as xxx_replace_bits(), but adds
> > > > > the compile time checking to catch incorrect parameter type errors.
> > > > > 
> > > > > This series also converts the four instances of opencoded FIELD_MODIFY()
> > > > > that are found in the core kernel files, to instead use the new
> > > > > FIELD_MODIFY() macro. This is achieved with Coccinelle, by adding
> > > > > the script field_modify.cocci.
> > > > > 
> > > > > The changes are validated on IPQ9574 SoC which uses ARM64 architecture.
> > > > 
> > > > We already have the *_replace_bits() functions (see
> > > > include/linux/bitfield.h).
> > > > 
> > > > Why do we need extra helpers?
> > > 
> > > If you look at bitfield.h, the *_replace_bits() seem to be
> > > undocumented internal macro magic, not something you are expected to
> > > use. What you are expected to use in that file is however well
> > > documented. The macro magic also means that cross referencing tools
> > > don't find them.
> > 
> > $ git grep _replace_bits|  wc -l
> > 1514
> 
> FIELD_PREP() only is used 10 times more.

And? I'm sure that if you count "+", you'll find it to be yet a few
order of magnitudes more.

>  
> > I think a bunch of people have found them, tooling notwithstanding.
> > 
> > As for the documentation, the commit message in 00b0c9b82663ac would
> > be advantageously promoted to full-fledged kernel-doc.
> 
> The FIELD_MODIFY() and uxx_replace_bits() are simply different things.
> 
> FIELD_MODIFY() employs __BF_FIELD_CHECK(), which allows strict
> parameters checking at compile time. And people like it. See
> recent fixed-size GENMASK() series:
> 
> https://patchwork.kernel.org/comment/26283604/

I don't care much for what people like or not. I don't see this
FIELD_MODIFY() as a particular improvement on *_replace_bits().

> The _replace_bits() functions return fixed-width values, and intended
> for: "manipulating bitfields both in host- and fixed-endian", as the
> very first line in the commit message says.
> 
> Those using _replace_bits() for something else abuse the API, and
> should switch to FIELD_MODIFY().

Or not.

	M.

-- 
Jazz isn't dead. It just smells funny.

Re: [PATCH v3 0/6] Add FIELD_MODIFY() helper

[Yury Norov]

On Fri, Apr 18, 2025 at 04:35:22PM +0100, Marc Zyngier wrote:
> On Fri, 18 Apr 2025 16:08:38 +0100,
> Yury Norov <yury.norov@gmail.com> wrote:
> > 
> > On Thu, Apr 17, 2025 at 06:45:12PM +0100, Marc Zyngier wrote:
> > > On Thu, 17 Apr 2025 18:22:29 +0100,
> > > Andrew Lunn <andrew@lunn.ch> wrote:
> > > > 
> > > > On Thu, Apr 17, 2025 at 12:10:54PM +0100, Marc Zyngier wrote:
> > > > > On Thu, 17 Apr 2025 11:47:07 +0100,
> > > > > Luo Jie <quic_luoj@quicinc.com> wrote:
> > > > > > 
> > > > > > Add the helper FIELD_MODIFY() to the FIELD_XXX family of bitfield
> > > > > > macros. It is functionally similar as xxx_replace_bits(), but adds
> > > > > > the compile time checking to catch incorrect parameter type errors.
> > > > > > 
> > > > > > This series also converts the four instances of opencoded FIELD_MODIFY()
> > > > > > that are found in the core kernel files, to instead use the new
> > > > > > FIELD_MODIFY() macro. This is achieved with Coccinelle, by adding
> > > > > > the script field_modify.cocci.
> > > > > > 
> > > > > > The changes are validated on IPQ9574 SoC which uses ARM64 architecture.
> > > > > 
> > > > > We already have the *_replace_bits() functions (see
> > > > > include/linux/bitfield.h).
> > > > > 
> > > > > Why do we need extra helpers?
> > > > 
> > > > If you look at bitfield.h, the *_replace_bits() seem to be
> > > > undocumented internal macro magic, not something you are expected to
> > > > use. What you are expected to use in that file is however well
> > > > documented. The macro magic also means that cross referencing tools
> > > > don't find them.
> > > 
> > > $ git grep _replace_bits|  wc -l
> > > 1514
> > 
> > FIELD_PREP() only is used 10 times more.
> 
> And? I'm sure that if you count "+", you'll find it to be yet a few
> order of magnitudes more.

And nothing. It seems that you like statistics, so I shared some more.

> > > I think a bunch of people have found them, tooling notwithstanding.
> > > 
> > > As for the documentation, the commit message in 00b0c9b82663ac would
> > > be advantageously promoted to full-fledged kernel-doc.
> > 
> > The FIELD_MODIFY() and uxx_replace_bits() are simply different things.
> > 
> > FIELD_MODIFY() employs __BF_FIELD_CHECK(), which allows strict
> > parameters checking at compile time. And people like it. See
> > recent fixed-size GENMASK() series:
> > 
> > https://patchwork.kernel.org/comment/26283604/
> 
> I don't care much for what people like or not. I don't see this
> FIELD_MODIFY() as a particular improvement on *_replace_bits().

Sad to hear that. Those people are all kernel engineers, and they
deserve some respect.

We are talking about tooling here. People use tools only if they like
them. Luo likes FIELD_MODIFY() over (yes, undocumented and ungreppable)
xx_replace_bits() for the reasons he described very clearly. He's going
to use it in his driver shortly, and this arm64 detour has been made
after my request.

From my perspective, both functions have their right to live in kernel.
They are similar but not identical.

I'll take patch #1 in my branch. Regarding ARM64 part - it's up to you
either to switch to FIELD_MODIFY(), _replace_bits(), or do nothing.

Thanks,
Yury

Re: [PATCH v3 1/6] bitfield: Add FIELD_MODIFY() helper

[Yury Norov]

On Thu, Apr 17, 2025 at 06:47:08PM +0800, Luo Jie wrote:
> Add a helper for replacing the contents of bitfield in memory
> with the specified value.
> 
> Even though a helper xxx_replace_bits() is available, it is not
> well documented, and only reports errors at the run time, which
> will not be helpful to catch possible overflow errors due to
> incorrect parameter types used.
> 
> Add the helper FIELD_MODIFY() to the FIELD_XXX family of bitfield
> macros. It is functionally similar as xxx_replace_bits(), and in
> addition adds the compile time type checking.

This paragraph duplicates the above. I'll drop it and take this
patch to bitmap-for-next. Regarding the rest of the series - it's up
to ARM64 and Cocci maintainers if they want them or not.

Thanks for the work!

Thanks,
Yury
 
> FIELD_MODIFY(REG_FIELD_C, &reg, c) is the wrapper of the code below.
> reg &= ~REG_FIELD_C;
> reg |= FIELD_PREP(REG_FIELD_C, c);
> 
> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> ---
>  include/linux/bitfield.h | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> index 63928f173223..2eaefa76f759 100644
> --- a/include/linux/bitfield.h
> +++ b/include/linux/bitfield.h
> @@ -8,6 +8,7 @@
>  #define _LINUX_BITFIELD_H
>  
>  #include <linux/build_bug.h>
> +#include <linux/typecheck.h>
>  #include <asm/byteorder.h>
>  
>  /*
> @@ -38,8 +39,7 @@
>   *	  FIELD_PREP(REG_FIELD_D, 0x40);
>   *
>   * Modify:
> - *  reg &= ~REG_FIELD_C;
> - *  reg |= FIELD_PREP(REG_FIELD_C, c);
> + *  FIELD_MODIFY(REG_FIELD_C, &reg, c);
>   */
>  
>  #define __bf_shf(x) (__builtin_ffsll(x) - 1)
> @@ -156,6 +156,23 @@
>  		(typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask));	\
>  	})
>  
> +/**
> + * FIELD_MODIFY() - modify a bitfield element
> + * @_mask: shifted mask defining the field's length and position
> + * @_reg_p: pointer to the memory that should be updated
> + * @_val: value to store in the bitfield
> + *
> + * FIELD_MODIFY() modifies the set of bits in @_reg_p specified by @_mask,
> + * by replacing them with the bitfield value passed in as @_val.
> + */
> +#define FIELD_MODIFY(_mask, _reg_p, _val)				\
> +	({								\
> +		typecheck_pointer(_reg_p);				\
> +		__BF_FIELD_CHECK(_mask, *(_reg_p), _val, "FIELD_MODIFY: ");		\
> +		*(_reg_p) &= ~(_mask);							\
> +		*(_reg_p) |= (((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask));	\
> +	})
> +
>  extern void __compiletime_error("value doesn't fit into mask")
>  __field_overflow(void);
>  extern void __compiletime_error("bad bitfield mask")
> 
> -- 
> 2.34.1

Re: [cocci] [PATCH v3 2/6] coccinelle: misc: Add field_modify script

[Markus Elfring]

> This script finds and suggests conversions of opencoded field
> modify patterns with the wrapper FIELD_MODIFY() API defined in
> include/linux/bitfield.h for better readability.

See also:
https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.15-rc3#n94


…
> +++ b/scripts/coccinelle/misc/field_modify.cocci
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +///

I suggest to omit a blank comment line here.


> +/// Replaced below code with the wrapper FIELD_MODIFY(MASK, &reg, val)

Replace?


…
> +// Copyright (C) 2025 Qualcomm Innovation Center, Inc.

Copyright: ?


> +// URL: https://coccinelle.gitlabpages.inria.fr/website

I suggest to omit such information here.


…
> +virtual patch

How do you think about to support additional operation modes?


…
> +- reg &= ~mask;
> +- reg |= FIELD_PREP(mask, val);
> ++ FIELD_MODIFY(mask, &reg, val);

Would you like to integrate the following SmPL code variant?

-reg &= ~mask;
-reg |= FIELD_PREP
+       FIELD_MODIFY
                  (mask,
+                  &reg,
                   val
                  );


Regards,
Markus

Re: [cocci] [PATCH v3 2/6] coccinelle: misc: Add field_modify script

[Jie Luo]

On 4/23/2025 7:01 PM, Markus Elfring wrote:
>> This script finds and suggests conversions of opencoded field
>> modify patterns with the wrapper FIELD_MODIFY() API defined in
>> include/linux/bitfield.h for better readability.
> 
> See also:
> https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.15-rc3#n94

OK, I will update the commit message with the imperative mood.

> 
> 
> …
>> +++ b/scripts/coccinelle/misc/field_modify.cocci
>> @@ -0,0 +1,24 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +///
> 
> I suggest to omit a blank comment line here.

OK.

> 
> 
>> +/// Replaced below code with the wrapper FIELD_MODIFY(MASK, &reg, val)
> 
> Replace?

I will correct it.

> 
> 
> …
>> +// Copyright (C) 2025 Qualcomm Innovation Center, Inc.
> 
> Copyright: ?

I will fix it.

> 
> 
>> +// URL: https://coccinelle.gitlabpages.inria.fr/website
> 
> I suggest to omit such information here.

OK.

> 
> 
> …
>> +virtual patch
> 
> How do you think about to support additional operation modes?

Sure, I will update the patch to support other operation modes.

> 
> 
> …
>> +- reg &= ~mask;
>> +- reg |= FIELD_PREP(mask, val);
>> ++ FIELD_MODIFY(mask, &reg, val);
> 
> Would you like to integrate the following SmPL code variant?
> 
> -reg &= ~mask;
> -reg |= FIELD_PREP
> +       FIELD_MODIFY
>                    (mask,
> +                  &reg,
>                     val
>                    );
> 
> 
> Regards,
> Markus
> 

With this code variant, there is no space prior to &reg, here is the
example code changes generated by the SmPL code as below, is this
expected?

Thanks for the review comments.

--- a/drivers/phy/starfive/phy-jh7110-dphy-tx.c
+++ b/drivers/phy/starfive/phy-jh7110-dphy-tx.c
@@ -244,8 +244,7 @@ static int stf_dphy_configure(struct phy
         i = stf_dphy_get_config_index(bitrate);

         tmp = readl(dphy->topsys + STF_DPHY_APBIFSAIF_SYSCFG(100));
-       tmp &= ~STF_DPHY_REFCLK_IN_SEL;
-       tmp |= FIELD_PREP(STF_DPHY_REFCLK_IN_SEL, STF_DPHY_REFCLK_12M);
+       FIELD_MODIFY(STF_DPHY_REFCLK_IN_SEL,&tmp, STF_DPHY_REFCLK_12M);
         writel(tmp, dphy->topsys + STF_DPHY_APBIFSAIF_SYSCFG(100));

Re: [PATCH v3 1/6] bitfield: Add FIELD_MODIFY() helper

[Jie Luo]

On 4/19/2025 1:11 AM, Yury Norov wrote:
>> Add a helper for replacing the contents of bitfield in memory
>> with the specified value.
>>
>> Even though a helper xxx_replace_bits() is available, it is not
>> well documented, and only reports errors at the run time, which
>> will not be helpful to catch possible overflow errors due to
>> incorrect parameter types used.
>>
>> Add the helper FIELD_MODIFY() to the FIELD_XXX family of bitfield
>> macros. It is functionally similar as xxx_replace_bits(), and in
>> addition adds the compile time type checking.
> This paragraph duplicates the above. I'll drop it and take this
> patch to bitmap-for-next. Regarding the rest of the series - it's up
> to ARM64 and Cocci maintainers if they want them or not.
> 
> Thanks for the work!
> 
> Thanks,
> Yury

OK, thanks.

Re: [PATCH v3 3/6] arm64: tlb: Convert the opencoded field modify

[Jie Luo]

On 4/18/2025 2:11 AM, Russell King (Oracle) wrote:
> On Thu, Apr 17, 2025 at 06:47:10PM +0800, Luo Jie wrote:
>> Replaced below code with the wrapper FIELD_MODIFY(MASK, &reg, val)
>> - reg &= ~MASK;
>> - reg |= FIELD_PREP(MASK, val);
>> The semantic patch that makes this change is available
>> in scripts/coccinelle/misc/field_modify.cocci.
>>
>> More information about semantic patching is available at
>> https://coccinelle.gitlabpages.inria.fr/website
>>
>> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
>> ---
>>   arch/arm64/include/asm/tlbflush.h | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/tlbflush.h b/arch/arm64/include/asm/tlbflush.h
>> index eba1a98657f1..0d250ef4161c 100644
>> --- a/arch/arm64/include/asm/tlbflush.h
>> +++ b/arch/arm64/include/asm/tlbflush.h
>> @@ -112,8 +112,7 @@ static inline unsigned long get_trans_granule(void)
>>   	    level >= 0 && level <= 3) {					\
>>   		u64 ttl = level & 3;					\
>>   		ttl |= get_trans_granule() << 2;			\
>> -		arg &= ~TLBI_TTL_MASK;					\
>> -		arg |= FIELD_PREP(TLBI_TTL_MASK, ttl);			\
>> +		FIELD_MODIFY(TLBI_TTL_MASK, &arg, ttl);			\
> 
> This could equally be:
> 		arg = u64_replace_bits(arg, ttl, TLBI_TTL_MASK);
> 
> which already exists, so doesn't require a new macro to be introduced.
> 
Looks like the new macro FIELD_MODIFY() is accepted by Yury, maybe we
can keep to use FIELD_MODIFY() with this change for better readability?

Re: [PATCH v3 0/6] Add FIELD_MODIFY() helper

[Jie Luo]

On 4/19/2025 1:04 AM, Yury Norov wrote:
>>>> I think a bunch of people have found them, tooling notwithstanding.
>>>>
>>>> As for the documentation, the commit message in 00b0c9b82663ac would
>>>> be advantageously promoted to full-fledged kernel-doc.
>>> The FIELD_MODIFY() and uxx_replace_bits() are simply different things.
>>>
>>> FIELD_MODIFY() employs __BF_FIELD_CHECK(), which allows strict
>>> parameters checking at compile time. And people like it. See
>>> recent fixed-size GENMASK() series:
>>>
>>> https://patchwork.kernel.org/comment/26283604/
>> I don't care much for what people like or not. I don't see this
>> FIELD_MODIFY() as a particular improvement on *_replace_bits().
> Sad to hear that. Those people are all kernel engineers, and they
> deserve some respect.
> 
> We are talking about tooling here. People use tools only if they like
> them. Luo likes FIELD_MODIFY() over (yes, undocumented and ungreppable)
> xx_replace_bits() for the reasons he described very clearly. He's going
> to use it in his driver shortly, and this arm64 detour has been made
> after my request.
> 
>  From my perspective, both functions have their right to live in kernel.
> They are similar but not identical.
> 
> I'll take patch #1 in my branch. Regarding ARM64 part - it's up to you
> either to switch to FIELD_MODIFY(), _replace_bits(), or do nothing.
> 
> Thanks,
> Yury

Thank you for reviewing and discussing this patch series. The newly
added macro FIELD_MODIFY() will be utilized by the Qualcomm PPE (Packet
Processing Engine) Ethernet driver. Regarding the ARM64 core files,
could you please provide guidance on the preferred approach?

Re: [cocci] [PATCH v3 2/6] coccinelle: misc: Add field_modify script

[Markus Elfring]

…
>> -reg &= ~mask;
>> -reg |= FIELD_PREP
>> +       FIELD_MODIFY
>>                    (mask,
>> +                  &reg,
>>                     val
>>                    );
…
> With this code variant, there is no space prior to &reg, here is the
> example code changes generated by the SmPL code as below, is this expected?
…
> +++ b/drivers/phy/starfive/phy-jh7110-dphy-tx.c
> @@ -244,8 +244,7 @@ static int stf_dphy_configure(struct phy
>         i = stf_dphy_get_config_index(bitrate);
> 
>         tmp = readl(dphy->topsys + STF_DPHY_APBIFSAIF_SYSCFG(100));
> -       tmp &= ~STF_DPHY_REFCLK_IN_SEL;
> -       tmp |= FIELD_PREP(STF_DPHY_REFCLK_IN_SEL, STF_DPHY_REFCLK_12M);
> +       FIELD_MODIFY(STF_DPHY_REFCLK_IN_SEL,&tmp, STF_DPHY_REFCLK_12M);
>         writel(tmp, dphy->topsys + STF_DPHY_APBIFSAIF_SYSCFG(100));

The Coccinelle software is still evolving somehow.
Thus your test result can trigger further development considerations.
I hope that clarifications and corresponding improvements can be achieved
also according to such source code layout concerns.

Regards,
Markus

RE: [cocci] [PATCH v3 3/6] arm64: tlb: Convert the opencoded field modify

[Keller, Jacob E]

> -----Original Message-----
> From: cocci-request@inria.fr <cocci-request@inria.fr> On Behalf Of Russell King
> (Oracle)
> Sent: Thursday, April 17, 2025 11:11 AM
> To: Luo Jie <quic_luoj@quicinc.com>
> Cc: Yury Norov <yury.norov@gmail.com>; Rasmus Villemoes
> <linux@rasmusvillemoes.dk>; Julia Lawall <Julia.Lawall@inria.fr>; Nicolas Palix
> <nicolas.palix@imag.fr>; Catalin Marinas <catalin.marinas@arm.com>; Will
> Deacon <will@kernel.org>; Marc Zyngier <maz@kernel.org>; Oliver Upton
> <oliver.upton@linux.dev>; Joey Gouly <joey.gouly@arm.com>; Suzuki K Poulose
> <suzuki.poulose@arm.com>; Zenghui Yu <yuzenghui@huawei.com>; linux-
> kernel@vger.kernel.org; cocci@inria.fr; linux-arm-kernel@lists.infradead.org;
> kvmarm@lists.linux.dev; andrew@lunn.ch; quic_kkumarcs@quicinc.com;
> quic_linchen@quicinc.com; quic_leiwei@quicinc.com;
> quic_suruchia@quicinc.com; quic_pavir@quicinc.com
> Subject: Re: [cocci] [PATCH v3 3/6] arm64: tlb: Convert the opencoded field
> modify
> 
> On Thu, Apr 17, 2025 at 06:47:10PM +0800, Luo Jie wrote:
> > Replaced below code with the wrapper FIELD_MODIFY(MASK, &reg, val)
> > - reg &= ~MASK;
> > - reg |= FIELD_PREP(MASK, val);
> > The semantic patch that makes this change is available
> > in scripts/coccinelle/misc/field_modify.cocci.
> >
> > More information about semantic patching is available at
> > https://coccinelle.gitlabpages.inria.fr/website
> >
> > Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> > ---
> >  arch/arm64/include/asm/tlbflush.h | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/tlbflush.h
> b/arch/arm64/include/asm/tlbflush.h
> > index eba1a98657f1..0d250ef4161c 100644
> > --- a/arch/arm64/include/asm/tlbflush.h
> > +++ b/arch/arm64/include/asm/tlbflush.h
> > @@ -112,8 +112,7 @@ static inline unsigned long get_trans_granule(void)
> >  	    level >= 0 && level <= 3) {					\
> >  		u64 ttl = level & 3;					\
> >  		ttl |= get_trans_granule() << 2;			\
> > -		arg &= ~TLBI_TTL_MASK;					\
> > -		arg |= FIELD_PREP(TLBI_TTL_MASK, ttl);			\
> > +		FIELD_MODIFY(TLBI_TTL_MASK, &arg, ttl);			\
> 
> This could equally be:
> 		arg = u64_replace_bits(arg, ttl, TLBI_TTL_MASK);
> 
> which already exists, so doesn't require a new macro to be introduced.
> 

I had been looking for something like this the other day but was grepping for "FIELD_REPLACE" and similar which didn't find things.. Neat to know it exists already :D

> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v3 0/6] Add FIELD_MODIFY() helper

[Russell King (Oracle)]

On Fri, Apr 18, 2025 at 11:08:38AM -0400, Yury Norov wrote:
> The _replace_bits() functions return fixed-width values, and intended
> for: "manipulating bitfields both in host- and fixed-endian", as the
> very first line in the commit message says.
> 
> Those using _replace_bits() for something else abuse the API, and
> should switch to FIELD_MODIFY().

Sorry, but please explain this statement, because it means nothing to
me.

FIELD_MODIFY() replaces bits in host endian. _replace_bits() also
replaces bits, but has a wider range of which encompass FIELD_MODIFY().

I see nothing that precludes using using _replace_bits() with
bitfields.

I see nothing that would differentiate the behaviour, other than maybe
religous ideals about C functions vs macros or upper vs lower case.

Please explain why you think there's a difference between the two
because I really can't see any reason not to use one over the other
apart from asthetics.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v3 4/6] arm64: nvhe: Convert the opencoded field modify

[Russell King (Oracle)]

On Fri, Apr 18, 2025 at 11:14:48AM -0400, Yury Norov wrote:
> On Thu, Apr 17, 2025 at 12:23:10PM +0100, Marc Zyngier wrote:
> > On Thu, 17 Apr 2025 11:47:11 +0100,
> > Luo Jie <quic_luoj@quicinc.com> wrote:
> > > 
> > > Replaced below code with the wrapper FIELD_MODIFY(MASK, &reg, val)
> > > - reg &= ~MASK;
> > > - reg |= FIELD_PREP(MASK, val);
> > > The semantic patch that makes this change is available
> > > in scripts/coccinelle/misc/field_modify.cocci.
> > > 
> > > More information about semantic patching is available at
> > > https://coccinelle.gitlabpages.inria.fr/website
> > > 
> > > Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> > > ---
> > >  arch/arm64/kvm/hyp/include/nvhe/memory.h | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kvm/hyp/include/nvhe/memory.h b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > > index 34233d586060..b2af748964d0 100644
> > > --- a/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > > +++ b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > > @@ -30,8 +30,7 @@ enum pkvm_page_state {
> > >  static inline enum kvm_pgtable_prot pkvm_mkstate(enum kvm_pgtable_prot prot,
> > >  						 enum pkvm_page_state state)
> > >  {
> > > -	prot &= ~PKVM_PAGE_STATE_PROT_MASK;
> > > -	prot |= FIELD_PREP(PKVM_PAGE_STATE_PROT_MASK, state);
> > > +	FIELD_MODIFY(PKVM_PAGE_STATE_PROT_MASK, &prot, state);
> > >  	return prot;
> > >  }
> > 
> > Following up on my suggestion to *not* add anything new, this patch
> > could be written as:
> > 
> > diff --git a/arch/arm64/kvm/hyp/include/nvhe/memory.h b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > index 34233d5860607..08cb6ba0e0716 100644
> > --- a/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > +++ b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > @@ -30,9 +30,8 @@ enum pkvm_page_state {
> >  static inline enum kvm_pgtable_prot pkvm_mkstate(enum kvm_pgtable_prot prot,
> >  						 enum pkvm_page_state state)
> >  {
> > -	prot &= ~PKVM_PAGE_STATE_PROT_MASK;
> > -	prot |= FIELD_PREP(PKVM_PAGE_STATE_PROT_MASK, state);
> > -	return prot;
> > +	u64 p = prot;
> > +	return u64_replace_bits(p, state, PKVM_PAGE_STATE_PROT_MASK);
> >  }
> 
> This is a great example where u64_replace_bit() should NOT be used. 

Why not? Explain it. Don't leave people in the dark, because right
now it looks like it's purely a religous fanaticism about what
should and should not be used. Where's the technical reasoning?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v3 4/6] arm64: nvhe: Convert the opencoded field modify

[Yury Norov]

On Wed, Apr 23, 2025 at 06:48:34PM +0100, Russell King (Oracle) wrote:
> On Fri, Apr 18, 2025 at 11:14:48AM -0400, Yury Norov wrote:
> > On Thu, Apr 17, 2025 at 12:23:10PM +0100, Marc Zyngier wrote:
> > > On Thu, 17 Apr 2025 11:47:11 +0100,
> > > Luo Jie <quic_luoj@quicinc.com> wrote:
> > > > 
> > > > Replaced below code with the wrapper FIELD_MODIFY(MASK, &reg, val)
> > > > - reg &= ~MASK;
> > > > - reg |= FIELD_PREP(MASK, val);
> > > > The semantic patch that makes this change is available
> > > > in scripts/coccinelle/misc/field_modify.cocci.
> > > > 
> > > > More information about semantic patching is available at
> > > > https://coccinelle.gitlabpages.inria.fr/website
> > > > 
> > > > Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> > > > ---
> > > >  arch/arm64/kvm/hyp/include/nvhe/memory.h | 3 +--
> > > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > > 
> > > > diff --git a/arch/arm64/kvm/hyp/include/nvhe/memory.h b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > > > index 34233d586060..b2af748964d0 100644
> > > > --- a/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > > > +++ b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > > > @@ -30,8 +30,7 @@ enum pkvm_page_state {
> > > >  static inline enum kvm_pgtable_prot pkvm_mkstate(enum kvm_pgtable_prot prot,
> > > >  						 enum pkvm_page_state state)
> > > >  {
> > > > -	prot &= ~PKVM_PAGE_STATE_PROT_MASK;
> > > > -	prot |= FIELD_PREP(PKVM_PAGE_STATE_PROT_MASK, state);
> > > > +	FIELD_MODIFY(PKVM_PAGE_STATE_PROT_MASK, &prot, state);
> > > >  	return prot;
> > > >  }
> > > 
> > > Following up on my suggestion to *not* add anything new, this patch
> > > could be written as:
> > > 
> > > diff --git a/arch/arm64/kvm/hyp/include/nvhe/memory.h b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > > index 34233d5860607..08cb6ba0e0716 100644
> > > --- a/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > > +++ b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > > @@ -30,9 +30,8 @@ enum pkvm_page_state {
> > >  static inline enum kvm_pgtable_prot pkvm_mkstate(enum kvm_pgtable_prot prot,
> > >  						 enum pkvm_page_state state)
> > >  {
> > > -	prot &= ~PKVM_PAGE_STATE_PROT_MASK;
> > > -	prot |= FIELD_PREP(PKVM_PAGE_STATE_PROT_MASK, state);
> > > -	return prot;
> > > +	u64 p = prot;
> > > +	return u64_replace_bits(p, state, PKVM_PAGE_STATE_PROT_MASK);
> > >  }
> > 
> > This is a great example where u64_replace_bit() should NOT be used. 
> 
> Why not? Explain it. Don't leave people in the dark, because right
> now it looks like it's purely a religous fanaticism about what
> should and should not be used. Where's the technical reasoning?

Because enum is an integer, i.e. 32-bit type. Now, the snippet above
typecasts it to 64-bit fixed size type, passes to 64-bit fixed-type
function, and the returned value is typecasted back to 32-bit int.

Doesn't sound the most efficient solution, right? On 32-bit arch it
may double the function size, I guess.

But the most important is that if we adopt this practice and spread it
around, it will be really easy to overflow the 32-bit storage. The
compiler will keep silence about that.

Fixed types are very useful in their specific areas - cross-ABI data
transfer, etc. But mixing them with native types like int may hurt
badly. 

Hope that helps.

Thanks,
Yury

Re: [PATCH v3 0/6] Add FIELD_MODIFY() helper

[Yury Norov]

On Wed, Apr 23, 2025 at 06:44:15PM +0100, Russell King (Oracle) wrote:
> On Fri, Apr 18, 2025 at 11:08:38AM -0400, Yury Norov wrote:
> > The _replace_bits() functions return fixed-width values, and intended
> > for: "manipulating bitfields both in host- and fixed-endian", as the
> > very first line in the commit message says.
> > 
> > Those using _replace_bits() for something else abuse the API, and
> > should switch to FIELD_MODIFY().
> 
> Sorry, but please explain this statement, because it means nothing to
> me.
> 
> FIELD_MODIFY() replaces bits in host endian. _replace_bits() also
> replaces bits, but has a wider range of which encompass FIELD_MODIFY().
> 
> I see nothing that precludes using using _replace_bits() with
> bitfields.
> 
> I see nothing that would differentiate the behaviour, other than maybe
> religous ideals about C functions vs macros or upper vs lower case.

Interesting, never heard about religious ideals in C.
 
> Please explain why you think there's a difference between the two
> because I really can't see any reason not to use one over the other
> apart from asthetics.

I explained that in subtread for 4/6 in this series. Shortly it's
about compiler's ability to catch various errors, like overflows,
and (not unlikely) generated code quality.

Re: [PATCH v3 4/6] arm64: nvhe: Convert the opencoded field modify

[Russell King (Oracle)]

On Wed, Apr 23, 2025 at 02:27:06PM -0400, Yury Norov wrote:
> On Wed, Apr 23, 2025 at 06:48:34PM +0100, Russell King (Oracle) wrote:
> > On Fri, Apr 18, 2025 at 11:14:48AM -0400, Yury Norov wrote:
> > > On Thu, Apr 17, 2025 at 12:23:10PM +0100, Marc Zyngier wrote:
> > > > On Thu, 17 Apr 2025 11:47:11 +0100,
> > > > Luo Jie <quic_luoj@quicinc.com> wrote:
> > > > > 
> > > > > Replaced below code with the wrapper FIELD_MODIFY(MASK, &reg, val)
> > > > > - reg &= ~MASK;
> > > > > - reg |= FIELD_PREP(MASK, val);
> > > > > The semantic patch that makes this change is available
> > > > > in scripts/coccinelle/misc/field_modify.cocci.
> > > > > 
> > > > > More information about semantic patching is available at
> > > > > https://coccinelle.gitlabpages.inria.fr/website
> > > > > 
> > > > > Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> > > > > ---
> > > > >  arch/arm64/kvm/hyp/include/nvhe/memory.h | 3 +--
> > > > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/arch/arm64/kvm/hyp/include/nvhe/memory.h b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > > > > index 34233d586060..b2af748964d0 100644
> > > > > --- a/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > > > > +++ b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > > > > @@ -30,8 +30,7 @@ enum pkvm_page_state {
> > > > >  static inline enum kvm_pgtable_prot pkvm_mkstate(enum kvm_pgtable_prot prot,
> > > > >  						 enum pkvm_page_state state)
> > > > >  {
> > > > > -	prot &= ~PKVM_PAGE_STATE_PROT_MASK;
> > > > > -	prot |= FIELD_PREP(PKVM_PAGE_STATE_PROT_MASK, state);
> > > > > +	FIELD_MODIFY(PKVM_PAGE_STATE_PROT_MASK, &prot, state);
> > > > >  	return prot;
> > > > >  }
> > > > 
> > > > Following up on my suggestion to *not* add anything new, this patch
> > > > could be written as:
> > > > 
> > > > diff --git a/arch/arm64/kvm/hyp/include/nvhe/memory.h b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > > > index 34233d5860607..08cb6ba0e0716 100644
> > > > --- a/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > > > +++ b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > > > @@ -30,9 +30,8 @@ enum pkvm_page_state {
> > > >  static inline enum kvm_pgtable_prot pkvm_mkstate(enum kvm_pgtable_prot prot,
> > > >  						 enum pkvm_page_state state)
> > > >  {
> > > > -	prot &= ~PKVM_PAGE_STATE_PROT_MASK;
> > > > -	prot |= FIELD_PREP(PKVM_PAGE_STATE_PROT_MASK, state);
> > > > -	return prot;
> > > > +	u64 p = prot;
> > > > +	return u64_replace_bits(p, state, PKVM_PAGE_STATE_PROT_MASK);
> > > >  }
> > > 
> > > This is a great example where u64_replace_bit() should NOT be used. 
> > 
> > Why not? Explain it. Don't leave people in the dark, because right
> > now it looks like it's purely a religous fanaticism about what
> > should and should not be used. Where's the technical reasoning?
> 
> Because enum is an integer, i.e. 32-bit type.

This statement is false, in this case.

The kernel currently uses -std=gnu11, and GNU tends to be more relaxed
about things, and while the C standard may say that enums are ints,
that isn't the case - gcc appears to follow C++ and allow enums that
are wider than ints.

$ aarch64-linux-gnu-gcc -S -o - -std=gnu99 -x c -
enum foo {
A = 1L << 0,
B = 1L << 53,
};
int main()
{ return sizeof(enum foo); }

Gives the following code:

main:
.LFB0:
        .cfi_startproc
        mov     w0, 8
        ret
        .cfi_endproc

meaning that sizeof(enum foo) is 8 or 64-bit.

If B were 1L << 31, then sizeof(enum foo) is 4.

> Now, the snippet above
> typecasts it to 64-bit fixed size type, passes to 64-bit fixed-type
> function, and the returned value is typecasted back to 32-bit int.

In this case, the enum is defined using:

        KVM_PGTABLE_PROT_X                      = BIT(0),
        KVM_PGTABLE_PROT_W                      = BIT(1),
        KVM_PGTABLE_PROT_R                      = BIT(2),

        KVM_PGTABLE_PROT_DEVICE                 = BIT(3),
        KVM_PGTABLE_PROT_NORMAL_NC              = BIT(4),

        KVM_PGTABLE_PROT_SW0                    = BIT(55),
        KVM_PGTABLE_PROT_SW1                    = BIT(56),
        KVM_PGTABLE_PROT_SW2                    = BIT(57),
        KVM_PGTABLE_PROT_SW3                    = BIT(58),

As it contains bits beyond bit 31, and we use -std=gnu11 when building
the kernel, this enum is represented using a 64-bit integer type. So,
the casting to a u64 is not increasing the size of the enum, and the
return value is not getting truncated down to 32-bits.

> Doesn't sound the most efficient solution, right? On 32-bit arch it
> may double the function size, I guess.

Given that there's no inefficiency here, and that this is arm64 code
which is a 64-bit arch, both those points you mention seem to be
incorrect or not relevant.

> But the most important is that if we adopt this practice and spread it
> around, it will be really easy to overflow the 32-bit storage. The
> compiler will keep silence about that.

Given that in Marc's suggestion, "prot" is a 64-bit value, it's being
assigned to a u64, which is then being operated on by the u64 variant
of _replace_bits(), which returns the u64 result, which then gets
returned as a 64-bit enum, there is no issue here as far as I can see.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v3 4/6] arm64: nvhe: Convert the opencoded field modify

[Yury Norov]

On Wed, Apr 23, 2025 at 08:11:18PM +0100, Russell King (Oracle) wrote:
> On Wed, Apr 23, 2025 at 02:27:06PM -0400, Yury Norov wrote:
> > On Wed, Apr 23, 2025 at 06:48:34PM +0100, Russell King (Oracle) wrote:
> > > On Fri, Apr 18, 2025 at 11:14:48AM -0400, Yury Norov wrote:
> > > > On Thu, Apr 17, 2025 at 12:23:10PM +0100, Marc Zyngier wrote:
> > > > > On Thu, 17 Apr 2025 11:47:11 +0100,
> > > > > Luo Jie <quic_luoj@quicinc.com> wrote:
> > > > > > 
> > > > > > Replaced below code with the wrapper FIELD_MODIFY(MASK, &reg, val)
> > > > > > - reg &= ~MASK;
> > > > > > - reg |= FIELD_PREP(MASK, val);
> > > > > > The semantic patch that makes this change is available
> > > > > > in scripts/coccinelle/misc/field_modify.cocci.
> > > > > > 
> > > > > > More information about semantic patching is available at
> > > > > > https://coccinelle.gitlabpages.inria.fr/website
> > > > > > 
> > > > > > Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> > > > > > ---
> > > > > >  arch/arm64/kvm/hyp/include/nvhe/memory.h | 3 +--
> > > > > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/arch/arm64/kvm/hyp/include/nvhe/memory.h b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > > > > > index 34233d586060..b2af748964d0 100644
> > > > > > --- a/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > > > > > +++ b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > > > > > @@ -30,8 +30,7 @@ enum pkvm_page_state {
> > > > > >  static inline enum kvm_pgtable_prot pkvm_mkstate(enum kvm_pgtable_prot prot,
> > > > > >  						 enum pkvm_page_state state)
> > > > > >  {
> > > > > > -	prot &= ~PKVM_PAGE_STATE_PROT_MASK;
> > > > > > -	prot |= FIELD_PREP(PKVM_PAGE_STATE_PROT_MASK, state);
> > > > > > +	FIELD_MODIFY(PKVM_PAGE_STATE_PROT_MASK, &prot, state);
> > > > > >  	return prot;
> > > > > >  }
> > > > > 
> > > > > Following up on my suggestion to *not* add anything new, this patch
> > > > > could be written as:
> > > > > 
> > > > > diff --git a/arch/arm64/kvm/hyp/include/nvhe/memory.h b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > > > > index 34233d5860607..08cb6ba0e0716 100644
> > > > > --- a/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > > > > +++ b/arch/arm64/kvm/hyp/include/nvhe/memory.h
> > > > > @@ -30,9 +30,8 @@ enum pkvm_page_state {
> > > > >  static inline enum kvm_pgtable_prot pkvm_mkstate(enum kvm_pgtable_prot prot,
> > > > >  						 enum pkvm_page_state state)
> > > > >  {
> > > > > -	prot &= ~PKVM_PAGE_STATE_PROT_MASK;
> > > > > -	prot |= FIELD_PREP(PKVM_PAGE_STATE_PROT_MASK, state);
> > > > > -	return prot;
> > > > > +	u64 p = prot;
> > > > > +	return u64_replace_bits(p, state, PKVM_PAGE_STATE_PROT_MASK);
> > > > >  }
> > > > 
> > > > This is a great example where u64_replace_bit() should NOT be used. 
> > > 
> > > Why not? Explain it. Don't leave people in the dark, because right
> > > now it looks like it's purely a religous fanaticism about what
> > > should and should not be used. Where's the technical reasoning?
> > 
> > Because enum is an integer, i.e. 32-bit type.
> 
> This statement is false, in this case.
> 
> The kernel currently uses -std=gnu11, and GNU tends to be more relaxed
> about things, and while the C standard may say that enums are ints,
> that isn't the case - gcc appears to follow C++ and allow enums that
> are wider than ints.
> 
> $ aarch64-linux-gnu-gcc -S -o - -std=gnu99 -x c -
> enum foo {
> A = 1L << 0,
> B = 1L << 53,
> };
> int main()
> { return sizeof(enum foo); }
> 
> Gives the following code:
> 
> main:
> .LFB0:
>         .cfi_startproc
>         mov     w0, 8
>         ret
>         .cfi_endproc
> 
> meaning that sizeof(enum foo) is 8 or 64-bit.
> 
> If B were 1L << 31, then sizeof(enum foo) is 4.
> 
> > Now, the snippet above
> > typecasts it to 64-bit fixed size type, passes to 64-bit fixed-type
> > function, and the returned value is typecasted back to 32-bit int.
> 
> In this case, the enum is defined using:
> 
>         KVM_PGTABLE_PROT_X                      = BIT(0),
>         KVM_PGTABLE_PROT_W                      = BIT(1),
>         KVM_PGTABLE_PROT_R                      = BIT(2),
> 
>         KVM_PGTABLE_PROT_DEVICE                 = BIT(3),
>         KVM_PGTABLE_PROT_NORMAL_NC              = BIT(4),
> 
>         KVM_PGTABLE_PROT_SW0                    = BIT(55),
>         KVM_PGTABLE_PROT_SW1                    = BIT(56),
>         KVM_PGTABLE_PROT_SW2                    = BIT(57),
>         KVM_PGTABLE_PROT_SW3                    = BIT(58),
> 
> As it contains bits beyond bit 31, and we use -std=gnu11 when building
> the kernel, this enum is represented using a 64-bit integer type. So,
> the casting to a u64 is not increasing the size of the enum, and the
> return value is not getting truncated down to 32-bits.
> 
> > Doesn't sound the most efficient solution, right? On 32-bit arch it
> > may double the function size, I guess.
> 
> Given that there's no inefficiency here, and that this is arm64 code
> which is a 64-bit arch, both those points you mention seem to be
> incorrect or not relevant.
> 
> > But the most important is that if we adopt this practice and spread it
> > around, it will be really easy to overflow the 32-bit storage. The
> > compiler will keep silence about that.
> 
> Given that in Marc's suggestion, "prot" is a 64-bit value, it's being
> assigned to a u64, which is then being operated on by the u64 variant
> of _replace_bits(), which returns the u64 result, which then gets
> returned as a 64-bit enum, there is no issue here as far as I can see.

Ah, OK. You're right. On the other hand, enum is a bad specifier here,
because this thing is not an enumeration. It's clearly a bit structure
that reflects attributes in the page table record.

This enum confused me (and probably others), and could better be an
u64. And because this is really the 64-bit storage that tightly coupled
to MMU layout, it should be a fixed-type, and should be handled with
u64_xx_bits() functions.

If it was a true enumeration, something like dma_data_direction or
ucount_type, or if it was a true native type like long, using this
u64_xx_bits() is not optimal.

RE: [cocci] [PATCH v3 3/6] arm64: tlb: Convert the opencoded field modify

[Keller, Jacob E]

> -----Original Message-----
> From: cocci-request@inria.fr <cocci-request@inria.fr> On Behalf Of Jie Luo
> Sent: Wednesday, April 23, 2025 6:16 AM
> To: Russell King (Oracle) <linux@armlinux.org.uk>
> Cc: Yury Norov <yury.norov@gmail.com>; Rasmus Villemoes
> <linux@rasmusvillemoes.dk>; Julia Lawall <Julia.Lawall@inria.fr>; Nicolas Palix
> <nicolas.palix@imag.fr>; Catalin Marinas <catalin.marinas@arm.com>; Will
> Deacon <will@kernel.org>; Marc Zyngier <maz@kernel.org>; Oliver Upton
> <oliver.upton@linux.dev>; Joey Gouly <joey.gouly@arm.com>; Suzuki K Poulose
> <suzuki.poulose@arm.com>; Zenghui Yu <yuzenghui@huawei.com>; linux-
> kernel@vger.kernel.org; cocci@inria.fr; linux-arm-kernel@lists.infradead.org;
> kvmarm@lists.linux.dev; andrew@lunn.ch; quic_kkumarcs@quicinc.com;
> quic_linchen@quicinc.com; quic_leiwei@quicinc.com;
> quic_suruchia@quicinc.com; quic_pavir@quicinc.com
> Subject: Re: [cocci] [PATCH v3 3/6] arm64: tlb: Convert the opencoded field
> modify
> 
> 
> 
> On 4/18/2025 2:11 AM, Russell King (Oracle) wrote:
> > On Thu, Apr 17, 2025 at 06:47:10PM +0800, Luo Jie wrote:
> >> Replaced below code with the wrapper FIELD_MODIFY(MASK, &reg, val)
> >> - reg &= ~MASK;
> >> - reg |= FIELD_PREP(MASK, val);
> >> The semantic patch that makes this change is available
> >> in scripts/coccinelle/misc/field_modify.cocci.
> >>
> >> More information about semantic patching is available at
> >> https://coccinelle.gitlabpages.inria.fr/website
> >>
> >> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> >> ---
> >>   arch/arm64/include/asm/tlbflush.h | 3 +--
> >>   1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/arch/arm64/include/asm/tlbflush.h
> b/arch/arm64/include/asm/tlbflush.h
> >> index eba1a98657f1..0d250ef4161c 100644
> >> --- a/arch/arm64/include/asm/tlbflush.h
> >> +++ b/arch/arm64/include/asm/tlbflush.h
> >> @@ -112,8 +112,7 @@ static inline unsigned long get_trans_granule(void)
> >>   	    level >= 0 && level <= 3) {					\
> >>   		u64 ttl = level & 3;					\
> >>   		ttl |= get_trans_granule() << 2;			\
> >> -		arg &= ~TLBI_TTL_MASK;					\
> >> -		arg |= FIELD_PREP(TLBI_TTL_MASK, ttl);			\
> >> +		FIELD_MODIFY(TLBI_TTL_MASK, &arg, ttl);			\
> >
> > This could equally be:
> > 		arg = u64_replace_bits(arg, ttl, TLBI_TTL_MASK);
> >
> > which already exists, so doesn't require a new macro to be introduced.
> >
> Looks like the new macro FIELD_MODIFY() is accepted by Yury, maybe we
> can keep to use FIELD_MODIFY() with this change for better readability?

I'd prefer this version too rather than a pointer that modifies in place. Feels like it fits better with the other FIELD macros which don't take pointers and return their bits by value.

Re: [cocci] [PATCH v3 2/6] coccinelle: misc: Add field_modify script

[Luo Jie]

On 4/24/2025 12:35 AM, Markus Elfring wrote:
> …
>>> -reg &= ~mask;
>>> -reg |= FIELD_PREP
>>> +       FIELD_MODIFY
>>>                     (mask,
>>> +                  &reg,
>>>                      val
>>>                     );
> …
>> With this code variant, there is no space prior to &reg, here is the
>> example code changes generated by the SmPL code as below, is this expected?
> …
>> +++ b/drivers/phy/starfive/phy-jh7110-dphy-tx.c
>> @@ -244,8 +244,7 @@ static int stf_dphy_configure(struct phy
>>          i = stf_dphy_get_config_index(bitrate);
>>
>>          tmp = readl(dphy->topsys + STF_DPHY_APBIFSAIF_SYSCFG(100));
>> -       tmp &= ~STF_DPHY_REFCLK_IN_SEL;
>> -       tmp |= FIELD_PREP(STF_DPHY_REFCLK_IN_SEL, STF_DPHY_REFCLK_12M);
>> +       FIELD_MODIFY(STF_DPHY_REFCLK_IN_SEL,&tmp, STF_DPHY_REFCLK_12M);
>>          writel(tmp, dphy->topsys + STF_DPHY_APBIFSAIF_SYSCFG(100));
> 
> The Coccinelle software is still evolving somehow.
> Thus your test result can trigger further development considerations.
> I hope that clarifications and corresponding improvements can be achieved
> also according to such source code layout concerns.
> 
> Regards,
> Markus

OK, understand. I will keep the original patch as below for now, as we
need to ensure the script generates code with the expected style.
+- reg &= ~mask;
+- reg |= FIELD_PREP(mask, val);
++ FIELD_MODIFY(mask, &reg, val);

In addition, below case is filed to request this improvement on the
coccinelle project.
https://github.com/coccinelle/coccinelle/issues/397

Thanks.

Re: [cocci] [PATCH v3 2/6] coccinelle: misc: Add field_modify script

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 1861 bytes --]



On Mon, 19 May 2025, Luo Jie wrote:

>
>
> On 4/24/2025 12:35 AM, Markus Elfring wrote:
> > …
> > > > -reg &= ~mask;
> > > > -reg |= FIELD_PREP
> > > > +       FIELD_MODIFY
> > > >                     (mask,
> > > > +                  &reg,
> > > >                      val
> > > >                     );
> > …
> > > With this code variant, there is no space prior to &reg, here is the
> > > example code changes generated by the SmPL code as below, is this
> > > expected?
> > …
> > > +++ b/drivers/phy/starfive/phy-jh7110-dphy-tx.c
> > > @@ -244,8 +244,7 @@ static int stf_dphy_configure(struct phy
> > >          i = stf_dphy_get_config_index(bitrate);
> > >
> > >          tmp = readl(dphy->topsys + STF_DPHY_APBIFSAIF_SYSCFG(100));
> > > -       tmp &= ~STF_DPHY_REFCLK_IN_SEL;
> > > -       tmp |= FIELD_PREP(STF_DPHY_REFCLK_IN_SEL, STF_DPHY_REFCLK_12M);
> > > +       FIELD_MODIFY(STF_DPHY_REFCLK_IN_SEL,&tmp, STF_DPHY_REFCLK_12M);
> > >          writel(tmp, dphy->topsys + STF_DPHY_APBIFSAIF_SYSCFG(100));
> >
> > The Coccinelle software is still evolving somehow.
> > Thus your test result can trigger further development considerations.
> > I hope that clarifications and corresponding improvements can be achieved
> > also according to such source code layout concerns.
> >
> > Regards,
> > Markus
>
> OK, understand. I will keep the original patch as below for now, as we
> need to ensure the script generates code with the expected style.
> +- reg &= ~mask;
> +- reg |= FIELD_PREP(mask, val);
> ++ FIELD_MODIFY(mask, &reg, val);
>
> In addition, below case is filed to request this improvement on the
> coccinelle project.
> https://github.com/coccinelle/coccinelle/issues/397

Thanks for the report.

julia


>
> Thanks.
>