| Message ID | 20241022-virtio-v2-2-b2394236e053@daynix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | virtio: Convert feature properties to OnOffAuto | expand |
On Tue, Oct 22, 2024 at 01:50:39PM +0900, Akihiko Odaki wrote: > DEFINE_PROP_ON_OFF_AUTO_BIT64() corresponds to DEFINE_PROP_ON_OFF_AUTO() > as DEFINE_PROP_BIT64() corresponds to DEFINE_PROP_BOOL(). The difference > is that DEFINE_PROP_ON_OFF_AUTO_BIT64() exposes OnOffAuto instead of > bool. Again, same feedback as last time. With this design, existing users of DEFINE_PROP_BIT64 that get converted to DEFINE_PROP_ON_OFF_AUTO_BIT64, in addition to gaining the desired "auto" value, also gain redundant 'on' and 'off' values as side-effects. In the next patch, the stated problem is that virtio code needs to distinguish between bits that are user set, and bits that are set based on available host backend features. That doesn't seem to require us to accept any new values from the user. It should be sufficient to track user specified features, separately from user specified values. ie when parsing user input for bitfields, we need to parse into a pair of fields uint64_t has_user_features; /* which bits were specified */ uint64_t user_features; /* values of specified bits*/ > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > include/hw/qdev-properties.h | 18 ++++++++++++ > hw/core/qdev-properties.c | 66 +++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 83 insertions(+), 1 deletion(-) > > diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h > index 09aa04ca1e27..afec53a48470 100644 > --- a/include/hw/qdev-properties.h > +++ b/include/hw/qdev-properties.h > @@ -43,11 +43,22 @@ struct PropertyInfo { > ObjectPropertyRelease *release; > }; > > +/** > + * struct OnOffAutoBit64 - OnOffAuto storage with 64 elements. > + * @on_bits: Bitmap of elements with "on". > + * @auto_bits: Bitmap of elements with "auto". > + */ > +typedef struct OnOffAutoBit64 { > + uint64_t on_bits; > + uint64_t auto_bits; > +} OnOffAutoBit64; > + > > /*** qdev-properties.c ***/ > > extern const PropertyInfo qdev_prop_bit; > extern const PropertyInfo qdev_prop_bit64; > +extern const PropertyInfo qdev_prop_on_off_auto_bit64; > extern const PropertyInfo qdev_prop_bool; > extern const PropertyInfo qdev_prop_enum; > extern const PropertyInfo qdev_prop_uint8; > @@ -100,6 +111,13 @@ extern const PropertyInfo qdev_prop_link; > .set_default = true, \ > .defval.u = (bool)_defval) > > +#define DEFINE_PROP_ON_OFF_AUTO_BIT64(_name, _state, _field, _bit, _defval) \ > + DEFINE_PROP(_name, _state, _field, qdev_prop_on_off_auto_bit64, \ > + OnOffAutoBit64, \ > + .bitnr = (_bit), \ > + .set_default = true, \ > + .defval.i = (OnOffAuto)_defval) > + > #define DEFINE_PROP_BOOL(_name, _state, _field, _defval) \ > DEFINE_PROP(_name, _state, _field, qdev_prop_bool, bool, \ > .set_default = true, \ > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c > index f0a270bb4f61..cc76ff0dfae6 100644 > --- a/hw/core/qdev-properties.c > +++ b/hw/core/qdev-properties.c > @@ -187,7 +187,8 @@ const PropertyInfo qdev_prop_bit = { > > static uint64_t qdev_get_prop_mask64(Property *prop) > { > - assert(prop->info == &qdev_prop_bit64); > + assert(prop->info == &qdev_prop_bit64 || > + prop->info == &qdev_prop_on_off_auto_bit64); > return 0x1ull << prop->bitnr; > } > > @@ -232,6 +233,69 @@ const PropertyInfo qdev_prop_bit64 = { > .set_default_value = set_default_value_bool, > }; > > +static void prop_get_on_off_auto_bit64(Object *obj, Visitor *v, > + const char *name, void *opaque, > + Error **errp) > +{ > + Property *prop = opaque; > + OnOffAutoBit64 *p = object_field_prop_ptr(obj, prop); > + int value; > + uint64_t mask = qdev_get_prop_mask64(prop); > + > + if (p->auto_bits & mask) { > + value = ON_OFF_AUTO_AUTO; > + } else if (p->on_bits & mask) { > + value = ON_OFF_AUTO_ON; > + } else { > + value = ON_OFF_AUTO_OFF; > + } > + > + visit_type_enum(v, name, &value, &OnOffAuto_lookup, errp); > +} > + > +static void prop_set_on_off_auto_bit64(Object *obj, Visitor *v, > + const char *name, void *opaque, > + Error **errp) > +{ > + Property *prop = opaque; > + OnOffAutoBit64 *p = object_field_prop_ptr(obj, prop); > + bool bool_value; > + int value; > + uint64_t mask = qdev_get_prop_mask64(prop); > + > + if (visit_type_bool(v, name, &bool_value, NULL)) { > + value = bool_value ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF; > + } else if (!visit_type_enum(v, name, &value, &OnOffAuto_lookup, errp)) { > + return; > + } > + > + switch (value) { > + case ON_OFF_AUTO_AUTO: > + p->on_bits &= ~mask; > + p->auto_bits |= mask; > + break; > + > + case ON_OFF_AUTO_ON: > + p->on_bits |= mask; > + p->auto_bits &= ~mask; > + break; > + > + case ON_OFF_AUTO_OFF: > + p->on_bits &= ~mask; > + p->auto_bits &= ~mask; > + break; > + } > +} > + > +const PropertyInfo qdev_prop_on_off_auto_bit64 = { > + .name = "OnOffAuto", > + .description = "on/off/auto", > + .enum_table = &OnOffAuto_lookup, > + .get = prop_get_on_off_auto_bit64, > + .set = prop_set_on_off_auto_bit64, > + .set_default_value = qdev_propinfo_set_default_value_enum, > +}; > + > /* --- bool --- */ > > static void get_bool(Object *obj, Visitor *v, const char *name, void *opaque, > > -- > 2.47.0 > > With regards, Daniel
On 2024/10/29 1:50, Daniel P. Berrangé wrote: > On Tue, Oct 22, 2024 at 01:50:39PM +0900, Akihiko Odaki wrote: >> DEFINE_PROP_ON_OFF_AUTO_BIT64() corresponds to DEFINE_PROP_ON_OFF_AUTO() >> as DEFINE_PROP_BIT64() corresponds to DEFINE_PROP_BOOL(). The difference >> is that DEFINE_PROP_ON_OFF_AUTO_BIT64() exposes OnOffAuto instead of >> bool. > > Again, same feedback as last time. > > With this design, existing users of DEFINE_PROP_BIT64 that > get converted to DEFINE_PROP_ON_OFF_AUTO_BIT64, in addition > to gaining the desired "auto" value, also gain redundant > 'on' and 'off' values as side-effects. > > In the next patch, the stated problem is that virtio code > needs to distinguish between bits that are user set, and > bits that are set based on available host backend features. > > That doesn't seem to require us to accept any new values > from the user. It should be sufficient to track user > specified features, separately from user specified values. > ie when parsing user input for bitfields, we need to parse > into a pair of fields > > uint64_t has_user_features; /* which bits were specified */ > uint64_t user_features; /* values of specified bits*/ Properties also have getters. We don't know what to return with them without the new value. Regards, Akihiko Odaki > >> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >> --- >> include/hw/qdev-properties.h | 18 ++++++++++++ >> hw/core/qdev-properties.c | 66 +++++++++++++++++++++++++++++++++++++++++++- >> 2 files changed, 83 insertions(+), 1 deletion(-) >> >> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h >> index 09aa04ca1e27..afec53a48470 100644 >> --- a/include/hw/qdev-properties.h >> +++ b/include/hw/qdev-properties.h >> @@ -43,11 +43,22 @@ struct PropertyInfo { >> ObjectPropertyRelease *release; >> }; >> >> +/** >> + * struct OnOffAutoBit64 - OnOffAuto storage with 64 elements. >> + * @on_bits: Bitmap of elements with "on". >> + * @auto_bits: Bitmap of elements with "auto". >> + */ >> +typedef struct OnOffAutoBit64 { >> + uint64_t on_bits; >> + uint64_t auto_bits; >> +} OnOffAutoBit64; >> + >> >> /*** qdev-properties.c ***/ >> >> extern const PropertyInfo qdev_prop_bit; >> extern const PropertyInfo qdev_prop_bit64; >> +extern const PropertyInfo qdev_prop_on_off_auto_bit64; >> extern const PropertyInfo qdev_prop_bool; >> extern const PropertyInfo qdev_prop_enum; >> extern const PropertyInfo qdev_prop_uint8; >> @@ -100,6 +111,13 @@ extern const PropertyInfo qdev_prop_link; >> .set_default = true, \ >> .defval.u = (bool)_defval) >> >> +#define DEFINE_PROP_ON_OFF_AUTO_BIT64(_name, _state, _field, _bit, _defval) \ >> + DEFINE_PROP(_name, _state, _field, qdev_prop_on_off_auto_bit64, \ >> + OnOffAutoBit64, \ >> + .bitnr = (_bit), \ >> + .set_default = true, \ >> + .defval.i = (OnOffAuto)_defval) >> + >> #define DEFINE_PROP_BOOL(_name, _state, _field, _defval) \ >> DEFINE_PROP(_name, _state, _field, qdev_prop_bool, bool, \ >> .set_default = true, \ >> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c >> index f0a270bb4f61..cc76ff0dfae6 100644 >> --- a/hw/core/qdev-properties.c >> +++ b/hw/core/qdev-properties.c >> @@ -187,7 +187,8 @@ const PropertyInfo qdev_prop_bit = { >> >> static uint64_t qdev_get_prop_mask64(Property *prop) >> { >> - assert(prop->info == &qdev_prop_bit64); >> + assert(prop->info == &qdev_prop_bit64 || >> + prop->info == &qdev_prop_on_off_auto_bit64); >> return 0x1ull << prop->bitnr; >> } >> >> @@ -232,6 +233,69 @@ const PropertyInfo qdev_prop_bit64 = { >> .set_default_value = set_default_value_bool, >> }; >> >> +static void prop_get_on_off_auto_bit64(Object *obj, Visitor *v, >> + const char *name, void *opaque, >> + Error **errp) >> +{ >> + Property *prop = opaque; >> + OnOffAutoBit64 *p = object_field_prop_ptr(obj, prop); >> + int value; >> + uint64_t mask = qdev_get_prop_mask64(prop); >> + >> + if (p->auto_bits & mask) { >> + value = ON_OFF_AUTO_AUTO; >> + } else if (p->on_bits & mask) { >> + value = ON_OFF_AUTO_ON; >> + } else { >> + value = ON_OFF_AUTO_OFF; >> + } >> + >> + visit_type_enum(v, name, &value, &OnOffAuto_lookup, errp); >> +} >> + >> +static void prop_set_on_off_auto_bit64(Object *obj, Visitor *v, >> + const char *name, void *opaque, >> + Error **errp) >> +{ >> + Property *prop = opaque; >> + OnOffAutoBit64 *p = object_field_prop_ptr(obj, prop); >> + bool bool_value; >> + int value; >> + uint64_t mask = qdev_get_prop_mask64(prop); >> + >> + if (visit_type_bool(v, name, &bool_value, NULL)) { >> + value = bool_value ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF; >> + } else if (!visit_type_enum(v, name, &value, &OnOffAuto_lookup, errp)) { >> + return; >> + } >> + >> + switch (value) { >> + case ON_OFF_AUTO_AUTO: >> + p->on_bits &= ~mask; >> + p->auto_bits |= mask; >> + break; >> + >> + case ON_OFF_AUTO_ON: >> + p->on_bits |= mask; >> + p->auto_bits &= ~mask; >> + break; >> + >> + case ON_OFF_AUTO_OFF: >> + p->on_bits &= ~mask; >> + p->auto_bits &= ~mask; >> + break; >> + } >> +} >> + >> +const PropertyInfo qdev_prop_on_off_auto_bit64 = { >> + .name = "OnOffAuto", >> + .description = "on/off/auto", >> + .enum_table = &OnOffAuto_lookup, >> + .get = prop_get_on_off_auto_bit64, >> + .set = prop_set_on_off_auto_bit64, >> + .set_default_value = qdev_propinfo_set_default_value_enum, >> +}; >> + >> /* --- bool --- */ >> >> static void get_bool(Object *obj, Visitor *v, const char *name, void *opaque, >> >> -- >> 2.47.0 >> >> > > With regards, > Daniel
On Thu, Oct 31, 2024 at 04:21:53PM +0900, Akihiko Odaki wrote: > On 2024/10/29 1:50, Daniel P. Berrangé wrote: > > On Tue, Oct 22, 2024 at 01:50:39PM +0900, Akihiko Odaki wrote: > > > DEFINE_PROP_ON_OFF_AUTO_BIT64() corresponds to DEFINE_PROP_ON_OFF_AUTO() > > > as DEFINE_PROP_BIT64() corresponds to DEFINE_PROP_BOOL(). The difference > > > is that DEFINE_PROP_ON_OFF_AUTO_BIT64() exposes OnOffAuto instead of > > > bool. > > > > Again, same feedback as last time. > > > > With this design, existing users of DEFINE_PROP_BIT64 that > > get converted to DEFINE_PROP_ON_OFF_AUTO_BIT64, in addition > > to gaining the desired "auto" value, also gain redundant > > 'on' and 'off' values as side-effects. > > > > In the next patch, the stated problem is that virtio code > > needs to distinguish between bits that are user set, and > > bits that are set based on available host backend features. > > > > That doesn't seem to require us to accept any new values > > from the user. It should be sufficient to track user > > specified features, separately from user specified values. > > ie when parsing user input for bitfields, we need to parse > > into a pair of fields > > > > uint64_t has_user_features; /* which bits were specified */ > > uint64_t user_features; /* values of specified bits*/ > > Properties also have getters. We don't know what to return with them without > the new value. The virtio changes in the next patch are just accessing the bitsets directly. A getter could just be made to return false for unset values, on the assumption that any caller should be checking the has_user_features bits beforehand. With regards, Daniel
On 2024/11/01 20:44, Daniel P. Berrangé wrote: > On Thu, Oct 31, 2024 at 04:21:53PM +0900, Akihiko Odaki wrote: >> On 2024/10/29 1:50, Daniel P. Berrangé wrote: >>> On Tue, Oct 22, 2024 at 01:50:39PM +0900, Akihiko Odaki wrote: >>>> DEFINE_PROP_ON_OFF_AUTO_BIT64() corresponds to DEFINE_PROP_ON_OFF_AUTO() >>>> as DEFINE_PROP_BIT64() corresponds to DEFINE_PROP_BOOL(). The difference >>>> is that DEFINE_PROP_ON_OFF_AUTO_BIT64() exposes OnOffAuto instead of >>>> bool. >>> >>> Again, same feedback as last time. >>> >>> With this design, existing users of DEFINE_PROP_BIT64 that >>> get converted to DEFINE_PROP_ON_OFF_AUTO_BIT64, in addition >>> to gaining the desired "auto" value, also gain redundant >>> 'on' and 'off' values as side-effects. >>> >>> In the next patch, the stated problem is that virtio code >>> needs to distinguish between bits that are user set, and >>> bits that are set based on available host backend features. >>> >>> That doesn't seem to require us to accept any new values >>> from the user. It should be sufficient to track user >>> specified features, separately from user specified values. >>> ie when parsing user input for bitfields, we need to parse >>> into a pair of fields >>> >>> uint64_t has_user_features; /* which bits were specified */ >>> uint64_t user_features; /* values of specified bits*/ >> >> Properties also have getters. We don't know what to return with them without >> the new value. > > The virtio changes in the next patch are just accessing the bitsets > directly. A getter could just be made to return false for unset > values, on the assumption that any caller should be checking the > has_user_features bits beforehand. It means this construct is specialized for the case when the getter will never be called for the default value. I think it is difficult to convince others to introduce such a new mechanism when we already have OnOffAuto as a generic solution. There is even DEFINE_PROP_ON_OFF_AUTO() for qdev properties. The question is: when we have BOOL, BIT64, and ON_OFF_AUTO, shouldn't we have ON_OFF_AUTO_BIT64? Whether OnOffAuto is good or not is not a problem here unless we are going to deprecate DEFINE_PROP_ON_OFF_AUTO(). Regards, Akihiko Odaki
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h index 09aa04ca1e27..afec53a48470 100644 --- a/include/hw/qdev-properties.h +++ b/include/hw/qdev-properties.h @@ -43,11 +43,22 @@ struct PropertyInfo { ObjectPropertyRelease *release; }; +/** + * struct OnOffAutoBit64 - OnOffAuto storage with 64 elements. + * @on_bits: Bitmap of elements with "on". + * @auto_bits: Bitmap of elements with "auto". + */ +typedef struct OnOffAutoBit64 { + uint64_t on_bits; + uint64_t auto_bits; +} OnOffAutoBit64; + /*** qdev-properties.c ***/ extern const PropertyInfo qdev_prop_bit; extern const PropertyInfo qdev_prop_bit64; +extern const PropertyInfo qdev_prop_on_off_auto_bit64; extern const PropertyInfo qdev_prop_bool; extern const PropertyInfo qdev_prop_enum; extern const PropertyInfo qdev_prop_uint8; @@ -100,6 +111,13 @@ extern const PropertyInfo qdev_prop_link; .set_default = true, \ .defval.u = (bool)_defval) +#define DEFINE_PROP_ON_OFF_AUTO_BIT64(_name, _state, _field, _bit, _defval) \ + DEFINE_PROP(_name, _state, _field, qdev_prop_on_off_auto_bit64, \ + OnOffAutoBit64, \ + .bitnr = (_bit), \ + .set_default = true, \ + .defval.i = (OnOffAuto)_defval) + #define DEFINE_PROP_BOOL(_name, _state, _field, _defval) \ DEFINE_PROP(_name, _state, _field, qdev_prop_bool, bool, \ .set_default = true, \ diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c index f0a270bb4f61..cc76ff0dfae6 100644 --- a/hw/core/qdev-properties.c +++ b/hw/core/qdev-properties.c @@ -187,7 +187,8 @@ const PropertyInfo qdev_prop_bit = { static uint64_t qdev_get_prop_mask64(Property *prop) { - assert(prop->info == &qdev_prop_bit64); + assert(prop->info == &qdev_prop_bit64 || + prop->info == &qdev_prop_on_off_auto_bit64); return 0x1ull << prop->bitnr; } @@ -232,6 +233,69 @@ const PropertyInfo qdev_prop_bit64 = { .set_default_value = set_default_value_bool, }; +static void prop_get_on_off_auto_bit64(Object *obj, Visitor *v, + const char *name, void *opaque, + Error **errp) +{ + Property *prop = opaque; + OnOffAutoBit64 *p = object_field_prop_ptr(obj, prop); + int value; + uint64_t mask = qdev_get_prop_mask64(prop); + + if (p->auto_bits & mask) { + value = ON_OFF_AUTO_AUTO; + } else if (p->on_bits & mask) { + value = ON_OFF_AUTO_ON; + } else { + value = ON_OFF_AUTO_OFF; + } + + visit_type_enum(v, name, &value, &OnOffAuto_lookup, errp); +} + +static void prop_set_on_off_auto_bit64(Object *obj, Visitor *v, + const char *name, void *opaque, + Error **errp) +{ + Property *prop = opaque; + OnOffAutoBit64 *p = object_field_prop_ptr(obj, prop); + bool bool_value; + int value; + uint64_t mask = qdev_get_prop_mask64(prop); + + if (visit_type_bool(v, name, &bool_value, NULL)) { + value = bool_value ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF; + } else if (!visit_type_enum(v, name, &value, &OnOffAuto_lookup, errp)) { + return; + } + + switch (value) { + case ON_OFF_AUTO_AUTO: + p->on_bits &= ~mask; + p->auto_bits |= mask; + break; + + case ON_OFF_AUTO_ON: + p->on_bits |= mask; + p->auto_bits &= ~mask; + break; + + case ON_OFF_AUTO_OFF: + p->on_bits &= ~mask; + p->auto_bits &= ~mask; + break; + } +} + +const PropertyInfo qdev_prop_on_off_auto_bit64 = { + .name = "OnOffAuto", + .description = "on/off/auto", + .enum_table = &OnOffAuto_lookup, + .get = prop_get_on_off_auto_bit64, + .set = prop_set_on_off_auto_bit64, + .set_default_value = qdev_propinfo_set_default_value_enum, +}; + /* --- bool --- */ static void get_bool(Object *obj, Visitor *v, const char *name, void *opaque,
DEFINE_PROP_ON_OFF_AUTO_BIT64() corresponds to DEFINE_PROP_ON_OFF_AUTO() as DEFINE_PROP_BIT64() corresponds to DEFINE_PROP_BOOL(). The difference is that DEFINE_PROP_ON_OFF_AUTO_BIT64() exposes OnOffAuto instead of bool. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- include/hw/qdev-properties.h | 18 ++++++++++++ hw/core/qdev-properties.c | 66 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 83 insertions(+), 1 deletion(-)