xen: Drop the (almost) unused extern start[]

[PATCH] xen: Drop the (almost) unused extern start[]

Posted by Andrew Cooper 2 years, 6 months ago

This global variable is shadowed by plenty local variables, violating MISRA
rule 5.3.  Some architectures happen to have a symbol by the name of start in
their head.S's, but it's not a useful symbol to reference from C.

In fact, the single use of the global start[] in RISC-V is wrong and means to
use _start[] as the linker symbol at the beginning of the .text section, not
the function which happens to be in the same location.

Fix RISC-V to use the right symbol for it's calculation, and drop the extern.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Bob Eshleman <bobbyeshleman@gmail.com>
CC: Alistair Francis <alistair.francis@wdc.com>
CC: Connor Davis <connojdavis@gmail.com>
CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
CC: Roberto Bagnara <roberto.bagnara@bugseng.com>

I'm expecting:

  https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/945105609

to come back and show green, but Gitlab is very backed up right now.  I've
compiled each architecture locally.
---
 xen/arch/riscv/mm.c      | 2 +-
 xen/include/xen/kernel.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
index fddb3cd0bdeb..99ed5e9623cc 100644
--- a/xen/arch/riscv/mm.c
+++ b/xen/arch/riscv/mm.c
@@ -148,7 +148,7 @@ static bool __init check_pgtbl_mode_support(struct mmu_desc *mmu_desc,
 
     unsigned long aligned_load_start = load_start & level_map_mask;
     unsigned long aligned_page_size = XEN_PT_LEVEL_SIZE(page_table_level);
-    unsigned long xen_size = (unsigned long)(_end - start);
+    unsigned long xen_size = (unsigned long)(_end - _start);
 
     if ( (load_start + xen_size) > (aligned_load_start + aligned_page_size) )
     {
diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h
index 9ac2694dc7b1..46b3c9c02625 100644
--- a/xen/include/xen/kernel.h
+++ b/xen/include/xen/kernel.h
@@ -66,7 +66,7 @@
 })
 
 /* SAF-0-safe */
-extern char _start[], _end[], start[];
+extern char _start[], _end[];
 #define is_kernel(p) ({                         \
     char *__p = (char *)(unsigned long)(p);     \
     (__p >= _start) && (__p < _end);            \

base-commit: 0b1171be87698bc7d14760383c0770aeb6e41dd4
-- 
2.30.2

Re: [PATCH] xen: Drop the (almost) unused extern start[]

Posted by Oleksii 2 years, 6 months ago

On Tue, 2023-07-25 at 19:20 +0100, Andrew Cooper wrote:
> This global variable is shadowed by plenty local variables, violating
> MISRA
> rule 5.3.  Some architectures happen to have a symbol by the name of
> start in
> their head.S's, but it's not a useful symbol to reference from C.
> 
> In fact, the single use of the global start[] in RISC-V is wrong and
> means to
> use _start[] as the linker symbol at the beginning of the .text
> section, not
> the function which happens to be in the same location.
> 
> Fix RISC-V to use the right symbol for it's calculation, and drop the
> extern.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Really missed '_start'.

Reviewed-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis <bertrand.marquis@arm.com>
> CC: Bob Eshleman <bobbyeshleman@gmail.com>
> CC: Alistair Francis <alistair.francis@wdc.com>
> CC: Connor Davis <connojdavis@gmail.com>
> CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> CC: Roberto Bagnara <roberto.bagnara@bugseng.com>
> 
> I'm expecting:
> 
>  
> https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/945105609
> 
> to come back and show green, but Gitlab is very backed up right now. 
> I've
> compiled each architecture locally.
> ---
>  xen/arch/riscv/mm.c      | 2 +-
>  xen/include/xen/kernel.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
> index fddb3cd0bdeb..99ed5e9623cc 100644
> --- a/xen/arch/riscv/mm.c
> +++ b/xen/arch/riscv/mm.c
> @@ -148,7 +148,7 @@ static bool __init
> check_pgtbl_mode_support(struct mmu_desc *mmu_desc,
>  
>      unsigned long aligned_load_start = load_start & level_map_mask;
>      unsigned long aligned_page_size =
> XEN_PT_LEVEL_SIZE(page_table_level);
> -    unsigned long xen_size = (unsigned long)(_end - start);
> +    unsigned long xen_size = (unsigned long)(_end - _start);
>  
>      if ( (load_start + xen_size) > (aligned_load_start +
> aligned_page_size) )
>      {
> diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h
> index 9ac2694dc7b1..46b3c9c02625 100644
> --- a/xen/include/xen/kernel.h
> +++ b/xen/include/xen/kernel.h
> @@ -66,7 +66,7 @@
>  })
>  
>  /* SAF-0-safe */
> -extern char _start[], _end[], start[];
> +extern char _start[], _end[];
>  #define is_kernel(p) ({                         \
>      char *__p = (char *)(unsigned long)(p);     \
>      (__p >= _start) && (__p < _end);            \
> 
> base-commit: 0b1171be87698bc7d14760383c0770aeb6e41dd4

Re: [PATCH] xen: Drop the (almost) unused extern start[]

Posted by Stefano Stabellini 2 years, 6 months ago

On Tue, 25 Jul 2023, Andrew Cooper wrote:
> This global variable is shadowed by plenty local variables, violating MISRA
> rule 5.3.  Some architectures happen to have a symbol by the name of start in
> their head.S's, but it's not a useful symbol to reference from C.
> 
> In fact, the single use of the global start[] in RISC-V is wrong and means to
> use _start[] as the linker symbol at the beginning of the .text section, not
> the function which happens to be in the same location.
> 
> Fix RISC-V to use the right symbol for it's calculation, and drop the extern.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis <bertrand.marquis@arm.com>
> CC: Bob Eshleman <bobbyeshleman@gmail.com>
> CC: Alistair Francis <alistair.francis@wdc.com>
> CC: Connor Davis <connojdavis@gmail.com>
> CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> CC: Roberto Bagnara <roberto.bagnara@bugseng.com>
> 
> I'm expecting:
> 
>   https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/945105609
> 
> to come back and show green, but Gitlab is very backed up right now.  I've
> compiled each architecture locally.
> ---
>  xen/arch/riscv/mm.c      | 2 +-
>  xen/include/xen/kernel.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
> index fddb3cd0bdeb..99ed5e9623cc 100644
> --- a/xen/arch/riscv/mm.c
> +++ b/xen/arch/riscv/mm.c
> @@ -148,7 +148,7 @@ static bool __init check_pgtbl_mode_support(struct mmu_desc *mmu_desc,
>  
>      unsigned long aligned_load_start = load_start & level_map_mask;
>      unsigned long aligned_page_size = XEN_PT_LEVEL_SIZE(page_table_level);
> -    unsigned long xen_size = (unsigned long)(_end - start);
> +    unsigned long xen_size = (unsigned long)(_end - _start);
>  
>      if ( (load_start + xen_size) > (aligned_load_start + aligned_page_size) )
>      {
> diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h
> index 9ac2694dc7b1..46b3c9c02625 100644
> --- a/xen/include/xen/kernel.h
> +++ b/xen/include/xen/kernel.h
> @@ -66,7 +66,7 @@
>  })
>  
>  /* SAF-0-safe */
> -extern char _start[], _end[], start[];
> +extern char _start[], _end[];
>  #define is_kernel(p) ({                         \
>      char *__p = (char *)(unsigned long)(p);     \
>      (__p >= _start) && (__p < _end);            \
> 
> base-commit: 0b1171be87698bc7d14760383c0770aeb6e41dd4
> -- 
> 2.30.2
>