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

[Julien Grall <julien@xxxxxxx>]

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 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?

>   #include <xen/mm.h>
>   #include <xen/sched.h>
>   #include <xen/acpi.h>
Cheers,

--
Julien Grall