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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

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

Otherwise if we start adding those kinds of checks, we will have to add them in 
at least 3 places in xen code.

Cheers
Bertrand

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