From: Julien Grall <jgrall@amazon.com>

In a follow-up patch will we want to add support for EFI framebuffer
in dom0. Yet, Xen may not use the framebuffer, so it would be ideal
to not have to enable CONFIG_VIDEO/CONFIG_VGA.

Introduce vga_console_info in a hacky way and move the code
to fill it up from x86 to common.

Signed-off-by: Julien Grall <jgrall@amazon.com>

----

This is a bit of a hack. Sent early to gather opinion on whether
we should enable allow Dom0 to use the EFI Framebuffer even
if Xen is built with CONFIG_VIDEO=n on Arm.
---
 xen/arch/arm/efi/efi-boot.h |  6 ---
 xen/arch/arm/setup.c        |  4 ++
 xen/arch/x86/efi/efi-boot.h | 72 ------------------------------------
 xen/common/efi/boot.c       | 74 ++++++++++++++++++++++++++++++++++++-
 xen/include/xen/vga.h       |  2 +-
 5 files changed, 78 insertions(+), 80 deletions(-)

diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index ae8627134e5a..17a3d46c59ae 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -1000,12 +1000,6 @@ static void __init efi_arch_console_init(UINTN cols, UINTN rows)
 {
 }
 
-static void __init efi_arch_video_init(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
-                                       UINTN info_size,
-                                       EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info)
-{
-}
-
 static void __init efi_arch_flush_dcache_area(const void *vaddr, UINTN size)
 {
     __flush_dcache_area(vaddr, size);
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index d5d0792ed48a..a336ee58179c 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -71,6 +71,10 @@ static unsigned long opt_xenheap_megabytes __initdata;
 integer_param("xenheap_megabytes", opt_xenheap_megabytes);
 #endif
 
+#ifndef CONFIG_VIDEO
+struct xen_vga_console_info vga_console_info;
+#endif
+
 domid_t __read_mostly max_init_domid;
 
 static __used void init_done(void)
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index f69509a2103a..cba3fa75a475 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -3,7 +3,6 @@
  * is intended to be included by common/efi/boot.c _only_, and
  * therefore can define arch specific global variables.
  */
-#include <xen/vga.h>
 #include <asm/e820.h>
 #include <asm/edd.h>
 #include <asm/microcode.h>
@@ -497,77 +496,6 @@ static void __init efi_arch_console_init(UINTN cols, UINTN rows)
 #endif
 }
 
-static void __init efi_arch_video_init(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
-                                       UINTN info_size,
-                                       EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info)
-{
-#ifdef CONFIG_VIDEO
-    int bpp = 0;
-
-    switch ( mode_info->PixelFormat )
-    {
-    case PixelRedGreenBlueReserved8BitPerColor:
-        vga_console_info.u.vesa_lfb.red_pos = 0;
-        vga_console_info.u.vesa_lfb.red_size = 8;
-        vga_console_info.u.vesa_lfb.green_pos = 8;
-        vga_console_info.u.vesa_lfb.green_size = 8;
-        vga_console_info.u.vesa_lfb.blue_pos = 16;
-        vga_console_info.u.vesa_lfb.blue_size = 8;
-        vga_console_info.u.vesa_lfb.rsvd_pos = 24;
-        vga_console_info.u.vesa_lfb.rsvd_size = 8;
-        bpp = 32;
-        break;
-    case PixelBlueGreenRedReserved8BitPerColor:
-        vga_console_info.u.vesa_lfb.red_pos = 16;
-        vga_console_info.u.vesa_lfb.red_size = 8;
-        vga_console_info.u.vesa_lfb.green_pos = 8;
-        vga_console_info.u.vesa_lfb.green_size = 8;
-        vga_console_info.u.vesa_lfb.blue_pos = 0;
-        vga_console_info.u.vesa_lfb.blue_size = 8;
-        vga_console_info.u.vesa_lfb.rsvd_pos = 24;
-        vga_console_info.u.vesa_lfb.rsvd_size = 8;
-        bpp = 32;
-        break;
-    case PixelBitMask:
-        bpp = set_color(mode_info->PixelInformation.RedMask, bpp,
-                        &vga_console_info.u.vesa_lfb.red_pos,
-                        &vga_console_info.u.vesa_lfb.red_size);
-        bpp = set_color(mode_info->PixelInformation.GreenMask, bpp,
-                        &vga_console_info.u.vesa_lfb.green_pos,
-                        &vga_console_info.u.vesa_lfb.green_size);
-        bpp = set_color(mode_info->PixelInformation.BlueMask, bpp,
-                        &vga_console_info.u.vesa_lfb.blue_pos,
-                        &vga_console_info.u.vesa_lfb.blue_size);
-        if ( mode_info->PixelInformation.ReservedMask )
-            bpp = set_color(mode_info->PixelInformation.ReservedMask, bpp,
-                            &vga_console_info.u.vesa_lfb.rsvd_pos,
-                            &vga_console_info.u.vesa_lfb.rsvd_size);
-        if ( bpp > 0 )
-            break;
-        /* fall through */
-    default:
-        PrintErr(L"Current graphics mode is unsupported!\r\n");
-        bpp  = 0;
-        break;
-    }
-    if ( bpp > 0 )
-    {
-        vga_console_info.video_type = XEN_VGATYPE_EFI_LFB;
-        vga_console_info.u.vesa_lfb.gbl_caps = 2; /* possibly non-VGA */
-        vga_console_info.u.vesa_lfb.width =
-            mode_info->HorizontalResolution;
-        vga_console_info.u.vesa_lfb.height = mode_info->VerticalResolution;
-        vga_console_info.u.vesa_lfb.bits_per_pixel = bpp;
-        vga_console_info.u.vesa_lfb.bytes_per_line =
-            (mode_info->PixelsPerScanLine * bpp + 7) >> 3;
-        vga_console_info.u.vesa_lfb.lfb_base = gop->Mode->FrameBufferBase;
-        vga_console_info.u.vesa_lfb.ext_lfb_base = gop->Mode->FrameBufferBase >> 32;
-        vga_console_info.u.vesa_lfb.lfb_size =
-            (gop->Mode->FrameBufferSize + 0xffff) >> 16;
-    }
-#endif
-}
-
 static void __init efi_arch_memory_setup(void)
 {
     unsigned int i;
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 80e4e32623c4..2bc38ae40fff 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -20,6 +20,7 @@
 #endif
 #include <xen/string.h>
 #include <xen/stringify.h>
+#include <xen/vga.h>
 #ifdef CONFIG_X86
 /*
  * Keep this arch-specific modified include in the common file, as moving
@@ -1025,6 +1026,77 @@ static void __init efi_get_apple_properties(void)
     }
 }
 
+static void __init efi_video_init(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
+                                  UINTN info_size,
+                                  EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info)
+{
+#if defined(CONFIG_VIDEO) || defined(CONFIG_ARM)
+    int bpp = 0;
+
+    switch ( mode_info->PixelFormat )
+    {
+    case PixelRedGreenBlueReserved8BitPerColor:
+        vga_console_info.u.vesa_lfb.red_pos = 0;
+        vga_console_info.u.vesa_lfb.red_size = 8;
+        vga_console_info.u.vesa_lfb.green_pos = 8;
+        vga_console_info.u.vesa_lfb.green_size = 8;
+        vga_console_info.u.vesa_lfb.blue_pos = 16;
+        vga_console_info.u.vesa_lfb.blue_size = 8;
+        vga_console_info.u.vesa_lfb.rsvd_pos = 24;
+        vga_console_info.u.vesa_lfb.rsvd_size = 8;
+        bpp = 32;
+        break;
+    case PixelBlueGreenRedReserved8BitPerColor:
+        vga_console_info.u.vesa_lfb.red_pos = 16;
+        vga_console_info.u.vesa_lfb.red_size = 8;
+        vga_console_info.u.vesa_lfb.green_pos = 8;
+        vga_console_info.u.vesa_lfb.green_size = 8;
+        vga_console_info.u.vesa_lfb.blue_pos = 0;
+        vga_console_info.u.vesa_lfb.blue_size = 8;
+        vga_console_info.u.vesa_lfb.rsvd_pos = 24;
+        vga_console_info.u.vesa_lfb.rsvd_size = 8;
+        bpp = 32;
+        break;
+    case PixelBitMask:
+        bpp = set_color(mode_info->PixelInformation.RedMask, bpp,
+                        &vga_console_info.u.vesa_lfb.red_pos,
+                        &vga_console_info.u.vesa_lfb.red_size);
+        bpp = set_color(mode_info->PixelInformation.GreenMask, bpp,
+                        &vga_console_info.u.vesa_lfb.green_pos,
+                        &vga_console_info.u.vesa_lfb.green_size);
+        bpp = set_color(mode_info->PixelInformation.BlueMask, bpp,
+                        &vga_console_info.u.vesa_lfb.blue_pos,
+                        &vga_console_info.u.vesa_lfb.blue_size);
+        if ( mode_info->PixelInformation.ReservedMask )
+            bpp = set_color(mode_info->PixelInformation.ReservedMask, bpp,
+                            &vga_console_info.u.vesa_lfb.rsvd_pos,
+                            &vga_console_info.u.vesa_lfb.rsvd_size);
+        if ( bpp > 0 )
+            break;
+        /* fall through */
+    default:
+        PrintErr(L"Current graphics mode is unsupported!\r\n");
+        bpp  = 0;
+        break;
+    }
+    if ( bpp > 0 )
+    {
+        vga_console_info.video_type = XEN_VGATYPE_EFI_LFB;
+        vga_console_info.u.vesa_lfb.gbl_caps = 2; /* possibly non-VGA */
+        vga_console_info.u.vesa_lfb.width =
+            mode_info->HorizontalResolution;
+        vga_console_info.u.vesa_lfb.height = mode_info->VerticalResolution;
+        vga_console_info.u.vesa_lfb.bits_per_pixel = bpp;
+        vga_console_info.u.vesa_lfb.bytes_per_line =
+            (mode_info->PixelsPerScanLine * bpp + 7) >> 3;
+        vga_console_info.u.vesa_lfb.lfb_base = gop->Mode->FrameBufferBase;
+        vga_console_info.u.vesa_lfb.ext_lfb_base = gop->Mode->FrameBufferBase >> 32;
+        vga_console_info.u.vesa_lfb.lfb_size =
+            (gop->Mode->FrameBufferSize + 0xffff) >> 16;
+    }
+#endif
+}
+
 static void __init efi_set_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, UINTN gop_mode)
 {
     EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info;
@@ -1042,7 +1114,7 @@ static void __init efi_set_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, UINTN gop
     /* Get graphics and frame buffer info. */
     status = gop->QueryMode(gop, gop->Mode->Mode, &info_size, &mode_info);
     if ( !EFI_ERROR(status) )
-        efi_arch_video_init(gop, info_size, mode_info);
+        efi_video_init(gop, info_size, mode_info);
 }
 
 #define INVALID_VIRTUAL_ADDRESS (0xBAAADUL << \
diff --git a/xen/include/xen/vga.h b/xen/include/xen/vga.h
index f72b63d446b1..39b4c2eae198 100644
--- a/xen/include/xen/vga.h
+++ b/xen/include/xen/vga.h
@@ -11,7 +11,7 @@
 
 #include <xen/video.h>
 
-#ifdef CONFIG_VGA
+#if defined(CONFIG_VGA) || defined(CONFIG_ARM)
 extern struct xen_vga_console_info vga_console_info;
 #endif
 
-- 
2.32.0

On Sun, 6 Feb 2022, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> In a follow-up patch will we want to add support for EFI framebuffer
> in dom0. Yet, Xen may not use the framebuffer, so it would be ideal
> to not have to enable CONFIG_VIDEO/CONFIG_VGA.
> 
> Introduce vga_console_info in a hacky way and move the code
> to fill it up from x86 to common.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> ----
> 
> This is a bit of a hack. Sent early to gather opinion on whether
> we should enable allow Dom0 to use the EFI Framebuffer even
> if Xen is built with CONFIG_VIDEO=n on Arm.

Yes, I think we should enable Dom0 to use the EFI framebuffer even if
Xen is built with CONFIG_VIDEO=n. I think CONFIG_VIDEO should be about
Xen's support for video output, not the guest's support for video
(including dom0's).

If we want to enable a super-minimal build of Xen with EFI support but
without VIDEO support even for Dom0, we could introduce a separate
Kconfig option for it. Probably not worth it.


> ---
>  xen/arch/arm/efi/efi-boot.h |  6 ---
>  xen/arch/arm/setup.c        |  4 ++
>  xen/arch/x86/efi/efi-boot.h | 72 ------------------------------------
>  xen/common/efi/boot.c       | 74 ++++++++++++++++++++++++++++++++++++-
>  xen/include/xen/vga.h       |  2 +-
>  5 files changed, 78 insertions(+), 80 deletions(-)
> 
> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> index ae8627134e5a..17a3d46c59ae 100644
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -1000,12 +1000,6 @@ static void __init efi_arch_console_init(UINTN cols, UINTN rows)
>  {
>  }
>  
> -static void __init efi_arch_video_init(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
> -                                       UINTN info_size,
> -                                       EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info)
> -{
> -}
> -
>  static void __init efi_arch_flush_dcache_area(const void *vaddr, UINTN size)
>  {
>      __flush_dcache_area(vaddr, size);
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index d5d0792ed48a..a336ee58179c 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -71,6 +71,10 @@ static unsigned long opt_xenheap_megabytes __initdata;
>  integer_param("xenheap_megabytes", opt_xenheap_megabytes);
>  #endif
>  
> +#ifndef CONFIG_VIDEO
> +struct xen_vga_console_info vga_console_info;
> +#endif
> +
>  domid_t __read_mostly max_init_domid;
>  
>  static __used void init_done(void)
> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index f69509a2103a..cba3fa75a475 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -3,7 +3,6 @@
>   * is intended to be included by common/efi/boot.c _only_, and
>   * therefore can define arch specific global variables.
>   */
> -#include <xen/vga.h>
>  #include <asm/e820.h>
>  #include <asm/edd.h>
>  #include <asm/microcode.h>
> @@ -497,77 +496,6 @@ static void __init efi_arch_console_init(UINTN cols, UINTN rows)
>  #endif
>  }
>  
> -static void __init efi_arch_video_init(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
> -                                       UINTN info_size,
> -                                       EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info)
> -{
> -#ifdef CONFIG_VIDEO
> -    int bpp = 0;
> -
> -    switch ( mode_info->PixelFormat )
> -    {
> -    case PixelRedGreenBlueReserved8BitPerColor:
> -        vga_console_info.u.vesa_lfb.red_pos = 0;
> -        vga_console_info.u.vesa_lfb.red_size = 8;
> -        vga_console_info.u.vesa_lfb.green_pos = 8;
> -        vga_console_info.u.vesa_lfb.green_size = 8;
> -        vga_console_info.u.vesa_lfb.blue_pos = 16;
> -        vga_console_info.u.vesa_lfb.blue_size = 8;
> -        vga_console_info.u.vesa_lfb.rsvd_pos = 24;
> -        vga_console_info.u.vesa_lfb.rsvd_size = 8;
> -        bpp = 32;
> -        break;
> -    case PixelBlueGreenRedReserved8BitPerColor:
> -        vga_console_info.u.vesa_lfb.red_pos = 16;
> -        vga_console_info.u.vesa_lfb.red_size = 8;
> -        vga_console_info.u.vesa_lfb.green_pos = 8;
> -        vga_console_info.u.vesa_lfb.green_size = 8;
> -        vga_console_info.u.vesa_lfb.blue_pos = 0;
> -        vga_console_info.u.vesa_lfb.blue_size = 8;
> -        vga_console_info.u.vesa_lfb.rsvd_pos = 24;
> -        vga_console_info.u.vesa_lfb.rsvd_size = 8;
> -        bpp = 32;
> -        break;
> -    case PixelBitMask:
> -        bpp = set_color(mode_info->PixelInformation.RedMask, bpp,
> -                        &vga_console_info.u.vesa_lfb.red_pos,
> -                        &vga_console_info.u.vesa_lfb.red_size);
> -        bpp = set_color(mode_info->PixelInformation.GreenMask, bpp,
> -                        &vga_console_info.u.vesa_lfb.green_pos,
> -                        &vga_console_info.u.vesa_lfb.green_size);
> -        bpp = set_color(mode_info->PixelInformation.BlueMask, bpp,
> -                        &vga_console_info.u.vesa_lfb.blue_pos,
> -                        &vga_console_info.u.vesa_lfb.blue_size);
> -        if ( mode_info->PixelInformation.ReservedMask )
> -            bpp = set_color(mode_info->PixelInformation.ReservedMask, bpp,
> -                            &vga_console_info.u.vesa_lfb.rsvd_pos,
> -                            &vga_console_info.u.vesa_lfb.rsvd_size);
> -        if ( bpp > 0 )
> -            break;
> -        /* fall through */
> -    default:
> -        PrintErr(L"Current graphics mode is unsupported!\r\n");
> -        bpp  = 0;
> -        break;
> -    }
> -    if ( bpp > 0 )
> -    {
> -        vga_console_info.video_type = XEN_VGATYPE_EFI_LFB;
> -        vga_console_info.u.vesa_lfb.gbl_caps = 2; /* possibly non-VGA */
> -        vga_console_info.u.vesa_lfb.width =
> -            mode_info->HorizontalResolution;
> -        vga_console_info.u.vesa_lfb.height = mode_info->VerticalResolution;
> -        vga_console_info.u.vesa_lfb.bits_per_pixel = bpp;
> -        vga_console_info.u.vesa_lfb.bytes_per_line =
> -            (mode_info->PixelsPerScanLine * bpp + 7) >> 3;
> -        vga_console_info.u.vesa_lfb.lfb_base = gop->Mode->FrameBufferBase;
> -        vga_console_info.u.vesa_lfb.ext_lfb_base = gop->Mode->FrameBufferBase >> 32;
> -        vga_console_info.u.vesa_lfb.lfb_size =
> -            (gop->Mode->FrameBufferSize + 0xffff) >> 16;
> -    }
> -#endif
> -}
> -
>  static void __init efi_arch_memory_setup(void)
>  {
>      unsigned int i;
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 80e4e32623c4..2bc38ae40fff 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -20,6 +20,7 @@
>  #endif
>  #include <xen/string.h>
>  #include <xen/stringify.h>
> +#include <xen/vga.h>
>  #ifdef CONFIG_X86
>  /*
>   * Keep this arch-specific modified include in the common file, as moving
> @@ -1025,6 +1026,77 @@ static void __init efi_get_apple_properties(void)
>      }
>  }
>  
> +static void __init efi_video_init(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
> +                                  UINTN info_size,
> +                                  EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info)
> +{
> +#if defined(CONFIG_VIDEO) || defined(CONFIG_ARM)
> +    int bpp = 0;
> +
> +    switch ( mode_info->PixelFormat )
> +    {
> +    case PixelRedGreenBlueReserved8BitPerColor:
> +        vga_console_info.u.vesa_lfb.red_pos = 0;
> +        vga_console_info.u.vesa_lfb.red_size = 8;
> +        vga_console_info.u.vesa_lfb.green_pos = 8;
> +        vga_console_info.u.vesa_lfb.green_size = 8;
> +        vga_console_info.u.vesa_lfb.blue_pos = 16;
> +        vga_console_info.u.vesa_lfb.blue_size = 8;
> +        vga_console_info.u.vesa_lfb.rsvd_pos = 24;
> +        vga_console_info.u.vesa_lfb.rsvd_size = 8;
> +        bpp = 32;
> +        break;
> +    case PixelBlueGreenRedReserved8BitPerColor:
> +        vga_console_info.u.vesa_lfb.red_pos = 16;
> +        vga_console_info.u.vesa_lfb.red_size = 8;
> +        vga_console_info.u.vesa_lfb.green_pos = 8;
> +        vga_console_info.u.vesa_lfb.green_size = 8;
> +        vga_console_info.u.vesa_lfb.blue_pos = 0;
> +        vga_console_info.u.vesa_lfb.blue_size = 8;
> +        vga_console_info.u.vesa_lfb.rsvd_pos = 24;
> +        vga_console_info.u.vesa_lfb.rsvd_size = 8;
> +        bpp = 32;
> +        break;
> +    case PixelBitMask:
> +        bpp = set_color(mode_info->PixelInformation.RedMask, bpp,
> +                        &vga_console_info.u.vesa_lfb.red_pos,
> +                        &vga_console_info.u.vesa_lfb.red_size);
> +        bpp = set_color(mode_info->PixelInformation.GreenMask, bpp,
> +                        &vga_console_info.u.vesa_lfb.green_pos,
> +                        &vga_console_info.u.vesa_lfb.green_size);
> +        bpp = set_color(mode_info->PixelInformation.BlueMask, bpp,
> +                        &vga_console_info.u.vesa_lfb.blue_pos,
> +                        &vga_console_info.u.vesa_lfb.blue_size);
> +        if ( mode_info->PixelInformation.ReservedMask )
> +            bpp = set_color(mode_info->PixelInformation.ReservedMask, bpp,
> +                            &vga_console_info.u.vesa_lfb.rsvd_pos,
> +                            &vga_console_info.u.vesa_lfb.rsvd_size);
> +        if ( bpp > 0 )
> +            break;
> +        /* fall through */
> +    default:
> +        PrintErr(L"Current graphics mode is unsupported!\r\n");
> +        bpp  = 0;
> +        break;
> +    }
> +    if ( bpp > 0 )
> +    {
> +        vga_console_info.video_type = XEN_VGATYPE_EFI_LFB;
> +        vga_console_info.u.vesa_lfb.gbl_caps = 2; /* possibly non-VGA */
> +        vga_console_info.u.vesa_lfb.width =
> +            mode_info->HorizontalResolution;
> +        vga_console_info.u.vesa_lfb.height = mode_info->VerticalResolution;
> +        vga_console_info.u.vesa_lfb.bits_per_pixel = bpp;
> +        vga_console_info.u.vesa_lfb.bytes_per_line =
> +            (mode_info->PixelsPerScanLine * bpp + 7) >> 3;
> +        vga_console_info.u.vesa_lfb.lfb_base = gop->Mode->FrameBufferBase;
> +        vga_console_info.u.vesa_lfb.ext_lfb_base = gop->Mode->FrameBufferBase >> 32;
> +        vga_console_info.u.vesa_lfb.lfb_size =
> +            (gop->Mode->FrameBufferSize + 0xffff) >> 16;
> +    }
> +#endif
> +}
> +
>  static void __init efi_set_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, UINTN gop_mode)
>  {
>      EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info;
> @@ -1042,7 +1114,7 @@ static void __init efi_set_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, UINTN gop
>      /* Get graphics and frame buffer info. */
>      status = gop->QueryMode(gop, gop->Mode->Mode, &info_size, &mode_info);
>      if ( !EFI_ERROR(status) )
> -        efi_arch_video_init(gop, info_size, mode_info);
> +        efi_video_init(gop, info_size, mode_info);
>  }
>  
>  #define INVALID_VIRTUAL_ADDRESS (0xBAAADUL << \
> diff --git a/xen/include/xen/vga.h b/xen/include/xen/vga.h
> index f72b63d446b1..39b4c2eae198 100644
> --- a/xen/include/xen/vga.h
> +++ b/xen/include/xen/vga.h
> @@ -11,7 +11,7 @@
>  
>  #include <xen/video.h>
>  
> -#ifdef CONFIG_VGA
> +#if defined(CONFIG_VGA) || defined(CONFIG_ARM)
>  extern struct xen_vga_console_info vga_console_info;
>  #endif
>  
> -- 
> 2.32.0
> 

On 06.02.2022 20:28, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> In a follow-up patch will we want to add support for EFI framebuffer
> in dom0. Yet, Xen may not use the framebuffer, so it would be ideal
> to not have to enable CONFIG_VIDEO/CONFIG_VGA.
> 
> Introduce vga_console_info in a hacky way and move the code
> to fill it up from x86 to common.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> ----
> 
> This is a bit of a hack. Sent early to gather opinion on whether
> we should enable allow Dom0 to use the EFI Framebuffer even
> if Xen is built with CONFIG_VIDEO=n on Arm.

I have no input here; this will need to be settled among you Arm folks.
I have no objection to the code movement, just one nit:

> @@ -1025,6 +1026,77 @@ static void __init efi_get_apple_properties(void)
>      }
>  }
>  
> +static void __init efi_video_init(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
> +                                  UINTN info_size,
> +                                  EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info)
> +{
> +#if defined(CONFIG_VIDEO) || defined(CONFIG_ARM)
> +    int bpp = 0;
> +
> +    switch ( mode_info->PixelFormat )
> +    {
> +    case PixelRedGreenBlueReserved8BitPerColor:
> +        vga_console_info.u.vesa_lfb.red_pos = 0;
> +        vga_console_info.u.vesa_lfb.red_size = 8;
> +        vga_console_info.u.vesa_lfb.green_pos = 8;
> +        vga_console_info.u.vesa_lfb.green_size = 8;
> +        vga_console_info.u.vesa_lfb.blue_pos = 16;
> +        vga_console_info.u.vesa_lfb.blue_size = 8;
> +        vga_console_info.u.vesa_lfb.rsvd_pos = 24;
> +        vga_console_info.u.vesa_lfb.rsvd_size = 8;
> +        bpp = 32;
> +        break;
> +    case PixelBlueGreenRedReserved8BitPerColor:
> +        vga_console_info.u.vesa_lfb.red_pos = 16;
> +        vga_console_info.u.vesa_lfb.red_size = 8;
> +        vga_console_info.u.vesa_lfb.green_pos = 8;
> +        vga_console_info.u.vesa_lfb.green_size = 8;
> +        vga_console_info.u.vesa_lfb.blue_pos = 0;
> +        vga_console_info.u.vesa_lfb.blue_size = 8;
> +        vga_console_info.u.vesa_lfb.rsvd_pos = 24;
> +        vga_console_info.u.vesa_lfb.rsvd_size = 8;
> +        bpp = 32;
> +        break;
> +    case PixelBitMask:
> +        bpp = set_color(mode_info->PixelInformation.RedMask, bpp,
> +                        &vga_console_info.u.vesa_lfb.red_pos,
> +                        &vga_console_info.u.vesa_lfb.red_size);
> +        bpp = set_color(mode_info->PixelInformation.GreenMask, bpp,
> +                        &vga_console_info.u.vesa_lfb.green_pos,
> +                        &vga_console_info.u.vesa_lfb.green_size);
> +        bpp = set_color(mode_info->PixelInformation.BlueMask, bpp,
> +                        &vga_console_info.u.vesa_lfb.blue_pos,
> +                        &vga_console_info.u.vesa_lfb.blue_size);
> +        if ( mode_info->PixelInformation.ReservedMask )
> +            bpp = set_color(mode_info->PixelInformation.ReservedMask, bpp,
> +                            &vga_console_info.u.vesa_lfb.rsvd_pos,
> +                            &vga_console_info.u.vesa_lfb.rsvd_size);
> +        if ( bpp > 0 )
> +            break;
> +        /* fall through */
> +    default:
> +        PrintErr(L"Current graphics mode is unsupported!\r\n");
> +        bpp  = 0;
> +        break;
> +    }
> +    if ( bpp > 0 )
> +    {
> +        vga_console_info.video_type = XEN_VGATYPE_EFI_LFB;
> +        vga_console_info.u.vesa_lfb.gbl_caps = 2; /* possibly non-VGA */
> +        vga_console_info.u.vesa_lfb.width =
> +            mode_info->HorizontalResolution;
> +        vga_console_info.u.vesa_lfb.height = mode_info->VerticalResolution;
> +        vga_console_info.u.vesa_lfb.bits_per_pixel = bpp;
> +        vga_console_info.u.vesa_lfb.bytes_per_line =
> +            (mode_info->PixelsPerScanLine * bpp + 7) >> 3;
> +        vga_console_info.u.vesa_lfb.lfb_base = gop->Mode->FrameBufferBase;
> +        vga_console_info.u.vesa_lfb.ext_lfb_base = gop->Mode->FrameBufferBase >> 32;
> +        vga_console_info.u.vesa_lfb.lfb_size =
> +            (gop->Mode->FrameBufferSize + 0xffff) >> 16;
> +    }
> +#endif
> +}

While you move this code, could you please insert blank lines between
non-fall-through case blocks, and perhaps another one between the switch()
and the if() blocks? And it looks like
- the "gop" parameter could also do with becoming pointer-to-const,
- the expanded #ifdef could do with a comment briefly explaining why Arm
  needs-special casing.

Jan

Hi,

On 07/02/2022 08:53, Jan Beulich wrote:
> On 06.02.2022 20:28, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> In a follow-up patch will we want to add support for EFI framebuffer
>> in dom0. Yet, Xen may not use the framebuffer, so it would be ideal
>> to not have to enable CONFIG_VIDEO/CONFIG_VGA.
>>
>> Introduce vga_console_info in a hacky way and move the code
>> to fill it up from x86 to common.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>
>> ----
>>
>> This is a bit of a hack. Sent early to gather opinion on whether
>> we should enable allow Dom0 to use the EFI Framebuffer even
>> if Xen is built with CONFIG_VIDEO=n on Arm.
> 
> I have no input here; this will need to be settled among you Arm folks.
> I have no objection to the code movement, just one nit:
> 
>> @@ -1025,6 +1026,77 @@ static void __init efi_get_apple_properties(void)
>>       }
>>   }
>>   
>> +static void __init efi_video_init(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
>> +                                  UINTN info_size,
>> +                                  EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info)
>> +{
>> +#if defined(CONFIG_VIDEO) || defined(CONFIG_ARM)
>> +    int bpp = 0;
>> +
>> +    switch ( mode_info->PixelFormat )
>> +    {
>> +    case PixelRedGreenBlueReserved8BitPerColor:
>> +        vga_console_info.u.vesa_lfb.red_pos = 0;
>> +        vga_console_info.u.vesa_lfb.red_size = 8;
>> +        vga_console_info.u.vesa_lfb.green_pos = 8;
>> +        vga_console_info.u.vesa_lfb.green_size = 8;
>> +        vga_console_info.u.vesa_lfb.blue_pos = 16;
>> +        vga_console_info.u.vesa_lfb.blue_size = 8;
>> +        vga_console_info.u.vesa_lfb.rsvd_pos = 24;
>> +        vga_console_info.u.vesa_lfb.rsvd_size = 8;
>> +        bpp = 32;
>> +        break;
>> +    case PixelBlueGreenRedReserved8BitPerColor:
>> +        vga_console_info.u.vesa_lfb.red_pos = 16;
>> +        vga_console_info.u.vesa_lfb.red_size = 8;
>> +        vga_console_info.u.vesa_lfb.green_pos = 8;
>> +        vga_console_info.u.vesa_lfb.green_size = 8;
>> +        vga_console_info.u.vesa_lfb.blue_pos = 0;
>> +        vga_console_info.u.vesa_lfb.blue_size = 8;
>> +        vga_console_info.u.vesa_lfb.rsvd_pos = 24;
>> +        vga_console_info.u.vesa_lfb.rsvd_size = 8;
>> +        bpp = 32;
>> +        break;
>> +    case PixelBitMask:
>> +        bpp = set_color(mode_info->PixelInformation.RedMask, bpp,
>> +                        &vga_console_info.u.vesa_lfb.red_pos,
>> +                        &vga_console_info.u.vesa_lfb.red_size);
>> +        bpp = set_color(mode_info->PixelInformation.GreenMask, bpp,
>> +                        &vga_console_info.u.vesa_lfb.green_pos,
>> +                        &vga_console_info.u.vesa_lfb.green_size);
>> +        bpp = set_color(mode_info->PixelInformation.BlueMask, bpp,
>> +                        &vga_console_info.u.vesa_lfb.blue_pos,
>> +                        &vga_console_info.u.vesa_lfb.blue_size);
>> +        if ( mode_info->PixelInformation.ReservedMask )
>> +            bpp = set_color(mode_info->PixelInformation.ReservedMask, bpp,
>> +                            &vga_console_info.u.vesa_lfb.rsvd_pos,
>> +                            &vga_console_info.u.vesa_lfb.rsvd_size);
>> +        if ( bpp > 0 )
>> +            break;
>> +        /* fall through */
>> +    default:
>> +        PrintErr(L"Current graphics mode is unsupported!\r\n");
>> +        bpp  = 0;
>> +        break;
>> +    }
>> +    if ( bpp > 0 )
>> +    {
>> +        vga_console_info.video_type = XEN_VGATYPE_EFI_LFB;
>> +        vga_console_info.u.vesa_lfb.gbl_caps = 2; /* possibly non-VGA */
>> +        vga_console_info.u.vesa_lfb.width =
>> +            mode_info->HorizontalResolution;
>> +        vga_console_info.u.vesa_lfb.height = mode_info->VerticalResolution;
>> +        vga_console_info.u.vesa_lfb.bits_per_pixel = bpp;
>> +        vga_console_info.u.vesa_lfb.bytes_per_line =
>> +            (mode_info->PixelsPerScanLine * bpp + 7) >> 3;
>> +        vga_console_info.u.vesa_lfb.lfb_base = gop->Mode->FrameBufferBase;
>> +        vga_console_info.u.vesa_lfb.ext_lfb_base = gop->Mode->FrameBufferBase >> 32;
>> +        vga_console_info.u.vesa_lfb.lfb_size =
>> +            (gop->Mode->FrameBufferSize + 0xffff) >> 16;
>> +    }
>> +#endif
>> +}
> 
> While you move this code, could you please insert blank lines between
> non-fall-through case blocks, and perhaps another one between the switch()
> and the if() blocks? And it looks like
> - the "gop" parameter could also do with becoming pointer-to-const,

I can do that.

> - the expanded #ifdef could do with a comment briefly explaining why Arm
>    needs-special casing.
Agree. I will wait input with the others regarding the #ifdef approach 
before respinning this patch.

Cheers,

-- 
Julien Grall