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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 7 Sep 2022, at 09:37, Julien Grall <julien@xxxxxxx> wrote:
> 
> 
> 
> 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.

I think a define in compile.h using stringify is the easiest solution:

#define XEN_STR_VERSION "__stringify(XEN_VERSION)"."__stringify(XEN_SUBVERSION)”

And then change the code in the following source code to use it:
arch/arm/domain_build.c
arch/arm/acpi/domain_build.c
common/efi/boot.c

@Leo: tell me if you need help or want me to do it

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall