Re: [PATCH RFC 2/3] xen/arm: efi: Introduce and fill the vga_console_info
On Sun, 6 Feb 2022, Julien Grall wrote:
> From: Julien Grall
>
> 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
>
>
>
> 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
> #include
> #include
> #include
> @@ -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,
> -_console_info.u.vesa_lfb.red_pos,
> -_console_info.u.vesa_lfb.red_size);
> -bpp = set_color(mode_info->PixelInformation.GreenMask, bpp,
> -_console_info.u.vesa_lfb.green_pos,
> -_console_info.u.vesa_lfb.green_size);
> -bpp = set_color(mode_info->PixelInformation.BlueMask, bpp,
> -_console_info.u.vesa_lfb.blue_pos,
> -_console_info.u.vesa_lfb.blue_size);
> -if (
Re: [PATCH RFC 2/3] xen/arm: efi: Introduce and fill the vga_console_info
Hi,
On 07/02/2022 08:53, Jan Beulich wrote:
On 06.02.2022 20:28, Julien Grall wrote:
From: Julien Grall
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
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,
+_console_info.u.vesa_lfb.red_pos,
+_console_info.u.vesa_lfb.red_size);
+bpp = set_color(mode_info->PixelInformation.GreenMask, bpp,
+_console_info.u.vesa_lfb.green_pos,
+_console_info.u.vesa_lfb.green_size);
+bpp = set_color(mode_info->PixelInformation.BlueMask, bpp,
+_console_info.u.vesa_lfb.blue_pos,
+_console_info.u.vesa_lfb.blue_size);
+if ( mode_info->PixelInformation.ReservedMask )
+bpp = set_color(mode_info->PixelInformation.ReservedMask, bpp,
+_console_info.u.vesa_lfb.rsvd_pos,
+_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 + 0x) >> 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
Re: [PATCH RFC 2/3] xen/arm: efi: Introduce and fill the vga_console_info
On 06.02.2022 20:28, Julien Grall wrote:
> From: Julien Grall
>
> 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
>
>
>
> 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,
> +_console_info.u.vesa_lfb.red_pos,
> +_console_info.u.vesa_lfb.red_size);
> +bpp = set_color(mode_info->PixelInformation.GreenMask, bpp,
> +_console_info.u.vesa_lfb.green_pos,
> +_console_info.u.vesa_lfb.green_size);
> +bpp = set_color(mode_info->PixelInformation.BlueMask, bpp,
> +_console_info.u.vesa_lfb.blue_pos,
> +_console_info.u.vesa_lfb.blue_size);
> +if ( mode_info->PixelInformation.ReservedMask )
> +bpp = set_color(mode_info->PixelInformation.ReservedMask, bpp,
> +_console_info.u.vesa_lfb.rsvd_pos,
> +_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 + 0x) >> 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
[PATCH RFC 2/3] xen/arm: efi: Introduce and fill the vga_console_info
From: Julien Grall
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
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
#include
#include
#include
@@ -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,
-_console_info.u.vesa_lfb.red_pos,
-_console_info.u.vesa_lfb.red_size);
-bpp = set_color(mode_info->PixelInformation.GreenMask, bpp,
-_console_info.u.vesa_lfb.green_pos,
-_console_info.u.vesa_lfb.green_size);
-bpp = set_color(mode_info->PixelInformation.BlueMask, bpp,
-_console_info.u.vesa_lfb.blue_pos,
-_console_info.u.vesa_lfb.blue_size);
-if ( mode_info->PixelInformation.ReservedMask )
-bpp = set_color(mode_info->PixelInformation.ReservedMask, bpp,
-_console_info.u.vesa_lfb.rsvd_pos,
-_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;
-
