| Message ID | 20220528113829.1043361-6-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM/arm64: Refactoring the vcpu flags | expand |
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. > + > +#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 ?) > +#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? Thanks, Reiji > + > + > /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */ > #define vcpu_sve_pffr(vcpu) (kern_hyp_va((vcpu)->arch.sve_state) + \ > sve_ffr_offset((vcpu)->arch.sve_max_vl)) > -- > 2.34.1 > > _______________________________________________ > kvmarm mailing list > kvmarm@lists.cs.columbia.edu > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
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.
Hi Marc, > > > +#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. > */ Looks good to me, thank you! Reiji
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) + +#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 +#define vcpu_flag_unpack(...) __flag_unpack(__VA_ARGS__) + + /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */ #define vcpu_sve_pffr(vcpu) (kern_hyp_va((vcpu)->arch.sve_state) + \ sve_ffr_offset((vcpu)->arch.sve_max_vl))
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(+)