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

[shashi . mallela]

On Thu, 2021-08-19 at 14:27 +0100, Peter Maydell wrote:
> 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
Since the memory map for sbsa-ref has been updated with ITS address
range added between distributor and redistributor regions(as per last
reveiw comments),this has resulted in a change in the redistributor
base address(as compared to previous sbsa-ref with no ITS
support).Hence there was a subsequent request for creating a versioning
logic to differentiate between ITS presence or absence which would be
of use to other modules (like TF-A) to pick the relevant redistributor
base address based on this versioning.