Re: [PATCH v4 30/75] ppc/spapr-caps: convert to QAPI-aware property registration

[Amit Machhiwal <amachhiw@linux.ibm.com>]

On 2026/09/04 11:58 PM, Marc-André Lureau wrote:
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

The patch itself looks correct, but the commit message is missing a body.  At
minimum, it should describe what is being converted and why.

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.

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

> ---
>  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
> 
>