Message ID | 20220427182934.27075-1-quic_llindhol@quicinc.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | hw/arm: add versioning to sbsa-ref machine DT | expand |
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
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
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
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);
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(+)