On Thu, 2021-08-19 at 14:27 +0100, Peter Maydell wrote:
> On Thu, 12 Aug 2021 at 17:53, Shashi Mallela <
> shashi.mall...@linaro.org> wrote:
> > Included creation of ITS as part of SBSA platform GIC
> > initialization.
> >
> > Signed-off-by: Shashi Mallela <shashi.mall...@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.
