[XEN PATCH v2 0/3] xen: fix inclusions and static storage duration

[XEN PATCH v2 0/3] xen: fix inclusions and static storage duration

[Nicola Vetrini]

The files touched by this series contain function or variable definitions with
no prior declaration visible, because it's inside an header that is not included
or it's not present anywhere. This is a risk in itself, but also violates
MISRA C:2012 Rule 8.4, which states the following:
"A compatible declaration shall be visible when an object or function with
external linkage is defined".

The resolution strategies are as follows:

1. make the functions/variables static
2. include the header that contains the compatible declaration

No functional change.

Additional notes:
- This series is a continuation of the work done here [1], so the same additional
  notes apply.

[1] https://lore.kernel.org/xen-devel/cover.1691655814.git.nicola.vetrini@bugseng.com/T/#m28da1b5ef8d9a7683937bfc345765e3438b89977

Changes in v2:
- Addressed review comments on the leftover patches
- Patches 1,2,4 from the previous version have already been committed

Nicola Vetrini (3):
  vm_event: rework inclusions to use arch-indipendent header
  vpci/msix: make 'get_slot' static
  drivers/video: make declarations of defined functions available

 xen/arch/arm/include/asm/vm_event.h |  1 -
 xen/arch/arm/vm_event.c             |  2 +-
 xen/arch/x86/include/asm/setup.h    |  6 ------
 xen/arch/x86/platform_hypercall.c   |  2 +-
 xen/arch/x86/pv/dom0_build.c        |  2 +-
 xen/arch/x86/vm_event.c             |  2 +-
 xen/drivers/video/vga.c             |  8 --------
 xen/drivers/vpci/msix.c             |  2 +-
 xen/include/xen/console.h           |  2 --
 xen/include/xen/vga.h               | 16 ++++++++++++++++
 xen/include/xen/vm_event.h          |  1 +
 11 files changed, 22 insertions(+), 22 deletions(-)

--
2.34.1

[XEN PATCH v2 1/3] vm_event: rework inclusions to use arch-indipendent header

[Nicola Vetrini]

The arch-specific header <asm/vm_event.h> should be included by the
common header <xen/vm_event.h>, so that the latter can be included
in the source files.

This also resolves violations of MISRA C:2012 Rule 8.4 that were
caused by declarations for
'vm_event_{fill_regs,set_registers,monitor_next_interrupt}'
in <asm/vm_event.h> not being visible when
defining functions in 'xen/arch/x86/vm_event.c'

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Changes in v2:
- Include the arch-specific header in the common one, and only include
  the latter in source files.

The following functions have been mainly touched by the following commits,
but the present commit does not solve a problem introduced by one of them per se, except perhaps the first one mentioned, which is why I didn't put a Fixes tag in this v2:
- 975efd3baa8d ("introduce VM_EVENT_FLAG_SET_REGISTERS")
- adc75eba8b15 ("x86/vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs")
- 9864841914c2 ("x86/vm_event: add support for VM_EVENT_REASON_INTERRUPT")
---
 xen/arch/arm/include/asm/vm_event.h | 1 -
 xen/arch/arm/vm_event.c             | 2 +-
 xen/arch/x86/vm_event.c             | 2 +-
 xen/include/xen/vm_event.h          | 1 +
 4 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/include/asm/vm_event.h b/xen/arch/arm/include/asm/vm_event.h
index abe7db1970ca..4d861373b38d 100644
--- a/xen/arch/arm/include/asm/vm_event.h
+++ b/xen/arch/arm/include/asm/vm_event.h
@@ -20,7 +20,6 @@
 #define __ASM_ARM_VM_EVENT_H__
 
 #include <xen/sched.h>
-#include <xen/vm_event.h>
 #include <public/domctl.h>
 
 static inline int vm_event_init_domain(struct domain *d)
diff --git a/xen/arch/arm/vm_event.c b/xen/arch/arm/vm_event.c
index ba99f56eb20c..ccfd25bbbca9 100644
--- a/xen/arch/arm/vm_event.c
+++ b/xen/arch/arm/vm_event.c
@@ -8,7 +8,7 @@
  */
 
 #include <xen/sched.h>
-#include <asm/vm_event.h>
+#include <xen/vm_event.h>
 
 void vm_event_fill_regs(vm_event_request_t *req)
 {
diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
index 7027c08a926b..e6c7ad5337dd 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -20,7 +20,7 @@
 
 #include <xen/sched.h>
 #include <xen/mem_access.h>
-#include <asm/vm_event.h>
+#include <xen/vm_event.h>
 
 /* Implicitly serialized by the domctl lock. */
 int vm_event_init_domain(struct domain *d)
diff --git a/xen/include/xen/vm_event.h b/xen/include/xen/vm_event.h
index 92811d9110e5..9a86358b42ae 100644
--- a/xen/include/xen/vm_event.h
+++ b/xen/include/xen/vm_event.h
@@ -25,6 +25,7 @@
 
 #include <xen/sched.h>
 #include <public/vm_event.h>
+#include <asm/vm_event.h>
 
 struct vm_event_domain
 {
-- 
2.34.1

Re: [XEN PATCH v2 1/3] vm_event: rework inclusions to use arch-indipendent header

[Stefano Stabellini]

On Thu, 17 Aug 2023, Nicola Vetrini wrote:
> The arch-specific header <asm/vm_event.h> should be included by the
> common header <xen/vm_event.h>, so that the latter can be included
> in the source files.
> 
> This also resolves violations of MISRA C:2012 Rule 8.4 that were
> caused by declarations for
> 'vm_event_{fill_regs,set_registers,monitor_next_interrupt}'
> in <asm/vm_event.h> not being visible when
> defining functions in 'xen/arch/x86/vm_event.c'
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

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


> ---
> Changes in v2:
> - Include the arch-specific header in the common one, and only include
>   the latter in source files.
> 
> The following functions have been mainly touched by the following commits,
> but the present commit does not solve a problem introduced by one of them per se, except perhaps the first one mentioned, which is why I didn't put a Fixes tag in this v2:
> - 975efd3baa8d ("introduce VM_EVENT_FLAG_SET_REGISTERS")
> - adc75eba8b15 ("x86/vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs")
> - 9864841914c2 ("x86/vm_event: add support for VM_EVENT_REASON_INTERRUPT")
> ---
>  xen/arch/arm/include/asm/vm_event.h | 1 -
>  xen/arch/arm/vm_event.c             | 2 +-
>  xen/arch/x86/vm_event.c             | 2 +-
>  xen/include/xen/vm_event.h          | 1 +
>  4 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/vm_event.h b/xen/arch/arm/include/asm/vm_event.h
> index abe7db1970ca..4d861373b38d 100644
> --- a/xen/arch/arm/include/asm/vm_event.h
> +++ b/xen/arch/arm/include/asm/vm_event.h
> @@ -20,7 +20,6 @@
>  #define __ASM_ARM_VM_EVENT_H__
>  
>  #include <xen/sched.h>
> -#include <xen/vm_event.h>
>  #include <public/domctl.h>
>  
>  static inline int vm_event_init_domain(struct domain *d)
> diff --git a/xen/arch/arm/vm_event.c b/xen/arch/arm/vm_event.c
> index ba99f56eb20c..ccfd25bbbca9 100644
> --- a/xen/arch/arm/vm_event.c
> +++ b/xen/arch/arm/vm_event.c
> @@ -8,7 +8,7 @@
>   */
>  
>  #include <xen/sched.h>
> -#include <asm/vm_event.h>
> +#include <xen/vm_event.h>
>  
>  void vm_event_fill_regs(vm_event_request_t *req)
>  {
> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
> index 7027c08a926b..e6c7ad5337dd 100644
> --- a/xen/arch/x86/vm_event.c
> +++ b/xen/arch/x86/vm_event.c
> @@ -20,7 +20,7 @@
>  
>  #include <xen/sched.h>
>  #include <xen/mem_access.h>
> -#include <asm/vm_event.h>
> +#include <xen/vm_event.h>
>  
>  /* Implicitly serialized by the domctl lock. */
>  int vm_event_init_domain(struct domain *d)
> diff --git a/xen/include/xen/vm_event.h b/xen/include/xen/vm_event.h
> index 92811d9110e5..9a86358b42ae 100644
> --- a/xen/include/xen/vm_event.h
> +++ b/xen/include/xen/vm_event.h
> @@ -25,6 +25,7 @@
>  
>  #include <xen/sched.h>
>  #include <public/vm_event.h>
> +#include <asm/vm_event.h>
>  
>  struct vm_event_domain
>  {
> -- 
> 2.34.1
> 

[XEN PATCH v2 2/3] vpci/msix: make 'get_slot' static

[Nicola Vetrini]

The function can become static since it's used only within this file.
This also resolves a violation of MISRA C:2012 Rule 8.4 due to the absence
of a declaration before the function definition.

Fixes: b177892d2d0e ("vpci/msix: handle accesses adjacent to the MSI-X table")
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
Changes in v2:
- Corrected wrong prefix in the commit subject.

CC-ing maintainers from "THE REST" as well, perhaps this trivial change can go
in straight away.
---
 xen/drivers/vpci/msix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
index 25bde77586a4..f9df506f29bf 100644
--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -223,7 +223,7 @@ static void __iomem *get_table(const struct vpci *vpci, unsigned int slot)
     return msix->table[slot];
 }

-unsigned int get_slot(const struct vpci *vpci, unsigned long addr)
+static unsigned int get_slot(const struct vpci *vpci, unsigned long addr)
 {
     unsigned long pfn = PFN_DOWN(addr);

--
2.34.1

Re: [XEN PATCH v2 2/3] vpci/msix: make 'get_slot' static

[Nicola Vetrini]

On 17/08/2023 14:39, Nicola Vetrini wrote:
> The function can become static since it's used only within this file.
> This also resolves a violation of MISRA C:2012 Rule 8.4 due to the 
> absence
> of a declaration before the function definition.
> 
> Fixes: b177892d2d0e ("vpci/msix: handle accesses adjacent to the MSI-X 
> table")
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> Changes in v2:
> - Corrected wrong prefix in the commit subject.
> 
> CC-ing maintainers from "THE REST" as well, perhaps this trivial change 
> can go
> in straight away.
> ---
>  xen/drivers/vpci/msix.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
> index 25bde77586a4..f9df506f29bf 100644
> --- a/xen/drivers/vpci/msix.c
> +++ b/xen/drivers/vpci/msix.c
> @@ -223,7 +223,7 @@ static void __iomem *get_table(const struct vpci
> *vpci, unsigned int slot)
>      return msix->table[slot];
>  }
> 
> -unsigned int get_slot(const struct vpci *vpci, unsigned long addr)
> +static unsigned int get_slot(const struct vpci *vpci, unsigned long 
> addr)
>  {
>      unsigned long pfn = PFN_DOWN(addr);
> 
> --
> 2.34.1

Any chance of an ack? The maintainer for that file is Roger Pau Monné as 
far as I can tell.

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

Re: [XEN PATCH v2 2/3] vpci/msix: make 'get_slot' static

[Roger Pau Monné]

On Thu, Aug 17, 2023 at 02:39:27PM +0200, Nicola Vetrini wrote:
> The function can become static since it's used only within this file.
> This also resolves a violation of MISRA C:2012 Rule 8.4 due to the absence
> of a declaration before the function definition.
> 
> Fixes: b177892d2d0e ("vpci/msix: handle accesses adjacent to the MSI-X table")
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Sorry for the delay.

Thanks, Roger.

[XEN PATCH v2 3/3] drivers/video: make declarations of defined functions available

[Nicola Vetrini]

The declarations for 'vesa_{init,early_init,endboot}' needed by
'xen/drivers/video/vesa.c' and 'fill_console_start_info' in 'vga.c'
are now available by moving the relative code inside 'vga.h'.
The latter is moved from 'xen/console.h' because of its close relation with vga.
This also resolves violations of MISRA C:2012 Rule 8.4.

Fixes: 6d9199bd0f22 ("x86-64: enable hypervisor output on VESA frame buffer")
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Changes in v2:
- Moved fill_console_start_info to vga.h
  (21bee1787021 introduced this function)
---
 xen/arch/x86/include/asm/setup.h  |  6 ------
 xen/arch/x86/platform_hypercall.c |  2 +-
 xen/arch/x86/pv/dom0_build.c      |  2 +-
 xen/drivers/video/vga.c           |  8 --------
 xen/include/xen/console.h         |  2 --
 xen/include/xen/vga.h             | 16 ++++++++++++++++
 6 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/include/asm/setup.h b/xen/arch/x86/include/asm/setup.h
index b0e6a39e2365..dfdd9e555149 100644
--- a/xen/arch/x86/include/asm/setup.h
+++ b/xen/arch/x86/include/asm/setup.h
@@ -25,12 +25,6 @@ void subarch_init_memory(void);

 void init_IRQ(void);

-#ifdef CONFIG_VIDEO
-void vesa_init(void);
-#else
-static inline void vesa_init(void) {};
-#endif
-
 int construct_dom0(
     struct domain *d,
     const module_t *image, unsigned long image_headroom,
diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
index 9ff2da8fc324..9469de9045c7 100644
--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -14,7 +14,6 @@
 #include <xen/event.h>
 #include <xen/domain_page.h>
 #include <xen/trace.h>
-#include <xen/console.h>
 #include <xen/iocap.h>
 #include <xen/guest_access.h>
 #include <xen/hypercall.h>
@@ -24,6 +23,7 @@
 #include <xen/pmstat.h>
 #include <xen/irq.h>
 #include <xen/symbols.h>
+#include <xen/vga.h>
 #include <asm/current.h>
 #include <public/platform.h>
 #include <acpi/cpufreq/processor_perf.h>
diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index 909ee9a899a4..5bbed3a36a21 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -4,7 +4,6 @@
  * Copyright (c) 2002-2005, K A Fraser
  */

-#include <xen/console.h>
 #include <xen/domain.h>
 #include <xen/domain_page.h>
 #include <xen/init.h>
@@ -13,6 +12,7 @@
 #include <xen/pfn.h>
 #include <xen/sched.h>
 #include <xen/softirq.h>
+#include <xen/vga.h>

 #include <asm/bzimage.h>
 #include <asm/dom0_build.h>
diff --git a/xen/drivers/video/vga.c b/xen/drivers/video/vga.c
index 0a03508bee60..18b590cdf072 100644
--- a/xen/drivers/video/vga.c
+++ b/xen/drivers/video/vga.c
@@ -54,14 +54,6 @@ string_param("vga", opt_vga);
 static unsigned int columns, lines;
 #define ATTRIBUTE   7

-#ifdef CONFIG_X86
-void vesa_early_init(void);
-void vesa_endboot(bool_t keep);
-#else
-#define vesa_early_init() ((void)0)
-#define vesa_endboot(x)   ((void)0)
-#endif
-
 void __init video_init(void)
 {
     char *p;
diff --git a/xen/include/xen/console.h b/xen/include/xen/console.h
index 53c56191ba9e..ab5c30c0daf2 100644
--- a/xen/include/xen/console.h
+++ b/xen/include/xen/console.h
@@ -20,8 +20,6 @@ void console_init_postirq(void);
 void console_endboot(void);
 int console_has(const char *device);

-int fill_console_start_info(struct dom0_vga_console_info *);
-
 unsigned long console_lock_recursive_irqsave(void);
 void console_unlock_recursive_irqrestore(unsigned long flags);
 void console_force_unlock(void);
diff --git a/xen/include/xen/vga.h b/xen/include/xen/vga.h
index f72b63d446b1..1d53f0149433 100644
--- a/xen/include/xen/vga.h
+++ b/xen/include/xen/vga.h
@@ -15,4 +15,20 @@
 extern struct xen_vga_console_info vga_console_info;
 #endif

+int fill_console_start_info(struct dom0_vga_console_info *);
+
+#ifdef CONFIG_X86
+void vesa_early_init(void);
+void vesa_endboot(bool_t keep);
+#else
+#define vesa_early_init() ((void)0)
+#define vesa_endboot(x)   ((void)0)
+#endif
+
+#ifdef CONFIG_VIDEO
+void vesa_init(void);
+#else
+static inline void vesa_init(void) {};
+#endif
+
 #endif /* _XEN_VGA_H */
--
2.34.1

Re: [XEN PATCH v2 3/3] drivers/video: make declarations of defined functions available

[Jan Beulich]

On 17.08.2023 14:39, Nicola Vetrini wrote:
> --- a/xen/include/xen/vga.h
> +++ b/xen/include/xen/vga.h
> @@ -15,4 +15,20 @@
>  extern struct xen_vga_console_info vga_console_info;
>  #endif
> 
> +int fill_console_start_info(struct dom0_vga_console_info *);
> +
> +#ifdef CONFIG_X86
> +void vesa_early_init(void);
> +void vesa_endboot(bool_t keep);
> +#else
> +#define vesa_early_init() ((void)0)
> +#define vesa_endboot(x)   ((void)0)
> +#endif
> +
> +#ifdef CONFIG_VIDEO
> +void vesa_init(void);
> +#else
> +static inline void vesa_init(void) {};
> +#endif

Hmm, on one hand you simply move existing code here. But then why don't
you leverage the existing #ifdef? The more that it's more specific and
in line with drivers/video/Makefile having

obj-$(CONFIG_VGA) := vga.o

and

obj-$(CONFIG_VGA) += vesa.o

Jan

Re: [XEN PATCH v2 3/3] drivers/video: make declarations of defined functions available

[Nicola Vetrini]

On 17/08/2023 15:28, Jan Beulich wrote:
> On 17.08.2023 14:39, Nicola Vetrini wrote:
>> --- a/xen/include/xen/vga.h
>> +++ b/xen/include/xen/vga.h
>> @@ -15,4 +15,20 @@
>>  extern struct xen_vga_console_info vga_console_info;
>>  #endif
>> 
>> +int fill_console_start_info(struct dom0_vga_console_info *);
>> +
>> +#ifdef CONFIG_X86
>> +void vesa_early_init(void);
>> +void vesa_endboot(bool_t keep);
>> +#else
>> +#define vesa_early_init() ((void)0)
>> +#define vesa_endboot(x)   ((void)0)
>> +#endif
>> +
>> +#ifdef CONFIG_VIDEO
>> +void vesa_init(void);
>> +#else
>> +static inline void vesa_init(void) {};
>> +#endif
> 
> Hmm, on one hand you simply move existing code here. But then why don't
> you leverage the existing #ifdef? The more that it's more specific and
> in line with drivers/video/Makefile having
> 
> obj-$(CONFIG_VGA) := vga.o
> 
> and
> 
> obj-$(CONFIG_VGA) += vesa.o
> 
> Jan

Are you saying that CONFIG_VGA implies CONFIG_VIDEO and therefore 
"#ifdef CONFIG_VGA"
at line 14 of vga.h can be used instead of the #ifdefs inherited from 
the original locations
to wrap all the declarations that are being moved?

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

Re: [XEN PATCH v2 3/3] drivers/video: make declarations of defined functions available

[Jan Beulich]

On 17.08.2023 16:52, Nicola Vetrini wrote:
> On 17/08/2023 15:28, Jan Beulich wrote:
>> On 17.08.2023 14:39, Nicola Vetrini wrote:
>>> --- a/xen/include/xen/vga.h
>>> +++ b/xen/include/xen/vga.h
>>> @@ -15,4 +15,20 @@
>>>  extern struct xen_vga_console_info vga_console_info;
>>>  #endif
>>>
>>> +int fill_console_start_info(struct dom0_vga_console_info *);
>>> +
>>> +#ifdef CONFIG_X86
>>> +void vesa_early_init(void);
>>> +void vesa_endboot(bool_t keep);
>>> +#else
>>> +#define vesa_early_init() ((void)0)
>>> +#define vesa_endboot(x)   ((void)0)
>>> +#endif
>>> +
>>> +#ifdef CONFIG_VIDEO
>>> +void vesa_init(void);
>>> +#else
>>> +static inline void vesa_init(void) {};
>>> +#endif
>>
>> Hmm, on one hand you simply move existing code here. But then why don't
>> you leverage the existing #ifdef? The more that it's more specific and
>> in line with drivers/video/Makefile having
>>
>> obj-$(CONFIG_VGA) := vga.o
>>
>> and
>>
>> obj-$(CONFIG_VGA) += vesa.o
> 
> Are you saying that CONFIG_VGA implies CONFIG_VIDEO and therefore 
> "#ifdef CONFIG_VGA"
> at line 14 of vga.h can be used instead of the #ifdefs inherited from 
> the original locations
> to wrap all the declarations that are being moved?

Yes - see drivers/video/Kconfig.

Jan

Re: [XEN PATCH v2 3/3] drivers/video: make declarations of defined functions available

[Nicola Vetrini]

>> 
>> Are you saying that CONFIG_VGA implies CONFIG_VIDEO and therefore
>> "#ifdef CONFIG_VGA"
>> at line 14 of vga.h can be used instead of the #ifdefs inherited from
>> the original locations
>> to wrap all the declarations that are being moved?
> 
> Yes - see drivers/video/Kconfig.
> 
> Jan

Ok then. I guess I can make a standalone patch with this modification
if the other patches of this series go in as is.

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