Re: [PATCH] xen: Add macro for version number string

[Leo Yan <leo.yan@xxxxxxxxxx>]

Hi Jan,

On Wed, Sep 07, 2022 at 02:12:25PM +0200, Jan Beulich wrote:
> On 07.09.2022 14:04, Leo Yan wrote:
> > On Arm64 Linux kernel prints log for Xen version number:
> > 
> >   Xen XEN_VERSION.XEN_SUBVERSION support found
> > 
> > The header file "xen/compile.h" is missed so that XEN_VERSION and
> > XEN_SUBVERSION are not defined, __stringify() wrongly converts them as
> > strings and concatenate to string "XEN_VERSION.XEN_SUBVERSION".
> > 
> > This patch introduces a string macro XEN_VERSION_STRING, we can directly
> > use it as version number string, as a result it drops to use of
> > __stringify() to make the code more readable.
> > 
> > The change has been tested on Ampere AVA Arm64 platform.
> > 
> > Fixes: 5d797ee199b3 ("xen/arm: split domain_build.c")
> > Suggested-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> > Signed-off-by: Leo Yan <leo.yan@xxxxxxxxxx>
> 
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> with perhaps a small adjustment (but it'll be the Arm maintainers to judge):
> 
> > @@ -91,7 +92,7 @@ static int __init acpi_make_hypervisor_node(const struct 
> > kernel_info *kinfo,
> >                                              struct membank tbl_add[])
> >  {
> >      const char compat[] =
> > -        
> > "xen,xen-"__stringify(XEN_VERSION)"."__stringify(XEN_SUBVERSION)"\0"
> > +        "xen,xen-"XEN_VERSION_STRING"\0"
> 
> I think readability would benefit here from adding blanks around
> XEN_VERSION_STRING here and ...

Agree that adding blanks is better.  Will do.

> 
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -1367,7 +1367,7 @@ static int __init make_hypervisor_node(struct domain 
> > *d,
> >                                         int addrcells, int sizecells)
> >  {
> >      const char compat[] =
> > -        
> > "xen,xen-"__stringify(XEN_VERSION)"."__stringify(XEN_SUBVERSION)"\0"
> > +        "xen,xen-"XEN_VERSION_STRING"\0"
> 
> ... here (as an aside I wonder why these variables aren't static
> __initconst), just like ...

Will add blanks.

> > --- a/xen/common/efi/boot.c
> > +++ b/xen/common/efi/boot.c
> > @@ -1341,8 +1341,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
> > *SystemTable)
> >              efi_console_set_mode();
> >      }
> >  
> > -    PrintStr(L"Xen " __stringify(XEN_VERSION) "." 
> > __stringify(XEN_SUBVERSION)
> > -             XEN_EXTRAVERSION " (c/s " XEN_CHANGESET ") EFI loader\r\n");
> > +    PrintStr(L"Xen " XEN_VERSION_STRING XEN_EXTRAVERSION
> > +        " (c/s " XEN_CHANGESET ") EFI loader\r\n");
> 
> ... it is here in particular for XEN_CHANGESET.
> 
> The other general remark I have: Please follow patch submission guidelines
> and send To: the list with maintainers on Cc:.

Ah, just now quickly went through docs/process/sending-patches.pandoc,
thanks for reminding.

A question, since commit 5d797ee199b3 was merged in 4.11.0-rc6, for
fixing it, should I explictly add backport tag as below?

  Backport: 4.11+

Thanks,
Leo