[PATCH v4 0/5] Add FIELD_MODIFY() helper

[PATCH v4 0/5] 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 v4:
- Add org, report and context mode for coccinelle script.
- Fix other comments on coccinelle script patch.
- Remove the FIELD_MODIFY patch as it is merged.
- Link to v3: https://lore.kernel.org/r/20250417-field_modify-v3-0-6f7992aafcb7@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 (5):
      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 +-
 scripts/coccinelle/misc/field_modify.cocci | 61 ++++++++++++++++++++++++++++++
 5 files changed, 66 insertions(+), 10 deletions(-)
---
base-commit: 0bb71d301869446810a0b13d3da290bd455d7c78
change-id: 20250612-field_modify-27139f673881

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

[PATCH v4 1/5] coccinelle: misc: Add field_modify script

[Luo Jie]

Find and suggest conversions of opencoded field modify patterns with
the wrapper FIELD_MODIFY() API defined in include/linux/bitfield.h
for catching the possible parameter type error in the compile time.

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

diff --git a/scripts/coccinelle/misc/field_modify.cocci b/scripts/coccinelle/misc/field_modify.cocci
new file mode 100644
index 000000000000..48b00194a265
--- /dev/null
+++ b/scripts/coccinelle/misc/field_modify.cocci
@@ -0,0 +1,61 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/// Replace 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.
+// Keywords: FIELD_PREP, FIELD_MODIFY
+// Options: --include-headers
+
+virtual context
+virtual patch
+virtual org
+virtual report
+
+@ depends on context && !patch && !org && !report@
+identifier reg, val;
+constant mask;
+symbol FIELD_PREP;
+@@
+
+* reg &= ~mask;
+* reg |= FIELD_PREP(mask, val);
+
+@ depends on !context && patch && !org && !report @
+identifier reg, val;
+constant mask;
+symbol FIELD_PREP, FIELD_MODIFY;
+@@
+
+-reg &= ~mask;
+-reg |= FIELD_PREP(mask, val);
++FIELD_MODIFY(mask, &reg, val);
+
+@r depends on !context && !patch && (org || report)@
+identifier reg, val;
+constant mask;
+symbol FIELD_PREP;
+position p;
+@@
+
+ reg &= ~mask;
+ reg |= FIELD_PREP@p(mask, val);
+
+@script:python depends on report@
+p << r.p;
+x << r.reg;
+@@
+
+msg="WARNING: Consider using FIELD_MODIFY helper on %s" % (x)
+coccilib.report.print_report(p[0], msg)
+
+@script:python depends on org@
+p << r.p;
+x << r.reg;
+@@
+
+msg="WARNING: Consider using FIELD_MODIFY helper on %s" % (x)
+msg_safe=msg.replace("[","@(").replace("]",")")
+coccilib.org.print_todo(p[0], msg_safe)

-- 
2.34.1

Re: [cocci] [PATCH v4 1/5] coccinelle: misc: Add field_modify script

[Markus Elfring]

…
> ---
>  scripts/coccinelle/misc/field_modify.cocci | 61 ++++++++++++++++++++++++++++++
…

Did you overlook the addition of patch version descriptions?
https://lore.kernel.org/all/?q=%22This+looks+like+a+new+version+of+a+previously+submitted+patch%22
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.15#n310


…
> +@ depends on context && !patch && !org && !report@
…

I imagine that the condition selections can be simplified.


…
> +@script:python depends on report@
> +p << r.p;
> +x << r.reg;
> +@@
> +
> +msg="WARNING: Consider using FIELD_MODIFY helper on %s" % (x)
> +coccilib.report.print_report(p[0], msg)
Do you know that a string construction can also be directly passed
to such a function call?

Regards,
Markus

Re: [cocci] [PATCH v4 1/5] coccinelle: misc: Add field_modify script

[Luo Jie]

On 6/13/2025 12:48 AM, Markus Elfring wrote:
> …
>> ---
>>   scripts/coccinelle/misc/field_modify.cocci | 61 ++++++++++++++++++++++++++++++
> …
> 
> Did you overlook the addition of patch version descriptions?
> https://lore.kernel.org/all/?q=%22This+looks+like+a+new+version+of+a+previously+submitted+patch%22
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.15#n310

Thank you for highlighting this, and for the references. A brief
description of the differences in this script patch compared to
the previous (v3) version were included in the cover letter, so
a changelog was not added to the individual patch's commit message.
Hope this approach is acceptable.

I will ensure to include the reviewer in the CC list for future
submissions, as recommended in the documentation.

> 
> 
> …
>> +@ depends on context && !patch && !org && !report@
> …
> 
> I imagine that the condition selections can be simplified.
> 

I agree that the condition selections can be simplified, I will
update it to "@ depends on context@". Hope it is fine.

> 
> …
>> +@script:python depends on report@
>> +p << r.p;
>> +x << r.reg;
>> +@@
>> +
>> +msg="WARNING: Consider using FIELD_MODIFY helper on %s" % (x)
>> +coccilib.report.print_report(p[0], msg)
> Do you know that a string construction can also be directly passed
> to such a function call?

I appreciate the tip. I'll update the patch to pass the formatted
string directly to the function call in the next revision.

> 
> Regards,
> Markus

[PATCH v4 2/5] arm64: tlb: Convert the opencoded field modify

[Luo Jie]

Replace 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
http://coccinelle.lip6.fr/

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 aa9efee17277..cdcee05bdf6d 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

Re: [cocci] [PATCH v4 2/5] arm64: tlb: Convert the opencoded field modify

[Markus Elfring]

I see further refinement possibilities for such a change description.


> Replace below code with the wrapper FIELD_MODIFY(MASK, &reg, val)
> - reg &= ~MASK;
> - reg |= FIELD_PREP(MASK, val);

* How do you think about to omit leading minus characters?

* Subsequent blank line?


> More information about semantic patching is available at
> http://coccinelle.lip6.fr/

I suggest to omit this information here (and in similar patches).

Regards,
Markus

Re: [cocci] [PATCH v4 2/5] arm64: tlb: Convert the opencoded field modify

[Luo Jie]

On 6/13/2025 4:15 AM, Markus Elfring wrote:
> I see further refinement possibilities for such a change description.
> 
> 
>> Replace below code with the wrapper FIELD_MODIFY(MASK, &reg, val)
>> - reg &= ~MASK;
>> - reg |= FIELD_PREP(MASK, val);
> 
> * How do you think about to omit leading minus characters?
> 
> * Subsequent blank line?
> 
> 
>> More information about semantic patching is available at
>> http://coccinelle.lip6.fr/
> 
> I suggest to omit this information here (and in similar patches).
> 
> Regards,
> Markus

Thank you for your suggestions. The current commit message was generated
by the following patch mode command:
```
make coccicheck MODE=patch 
COCCI=scripts/coccinelle/misc/field_modify.cocci V=1
```
However, as I understand, the discussion on the ARM patches (between
Russel/Marc/Yury) has concluded that the ARM arch changes may not be
adding value over the current code, so I will drop the ARM patches
in the next version.

I will review the generated message to improve the formatting as you
suggested, the next time I use it.

Re: [cocci] [PATCH v4 2/5] arm64: tlb: Convert the opencoded field modify

[Will Deacon]

On Mon, Jun 16, 2025 at 06:37:41PM +0800, Luo Jie wrote:
> 
> 
> On 6/13/2025 4:15 AM, Markus Elfring wrote:
> > I see further refinement possibilities for such a change description.
> > 
> > 
> > > Replace below code with the wrapper FIELD_MODIFY(MASK, &reg, val)
> > > - reg &= ~MASK;
> > > - reg |= FIELD_PREP(MASK, val);
> > 
> > * How do you think about to omit leading minus characters?
> > 
> > * Subsequent blank line?
> > 
> > 
> > > More information about semantic patching is available at
> > > http://coccinelle.lip6.fr/
> > 
> > I suggest to omit this information here (and in similar patches).
> > 
> > Regards,
> > Markus
> 
> Thank you for your suggestions. The current commit message was generated
> by the following patch mode command:
> ```
> make coccicheck MODE=patch COCCI=scripts/coccinelle/misc/field_modify.cocci
> V=1
> ```
> However, as I understand, the discussion on the ARM patches (between
> Russel/Marc/Yury) has concluded that the ARM arch changes may not be
> adding value over the current code, so I will drop the ARM patches
> in the next version.

Well, hang on a second. From what I can tell, the objections haven't
been specific to arch/arm{,64}/. You haven't really explained why this
new helper is needed and what value it brings over the existing set of
functionality.

So maybe start there, rather than dropping the parts that attracted the
comments to start with?

Will

Re: [cocci] [PATCH v4 2/5] arm64: tlb: Convert the opencoded field modify

[Luo Jie]

On 6/16/2025 6:41 PM, Will Deacon wrote:
> On Mon, Jun 16, 2025 at 06:37:41PM +0800, Luo Jie wrote:
>>
>>
>> On 6/13/2025 4:15 AM, Markus Elfring wrote:
>>> I see further refinement possibilities for such a change description.
>>>
>>>
>>>> Replace below code with the wrapper FIELD_MODIFY(MASK, &reg, val)
>>>> - reg &= ~MASK;
>>>> - reg |= FIELD_PREP(MASK, val);
>>>
>>> * How do you think about to omit leading minus characters?
>>>
>>> * Subsequent blank line?
>>>
>>>
>>>> More information about semantic patching is available at
>>>> http://coccinelle.lip6.fr/
>>>
>>> I suggest to omit this information here (and in similar patches).
>>>
>>> Regards,
>>> Markus
>>
>> Thank you for your suggestions. The current commit message was generated
>> by the following patch mode command:
>> ```
>> make coccicheck MODE=patch COCCI=scripts/coccinelle/misc/field_modify.cocci
>> V=1
>> ```
>> However, as I understand, the discussion on the ARM patches (between
>> Russel/Marc/Yury) has concluded that the ARM arch changes may not be
>> adding value over the current code, so I will drop the ARM patches
>> in the next version.
> 
> Well, hang on a second. From what I can tell, the objections haven't
> been specific to arch/arm{,64}/. You haven't really explained why this
> new helper is needed and what value it brings over the existing set of
> functionality.
> 
> So maybe start there, rather than dropping the parts that attracted the
> comments to start with?
> 
> Will

FIELD_MODIFY is similar to uxx_replace_bits() for in-memory bit field
modification, but with the added advantage of strict parameter type
checking at compile time.

Previous discussions (in patch series v3) among Russell, Marc, and Yury
focused on whether there is any added advantage of using FIELD_MODIFY()
(possibility of size overflow checking) for the specific cases being
modified by the patch series inside arm64. For the one case where enum
was being used, it was originally thought that there may be a
possibility of size overflow due to 32bit size used by the compiler for
the enum. However it was identified that the kernel's compiler already
uses 64bit types for enum if the number of bits used with the enum is
more than 32 (the code in this case used more than 32 bits).So, it seems
that there is no additional benefit of using FIELD_MODIFY() in the
specific cases in arm64/.

Please note that the current ARM64 code using FIELD_PREP() also supports
strict parameter checking at compile time. Both FIELD_PREP and
FIELD_MODIFY utilize `__BF_FIELD_CHECK()` to support this compile-time
parameter validation.

Re: [cocci] [v4 2/5] arm64: tlb: Convert the opencoded field modify

[Markus Elfring]

>>> Replace below code with the wrapper FIELD_MODIFY(MASK, &reg, val)
>>> - reg &= ~MASK;
>>> - reg |= FIELD_PREP(MASK, val);
…
> I will review the generated message to improve the formatting as you
> suggested, the next time I use it.
Would indentation be more helpful for the mentioned code excerpt
(instead of trying to describe lines with bullet points)?

Regards,
Markus

Re: [cocci] [v4 2/5] arm64: tlb: Convert the opencoded field modify

[Luo Jie]

On 6/16/2025 9:52 PM, Markus Elfring wrote:
>>>> Replace below code with the wrapper FIELD_MODIFY(MASK, &reg, val)
>>>> - reg &= ~MASK;
>>>> - reg |= FIELD_PREP(MASK, val);
> …
>> I will review the generated message to improve the formatting as you
>> suggested, the next time I use it.
> Would indentation be more helpful for the mentioned code excerpt
> (instead of trying to describe lines with bullet points)?
> 
> Regards,
> Markus

I agree that using indentation would make the code excerpt clearer.
Thanks.

[PATCH v4 3/5] arm64: nvhe: Convert the opencoded field modify

[Luo Jie]

Replace 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
http://coccinelle.lip6.fr/

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 dee1a406b0c2..c1dbea320131 100644
--- a/arch/arm64/kvm/hyp/include/nvhe/memory.h
+++ b/arch/arm64/kvm/hyp/include/nvhe/memory.h
@@ -37,8 +37,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 v4 4/5] arm64: kvm: Convert the opencoded field modify

[Luo Jie]

Replace 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
http://coccinelle.lip6.fr/

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 v4 5/5] arm64: mm: Convert the opencoded field modify

[Luo Jie]

Replace 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
http://coccinelle.lip6.fr/

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 v4 0/5] Add FIELD_MODIFY() helper

[Marc Zyngier]

On Thu, 12 Jun 2025 14:46: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.
> 
> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>

I already indicated that the *pre-existing* set of helpers are enough
for what we want to do, that we *already* use them for KVM/arm64, and
that I didn't need nor want two ways to do the same thing in the same
code base.

My opinion hasn't changed on that front, and I don't see a point in
these patches.

	M.

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

Re: [PATCH v4 0/5] Add FIELD_MODIFY() helper

[Luo Jie]

On 6/12/2025 10:11 PM, Marc Zyngier wrote:
> On Thu, 12 Jun 2025 14:46: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.
>>
>> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> 
> I already indicated that the *pre-existing* set of helpers are enough
> for what we want to do, that we *already* use them for KVM/arm64, and
> that I didn't need nor want two ways to do the same thing in the same
> code base.
> 
> My opinion hasn't changed on that front, and I don't see a point in
> these patches.
> 
> 	M.
> 

OK. I will drop the ARM64 patches and only keep the coccinelle script
patch in the next version.