[v3] Add FIELD_MODIFY() helper

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

Posted by Luo Jie 8 months ago

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

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

Posted by Marc Zyngier 8 months ago

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 4/6] arm64: nvhe: Convert the opencoded field modify

Posted by Yury Norov 8 months ago

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

Posted by Russell King (Oracle) 7 months, 3 weeks ago

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

Posted by Yury Norov 7 months, 3 weeks ago

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 4/6] arm64: nvhe: Convert the opencoded field modify

Posted by Russell King (Oracle) 7 months, 3 weeks ago

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

Posted by Yury Norov 7 months, 3 weeks ago

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: [PATCH v3 4/6] arm64: nvhe: Convert the opencoded field modify

Posted by Marc Zyngier 8 months ago

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.