[PATCH v3 4/6] arm64: nvhe: Convert the opencoded field modify
Yury Norov
yury.norov at gmail.com
Wed Apr 23 12:40:45 PDT 2025
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 at quicinc.com> wrote:
> > > > > >
> > > > > > Replaced below code with the wrapper FIELD_MODIFY(MASK, ®, 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 at 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.
More information about the linux-arm-kernel
mailing list