Re: [PATCH 05/18] KVM: arm64: Add helpers to manipulate vcpu flags among a set

[Marc Zyngier <maz@kernel.org>]

On Wed, 08 Jun 2022 06:26:44 +0100,
Reiji Watanabe <reijiw@google.com> wrote:
> 
> Hi Marc,
> 
> On Sat, May 28, 2022 at 4:38 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > Careful analysis of the vcpu flags show that this is a mix of
> > configuration, communication between the host and the hypervisor,
> > as well as anciliary state that has no consistency. It'd be a lot
> > better if we could split these flags into consistent categories.
> >
> > However, even if we split these flags apart, we want to make sure
> > that each flag can only be applied to its own set, and not across
> > sets.
> >
> > To achieve this, use a preprocessor hack so that each flag is always
> > associated with:
> >
> > - the set that contains it,
> >
> > - a mask that describe all the bits that contain it (for a simple
> >   flag, this is the same thing as the flag itself, but we will
> >   eventually have values that cover multiple bits at once).
> >
> > Each flag is thus a triplet that is not directly usable as a value,
> > but used by three helpers that allow the flag to be set, cleared,
> > and fetched. By mandating the use of such helper, we can easily
> > enforce that a flag can only be used with the set it belongs to.
> >
> > Finally, one last helper "unpacks" the raw value from the triplet
> > that represents a flag, which is useful for multi-bit values that
> > need to be enumerated (in a switch statement, for example).
> >
> > Further patches will start making use of this infrastructure.
> >
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/include/asm/kvm_host.h | 33 +++++++++++++++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index a46f952b97f6..5eb6791df608 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -418,6 +418,39 @@ struct kvm_vcpu_arch {
> >         } steal;
> >  };
> >
> > +#define __vcpu_get_flag(v, flagset, f, m)                      \
> > +       ({                                                      \
> > +               v->arch.flagset & (m);                          \
> > +       })
> > +
> > +#define __vcpu_set_flag(v, flagset, f, m)                      \
> > +       do {                                                    \
> > +               typeof(v->arch.flagset) *fset;                  \
> > +                                                               \
> > +               fset = &v->arch.flagset;                        \
> > +               if (HWEIGHT(m) > 1)                             \
> > +                       *fset &= ~(m);                          \
> > +               *fset |= (f);                                   \
> > +       } while (0)
> > +
> > +#define __vcpu_clear_flag(v, flagset, f, m)                    \
> > +       do {                                                    \
> > +               typeof(v->arch.flagset) *fset;                  \
> > +                                                               \
> > +               fset = &v->arch.flagset;                        \
> > +               *fset &= ~(m);                                  \
> > +       } while (0)
> 
> I think 'v' should be enclosed in parentheses in those three macros.

Fair enough.

>
> 
> > +
> > +#define vcpu_get_flag(v, ...)  __vcpu_get_flag(v, __VA_ARGS__)
> > +#define vcpu_set_flag(v, ...)  __vcpu_set_flag(v, __VA_ARGS__)
> > +#define vcpu_clear_flag(v, ...)        __vcpu_clear_flag(v, __VA_ARGS__)
> > +
> > +#define __vcpu_single_flag(_set, _f)   _set, (_f), (_f)
> > +
> > +#define __flag_unpack(_set, _f, _m)    _f
> 
> Nit: Probably it might be worth adding a comment that explains the
> above two macros ? (e.g. what is each element of the triplets ?)

How about this?

/*
 * Each 'flag' is composed of a comma-separated triplet:
 *
 * - the flag-set it belongs to in the vcpu->arch structure
 * - the value for that flag
 * - the mask for that flag
 *
 *  __vcpu_single_flag() builds such a triplet for a single-bit flag.
 * unpack_vcpu_flag() extract the flag value from the triplet for
 * direct use outside of the flag accessors.
 */

>
> > +#define vcpu_flag_unpack(...)          __flag_unpack(__VA_ARGS__)
> 
> Minor nit: KVM Functions and macros whose names begin with "vcpu_"
> make me think that they are the operations for a vCPU specified in
> the argument, but this macro is not (this might just my own
> assumption?). So, IMHO I would prefer a name whose prefix is not
> "vcpu_". Having said that, I don't have any good suggestions though...
> Perhaps I might prefer "unpack_vcpu_flag" a bit instead?

Sold!

Thanks,

	M.

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