xen/arm: acpi: Include header file for version number

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

Posted by Leo Yan 1 year, 7 months ago

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@linaro.org>
---
 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>
 #include <xen/mm.h>
 #include <xen/sched.h>
 #include <xen/acpi.h>
-- 
2.34.1

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

Posted by Julien Grall 1 year, 7 months ago

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@linaro.org>

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

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

Posted by Bertrand Marquis 1 year, 7 months ago

Hi Julien,

> On 7 Sep 2022, at 09:26, Julien Grall <julien@xen.org> 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@linaro.org>
> 
> 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

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

Posted by Julien Grall 1 year, 7 months ago


On 07/09/2022 09:30, Bertrand Marquis wrote:
>> On 7 Sep 2022, at 09:26, Julien Grall <julien@xen.org> 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@linaro.org>
>>
>> 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

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

Posted by Bertrand Marquis 1 year, 7 months ago

Hi Julien,

> On 7 Sep 2022, at 09:37, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 07/09/2022 09:30, Bertrand Marquis wrote:
>>> On 7 Sep 2022, at 09:26, Julien Grall <julien@xen.org> 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@linaro.org>
>>> 
>>> 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

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

Posted by Julien Grall 1 year, 7 months ago


On 07/09/2022 09:53, Bertrand Marquis wrote:
>>> 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:

Ah! I thought you were suggesting to tweak __stringify. This is ...
> 
> #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

... much better.

Cheers,

-- 
Julien Grall

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

Posted by Bertrand Marquis 1 year, 7 months ago


> On 7 Sep 2022, at 09:55, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 07/09/2022 09:53, Bertrand Marquis wrote:
>>>> 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:
> 
> Ah! I thought you were suggesting to tweak __stringify. This is ...

Also possible but a bit more tricky

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

Quotes at beginning and end should not be there.

>> 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
> 
> ... much better.

Thanks :-)

Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall

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

Posted by Jan Beulich 1 year, 7 months ago

On 07.09.2022 10:58, Bertrand Marquis wrote:
>> On 7 Sep 2022, at 09:55, Julien Grall <julien@xen.org> wrote:
>> On 07/09/2022 09:53, Bertrand Marquis wrote:
>>>>> 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:
>>
>> Ah! I thought you were suggesting to tweak __stringify. This is ...
> 
> Also possible but a bit more tricky
> 
>>> #define XEN_STR_VERSION "__stringify(XEN_VERSION)"."__stringify(XEN_SUBVERSION)”
> 
> Quotes at beginning and end should not be there.

I have to admit that I dislike the STR infix. I'd prefer a suffixed variant
(e.g. XEN_VERSION_STRING) or one omitting "string" altogether, e.g.
XEN_FULL_VERSION (albeit I see "full" as being potentially ambiguous here,
since one might expect that to include XEN_EXTRAVERSION as well then).

Jan

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

Posted by Bertrand Marquis 1 year, 7 months ago

Hi Jan,

> On 7 Sep 2022, at 10:14, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 07.09.2022 10:58, Bertrand Marquis wrote:
>>> On 7 Sep 2022, at 09:55, Julien Grall <julien@xen.org> wrote:
>>> On 07/09/2022 09:53, Bertrand Marquis wrote:
>>>>>> 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:
>>> 
>>> Ah! I thought you were suggesting to tweak __stringify. This is ...
>> 
>> Also possible but a bit more tricky
>> 
>>>> #define XEN_STR_VERSION "__stringify(XEN_VERSION)"."__stringify(XEN_SUBVERSION)”
>> 
>> Quotes at beginning and end should not be there.
> 
> I have to admit that I dislike the STR infix. I'd prefer a suffixed variant
> (e.g. XEN_VERSION_STRING) or one omitting "string" altogether, e.g.
> XEN_FULL_VERSION (albeit I see "full" as being potentially ambiguous here,
> since one might expect that to include XEN_EXTRAVERSION as well then).


Version is a value so here I though it made sense to distinguish that one as it is a string representation of it.

XEN_VERSION_STRING is ok I think.

I generally dislike anything named FULL, EXTRA, BASE or other which are just unclear.

Bertrand

> 
> Jan

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

Posted by Leo Yan 1 year, 7 months ago

On Wed, Sep 07, 2022 at 10:05:54AM +0000, Bertrand Marquis wrote:

[...]

> >>>> I think a define in compile.h using stringify is the easiest solution:
> >>> 
> >>> Ah! I thought you were suggesting to tweak __stringify. This is ...
> >> 
> >> Also possible but a bit more tricky
> >> 
> >>>> #define XEN_STR_VERSION "__stringify(XEN_VERSION)"."__stringify(XEN_SUBVERSION)”

Just remind, We need to define XEN_VERSION_STRING in compile.h.in rather
than in compile.h, something like:

  #define XEN_VERSION_STRING @@version@@.@@subversion@@

> >> Quotes at beginning and end should not be there.
> > 
> > I have to admit that I dislike the STR infix. I'd prefer a suffixed variant
> > (e.g. XEN_VERSION_STRING) or one omitting "string" altogether, e.g.
> > XEN_FULL_VERSION (albeit I see "full" as being potentially ambiguous here,
> > since one might expect that to include XEN_EXTRAVERSION as well then).
> 
> 
> Version is a value so here I though it made sense to distinguish that one as it is a string representation of it.
> 
> XEN_VERSION_STRING is ok I think.
> 
> I generally dislike anything named FULL, EXTRA, BASE or other which are just unclear.

XEN_VERSION_STRING is good for me.

Hi Bertrand, just let me know if you prefer to cook your own patch for
this (essentially this idea is coming from you) or you want me to
follow up for a new patch?  Either way is fine for me.

Thanks,
Leo

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

Posted by Bertrand Marquis 1 year, 7 months ago

Hi Leo,

> On 7 Sep 2022, at 12:08, Leo Yan <leo.yan@linaro.org> wrote:
> 
> On Wed, Sep 07, 2022 at 10:05:54AM +0000, Bertrand Marquis wrote:
> 
> [...]
> 
>>>>>> I think a define in compile.h using stringify is the easiest solution:
>>>>> 
>>>>> Ah! I thought you were suggesting to tweak __stringify. This is ...
>>>> 
>>>> Also possible but a bit more tricky
>>>> 
>>>>>> #define XEN_STR_VERSION "__stringify(XEN_VERSION)"."__stringify(XEN_SUBVERSION)”
> 
> Just remind, We need to define XEN_VERSION_STRING in compile.h.in rather
> than in compile.h, something like:
> 
>  #define XEN_VERSION_STRING @@version@@.@@subversion@@

Very true but you will need the quotes here

> 
>>>> Quotes at beginning and end should not be there.
>>> 
>>> I have to admit that I dislike the STR infix. I'd prefer a suffixed variant
>>> (e.g. XEN_VERSION_STRING) or one omitting "string" altogether, e.g.
>>> XEN_FULL_VERSION (albeit I see "full" as being potentially ambiguous here,
>>> since one might expect that to include XEN_EXTRAVERSION as well then).
>> 
>> 
>> Version is a value so here I though it made sense to distinguish that one as it is a string representation of it.
>> 
>> XEN_VERSION_STRING is ok I think.
>> 
>> I generally dislike anything named FULL, EXTRA, BASE or other which are just unclear.
> 
> XEN_VERSION_STRING is good for me.
> 
> Hi Bertrand, just let me know if you prefer to cook your own patch for
> this (essentially this idea is coming from you) or you want me to
> follow up for a new patch?  Either way is fine for me.

Please push a new patch and add:
Suggested-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> 
> Thanks,
> Leo

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

Posted by Leo Yan 1 year, 7 months ago

On Wed, Sep 07, 2022 at 11:12:24AM +0000, Bertrand Marquis wrote:

[...]

> > Just remind, We need to define XEN_VERSION_STRING in compile.h.in rather
> > than in compile.h, something like:
> > 
> >  #define XEN_VERSION_STRING @@version@@.@@subversion@@
> 
> Very true but you will need the quotes here

Yeah.

> >>>> Quotes at beginning and end should not be there.
> >>> 
> >>> I have to admit that I dislike the STR infix. I'd prefer a suffixed variant
> >>> (e.g. XEN_VERSION_STRING) or one omitting "string" altogether, e.g.
> >>> XEN_FULL_VERSION (albeit I see "full" as being potentially ambiguous here,
> >>> since one might expect that to include XEN_EXTRAVERSION as well then).
> >> 
> >> 
> >> Version is a value so here I though it made sense to distinguish that one as it is a string representation of it.
> >> 
> >> XEN_VERSION_STRING is ok I think.
> >> 
> >> I generally dislike anything named FULL, EXTRA, BASE or other which are just unclear.
> > 
> > XEN_VERSION_STRING is good for me.
> > 
> > Hi Bertrand, just let me know if you prefer to cook your own patch for
> > this (essentially this idea is coming from you) or you want me to
> > follow up for a new patch?  Either way is fine for me.
> 
> Please push a new patch and add:
> Suggested-by: Bertrand Marquis <bertrand.marquis@arm.com>

Sure, will do.

Thanks all for suggestions.

Leo

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

Posted by Bertrand Marquis 1 year, 7 months ago

Hi Leo,

> On 6 Sep 2022, at 12:31, Leo Yan <leo.yan@linaro.org> 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@linaro.org>

Very nice finding and side effect from stringify.

Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> ---
> 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>
> #include <xen/mm.h>
> #include <xen/sched.h>
> #include <xen/acpi.h>
> -- 
> 2.34.1
>

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

Posted by Leo Yan 1 year, 7 months ago

On Tue, Sep 06, 2022 at 01:09:28PM +0000, Bertrand Marquis wrote:
> Hi Leo,
> 
> > On 6 Sep 2022, at 12:31, Leo Yan <leo.yan@linaro.org> 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@linaro.org>
> 
> Very nice finding and side effect from stringify.
> 
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Thanks for review, Bertrand.