On Sat, 11 Jun 2022 19:37:50 +0100,
Reiji Watanabe <reijiw@xxxxxxxxxx> wrote:
>
> On Fri, Jun 10, 2022 at 2:28 AM Marc Zyngier <maz@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > arch/arm64/include/asm/kvm_host.h | 44 +++++++++++++++++++++++++++++++
> > 1 file changed, 44 insertions(+)
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index 372c5642cfab..6d30ac7e3164 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -415,6 +415,50 @@ struct kvm_vcpu_arch {
> > } steal;
> > };
> >
> > +/*
> > + * 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_single_flag(_set, _f) _set, (_f), (_f)
> > +
> > +#define __unpack_flag(_set, _f, _m) _f
> > +#define unpack_vcpu_flag(...) __unpack_flag(__VA_ARGS__)
> > +
> > +#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)
>
> Reviewed-by: Reiji Watanabe <reijiw@xxxxxxxxxx>
>
> IMHO I would prefer to have 'v' enclosed in parentheses in the
> implementation of __vcpu_{get,set,clear}_flag rather than in
> the implementation of vcpu_{get,set,clear}_flag though.
> (That was what I meant in my comment for v1)
I understood what you were asking the first place, but I don't think
this has any advantage over what is above. __vcpu_{get,set,clear}_flag
are not meant to be used directly, and adding extra bracketing to
these only makes them more painful to read.
If you can show a case where it breaks, I'll be happy to revisit this.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
