Message-ID: <aAk7VqNOLujcyZS0@shell.armlinux.org.uk>
Date: Wed, 23 Apr 2025 20:11:18 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Yury Norov <yury.norov@...il.com>
Cc: Marc Zyngier <maz@...nel.org>, Luo Jie <quic_luoj@...cinc.com>,
	Rasmus Villemoes <linux@...musvillemoes.dk>,
	Julia Lawall <Julia.Lawall@...ia.fr>,
	Nicolas Palix <nicolas.palix@...g.fr>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>,
	Oliver Upton <oliver.upton@...ux.dev>,
	Joey Gouly <joey.gouly@....com>,
	Suzuki K Poulose <suzuki.poulose@....com>,
	Zenghui Yu <yuzenghui@...wei.com>, linux-kernel@...r.kernel.org,
	cocci@...ia.fr, linux-arm-kernel@...ts.infradead.org,
	kvmarm@...ts.linux.dev, andrew@...n.ch, quic_kkumarcs@...cinc.com,
	quic_linchen@...cinc.com, quic_leiwei@...cinc.com,
	quic_suruchia@...cinc.com, quic_pavir@...cinc.com
Subject: Re: [PATCH v3 4/6] arm64: nvhe: Convert the opencoded field modify

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@...cinc.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@...cinc.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!

Powered by blists - more mailing lists