The sbsa-ref machine is continuously evolving. Some of the changes we
want to make in the near future, to align with real components (e.g.
the GIC-700), will break compatibility for existing firmware.

Introduce two new properties to the DT generated on machine generation:
- machine-version-major
  To be incremented when a platform change makes the machine
  incompatible with existing firmware.
- machine-version-minor
  To be incremented when functionality is added to the machine
  without causing incompatibility with existing firmware.
  to be reset to 0 when machine-version-major is incremented.

These properties are both introduced with the value 0.
(Hence, a machine where the DT is lacking these nodes is equivalent
to version 0.0.)

Signed-off-by: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Radoslaw Biernacki <rad@semihalf.com>
Cc: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/sbsa-ref.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index 2387401963..e05f6056c7 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -190,6 +190,9 @@ static void create_fdt(SBSAMachineState *sms)
     qemu_fdt_setprop_cell(fdt, "/", "#address-cells", 0x2);
     qemu_fdt_setprop_cell(fdt, "/", "#size-cells", 0x2);
 
+    qemu_fdt_setprop_cell(fdt, "/", "machine-version-major", 0);
+    qemu_fdt_setprop_cell(fdt, "/", "machine-version-minor", 0);
+
     if (ms->numa_state->have_numa_distance) {
         int size = nb_numa_nodes * nb_numa_nodes * 3 * sizeof(uint32_t);
         uint32_t *matrix = g_malloc0(size);
-- 
2.30.2

On Wed, 27 Apr 2022 at 19:29, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> The sbsa-ref machine is continuously evolving. Some of the changes we
> want to make in the near future, to align with real components (e.g.
> the GIC-700), will break compatibility for existing firmware.
>
> Introduce two new properties to the DT generated on machine generation:
> - machine-version-major
>   To be incremented when a platform change makes the machine
>   incompatible with existing firmware.
> - machine-version-minor
>   To be incremented when functionality is added to the machine
>   without causing incompatibility with existing firmware.
>   to be reset to 0 when machine-version-major is incremented.
>
> These properties are both introduced with the value 0.
> (Hence, a machine where the DT is lacking these nodes is equivalent
> to version 0.0.)
>
> Signed-off-by: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Radoslaw Biernacki <rad@semihalf.com>
> Cc: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/arm/sbsa-ref.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> index 2387401963..e05f6056c7 100644
> --- a/hw/arm/sbsa-ref.c
> +++ b/hw/arm/sbsa-ref.c
> @@ -190,6 +190,9 @@ static void create_fdt(SBSAMachineState *sms)
>      qemu_fdt_setprop_cell(fdt, "/", "#address-cells", 0x2);
>      qemu_fdt_setprop_cell(fdt, "/", "#size-cells", 0x2);
>
> +    qemu_fdt_setprop_cell(fdt, "/", "machine-version-major", 0);
> +    qemu_fdt_setprop_cell(fdt, "/", "machine-version-minor", 0);

Seems reasonable to me, but I think:
 * we should say explicitly that the hardware version defined
by these values has nothing to do with QEMU versioned machine
types (and is more akin to a hardware motherboard revision
A/B/C kind of thing), and maybe that it's not aligned with any
specification version number either?
 * we should perhaps say that on real hardware this information
would be more commonly passed to firmware as part of some
sort of revision ID register, but that for sbsa-ref we put
it in the DTB for convenience since we already use that to
tell firmware a few things about the emulated hardware
 * at least some of this should be in a comment as well as
the commit message, so nobody in future decides this should
be "tidied up" to be a versioned machine type

thanks
-- PMM

On 4/27/22 20:29, Leif Lindholm wrote:
> The sbsa-ref machine is continuously evolving. Some of the changes we
> want to make in the near future, to align with real components (e.g.
> the GIC-700), will break compatibility for existing firmware.
> 
> Introduce two new properties to the DT generated on machine generation:
> - machine-version-major
>    To be incremented when a platform change makes the machine
>    incompatible with existing firmware.
> - machine-version-minor
>    To be incremented when functionality is added to the machine
>    without causing incompatibility with existing firmware.
>    to be reset to 0 when machine-version-major is incremented.
> 
> These properties are both introduced with the value 0.
> (Hence, a machine where the DT is lacking these nodes is equivalent
> to version 0.0.)
> 
> Signed-off-by: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Radoslaw Biernacki <rad@semihalf.com>
> Cc: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/arm/sbsa-ref.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> index 2387401963..e05f6056c7 100644
> --- a/hw/arm/sbsa-ref.c
> +++ b/hw/arm/sbsa-ref.c
> @@ -190,6 +190,9 @@ static void create_fdt(SBSAMachineState *sms)
>       qemu_fdt_setprop_cell(fdt, "/", "#address-cells", 0x2);
>       qemu_fdt_setprop_cell(fdt, "/", "#size-cells", 0x2);
>   
> +    qemu_fdt_setprop_cell(fdt, "/", "machine-version-major", 0);
> +    qemu_fdt_setprop_cell(fdt, "/", "machine-version-minor", 0);


Thanks for your reply in the other email. From what I captured, the
DT aspect is not that important, but still, we could may be use some
specific 'sbsa' property names :

     qemu_fdt_setprop_cell(fdt, "/", "sbsa,version-major", 0);
     qemu_fdt_setprop_cell(fdt, "/", "sbsa,version-minor", 0);


?

C.

> +
>       if (ms->numa_state->have_numa_distance) {
>           int size = nb_numa_nodes * nb_numa_nodes * 3 * sizeof(uint32_t);
>           uint32_t *matrix = g_malloc0(size);

On Fri, Apr 29, 2022 at 09:17:09 +0200, Cédric Le Goater wrote:
> > Signed-off-by: Leif Lindholm <quic_llindhol@quicinc.com>
> > Cc: Peter Maydell <peter.maydell@linaro.org>
> > Cc: Radoslaw Biernacki <rad@semihalf.com>
> > Cc: Cédric Le Goater <clg@kaod.org>
> > ---
> >   hw/arm/sbsa-ref.c | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> > index 2387401963..e05f6056c7 100644
> > --- a/hw/arm/sbsa-ref.c
> > +++ b/hw/arm/sbsa-ref.c
> > @@ -190,6 +190,9 @@ static void create_fdt(SBSAMachineState *sms)
> >       qemu_fdt_setprop_cell(fdt, "/", "#address-cells", 0x2);
> >       qemu_fdt_setprop_cell(fdt, "/", "#size-cells", 0x2);
> > +    qemu_fdt_setprop_cell(fdt, "/", "machine-version-major", 0);
> > +    qemu_fdt_setprop_cell(fdt, "/", "machine-version-minor", 0);
> 
> 
> Thanks for your reply in the other email. From what I captured, the
> DT aspect is not that important, but still, we could may be use some
> specific 'sbsa' property names :
> 
>     qemu_fdt_setprop_cell(fdt, "/", "sbsa,version-major", 0);
>     qemu_fdt_setprop_cell(fdt, "/", "sbsa,version-minor", 0);

I'm not wedded to the names, but given that SBSA is the (now defunct)
name of a (very much not defunct) versioned specification, I think it
would add to rather than remove from the confusion if we changed to
this; it makes it look like it's declaring compliance with a version
of the spec.

Fundamentally, these properties have no meaning to anything other than
the piece of firmware that knows that it is executing on top of the
qemu sbsa-ref machine. On one level, making it look *less* like a
well-designed device tree-binding is beneficial.

Best Regards,

Leif

Hello,

On 4/27/22 20:29, Leif Lindholm wrote:
> The sbsa-ref machine is continuously evolving. Some of the changes we
> want to make in the near future, to align with real components (e.g.
> the GIC-700), will break compatibility for existing firmware.
> 
> Introduce two new properties to the DT generated on machine generation:
> - machine-version-major
>    To be incremented when a platform change makes the machine
>    incompatible with existing firmware.
> - machine-version-minor
>    To be incremented when functionality is added to the machine
>    without causing incompatibility with existing firmware.
>    to be reset to 0 when machine-version-major is incremented.
> 
> These properties are both introduced with the value 0.
> (Hence, a machine where the DT is lacking these nodes is equivalent
> to version 0.0.)

This raises a lot of questions. I am talking with my PAPR specs
experience there, which might be off topic for SBSA, but it's a
way to clarify my understanding.


If we need to introduce incompatible changes in the sbsa machine,
that would break existing firmwares, I think we should start versioning
the sbsa machine like the other do : arm/i440fx/m68k/q35/s390x/spapr
and add class attributes describing features being activated or not.
This to make sure that firmware already shipped can always be run.

Regarding the DT changes, we could also expose/advertise the new
platform features by name with property nodes. A version is OK but
literal names are more explicit and we might want to de/activate
features one by one for test purposes or for some other reason.

Also, if some reconfiguration of the platform is needed to activate
a new feature, software (firmware or OS) needs a way (a trap or
something) to negotiate with the platform what should be done.
I might be anticipating a bit too much but pSeries has such needs
for the MMU and interrupt modes.

What about the SBSA specs ? Do they need a change ? It is true that
there are a bit vague regarding the DT, only referencing the Arm
Base Boot Requirements document which references the Device Tree
specs v0.3. I didn't find anything about versioning.

Thanks,

C.


> Signed-off-by: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Radoslaw Biernacki <rad@semihalf.com>
> Cc: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/arm/sbsa-ref.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> index 2387401963..e05f6056c7 100644
> --- a/hw/arm/sbsa-ref.c
> +++ b/hw/arm/sbsa-ref.c
> @@ -190,6 +190,9 @@ static void create_fdt(SBSAMachineState *sms)
>       qemu_fdt_setprop_cell(fdt, "/", "#address-cells", 0x2);
>       qemu_fdt_setprop_cell(fdt, "/", "#size-cells", 0x2);
>   
> +    qemu_fdt_setprop_cell(fdt, "/", "machine-version-major", 0);
> +    qemu_fdt_setprop_cell(fdt, "/", "machine-version-minor", 0);
> +
>       if (ms->numa_state->have_numa_distance) {
>           int size = nb_numa_nodes * nb_numa_nodes * 3 * sizeof(uint32_t);
>           uint32_t *matrix = g_malloc0(size);

Hi Cedric,

On Thu, Apr 28, 2022 at 10:55:54 +0200, Cédric Le Goater wrote:
> > The sbsa-ref machine is continuously evolving. Some of the changes we
> > want to make in the near future, to align with real components (e.g.
> > the GIC-700), will break compatibility for existing firmware.
> > 
> > Introduce two new properties to the DT generated on machine generation:
> > - machine-version-major
> >    To be incremented when a platform change makes the machine
> >    incompatible with existing firmware.
> > - machine-version-minor
> >    To be incremented when functionality is added to the machine
> >    without causing incompatibility with existing firmware.
> >    to be reset to 0 when machine-version-major is incremented.
> > 
> > These properties are both introduced with the value 0.
> > (Hence, a machine where the DT is lacking these nodes is equivalent
> > to version 0.0.)
> 
> This raises a lot of questions. I am talking with my PAPR specs
> experience there, which might be off topic for SBSA, but it's a
> way to clarify my understanding.

That is a reasonable assumption: you may expect the ARM platform
specifications to usefully describe a general platform which sofware
can be developed against. However, they are not. They describe the
absolute minimum that it is even theoretically possible to develop
portable software against.

> If we need to introduce incompatible changes in the sbsa machine,
> that would break existing firmwares, I think we should start versioning
> the sbsa machine like the other do : arm/i440fx/m68k/q35/s390x/spapr
> and add class attributes describing features being activated or not.
> This to make sure that firmware already shipped can always be run.

The versioning I'm introducing here is a separate one from the SBSA
numbering. See this thread for some history:

https://lore.kernel.org/qemu-devel/20211015122351.vc55mwzjbevl6wjy@leviathan/

Without derailing this thread too much, I will add that trying to
align a machine against specific SBSA versions is tricky, since ARM do
not put enough resource into QEMU development to keep up with
architectural feature support. ARM's strategy for that sort of thing
relies on proprietary software models.

It *would* be possible to retroactively add support for older
versions as the qemu feature set catches up, but that would be a
separate versioning scheme, mostly orhogonal with this one.
If ARM *did* start shifting to a focus on getting QEMU enablement done
in sync with architectural evolution, the versioning scheme would
remain mostly orthogonal.

> Regarding the DT changes, we could also expose/advertise the new
> platform features by name with property nodes.

That would defeat the purpose of this platform, which is to serve as a
realistic target for developing SBBR firmware against. That we used a
DT at all to communicate information about the machine configuration
was in hindsight possibly a mistake, since it frequently leads to this
misunderstanding - but I opted for it in order to avoid inventing a
new data encapsulation format *only* for avoiding this topic popping
up at a regular cadence.

> What about the SBSA specs ? Do they need a change ? It is true that
> there are a bit vague regarding the DT, only referencing the Arm

DT is not relevant for ServerReady SR (which is what SBSA got renamed
to). There are embedded profiles defined by the ServerReady documents
that mention DT. Support for those, if anyone is interested in
creating/maintaining them, would be handled by a separate machine type.

Regards,

Leif