Re: [PATCH] xen/arm: acpi: Include header file for version number

[Julien Grall <julien@xxxxxxx>]

On 07/09/2022 09:30, Bertrand Marquis wrote:
> > On 7 Sep 2022, at 09:26, Julien Grall <julien@xxxxxxx> wrote:
> >
> > Hi Leo,
> >
> > On 06/09/2022 12:31, Leo Yan wrote:
> > > On Arm64 Linux kernel prints log for Xen version number:
> > >    [    0.000000] Xen XEN_VERSION.XEN_SUBVERSION support found
> > > Because the header file "xen/compile.h" is missed, XEN_VERSION and
> > > XEN_SUBVERSION are not defined, thus compiler directly uses the
> > > string "XEN_VERSION" and "XEN_SUBVERSION" in the compatible string.
> > > This patch includes the header "xen/compile.h" which defines macros for
> > > XEN_VERSION and XEN_SUBVERSION, thus Xen can pass the version number via
> > > hypervisor node.
> > > Signed-off-by: Leo Yan <leo.yan@xxxxxxxxxx>
> > AFAICT, the problem was introduced when we split the ACPI code from 
> > arch/domain_build.c. So I would add the following tag:
> >
> > Fixes: 5d797ee199b3 ("xen/arm: split domain_build.c")
> >
> > Now, this means we shipped Xen for ~4 years with the wrong compatible. The 
> > compatible is meant to indicate the Xen version. However, I don't think this is 
> > used for anything other than printing the version on the console.
> >
> > Also, the problem occurs only when using ACPI. This is still in tech preview, 
> > so I think we don't need to document the issue in the documentation (we can 
> > easily break ABI).
> >
> > > ---
> > >   xen/arch/arm/acpi/domain_build.c | 1 +
> > >   1 file changed, 1 insertion(+)
> > > diff --git a/xen/arch/arm/acpi/domain_build.c b/xen/arch/arm/acpi/domain_build.c
> > > index bbdc90f92c..2649e11fd4 100644
> > > --- a/xen/arch/arm/acpi/domain_build.c
> > > +++ b/xen/arch/arm/acpi/domain_build.c
> > > @@ -9,6 +9,7 @@
> > >    * GNU General Public License for more details.
> > >    */
> > >   +#include <xen/compile.h>
> > So this is fixing the immediate problem. Given the subtlety of the bug, I think 
> > it would be good to also harden the code at the same time.
> I think we should commit the patch as is and harden the code in a subsequent 
> patch.
I thought about this. However, if we do the hardening in the same patch, 
then it makes a lot easier to confirm that the patch works when ingested 
in downstream code or backported.
> > I can see two way to do that:
> >   1) Dropping the use of __stringify
> >   2) Check if XEN_VERSION and XEN_SUBVERSION are defined
> >
> > The latter is probably lightweight. This could be added right next to 
> > acpi_make_hypervisor_node() for clarify.
> >
> > Something like:
> >
> > #ifndef XEN_VERSION
> > # error "XEN_VERSION is not defined"
> > #endif
> >
> > #ifndef XEN_SUBVERSION
> > # error "XEN_SUBVERSION is not defined"
> > #endif
> >
> > Could you have a look?
> There are actually several places in the code where we use the stringify system.
> Would it make sense to actually have a string version provided in compile.h and 
> use it instead ?
I think so.

> Otherwise if we start adding those kinds of checks, we will have to add them in 
> at least 3 places in xen code.
The solution I proposed above is easy to implement right now. My gut 
feeling is tweaking __stringify (or else) will take a bit more time.
If you (or Leo) can come up with a solution quickly then fine. 
Otherwise, I think we still want some hardening for backporting purpose.
Cheers,

--
Julien Grall