[XEN PATCH 0/7] address some violations of MISRA C Rule 8.4

[XEN PATCH 0/7] address some violations of MISRA C Rule 8.4

[Nicola Vetrini]

Hi all,

this series addresses some of the remaining violations of MISRA C:2012 Rule 8.4.

Some of the modifications are done according to the feedback received in this
thread [1]


Nicola Vetrini (7):
  xen/arm: mmu: add headers for missing declarations
  x86/i8259: add missing header for init_IRQ declaration
  xen/x86: add missing instances of asmlinkage attributes
  x86/viridian: make build_assertions static
  docs/misra: add entry to exclude-list.json
  xen/x86: remove stale comment
  xen/page_alloc: deviate first_valid_mfn for MISRA C Rule 8.4

 docs/misra/exclude-list.json         | 4 ++++
 xen/arch/arm/mmu/setup.c             | 1 +
 xen/arch/arm/mmu/smpboot.c           | 2 ++
 xen/arch/x86/cpu/mcheck/mce.c        | 2 +-
 xen/arch/x86/desc.c                  | 2 +-
 xen/arch/x86/efi/efi-boot.h          | 5 +++--
 xen/arch/x86/hvm/viridian/synic.c    | 2 +-
 xen/arch/x86/i8259.c                 | 1 +
 xen/arch/x86/include/asm/hypercall.h | 2 +-
 xen/arch/x86/mm.c                    | 2 +-
 xen/arch/x86/smpboot.c               | 2 +-
 xen/common/page_alloc.c              | 1 +
 12 files changed, 18 insertions(+), 8 deletions(-)

-- 
2.34.1

[XEN PATCH 1/7] xen/arm: mmu: add headers for missing declarations

[Nicola Vetrini]

The definitions needing the inclusion of asm/setup.h are
boot_{first,second,third}(_id)?, whereas vmap.h is needed by arch_vmap_virt_end.

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/arm/mmu/setup.c   | 1 +
 xen/arch/arm/mmu/smpboot.c | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/xen/arch/arm/mmu/setup.c b/xen/arch/arm/mmu/setup.c
index a5a9b538ff13..d5264e51bc44 100644
--- a/xen/arch/arm/mmu/setup.c
+++ b/xen/arch/arm/mmu/setup.c
@@ -8,6 +8,7 @@
 #include <xen/init.h>
 #include <xen/libfdt/libfdt.h>
 #include <xen/sizes.h>
+#include <xen/vmap.h>
 
 #include <asm/fixmap.h>
 
diff --git a/xen/arch/arm/mmu/smpboot.c b/xen/arch/arm/mmu/smpboot.c
index 12f1a5d761e7..b6fc0aae07f1 100644
--- a/xen/arch/arm/mmu/smpboot.c
+++ b/xen/arch/arm/mmu/smpboot.c
@@ -7,6 +7,8 @@
 
 #include <xen/domain_page.h>
 
+#include <asm/setup.h>
+
 /*
  * Static start-of-day pagetables that we use before the allocators
  * are up. These are used by all CPUs during bringup before switching
-- 
2.34.1

[XEN PATCH 2/7] x86/i8259: add missing header for init_IRQ declaration

[Nicola Vetrini]

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/i8259.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/x86/i8259.c b/xen/arch/x86/i8259.c
index e0fa1f96b4f2..470d690c3594 100644
--- a/xen/arch/x86/i8259.c
+++ b/xen/arch/x86/i8259.c
@@ -19,6 +19,7 @@
 #include <xen/delay.h>
 #include <asm/apic.h>
 #include <asm/asm_defns.h>
+#include <asm/setup.h>
 #include <io_ports.h>
 #include <irq_vectors.h>
 
-- 
2.34.1

[XEN PATCH 3/7] xen/x86: add missing instances of asmlinkage attributes

[Nicola Vetrini]

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/desc.c         | 2 +-
 xen/arch/x86/efi/efi-boot.h | 5 +++--
 xen/arch/x86/mm.c           | 2 +-
 xen/arch/x86/smpboot.c      | 2 +-
 4 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/desc.c b/xen/arch/x86/desc.c
index 39080ca67211..b332adedf28e 100644
--- a/xen/arch/x86/desc.c
+++ b/xen/arch/x86/desc.c
@@ -91,7 +91,7 @@ seg_desc_t boot_compat_gdt[PAGE_SIZE / sizeof(seg_desc_t)] =
  * References boot_cpu_gdt_table for a short period, until the CPUs switch
  * onto their per-CPU GDTs.
  */
-const struct desc_ptr boot_gdtr = {
+const struct desc_ptr asmlinkage boot_gdtr = {
     .limit = LAST_RESERVED_GDT_BYTE,
     .base = (unsigned long)(boot_gdt - FIRST_RESERVED_GDT_ENTRY),
 };
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 86467da301e5..8ea64e31cdc2 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -808,8 +808,9 @@ static const char *__init get_option(const char *cmd, const char *opt)
     return o;
 }
 
-void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable,
-                           const char *cmdline)
+void asmlinkage __init efi_multiboot2(EFI_HANDLE ImageHandle,
+                                      EFI_SYSTEM_TABLE *SystemTable,
+                                      const char *cmdline)
 {
     EFI_GRAPHICS_OUTPUT_PROTOCOL *gop;
     EFI_HANDLE gop_handle;
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 0a66db10b959..9ce21c443bea 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -143,7 +143,7 @@
 /* Mapping of the fixmap space needed early. */
 l1_pgentry_t __section(".bss.page_aligned") __aligned(PAGE_SIZE)
     l1_fixmap[L1_PAGETABLE_ENTRIES];
-l1_pgentry_t __section(".bss.page_aligned") __aligned(PAGE_SIZE)
+l1_pgentry_t asmlinkage __section(".bss.page_aligned") __aligned(PAGE_SIZE)
     l1_fixmap_x[L1_PAGETABLE_ENTRIES];
 
 bool __read_mostly machine_to_phys_mapping_valid;
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 4c54ecbc91d7..8aa621533f3d 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -310,7 +310,7 @@ static void set_cpu_sibling_map(unsigned int cpu)
     }
 }
 
-void start_secondary(void *unused)
+void asmlinkage start_secondary(void *unused)
 {
     struct cpu_info *info = get_cpu_info();
 
-- 
2.34.1

[XEN PATCH 4/7] x86/viridian: make build_assertions static

[Nicola Vetrini]

This is consistent with other instances of the same function
and also resolves a violation of MISRA C:2012 Rule 8.4.

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/hvm/viridian/synic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/viridian/synic.c b/xen/arch/x86/hvm/viridian/synic.c
index 8cf600cec68f..3375e55e95ca 100644
--- a/xen/arch/x86/hvm/viridian/synic.c
+++ b/xen/arch/x86/hvm/viridian/synic.c
@@ -18,7 +18,7 @@
 #include "private.h"
 
 
-void __init __maybe_unused build_assertions(void)
+static void __init __maybe_unused build_assertions(void)
 {
     BUILD_BUG_ON(sizeof(struct hv_message) != HV_MESSAGE_SIZE);
 }
-- 
2.34.1

[XEN PATCH 5/7] docs/misra: add entry to exclude-list.json

[Nicola Vetrini]

x86/efi/check.c is not part of the final Xen binary, therefore
it doesn't need to conform to MISRA guidelines at the moment.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 docs/misra/exclude-list.json | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/docs/misra/exclude-list.json b/docs/misra/exclude-list.json
index b858a0baa106..24e4de3ca524 100644
--- a/docs/misra/exclude-list.json
+++ b/docs/misra/exclude-list.json
@@ -93,6 +93,10 @@
             "rel_path": "arch/x86/x86_64/mmconf-fam10h.c",
             "comment": "Imported from Linux, ignore for now"
         },
+        {
+            "rel_path": "arch/x86/efi/check.c",
+            "comment": "The resulting code is not included in the final Xen binary, ignore for now"
+        },
         {
             "rel_path": "common/bitmap.c",
             "comment": "Imported from Linux, ignore for now"
-- 
2.34.1

[XEN PATCH 6/7] xen/x86: remove stale comment

[Nicola Vetrini]

The comment referred to the declaration for do_mca, which
now is part of hypercall-defs.h, therefore the comment is stale.

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/cpu/mcheck/mce.c        | 2 +-
 xen/arch/x86/include/asm/hypercall.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
index 779a458cd88f..53493c8e4778 100644
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -14,7 +14,7 @@
 #include <xen/cpumask.h>
 #include <xen/event.h>
 #include <xen/guest_access.h>
-#include <xen/hypercall.h> /* for do_mca */
+#include <xen/hypercall.h>
 #include <xen/cpu.h>
 
 #include <asm/processor.h>
diff --git a/xen/arch/x86/include/asm/hypercall.h b/xen/arch/x86/include/asm/hypercall.h
index ec2edc771e9d..76658fff19ff 100644
--- a/xen/arch/x86/include/asm/hypercall.h
+++ b/xen/arch/x86/include/asm/hypercall.h
@@ -12,7 +12,7 @@
 #include <xen/types.h>
 #include <public/physdev.h>
 #include <public/event_channel.h>
-#include <public/arch-x86/xen-mca.h> /* for do_mca */
+#include <public/arch-x86/xen-mca.h>
 #include <asm/paging.h>
 
 #define __HYPERVISOR_paging_domctl_cont __HYPERVISOR_arch_1
-- 
2.34.1

[XEN PATCH 7/7] xen/page_alloc: deviate first_valid_mfn for MISRA C Rule 8.4

[Nicola Vetrini]

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
The preferred way to deviate is to use asmlinkage, but this modification is only
the consequence of NUMA on ARM (and possibly PPC) being a work in progress.
As stated in the comment above the textual deviation, first_valid_mfn will
likely then become static and there would be no need for the comment anymore.
This works towards having the analysis for this rule clean (i.e. no violations);
the interest in having a clean rule is that then it could be used to signal
newly introduced violations by making the analysis job fail.
---
 xen/common/page_alloc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 9b5df74fddab..794d7689b179 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -258,6 +258,7 @@ static PAGE_LIST_HEAD(page_broken_list);
  * first_valid_mfn is exported because it is use in ARM specific NUMA
  * helpers. See comment in arch/arm/include/asm/numa.h.
  */
+/* SAF-1-safe */
 mfn_t first_valid_mfn = INVALID_MFN_INITIALIZER;
 
 struct bootmem_region {
-- 
2.34.1

Re: [XEN PATCH 0/7] address some violations of MISRA C Rule 8.4

[Nicola Vetrini]

On 2023-11-29 16:24, Nicola Vetrini wrote:
> Hi all,
> 
> this series addresses some of the remaining violations of MISRA C:2012 
> Rule 8.4.
> 
> Some of the modifications are done according to the feedback received 
> in this
> thread [1]
> 

missing a reference:

[1] 
https://lore.kernel.org/xen-devel/51df0275d0988af618c22adb8d55138c@bugseng.com/T/#t


-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 5/7] docs/misra: add entry to exclude-list.json

[Jan Beulich]

On 29.11.2023 16:24, Nicola Vetrini wrote:
> x86/efi/check.c is not part of the final Xen binary, therefore
> it doesn't need to conform to MISRA guidelines at the moment.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

Re: [XEN PATCH 6/7] xen/x86: remove stale comment

[Jan Beulich]

On 29.11.2023 16:24, Nicola Vetrini wrote:
> The comment referred to the declaration for do_mca, which
> now is part of hypercall-defs.h, therefore the comment is stale.

If the comments were stale, the #include-s should also be able to
disappear?

> --- a/xen/arch/x86/cpu/mcheck/mce.c
> +++ b/xen/arch/x86/cpu/mcheck/mce.c
> @@ -14,7 +14,7 @@
>  #include <xen/cpumask.h>
>  #include <xen/event.h>
>  #include <xen/guest_access.h>
> -#include <xen/hypercall.h> /* for do_mca */
> +#include <xen/hypercall.h>
>  #include <xen/cpu.h>

Here specifically I think the comment isn't stale, as xen/hypercall.h
includes xen/hypercall-defs.h.

> --- a/xen/arch/x86/include/asm/hypercall.h
> +++ b/xen/arch/x86/include/asm/hypercall.h
> @@ -12,7 +12,7 @@
>  #include <xen/types.h>
>  #include <public/physdev.h>
>  #include <public/event_channel.h>
> -#include <public/arch-x86/xen-mca.h> /* for do_mca */
> +#include <public/arch-x86/xen-mca.h>
>  #include <asm/paging.h>

Here otoh I'm not even sure this public header (or the others) is (are)
really needed.

Jan

Re: [XEN PATCH 3/7] xen/x86: add missing instances of asmlinkage attributes

[Jan Beulich]

On 29.11.2023 16:24, Nicola Vetrini wrote:
> --- a/xen/arch/x86/desc.c
> +++ b/xen/arch/x86/desc.c
> @@ -91,7 +91,7 @@ seg_desc_t boot_compat_gdt[PAGE_SIZE / sizeof(seg_desc_t)] =
>   * References boot_cpu_gdt_table for a short period, until the CPUs switch
>   * onto their per-CPU GDTs.
>   */
> -const struct desc_ptr boot_gdtr = {
> +const struct desc_ptr asmlinkage boot_gdtr = {
>      .limit = LAST_RESERVED_GDT_BYTE,
>      .base = (unsigned long)(boot_gdt - FIRST_RESERVED_GDT_ENTRY),
>  };

I'm not convinced asmlinkage is okay to use on data. Recall that in principle
it may expand to an attribute specifying a non-default calling convention.
Such attributes cannot be assumed to continue to be possible to apply to
non-functions, even if such may happen to work with a particular compiler
version.

Jan

Re: [XEN PATCH 2/7] x86/i8259: add missing header for init_IRQ declaration

[Jan Beulich]

On 29.11.2023 16:24, Nicola Vetrini wrote:
> --- a/xen/arch/x86/i8259.c
> +++ b/xen/arch/x86/i8259.c
> @@ -19,6 +19,7 @@
>  #include <xen/delay.h>
>  #include <asm/apic.h>
>  #include <asm/asm_defns.h>
> +#include <asm/setup.h>
>  #include <io_ports.h>
>  #include <irq_vectors.h>

A patch doing this (among other things) is already pending: "x86: detect
PIC aliasing on ports other than 0x[2A][01]".

Jan

Re: [XEN PATCH 2/7] x86/i8259: add missing header for init_IRQ declaration

[Nicola Vetrini]

On 2023-11-30 17:48, Jan Beulich wrote:
> On 29.11.2023 16:24, Nicola Vetrini wrote:
>> --- a/xen/arch/x86/i8259.c
>> +++ b/xen/arch/x86/i8259.c
>> @@ -19,6 +19,7 @@
>>  #include <xen/delay.h>
>>  #include <asm/apic.h>
>>  #include <asm/asm_defns.h>
>> +#include <asm/setup.h>
>>  #include <io_ports.h>
>>  #include <irq_vectors.h>
> 
> A patch doing this (among other things) is already pending: "x86: 
> detect
> PIC aliasing on ports other than 0x[2A][01]".
> 

Ok

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 1/7] xen/arm: mmu: add headers for missing declarations

[Stefano Stabellini]

On Wed, 29 Nov 2023, Nicola Vetrini wrote:
> The definitions needing the inclusion of asm/setup.h are
> boot_{first,second,third}(_id)?, whereas vmap.h is needed by arch_vmap_virt_end.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

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

Re: [XEN PATCH 4/7] x86/viridian: make build_assertions static

[Stefano Stabellini]

On Wed, 29 Nov 2023, Nicola Vetrini wrote:
> This is consistent with other instances of the same function
> and also resolves a violation of MISRA C:2012 Rule 8.4.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>


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

Re: [XEN PATCH 7/7] xen/page_alloc: deviate first_valid_mfn for MISRA C Rule 8.4

[Stefano Stabellini]

On Wed, 29 Nov 2023, Nicola Vetrini wrote:
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
> The preferred way to deviate is to use asmlinkage, but this modification is only
> the consequence of NUMA on ARM (and possibly PPC) being a work in progress.
> As stated in the comment above the textual deviation, first_valid_mfn will
> likely then become static and there would be no need for the comment anymore.
> This works towards having the analysis for this rule clean (i.e. no violations);
> the interest in having a clean rule is that then it could be used to signal
> newly introduced violations by making the analysis job fail.

Please add this text as part of the commit message. It can be done on
commit. With that:

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


> ---
>  xen/common/page_alloc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 9b5df74fddab..794d7689b179 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -258,6 +258,7 @@ static PAGE_LIST_HEAD(page_broken_list);
>   * first_valid_mfn is exported because it is use in ARM specific NUMA
>   * helpers. See comment in arch/arm/include/asm/numa.h.
>   */
> +/* SAF-1-safe */
>  mfn_t first_valid_mfn = INVALID_MFN_INITIALIZER;
>  
>  struct bootmem_region {
> -- 
> 2.34.1
> 

Re: [XEN PATCH 7/7] xen/page_alloc: deviate first_valid_mfn for MISRA C Rule 8.4

[Jan Beulich]

On 01.12.2023 03:47, Stefano Stabellini wrote:
> On Wed, 29 Nov 2023, Nicola Vetrini wrote:
>> No functional change.
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>> The preferred way to deviate is to use asmlinkage, but this modification is only
>> the consequence of NUMA on ARM (and possibly PPC) being a work in progress.
>> As stated in the comment above the textual deviation, first_valid_mfn will
>> likely then become static and there would be no need for the comment anymore.
>> This works towards having the analysis for this rule clean (i.e. no violations);
>> the interest in having a clean rule is that then it could be used to signal
>> newly introduced violations by making the analysis job fail.
> 
> Please add this text as part of the commit message. It can be done on
> commit.

I assume you saw my reply on another of the patches in this series as to
asmlinkage use on variables? IOW I think this paragraph would also need
adjustment to account for that.

> With that:
> 
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> 
>> ---
>>  xen/common/page_alloc.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
>> index 9b5df74fddab..794d7689b179 100644
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -258,6 +258,7 @@ static PAGE_LIST_HEAD(page_broken_list);
>>   * first_valid_mfn is exported because it is use in ARM specific NUMA
>>   * helpers. See comment in arch/arm/include/asm/numa.h.
>>   */
>> +/* SAF-1-safe */
>>  mfn_t first_valid_mfn = INVALID_MFN_INITIALIZER;
>>  
>>  struct bootmem_region {
>> -- 
>> 2.34.1
>>

Re: [XEN PATCH 3/7] xen/x86: add missing instances of asmlinkage attributes

[Nicola Vetrini]

On 2023-11-30 17:44, Jan Beulich wrote:
> On 29.11.2023 16:24, Nicola Vetrini wrote:
>> --- a/xen/arch/x86/desc.c
>> +++ b/xen/arch/x86/desc.c
>> @@ -91,7 +91,7 @@ seg_desc_t boot_compat_gdt[PAGE_SIZE / 
>> sizeof(seg_desc_t)] =
>>   * References boot_cpu_gdt_table for a short period, until the CPUs 
>> switch
>>   * onto their per-CPU GDTs.
>>   */
>> -const struct desc_ptr boot_gdtr = {
>> +const struct desc_ptr asmlinkage boot_gdtr = {
>>      .limit = LAST_RESERVED_GDT_BYTE,
>>      .base = (unsigned long)(boot_gdt - FIRST_RESERVED_GDT_ENTRY),
>>  };
> 
> I'm not convinced asmlinkage is okay to use on data. Recall that in 
> principle
> it may expand to an attribute specifying a non-default calling 
> convention.
> Such attributes cannot be assumed to continue to be possible to apply 
> to
> non-functions, even if such may happen to work with a particular 
> compiler
> version.
> 

It's already being used on variables, I believe.

xen/arch/x86/mm.c:l1_pgentry_t asmlinkage __section(".bss.page_aligned") 
__aligned(PAGE_SIZE)
xen/arch/x86/setup.c:unsigned long asmlinkage __read_mostly 
cr4_pv32_mask;
xen/arch/x86/setup.c:char asmlinkage 
__section(".init.bss.stack_aligned") __aligned(STACK_SIZE)
xen/arch/x86/setup.c:unsigned int asmlinkage __initdata multiboot_ptr;

If you have concern about this particular variable, then we can fall 
back on SAF or just put a declaration in the appropriate place.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 3/7] xen/x86: add missing instances of asmlinkage attributes

[Jan Beulich]

On 01.12.2023 09:42, Nicola Vetrini wrote:
> On 2023-11-30 17:44, Jan Beulich wrote:
>> On 29.11.2023 16:24, Nicola Vetrini wrote:
>>> --- a/xen/arch/x86/desc.c
>>> +++ b/xen/arch/x86/desc.c
>>> @@ -91,7 +91,7 @@ seg_desc_t boot_compat_gdt[PAGE_SIZE / 
>>> sizeof(seg_desc_t)] =
>>>   * References boot_cpu_gdt_table for a short period, until the CPUs 
>>> switch
>>>   * onto their per-CPU GDTs.
>>>   */
>>> -const struct desc_ptr boot_gdtr = {
>>> +const struct desc_ptr asmlinkage boot_gdtr = {
>>>      .limit = LAST_RESERVED_GDT_BYTE,
>>>      .base = (unsigned long)(boot_gdt - FIRST_RESERVED_GDT_ENTRY),
>>>  };
>>
>> I'm not convinced asmlinkage is okay to use on data. Recall that in 
>> principle
>> it may expand to an attribute specifying a non-default calling 
>> convention.
>> Such attributes cannot be assumed to continue to be possible to apply 
>> to
>> non-functions, even if such may happen to work with a particular 
>> compiler
>> version.
>>
> 
> It's already being used on variables, I believe.
> 
> xen/arch/x86/mm.c:l1_pgentry_t asmlinkage __section(".bss.page_aligned") 
> __aligned(PAGE_SIZE)
> xen/arch/x86/setup.c:unsigned long asmlinkage __read_mostly 
> cr4_pv32_mask;
> xen/arch/x86/setup.c:char asmlinkage 
> __section(".init.bss.stack_aligned") __aligned(STACK_SIZE)
> xen/arch/x86/setup.c:unsigned int asmlinkage __initdata multiboot_ptr;

Yeah, I was fearing that something was overlooked earlier on.

Jan

> If you have concern about this particular variable, then we can fall 
> back on SAF or just put a declaration in the appropriate place.
> 

Re: [XEN PATCH 6/7] xen/x86: remove stale comment

[Nicola Vetrini]

On 2023-11-30 17:41, Jan Beulich wrote:
> On 29.11.2023 16:24, Nicola Vetrini wrote:
>> The comment referred to the declaration for do_mca, which
>> now is part of hypercall-defs.h, therefore the comment is stale.
> 
> If the comments were stale, the #include-s should also be able to
> disappear?
> 
>> --- a/xen/arch/x86/cpu/mcheck/mce.c
>> +++ b/xen/arch/x86/cpu/mcheck/mce.c
>> @@ -14,7 +14,7 @@
>>  #include <xen/cpumask.h>
>>  #include <xen/event.h>
>>  #include <xen/guest_access.h>
>> -#include <xen/hypercall.h> /* for do_mca */
>> +#include <xen/hypercall.h>
>>  #include <xen/cpu.h>
> 
> Here specifically I think the comment isn't stale, as xen/hypercall.h
> includes xen/hypercall-defs.h.
> 

Ok, I see your point

>> --- a/xen/arch/x86/include/asm/hypercall.h
>> +++ b/xen/arch/x86/include/asm/hypercall.h
>> @@ -12,7 +12,7 @@
>>  #include <xen/types.h>
>>  #include <public/physdev.h>
>>  #include <public/event_channel.h>
>> -#include <public/arch-x86/xen-mca.h> /* for do_mca */
>> +#include <public/arch-x86/xen-mca.h>
>>  #include <asm/paging.h>
> 
> Here otoh I'm not even sure this public header (or the others) is (are)
> really needed.
> 

I confirm this. It build even without this header.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 7/7] xen/page_alloc: deviate first_valid_mfn for MISRA C Rule 8.4

[Stefano Stabellini]

On Fri, 1 Dec 2023, Jan Beulich wrote:
> On 01.12.2023 03:47, Stefano Stabellini wrote:
> > On Wed, 29 Nov 2023, Nicola Vetrini wrote:
> >> No functional change.
> >>
> >> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> >> ---
> >> The preferred way to deviate is to use asmlinkage, but this modification is only
> >> the consequence of NUMA on ARM (and possibly PPC) being a work in progress.
> >> As stated in the comment above the textual deviation, first_valid_mfn will
> >> likely then become static and there would be no need for the comment anymore.
> >> This works towards having the analysis for this rule clean (i.e. no violations);
> >> the interest in having a clean rule is that then it could be used to signal
> >> newly introduced violations by making the analysis job fail.
> > 
> > Please add this text as part of the commit message. It can be done on
> > commit.
> 
> I assume you saw my reply on another of the patches in this series as to
> asmlinkage use on variables? IOW I think this paragraph would also need
> adjustment to account for that.

I was going to ask you about that: reading your reply
https://marc.info/?l=xen-devel&m=170142048615336 it is not clear to me
what you are asking or suggesting as next step in regard to asmlinkage
use on variables.

Re: [XEN PATCH 7/7] xen/page_alloc: deviate first_valid_mfn for MISRA C Rule 8.4

[Julien Grall]

Hi,

On 02/12/2023 04:03, Stefano Stabellini wrote:
> On Fri, 1 Dec 2023, Jan Beulich wrote:
>> On 01.12.2023 03:47, Stefano Stabellini wrote:
>>> On Wed, 29 Nov 2023, Nicola Vetrini wrote:
>>>> No functional change.
>>>>
>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>> ---
>>>> The preferred way to deviate is to use asmlinkage, but this modification is only
>>>> the consequence of NUMA on ARM (and possibly PPC) being a work in progress.
>>>> As stated in the comment above the textual deviation, first_valid_mfn will
>>>> likely then become static and there would be no need for the comment anymore.
>>>> This works towards having the analysis for this rule clean (i.e. no violations);
>>>> the interest in having a clean rule is that then it could be used to signal
>>>> newly introduced violations by making the analysis job fail.
>>>
>>> Please add this text as part of the commit message. It can be done on
>>> commit.
>>
>> I assume you saw my reply on another of the patches in this series as to
>> asmlinkage use on variables? IOW I think this paragraph would also need
>> adjustment to account for that.
> 
> I was going to ask you about that: reading your reply
> https://marc.info/?l=xen-devel&m=170142048615336 it is not clear to me
> what you are asking or suggesting as next step in regard to asmlinkage
> use on variables.

Regardless what Jan wrote there, I think it would be an abuse to add 
asmlinkage for first_valid_mfn because it is not used by assembly code.

The same goes for the use of SAF-1-safe. This is defined as:

"Functions and variables used only by asm modules do not need to have a 
visible declaration prior to their definition."

Now, regarding the violation itself. AFAIU, this would be a violation 
only on x86 because Arm, PPC, RISC-V will all need the declaration as 
NUMA is not supported. For Arm, we may have NUMA support soon, but I 
doubt this will happen in the next couple of years for the others.

Furthermore, I am sure this is not the only exported variable/function 
which may be used by some archictectures but not others. So I think it 
would be better to declare first_valid_mfn in a common header.

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 7/7] xen/page_alloc: deviate first_valid_mfn for MISRA C Rule 8.4

[Jan Beulich]

On 02.12.2023 04:03, Stefano Stabellini wrote:
> On Fri, 1 Dec 2023, Jan Beulich wrote:
>> On 01.12.2023 03:47, Stefano Stabellini wrote:
>>> On Wed, 29 Nov 2023, Nicola Vetrini wrote:
>>>> No functional change.
>>>>
>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>> ---
>>>> The preferred way to deviate is to use asmlinkage, but this modification is only
>>>> the consequence of NUMA on ARM (and possibly PPC) being a work in progress.
>>>> As stated in the comment above the textual deviation, first_valid_mfn will
>>>> likely then become static and there would be no need for the comment anymore.
>>>> This works towards having the analysis for this rule clean (i.e. no violations);
>>>> the interest in having a clean rule is that then it could be used to signal
>>>> newly introduced violations by making the analysis job fail.
>>>
>>> Please add this text as part of the commit message. It can be done on
>>> commit.
>>
>> I assume you saw my reply on another of the patches in this series as to
>> asmlinkage use on variables? IOW I think this paragraph would also need
>> adjustment to account for that.
> 
> I was going to ask you about that: reading your reply
> https://marc.info/?l=xen-devel&m=170142048615336 it is not clear to me
> what you are asking or suggesting as next step in regard to asmlinkage
> use on variables.

Either we need a separate attribute, or we need affirmation that calling
convention attributes are ignored (and going to be going forward) for
variables, or we need to resort to SAF-* comments. I'm not sure what's
best (assuming the "affirm" wouldn't really be possible).

Jan

Re: [XEN PATCH 7/7] xen/page_alloc: deviate first_valid_mfn for MISRA C Rule 8.4

[Nicola Vetrini]

On 2023-12-04 08:44, Jan Beulich wrote:
> On 02.12.2023 04:03, Stefano Stabellini wrote:
>> On Fri, 1 Dec 2023, Jan Beulich wrote:
>>> On 01.12.2023 03:47, Stefano Stabellini wrote:
>>>> On Wed, 29 Nov 2023, Nicola Vetrini wrote:
>>>>> No functional change.
>>>>> 
>>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>>> ---
>>>>> The preferred way to deviate is to use asmlinkage, but this 
>>>>> modification is only
>>>>> the consequence of NUMA on ARM (and possibly PPC) being a work in 
>>>>> progress.
>>>>> As stated in the comment above the textual deviation, 
>>>>> first_valid_mfn will
>>>>> likely then become static and there would be no need for the 
>>>>> comment anymore.
>>>>> This works towards having the analysis for this rule clean (i.e. no 
>>>>> violations);
>>>>> the interest in having a clean rule is that then it could be used 
>>>>> to signal
>>>>> newly introduced violations by making the analysis job fail.
>>>> 
>>>> Please add this text as part of the commit message. It can be done 
>>>> on
>>>> commit.
>>> 
>>> I assume you saw my reply on another of the patches in this series as 
>>> to
>>> asmlinkage use on variables? IOW I think this paragraph would also 
>>> need
>>> adjustment to account for that.
>> 
>> I was going to ask you about that: reading your reply
>> https://marc.info/?l=xen-devel&m=170142048615336 it is not clear to me
>> what you are asking or suggesting as next step in regard to asmlinkage
>> use on variables.
> 
> Either we need a separate attribute, or we need affirmation that 
> calling
> convention attributes are ignored (and going to be going forward) for
> variables, or we need to resort to SAF-* comments. I'm not sure what's
> best (assuming the "affirm" wouldn't really be possible).
> 

Well, gcc does warn on unsupported attributes for the entity which are 
being dropped. This appears to be the case for calling convention 
attributes, as they are not listed in their documentation for variable 
attributes, but some more digging would be required to determine whether 
that's always the case.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 6/7] xen/x86: remove stale comment

[Nicola Vetrini]

On 2023-12-01 17:57, Nicola Vetrini wrote:
> On 2023-11-30 17:41, Jan Beulich wrote:
>> On 29.11.2023 16:24, Nicola Vetrini wrote:
>>> The comment referred to the declaration for do_mca, which
>>> now is part of hypercall-defs.h, therefore the comment is stale.
>> 
>> If the comments were stale, the #include-s should also be able to
>> disappear?

>>> --- a/xen/arch/x86/include/asm/hypercall.h
>>> +++ b/xen/arch/x86/include/asm/hypercall.h
>>> @@ -12,7 +12,7 @@
>>>  #include <xen/types.h>
>>>  #include <public/physdev.h>
>>>  #include <public/event_channel.h>
>>> -#include <public/arch-x86/xen-mca.h> /* for do_mca */
>>> +#include <public/arch-x86/xen-mca.h>
>>>  #include <asm/paging.h>
>> 
>> Here otoh I'm not even sure this public header (or the others) is 
>> (are)
>> really needed.
>> 
> 
> I confirm this. It build even without this header.

It does appear to be needed after all. I did two differential pipeline 
runs, and some jobs fail to compile when I remove the header (e.g., 
[1]). Looking trough the build log, it's not entirely clear what is the 
relationship, but it seems related to some use of this struct defined in 
xen-mca.h:

typedef struct xen_mc xen_mc_t;
DEFINE_XEN_GUEST_HANDLE(xen_mc_t);

[1] https://gitlab.com/xen-project/people/bugseng/xen/-/jobs/5675760184

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 6/7] xen/x86: remove stale comment

[Jan Beulich]

On 04.12.2023 17:26, Nicola Vetrini wrote:
> On 2023-12-01 17:57, Nicola Vetrini wrote:
>> On 2023-11-30 17:41, Jan Beulich wrote:
>>> On 29.11.2023 16:24, Nicola Vetrini wrote:
>>>> The comment referred to the declaration for do_mca, which
>>>> now is part of hypercall-defs.h, therefore the comment is stale.
>>>
>>> If the comments were stale, the #include-s should also be able to
>>> disappear?
> 
>>>> --- a/xen/arch/x86/include/asm/hypercall.h
>>>> +++ b/xen/arch/x86/include/asm/hypercall.h
>>>> @@ -12,7 +12,7 @@
>>>>  #include <xen/types.h>
>>>>  #include <public/physdev.h>
>>>>  #include <public/event_channel.h>
>>>> -#include <public/arch-x86/xen-mca.h> /* for do_mca */
>>>> +#include <public/arch-x86/xen-mca.h>
>>>>  #include <asm/paging.h>
>>>
>>> Here otoh I'm not even sure this public header (or the others) is 
>>> (are)
>>> really needed.
>>>
>>
>> I confirm this. It build even without this header.
> 
> It does appear to be needed after all. I did two differential pipeline 
> runs, and some jobs fail to compile when I remove the header (e.g., 
> [1]). Looking trough the build log, it's not entirely clear what is the 
> relationship, but it seems related to some use of this struct defined in 
> xen-mca.h:
> 
> typedef struct xen_mc xen_mc_t;
> DEFINE_XEN_GUEST_HANDLE(xen_mc_t);

That do_mca()'s parameter type, so in a way the comment is still correct
then.

Jan

Re: [XEN PATCH 6/7] xen/x86: remove stale comment

[Nicola Vetrini]

On 2023-12-04 17:40, Jan Beulich wrote:
> On 04.12.2023 17:26, Nicola Vetrini wrote:
>> On 2023-12-01 17:57, Nicola Vetrini wrote:
>>> On 2023-11-30 17:41, Jan Beulich wrote:
>>>> On 29.11.2023 16:24, Nicola Vetrini wrote:
>>>>> The comment referred to the declaration for do_mca, which
>>>>> now is part of hypercall-defs.h, therefore the comment is stale.
>>>> 
>>>> If the comments were stale, the #include-s should also be able to
>>>> disappear?
>> 
>>>>> --- a/xen/arch/x86/include/asm/hypercall.h
>>>>> +++ b/xen/arch/x86/include/asm/hypercall.h
>>>>> @@ -12,7 +12,7 @@
>>>>>  #include <xen/types.h>
>>>>>  #include <public/physdev.h>
>>>>>  #include <public/event_channel.h>
>>>>> -#include <public/arch-x86/xen-mca.h> /* for do_mca */
>>>>> +#include <public/arch-x86/xen-mca.h>
>>>>>  #include <asm/paging.h>
>>>> 
>>>> Here otoh I'm not even sure this public header (or the others) is
>>>> (are)
>>>> really needed.
>>>> 
>>> 
>>> I confirm this. It build even without this header.
>> 
>> It does appear to be needed after all. I did two differential pipeline
>> runs, and some jobs fail to compile when I remove the header (e.g.,
>> [1]). Looking trough the build log, it's not entirely clear what is 
>> the
>> relationship, but it seems related to some use of this struct defined 
>> in
>> xen-mca.h:
>> 
>> typedef struct xen_mc xen_mc_t;
>> DEFINE_XEN_GUEST_HANDLE(xen_mc_t);
> 
> That do_mca()'s parameter type, so in a way the comment is still 
> correct
> then.
> 
> Jan

Yeah, this patch can be dropped.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 7/7] xen/page_alloc: deviate first_valid_mfn for MISRA C Rule 8.4

[Stefano Stabellini]

On Mon, 4 Dec 2023, Nicola Vetrini wrote:
> On 2023-12-04 08:44, Jan Beulich wrote:
> > On 02.12.2023 04:03, Stefano Stabellini wrote:
> > > On Fri, 1 Dec 2023, Jan Beulich wrote:
> > > > On 01.12.2023 03:47, Stefano Stabellini wrote:
> > > > > On Wed, 29 Nov 2023, Nicola Vetrini wrote:
> > > > > > No functional change.
> > > > > > 
> > > > > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> > > > > > ---
> > > > > > The preferred way to deviate is to use asmlinkage, but this
> > > > > > modification is only
> > > > > > the consequence of NUMA on ARM (and possibly PPC) being a work in
> > > > > > progress.
> > > > > > As stated in the comment above the textual deviation,
> > > > > > first_valid_mfn will
> > > > > > likely then become static and there would be no need for the comment
> > > > > > anymore.
> > > > > > This works towards having the analysis for this rule clean (i.e. no
> > > > > > violations);
> > > > > > the interest in having a clean rule is that then it could be used to
> > > > > > signal
> > > > > > newly introduced violations by making the analysis job fail.
> > > > > 
> > > > > Please add this text as part of the commit message. It can be done on
> > > > > commit.
> > > > 
> > > > I assume you saw my reply on another of the patches in this series as to
> > > > asmlinkage use on variables? IOW I think this paragraph would also need
> > > > adjustment to account for that.
> > > 
> > > I was going to ask you about that: reading your reply
> > > https://marc.info/?l=xen-devel&m=170142048615336 it is not clear to me
> > > what you are asking or suggesting as next step in regard to asmlinkage
> > > use on variables.
> > 
> > Either we need a separate attribute, or we need affirmation that calling
> > convention attributes are ignored (and going to be going forward) for
> > variables, or we need to resort to SAF-* comments. I'm not sure what's
> > best (assuming the "affirm" wouldn't really be possible).
> > 
> 
> Well, gcc does warn on unsupported attributes for the entity which are being
> dropped. This appears to be the case for calling convention attributes, as
> they are not listed in their documentation for variable attributes, but some
> more digging would be required to determine whether that's always the case.

Given that I don't suppose we have many variables that need deviating
(probably only 2-3 overall?) I think it is just easier to add a SAF
comment.

Re: [XEN PATCH 7/7] xen/page_alloc: deviate first_valid_mfn for MISRA C Rule 8.4

[Stefano Stabellini]

On Sat, 2 Dec 2023, Julien Grall wrote:
> Hi,
> 
> On 02/12/2023 04:03, Stefano Stabellini wrote:
> > On Fri, 1 Dec 2023, Jan Beulich wrote:
> > > On 01.12.2023 03:47, Stefano Stabellini wrote:
> > > > On Wed, 29 Nov 2023, Nicola Vetrini wrote:
> > > > > No functional change.
> > > > > 
> > > > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> > > > > ---
> > > > > The preferred way to deviate is to use asmlinkage, but this
> > > > > modification is only
> > > > > the consequence of NUMA on ARM (and possibly PPC) being a work in
> > > > > progress.
> > > > > As stated in the comment above the textual deviation, first_valid_mfn
> > > > > will
> > > > > likely then become static and there would be no need for the comment
> > > > > anymore.
> > > > > This works towards having the analysis for this rule clean (i.e. no
> > > > > violations);
> > > > > the interest in having a clean rule is that then it could be used to
> > > > > signal
> > > > > newly introduced violations by making the analysis job fail.
> > > > 
> > > > Please add this text as part of the commit message. It can be done on
> > > > commit.
> > > 
> > > I assume you saw my reply on another of the patches in this series as to
> > > asmlinkage use on variables? IOW I think this paragraph would also need
> > > adjustment to account for that.
> > 
> > I was going to ask you about that: reading your reply
> > https://marc.info/?l=xen-devel&m=170142048615336 it is not clear to me
> > what you are asking or suggesting as next step in regard to asmlinkage
> > use on variables.
> 
> Regardless what Jan wrote there, I think it would be an abuse to add
> asmlinkage for first_valid_mfn because it is not used by assembly code.
> 
> The same goes for the use of SAF-1-safe. This is defined as:
> 
> "Functions and variables used only by asm modules do not need to have a
> visible declaration prior to their definition."
> 
> Now, regarding the violation itself. AFAIU, this would be a violation only on
> x86 because Arm, PPC, RISC-V will all need the declaration as NUMA is not
> supported. For Arm, we may have NUMA support soon, but I doubt this will
> happen in the next couple of years for the others.
> 
> Furthermore, I am sure this is not the only exported variable/function which
> may be used by some archictectures but not others. So I think it would be
> better to declare first_valid_mfn in a common header.

That is true

Re: [XEN PATCH 7/7] xen/page_alloc: deviate first_valid_mfn for MISRA C Rule 8.4

[Nicola Vetrini]

On 2023-12-05 00:18, Stefano Stabellini wrote:
> On Mon, 4 Dec 2023, Nicola Vetrini wrote:
>> On 2023-12-04 08:44, Jan Beulich wrote:
>> > On 02.12.2023 04:03, Stefano Stabellini wrote:
>> > > On Fri, 1 Dec 2023, Jan Beulich wrote:
>> > > > On 01.12.2023 03:47, Stefano Stabellini wrote:
>> > > > > On Wed, 29 Nov 2023, Nicola Vetrini wrote:
>> > > > > > No functional change.
>> > > > > >
>> > > > > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> > > > > > ---
>> > > > > > The preferred way to deviate is to use asmlinkage, but this
>> > > > > > modification is only
>> > > > > > the consequence of NUMA on ARM (and possibly PPC) being a work in
>> > > > > > progress.
>> > > > > > As stated in the comment above the textual deviation,
>> > > > > > first_valid_mfn will
>> > > > > > likely then become static and there would be no need for the comment
>> > > > > > anymore.
>> > > > > > This works towards having the analysis for this rule clean (i.e. no
>> > > > > > violations);
>> > > > > > the interest in having a clean rule is that then it could be used to
>> > > > > > signal
>> > > > > > newly introduced violations by making the analysis job fail.
>> > > > >
>> > > > > Please add this text as part of the commit message. It can be done on
>> > > > > commit.
>> > > >
>> > > > I assume you saw my reply on another of the patches in this series as to
>> > > > asmlinkage use on variables? IOW I think this paragraph would also need
>> > > > adjustment to account for that.
>> > >
>> > > I was going to ask you about that: reading your reply
>> > > https://marc.info/?l=xen-devel&m=170142048615336 it is not clear to me
>> > > what you are asking or suggesting as next step in regard to asmlinkage
>> > > use on variables.
>> >
>> > Either we need a separate attribute, or we need affirmation that calling
>> > convention attributes are ignored (and going to be going forward) for
>> > variables, or we need to resort to SAF-* comments. I'm not sure what's
>> > best (assuming the "affirm" wouldn't really be possible).
>> >
>> 
>> Well, gcc does warn on unsupported attributes for the entity which are 
>> being
>> dropped. This appears to be the case for calling convention 
>> attributes, as
>> they are not listed in their documentation for variable attributes, 
>> but some
>> more digging would be required to determine whether that's always the 
>> case.
> 
> Given that I don't suppose we have many variables that need deviating
> (probably only 2-3 overall?) I think it is just easier to add a SAF
> comment.

I agree, but then given what Julien wrote in the thread, I'll see what I 
can do to avoid creating a new SAF entry.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)