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

[Leif Lindholm]

Hi Peter,

On Thu, Aug 19, 2021 at 14:27:19 +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.

Hmm. I mean, you're absolutely right about the complexity and need for
discussion. My concern is that we cannot insert the ITS block in the
memory map where it would be in an ARM GIC implementation without
changing the memory map (pushing the redistributor further down).

It is also true that simply versioning sbsa-ref does not mean we end
up with a properly aligned with ARM IP register frame layout. Some
additional logic is required for that.

If we assume that we don't want to further complicate this set by
adding the additional logic *now*, I see three options:
- Implement memory map versioning for sbsa-ref for this set, placing
  the ITS (if enabled) directly after the DIST for sbsa-ref-6.2.
- In this set, place the ITS frames in a different location relative
  to the REDIST frames than it will end up once the further logic is
  implemented.
- Drop the sbsa-ref ITS support from this set, and bring it in with
  the set implementing the additional logic.

Typing that, I'm getting the feeling that if I was the maintainer,
the third option would be my preference...

/
    Leif