Re: [PATCH v5 09/14] KVM: arm64: Atomically update stage 2 leaf attributes in parallel walks

[Marc Zyngier <maz@kernel.org>]

On Wed, 09 Nov 2022 23:00:16 +0000,
Ben Gardon <bgardon@google.com> wrote:
> 
> On Wed, Nov 9, 2022 at 2:42 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Wed, Nov 09, 2022, Ben Gardon wrote:
> > > On Mon, Nov 7, 2022 at 1:58 PM Oliver Upton <oliver.upton@linux.dev> wrote:
> > > > @@ -1054,7 +1066,7 @@ kvm_pte_t kvm_pgtable_stage2_mkold(struct kvm_pgtable *pgt, u64 addr)
> > > >  bool kvm_pgtable_stage2_is_young(struct kvm_pgtable *pgt, u64 addr)
> > > >  {
> > > >         kvm_pte_t pte = 0;
> > > > -       stage2_update_leaf_attrs(pgt, addr, 1, 0, 0, &pte, NULL);
> > > > +       stage2_update_leaf_attrs(pgt, addr, 1, 0, 0, &pte, NULL, 0);
> > >
> > > Would be nice to have an enum for KVM_PGTABLE_WALK_EXCLUSIVE so this
> > > doesn't just have to pass 0.
> >
> > That's also dangerous though since the param is a set of flags, not unique,
> > arbitrary values.  E.g. this won't do the expected thing
> >
> >         if (flags & KVM_PGTABLE_WALK_EXCLUSIVE)
> >
> > I assume compilers would complain, but never say never when it comes to compilers :-)
> 
> Yeah, I was thinking about that too. IMO using one enum for multiple
> flags is kind of an abuse of the enum. If you're going to put multiple
> orthogonal flags in an int or whatever, it would probably be best to
> have separate enums for each flag. That way you can define masks to
> extract the enum from the int and only compare with == and != as
> opposed to using &.

Too late. The kernel is filled of this (look at the irq code, for
example), and we happily use this construct all over the (oh wait!)
page table code to construct permissions and other things.

At this stage, this is an established construct. Compiler people can
try and break this habit, good luck to them ;-).

	M.

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