[XEN PATCH 0/8] xen: address MISRA C:2012 Rule 8.4

[XEN PATCH 0/8] xen: address MISRA C:2012 Rule 8.4

[Nicola Vetrini]

Rule states the following:
"A compatible declaration shall be visible when an object or function with
external linkage is defined".

This series deals with violations concerning functions and variables used by C
code (thus not used only within asm). The resolution strategies are as follows:

1. make the functions/variables static
2. include the header that contains the compatible declaration, or add one in
   the header if that's not the case.

No functional change.

Additional notes:
- other cases, such as functions/variables used only in asm code will be possibly
  dealt with in future patches.
- functions in 'gcov_base.c' are deviated, since they are called only from code
  generated by gcc in a particular non-release build configuration;
- variables in 'xen/include/acpi/acglobal.h' are deviated temporarily, since
  it's uncertain whether modifying the include pattern to be compliant with the
	rule would introduce subtle bugs.

Nicola Vetrini (8):
  arm/efi: address MISRA C:2012 Rule 8.4
  xen/memory: address MISRA C:2012 Rule 8.4
  xen: address MISRA C:2012 Rule 8.4
  xen/arm: address MISRA C:2012 Rule 8.4
  x86: address MISRA C:2012 Rule 8.4
  xen/arm: mm: address MISRA C:2012 Rule 8.4
  x86/i8259: address MISRA C:2012 Rule 8.4
  x86/nmi: address MISRA C:2012 Rule 8.4

 xen/arch/arm/efi/efi-boot.h       | 2 +-
 xen/arch/arm/include/asm/mm.h     | 3 +++
 xen/arch/arm/setup.c              | 1 +
 xen/arch/arm/time.c               | 1 +
 xen/arch/x86/cpu/mcheck/mce.c     | 2 +-
 xen/arch/x86/cpu/mcheck/mce_amd.c | 2 +-
 xen/arch/x86/cpu/microcode/core.c | 2 +-
 xen/arch/x86/i8259.c              | 1 +
 xen/arch/x86/nmi.c                | 3 ++-
 xen/arch/x86/spec_ctrl.c          | 4 ++--
 xen/common/kernel.c               | 2 +-
 xen/common/memory.c               | 2 +-
 xen/include/xen/cper.h            | 3 +--
 xen/include/xen/time.h            | 1 +
 14 files changed, 18 insertions(+), 11 deletions(-)

--
2.34.1

[XEN PATCH 1/8] arm/efi: address MISRA C:2012 Rule 8.4

[Nicola Vetrini]

the function 'fdt_add_uefi_nodes' can be defined static, as its
only callers are within the same file. This in turn avoids
violating Rule 8.4 because no declaration is present.

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/arm/efi/efi-boot.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index f24df2abb9..1c3640bb65 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -227,7 +227,7 @@ static EFI_STATUS __init efi_process_memory_map_bootinfo(EFI_MEMORY_DESCRIPTOR *
  * of the System table address, the address of the final EFI memory map,
  * and memory map information.
  */
-EFI_STATUS __init fdt_add_uefi_nodes(EFI_SYSTEM_TABLE *sys_table,
+static EFI_STATUS __init fdt_add_uefi_nodes(EFI_SYSTEM_TABLE *sys_table,
                                             void *fdt,
                                             EFI_MEMORY_DESCRIPTOR *memory_map,
                                             UINTN map_size,
-- 
2.34.1

Re: [XEN PATCH 1/8] arm/efi: address MISRA C:2012 Rule 8.4

[Luca Fancellu]

> On 9 Aug 2023, at 12:02, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote:
> 
> the function 'fdt_add_uefi_nodes' can be defined static, as its
> only callers are within the same file. This in turn avoids
> violating Rule 8.4 because no declaration is present.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

With

Fixes: 6d70ea10d49f ("Add ARM EFI boot support”)

Re: [XEN PATCH 1/8] arm/efi: address MISRA C:2012 Rule 8.4

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 590 bytes --]

On Wed, 9 Aug 2023, Luca Fancellu wrote:
> > On 9 Aug 2023, at 12:02, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote:
> > 
> > the function 'fdt_add_uefi_nodes' can be defined static, as its
> > only callers are within the same file. This in turn avoids
> > violating Rule 8.4 because no declaration is present.
> > 
> > No functional change.
> > 
> > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> 
> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
> 
> With
> 
> Fixes: 6d70ea10d49f ("Add ARM EFI boot support”)

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

[XEN PATCH 2/8] xen/memory: address MISRA C:2012 Rule 8.4

[Nicola Vetrini]

The function 'ioreq_server_max_frames' can be defined static,
as its only uses are within the same file. This in turn avoids
violating Rule 8.4 because no declaration is present.

No functional change.

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

diff --git a/xen/common/memory.c b/xen/common/memory.c
index c206fa4808..b1dcbaf551 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -1120,7 +1120,7 @@ static long xatp_permission_check(struct domain *d, unsigned int space)
     return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
 }
 
-unsigned int ioreq_server_max_frames(const struct domain *d)
+static unsigned int ioreq_server_max_frames(const struct domain *d)
 {
     unsigned int nr = 0;
 
-- 
2.34.1

Re: [XEN PATCH 2/8] xen/memory: address MISRA C:2012 Rule 8.4

[Luca Fancellu]

> On 9 Aug 2023, at 12:02, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote:
> 
> The function 'ioreq_server_max_frames' can be defined static,
> as its only uses are within the same file. This in turn avoids
> violating Rule 8.4 because no declaration is present.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Makes sense,

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

Maybe it’s also better adding this:
Fixes: 9244528955de ("xen/memory: Fix acquire_resource size semantics”)

If the maintainers agree

Re: [XEN PATCH 2/8] xen/memory: address MISRA C:2012 Rule 8.4

[Jan Beulich]

On 09.08.2023 15:34, Luca Fancellu wrote:
>> On 9 Aug 2023, at 12:02, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote:
>>
>> The function 'ioreq_server_max_frames' can be defined static,
>> as its only uses are within the same file. This in turn avoids
>> violating Rule 8.4 because no declaration is present.
>>
>> No functional change.
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> 
> Makes sense,
> 
> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
> 
> Maybe it’s also better adding this:
> Fixes: 9244528955de ("xen/memory: Fix acquire_resource size semantics”)
> 
> If the maintainers agree

I for one agree; in fact I did ask for Fixes: tags throughout this series,
but in the context of another patch. As said there I also think titles and
descriptions want writing differently here.

Jan

Re: [XEN PATCH 2/8] xen/memory: address MISRA C:2012 Rule 8.4

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 697 bytes --]

On Wed, 9 Aug 2023, Luca Fancellu wrote:
> > On 9 Aug 2023, at 12:02, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote:
> > 
> > The function 'ioreq_server_max_frames' can be defined static,
> > as its only uses are within the same file. This in turn avoids
> > violating Rule 8.4 because no declaration is present.
> > 
> > No functional change.
> > 
> > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> 
> Makes sense,
> 
> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

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


> Maybe it’s also better adding this:
> Fixes: 9244528955de ("xen/memory: Fix acquire_resource size semantics”)
> 
> If the maintainers agree

Yes

[XEN PATCH 3/8] xen: address MISRA C:2012 Rule 8.4

[Nicola Vetrini]

The variable 'saved_cmdline' can be defined static,
as its only uses are within the same file. This in turn avoids
violating Rule 8.4 because no declaration is present.

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

diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index fb919f3d9c..52aa287627 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -28,7 +28,7 @@ CHECK_feature_info;
 
 enum system_state system_state = SYS_STATE_early_boot;
 
-xen_commandline_t saved_cmdline;
+static xen_commandline_t saved_cmdline;
 static const char __initconst opt_builtin_cmdline[] = CONFIG_CMDLINE;
 
 static int assign_integer_param(const struct kernel_param *param, uint64_t val)
-- 
2.34.1

Re: [XEN PATCH 3/8] xen: address MISRA C:2012 Rule 8.4

[Luca Fancellu]

> On 9 Aug 2023, at 12:02, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote:
> 
> The variable 'saved_cmdline' can be defined static,
> as its only uses are within the same file. This in turn avoids
> violating Rule 8.4 because no declaration is present.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
> xen/common/kernel.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/common/kernel.c b/xen/common/kernel.c
> index fb919f3d9c..52aa287627 100644
> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -28,7 +28,7 @@ CHECK_feature_info;
> 
> enum system_state system_state = SYS_STATE_early_boot;
> 
> -xen_commandline_t saved_cmdline;
> +static xen_commandline_t saved_cmdline;

I see this line was touched by fa97833ae18e4a42c0e5ba4e781173457b5d3397,
have you checked that making it static was not affecting anything else?


> static const char __initconst opt_builtin_cmdline[] = CONFIG_CMDLINE;
> 
> static int assign_integer_param(const struct kernel_param *param, uint64_t val)
> -- 
> 2.34.1
> 
> 

Re: [XEN PATCH 3/8] xen: address MISRA C:2012 Rule 8.4

[Jan Beulich]

On 09.08.2023 15:50, Luca Fancellu wrote:
>> On 9 Aug 2023, at 12:02, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote:
>>
>> The variable 'saved_cmdline' can be defined static,
>> as its only uses are within the same file. This in turn avoids
>> violating Rule 8.4 because no declaration is present.
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>> xen/common/kernel.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/common/kernel.c b/xen/common/kernel.c
>> index fb919f3d9c..52aa287627 100644
>> --- a/xen/common/kernel.c
>> +++ b/xen/common/kernel.c
>> @@ -28,7 +28,7 @@ CHECK_feature_info;
>>
>> enum system_state system_state = SYS_STATE_early_boot;
>>
>> -xen_commandline_t saved_cmdline;
>> +static xen_commandline_t saved_cmdline;
> 
> I see this line was touched by fa97833ae18e4a42c0e5ba4e781173457b5d3397,
> have you checked that making it static was not affecting anything else?

The code requiring the symbol to be non-static went away in e6ee01ad24b6
("xen/version: Drop compat/kernel.c"). That's where the symbol would have
wanted to become static. But that was very easy to overlook.

Jan

Re: [XEN PATCH 3/8] xen: address MISRA C:2012 Rule 8.4

[Luca Fancellu]

> On 9 Aug 2023, at 15:06, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 09.08.2023 15:50, Luca Fancellu wrote:
>>> On 9 Aug 2023, at 12:02, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote:
>>> 
>>> The variable 'saved_cmdline' can be defined static,
>>> as its only uses are within the same file. This in turn avoids
>>> violating Rule 8.4 because no declaration is present.
>>> 
>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>> ---
>>> xen/common/kernel.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/xen/common/kernel.c b/xen/common/kernel.c
>>> index fb919f3d9c..52aa287627 100644
>>> --- a/xen/common/kernel.c
>>> +++ b/xen/common/kernel.c
>>> @@ -28,7 +28,7 @@ CHECK_feature_info;
>>> 
>>> enum system_state system_state = SYS_STATE_early_boot;
>>> 
>>> -xen_commandline_t saved_cmdline;
>>> +static xen_commandline_t saved_cmdline;
>> 
>> I see this line was touched by fa97833ae18e4a42c0e5ba4e781173457b5d3397,
>> have you checked that making it static was not affecting anything else?
> 
> The code requiring the symbol to be non-static went away in e6ee01ad24b6
> ("xen/version: Drop compat/kernel.c"). That's where the symbol would have
> wanted to become static. But that was very easy to overlook.

Yes you are right Jan,

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

Possibly with the Fixes tag

Cheers,
Luca

Re: [XEN PATCH 3/8] xen: address MISRA C:2012 Rule 8.4

[Stefano Stabellini]

On Wed, 9 Aug 2023, Luca Fancellu wrote:
> > On 9 Aug 2023, at 15:06, Jan Beulich <jbeulich@suse.com> wrote:
> > 
> > On 09.08.2023 15:50, Luca Fancellu wrote:
> >>> On 9 Aug 2023, at 12:02, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote:
> >>> 
> >>> The variable 'saved_cmdline' can be defined static,
> >>> as its only uses are within the same file. This in turn avoids
> >>> violating Rule 8.4 because no declaration is present.
> >>> 
> >>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> >>> ---
> >>> xen/common/kernel.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>> 
> >>> diff --git a/xen/common/kernel.c b/xen/common/kernel.c
> >>> index fb919f3d9c..52aa287627 100644
> >>> --- a/xen/common/kernel.c
> >>> +++ b/xen/common/kernel.c
> >>> @@ -28,7 +28,7 @@ CHECK_feature_info;
> >>> 
> >>> enum system_state system_state = SYS_STATE_early_boot;
> >>> 
> >>> -xen_commandline_t saved_cmdline;
> >>> +static xen_commandline_t saved_cmdline;
> >> 
> >> I see this line was touched by fa97833ae18e4a42c0e5ba4e781173457b5d3397,
> >> have you checked that making it static was not affecting anything else?
> > 
> > The code requiring the symbol to be non-static went away in e6ee01ad24b6
> > ("xen/version: Drop compat/kernel.c"). That's where the symbol would have
> > wanted to become static. But that was very easy to overlook.
> 
> Yes you are right Jan,
> 
> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
> 
> Possibly with the Fixes tag

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

Re: [XEN PATCH 3/8] xen: address MISRA C:2012 Rule 8.4

[Nicola Vetrini]

On 09/08/2023 15:50, Luca Fancellu wrote:
>> On 9 Aug 2023, at 12:02, Nicola Vetrini <nicola.vetrini@bugseng.com> 
>> wrote:
>> 
>> The variable 'saved_cmdline' can be defined static,
>> as its only uses are within the same file. This in turn avoids
>> violating Rule 8.4 because no declaration is present.
>> 
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>> xen/common/kernel.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/xen/common/kernel.c b/xen/common/kernel.c
>> index fb919f3d9c..52aa287627 100644
>> --- a/xen/common/kernel.c
>> +++ b/xen/common/kernel.c
>> @@ -28,7 +28,7 @@ CHECK_feature_info;
>> 
>> enum system_state system_state = SYS_STATE_early_boot;
>> 
>> -xen_commandline_t saved_cmdline;
>> +static xen_commandline_t saved_cmdline;
> 
> I see this line was touched by 
> fa97833ae18e4a42c0e5ba4e781173457b5d3397,
> have you checked that making it static was not affecting anything else?
> 
> 

Though Jan already replied on this, the commit(s) were tested by patchew 
and our
pipeline. This is normally our process, apart from MISRA checks.

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

[XEN PATCH 4/8] xen/arm: address MISRA C:2012 Rule 8.4

[Nicola Vetrini]

'xen/hypercall.h' is included in 'xen/arch/arm/setup.c' to allow
the declaration of 'arch_get_xen_caps' to be visible when
defining the function.

The header 'xen/delay.h' is included in 'xen/arch/arm/time.c'
to allow the declaration of 'udelay' to be visible.

At the same time, a declaration for 'get_sec' is added in
'xen/include/xen/time.h' to be available for every call site
(in particular cper.h).

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

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index bbf72b69aa..44ccea03ca 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -32,6 +32,7 @@
 #include <xen/libfdt/libfdt-xen.h>
 #include <xen/acpi.h>
 #include <xen/warning.h>
+#include <xen/hypercall.h>
 #include <asm/alternative.h>
 #include <asm/page.h>
 #include <asm/current.h>
diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index 0b482d7db3..3535bd8ac7 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -17,6 +17,7 @@
 #include <xen/softirq.h>
 #include <xen/sched.h>
 #include <xen/time.h>
+#include <xen/delay.h>
 #include <xen/sched.h>
 #include <xen/event.h>
 #include <xen/acpi.h>
diff --git a/xen/include/xen/cper.h b/xen/include/xen/cper.h
index 7c6a4c45ce..de8f385bdd 100644
--- a/xen/include/xen/cper.h
+++ b/xen/include/xen/cper.h
@@ -23,8 +23,7 @@
 
 #include <xen/types.h>
 #include <xen/string.h>
-
-extern unsigned long get_sec(void);
+#include <xen/time.h>
 
 typedef struct {
 	uint8_t b[16];
diff --git a/xen/include/xen/time.h b/xen/include/xen/time.h
index 5aafdda4f3..67c586b736 100644
--- a/xen/include/xen/time.h
+++ b/xen/include/xen/time.h
@@ -36,6 +36,7 @@ s_time_t get_s_time_fixed(u64 at_tick);
 s_time_t get_s_time(void);
 unsigned long get_localtime(struct domain *d);
 uint64_t get_localtime_us(struct domain *d);
+unsigned long get_sec(void);
 
 struct tm {
     int     tm_sec;         /* seconds */
-- 
2.34.1

Re: [XEN PATCH 4/8] xen/arm: address MISRA C:2012 Rule 8.4

[Jan Beulich]

On 09.08.2023 13:02, Nicola Vetrini wrote:
> 'xen/hypercall.h' is included in 'xen/arch/arm/setup.c' to allow
> the declaration of 'arch_get_xen_caps' to be visible when
> defining the function.
> 
> The header 'xen/delay.h' is included in 'xen/arch/arm/time.c'
> to allow the declaration of 'udelay' to be visible.
> 
> At the same time, a declaration for 'get_sec' is added in
> 'xen/include/xen/time.h' to be available for every call site
> (in particular cper.h).
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
>  xen/arch/arm/setup.c   | 1 +
>  xen/arch/arm/time.c    | 1 +
>  xen/include/xen/cper.h | 3 +--
>  xen/include/xen/time.h | 1 +
>  4 files changed, 4 insertions(+), 2 deletions(-)

I would have almost put this off as Arm-only, but then saw this diffstat.
How come you consider cper.h Arm-related? This is used only by APEI source
files, which in turn are used only by x86 afaics. So I think you want to
split off the movement of the get_sec() declaration.

As to title and description (perhaps affecting more than just this patch):
Failing to have declarations in scope where definitions appear is an at
least latent bug. We fix these as we find them anyway, so Misra is
secondary here. Hence I'd like to suggest that you declare any such
change as an actual bugfix, ideally including a Fixes: tag. It is of
course fine to mention that this then also addresses Misra rule 8.4.

Jan

Re: [XEN PATCH 4/8] xen/arm: address MISRA C:2012 Rule 8.4

[Luca Fancellu]

> On 9 Aug 2023, at 13:42, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 09.08.2023 13:02, Nicola Vetrini wrote:
>> 'xen/hypercall.h' is included in 'xen/arch/arm/setup.c' to allow
>> the declaration of 'arch_get_xen_caps' to be visible when
>> defining the function.
>> 
>> The header 'xen/delay.h' is included in 'xen/arch/arm/time.c'
>> to allow the declaration of 'udelay' to be visible.
>> 
>> At the same time, a declaration for 'get_sec' is added in
>> 'xen/include/xen/time.h' to be available for every call site
>> (in particular cper.h).
>> 
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>> xen/arch/arm/setup.c   | 1 +
>> xen/arch/arm/time.c    | 1 +
>> xen/include/xen/cper.h | 3 +--
>> xen/include/xen/time.h | 1 +
>> 4 files changed, 4 insertions(+), 2 deletions(-)
> 
> I would have almost put this off as Arm-only, but then saw this diffstat.
> How come you consider cper.h Arm-related? This is used only by APEI source
> files, which in turn are used only by x86 afaics. So I think you want to
> split off the movement of the get_sec() declaration.
> 
> As to title and description (perhaps affecting more than just this patch):
> Failing to have declarations in scope where definitions appear is an at
> least latent bug. We fix these as we find them anyway, so Misra is
> secondary here. Hence I'd like to suggest that you declare any such
> change as an actual bugfix, ideally including a Fixes: tag.

To prevent back and forth I would suggest also to have a look in sending-patches.pandoc,
### Fixes section, on the expected syntax for the tag

> It is of
> course fine to mention that this then also addresses Misra rule 8.4.
> 
> Jan
> 

Re: [XEN PATCH 4/8] xen/arm: address MISRA C:2012 Rule 8.4

[Nicola Vetrini]

> To prevent back and forth I would suggest also to have a look in
> sending-patches.pandoc,
> ### Fixes section, on the expected syntax for the tag
> 

Thanks

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

Re: [XEN PATCH 4/8] xen/arm: address MISRA C:2012 Rule 8.4

[Nicola Vetrini]

On 09/08/2023 14:42, Jan Beulich wrote:
> On 09.08.2023 13:02, Nicola Vetrini wrote:
>> 'xen/hypercall.h' is included in 'xen/arch/arm/setup.c' to allow
>> the declaration of 'arch_get_xen_caps' to be visible when
>> defining the function.
>> 
>> The header 'xen/delay.h' is included in 'xen/arch/arm/time.c'
>> to allow the declaration of 'udelay' to be visible.
>> 
>> At the same time, a declaration for 'get_sec' is added in
>> 'xen/include/xen/time.h' to be available for every call site
>> (in particular cper.h).
>> 
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>>  xen/arch/arm/setup.c   | 1 +
>>  xen/arch/arm/time.c    | 1 +
>>  xen/include/xen/cper.h | 3 +--
>>  xen/include/xen/time.h | 1 +
>>  4 files changed, 4 insertions(+), 2 deletions(-)
> 
> I would have almost put this off as Arm-only, but then saw this 
> diffstat.
> How come you consider cper.h Arm-related? This is used only by APEI 
> source
> files, which in turn are used only by x86 afaics. So I think you want 
> to
> split off the movement of the get_sec() declaration.
> 

My mistake, I squashed the arm and the cper part together to avoid 
touching the
time headers twice, but the old tag remained.

> As to title and description (perhaps affecting more than just this 
> patch):
> Failing to have declarations in scope where definitions appear is an at
> least latent bug. We fix these as we find them anyway, so Misra is
> secondary here. Hence I'd like to suggest that you declare any such
> change as an actual bugfix, ideally including a Fixes: tag. It is of
> course fine to mention that this then also addresses Misra rule 8.4.
> 
> Jan

Ok

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

Re: [XEN PATCH 4/8] xen/arm: address MISRA C:2012 Rule 8.4

[Stefano Stabellini]

On Wed, 9 Aug 2023, Jan Beulich wrote:
> On 09.08.2023 13:02, Nicola Vetrini wrote:
> > 'xen/hypercall.h' is included in 'xen/arch/arm/setup.c' to allow
> > the declaration of 'arch_get_xen_caps' to be visible when
> > defining the function.
> > 
> > The header 'xen/delay.h' is included in 'xen/arch/arm/time.c'
> > to allow the declaration of 'udelay' to be visible.
> > 
> > At the same time, a declaration for 'get_sec' is added in
> > 'xen/include/xen/time.h' to be available for every call site
> > (in particular cper.h).
> > 
> > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> > ---
> >  xen/arch/arm/setup.c   | 1 +
> >  xen/arch/arm/time.c    | 1 +
> >  xen/include/xen/cper.h | 3 +--
> >  xen/include/xen/time.h | 1 +
> >  4 files changed, 4 insertions(+), 2 deletions(-)
> 
> I would have almost put this off as Arm-only, but then saw this diffstat.
> How come you consider cper.h Arm-related? This is used only by APEI source
> files, which in turn are used only by x86 afaics. So I think you want to
> split off the movement of the get_sec() declaration.
> 
> As to title and description (perhaps affecting more than just this patch):
> Failing to have declarations in scope where definitions appear is an at
> least latent bug. We fix these as we find them anyway, so Misra is
> secondary here. Hence I'd like to suggest that you declare any such
> change as an actual bugfix, ideally including a Fixes: tag. It is of
> course fine to mention that this then also addresses Misra rule 8.4.

As you split the patches moving cper.h out, and fixing the commit
message, please add my Reviewed-by for the setup.c/time.c/time.h
changes.

[XEN PATCH 5/8] x86: address MISRA C:2012 Rule 8.4

[Nicola Vetrini]

The functions and variables touched by this commit can be static,
as they are only used within the same unit. This in turn addresses
the absence of declarations that violates Rule 8.4.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/cpu/mcheck/mce.c     | 2 +-
 xen/arch/x86/cpu/mcheck/mce_amd.c | 2 +-
 xen/arch/x86/cpu/microcode/core.c | 2 +-
 xen/arch/x86/nmi.c                | 2 +-
 xen/arch/x86/spec_ctrl.c          | 4 ++--
 5 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
index 57e1eb221e..6141b7eb9c 100644
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -599,7 +599,7 @@ unsigned int mce_firstbank(struct cpuinfo_x86 *c)
            c->x86_vendor == X86_VENDOR_INTEL && c->x86_model < 0x1a;
 }
 
-int show_mca_info(int inited, struct cpuinfo_x86 *c)
+static int show_mca_info(int inited, struct cpuinfo_x86 *c)
 {
     static enum mcheck_type g_type = mcheck_unset;
 
diff --git a/xen/arch/x86/cpu/mcheck/mce_amd.c b/xen/arch/x86/cpu/mcheck/mce_amd.c
index adc36522cc..72f3feeaee 100644
--- a/xen/arch/x86/cpu/mcheck/mce_amd.c
+++ b/xen/arch/x86/cpu/mcheck/mce_amd.c
@@ -87,7 +87,7 @@ enum mc_ec_type {
     MC_EC_BUS_TYPE = 0x0800,
 };
 
-enum mc_ec_type
+static enum mc_ec_type
 mc_ec2type(uint16_t errorcode)
 {
     if ( errorcode & MC_EC_BUS_TYPE )
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index bec8b55db2..9fcb9c1c3a 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -149,7 +149,7 @@ static int __init cf_check parse_ucode(const char *s)
 }
 custom_param("ucode", parse_ucode);
 
-void __init microcode_scan_module(
+static void __init microcode_scan_module(
     unsigned long *module_map,
     const multiboot_info_t *mbi)
 {
diff --git a/xen/arch/x86/nmi.c b/xen/arch/x86/nmi.c
index c5c28300b0..104e366bd3 100644
--- a/xen/arch/x86/nmi.c
+++ b/xen/arch/x86/nmi.c
@@ -108,7 +108,7 @@ static unsigned int lapic_nmi_owner;
  *     be enabled
  * -1: the lapic NMI watchdog is disabled, but can be enabled
  */
-int nmi_active;
+static int nmi_active;
 
 #define K7_EVNTSEL_ENABLE	(1 << 22)
 #define K7_EVNTSEL_INT		(1 << 20)
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index ebe56a96cd..9b8fdb5303 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -43,9 +43,9 @@ static enum ind_thunk {
 } opt_thunk __initdata = THUNK_DEFAULT;
 
 static int8_t __initdata opt_ibrs = -1;
-int8_t __initdata opt_stibp = -1;
+static int8_t __initdata opt_stibp = -1;
 bool __ro_after_init opt_ssbd;
-int8_t __initdata opt_psfd = -1;
+static int8_t __initdata opt_psfd = -1;
 
 int8_t __ro_after_init opt_ibpb_ctxt_switch = -1;
 int8_t __read_mostly opt_eager_fpu = -1;
-- 
2.34.1

Re: [XEN PATCH 5/8] x86: address MISRA C:2012 Rule 8.4

[Jan Beulich]

On 09.08.2023 13:02, Nicola Vetrini wrote:
> The functions and variables touched by this commit can be static,
> as they are only used within the same unit. This in turn addresses
> the absence of declarations that violates Rule 8.4.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

I'm happy with the code changes here and also in patch 3, but please
make the titles meaningful (in patch 3 the prefix also wants to be
more specific). As mentioned, the Misra rule is secondary here, and
hence doesn't need to be present in the titles.

Jan

[XEN PATCH 6/8] xen/arm: mm: address MISRA C:2012 Rule 8.4

[Nicola Vetrini]

Add a declaration for the variable 'init_ttbr' to resolve
the violation of Rule 8.4 present in the associated source file 'mm.c'.

No functional changes.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/arm/include/asm/mm.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
index 5b530f0f40..698e54aff0 100644
--- a/xen/arch/arm/include/asm/mm.h
+++ b/xen/arch/arm/include/asm/mm.h
@@ -165,6 +165,9 @@ struct page_info
 #define _PGC_need_scrub   _PGC_allocated
 #define PGC_need_scrub    PGC_allocated
 
+/* Non-boot CPUs use this to find the correct pagetables. */
+extern uint64_t init_ttbr;
+
 extern mfn_t directmap_mfn_start, directmap_mfn_end;
 extern vaddr_t directmap_virt_end;
 #ifdef CONFIG_ARM_64
-- 
2.34.1

Re: [XEN PATCH 6/8] xen/arm: mm: address MISRA C:2012 Rule 8.4

[Luca Fancellu]

> On 9 Aug 2023, at 12:02, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote:
> 
> Add a declaration for the variable 'init_ttbr' to resolve
> the violation of Rule 8.4 present in the associated source file 'mm.c'.
> 
> No functional changes.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

With:

Fixes: 4557c2292854 ("xen: arm: rewrite start of day page table and cpu bring up")

If maintainers are ok

> xen/arch/arm/include/asm/mm.h | 3 +++
> 1 file changed, 3 insertions(+)
> 
> diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
> index 5b530f0f40..698e54aff0 100644
> --- a/xen/arch/arm/include/asm/mm.h
> +++ b/xen/arch/arm/include/asm/mm.h
> @@ -165,6 +165,9 @@ struct page_info
> #define _PGC_need_scrub   _PGC_allocated
> #define PGC_need_scrub    PGC_allocated
> 
> +/* Non-boot CPUs use this to find the correct pagetables. */
> +extern uint64_t init_ttbr;
> +
> extern mfn_t directmap_mfn_start, directmap_mfn_end;
> extern vaddr_t directmap_virt_end;
> #ifdef CONFIG_ARM_64
> -- 
> 2.34.1
> 
> 

Re: [XEN PATCH 6/8] xen/arm: mm: address MISRA C:2012 Rule 8.4

[Stefano Stabellini]

On Wed, 9 Aug 2023, Luca Fancellu wrote:
> > On 9 Aug 2023, at 12:02, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote:
> > 
> > Add a declaration for the variable 'init_ttbr' to resolve
> > the violation of Rule 8.4 present in the associated source file 'mm.c'.
> > 
> > No functional changes.
> > 
> > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> > ---
> 
> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
> 
> With:
> 
> Fixes: 4557c2292854 ("xen: arm: rewrite start of day page table and cpu bring up")
> 
> If maintainers are ok

Yes

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

[XEN PATCH 7/8] x86/i8259: address MISRA C:2012 Rule 8.4

[Nicola Vetrini]

The additional header file makes the declaration for the function
'init_IRQ', defined in this file visible, thereby resolving the
violation of Rule 8.4.

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 6b35be10f0..9b02a3a0ae 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

Re: [XEN PATCH 7/8] x86/i8259: address MISRA C:2012 Rule 8.4

[Jan Beulich]

On 09.08.2023 13:02, Nicola Vetrini wrote:
> The additional header file makes the declaration for the function
> 'init_IRQ', defined in this file visible, thereby resolving the
> violation of Rule 8.4.
> 
> 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 6b35be10f0..9b02a3a0ae 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>

A patch adding this #include has been pending for almost 3 months:
https://lists.xen.org/archives/html/xen-devel/2023-05/msg00896.html

Jan

Re: [XEN PATCH 7/8] x86/i8259: address MISRA C:2012 Rule 8.4

[Nicola Vetrini]

On 09/08/2023 14:52, Jan Beulich wrote:
> On 09.08.2023 13:02, Nicola Vetrini wrote:
>> The additional header file makes the declaration for the function
>> 'init_IRQ', defined in this file visible, thereby resolving the
>> violation of Rule 8.4.
>> 
>> 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 6b35be10f0..9b02a3a0ae 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>
> 
> A patch adding this #include has been pending for almost 3 months:
> https://lists.xen.org/archives/html/xen-devel/2023-05/msg00896.html
> 
> Jan

So do you prefer going forward with that patch or this one (mentioning 
it
in the additional commit context of course)?

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

Re: [XEN PATCH 7/8] x86/i8259: address MISRA C:2012 Rule 8.4

[Jan Beulich]

On 09.08.2023 16:17, Nicola Vetrini wrote:
> On 09/08/2023 14:52, Jan Beulich wrote:
>> On 09.08.2023 13:02, Nicola Vetrini wrote:
>>> The additional header file makes the declaration for the function
>>> 'init_IRQ', defined in this file visible, thereby resolving the
>>> violation of Rule 8.4.
>>>
>>> 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 6b35be10f0..9b02a3a0ae 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>
>>
>> A patch adding this #include has been pending for almost 3 months:
>> https://lists.xen.org/archives/html/xen-devel/2023-05/msg00896.html
> 
> So do you prefer going forward with that patch or this one (mentioning 
> it
> in the additional commit context of course)?

I would prefer using the much older patch, but of course this requires
someone providing R-b or A-b.

Jan

Re: [XEN PATCH 7/8] x86/i8259: address MISRA C:2012 Rule 8.4

[Stefano Stabellini]

On Wed, 9 Aug 2023, Jan Beulich wrote:
> On 09.08.2023 16:17, Nicola Vetrini wrote:
> > On 09/08/2023 14:52, Jan Beulich wrote:
> >> On 09.08.2023 13:02, Nicola Vetrini wrote:
> >>> The additional header file makes the declaration for the function
> >>> 'init_IRQ', defined in this file visible, thereby resolving the
> >>> violation of Rule 8.4.
> >>>
> >>> 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 6b35be10f0..9b02a3a0ae 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>
> >>
> >> A patch adding this #include has been pending for almost 3 months:
> >> https://lists.xen.org/archives/html/xen-devel/2023-05/msg00896.html
> > 
> > So do you prefer going forward with that patch or this one (mentioning 
> > it
> > in the additional commit context of course)?
> 
> I would prefer using the much older patch, but of course this requires
> someone providing R-b or A-b.

Hi Jan, normally I'd be happy to do that but that patch makes other
changes that I don't feel confident enough to Ack.

For instance:

+    for ( offs = 0, i = pic_alias_mask & -pic_alias_mask ?: 2;
+          offs <= pic_alias_mask; offs += i )

pic_alias_mask is declared as unsigned int.

Re: [XEN PATCH 7/8] x86/i8259: address MISRA C:2012 Rule 8.4

[Jan Beulich]

On 09.08.2023 22:15, Stefano Stabellini wrote:
> On Wed, 9 Aug 2023, Jan Beulich wrote:
>> On 09.08.2023 16:17, Nicola Vetrini wrote:
>>> On 09/08/2023 14:52, Jan Beulich wrote:
>>>> On 09.08.2023 13:02, Nicola Vetrini wrote:
>>>>> The additional header file makes the declaration for the function
>>>>> 'init_IRQ', defined in this file visible, thereby resolving the
>>>>> violation of Rule 8.4.
>>>>>
>>>>> 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 6b35be10f0..9b02a3a0ae 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>
>>>>
>>>> A patch adding this #include has been pending for almost 3 months:
>>>> https://lists.xen.org/archives/html/xen-devel/2023-05/msg00896.html
>>>
>>> So do you prefer going forward with that patch or this one (mentioning 
>>> it
>>> in the additional commit context of course)?
>>
>> I would prefer using the much older patch, but of course this requires
>> someone providing R-b or A-b.
> 
> Hi Jan, normally I'd be happy to do that but that patch makes other
> changes that I don't feel confident enough to Ack.

I understand that in general, but for this specific case ...

> For instance:
> 
> +    for ( offs = 0, i = pic_alias_mask & -pic_alias_mask ?: 2;
> +          offs <= pic_alias_mask; offs += i )
> 
> pic_alias_mask is declared as unsigned int.

... you're concerned of me negating it? That's a common pattern to determine
the largest power-of-2 factor. I'm unaware of a good alternative.

Jan

[XEN PATCH 8/8] x86/nmi: address MISRA C:2012 Rule 8.4

[Nicola Vetrini]

Include an additional header to make the declarations for
functions 'watchdog_*' visible prior to their definition in
the file, thereby resolving the violations of Rule 8.4.

No functional change.

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

diff --git a/xen/arch/x86/nmi.c b/xen/arch/x86/nmi.c
index 104e366bd3..dc79c25e3f 100644
--- a/xen/arch/x86/nmi.c
+++ b/xen/arch/x86/nmi.c
@@ -24,6 +24,7 @@
 #include <xen/console.h>
 #include <xen/smp.h>
 #include <xen/keyhandler.h>
+#include <xen/watchdog.h>
 #include <xen/cpu.h>
 #include <asm/current.h>
 #include <asm/mc146818rtc.h>
-- 
2.34.1

Re: [XEN PATCH 8/8] x86/nmi: address MISRA C:2012 Rule 8.4

[Jan Beulich]

On 09.08.2023 13:02, Nicola Vetrini wrote:
> Include an additional header to make the declarations for
> functions 'watchdog_*' visible prior to their definition in
> the file, thereby resolving the violations of Rule 8.4.
> 
> No functional change.

Fixes: c8177e691f0f ("watchdog: Move watchdog from being x86 specific to common code")

> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

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