[XEN PATCH 0/6] xen: fix missing headers and static storage duration

[XEN PATCH 0/6] xen: fix missing headers 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, or add one in
   the header if that's not the case.

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

Nicola Vetrini (6):
  x86/hpet: make variable 'per_cpu__cpu_bc_channel' static
  x86/setup: add missing headers
  x86/vm_event: add missing include
  cpufreq: add missing include of header 'pmstat.h'
  vpic/msix: make 'get_slot' static
  drivers/video: make declarations of defined functions available

 xen/arch/x86/hpet.c              |  2 +-
 xen/arch/x86/include/asm/setup.h |  6 ------
 xen/arch/x86/setup.c             |  4 +++-
 xen/arch/x86/vm_event.c          |  1 +
 xen/drivers/cpufreq/cpufreq.c    |  1 +
 xen/drivers/video/vga.c          |  9 +--------
 xen/drivers/vpci/msix.c          |  2 +-
 xen/include/xen/cpuidle.h        |  2 +-
 xen/include/xen/vga.h            | 14 ++++++++++++++
 9 files changed, 23 insertions(+), 18 deletions(-)

--
2.34.1

[XEN PATCH 1/6] x86/hpet: make variable 'per_cpu__cpu_bc_channel' static

[Nicola Vetrini]

The variable is only ever used inside the file where it's
defined, therefore it can have static storage. This also
resolves a violation of MISRA C:2012 Rule 8.4 due to the absence
of a declaration prior to the definition.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Fixes: 3ba523ff957c ("CPUIDLE: enable MSI capable HPET for timer broadcast")
---
 xen/arch/x86/hpet.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
index a2df1c7df401..79c07f6a9e09 100644
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -49,7 +49,7 @@ static struct hpet_event_channel *__read_mostly hpet_events;
 /* msi hpet channels used for broadcast */
 static unsigned int __read_mostly num_hpets_used;
 
-DEFINE_PER_CPU(struct hpet_event_channel *, cpu_bc_channel);
+static DEFINE_PER_CPU(struct hpet_event_channel *, cpu_bc_channel);
 
 unsigned long __initdata hpet_address;
 int8_t __initdata opt_hpet_legacy_replacement = -1;
-- 
2.34.1

[XEN PATCH 2/6] x86/setup: add missing headers

[Nicola Vetrini]

The missing headers declare variables 'xen_cpuidle' and 'use_invpcid'
that are then defined inside the file.
This is undesirable and also violates MISRA C:2012 Rule 8.4.
Adding suitable "#include"s resolves the issue.

The type of the variable 'xen_cpuidle' also changes according to
s/s8/int8_t/.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Fixes: 3eab82196b02 ("x86: PIT broadcast to fix local APIC timer stop issue for Deep C state")
Fixes: 63dc135aeaf9 ("x86: invpcid support")
---
 xen/arch/x86/setup.c      | 4 +++-
 xen/include/xen/cpuidle.h | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 80ae973d64e4..2bfc1fd00f8c 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -47,10 +47,12 @@
 #include <asm/mach-generic/mach_apic.h> /* for generic_apic_probe */
 #include <asm/setup.h>
 #include <xen/cpu.h>
+#include <xen/cpuidle.h>
 #include <asm/nmi.h>
 #include <asm/alternative.h>
 #include <asm/mc146818rtc.h>
 #include <asm/cpu-policy.h>
+#include <asm/invpcid.h>
 #include <asm/spec_ctrl.h>
 #include <asm/guest.h>
 #include <asm/microcode.h>
@@ -88,7 +90,7 @@ boolean_param("noapic", skip_ioapic_setup);
 
 /* **** Linux config option: propagated to domain0. */
 /* xen_cpuidle: xen control cstate. */
-s8 __read_mostly xen_cpuidle = -1;
+int8_t __read_mostly xen_cpuidle = -1;
 boolean_param("cpuidle", xen_cpuidle);
 
 #ifndef NDEBUG
diff --git a/xen/include/xen/cpuidle.h b/xen/include/xen/cpuidle.h
index 521a8deb04c2..705d0c1135f0 100644
--- a/xen/include/xen/cpuidle.h
+++ b/xen/include/xen/cpuidle.h
@@ -86,7 +86,7 @@ struct cpuidle_governor
     void (*reflect)         (struct acpi_processor_power *dev);
 };
 
-extern s8 xen_cpuidle;
+extern int8_t xen_cpuidle;
 extern struct cpuidle_governor *cpuidle_current_governor;
 
 bool cpuidle_using_deep_cstate(void);
-- 
2.34.1

[XEN PATCH 3/6] x86/vm_event: add missing include

[Nicola Vetrini]

The missing header included by this patch provides declarations for
the functions 'vm_event_{fill_regs,set_registers,monitor_next_interrupt}'
that are defined in the source file. This also resolves violations
of MISRA C:2012 Rule 8.4.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Fixes: adc75eba8b15 ("x86/vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs")
Fixes: 975efd3baa8d ("introduce VM_EVENT_FLAG_SET_REGISTERS")
Fixes: 9864841914c2 ("x86/vm_event: add support for VM_EVENT_REASON_INTERRUPT")
---
 xen/arch/x86/vm_event.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
index 7027c08a926b..499b6b349d79 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -20,6 +20,7 @@
 
 #include <xen/sched.h>
 #include <xen/mem_access.h>
+#include <xen/vm_event.h>
 #include <asm/vm_event.h>
 
 /* Implicitly serialized by the domctl lock. */
-- 
2.34.1

[XEN PATCH 4/6] cpufreq: add missing include of header 'pmstat.h'

[Nicola Vetrini]

The missing header included by this patch provides a declaration for
'set_px_pminfo' that is visible prior to the definition in this file.
This also resolves a violation of MISRA C:2012 Rule 8.4.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Fixes: 452119c09420 ("x86 and ia64: move cpufreq notify code to commone place")
---
 xen/drivers/cpufreq/cpufreq.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/drivers/cpufreq/cpufreq.c b/xen/drivers/cpufreq/cpufreq.c
index 2321c7dd07b1..4482bbe53a1e 100644
--- a/xen/drivers/cpufreq/cpufreq.c
+++ b/xen/drivers/cpufreq/cpufreq.c
@@ -39,6 +39,7 @@
 #include <xen/guest_access.h>
 #include <xen/domain.h>
 #include <xen/cpu.h>
+#include <xen/pmstat.h>
 #include <asm/io.h>
 #include <asm/processor.h>
 
-- 
2.34.1

[XEN PATCH 5/6] vpic/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.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Fixes: b177892d2d0e ("vpci/msix: handle accesses adjacent to the MSI-X table")
---
 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

[XEN PATCH 6/6] 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' and
including "<xen/console.h>" respectively.
This also resolves violations of MISRA C:2012 Rule 8.4.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Fixes: ebb26b509f1a ("xen/x86: make VGA support selectable")
Fixes: 6d9199bd0f22 ("x86-64: enable hypervisor output on VESA frame buffer")
---
 xen/arch/x86/include/asm/setup.h |  6 ------
 xen/drivers/video/vga.c          |  9 +--------
 xen/include/xen/vga.h            | 14 ++++++++++++++
 3 files changed, 15 insertions(+), 14 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/drivers/video/vga.c b/xen/drivers/video/vga.c
index 0a03508bee60..b62a47e000e7 100644
--- a/xen/drivers/video/vga.c
+++ b/xen/drivers/video/vga.c
@@ -9,6 +9,7 @@
 #include <xen/mm.h>
 #include <xen/param.h>
 #include <xen/vga.h>
+#include <xen/console.h>
 #include <xen/pci.h>
 #include <asm/io.h>
 
@@ -54,14 +55,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/vga.h b/xen/include/xen/vga.h
index f72b63d446b1..ffd73c9db135 100644
--- a/xen/include/xen/vga.h
+++ b/xen/include/xen/vga.h
@@ -15,4 +15,18 @@
 extern struct xen_vga_console_info vga_console_info;
 #endif
 
+#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 1/6] x86/hpet: make variable 'per_cpu__cpu_bc_channel' static

[Stefano Stabellini]

On Fri, 11 Aug 2023, Nicola Vetrini wrote:
> The variable is only ever used inside the file where it's
> defined, therefore it can have static storage. This also
> resolves a violation of MISRA C:2012 Rule 8.4 due to the absence
> of a declaration prior to the definition.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> Fixes: 3ba523ff957c ("CPUIDLE: enable MSI capable HPET for timer broadcast")

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


> ---
>  xen/arch/x86/hpet.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
> index a2df1c7df401..79c07f6a9e09 100644
> --- a/xen/arch/x86/hpet.c
> +++ b/xen/arch/x86/hpet.c
> @@ -49,7 +49,7 @@ static struct hpet_event_channel *__read_mostly hpet_events;
>  /* msi hpet channels used for broadcast */
>  static unsigned int __read_mostly num_hpets_used;
>  
> -DEFINE_PER_CPU(struct hpet_event_channel *, cpu_bc_channel);
> +static DEFINE_PER_CPU(struct hpet_event_channel *, cpu_bc_channel);
>  
>  unsigned long __initdata hpet_address;
>  int8_t __initdata opt_hpet_legacy_replacement = -1;
> -- 
> 2.34.1
> 

Re: [XEN PATCH 1/6] x86/hpet: make variable 'per_cpu__cpu_bc_channel' static

[Stefano Stabellini]

Adding x86 maintainers

On Fri, 11 Aug 2023, Stefano Stabellini wrote:
> On Fri, 11 Aug 2023, Nicola Vetrini wrote:
> > The variable is only ever used inside the file where it's
> > defined, therefore it can have static storage. This also
> > resolves a violation of MISRA C:2012 Rule 8.4 due to the absence
> > of a declaration prior to the definition.
> > 
> > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> > Fixes: 3ba523ff957c ("CPUIDLE: enable MSI capable HPET for timer broadcast")
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> 
> > ---
> >  xen/arch/x86/hpet.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
> > index a2df1c7df401..79c07f6a9e09 100644
> > --- a/xen/arch/x86/hpet.c
> > +++ b/xen/arch/x86/hpet.c
> > @@ -49,7 +49,7 @@ static struct hpet_event_channel *__read_mostly hpet_events;
> >  /* msi hpet channels used for broadcast */
> >  static unsigned int __read_mostly num_hpets_used;
> >  
> > -DEFINE_PER_CPU(struct hpet_event_channel *, cpu_bc_channel);
> > +static DEFINE_PER_CPU(struct hpet_event_channel *, cpu_bc_channel);
> >  
> >  unsigned long __initdata hpet_address;
> >  int8_t __initdata opt_hpet_legacy_replacement = -1;
> > -- 
> > 2.34.1
> > 
> 

Re: [XEN PATCH 2/6] x86/setup: add missing headers

[Stefano Stabellini]

On Fri, 11 Aug 2023, Nicola Vetrini wrote:
> The missing headers declare variables 'xen_cpuidle' and 'use_invpcid'
> that are then defined inside the file.
> This is undesirable and also violates MISRA C:2012 Rule 8.4.
> Adding suitable "#include"s resolves the issue.
> 
> The type of the variable 'xen_cpuidle' also changes according to
> s/s8/int8_t/.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> Fixes: 3eab82196b02 ("x86: PIT broadcast to fix local APIC timer stop issue for Deep C state")
> Fixes: 63dc135aeaf9 ("x86: invpcid support")

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


> ---
>  xen/arch/x86/setup.c      | 4 +++-
>  xen/include/xen/cpuidle.h | 2 +-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 80ae973d64e4..2bfc1fd00f8c 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -47,10 +47,12 @@
>  #include <asm/mach-generic/mach_apic.h> /* for generic_apic_probe */
>  #include <asm/setup.h>
>  #include <xen/cpu.h>
> +#include <xen/cpuidle.h>
>  #include <asm/nmi.h>
>  #include <asm/alternative.h>
>  #include <asm/mc146818rtc.h>
>  #include <asm/cpu-policy.h>
> +#include <asm/invpcid.h>
>  #include <asm/spec_ctrl.h>
>  #include <asm/guest.h>
>  #include <asm/microcode.h>
> @@ -88,7 +90,7 @@ boolean_param("noapic", skip_ioapic_setup);
>  
>  /* **** Linux config option: propagated to domain0. */
>  /* xen_cpuidle: xen control cstate. */
> -s8 __read_mostly xen_cpuidle = -1;
> +int8_t __read_mostly xen_cpuidle = -1;
>  boolean_param("cpuidle", xen_cpuidle);
>  
>  #ifndef NDEBUG
> diff --git a/xen/include/xen/cpuidle.h b/xen/include/xen/cpuidle.h
> index 521a8deb04c2..705d0c1135f0 100644
> --- a/xen/include/xen/cpuidle.h
> +++ b/xen/include/xen/cpuidle.h
> @@ -86,7 +86,7 @@ struct cpuidle_governor
>      void (*reflect)         (struct acpi_processor_power *dev);
>  };
>  
> -extern s8 xen_cpuidle;
> +extern int8_t xen_cpuidle;
>  extern struct cpuidle_governor *cpuidle_current_governor;
>  
>  bool cpuidle_using_deep_cstate(void);
> -- 
> 2.34.1
> 

Re: [XEN PATCH 3/6] x86/vm_event: add missing include

[Stefano Stabellini]

On Fri, 11 Aug 2023, Nicola Vetrini wrote:
> The missing header included by this patch provides declarations for
> the functions 'vm_event_{fill_regs,set_registers,monitor_next_interrupt}'
> that are defined in the source file. This also resolves violations
> of MISRA C:2012 Rule 8.4.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> Fixes: adc75eba8b15 ("x86/vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs")
> Fixes: 975efd3baa8d ("introduce VM_EVENT_FLAG_SET_REGISTERS")
> Fixes: 9864841914c2 ("x86/vm_event: add support for VM_EVENT_REASON_INTERRUPT")
> ---
>  xen/arch/x86/vm_event.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
> index 7027c08a926b..499b6b349d79 100644
> --- a/xen/arch/x86/vm_event.c
> +++ b/xen/arch/x86/vm_event.c
> @@ -20,6 +20,7 @@
>  
>  #include <xen/sched.h>
>  #include <xen/mem_access.h>
> +#include <xen/vm_event.h>
>  #include <asm/vm_event.h>
>  
>  /* Implicitly serialized by the domctl lock. */

I think the problem here is that ./arch/x86/include/asm/vm_event.h,
differently from ./arch/arm/include/asm/vm_event.h, doesn't #include
<xen/vm_event.h>

Re: [XEN PATCH 4/6] cpufreq: add missing include of header 'pmstat.h'

[Stefano Stabellini]

On Fri, 11 Aug 2023, Nicola Vetrini wrote:
> The missing header included by this patch provides a declaration for
> 'set_px_pminfo' that is visible prior to the definition in this file.
> This also resolves a violation of MISRA C:2012 Rule 8.4.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> Fixes: 452119c09420 ("x86 and ia64: move cpufreq notify code to commone place")

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


> ---
>  xen/drivers/cpufreq/cpufreq.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/xen/drivers/cpufreq/cpufreq.c b/xen/drivers/cpufreq/cpufreq.c
> index 2321c7dd07b1..4482bbe53a1e 100644
> --- a/xen/drivers/cpufreq/cpufreq.c
> +++ b/xen/drivers/cpufreq/cpufreq.c
> @@ -39,6 +39,7 @@
>  #include <xen/guest_access.h>
>  #include <xen/domain.h>
>  #include <xen/cpu.h>
> +#include <xen/pmstat.h>
>  #include <asm/io.h>
>  #include <asm/processor.h>
>  
> -- 
> 2.34.1
> 

Re: [XEN PATCH 5/6] vpic/msix: make 'get_slot' static

[Stefano Stabellini]

On Fri, 11 Aug 2023, 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.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> Fixes: b177892d2d0e ("vpci/msix: handle accesses adjacent to the MSI-X table")

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


> ---
>  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 6/6] drivers/video: make declarations of defined functions available

[Stefano Stabellini]

On Fri, 11 Aug 2023, Nicola Vetrini wrote:
> 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' and
> including "<xen/console.h>" respectively.
> This also resolves violations of MISRA C:2012 Rule 8.4.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> Fixes: ebb26b509f1a ("xen/x86: make VGA support selectable")
> Fixes: 6d9199bd0f22 ("x86-64: enable hypervisor output on VESA frame buffer")

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


> ---
>  xen/arch/x86/include/asm/setup.h |  6 ------
>  xen/drivers/video/vga.c          |  9 +--------
>  xen/include/xen/vga.h            | 14 ++++++++++++++
>  3 files changed, 15 insertions(+), 14 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/drivers/video/vga.c b/xen/drivers/video/vga.c
> index 0a03508bee60..b62a47e000e7 100644
> --- a/xen/drivers/video/vga.c
> +++ b/xen/drivers/video/vga.c
> @@ -9,6 +9,7 @@
>  #include <xen/mm.h>
>  #include <xen/param.h>
>  #include <xen/vga.h>
> +#include <xen/console.h>
>  #include <xen/pci.h>
>  #include <asm/io.h>
>  
> @@ -54,14 +55,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/vga.h b/xen/include/xen/vga.h
> index f72b63d446b1..ffd73c9db135 100644
> --- a/xen/include/xen/vga.h
> +++ b/xen/include/xen/vga.h
> @@ -15,4 +15,18 @@
>  extern struct xen_vga_console_info vga_console_info;
>  #endif
>  
> +#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 3/6] x86/vm_event: add missing include

[Nicola Vetrini]

>> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
>> index 7027c08a926b..499b6b349d79 100644
>> --- a/xen/arch/x86/vm_event.c
>> +++ b/xen/arch/x86/vm_event.c
>> @@ -20,6 +20,7 @@
>> 
>>  #include <xen/sched.h>
>>  #include <xen/mem_access.h>
>> +#include <xen/vm_event.h>
>>  #include <asm/vm_event.h>
>> 
>>  /* Implicitly serialized by the domctl lock. */
> 
> I think the problem here is that ./arch/x86/include/asm/vm_event.h,
> differently from ./arch/arm/include/asm/vm_event.h, doesn't #include
> <xen/vm_event.h>

I see your point. Do you think it would be better to include 
xen/vm_event.h
in asm/vm_event.h for x86 or move the inclusion of xen/vm_event.h for 
arm to
the source file, as done in the patch?

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

Re: [XEN PATCH 3/6] x86/vm_event: add missing include

[Julien Grall]

Hi,

On 12/08/2023 10:53, Nicola Vetrini wrote:
> 
>>> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
>>> index 7027c08a926b..499b6b349d79 100644
>>> --- a/xen/arch/x86/vm_event.c
>>> +++ b/xen/arch/x86/vm_event.c
>>> @@ -20,6 +20,7 @@
>>>
>>>  #include <xen/sched.h>
>>>  #include <xen/mem_access.h>
>>> +#include <xen/vm_event.h>
>>>  #include <asm/vm_event.h>
>>>
>>>  /* Implicitly serialized by the domctl lock. */
>>
>> I think the problem here is that ./arch/x86/include/asm/vm_event.h,
>> differently from ./arch/arm/include/asm/vm_event.h, doesn't #include
>> <xen/vm_event.h>
> 
> I see your point. Do you think it would be better to include xen/vm_event.h
> in asm/vm_event.h for x86 or move the inclusion of xen/vm_event.h for 
> arm to
> the source file, as done in the patch?

I think it is a bit odd require the C file to include the arch-specific 
header and the common one. It would be better to include only one.

My preference would be to include <asm/...> from <xen/...> and then only 
include the latter in the C file.

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 3/6] x86/vm_event: add missing include

[Jan Beulich]

On 12.08.2023 13:37, Julien Grall wrote:
> On 12/08/2023 10:53, Nicola Vetrini wrote:
>>>> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
>>>> index 7027c08a926b..499b6b349d79 100644
>>>> --- a/xen/arch/x86/vm_event.c
>>>> +++ b/xen/arch/x86/vm_event.c
>>>> @@ -20,6 +20,7 @@
>>>>
>>>>  #include <xen/sched.h>
>>>>  #include <xen/mem_access.h>
>>>> +#include <xen/vm_event.h>
>>>>  #include <asm/vm_event.h>
>>>>
>>>>  /* Implicitly serialized by the domctl lock. */
>>>
>>> I think the problem here is that ./arch/x86/include/asm/vm_event.h,
>>> differently from ./arch/arm/include/asm/vm_event.h, doesn't #include
>>> <xen/vm_event.h>
>>
>> I see your point. Do you think it would be better to include xen/vm_event.h
>> in asm/vm_event.h for x86 or move the inclusion of xen/vm_event.h for 
>> arm to
>> the source file, as done in the patch?
> 
> I think it is a bit odd require the C file to include the arch-specific 
> header and the common one. It would be better to include only one.
> 
> My preference would be to include <asm/...> from <xen/...> and then only 
> include the latter in the C file.

+1

Jan

Re: [XEN PATCH 1/6] x86/hpet: make variable 'per_cpu__cpu_bc_channel' static

[Jan Beulich]

On 12.08.2023 00:54, Stefano Stabellini wrote:
> On Fri, 11 Aug 2023, Nicola Vetrini wrote:
>> The variable is only ever used inside the file where it's
>> defined, therefore it can have static storage. This also
>> resolves a violation of MISRA C:2012 Rule 8.4 due to the absence
>> of a declaration prior to the definition.
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> Fixes: 3ba523ff957c ("CPUIDLE: enable MSI capable HPET for timer broadcast")
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

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

Re: [XEN PATCH 2/6] x86/setup: add missing headers

[Jan Beulich]

On 12.08.2023 00:58, Stefano Stabellini wrote:
> On Fri, 11 Aug 2023, Nicola Vetrini wrote:
>> The missing headers declare variables 'xen_cpuidle' and 'use_invpcid'
>> that are then defined inside the file.
>> This is undesirable and also violates MISRA C:2012 Rule 8.4.
>> Adding suitable "#include"s resolves the issue.
>>
>> The type of the variable 'xen_cpuidle' also changes according to
>> s/s8/int8_t/.
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> Fixes: 3eab82196b02 ("x86: PIT broadcast to fix local APIC timer stop issue for Deep C state")
>> Fixes: 63dc135aeaf9 ("x86: invpcid support")

One request: Please can you get used to putting the Fixes: tags first,
so one doesn't need to (try to) remember to move them in the course of
committing?

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

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

Jan

Re: [XEN PATCH 3/6] x86/vm_event: add missing include

[Jan Beulich]

On 11.08.2023 09:19, Nicola Vetrini wrote:
> The missing header included by this patch provides declarations for
> the functions 'vm_event_{fill_regs,set_registers,monitor_next_interrupt}'
> that are defined in the source file. This also resolves violations
> of MISRA C:2012 Rule 8.4.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> Fixes: adc75eba8b15 ("x86/vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs")
> Fixes: 975efd3baa8d ("introduce VM_EVENT_FLAG_SET_REGISTERS")
> Fixes: 9864841914c2 ("x86/vm_event: add support for VM_EVENT_REASON_INTERRUPT")

It's hard to see how it can be three commit here. The oldest one is at
fault, I would say.

Also please remember to Cc maintainers.

Jan

> --- a/xen/arch/x86/vm_event.c
> +++ b/xen/arch/x86/vm_event.c
> @@ -20,6 +20,7 @@
>  
>  #include <xen/sched.h>
>  #include <xen/mem_access.h>
> +#include <xen/vm_event.h>
>  #include <asm/vm_event.h>
>  
>  /* Implicitly serialized by the domctl lock. */

Re: [XEN PATCH 4/6] cpufreq: add missing include of header 'pmstat.h'

[Jan Beulich]

On 12.08.2023 01:03, Stefano Stabellini wrote:
> On Fri, 11 Aug 2023, Nicola Vetrini wrote:
>> The missing header included by this patch provides a declaration for
>> 'set_px_pminfo' that is visible prior to the definition in this file.
>> This also resolves a violation of MISRA C:2012 Rule 8.4.
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> Fixes: 452119c09420 ("x86 and ia64: move cpufreq notify code to commone place")
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

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

Re: [XEN PATCH 2/6] x86/setup: add missing headers

[Nicola Vetrini]

>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>> Fixes: 3eab82196b02 ("x86: PIT broadcast to fix local APIC timer stop 
>>> issue for Deep C state")
>>> Fixes: 63dc135aeaf9 ("x86: invpcid support")
> 
> One request: Please can you get used to putting the Fixes: tags first,
> so one doesn't need to (try to) remember to move them in the course of
> committing?
> 

Sure. Sorry, I didn't notice the ordering in other commits.

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

Re: [XEN PATCH 5/6] vpic/msix: make 'get_slot' static

[Jan Beulich]

On 11.08.2023 09:19, 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.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> Fixes: b177892d2d0e ("vpci/msix: handle accesses adjacent to the MSI-X table")

Nit (typo): In the title you mean "vpci/msix", just like this quoted
commit has it. This is important-ish because we also have "vpic"
elsewhere in the tree. (Can certainly be adjusted while committing.)

Jan

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

[Jan Beulich]

On 11.08.2023 09:19, Nicola Vetrini wrote:
> --- a/xen/drivers/video/vga.c
> +++ b/xen/drivers/video/vga.c
> @@ -9,6 +9,7 @@
>  #include <xen/mm.h>
>  #include <xen/param.h>
>  #include <xen/vga.h>
> +#include <xen/console.h>

xen/vga.h, which you move the declarations to, is already included here.
Why the need for xen/console.h?

Jan

Re: [XEN PATCH 5/6] vpic/msix: make 'get_slot' static

[Nicola Vetrini]

On 14/08/2023 09:44, Jan Beulich wrote:
> On 11.08.2023 09:19, 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.
>> 
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> Fixes: b177892d2d0e ("vpci/msix: handle accesses adjacent to the MSI-X 
>> table")
> 
> Nit (typo): In the title you mean "vpci/msix", just like this quoted
> commit has it. This is important-ish because we also have "vpic"
> elsewhere in the tree. (Can certainly be adjusted while committing.)
> 
> Jan

Yes, I meant to write 'vpci/msix'.

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

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

[Nicola Vetrini]

On 14/08/2023 09:47, Jan Beulich wrote:
> On 11.08.2023 09:19, Nicola Vetrini wrote:
>> --- a/xen/drivers/video/vga.c
>> +++ b/xen/drivers/video/vga.c
>> @@ -9,6 +9,7 @@
>>  #include <xen/mm.h>
>>  #include <xen/param.h>
>>  #include <xen/vga.h>
>> +#include <xen/console.h>
> 
> xen/vga.h, which you move the declarations to, is already included 
> here.
> Why the need for xen/console.h?
> 
> Jan

vga.c needs a declaration for fill_console_start_info, which is declared 
in console.h, as
stated in the commit message (it could be clarified perhaps).

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

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

[Jan Beulich]

On 14.08.2023 09:56, Nicola Vetrini wrote:
> On 14/08/2023 09:47, Jan Beulich wrote:
>> On 11.08.2023 09:19, Nicola Vetrini wrote:
>>> --- a/xen/drivers/video/vga.c
>>> +++ b/xen/drivers/video/vga.c
>>> @@ -9,6 +9,7 @@
>>>  #include <xen/mm.h>
>>>  #include <xen/param.h>
>>>  #include <xen/vga.h>
>>> +#include <xen/console.h>
>>
>> xen/vga.h, which you move the declarations to, is already included 
>> here.
>> Why the need for xen/console.h?
> 
> vga.c needs a declaration for fill_console_start_info, which is declared 
> in console.h, as
> stated in the commit message (it could be clarified perhaps).

Ah, I see. It's not very fortunate mixing of two separate adjustments.
But then I'm inclined to ask: When fill_console_start_info() is
defined in vga.c, wouldn't it make sense to move its declaration to
vga.h? The more considering the type of its parameter?

Jan

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

[Nicola Vetrini]

On 14/08/2023 10:12, Jan Beulich wrote:
> On 14.08.2023 09:56, Nicola Vetrini wrote:
>> On 14/08/2023 09:47, Jan Beulich wrote:
>>> On 11.08.2023 09:19, Nicola Vetrini wrote:
>>>> --- a/xen/drivers/video/vga.c
>>>> +++ b/xen/drivers/video/vga.c
>>>> @@ -9,6 +9,7 @@
>>>>  #include <xen/mm.h>
>>>>  #include <xen/param.h>
>>>>  #include <xen/vga.h>
>>>> +#include <xen/console.h>
>>> 
>>> xen/vga.h, which you move the declarations to, is already included
>>> here.
>>> Why the need for xen/console.h?
>> 
>> vga.c needs a declaration for fill_console_start_info, which is 
>> declared
>> in console.h, as
>> stated in the commit message (it could be clarified perhaps).
> 
> Ah, I see. It's not very fortunate mixing of two separate adjustments.

Well, they are not so separate, because they both deal with what the 
functions in vga.c need
to be compliant with Rule 8.4.

> But then I'm inclined to ask: When fill_console_start_info() is
> defined in vga.c, wouldn't it make sense to move its declaration to
> vga.h? The more considering the type of its parameter?
> 
> Jan

I see no downside. This change would imply having to include <xen/vga.h> 
in both
- x86/platform_hypercall.c
- x86/pv/dom0_build.c
which seems okay.

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

Re: [XEN PATCH 3/6] x86/vm_event: add missing include

[Nicola Vetrini]

On 14/08/2023 09:39, Jan Beulich wrote:
> On 11.08.2023 09:19, Nicola Vetrini wrote:
>> The missing header included by this patch provides declarations for
>> the functions 
>> 'vm_event_{fill_regs,set_registers,monitor_next_interrupt}'
>> that are defined in the source file. This also resolves violations
>> of MISRA C:2012 Rule 8.4.
>> 
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> Fixes: adc75eba8b15 ("x86/vm_event: consolidate hvm_event_fill_regs 
>> and p2m_vm_event_fill_regs")
>> Fixes: 975efd3baa8d ("introduce VM_EVENT_FLAG_SET_REGISTERS")
>> Fixes: 9864841914c2 ("x86/vm_event: add support for 
>> VM_EVENT_REASON_INTERRUPT")
> 
> It's hard to see how it can be three commit here. The oldest one is at
> fault, I would say.

Since the patch is concerned with more than one function then in a sense 
I agree
with you (the headers should have been included in the proper way the 
first time around), but
then more definitions have been added by adc75eba8b15 and 9864841914c2, 
and these should have
triggered a refactoring too. I can leave just 975efd3baa8d in the Fixes 
if the preferred way is to list just the first problematic commit 
(perhaps with a little explanation after --- ).

> 
> Also please remember to Cc maintainers.

Yes, sorry. I must have forgotten to run add_maintainers.

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

Re: [XEN PATCH 3/6] x86/vm_event: add missing include

[Julien Grall]

Hi,

On 14/08/2023 11:33, Nicola Vetrini wrote:
> On 14/08/2023 09:39, Jan Beulich wrote:
>> On 11.08.2023 09:19, Nicola Vetrini wrote:
>>> The missing header included by this patch provides declarations for
>>> the functions 
>>> 'vm_event_{fill_regs,set_registers,monitor_next_interrupt}'
>>> that are defined in the source file. This also resolves violations
>>> of MISRA C:2012 Rule 8.4.
>>>
>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>> Fixes: adc75eba8b15 ("x86/vm_event: consolidate hvm_event_fill_regs 
>>> and p2m_vm_event_fill_regs")
>>> Fixes: 975efd3baa8d ("introduce VM_EVENT_FLAG_SET_REGISTERS")
>>> Fixes: 9864841914c2 ("x86/vm_event: add support for 
>>> VM_EVENT_REASON_INTERRUPT")
>>
>> It's hard to see how it can be three commit here. The oldest one is at
>> fault, I would say.
> 
> Since the patch is concerned with more than one function then in a sense 
> I agree
> with you (the headers should have been included in the proper way the 
> first time around), but
> then more definitions have been added by adc75eba8b15 and 9864841914c2, 
> and these should have
> triggered a refactoring too. I can leave just 975efd3baa8d in the Fixes 
> if the preferred way is to list just the first problematic commit 
> (perhaps with a little explanation after --- ).

To be honest, I don't exactly see the value of using Fixes tag for those 
patches. I agree they are technically issues, but they are unlikely 
going to be backported.

So if it were me, I would just drop all the Fixes tags for missing 
includes unless there is an actual bug associated
with them (e.g. a caller was miscalling the function because the
prototype was incorrect).

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 3/6] x86/vm_event: add missing include

[Nicola Vetrini]

Hi,

On 14/08/2023 13:01, Julien Grall wrote:
> Hi,
> 
> On 14/08/2023 11:33, Nicola Vetrini wrote:
>> On 14/08/2023 09:39, Jan Beulich wrote:
>>> On 11.08.2023 09:19, Nicola Vetrini wrote:
>>>> The missing header included by this patch provides declarations for
>>>> the functions 
>>>> 'vm_event_{fill_regs,set_registers,monitor_next_interrupt}'
>>>> that are defined in the source file. This also resolves violations
>>>> of MISRA C:2012 Rule 8.4.
>>>> 
>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>> Fixes: adc75eba8b15 ("x86/vm_event: consolidate hvm_event_fill_regs 
>>>> and p2m_vm_event_fill_regs")
>>>> Fixes: 975efd3baa8d ("introduce VM_EVENT_FLAG_SET_REGISTERS")
>>>> Fixes: 9864841914c2 ("x86/vm_event: add support for 
>>>> VM_EVENT_REASON_INTERRUPT")
>>> 
>>> It's hard to see how it can be three commit here. The oldest one is 
>>> at
>>> fault, I would say.
>> 
>> Since the patch is concerned with more than one function then in a 
>> sense I agree
>> with you (the headers should have been included in the proper way the 
>> first time around), but
>> then more definitions have been added by adc75eba8b15 and 
>> 9864841914c2, and these should have
>> triggered a refactoring too. I can leave just 975efd3baa8d in the 
>> Fixes if the preferred way is to list just the first problematic 
>> commit (perhaps with a little explanation after --- ).
> 
> To be honest, I don't exactly see the value of using Fixes tag for
> those patches. I agree they are technically issues, but they are
> unlikely going to be backported.
> 
> So if it were me, I would just drop all the Fixes tags for missing
> includes unless there is an actual bug associated
> with them (e.g. a caller was miscalling the function because the
> prototype was incorrect).
> 
> Cheers,

Adding those tags for this kind of situation was requested on the 
previous discussion [1],
so in this series I kept the same strategy (though probably here I put 
too many of them).

[1] 
https://lore.kernel.org/xen-devel/cfbc7569-3714-2200-054c-49ba593d6903@suse.com/


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

Re: [XEN PATCH 3/6] x86/vm_event: add missing include

[Nicola Vetrini]

CC-ing the missing maintainers here, both from x86 and vm_event

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

Re: [XEN PATCH 3/6] x86/vm_event: add missing include

[Julien Grall]

Hi Nicola,

On 14/08/2023 13:53, Nicola Vetrini wrote:
> On 14/08/2023 13:01, Julien Grall wrote:
>> Hi,
>>
>> On 14/08/2023 11:33, Nicola Vetrini wrote:
>>> On 14/08/2023 09:39, Jan Beulich wrote:
>>>> On 11.08.2023 09:19, Nicola Vetrini wrote:
>>>>> The missing header included by this patch provides declarations for
>>>>> the functions 
>>>>> 'vm_event_{fill_regs,set_registers,monitor_next_interrupt}'
>>>>> that are defined in the source file. This also resolves violations
>>>>> of MISRA C:2012 Rule 8.4.
>>>>>
>>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>>> Fixes: adc75eba8b15 ("x86/vm_event: consolidate hvm_event_fill_regs 
>>>>> and p2m_vm_event_fill_regs")
>>>>> Fixes: 975efd3baa8d ("introduce VM_EVENT_FLAG_SET_REGISTERS")
>>>>> Fixes: 9864841914c2 ("x86/vm_event: add support for 
>>>>> VM_EVENT_REASON_INTERRUPT")
>>>>
>>>> It's hard to see how it can be three commit here. The oldest one is at
>>>> fault, I would say.
>>>
>>> Since the patch is concerned with more than one function then in a 
>>> sense I agree
>>> with you (the headers should have been included in the proper way the 
>>> first time around), but
>>> then more definitions have been added by adc75eba8b15 and 
>>> 9864841914c2, and these should have
>>> triggered a refactoring too. I can leave just 975efd3baa8d in the 
>>> Fixes if the preferred way is to list just the first problematic 
>>> commit (perhaps with a little explanation after --- ).
>>
>> To be honest, I don't exactly see the value of using Fixes tag for
>> those patches. I agree they are technically issues, but they are
>> unlikely going to be backported.
>>
>> So if it were me, I would just drop all the Fixes tags for missing
>> includes unless there is an actual bug associated
>> with them (e.g. a caller was miscalling the function because the
>> prototype was incorrect).
>>
>> Cheers,
> 
> Adding those tags for this kind of situation was requested on the 
> previous discussion [1],
> so in this series I kept the same strategy (though probably here I put 
> too many of them).

I disagree with the suggestion made. They are just noise for this sort 
of patch and require extra digging (I assume you spent 10-15min to 
figure out the multiple fixes) for a limited benefits (I don't expect 
anyone to backport the patches).

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 3/6] x86/vm_event: add missing include

[Jan Beulich]

On 14.08.2023 15:06, Julien Grall wrote:
> Hi Nicola,
> 
> On 14/08/2023 13:53, Nicola Vetrini wrote:
>> On 14/08/2023 13:01, Julien Grall wrote:
>>> Hi,
>>>
>>> On 14/08/2023 11:33, Nicola Vetrini wrote:
>>>> On 14/08/2023 09:39, Jan Beulich wrote:
>>>>> On 11.08.2023 09:19, Nicola Vetrini wrote:
>>>>>> The missing header included by this patch provides declarations for
>>>>>> the functions 
>>>>>> 'vm_event_{fill_regs,set_registers,monitor_next_interrupt}'
>>>>>> that are defined in the source file. This also resolves violations
>>>>>> of MISRA C:2012 Rule 8.4.
>>>>>>
>>>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>>>> Fixes: adc75eba8b15 ("x86/vm_event: consolidate hvm_event_fill_regs 
>>>>>> and p2m_vm_event_fill_regs")
>>>>>> Fixes: 975efd3baa8d ("introduce VM_EVENT_FLAG_SET_REGISTERS")
>>>>>> Fixes: 9864841914c2 ("x86/vm_event: add support for 
>>>>>> VM_EVENT_REASON_INTERRUPT")
>>>>>
>>>>> It's hard to see how it can be three commit here. The oldest one is at
>>>>> fault, I would say.
>>>>
>>>> Since the patch is concerned with more than one function then in a 
>>>> sense I agree
>>>> with you (the headers should have been included in the proper way the 
>>>> first time around), but
>>>> then more definitions have been added by adc75eba8b15 and 
>>>> 9864841914c2, and these should have
>>>> triggered a refactoring too. I can leave just 975efd3baa8d in the 
>>>> Fixes if the preferred way is to list just the first problematic 
>>>> commit (perhaps with a little explanation after --- ).
>>>
>>> To be honest, I don't exactly see the value of using Fixes tag for
>>> those patches. I agree they are technically issues, but they are
>>> unlikely going to be backported.
>>>
>>> So if it were me, I would just drop all the Fixes tags for missing
>>> includes unless there is an actual bug associated
>>> with them (e.g. a caller was miscalling the function because the
>>> prototype was incorrect).
>>>
>>> Cheers,
>>
>> Adding those tags for this kind of situation was requested on the 
>> previous discussion [1],
>> so in this series I kept the same strategy (though probably here I put 
>> too many of them).
> 
> I disagree with the suggestion made. They are just noise for this sort 
> of patch and require extra digging (I assume you spent 10-15min to 
> figure out the multiple fixes) for a limited benefits (I don't expect 
> anyone to backport the patches).

While I agree that Fixes: is primarily for marking of backporting
candidates, I don't think this is its exclusive purpose. As far as I'm
concerned, it also aids review in the specific cases here (this isn't
a commonly occurring aspect, though, I agree). Yet the primary reason
I asked for them to be added is because each of the omissions is an
at least latent bug.

Jan

Re: [XEN PATCH 3/6] x86/vm_event: add missing include

[Julien Grall]

Hi Jan,

On 14/08/2023 14:31, Jan Beulich wrote:
> On 14.08.2023 15:06, Julien Grall wrote:
>> Hi Nicola,
>>
>> On 14/08/2023 13:53, Nicola Vetrini wrote:
>>> On 14/08/2023 13:01, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 14/08/2023 11:33, Nicola Vetrini wrote:
>>>>> On 14/08/2023 09:39, Jan Beulich wrote:
>>>>>> On 11.08.2023 09:19, Nicola Vetrini wrote:
>>>>>>> The missing header included by this patch provides declarations for
>>>>>>> the functions
>>>>>>> 'vm_event_{fill_regs,set_registers,monitor_next_interrupt}'
>>>>>>> that are defined in the source file. This also resolves violations
>>>>>>> of MISRA C:2012 Rule 8.4.
>>>>>>>
>>>>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>>>>> Fixes: adc75eba8b15 ("x86/vm_event: consolidate hvm_event_fill_regs
>>>>>>> and p2m_vm_event_fill_regs")
>>>>>>> Fixes: 975efd3baa8d ("introduce VM_EVENT_FLAG_SET_REGISTERS")
>>>>>>> Fixes: 9864841914c2 ("x86/vm_event: add support for
>>>>>>> VM_EVENT_REASON_INTERRUPT")
>>>>>>
>>>>>> It's hard to see how it can be three commit here. The oldest one is at
>>>>>> fault, I would say.
>>>>>
>>>>> Since the patch is concerned with more than one function then in a
>>>>> sense I agree
>>>>> with you (the headers should have been included in the proper way the
>>>>> first time around), but
>>>>> then more definitions have been added by adc75eba8b15 and
>>>>> 9864841914c2, and these should have
>>>>> triggered a refactoring too. I can leave just 975efd3baa8d in the
>>>>> Fixes if the preferred way is to list just the first problematic
>>>>> commit (perhaps with a little explanation after --- ).
>>>>
>>>> To be honest, I don't exactly see the value of using Fixes tag for
>>>> those patches. I agree they are technically issues, but they are
>>>> unlikely going to be backported.
>>>>
>>>> So if it were me, I would just drop all the Fixes tags for missing
>>>> includes unless there is an actual bug associated
>>>> with them (e.g. a caller was miscalling the function because the
>>>> prototype was incorrect).
>>>>
>>>> Cheers,
>>>
>>> Adding those tags for this kind of situation was requested on the
>>> previous discussion [1],
>>> so in this series I kept the same strategy (though probably here I put
>>> too many of them).
>>
>> I disagree with the suggestion made. They are just noise for this sort
>> of patch and require extra digging (I assume you spent 10-15min to
>> figure out the multiple fixes) for a limited benefits (I don't expect
>> anyone to backport the patches).
> 
> While I agree that Fixes: is primarily for marking of backporting
> candidates, I don't think this is its exclusive purpose. As far as I'm
> concerned, it also aids review in the specific cases here (this isn't
> a commonly occurring aspect, though, I agree). Yet the primary reason
> I asked for them to be added is because each of the omissions is an
> at least latent bug.

I agree they are latent bugs. But the question is whether the time 
looking up the histoy is well spent?

In this situation, we have a lot of of similar patches where we need to 
add Fixes. So there is quite a bit of time sinked into finding the 
original commit message. Is it valuable? I don't believe so.

A least for Arm, I would not require the tag to be added for any missing 
prototypes.

Cheers,

-- 
Julien Grall