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'.

While moving the code, the alternative definitions are now guarded by
CONFIG_VGA, since it implies all previously used constants.

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>
---
This is the last non-committed patch from that series, hence I put a v3
in the subject to convey the context of this patch.

Changes in v2:
- Moved fill_console_start_info to vga.h
  (21bee1787021 introduced this function)
Changes in v3:
- Changed the preprocessor guard
- Replace the inclusions of <xen/console.h> with <xen/vga.h> where needed.
---
 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             | 8 ++++++++
 6 files changed, 10 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..05f5013a05d8 100644
--- a/xen/include/xen/vga.h
+++ b/xen/include/xen/vga.h
@@ -13,6 +13,14 @@

 #ifdef CONFIG_VGA
 extern struct xen_vga_console_info vga_console_info;
+int fill_console_start_info(struct dom0_vga_console_info *);
+void vesa_init(void);
+void vesa_early_init(void);
+void vesa_endboot(bool_t keep);
+#else
+#define vesa_early_init() ((void)0)
+#define vesa_endboot(x)   ((void)0)
+static inline void vesa_init(void) {};
 #endif

 #endif /* _XEN_VGA_H */
--
2.34.1

On 01.09.2023 09:02, 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'.
> 
> While moving the code, the alternative definitions are now guarded by
> CONFIG_VGA, since it implies all previously used constants.

But such an implication doesn't mean said adjustment is correct. Easiest
would likely be to simply drop that half sentence.

> --- a/xen/include/xen/vga.h
> +++ b/xen/include/xen/vga.h
> @@ -13,6 +13,14 @@
> 
>  #ifdef CONFIG_VGA
>  extern struct xen_vga_console_info vga_console_info;
> +int fill_console_start_info(struct dom0_vga_console_info *);
> +void vesa_init(void);
> +void vesa_early_init(void);
> +void vesa_endboot(bool_t keep);

Nit: Just "bool" please.

> +#else
> +#define vesa_early_init() ((void)0)
> +#define vesa_endboot(x)   ((void)0)
> +static inline void vesa_init(void) {};

So why two #define-s and one inline function? Just because it was that
way originally doesn't mean it needs to stay that way. Then again are
the two #define-s actually needed at all? It looks like they were added
to vga.c just out of precaution, covering the "VGA but no VESA" case.
Now that things are properly moved to headers (and keyed to CONFIG_VGA)
I think we'd better omit those. They can be introduced again when we
gain a VGA/VESA split (and a CONFIG_VESA).

Jan

On 05/09/2023 16:42, Jan Beulich wrote:
> On 01.09.2023 09:02, 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'.
>> 
>> While moving the code, the alternative definitions are now guarded by
>> CONFIG_VGA, since it implies all previously used constants.
> 
> But such an implication doesn't mean said adjustment is correct. 
> Easiest
> would likely be to simply drop that half sentence.
> 

Maybe I didn't phrase that correctly, I'm ok with just dropping the last 
part
(it can be traced to the Kconfig anyway).

>> --- a/xen/include/xen/vga.h
>> +++ b/xen/include/xen/vga.h
>> @@ -13,6 +13,14 @@
>> 
>>  #ifdef CONFIG_VGA
>>  extern struct xen_vga_console_info vga_console_info;
>> +int fill_console_start_info(struct dom0_vga_console_info *);
>> +void vesa_init(void);
>> +void vesa_early_init(void);
>> +void vesa_endboot(bool_t keep);
> 
> Nit: Just "bool" please.
> 

Right, I missed it.

>> +#else
>> +#define vesa_early_init() ((void)0)
>> +#define vesa_endboot(x)   ((void)0)
>> +static inline void vesa_init(void) {};
> 
> So why two #define-s and one inline function? Just because it was that
> way originally doesn't mean it needs to stay that way. Then again are
> the two #define-s actually needed at all? It looks like they were added
> to vga.c just out of precaution, covering the "VGA but no VESA" case.
> Now that things are properly moved to headers (and keyed to CONFIG_VGA)
> I think we'd better omit those. They can be introduced again when we
> gain a VGA/VESA split (and a CONFIG_VESA).
> 

Ok on uniforming them to inline functions.
I don't have an opinion on the second matter. If you're otherwise okay 
with the patch
and no one objects I can drop them.

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

On 06.09.2023 14:36, Nicola Vetrini wrote:
> On 05/09/2023 16:42, Jan Beulich wrote:
>> On 01.09.2023 09:02, Nicola Vetrini wrote:
>>> +#else
>>> +#define vesa_early_init() ((void)0)
>>> +#define vesa_endboot(x)   ((void)0)
>>> +static inline void vesa_init(void) {};
>>
>> So why two #define-s and one inline function? Just because it was that
>> way originally doesn't mean it needs to stay that way. Then again are
>> the two #define-s actually needed at all? It looks like they were added
>> to vga.c just out of precaution, covering the "VGA but no VESA" case.
>> Now that things are properly moved to headers (and keyed to CONFIG_VGA)
>> I think we'd better omit those. They can be introduced again when we
>> gain a VGA/VESA split (and a CONFIG_VESA).
>>
> 
> Ok on uniforming them to inline functions.
> I don't have an opinion on the second matter. If you're otherwise okay 
> with the patch
> and no one objects I can drop them.

I am okay with the rest of the patch (as always subject to spotting
something else when looking at the next version), yes.

Jan