Re: [PATCH v8 07/10] hw/arm/sbsa-ref: add ITS support in SBSA GIC

[Peter Maydell]

On Thu, 12 Aug 2021 at 17:53, Shashi Mallela <shashi.mallela@linaro.org> wrote:
>
> Included creation of ITS as part of SBSA platform GIC
> initialization.
>
> Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
> ---
>  hw/arm/sbsa-ref.c | 79 ++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 75 insertions(+), 4 deletions(-)
>

> +static char *sbsa_get_version(Object *obj, Error **errp)
> +{
> +    SBSAMachineState *sms = SBSA_MACHINE(obj);
> +
> +    switch (sms->version) {
> +    case SBSA_DEFAULT:
> +        return g_strdup("default");
> +    case SBSA_ITS:
> +        return g_strdup("sbsaits");
> +    default:
> +        g_assert_not_reached();
> +    }
> +}
> +
> +static void sbsa_set_version(Object *obj, const char *value, Error **errp)
> +{
> +    SBSAMachineState *sms = SBSA_MACHINE(obj);
> +
> +    if (!strcmp(value, "sbsaits")) {
> +        sms->version = SBSA_ITS;
> +    } else if (!strcmp(value, "default")) {
> +        sms->version = SBSA_DEFAULT;
> +    } else {
> +        error_setg(errp, "Invalid version value");
> +        error_append_hint(errp, "Valid values are default, sbsaits.\n");
> +    }
> +}
>
>  static void sbsa_ref_instance_init(Object *obj)
>  {
>      SBSAMachineState *sms = SBSA_MACHINE(obj);
>
> +    sms->version = SBSA_ITS;
>      sbsa_flash_create(sms);
>  }
>
> @@ -850,6 +915,12 @@ static void sbsa_ref_class_init(ObjectClass *oc, void 
> *data)
>      mc->possible_cpu_arch_ids = sbsa_ref_possible_cpu_arch_ids;
>      mc->cpu_index_to_instance_props = sbsa_ref_cpu_index_to_props;
>      mc->get_default_cpu_node_id = sbsa_ref_get_default_cpu_node_id;
> +
> +    object_class_property_add_str(oc, "version", sbsa_get_version,
> +                                  sbsa_set_version);
> +    object_class_property_set_description(oc, "version",
> +                                          "Set the Version type. "
> +                                          "Valid values are default & 
> sbsaits");

This doesn't look right; where has it come from ?

If you want a command line switch to let the user say whether the
ITS should be present or not, you should use the same thing the
virt board does, which is a bool property "its".

If you want the sbsa-ref board to become a proper "versioned machine
type" such that users can say "-M sbsa-ref-6.1" and get the SBSA
board exactly as QEMU 6.1 supplied it, that looks completely different
(and is a heavy back-compatibility burden, so needs discussion about
whether now is the right time to do it), and probably is better not
tangled up with this patchseries.

thanks
-- PMM