Re: [PATCH] hw/arm: add versioning to sbsa-ref machine DT

[Peter Maydell]

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