Hi
On Thu, Sep 10, 2026 at 10:45 PM Amit Machhiwal <[email protected]> wrote:
>
> On 2026/09/04 11:58 PM, Marc-André Lureau wrote:
> > Signed-off-by: Marc-André Lureau <[email protected]>
>
> The patch itself looks correct, but the commit message is missing a body. At
> minimum, it should describe what is being converted and why.
Agree, but when you have dozen or more patches, it feels awkward to
repeat the same message. And if you need to fix it, it's a pain. I
added it for this patch.
>
> One thing worth calling out explicitly is `SPAPR_CAP_HPT_MAXPAGESIZE`. This
> is
> not a purely mechanical conversion: the old property type string was `"int"`,
> while `spapr_cap_get_pagesize()` and `spapr_cap_set_pagesize()` use
> `visit_type_size()`. Thus, the property has effectively had size semantics
> despite the old `"int"` type string.
Good point, I should have made a preliminary commit (like I did for
other properties that are now fixed upstream)
>
> Switching this to `&size_type_info` therefore also fixes a pre-existing
> mismatch
> between the property's declared type and the visitor used by its
> getter/setter.
> This seems worth documenting in the commit message so that future readers
> understand that the change is intentional.
>
> The conversion itself, including the switch to
> `object_class_property_add_qapi()` in `spapr_caps_add_properties()`, looks
> correct to me.
>
> Thanks,
> Amit
thanks
>
> > ---
> > hw/ppc/spapr_caps.c | 39 ++++++++++++++++++++-------------------
> > 1 file changed, 20 insertions(+), 19 deletions(-)
> >
> > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> > index 0d659e9b0251..e622c79eb52a 100644
> > --- a/hw/ppc/spapr_caps.c
> > +++ b/hw/ppc/spapr_caps.c
> > @@ -24,6 +24,7 @@
> >
> > #include "qemu/osdep.h"
> > #include "qemu/error-report.h"
> > +#include "qapi/qapi-builtin-type-infos.h"
> > #include "qapi/error.h"
> > #include "qapi/visitor.h"
> > #include "system/hw_accel.h"
> > @@ -59,7 +60,7 @@ typedef struct SpaprCapabilityInfo {
> > /* Getter and Setter Function Pointers */
> > ObjectPropertyAccessor *get;
> > ObjectPropertyAccessor *set;
> > - const char *type;
> > + const QAPITypeInfo *type;
> > /* Possible values if this is a custom string type */
> > SpaprCapPossible *possible;
> > /* Make sure the virtual hardware can support this capability */
> > @@ -725,7 +726,7 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> > .index = SPAPR_CAP_HTM,
> > .get = spapr_cap_get_bool,
> > .set = spapr_cap_set_bool,
> > - .type = "bool",
> > + .type = &bool_type_info,
> > .apply = cap_htm_apply,
> > },
> > [SPAPR_CAP_VSX] = {
> > @@ -734,7 +735,7 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> > .index = SPAPR_CAP_VSX,
> > .get = spapr_cap_get_bool,
> > .set = spapr_cap_set_bool,
> > - .type = "bool",
> > + .type = &bool_type_info,
> > .apply = cap_vsx_apply,
> > },
> > [SPAPR_CAP_DFP] = {
> > @@ -743,7 +744,7 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> > .index = SPAPR_CAP_DFP,
> > .get = spapr_cap_get_bool,
> > .set = spapr_cap_set_bool,
> > - .type = "bool",
> > + .type = &bool_type_info,
> > .apply = cap_dfp_apply,
> > },
> > [SPAPR_CAP_CFPC] = {
> > @@ -752,7 +753,7 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> > .index = SPAPR_CAP_CFPC,
> > .get = spapr_cap_get_string,
> > .set = spapr_cap_set_string,
> > - .type = "string",
> > + .type = &str_type_info,
> > .possible = &cap_cfpc_possible,
> > .apply = cap_safe_cache_apply,
> > },
> > @@ -762,7 +763,7 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> > .index = SPAPR_CAP_SBBC,
> > .get = spapr_cap_get_string,
> > .set = spapr_cap_set_string,
> > - .type = "string",
> > + .type = &str_type_info,
> > .possible = &cap_sbbc_possible,
> > .apply = cap_safe_bounds_check_apply,
> > },
> > @@ -774,7 +775,7 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> > .index = SPAPR_CAP_IBS,
> > .get = spapr_cap_get_string,
> > .set = spapr_cap_set_string,
> > - .type = "string",
> > + .type = &str_type_info,
> > .possible = &cap_ibs_possible,
> > .apply = cap_safe_indirect_branch_apply,
> > },
> > @@ -784,7 +785,7 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> > .index = SPAPR_CAP_HPT_MAXPAGESIZE,
> > .get = spapr_cap_get_pagesize,
> > .set = spapr_cap_set_pagesize,
> > - .type = "int",
> > + .type = &size_type_info,
> > .apply = cap_hpt_maxpagesize_apply,
> > .cpu_apply = cap_hpt_maxpagesize_cpu_apply,
> > },
> > @@ -794,7 +795,7 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> > .index = SPAPR_CAP_NESTED_KVM_HV,
> > .get = spapr_cap_get_bool,
> > .set = spapr_cap_set_bool,
> > - .type = "bool",
> > + .type = &bool_type_info,
> > .apply = cap_nested_kvm_hv_apply,
> > },
> > [SPAPR_CAP_NESTED_PAPR] = {
> > @@ -803,7 +804,7 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> > .index = SPAPR_CAP_NESTED_PAPR,
> > .get = spapr_cap_get_bool,
> > .set = spapr_cap_set_bool,
> > - .type = "bool",
> > + .type = &bool_type_info,
> > .apply = cap_nested_papr_apply,
> > },
> > [SPAPR_CAP_LARGE_DECREMENTER] = {
> > @@ -812,7 +813,7 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> > .index = SPAPR_CAP_LARGE_DECREMENTER,
> > .get = spapr_cap_get_bool,
> > .set = spapr_cap_set_bool,
> > - .type = "bool",
> > + .type = &bool_type_info,
> > .apply = cap_large_decr_apply,
> > .cpu_apply = cap_large_decr_cpu_apply,
> > },
> > @@ -822,7 +823,7 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> > .index = SPAPR_CAP_CCF_ASSIST,
> > .get = spapr_cap_get_bool,
> > .set = spapr_cap_set_bool,
> > - .type = "bool",
> > + .type = &bool_type_info,
> > .apply = cap_ccf_assist_apply,
> > },
> > [SPAPR_CAP_FWNMI] = {
> > @@ -831,7 +832,7 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> > .index = SPAPR_CAP_FWNMI,
> > .get = spapr_cap_get_bool,
> > .set = spapr_cap_set_bool,
> > - .type = "bool",
> > + .type = &bool_type_info,
> > .apply = cap_fwnmi_apply,
> > },
> > [SPAPR_CAP_RPT_INVALIDATE] = {
> > @@ -840,7 +841,7 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> > .index = SPAPR_CAP_RPT_INVALIDATE,
> > .get = spapr_cap_get_bool,
> > .set = spapr_cap_set_bool,
> > - .type = "bool",
> > + .type = &bool_type_info,
> > .apply = cap_rpt_invalidate_apply,
> > },
> > [SPAPR_CAP_AIL_MODE_3] = {
> > @@ -849,7 +850,7 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> > .index = SPAPR_CAP_AIL_MODE_3,
> > .get = spapr_cap_get_bool,
> > .set = spapr_cap_set_bool,
> > - .type = "bool",
> > + .type = &bool_type_info,
> > .apply = cap_ail_mode_3_apply,
> > },
> > [SPAPR_CAP_DAWR1] = {
> > @@ -858,7 +859,7 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> > .index = SPAPR_CAP_DAWR1,
> > .get = spapr_cap_get_bool,
> > .set = spapr_cap_set_bool,
> > - .type = "bool",
> > + .type = &bool_type_info,
> > .apply = cap_dawr1_apply,
> > },
> > };
> > @@ -1069,9 +1070,9 @@ void spapr_caps_add_properties(SpaprMachineClass *smc)
> > g_autofree char *name = g_strdup_printf("cap-%s", cap->name);
> > g_autofree char *desc = g_strdup(cap->description);
> >
> > - object_class_property_add(klass, name, cap->type,
> > - cap->get, cap->set,
> > - NULL, cap);
> > + object_class_property_add_qapi(klass, name, cap->type,
> > + cap->get, cap->set,
> > + NULL, cap);
> >
> > object_class_property_set_description(klass, name, desc);
> > }
> >
> > --
> > 2.55.0.543.g5ebe2ebe4ea8
> >
> >
>
