[PATCH RFC 0/3] xen/arm: Allow dom0 to use the EFI framebuffer

[PATCH RFC 0/3] xen/arm: Allow dom0 to use the EFI framebuffer

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Hi all,

This is a follow-up of the discussion [1].

When booting using EFI on Raspberry Pi 4, the graphical output will
be using the EFI framebuffer.

On baremetal, the information for the graphic output is retrieved
using the boot services. However, under Xen, dom0 has no access
to the EFI boot services. So we need to a different way to inform Dom0.

For x86 PV dom0, this is using the start_info. On Arm we have no such
thing, so this patch will introduce a new hypercall. This will
require a change in Linux [2] (Based on the 5.17-rc2) to issue the
hypercall and fill-up screen info.

I will properly send the Linux patch once we agree on the interface
between Xen and dom0.

With this series + Linux, I am able to use XFCE in dom0 on the
RPi 4 when booting using UEFI + ACPI.

Looking through the dmesg, there are still a few TODOs to handle:
    - Linux is not able to use the BGRT (Invalid address). This is
      because the driver checks the Image address is part of the UEFI
      Boot Services Area. Such area is not exposed to dom0 (yet).
      By-passing the check doesn't much help because all the EFI Boot
      Services area are given to the normal allocator. So we would
      need to reserve them.
    - The Wifi driver is crashing because it is dereferencing a NULL
      pointer (only seem to happen on Xen).
    - There are error messages of drivers trying to access the EFI
      runtime services (e.g. to fetch firmware for a device) but fails.
      This is expected as Xen doesn't yet expose the EFI runtimes
      services yet.

/!\ This has only been tested on Arm. I will sanity test x86 for the
next version.

Cheers,

[1] https://lore.kernel.org/xen-devel/YY3tSAFTCR4r2FaI@mattapan.m5p.com/
[2] https://pastebin.com/ztUm9Bf3

Julien Grall (3):
  xen/efi: Always query the console information and get GOP
  xen/arm: efi: Introduce and fill the vga_console_info
  xen: Introduce a platform sub-op to retrieve the VGA information

 xen/arch/arm/efi/efi-boot.h       |  6 --
 xen/arch/arm/platform_hypercall.c | 15 +++++
 xen/arch/arm/setup.c              |  4 ++
 xen/arch/x86/efi/efi-boot.h       | 72 -----------------------
 xen/common/efi/boot.c             | 96 +++++++++++++++++++++++++++----
 xen/include/public/platform.h     |  2 +
 xen/include/xen/vga.h             |  2 +-
 7 files changed, 106 insertions(+), 91 deletions(-)

-- 
2.32.0

[PATCH RFC 1/3] xen/efi: Always query the console information and get GOP

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Currently, the EFI stub will only query the console information and
get the GOP when using the configuration file.

However, GRUB is never providing the a configuration file. So the
EFI framebuffer will not be usable at least on Arm (support will
be added in a follow-up patch).

Move out the code outside of the configuration file section.

Take the opportunity to remove the variable 'size' which was
set but never used (interestingly GCC is only complaining if it is
initialization when declaring the variable).

With this change, GCC 8.3 will complain of argc potentially been
used unitiatlized. I suspect this is because the argc will
be iniitalized and used in a different if code-blocks. Yet they
are using the same check.

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

----

It is not entirely clear to me why the GOP was only fetched when
the configuration file is used.

I have tested this on RPI4 and it seems to work. Any chance this
was done to workaround an x86 platform?
---
 xen/common/efi/boot.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 12fd0844bd55..80e4e32623c4 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1129,9 +1129,11 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID;
     EFI_LOADED_IMAGE *loaded_image;
     EFI_STATUS status;
-    unsigned int i, argc;
+    /* Initialize argc to stop GCC complaining */
+    unsigned int i, argc = 0;
     CHAR16 **argv, *file_name, *cfg_file_name = NULL, *options = NULL;
-    UINTN gop_mode = ~0;
+    UINTN gop_mode = ~0, cols = 0, rows = 0;
+
     EFI_SHIM_LOCK_PROTOCOL *shim_lock;
     EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
     union string section = { NULL }, name;
@@ -1219,18 +1221,16 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
 
     efi_arch_relocate_image(0);
 
+    if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode,
+                           &cols, &rows) == EFI_SUCCESS )
+        efi_arch_console_init(cols, rows);
+
+    gop = efi_get_gop();
+
     if ( use_cfg_file )
     {
         EFI_FILE_HANDLE dir_handle;
-        UINTN depth, cols, rows, size;
-
-        size = cols = rows = depth = 0;
-
-        if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode,
-                               &cols, &rows) == EFI_SUCCESS )
-            efi_arch_console_init(cols, rows);
-
-        gop = efi_get_gop();
+        UINTN depth = 0;
 
         /* Get the file system interface. */
         dir_handle = get_parent_handle(loaded_image, &file_name);
-- 
2.32.0

[PATCH RFC 2/3] xen/arm: efi: Introduce and fill the vga_console_info

[Julien Grall]

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

[PATCH RFC 3/3] xen: Introduce a platform sub-op to retrieve the VGA information

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

When using EFI, the VGA information is fetched using the EFI
boot services. However, Xen will have exited the boot services.
Therefore, we need to find a different way to pass the information
to dom0.

For PV dom0, they are part of the start_info. But this is not
something that exists on Arm. So the best way would to be to
use a hypercall.

For now the structure layout is based on dom0_vga_console_info
for convenience. I am open on another proposal.

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

----

TODO:
    - Check the structure size has not changed (I would like to
      avoid bumping the platform interface).
---
 xen/arch/arm/platform_hypercall.c | 15 +++++++++++++++
 xen/include/public/platform.h     |  2 ++
 2 files changed, 17 insertions(+)

diff --git a/xen/arch/arm/platform_hypercall.c b/xen/arch/arm/platform_hypercall.c
index 8efac7ee602a..78ad328e2ab8 100644
--- a/xen/arch/arm/platform_hypercall.c
+++ b/xen/arch/arm/platform_hypercall.c
@@ -10,6 +10,7 @@
 #include <xen/sched.h>
 #include <xen/guest_access.h>
 #include <xen/spinlock.h>
+#include <xen/vga.h>
 #include <public/platform.h>
 #include <xsm/xsm.h>
 #include <asm/current.h>
@@ -58,6 +59,20 @@ long do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
             ret = -EINVAL;
         break;
 
+    case XENPF_firmware_info:
+        switch ( op->u.firmware_info.type )
+        {
+        case XEN_FW_VGA_INFO:
+            BUILD_BUG_ON(sizeof(op->u.firmware_info.u.vga) !=
+                         sizeof(vga_console_info));
+            memcpy(&op->u.firmware_info.u.vga, &vga_console_info,
+                   sizeof(vga_console_info));
+            if ( __copy_to_guest(u_xenpf_op, op, 1) )
+                ret = -EFAULT;
+            break;
+        }
+        break;
+
     default:
         ret = -ENOSYS;
         break;
diff --git a/xen/include/public/platform.h b/xen/include/public/platform.h
index a4c0eb62249a..4de42ce6cbc5 100644
--- a/xen/include/public/platform.h
+++ b/xen/include/public/platform.h
@@ -244,6 +244,7 @@ DEFINE_XEN_GUEST_HANDLE(xenpf_efi_runtime_call_t);
 #define  XEN_FW_EFI_PCI_ROM        5
 #define  XEN_FW_EFI_APPLE_PROPERTIES 6
 #define XEN_FW_KBD_SHIFT_FLAGS    5
+#define XEN_FW_VGA_INFO           6
 struct xenpf_firmware_info {
     /* IN variables. */
     uint32_t type;
@@ -311,6 +312,7 @@ struct xenpf_firmware_info {
 
         /* Int16, Fn02: Get keyboard shift flags. */
         uint8_t kbd_shift_flags; /* XEN_FW_KBD_SHIFT_FLAGS */
+        struct dom0_vga_console_info vga;
     } u;
 };
 typedef struct xenpf_firmware_info xenpf_firmware_info_t;
-- 
2.32.0

Re: [PATCH RFC 0/3] xen/arm: Allow dom0 to use the EFI framebuffer

[Jan Beulich]

On 06.02.2022 20:28, Julien Grall wrote:
> Looking through the dmesg, there are still a few TODOs to handle:
>     - Linux is not able to use the BGRT (Invalid address). This is
>       because the driver checks the Image address is part of the UEFI
>       Boot Services Area. Such area is not exposed to dom0 (yet).
>       By-passing the check doesn't much help because all the EFI Boot
>       Services area are given to the normal allocator. So we would
>       need to reserve them.

Respective checking is supposed to by done by (only) Xen in our case.
I thought we do so, hence if there's a bug in there I think it wants
fixing on the Xen side. The Dom0 kernel should never be exposed the
Boot Services areas, and hence it should really trust Xen having done
whatever needs doing.

Jan

Re: [PATCH RFC 1/3] xen/efi: Always query the console information and get GOP

[Jan Beulich]

On 06.02.2022 20:28, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Currently, the EFI stub will only query the console information and
> get the GOP when using the configuration file.
> 
> However, GRUB is never providing the a configuration file. So the
> EFI framebuffer will not be usable at least on Arm (support will
> be added in a follow-up patch).
> 
> Move out the code outside of the configuration file section.
> 
> Take the opportunity to remove the variable 'size' which was
> set but never used (interestingly GCC is only complaining if it is
> initialization when declaring the variable).
> 
> With this change, GCC 8.3 will complain of argc potentially been
> used unitiatlized. I suspect this is because the argc will
> be iniitalized and used in a different if code-blocks. Yet they
> are using the same check.

I'm inclined to suggest this wants to be a separate change, with its
own justification. You're not touching any use of argc here, after
all.

> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> ----
> 
> It is not entirely clear to me why the GOP was only fetched when
> the configuration file is used.
> 
> I have tested this on RPI4 and it seems to work. Any chance this
> was done to workaround an x86 platform?

This was done so in the context of making the code work for Arm. See
commit c38cf865ec82 ("EFI: ignore EFI commandline, skip console setup
when booted from GRUB"), the description of which explicitly says

"Don't do EFI console or video configuration when booted by GRUB.  The EFI boot
 code does some console and video initialization to support native EFI boot from
 the EFI boot manager or EFI shell.  This initlization should not be done when
 booted using GRUB."

What you say now is effectively the opposite (and unlike back then
x86 is now able to use this code path as well, so needs considering
too). Cc-ing Daniel for possibly having a GrUB-side opinion.

Jan

> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1129,9 +1129,11 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>      static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID;
>      EFI_LOADED_IMAGE *loaded_image;
>      EFI_STATUS status;
> -    unsigned int i, argc;
> +    /* Initialize argc to stop GCC complaining */
> +    unsigned int i, argc = 0;
>      CHAR16 **argv, *file_name, *cfg_file_name = NULL, *options = NULL;
> -    UINTN gop_mode = ~0;
> +    UINTN gop_mode = ~0, cols = 0, rows = 0;
> +
>      EFI_SHIM_LOCK_PROTOCOL *shim_lock;
>      EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
>      union string section = { NULL }, name;
> @@ -1219,18 +1221,16 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>  
>      efi_arch_relocate_image(0);
>  
> +    if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode,
> +                           &cols, &rows) == EFI_SUCCESS )
> +        efi_arch_console_init(cols, rows);
> +
> +    gop = efi_get_gop();
> +
>      if ( use_cfg_file )
>      {
>          EFI_FILE_HANDLE dir_handle;
> -        UINTN depth, cols, rows, size;
> -
> -        size = cols = rows = depth = 0;
> -
> -        if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode,
> -                               &cols, &rows) == EFI_SUCCESS )
> -            efi_arch_console_init(cols, rows);
> -
> -        gop = efi_get_gop();
> +        UINTN depth = 0;
>  
>          /* Get the file system interface. */
>          dir_handle = get_parent_handle(loaded_image, &file_name);

Re: [PATCH RFC 2/3] xen/arm: efi: Introduce and fill the vga_console_info

[Jan Beulich]

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

Re: [PATCH RFC 3/3] xen: Introduce a platform sub-op to retrieve the VGA information

[Jan Beulich]

On 06.02.2022 20:28, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> When using EFI, the VGA information is fetched using the EFI
> boot services. However, Xen will have exited the boot services.
> Therefore, we need to find a different way to pass the information
> to dom0.
> 
> For PV dom0, they are part of the start_info. But this is not
> something that exists on Arm. So the best way would to be to
> use a hypercall.
> 
> For now the structure layout is based on dom0_vga_console_info
> for convenience. I am open on another proposal.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Cc-ing Roger as this may want using for PVH Dom0 also on x86; my
first attempt to propagate this information was rejected.

> --- a/xen/include/public/platform.h
> +++ b/xen/include/public/platform.h
> @@ -244,6 +244,7 @@ DEFINE_XEN_GUEST_HANDLE(xenpf_efi_runtime_call_t);
>  #define  XEN_FW_EFI_PCI_ROM        5
>  #define  XEN_FW_EFI_APPLE_PROPERTIES 6
>  #define XEN_FW_KBD_SHIFT_FLAGS    5
> +#define XEN_FW_VGA_INFO           6

Perhaps s/VGA/VIDEO/, despite ...

>  struct xenpf_firmware_info {
>      /* IN variables. */
>      uint32_t type;
> @@ -311,6 +312,7 @@ struct xenpf_firmware_info {
>  
>          /* Int16, Fn02: Get keyboard shift flags. */
>          uint8_t kbd_shift_flags; /* XEN_FW_KBD_SHIFT_FLAGS */
> +        struct dom0_vga_console_info vga;

... the structure name including "vga" (but if the #define is adjusted,
the field name would want to become "video" as well).

Jan

Re: [PATCH RFC 3/3] xen: Introduce a platform sub-op to retrieve the VGA information

[Roger Pau Monné]

On Mon, Feb 07, 2022 at 09:57:55AM +0100, Jan Beulich wrote:
> On 06.02.2022 20:28, Julien Grall wrote:
> > From: Julien Grall <jgrall@amazon.com>
> > 
> > When using EFI, the VGA information is fetched using the EFI
> > boot services. However, Xen will have exited the boot services.
> > Therefore, we need to find a different way to pass the information
> > to dom0.
> > 
> > For PV dom0, they are part of the start_info. But this is not
> > something that exists on Arm. So the best way would to be to
> > use a hypercall.
> > 
> > For now the structure layout is based on dom0_vga_console_info
> > for convenience. I am open on another proposal.
> > 
> > Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> Cc-ing Roger as this may want using for PVH Dom0 also on x86; my
> first attempt to propagate this information was rejected.

I think it's easier to use a Xen specific layout in XENPF, as that's
already a Xen specific interface.

I wonder however if passing the information here (instead of doing it
in the start info or equivalent) could cause a delay in the
initialization of the video console. I guess the same happens when
using the Xen consoles (either the hypercall one or the shared ring),
so it's fine.

> > --- a/xen/include/public/platform.h
> > +++ b/xen/include/public/platform.h
> > @@ -244,6 +244,7 @@ DEFINE_XEN_GUEST_HANDLE(xenpf_efi_runtime_call_t);
> >  #define  XEN_FW_EFI_PCI_ROM        5
> >  #define  XEN_FW_EFI_APPLE_PROPERTIES 6
> >  #define XEN_FW_KBD_SHIFT_FLAGS    5
> > +#define XEN_FW_VGA_INFO           6
> 
> Perhaps s/VGA/VIDEO/, despite ...
> 
> >  struct xenpf_firmware_info {
> >      /* IN variables. */
> >      uint32_t type;
> > @@ -311,6 +312,7 @@ struct xenpf_firmware_info {
> >  
> >          /* Int16, Fn02: Get keyboard shift flags. */
> >          uint8_t kbd_shift_flags; /* XEN_FW_KBD_SHIFT_FLAGS */
> > +        struct dom0_vga_console_info vga;
> 
> ... the structure name including "vga" (but if the #define is adjusted,
> the field name would want to become "video" as well).

It's my understanding that this will forcefully be
XEN_VGATYPE_EFI_LFB, at which point we could consider giving a type
name to the vesa_lfb field of dom0_vga_console_info (video_lfb) and
use the same struct here?

There's no need to propagate XEN_VGATYPE_TEXT_MODE_3 into this
interface.

Thanks, Roger.

Re: [PATCH RFC 3/3] xen: Introduce a platform sub-op to retrieve the VGA information

[Jan Beulich]

On 07.02.2022 12:58, Roger Pau Monné wrote:
> On Mon, Feb 07, 2022 at 09:57:55AM +0100, Jan Beulich wrote:
>> On 06.02.2022 20:28, Julien Grall wrote:
>>> @@ -311,6 +312,7 @@ struct xenpf_firmware_info {
>>>  
>>>          /* Int16, Fn02: Get keyboard shift flags. */
>>>          uint8_t kbd_shift_flags; /* XEN_FW_KBD_SHIFT_FLAGS */
>>> +        struct dom0_vga_console_info vga;
>>
>> ... the structure name including "vga" (but if the #define is adjusted,
>> the field name would want to become "video" as well).
> 
> It's my understanding that this will forcefully be
> XEN_VGATYPE_EFI_LFB, at which point we could consider giving a type
> name to the vesa_lfb field of dom0_vga_console_info (video_lfb) and
> use the same struct here?
> 
> There's no need to propagate XEN_VGATYPE_TEXT_MODE_3 into this
> interface.

Hmm, yes, this is probably better / more clean. Julien, thoughts?

Jan

Re: [PATCH RFC 1/3] xen/efi: Always query the console information and get GOP

[Julien Grall]

Hi Jan,

On 07/02/2022 08:46, Jan Beulich wrote:
> On 06.02.2022 20:28, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Currently, the EFI stub will only query the console information and
>> get the GOP when using the configuration file.
>>
>> However, GRUB is never providing the a configuration file. So the
>> EFI framebuffer will not be usable at least on Arm (support will
>> be added in a follow-up patch).
>>
>> Move out the code outside of the configuration file section.
>>
>> Take the opportunity to remove the variable 'size' which was
>> set but never used (interestingly GCC is only complaining if it is
>> initialization when declaring the variable).
>>
>> With this change, GCC 8.3 will complain of argc potentially been
>> used unitiatlized. I suspect this is because the argc will
>> be iniitalized and used in a different if code-blocks. Yet they
>> are using the same check.
> 
> I'm inclined to suggest this wants to be a separate change, with its
> own justification. You're not touching any use of argc here, after
> all.

Ok. I will split it.

> 
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>
>> ----
>>
>> It is not entirely clear to me why the GOP was only fetched when
>> the configuration file is used.
>>
>> I have tested this on RPI4 and it seems to work. Any chance this
>> was done to workaround an x86 platform?
> 
> This was done so in the context of making the code work for Arm. See
> commit c38cf865ec82 ("EFI: ignore EFI commandline, skip console setup
> when booted from GRUB"), the description of which explicitly says
> 
> "Don't do EFI console or video configuration when booted by GRUB.  The EFI boot
>   code does some console and video initialization to support native EFI boot from
>   the EFI boot manager or EFI shell.  This initlization should not be done when
>   booted using GRUB."

I read that and still couldn't figure out why this was done like that.

> 
> What you say now is effectively the opposite (and unlike back then
> x86 is now able to use this code path as well, so needs considering
> too). Cc-ing Daniel for possibly having a GrUB-side opinion.

I am quite interested to know the answer. Linux is able to use the EFI 
framebuffer when booting via GRUB. So I am a bit puzzled why we are 
preventing this setup on dom0/Xen.

Cheers,

-- 
Julien Grall

Re: [PATCH RFC 2/3] xen/arm: efi: Introduce and fill the vga_console_info

[Julien Grall]

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

Re: [PATCH RFC 3/3] xen: Introduce a platform sub-op to retrieve the VGA information

[Julien Grall]

Hi Roger,

On 07/02/2022 11:58, Roger Pau Monné wrote:
> On Mon, Feb 07, 2022 at 09:57:55AM +0100, Jan Beulich wrote:
>> On 06.02.2022 20:28, Julien Grall wrote:
>>> From: Julien Grall <jgrall@amazon.com>
>>>
>>> When using EFI, the VGA information is fetched using the EFI
>>> boot services. However, Xen will have exited the boot services.
>>> Therefore, we need to find a different way to pass the information
>>> to dom0.
>>>
>>> For PV dom0, they are part of the start_info. But this is not
>>> something that exists on Arm. So the best way would to be to
>>> use a hypercall.
>>>
>>> For now the structure layout is based on dom0_vga_console_info
>>> for convenience. I am open on another proposal.
>>>
>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>
>> Cc-ing Roger as this may want using for PVH Dom0 also on x86; my
>> first attempt to propagate this information was rejected.
> 
> I think it's easier to use a Xen specific layout in XENPF, as that's
> already a Xen specific interface.
> 
> I wonder however if passing the information here (instead of doing it
> in the start info or equivalent) could cause a delay in the
> initialization of the video console.

My current plan for Arm is to issue the hypercall as part of an 
earlyinit call. But we can do much earlier (i.e. xen_early_init() which 
is called from setup_arch()) if necessary.

This should be early enough for Arm. How about x86?

> I guess the same happens when
> using the Xen consoles (either the hypercall one or the shared ring),
> so it's fine.
> 
>>> --- a/xen/include/public/platform.h
>>> +++ b/xen/include/public/platform.h
>>> @@ -244,6 +244,7 @@ DEFINE_XEN_GUEST_HANDLE(xenpf_efi_runtime_call_t);
>>>   #define  XEN_FW_EFI_PCI_ROM        5
>>>   #define  XEN_FW_EFI_APPLE_PROPERTIES 6
>>>   #define XEN_FW_KBD_SHIFT_FLAGS    5
>>> +#define XEN_FW_VGA_INFO           6
>>
>> Perhaps s/VGA/VIDEO/, despite ...
>>
>>>   struct xenpf_firmware_info {
>>>       /* IN variables. */
>>>       uint32_t type;
>>> @@ -311,6 +312,7 @@ struct xenpf_firmware_info {
>>>   
>>>           /* Int16, Fn02: Get keyboard shift flags. */
>>>           uint8_t kbd_shift_flags; /* XEN_FW_KBD_SHIFT_FLAGS */
>>> +        struct dom0_vga_console_info vga;
>>
>> ... the structure name including "vga" (but if the #define is adjusted,
>> the field name would want to become "video" as well).
> 

[...]

(Re-ordered the quote as it makes more sense for my reply)

> There's no need to propagate XEN_VGATYPE_TEXT_MODE_3 into this
> interface.

So for Arm, we are only caring about XEN_VGATYPE_EFI_LFB. I wasn't sure 
what would be your need on x86. Hence, why I keep it.

If you don't need then, then I am happy to trim the structure for the 
new hypercall.

 > It's my understanding that this will forcefully be
 > XEN_VGATYPE_EFI_LFB, at which point we could consider giving a type
 > name to the vesa_lfb field of dom0_vga_console_info (video_lfb) and
 > use the same struct here?>

Just to clarify, are you suggesting to only pass video_lfb? IOW, we will 
always assume it is an EFI framebuffer and not pass the video_type.

Cheers,

-- 
Julien Grall

Re: [PATCH RFC 3/3] xen: Introduce a platform sub-op to retrieve the VGA information

[Roger Pau Monné]

On Mon, Feb 07, 2022 at 07:24:12PM +0000, Julien Grall wrote:
> Hi Roger,
> 
> On 07/02/2022 11:58, Roger Pau Monné wrote:
> > On Mon, Feb 07, 2022 at 09:57:55AM +0100, Jan Beulich wrote:
> > > On 06.02.2022 20:28, Julien Grall wrote:
> > > > From: Julien Grall <jgrall@amazon.com>
> > > > 
> > > > When using EFI, the VGA information is fetched using the EFI
> > > > boot services. However, Xen will have exited the boot services.
> > > > Therefore, we need to find a different way to pass the information
> > > > to dom0.
> > > > 
> > > > For PV dom0, they are part of the start_info. But this is not
> > > > something that exists on Arm. So the best way would to be to
> > > > use a hypercall.
> > > > 
> > > > For now the structure layout is based on dom0_vga_console_info
> > > > for convenience. I am open on another proposal.
> > > > 
> > > > Signed-off-by: Julien Grall <jgrall@amazon.com>
> > > 
> > > Cc-ing Roger as this may want using for PVH Dom0 also on x86; my
> > > first attempt to propagate this information was rejected.
> > 
> > I think it's easier to use a Xen specific layout in XENPF, as that's
> > already a Xen specific interface.
> > 
> > I wonder however if passing the information here (instead of doing it
> > in the start info or equivalent) could cause a delay in the
> > initialization of the video console.
> 
> My current plan for Arm is to issue the hypercall as part of an earlyinit
> call. But we can do much earlier (i.e. xen_early_init() which is called from
> setup_arch()) if necessary.
> 
> This should be early enough for Arm. How about x86?

Yes, I think that's fine for x86 also.

> > I guess the same happens when
> > using the Xen consoles (either the hypercall one or the shared ring),
> > so it's fine.
> > 
> > > > --- a/xen/include/public/platform.h
> > > > +++ b/xen/include/public/platform.h
> > > > @@ -244,6 +244,7 @@ DEFINE_XEN_GUEST_HANDLE(xenpf_efi_runtime_call_t);
> > > >   #define  XEN_FW_EFI_PCI_ROM        5
> > > >   #define  XEN_FW_EFI_APPLE_PROPERTIES 6
> > > >   #define XEN_FW_KBD_SHIFT_FLAGS    5
> > > > +#define XEN_FW_VGA_INFO           6
> > > 
> > > Perhaps s/VGA/VIDEO/, despite ...
> > > 
> > > >   struct xenpf_firmware_info {
> > > >       /* IN variables. */
> > > >       uint32_t type;
> > > > @@ -311,6 +312,7 @@ struct xenpf_firmware_info {
> > > >           /* Int16, Fn02: Get keyboard shift flags. */
> > > >           uint8_t kbd_shift_flags; /* XEN_FW_KBD_SHIFT_FLAGS */
> > > > +        struct dom0_vga_console_info vga;
> > > 
> > > ... the structure name including "vga" (but if the #define is adjusted,
> > > the field name would want to become "video" as well).
> > 
> 
> [...]
> 
> (Re-ordered the quote as it makes more sense for my reply)
> 
> > There's no need to propagate XEN_VGATYPE_TEXT_MODE_3 into this
> > interface.
> 
> So for Arm, we are only caring about XEN_VGATYPE_EFI_LFB. I wasn't sure what
> would be your need on x86. Hence, why I keep it.
> 
> If you don't need then, then I am happy to trim the structure for the new
> hypercall.

Oh, so I was slightly confused. You are adding a top level
XEN_FW_VIDEO_INFO, not a EFI sub-op one. In which case, yes, we would
need to keep the MODE_3 as part of the interface.

> > It's my understanding that this will forcefully be
> > XEN_VGATYPE_EFI_LFB, at which point we could consider giving a type
> > name to the vesa_lfb field of dom0_vga_console_info (video_lfb) and
> > use the same struct here?>
> 
> Just to clarify, are you suggesting to only pass video_lfb? IOW, we will
> always assume it is an EFI framebuffer and not pass the video_type.

That would be the case if we add a XEN_FW_EFI_VIDEO sub option, if
instead we add a top level one (XEN_FW_VIDEO_INFO) we need to keep the
mode 3 support.

It might be best for x86 to introduce a global XEN_FW_VIDEO_INFO, as
we can then convey all the video information regardless of the boot
mode.

FWIW, I'm not a huge fan of the struct name (I would rather prefer
video_info or some such), but that ship sailed long time ago.

Thanks, Roger.

Re: [PATCH RFC 1/3] xen/efi: Always query the console information and get GOP

[Jan Beulich]

On 07.02.2022 19:52, Julien Grall wrote:
> On 07/02/2022 08:46, Jan Beulich wrote:
>> On 06.02.2022 20:28, Julien Grall wrote:
>>> It is not entirely clear to me why the GOP was only fetched when
>>> the configuration file is used.
>>>
>>> I have tested this on RPI4 and it seems to work. Any chance this
>>> was done to workaround an x86 platform?
>>
>> This was done so in the context of making the code work for Arm. See
>> commit c38cf865ec82 ("EFI: ignore EFI commandline, skip console setup
>> when booted from GRUB"), the description of which explicitly says
>>
>> "Don't do EFI console or video configuration when booted by GRUB.  The EFI boot
>>   code does some console and video initialization to support native EFI boot from
>>   the EFI boot manager or EFI shell.  This initlization should not be done when
>>   booted using GRUB."
> 
> I read that and still couldn't figure out why this was done like that.
> 
>>
>> What you say now is effectively the opposite (and unlike back then
>> x86 is now able to use this code path as well, so needs considering
>> too). Cc-ing Daniel for possibly having a GrUB-side opinion.
> 
> I am quite interested to know the answer. Linux is able to use the EFI 
> framebuffer when booting via GRUB. So I am a bit puzzled why we are 
> preventing this setup on dom0/Xen.

To be honest - same here.

Jan

Re: [PATCH RFC 2/3] xen/arm: efi: Introduce and fill the vga_console_info

[Stefano Stabellini]

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
> 

Re: [PATCH RFC 1/3] xen/efi: Always query the console information and get GOP

[Elliott Mitchell]

On Mon, Feb 07, 2022 at 06:52:57PM +0000, Julien Grall wrote:
> On 07/02/2022 08:46, Jan Beulich wrote:
> > On 06.02.2022 20:28, Julien Grall wrote:
> >>
> >> It is not entirely clear to me why the GOP was only fetched when
> >> the configuration file is used.
> >>
> >> I have tested this on RPI4 and it seems to work. Any chance this
> >> was done to workaround an x86 platform?
> > 
> > This was done so in the context of making the code work for Arm. See
> > commit c38cf865ec82 ("EFI: ignore EFI commandline, skip console setup
> > when booted from GRUB"), the description of which explicitly says
> > 
> > "Don't do EFI console or video configuration when booted by GRUB.  The EFI boot
> >   code does some console and video initialization to support native EFI boot from
> >   the EFI boot manager or EFI shell.  This initlization should not be done when
> >   booted using GRUB."
> 
> I read that and still couldn't figure out why this was done like that.

The most likely motivation was simply "Eww!  ACPI/UEFI use gobs of
memory!  Purge the abomination!"

Unfortunately ACPI/UEFI are large an complex due to trying to solve a
large and complex problem.  ACPI/UEFI attempt to provide an OS agnostic
presentation of the hardware layout.  Whereas device-trees are a common
*format* for presenting hardware to *an* OS (similar to how JSON is a
common format).

Due to the size and complexity, most developers have preferred the
simpler device-tree format even though that severely limits OS choice.
As such, nuking ACPI/UEFI's presence is common in the ARM world.  Versus
the x86 world where Intel dragged everyone onto ACPI/UEFI.

One can see this in patches like Roman Shaposhnik's "Making full 2G of
memory available to Xen on HiKey" which simply tosses EFI into the
garbage bin as useless overhead.

Yet the ARM world is now large enough to justify OS-agnostic solutions
such as ACPI/UEFI.  The standards behind device-trees might be heading in
this direction, but they're way behind.




You stated your patch was for 5.17-rc2.  How much backporting would you
expect this patch to be viable for?  (I'm unsure how much churn is
occuring in the relevant portions of Linux) The long-term branches of
Linux include 5.4.179, 5.10.100 and 5.15.23.  `patch` indicated it could
apply to 5.10.92 source with fuzz (hmm).  This suggests 5.15 is likely
viable, but 5.10 is risky and 5.4 is a very long shot.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445

Re: [PATCH RFC 1/3] xen/efi: Always query the console information and get GOP

[Julien Grall]

Hi,

On 12/02/2022 01:33, Elliott Mitchell wrote:
> On Mon, Feb 07, 2022 at 06:52:57PM +0000, Julien Grall wrote:
>> On 07/02/2022 08:46, Jan Beulich wrote:
>>> On 06.02.2022 20:28, Julien Grall wrote:
>>>>
>>>> It is not entirely clear to me why the GOP was only fetched when
>>>> the configuration file is used.
>>>>
>>>> I have tested this on RPI4 and it seems to work. Any chance this
>>>> was done to workaround an x86 platform?
>>>
>>> This was done so in the context of making the code work for Arm. See
>>> commit c38cf865ec82 ("EFI: ignore EFI commandline, skip console setup
>>> when booted from GRUB"), the description of which explicitly says
>>>
>>> "Don't do EFI console or video configuration when booted by GRUB.  The EFI boot
>>>    code does some console and video initialization to support native EFI boot from
>>>    the EFI boot manager or EFI shell.  This initlization should not be done when
>>>    booted using GRUB."
>>
>> I read that and still couldn't figure out why this was done like that.
> 
> The most likely motivation was simply "Eww!  ACPI/UEFI use gobs of
> memory!  Purge the abomination!"
> 
> Unfortunately ACPI/UEFI are large an complex due to trying to solve a
> large and complex problem.  ACPI/UEFI attempt to provide an OS agnostic
> presentation of the hardware layout.  Whereas device-trees are a common
> *format* for presenting hardware to *an* OS (similar to how JSON is a
> common format).
> 
> Due to the size and complexity, most developers have preferred the
> simpler device-tree format even though that severely limits OS choice.
> As such, nuking ACPI/UEFI's presence is common in the ARM world.  Versus > the x86 world where Intel dragged everyone onto ACPI/UEFI.
> 
> One can see this in patches like Roman Shaposhnik's "Making full 2G of
> memory available to Xen on HiKey" which simply tosses EFI into the
> garbage bin as useless overhead.

I couldn't find a series with this name in my archives. By any chance, 
are you referring to [1]?

[...]

> 
> You stated your patch was for 5.17-rc2.  How much backporting would you
> expect this patch to be viable for?  (I'm unsure how much churn is
> occuring in the relevant portions of Linux) The long-term branches of
> Linux include 5.4.179, 5.10.100 and 5.15.23.  `patch` indicated it could
> apply to 5.10.92 source with fuzz (hmm).  This suggests 5.15 is likely
> viable, but 5.10 is risky and 5.4 is a very long shot.
I haven't looked at backports, so I don't know. But this is not a patch 
I would consider to request for backport myself because IHMO this is 
adding device support.

Cheers,

[1] 
https://lore.kernel.org/all/CAMmSBy8Zh00tebTvz=__GDv478++b-2t4248YnkH0W9DVgqKbw@mail.gmail.com/

-- 
Julien Grall

Re: [PATCH RFC 1/3] xen/efi: Always query the console information and get GOP

[Elliott Mitchell]

On Sat, Feb 12, 2022 at 01:10:52PM +0000, Julien Grall wrote:
> 
> On 12/02/2022 01:33, Elliott Mitchell wrote:
> > On Mon, Feb 07, 2022 at 06:52:57PM +0000, Julien Grall wrote:
> >> On 07/02/2022 08:46, Jan Beulich wrote:
> >>> On 06.02.2022 20:28, Julien Grall wrote:
> >>>>
> >>>> It is not entirely clear to me why the GOP was only fetched when
> >>>> the configuration file is used.
> >>>>
> >>>> I have tested this on RPI4 and it seems to work. Any chance this
> >>>> was done to workaround an x86 platform?
> >>>
> >>> This was done so in the context of making the code work for Arm. See
> >>> commit c38cf865ec82 ("EFI: ignore EFI commandline, skip console setup
> >>> when booted from GRUB"), the description of which explicitly says
> >>>
> >>> "Don't do EFI console or video configuration when booted by GRUB.  The EFI boot
> >>>    code does some console and video initialization to support native EFI boot from
> >>>    the EFI boot manager or EFI shell.  This initlization should not be done when
> >>>    booted using GRUB."
> >>
> >> I read that and still couldn't figure out why this was done like that.
> > 
> > The most likely motivation was simply "Eww!  ACPI/UEFI use gobs of
> > memory!  Purge the abomination!"
> > 
> > Unfortunately ACPI/UEFI are large an complex due to trying to solve a
> > large and complex problem.  ACPI/UEFI attempt to provide an OS agnostic
> > presentation of the hardware layout.  Whereas device-trees are a common
> > *format* for presenting hardware to *an* OS (similar to how JSON is a
> > common format).
> > 
> > Due to the size and complexity, most developers have preferred the
> > simpler device-tree format even though that severely limits OS choice.
> > As such, nuking ACPI/UEFI's presence is common in the ARM world.  Versus > the x86 world where Intel dragged everyone onto ACPI/UEFI.
> > 
> > One can see this in patches like Roman Shaposhnik's "Making full 2G of
> > memory available to Xen on HiKey" which simply tosses EFI into the
> > garbage bin as useless overhead.
> 
> I couldn't find a series with this name in my archives. By any chance, 
> are you referring to [1]?

The patch may have appeared under more than one title.  The raw content
is publically visible at:

https://github.com/lf-edge/eve/blob/master/pkg/xen/arch/aarch64/0002-arm-efi-mem-detection.patch

The issue is few ARM projects are really trying to support enough
different devices for ACPI/UEFI to hit their forte.  At which point
ACPI/UEFI get treated as worthless overhead.



> > You stated your patch was for 5.17-rc2.  How much backporting would you
> > expect this patch to be viable for?  (I'm unsure how much churn is
> > occuring in the relevant portions of Linux) The long-term branches of
> > Linux include 5.4.179, 5.10.100 and 5.15.23.  `patch` indicated it could
> > apply to 5.10.92 source with fuzz (hmm).  This suggests 5.15 is likely
> > viable, but 5.10 is risky and 5.4 is a very long shot.

> I haven't looked at backports, so I don't know. But this is not a patch 
> I would consider to request for backport myself because IHMO this is 
> adding device support.

People who need this feature are likely to backport it themselves.

Looking at the 5.10.92 source I've got handy, it appears reasonable.  The
fuzz appears to have be a missed newline when I attempted to grab the
patch from the site you used.

You want test reports yet?


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445

Re: [PATCH RFC 1/3] xen/efi: Always query the console information and get GOP

[Julien Grall]

On 12/02/2022 18:20, Elliott Mitchell wrote:
> On Sat, Feb 12, 2022 at 01:10:52PM +0000, Julien Grall wrote:
>>
>> On 12/02/2022 01:33, Elliott Mitchell wrote:
>>> On Mon, Feb 07, 2022 at 06:52:57PM +0000, Julien Grall wrote:
>>>> On 07/02/2022 08:46, Jan Beulich wrote:
>>>>> On 06.02.2022 20:28, Julien Grall wrote:
>>>>>>
>>>>>> It is not entirely clear to me why the GOP was only fetched when
>>>>>> the configuration file is used.
>>>>>>
>>>>>> I have tested this on RPI4 and it seems to work. Any chance this
>>>>>> was done to workaround an x86 platform?
>>>>>
>>>>> This was done so in the context of making the code work for Arm. See
>>>>> commit c38cf865ec82 ("EFI: ignore EFI commandline, skip console setup
>>>>> when booted from GRUB"), the description of which explicitly says
>>>>>
>>>>> "Don't do EFI console or video configuration when booted by GRUB.  The EFI boot
>>>>>     code does some console and video initialization to support native EFI boot from
>>>>>     the EFI boot manager or EFI shell.  This initlization should not be done when
>>>>>     booted using GRUB."
>>>>
>>>> I read that and still couldn't figure out why this was done like that.
>>>
>>> The most likely motivation was simply "Eww!  ACPI/UEFI use gobs of
>>> memory!  Purge the abomination!"
>>>
>>> Unfortunately ACPI/UEFI are large an complex due to trying to solve a
>>> large and complex problem.  ACPI/UEFI attempt to provide an OS agnostic
>>> presentation of the hardware layout.  Whereas device-trees are a common
>>> *format* for presenting hardware to *an* OS (similar to how JSON is a
>>> common format).
>>>
>>> Due to the size and complexity, most developers have preferred the
>>> simpler device-tree format even though that severely limits OS choice.
>>> As such, nuking ACPI/UEFI's presence is common in the ARM world.  Versus > the x86 world where Intel dragged everyone onto ACPI/UEFI.
>>>
>>> One can see this in patches like Roman Shaposhnik's "Making full 2G of
>>> memory available to Xen on HiKey" which simply tosses EFI into the
>>> garbage bin as useless overhead.
>>
>> I couldn't find a series with this name in my archives. By any chance,
>> are you referring to [1]?
> 
> The patch may have appeared under more than one title.  The raw content
> is publically visible at:
> 
> https://github.com/lf-edge/eve/blob/master/pkg/xen/arch/aarch64/0002-arm-efi-mem-detection.patch
> 
> The issue is few ARM projects are really trying to support enough
> different devices for ACPI/UEFI to hit their forte.  At which point
> ACPI/UEFI get treated as worthless overhead.

Thanks! I wasn't aware of that patch.

>>> You stated your patch was for 5.17-rc2.  How much backporting would you
>>> expect this patch to be viable for?  (I'm unsure how much churn is
>>> occuring in the relevant portions of Linux) The long-term branches of
>>> Linux include 5.4.179, 5.10.100 and 5.15.23.  `patch` indicated it could
>>> apply to 5.10.92 source with fuzz (hmm).  This suggests 5.15 is likely
>>> viable, but 5.10 is risky and 5.4 is a very long shot.
> 
>> I haven't looked at backports, so I don't know. But this is not a patch
>> I would consider to request for backport myself because IHMO this is
>> adding device support.
> 
> People who need this feature are likely to backport it themselves.

Well, anyone can ask backport. I am not planning to ask nor do the 
backport work myselff. But feel free to send an e-mail to stable after 
the Linux patch is merged.

> 
> Looking at the 5.10.92 source I've got handy, it appears reasonable.  The
> fuzz appears to have be a missed newline when I attempted to grab the
> patch from the site you used.
> 
> You want test reports yet?

I need to do some respin. So I would say wait until the next version.

Cheers,

-- 
Julien Grall

Re: [PATCH RFC 1/3] xen/efi: Always query the console information and get GOP

[Julien Grall]

Hi Daniel,

Gentle ping. Your feedback on this approach would be helpful.

On 07/02/2022 08:46, Jan Beulich wrote:
> On 06.02.2022 20:28, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Currently, the EFI stub will only query the console information and
>> get the GOP when using the configuration file.
>>
>> However, GRUB is never providing the a configuration file. So the
>> EFI framebuffer will not be usable at least on Arm (support will
>> be added in a follow-up patch).
>>
>> Move out the code outside of the configuration file section.
>>
>> Take the opportunity to remove the variable 'size' which was
>> set but never used (interestingly GCC is only complaining if it is
>> initialization when declaring the variable).
>>
>> With this change, GCC 8.3 will complain of argc potentially been
>> used unitiatlized. I suspect this is because the argc will
>> be iniitalized and used in a different if code-blocks. Yet they
>> are using the same check.
> 
> I'm inclined to suggest this wants to be a separate change, with its
> own justification. You're not touching any use of argc here, after
> all.
> 
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>
>> ----
>>
>> It is not entirely clear to me why the GOP was only fetched when
>> the configuration file is used.
>>
>> I have tested this on RPI4 and it seems to work. Any chance this
>> was done to workaround an x86 platform?
> 
> This was done so in the context of making the code work for Arm. See
> commit c38cf865ec82 ("EFI: ignore EFI commandline, skip console setup
> when booted from GRUB"), the description of which explicitly says
> 
> "Don't do EFI console or video configuration when booted by GRUB.  The EFI boot
>   code does some console and video initialization to support native EFI boot from
>   the EFI boot manager or EFI shell.  This initlization should not be done when
>   booted using GRUB."
> 
> What you say now is effectively the opposite (and unlike back then
> x86 is now able to use this code path as well, so needs considering
> too). Cc-ing Daniel for possibly having a GrUB-side opinion.
> 
> Jan
> 
>> --- a/xen/common/efi/boot.c
>> +++ b/xen/common/efi/boot.c
>> @@ -1129,9 +1129,11 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>>       static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID;
>>       EFI_LOADED_IMAGE *loaded_image;
>>       EFI_STATUS status;
>> -    unsigned int i, argc;
>> +    /* Initialize argc to stop GCC complaining */
>> +    unsigned int i, argc = 0;
>>       CHAR16 **argv, *file_name, *cfg_file_name = NULL, *options = NULL;
>> -    UINTN gop_mode = ~0;
>> +    UINTN gop_mode = ~0, cols = 0, rows = 0;
>> +
>>       EFI_SHIM_LOCK_PROTOCOL *shim_lock;
>>       EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
>>       union string section = { NULL }, name;
>> @@ -1219,18 +1221,16 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>>   
>>       efi_arch_relocate_image(0);
>>   
>> +    if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode,
>> +                           &cols, &rows) == EFI_SUCCESS )
>> +        efi_arch_console_init(cols, rows);
>> +
>> +    gop = efi_get_gop();
>> +
>>       if ( use_cfg_file )
>>       {
>>           EFI_FILE_HANDLE dir_handle;
>> -        UINTN depth, cols, rows, size;
>> -
>> -        size = cols = rows = depth = 0;
>> -
>> -        if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode,
>> -                               &cols, &rows) == EFI_SUCCESS )
>> -            efi_arch_console_init(cols, rows);
>> -
>> -        gop = efi_get_gop();
>> +        UINTN depth = 0;
>>   
>>           /* Get the file system interface. */
>>           dir_handle = get_parent_handle(loaded_image, &file_name);
> 

Cheers,

-- 
Julien Grall