| Message ID | 20220907120420.387771-1-leo.yan@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: Add macro for version number string | expand |
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@arm.com> > Signed-off-by: Leo Yan <leo.yan@linaro.org> Reviewed-by: Jan Beulich <jbeulich@suse.com> 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 ... > --- 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 ... > --- 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:. Jan
Hi Leo, Thanks a lot for the quick handling here. > On 7 Sep 2022, at 13:04, Leo Yan <leo.yan@linaro.org> 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@arm.com> > Signed-off-by: Leo Yan <leo.yan@linaro.org> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> Regarding the change suggested by Jan to add spaces, I think it is a good idea so if the commiter agrees to do on it on commit please do, otherwise we can keep this as is. Cheers Bertrand > --- > xen/arch/arm/acpi/domain_build.c | 3 ++- > xen/arch/arm/domain_build.c | 2 +- > xen/common/efi/boot.c | 4 ++-- > xen/include/xen/compile.h.in | 1 + > 4 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/xen/arch/arm/acpi/domain_build.c b/xen/arch/arm/acpi/domain_build.c > index bbdc90f92c..b23c7cad7a 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> > @@ -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" > "xen,xen"; > int res; > /* Convenience alias */ > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 3fd1186b53..62602d2b86 100644 > --- 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" > "xen,xen"; > __be32 *reg, *cells; > gic_interrupt_t intr; > diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c > index a5b2d6ddb8..db0340c8e2 100644 > --- 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"); > > efi_arch_relocate_image(0); > > diff --git a/xen/include/xen/compile.h.in b/xen/include/xen/compile.h.in > index 440ecb25c1..3151d1e7d1 100644 > --- a/xen/include/xen/compile.h.in > +++ b/xen/include/xen/compile.h.in > @@ -7,6 +7,7 @@ > > #define XEN_VERSION @@version@@ > #define XEN_SUBVERSION @@subversion@@ > +#define XEN_VERSION_STRING "@@version@@.@@subversion@@" > #define XEN_EXTRAVERSION "@@extraversion@@" > > #define XEN_CHANGESET "@@changeset@@" > -- > 2.34.1 >
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@arm.com> > > Signed-off-by: Leo Yan <leo.yan@linaro.org> > > Reviewed-by: Jan Beulich <jbeulich@suse.com> > 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
On 07.09.2022 14:28, Leo Yan wrote: > 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+ That's up to you, I would say. We don't really use that tag all that much, the Fixes: tag is more relevant at least based on recent observations. Jan
On Wed, Sep 07, 2022 at 02:31:52PM +0200, Jan Beulich wrote: > On 07.09.2022 14:28, Leo Yan wrote: > > 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+ > > That's up to you, I would say. We don't really use that tag all that much, > the Fixes: tag is more relevant at least based on recent observations. Thanks a lot for confirmation. If so, I will just use Fixes tag. Leo
On 07.09.2022 14:20, Bertrand Marquis wrote: > Hi Leo, > > Thanks a lot for the quick handling here. > >> On 7 Sep 2022, at 13:04, Leo Yan <leo.yan@linaro.org> 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@arm.com> >> Signed-off-by: Leo Yan <leo.yan@linaro.org> > Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> > > Regarding the change suggested by Jan to add spaces, I think it is a > good idea so if the commiter agrees to do on it on commit please do, > otherwise we can keep this as is. If I end up committing this, I'd be happy to add the blanks, and therefore I'm inclined to say no need for a re-send. Jan
On Wed, Sep 07, 2022 at 02:34:25PM +0200, Jan Beulich wrote: > On 07.09.2022 14:20, Bertrand Marquis wrote: > > Hi Leo, > > > > Thanks a lot for the quick handling here. > > > >> On 7 Sep 2022, at 13:04, Leo Yan <leo.yan@linaro.org> 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@arm.com> > >> Signed-off-by: Leo Yan <leo.yan@linaro.org> > > Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> > > > > Regarding the change suggested by Jan to add spaces, I think it is a > > good idea so if the commiter agrees to do on it on commit please do, > > otherwise we can keep this as is. > > If I end up committing this, I'd be happy to add the blanks, and therefore > I'm inclined to say no need for a re-send. For easier for maintainers, have sent patch v2 with adding blank and adding review tags. Thanks! Leo
diff --git a/xen/arch/arm/acpi/domain_build.c b/xen/arch/arm/acpi/domain_build.c index bbdc90f92c..b23c7cad7a 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> @@ -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" "xen,xen"; int res; /* Convenience alias */ diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 3fd1186b53..62602d2b86 100644 --- 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" "xen,xen"; __be32 *reg, *cells; gic_interrupt_t intr; diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index a5b2d6ddb8..db0340c8e2 100644 --- 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"); efi_arch_relocate_image(0); diff --git a/xen/include/xen/compile.h.in b/xen/include/xen/compile.h.in index 440ecb25c1..3151d1e7d1 100644 --- a/xen/include/xen/compile.h.in +++ b/xen/include/xen/compile.h.in @@ -7,6 +7,7 @@ #define XEN_VERSION @@version@@ #define XEN_SUBVERSION @@subversion@@ +#define XEN_VERSION_STRING "@@version@@.@@subversion@@" #define XEN_EXTRAVERSION "@@extraversion@@" #define XEN_CHANGESET "@@changeset@@"
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@arm.com> Signed-off-by: Leo Yan <leo.yan@linaro.org> --- xen/arch/arm/acpi/domain_build.c | 3 ++- xen/arch/arm/domain_build.c | 2 +- xen/common/efi/boot.c | 4 ++-- xen/include/xen/compile.h.in | 1 + 4 files changed, 6 insertions(+), 4 deletions(-)