* [PATCH v4 0/8] x86/boot: (mostly) video mode handling adjustments
@ 2022-03-31 9:42 Jan Beulich
2022-03-31 9:44 ` [PATCH v4 1/8] x86/boot: make "vga=current" work with graphics modes Jan Beulich
` (7 more replies)
0 siblings, 8 replies; 38+ messages in thread
From: Jan Beulich @ 2022-03-31 9:42 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné
v4 largely has some re-basing changes compared to v3, plus one new patch.
I have to admit that I find it particularly sad that the enhancement done
by patch 1 has not made 4.16. This series had been submitted well in time.
1: make "vga=current" work with graphics modes
2: obtain video info from boot loader
3: EFI: retrieve EDID
4: simplify mode_table
5: fold branches in video handling code
6: fold/replace moves in video handling code
7: LEA -> MOV in video handling code
8: fold two MOVs into an ADD
Jan
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v4 1/8] x86/boot: make "vga=current" work with graphics modes
2022-03-31 9:42 [PATCH v4 0/8] x86/boot: (mostly) video mode handling adjustments Jan Beulich
@ 2022-03-31 9:44 ` Jan Beulich
2022-04-04 14:49 ` Roger Pau Monné
2022-04-05 8:45 ` Roger Pau Monné
2022-03-31 9:45 ` [PATCH v4 2/8] x86/boot: obtain video info from boot loader Jan Beulich
` (6 subsequent siblings)
7 siblings, 2 replies; 38+ messages in thread
From: Jan Beulich @ 2022-03-31 9:44 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, Wei Liu, Roger Pau Monné,
community.manager, Henry Wang
GrUB2 can be told to leave the screen in the graphics mode it has been
using (or any other one), via "set gfxpayload=keep" (or suitable
variants thereof). In this case we can avoid doing another mode switch
ourselves. This in particular avoids possibly setting the screen to a
less desirable mode: On one of my test systems the set of modes
reported available by the VESA BIOS depends on whether the interposed
KVM switch has that machine set as the active one. If it's not active,
only modes up to 1024x768 get reported, while when active 1280x1024
modes are also included. For things to always work with an explicitly
specified mode (via the "vga=" option), that mode therefore needs be a
1024x768 one.
For some reason this only works for me with "multiboot2" (and
"module2"); "multiboot" (and "module") still forces the screen into text
mode, despite my reading of the sources suggesting otherwise.
For starters I'm limiting this to graphics modes; I do think this ought
to also work for text modes, but
- I can't tell whether GrUB2 can set any text mode other than 80x25
(I've only found plain "text" to be valid as a "gfxpayload" setting),
- I'm uncertain whether supporting that is worth it, since I'm uncertain
how many people would be running their systems/screens in text mode,
- I'd like to limit the amount of code added to the realmode trampoline.
For starters I'm also limiting mode information retrieval to raw BIOS
accesses. This will allow things to work (in principle) also with other
boot environments where a graphics mode can be left in place. The
downside is that this then still is dependent upon switching back to
real mode, so retrieving the needed information from multiboot info is
likely going to be desirable down the road.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I'm not convinced boot_vid_mode really needs setting here; I'm doing so
mainly because setvesabysize also does.
---
v4: Add changelog entry.
v2: Use 0x9b instead of 0x99 for attributes check: I think the value
used by check_vesa also wants to be converted, to match vesa2's.
(Perhaps the value wants to become a #define, albeit before doing so
I'd question the requirement of the mode to be a color one.)
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,10 @@ The format is based on [Keep a Changelog
## [unstable UNRELEASED](https://xenbits.xen.org/gitweb/?p=xen.git;a=shortlog;h=staging) - TBD
+### Changed
+ - On x86 "vga=current" can now be used together with GrUB2's gfxpayload setting. Note that
+ this requires use of "multiboot2" (and "module2") as the GrUB commands loading Xen.
+
### Removed / support downgraded
- dropped support for the (x86-only) "vesa-mtrr" and "vesa-remap" command line options
--- a/xen/arch/x86/boot/video.S
+++ b/xen/arch/x86/boot/video.S
@@ -575,7 +575,6 @@ set14: movw $0x1111, %ax
movb $0x01, %ah # Define cursor scan lines 11-12
movw $0x0b0c, %cx
int $0x10
-set_current:
stc
ret
@@ -693,6 +692,39 @@ vga_modes:
.word VIDEO_80x60, 0x50,0x3c,0 # 80x60
vga_modes_end:
+# If the current mode is a VESA graphics one, obtain its parameters.
+set_current:
+ leaw vesa_glob_info, %di
+ movw $0x4f00, %ax
+ int $0x10
+ cmpw $0x004f, %ax
+ jne .Lsetc_done
+
+ movw $0x4f03, %ax
+ int $0x10
+ cmpw $0x004f, %ax
+ jne .Lsetc_done
+
+ leaw vesa_mode_info, %di # Get mode information structure
+ movw %bx, %cx
+ movw $0x4f01, %ax
+ int $0x10
+ cmpw $0x004f, %ax
+ jne .Lsetc_done
+
+ movb (%di), %al # Check mode attributes
+ andb $0x9b, %al
+ cmpb $0x9b, %al
+ jne .Lsetc_done # Doh! No linear frame buffer
+
+ movb $1, bootsym(graphic_mode)
+ movw %bx, bootsym(boot_vid_mode)
+ movw %bx, bootsym(video_mode)
+
+.Lsetc_done:
+ stc
+ ret
+
# Detect VESA modes.
vesa_modes:
movw %di, %bp # BP=original mode table end
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v4 2/8] x86/boot: obtain video info from boot loader
2022-03-31 9:42 [PATCH v4 0/8] x86/boot: (mostly) video mode handling adjustments Jan Beulich
2022-03-31 9:44 ` [PATCH v4 1/8] x86/boot: make "vga=current" work with graphics modes Jan Beulich
@ 2022-03-31 9:45 ` Jan Beulich
2022-04-05 9:35 ` Roger Pau Monné
2022-03-31 9:45 ` [PATCH v4 3/8] x86/EFI: retrieve EDID Jan Beulich
` (5 subsequent siblings)
7 siblings, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2022-03-31 9:45 UTC (permalink / raw)
To: xen-devel
Cc: Roger Pau Monné,
Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu
With MB2 the boot loader may provide this information, allowing us to
obtain it without needing to enter real mode (assuming we don't need to
set a new mode from "vga=", but can instead inherit the one the
bootloader may have established).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v4: Re-base.
v3: Re-base.
v2: New.
--- a/xen/arch/x86/boot/defs.h
+++ b/xen/arch/x86/boot/defs.h
@@ -53,6 +53,7 @@ typedef unsigned int u32;
typedef unsigned long long u64;
typedef unsigned int size_t;
typedef u8 uint8_t;
+typedef u16 uint16_t;
typedef u32 uint32_t;
typedef u64 uint64_t;
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -562,12 +562,18 @@ trampoline_setup:
mov %esi, sym_esi(xen_phys_start)
mov %esi, sym_esi(trampoline_xen_phys_start)
- mov sym_esi(trampoline_phys), %ecx
-
/* Get bottom-most low-memory stack address. */
+ mov sym_esi(trampoline_phys), %ecx
add $TRAMPOLINE_SPACE,%ecx
+#ifdef CONFIG_VIDEO
+ lea sym_esi(boot_vid_info), %edx
+#else
+ xor %edx, %edx
+#endif
+
/* Save Multiboot / PVH info struct (after relocation) for later use. */
+ push %edx /* Boot video info to be filled from MB2. */
push %ecx /* Bottom-most low-memory stack address. */
push %ebx /* Multiboot / PVH information address. */
push %eax /* Magic number. */
--- a/xen/arch/x86/boot/reloc.c
+++ b/xen/arch/x86/boot/reloc.c
@@ -14,9 +14,10 @@
/*
* This entry point is entered from xen/arch/x86/boot/head.S with:
- * - 0x4(%esp) = MAGIC,
- * - 0x8(%esp) = INFORMATION_ADDRESS,
- * - 0xc(%esp) = TOPMOST_LOW_MEMORY_STACK_ADDRESS.
+ * - 0x04(%esp) = MAGIC,
+ * - 0x08(%esp) = INFORMATION_ADDRESS,
+ * - 0x0c(%esp) = TOPMOST_LOW_MEMORY_STACK_ADDRESS.
+ * - 0x10(%esp) = BOOT_VIDEO_INFO_ADDRESS.
*/
asm (
" .text \n"
@@ -32,6 +33,39 @@ asm (
#include "../../../include/xen/kconfig.h"
#include <public/arch-x86/hvm/start_info.h>
+#ifdef CONFIG_VIDEO
+# include "video.h"
+
+/* VESA control information */
+struct __packed vesa_ctrl_info {
+ uint8_t signature[4];
+ uint16_t version;
+ uint32_t oem_name;
+ uint32_t capabilities;
+ uint32_t mode_list;
+ uint16_t mem_size;
+ /* We don't use any further fields. */
+};
+
+/* VESA 2.0 mode information */
+struct vesa_mode_info {
+ uint16_t attrib;
+ uint8_t window[14]; /* We don't use the individual fields. */
+ uint16_t bytes_per_line;
+ uint16_t width;
+ uint16_t height;
+ uint8_t cell_width;
+ uint8_t cell_height;
+ uint8_t nr_planes;
+ uint8_t depth;
+ uint8_t memory[5]; /* We don't use the individual fields. */
+ struct boot_video_colors colors;
+ uint8_t direct_color;
+ uint32_t base;
+ /* We don't use any further fields. */
+};
+#endif /* CONFIG_VIDEO */
+
#define get_mb2_data(tag, type, member) (((multiboot2_tag_##type##_t *)(tag))->member)
#define get_mb2_string(tag, type, member) ((u32)get_mb2_data(tag, type, member))
@@ -146,7 +180,7 @@ static multiboot_info_t *mbi_reloc(u32 m
return mbi_out;
}
-static multiboot_info_t *mbi2_reloc(u32 mbi_in)
+static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, uint32_t video_out)
{
const multiboot2_fixed_t *mbi_fix = _p(mbi_in);
const multiboot2_memory_map_t *mmap_src;
@@ -154,6 +188,9 @@ static multiboot_info_t *mbi2_reloc(u32
module_t *mbi_out_mods = NULL;
memory_map_t *mmap_dst;
multiboot_info_t *mbi_out;
+#ifdef CONFIG_VIDEO
+ struct boot_video_info *video = NULL;
+#endif
u32 ptr;
unsigned int i, mod_idx = 0;
@@ -254,17 +291,64 @@ static multiboot_info_t *mbi2_reloc(u32
++mod_idx;
break;
+#ifdef CONFIG_VIDEO
+ case MULTIBOOT2_TAG_TYPE_VBE:
+ if ( video_out )
+ {
+ const struct vesa_ctrl_info *ci;
+ const struct vesa_mode_info *mi;
+
+ video = _p(video_out);
+ ci = (void *)get_mb2_data(tag, vbe, vbe_control_info);
+ mi = (void *)get_mb2_data(tag, vbe, vbe_mode_info);
+
+ if ( ci->version >= 0x0200 && (mi->attrib & 0x9b) == 0x9b )
+ {
+ video->capabilities = ci->capabilities;
+ video->lfb_linelength = mi->bytes_per_line;
+ video->lfb_width = mi->width;
+ video->lfb_height = mi->height;
+ video->lfb_depth = mi->depth;
+ video->lfb_base = mi->base;
+ video->lfb_size = ci->mem_size;
+ video->colors = mi->colors;
+ video->vesa_attrib = mi->attrib;
+ }
+
+ video->vesapm.seg = get_mb2_data(tag, vbe, vbe_interface_seg);
+ video->vesapm.off = get_mb2_data(tag, vbe, vbe_interface_off);
+ }
+ break;
+
+ case MULTIBOOT2_TAG_TYPE_FRAMEBUFFER:
+ if ( (get_mb2_data(tag, framebuffer, framebuffer_type) !=
+ MULTIBOOT2_FRAMEBUFFER_TYPE_RGB) )
+ {
+ video_out = 0;
+ video = NULL;
+ }
+ break;
+#endif /* CONFIG_VIDEO */
+
case MULTIBOOT2_TAG_TYPE_END:
- return mbi_out;
+ goto end; /* Cannot "break;" here. */
default:
break;
}
+ end:
+
+#ifdef CONFIG_VIDEO
+ if ( video )
+ video->orig_video_isVGA = 0x23;
+#endif
+
return mbi_out;
}
-void * __stdcall reloc(u32 magic, u32 in, u32 trampoline)
+void *__stdcall reloc(uint32_t magic, uint32_t in, uint32_t trampoline,
+ uint32_t video_info)
{
alloc = trampoline;
@@ -274,7 +358,7 @@ void * __stdcall reloc(u32 magic, u32 in
return mbi_reloc(in);
case MULTIBOOT2_BOOTLOADER_MAGIC:
- return mbi2_reloc(in);
+ return mbi2_reloc(in, video_info);
case XEN_HVM_START_MAGIC_VALUE:
if ( IS_ENABLED(CONFIG_PVH_GUEST) )
--- a/xen/arch/x86/boot/video.h
+++ b/xen/arch/x86/boot/video.h
@@ -28,4 +28,45 @@
/* The "recalculate timings" flag */
#define VIDEO_RECALC 0x8000
+#ifndef __ASSEMBLY__
+struct boot_video_info {
+ uint8_t orig_x; /* 0x00 */
+ uint8_t orig_y; /* 0x01 */
+ uint8_t orig_video_mode; /* 0x02 */
+ uint8_t orig_video_cols; /* 0x03 */
+ uint8_t orig_video_lines; /* 0x04 */
+ uint8_t orig_video_isVGA; /* 0x05 */
+ uint16_t orig_video_points; /* 0x06 */
+
+ /* VESA graphic mode -- linear frame buffer */
+ uint32_t capabilities; /* 0x08 */
+ uint16_t lfb_linelength; /* 0x0c */
+ uint16_t lfb_width; /* 0x0e */
+ uint16_t lfb_height; /* 0x10 */
+ uint16_t lfb_depth; /* 0x12 */
+ uint32_t lfb_base; /* 0x14 */
+ uint32_t lfb_size; /* 0x18 */
+ union {
+ struct {
+ uint8_t red_size; /* 0x1c */
+ uint8_t red_pos; /* 0x1d */
+ uint8_t green_size; /* 0x1e */
+ uint8_t green_pos; /* 0x1f */
+ uint8_t blue_size; /* 0x20 */
+ uint8_t blue_pos; /* 0x21 */
+ uint8_t rsvd_size; /* 0x22 */
+ uint8_t rsvd_pos; /* 0x23 */
+ };
+ struct boot_video_colors {
+ uint8_t rgbr[8];
+ } colors;
+ };
+ struct {
+ uint16_t seg; /* 0x24 */
+ uint16_t off; /* 0x26 */
+ } vesapm;
+ uint16_t vesa_attrib; /* 0x28 */
+};
+#endif /* __ASSEMBLY__ */
+
#endif /* __BOOT_VIDEO_H__ */
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -532,35 +532,7 @@ static void __init setup_max_pdx(unsigne
static struct e820map __initdata boot_e820;
#ifdef CONFIG_VIDEO
-struct boot_video_info {
- u8 orig_x; /* 0x00 */
- u8 orig_y; /* 0x01 */
- u8 orig_video_mode; /* 0x02 */
- u8 orig_video_cols; /* 0x03 */
- u8 orig_video_lines; /* 0x04 */
- u8 orig_video_isVGA; /* 0x05 */
- u16 orig_video_points; /* 0x06 */
-
- /* VESA graphic mode -- linear frame buffer */
- u32 capabilities; /* 0x08 */
- u16 lfb_linelength; /* 0x0c */
- u16 lfb_width; /* 0x0e */
- u16 lfb_height; /* 0x10 */
- u16 lfb_depth; /* 0x12 */
- u32 lfb_base; /* 0x14 */
- u32 lfb_size; /* 0x18 */
- u8 red_size; /* 0x1c */
- u8 red_pos; /* 0x1d */
- u8 green_size; /* 0x1e */
- u8 green_pos; /* 0x1f */
- u8 blue_size; /* 0x20 */
- u8 blue_pos; /* 0x21 */
- u8 rsvd_size; /* 0x22 */
- u8 rsvd_pos; /* 0x23 */
- u16 vesapm_seg; /* 0x24 */
- u16 vesapm_off; /* 0x26 */
- u16 vesa_attrib; /* 0x28 */
-};
+# include "boot/video.h"
extern struct boot_video_info boot_vid_info;
#endif
--- a/xen/include/xen/multiboot2.h
+++ b/xen/include/xen/multiboot2.h
@@ -158,6 +158,59 @@ typedef struct {
multiboot2_memory_map_t entries[];
} multiboot2_tag_mmap_t;
+typedef struct
+{
+ uint32_t type;
+ uint32_t size;
+ uint16_t vbe_mode;
+ uint16_t vbe_interface_seg;
+ uint16_t vbe_interface_off;
+ uint16_t vbe_interface_len;
+ uint8_t vbe_control_info[512];
+ uint8_t vbe_mode_info[256];
+} multiboot2_tag_vbe_t;
+
+typedef struct
+{
+ uint8_t red;
+ uint8_t green;
+ uint8_t blue;
+} multiboot2_color_t;
+
+typedef struct
+{
+ uint32_t type;
+ uint32_t size;
+ uint64_t framebuffer_addr;
+ uint32_t framebuffer_pitch;
+ uint32_t framebuffer_width;
+ uint32_t framebuffer_height;
+ uint8_t framebuffer_bpp;
+#define MULTIBOOT2_FRAMEBUFFER_TYPE_INDEXED 0
+#define MULTIBOOT2_FRAMEBUFFER_TYPE_RGB 1
+#define MULTIBOOT2_FRAMEBUFFER_TYPE_EGA_TEXT 2
+ uint8_t framebuffer_type;
+ uint16_t reserved;
+
+ union
+ {
+ struct
+ {
+ uint16_t framebuffer_palette_num_colors;
+ multiboot2_color_t framebuffer_palette[];
+ };
+ struct
+ {
+ uint8_t framebuffer_red_field_position;
+ uint8_t framebuffer_red_mask_size;
+ uint8_t framebuffer_green_field_position;
+ uint8_t framebuffer_green_mask_size;
+ uint8_t framebuffer_blue_field_position;
+ uint8_t framebuffer_blue_mask_size;
+ };
+ };
+} multiboot2_tag_framebuffer_t;
+
typedef struct {
u32 type;
u32 size;
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v4 3/8] x86/EFI: retrieve EDID
2022-03-31 9:42 [PATCH v4 0/8] x86/boot: (mostly) video mode handling adjustments Jan Beulich
2022-03-31 9:44 ` [PATCH v4 1/8] x86/boot: make "vga=current" work with graphics modes Jan Beulich
2022-03-31 9:45 ` [PATCH v4 2/8] x86/boot: obtain video info from boot loader Jan Beulich
@ 2022-03-31 9:45 ` Jan Beulich
2022-04-01 10:15 ` Luca Fancellu
` (3 more replies)
2022-03-31 9:48 ` [PATCH v4 4/8] x86/boot: simplify mode_table Jan Beulich
` (4 subsequent siblings)
7 siblings, 4 replies; 38+ messages in thread
From: Jan Beulich @ 2022-03-31 9:45 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, Wei Liu, Roger Pau Monné,
Julien Grall, Stefano Stabellini, Volodymyr Babchuk,
Bertrand Marquis
When booting directly from EFI, obtaining this information from EFI is
the only possible way. And even when booting with a boot loader
interposed, it's more clean not to use legacy BIOS calls for this
purpose. (The downside being that there are no "capabilities" that we
can retrieve the EFI way.)
To achieve this we need to propagate the handle used to obtain the
EFI_GRAPHICS_OUTPUT_PROTOCOL instance for further obtaining an
EFI_EDID_*_PROTOCOL instance, which has been part of the spec since 2.5.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Setting boot_edid_caps to zero isn't desirable, but arbitrarily setting
one or both of the two low bits also doesn't seem appropriate.
GrUB also checks an "agp-internal-edid" variable. As I haven't been able
to find any related documentation, and as GrUB being happy about the
variable being any size (rather than at least / precisely 128 bytes),
I didn't follow that route.
---
v3: Re-base.
v2: New.
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -464,6 +464,10 @@ static void __init efi_arch_edd(void)
{
}
+static void __init efi_arch_edid(EFI_HANDLE gop_handle)
+{
+}
+
static void __init efi_arch_memory_setup(void)
{
}
--- a/xen/arch/x86/boot/video.S
+++ b/xen/arch/x86/boot/video.S
@@ -922,7 +922,14 @@ store_edid:
pushw %dx
pushw %di
- cmpb $1, bootsym(opt_edid) # EDID disabled on cmdline (edid=no)?
+ movb bootsym(opt_edid), %al
+ cmpw $0x1313, bootsym(boot_edid_caps) # Data already retrieved?
+ je .Lcheck_edid
+ cmpb $2, %al # EDID forced on cmdline (edid=force)?
+ jne .Lno_edid
+
+.Lcheck_edid:
+ cmpb $1, %al # EDID disabled on cmdline (edid=no)?
je .Lno_edid
leaw vesa_glob_info, %di
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -568,6 +568,49 @@ static void __init efi_arch_video_init(E
#endif
}
+#ifdef CONFIG_VIDEO
+static bool __init copy_edid(const void *buf, unsigned int size)
+{
+ /*
+ * Be conservative - for both undersized and oversized blobs it is unclear
+ * what to actually do with them. The more that unlike the VESA BIOS
+ * interface we also have no associated "capabilities" value (which might
+ * carry a hint as to possible interpretation).
+ */
+ if ( size != ARRAY_SIZE(boot_edid_info) )
+ return false;
+
+ memcpy(boot_edid_info, buf, size);
+ boot_edid_caps = 0;
+
+ return true;
+}
+#endif
+
+static void __init efi_arch_edid(EFI_HANDLE gop_handle)
+{
+#ifdef CONFIG_VIDEO
+ static EFI_GUID __initdata active_guid = EFI_EDID_ACTIVE_PROTOCOL_GUID;
+ static EFI_GUID __initdata discovered_guid = EFI_EDID_DISCOVERED_PROTOCOL_GUID;
+ EFI_EDID_ACTIVE_PROTOCOL *active_edid;
+ EFI_EDID_DISCOVERED_PROTOCOL *discovered_edid;
+ EFI_STATUS status;
+
+ status = efi_bs->OpenProtocol(gop_handle, &active_guid,
+ (void **)&active_edid, efi_ih, NULL,
+ EFI_OPEN_PROTOCOL_GET_PROTOCOL);
+ if ( status == EFI_SUCCESS &&
+ copy_edid(active_edid->Edid, active_edid->SizeOfEdid) )
+ return;
+
+ status = efi_bs->OpenProtocol(gop_handle, &discovered_guid,
+ (void **)&discovered_edid, efi_ih, NULL,
+ EFI_OPEN_PROTOCOL_GET_PROTOCOL);
+ if ( status == EFI_SUCCESS )
+ copy_edid(discovered_edid->Edid, discovered_edid->SizeOfEdid);
+#endif
+}
+
static void __init efi_arch_memory_setup(void)
{
unsigned int i;
@@ -729,6 +772,7 @@ static void __init efi_arch_flush_dcache
void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
{
EFI_GRAPHICS_OUTPUT_PROTOCOL *gop;
+ EFI_HANDLE gop_handle;
UINTN cols, gop_mode = ~0, rows;
__set_bit(EFI_BOOT, &efi_flags);
@@ -742,11 +786,15 @@ void __init efi_multiboot2(EFI_HANDLE Im
&cols, &rows) == EFI_SUCCESS )
efi_arch_console_init(cols, rows);
- gop = efi_get_gop();
+ gop = efi_get_gop(&gop_handle);
if ( gop )
+ {
gop_mode = efi_find_gop_mode(gop, 0, 0, 0);
+ efi_arch_edid(gop_handle);
+ }
+
efi_arch_edd();
efi_arch_cpu();
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -118,7 +118,7 @@ static bool read_section(const EFI_LOADE
static void efi_init(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable);
static void efi_console_set_mode(void);
-static EFI_GRAPHICS_OUTPUT_PROTOCOL *efi_get_gop(void);
+static EFI_GRAPHICS_OUTPUT_PROTOCOL *efi_get_gop(EFI_HANDLE *gop_handle);
static UINTN efi_find_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
UINTN cols, UINTN rows, UINTN depth);
static void efi_tables(void);
@@ -758,7 +758,7 @@ static void __init efi_console_set_mode(
StdOut->SetMode(StdOut, best);
}
-static EFI_GRAPHICS_OUTPUT_PROTOCOL __init *efi_get_gop(void)
+static EFI_GRAPHICS_OUTPUT_PROTOCOL __init *efi_get_gop(EFI_HANDLE *gop_handle)
{
EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info;
EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
@@ -783,7 +783,10 @@ static EFI_GRAPHICS_OUTPUT_PROTOCOL __in
continue;
status = gop->QueryMode(gop, gop->Mode->Mode, &info_size, &mode_info);
if ( !EFI_ERROR(status) )
+ {
+ *gop_handle = handles[i];
break;
+ }
}
if ( handles )
efi_bs->FreePool(handles);
@@ -1222,6 +1225,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
if ( use_cfg_file )
{
EFI_FILE_HANDLE dir_handle;
+ EFI_HANDLE gop_handle;
UINTN depth, cols, rows, size;
size = cols = rows = depth = 0;
@@ -1230,7 +1234,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
&cols, &rows) == EFI_SUCCESS )
efi_arch_console_init(cols, rows);
- gop = efi_get_gop();
+ gop = efi_get_gop(&gop_handle);
/* Get the file system interface. */
dir_handle = get_parent_handle(loaded_image, &file_name);
@@ -1360,7 +1364,11 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
dir_handle->Close(dir_handle);
if ( gop && !base_video )
+ {
gop_mode = efi_find_gop_mode(gop, cols, rows, depth);
+
+ efi_arch_edid(gop_handle);
+ }
}
/* Get the number of boot modules specified on the DT or an error (<0) */
@@ -1387,7 +1395,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
efi_arch_edd();
- /* XXX Collect EDID info. */
efi_arch_cpu();
efi_tables();
--- a/xen/include/efi/efiprot.h
+++ b/xen/include/efi/efiprot.h
@@ -724,5 +724,52 @@ struct _EFI_GRAPHICS_OUTPUT_PROTOCOL {
EFI_GRAPHICS_OUTPUT_PROTOCOL_BLT Blt;
EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE *Mode;
};
+
+/*
+ * EFI EDID Discovered Protocol
+ * UEFI Specification Version 2.5 Section 11.9
+ */
+#define EFI_EDID_DISCOVERED_PROTOCOL_GUID \
+ { 0x1C0C34F6, 0xD380, 0x41FA, { 0xA0, 0x49, 0x8a, 0xD0, 0x6C, 0x1A, 0x66, 0xAA} }
+
+typedef struct _EFI_EDID_DISCOVERED_PROTOCOL {
+ UINT32 SizeOfEdid;
+ UINT8 *Edid;
+} EFI_EDID_DISCOVERED_PROTOCOL;
+
+/*
+ * EFI EDID Active Protocol
+ * UEFI Specification Version 2.5 Section 11.9
+ */
+#define EFI_EDID_ACTIVE_PROTOCOL_GUID \
+ { 0xBD8C1056, 0x9F36, 0x44EC, { 0x92, 0xA8, 0xA6, 0x33, 0x7F, 0x81, 0x79, 0x86} }
+
+typedef struct _EFI_EDID_ACTIVE_PROTOCOL {
+ UINT32 SizeOfEdid;
+ UINT8 *Edid;
+} EFI_EDID_ACTIVE_PROTOCOL;
+
+/*
+ * EFI EDID Override Protocol
+ * UEFI Specification Version 2.5 Section 11.9
+ */
+#define EFI_EDID_OVERRIDE_PROTOCOL_GUID \
+ { 0x48ECB431, 0xFB72, 0x45C0, { 0xA9, 0x22, 0xF4, 0x58, 0xFE, 0x04, 0x0B, 0xD5} }
+
+INTERFACE_DECL(_EFI_EDID_OVERRIDE_PROTOCOL);
+
+typedef
+EFI_STATUS
+(EFIAPI *EFI_EDID_OVERRIDE_PROTOCOL_GET_EDID) (
+ IN struct _EFI_EDID_OVERRIDE_PROTOCOL *This,
+ IN EFI_HANDLE *ChildHandle,
+ OUT UINT32 *Attributes,
+ IN OUT UINTN *EdidSize,
+ IN OUT UINT8 **Edid);
+
+typedef struct _EFI_EDID_OVERRIDE_PROTOCOL {
+ EFI_EDID_OVERRIDE_PROTOCOL_GET_EDID GetEdid;
+} EFI_EDID_OVERRIDE_PROTOCOL;
+
#endif
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v4 4/8] x86/boot: simplify mode_table
2022-03-31 9:42 [PATCH v4 0/8] x86/boot: (mostly) video mode handling adjustments Jan Beulich
` (2 preceding siblings ...)
2022-03-31 9:45 ` [PATCH v4 3/8] x86/EFI: retrieve EDID Jan Beulich
@ 2022-03-31 9:48 ` Jan Beulich
2022-04-05 10:44 ` Roger Pau Monné
2022-03-31 9:49 ` [PATCH v4 5/8] x86/boot: fold branches in video handling code Jan Beulich
` (3 subsequent siblings)
7 siblings, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2022-03-31 9:48 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné
There's no point in writing 80x25 text mode information via multiple
insns all storing immediate values. The data can simply be included
first thing in the vga_modes table, allowing the already present
REP MOVSB to take care of everything in one go.
While touching this also correct a related but stale comment.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/boot/video.S
+++ b/xen/arch/x86/boot/video.S
@@ -655,8 +655,9 @@ outidx: outb %al, %dx
# Build the table of video modes (stored after the setup.S code at the
# `modelist' label. Each video mode record looks like:
# .word MODE-ID (our special mode ID (see above))
-# .byte rows (number of rows)
-# .byte columns (number of columns)
+# .word rows (number of rows)
+# .word columns (number of columns)
+# .word 0 (color depth; gfx modes only)
# Returns address of the end of the table in DI, the end is marked
# with a ASK_VGA ID.
mode_table:
@@ -665,12 +666,6 @@ mode_table:
jnz mtab1
leaw modelist, %di # Store standard modes:
- movw $VIDEO_80x25,(%di) # The 80x25 mode (ALL)
- movw $0x50,2(%di)
- movw $0x19,4(%di)
- movw $0x00,6(%di)
- addw $8,%di
-
leaw bootsym(vga_modes), %si # All modes for std VGA
movw $vga_modes_end-vga_modes, %cx
rep movsb
@@ -684,6 +679,7 @@ ret0: ret
# Modes usable on all standard VGAs
vga_modes:
+ .word VIDEO_80x25, 0x50,0x19,0 # 80x25
.word VIDEO_80x50, 0x50,0x32,0 # 80x50
.word VIDEO_80x43, 0x50,0x2b,0 # 80x43
.word VIDEO_80x28, 0x50,0x1c,0 # 80x28
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v4 5/8] x86/boot: fold branches in video handling code
2022-03-31 9:42 [PATCH v4 0/8] x86/boot: (mostly) video mode handling adjustments Jan Beulich
` (3 preceding siblings ...)
2022-03-31 9:48 ` [PATCH v4 4/8] x86/boot: simplify mode_table Jan Beulich
@ 2022-03-31 9:49 ` Jan Beulich
2022-04-05 10:55 ` Roger Pau Monné
2022-03-31 9:50 ` [PATCH v4 6/8] x86/boot: fold/replace moves " Jan Beulich
` (2 subsequent siblings)
7 siblings, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2022-03-31 9:49 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné
Using Jcc to branch around a JMP is necessary only in pre-386 code,
where Jcc is limited to disp8. Use the opposite Jcc directly in two
places. Since it's adjacent, also convert an ORB to TESTB.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/boot/video.S
+++ b/xen/arch/x86/boot/video.S
@@ -332,8 +332,7 @@ lment: movb $0, (%di)
cmpw $0x656d, (%si) # 'me'
jnz lmhx
cmpw $0x756e, 2(%si) # 'nu'
- jnz lmhx
- jmp listm
+ jz listm
lmhx: xorw %bx, %bx # Else => mode ID in hex
lmhex: lodsb
@@ -401,10 +400,8 @@ mode_set:
cmpb $VIDEO_FIRST_VESA>>8, %ah
jnc check_vesa
- orb %ah, %ah
- jnz setbad
-
- jmp setmenu
+ testb %ah, %ah
+ jz setmenu
setbad: clc
ret
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v4 6/8] x86/boot: fold/replace moves in video handling code
2022-03-31 9:42 [PATCH v4 0/8] x86/boot: (mostly) video mode handling adjustments Jan Beulich
` (4 preceding siblings ...)
2022-03-31 9:49 ` [PATCH v4 5/8] x86/boot: fold branches in video handling code Jan Beulich
@ 2022-03-31 9:50 ` Jan Beulich
2022-04-05 11:13 ` Roger Pau Monné
2022-03-31 9:50 ` [PATCH v4 7/8] x86/boot: LEA -> MOV " Jan Beulich
2022-03-31 9:51 ` [PATCH v4 8/8] x86/boot: fold two MOVs into an ADD Jan Beulich
7 siblings, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2022-03-31 9:50 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné
Replace (mainly) MOV forms with shorter insns (or sequences thereof).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Of course there's more room for improvement. For example there look to
be a number of LEAs which really could be MOVs.
--- a/xen/arch/x86/boot/video.S
+++ b/xen/arch/x86/boot/video.S
@@ -121,8 +121,7 @@ mopar_gr:
movw %ax, _param(PARAM_LFB_WIDTH)
movw 20(%di), %ax
movw %ax, _param(PARAM_LFB_HEIGHT)
- movb 25(%di), %al
- movb $0, %ah
+ movzbw 25(%di), %ax
movw %ax, _param(PARAM_LFB_DEPTH)
movl 40(%di), %eax
movl %eax, _param(PARAM_LFB_BASE)
@@ -135,8 +134,7 @@ mopar_gr:
# get video mem size
leaw vesa_glob_info, %di
- xorl %eax, %eax
- movw 18(%di), %ax
+ movzwl 18(%di), %eax
movl %eax, _param(PARAM_LFB_SIZE)
# store mode capabilities
@@ -144,14 +142,11 @@ mopar_gr:
movl %eax, _param(PARAM_CAPABILITIES)
# switching the DAC to 8-bit is for <= 8 bpp only
- movw _param(PARAM_LFB_DEPTH), %ax
- cmpw $8, %ax
+ cmpw $8, _param(PARAM_LFB_DEPTH)
jg dac_done
# get DAC switching capability
- xorl %eax, %eax
- movb 10(%di), %al
- testb $1, %al
+ testb $1, 10(%di)
jz dac_set
# attempt to switch DAC to 8-bit
@@ -164,17 +159,17 @@ mopar_gr:
dac_set:
# set color size to DAC size
- movb bootsym(dac_size), %al
+ movzbw bootsym(dac_size), %ax
movb %al, _param(PARAM_LFB_COLORS+0)
movb %al, _param(PARAM_LFB_COLORS+2)
movb %al, _param(PARAM_LFB_COLORS+4)
movb %al, _param(PARAM_LFB_COLORS+6)
# set color offsets to 0
- movb $0, _param(PARAM_LFB_COLORS+1)
- movb $0, _param(PARAM_LFB_COLORS+3)
- movb $0, _param(PARAM_LFB_COLORS+5)
- movb $0, _param(PARAM_LFB_COLORS+7)
+ movb %ah, _param(PARAM_LFB_COLORS+1)
+ movb %ah, _param(PARAM_LFB_COLORS+3)
+ movb %ah, _param(PARAM_LFB_COLORS+5)
+ movb %ah, _param(PARAM_LFB_COLORS+7)
dac_done:
# get protected mode interface information
@@ -504,7 +499,8 @@ setvesabysize:
call mode_table
leaw modelist,%si
1: add $8,%si
- cmpw $ASK_VGA,-8(%si) # End?
+ movw -8(%si),%bx
+ cmpw $ASK_VGA,%bx # End?
je setbad
movw -6(%si),%ax
cmpw %ax,bootsym(vesa_size)+0
@@ -515,9 +511,7 @@ setvesabysize:
movw -2(%si),%ax
cmpw %ax,bootsym(vesa_size)+4
jne 1b
- movw -8(%si),%ax
- movw %ax,%bx
- movw %ax,bootsym(boot_vid_mode)
+ movw %bx,bootsym(boot_vid_mode)
jmp check_vesa
# Table of routines for setting of the special modes.
@@ -773,8 +767,7 @@ vesa2: pushw %cx
movw %bx, 2(%di)
movw 0x14(%di), %bx # Height
movw %bx, 4(%di)
- xorw %bx, %bx
- movb 0x19(%di), %bl # Depth
+ movzbw 0x19(%di), %bx # Depth
movw %bx, 6(%di)
addw $8, %di # The mode is valid. Store it.
@@ -901,8 +894,7 @@ gettime:
movb %dh, %al # %dh contains the seconds
andb $0x0f, %al
movb %dh, %ah
- movb $0x04, %cl
- shrb %cl, %ah
+ shrb $4, %ah
aad
popw %cx
ret
@@ -959,8 +951,8 @@ store_edid:
.Lforce_edid:
movw $0x4f15, %ax # do VBE/DDC
movw $0x01, %bx
- movw $0x00, %cx
- movw $0x00, %dx
+ xorw %cx, %cx
+ xorw %dx, %dx
movw $bootsym(boot_edid_info), %di
int $0x10
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v4 7/8] x86/boot: LEA -> MOV in video handling code
2022-03-31 9:42 [PATCH v4 0/8] x86/boot: (mostly) video mode handling adjustments Jan Beulich
` (5 preceding siblings ...)
2022-03-31 9:50 ` [PATCH v4 6/8] x86/boot: fold/replace moves " Jan Beulich
@ 2022-03-31 9:50 ` Jan Beulich
2022-04-05 11:28 ` Roger Pau Monné
2022-03-31 9:51 ` [PATCH v4 8/8] x86/boot: fold two MOVs into an ADD Jan Beulich
7 siblings, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2022-03-31 9:50 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné
Replace most LEA instances with (one byte shorter) MOV.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v4: New.
--- a/xen/arch/x86/boot/video.S
+++ b/xen/arch/x86/boot/video.S
@@ -60,7 +60,7 @@ video: xorw %ax, %ax
call mode_set # Set the mode
jc vid1
- leaw bootsym(badmdt), %si # Invalid mode ID
+ movw $bootsym(badmdt), %si # Invalid mode ID
call prtstr
vid2: call mode_menu
vid1: call store_edid
@@ -113,7 +113,7 @@ mopar2: movb %al, _param(PARAM_VIDEO_
# Fetching of VESA frame buffer parameters
mopar_gr:
- leaw vesa_mode_info, %di
+ movw $vesa_mode_info, %di
movb $0x23, _param(PARAM_HAVE_VGA)
movw 16(%di), %ax
movw %ax, _param(PARAM_LFB_LINELENGTH)
@@ -133,7 +133,7 @@ mopar_gr:
movw %ax, _param(PARAM_VESA_ATTRIB)
# get video mem size
- leaw vesa_glob_info, %di
+ movw $vesa_glob_info, %di
movzwl 18(%di), %eax
movl %eax, _param(PARAM_LFB_SIZE)
@@ -189,7 +189,7 @@ no_pm: pushw %ds
# The video mode menu
mode_menu:
- leaw bootsym(keymsg), %si # "Return/Space/Timeout" message
+ movw $bootsym(keymsg), %si # "Return/Space/Timeout" message
call prtstr
call flush
nokey: call getkt
@@ -206,22 +206,22 @@ nokey: call getkt
defmd1: ret # No mode chosen? Default 80x25
listm: call mode_table # List mode table
-listm0: leaw bootsym(name_bann), %si # Print adapter name
+listm0: movw $bootsym(name_bann), %si # Print adapter name
call prtstr
movw bootsym(card_name), %si
orw %si, %si
jnz an2
- leaw bootsym(vga_name), %si
+ movw $bootsym(vga_name), %si
jmp an1
an2: call prtstr
- leaw bootsym(svga_name), %si
+ movw $bootsym(svga_name), %si
an1: call prtstr
- leaw bootsym(listhdr), %si # Table header
+ movw $bootsym(listhdr), %si # Table header
call prtstr
movb $0x30, %dl # DL holds mode number
- leaw modelist, %si
+ movw $modelist, %si
lm1: cmpw $ASK_VGA, (%si) # End?
jz lm2
@@ -240,7 +240,7 @@ lm1: cmpw $ASK_VGA, (%si)
testb $0xff,(%si)
jnz 1f
push %si
- leaw bootsym(textmode), %si
+ movw $bootsym(textmode), %si
call prtstr
pop %si
lodsw
@@ -257,7 +257,7 @@ lm1: cmpw $ASK_VGA, (%si)
cmpb $'z'+1, %dl
jnz skip_bail
- leaw bootsym(menu_bail_msg), %si
+ movw $bootsym(menu_bail_msg), %si
call prtstr
jmp lm2
@@ -266,13 +266,13 @@ skip_bail:
jnz skip_pause
push %si
push %dx
- leaw bootsym(menu_more_msg), %si # '<press space>'
+ movw $bootsym(menu_more_msg), %si # '<press space>'
call prtstr
call flush
1: call getkey
cmpb $0x20, %al # SPACE ?
jne 1b # yes - manual mode selection
- leaw bootsym(crlft), %si
+ movw $bootsym(crlft), %si
call prtstr
pop %dx
pop %si
@@ -283,9 +283,9 @@ skip_pause:
movb $'a', %dl
jmp lm1
-lm2: leaw bootsym(prompt), %si # Mode prompt
+lm2: movw $bootsym(prompt), %si # Mode prompt
call prtstr
- leaw bootsym(edit_buf), %di # Editor buffer
+ movw $bootsym(edit_buf), %di # Editor buffer
lm3: call getkey
cmpb $0x0d, %al # Enter?
jz lment
@@ -315,9 +315,9 @@ lmbs: cmpw $bootsym(edit_buf), %di
jmp lm3
lment: movb $0, (%di)
- leaw bootsym(crlft), %si
+ movw $bootsym(crlft), %si
call prtstr
- leaw bootsym(edit_buf), %si
+ movw $bootsym(edit_buf), %si
cmpb $0, (%si) # Empty string = default mode
jz lmdef
@@ -373,7 +373,7 @@ mnusel: lodsb
lmuse: call mode_set
jc lmdef
-lmbad: leaw bootsym(unknt), %si
+lmbad: movw $bootsym(unknt), %si
call prtstr
jmp mode_menu
lmdef: ret
@@ -424,13 +424,13 @@ setmenu:
jmp mode_set
check_vesa:
- leaw vesa_glob_info, %di
+ movw $vesa_glob_info, %di
movw $0x4f00, %ax
int $0x10
cmpw $0x004f, %ax
jnz setbad
- leaw vesa_mode_info, %di # Get mode information structure
+ movw $vesa_mode_info, %di # Get mode information structure
leaw -VIDEO_FIRST_VESA(%bx), %cx
movw $0x4f01, %ax
int $0x10
@@ -497,7 +497,7 @@ inidx: outb %al, %dx
setvesabysize:
call mode_table
- leaw modelist,%si
+ movw $modelist,%si
1: add $8,%si
movw -8(%si),%bx
cmpw $ASK_VGA,%bx # End?
@@ -656,8 +656,8 @@ mode_table:
orw %di, %di
jnz mtab1
- leaw modelist, %di # Store standard modes:
- leaw bootsym(vga_modes), %si # All modes for std VGA
+ movw $modelist, %di # Store standard modes:
+ movw $bootsym(vga_modes), %si # All modes for std VGA
movw $vga_modes_end-vga_modes, %cx
rep movsb
@@ -665,7 +665,7 @@ mode_table:
movw $ASK_VGA, (%di) # End marker
movw %di, bootsym(mt_end)
-mtab1: leaw modelist, %si # SI=mode list, DI=list end
+mtab1: movw $modelist, %si # SI=mode list, DI=list end
ret0: ret
# Modes usable on all standard VGAs
@@ -681,7 +681,7 @@ vga_modes_end:
# If the current mode is a VESA graphics one, obtain its parameters.
set_current:
- leaw vesa_glob_info, %di
+ movw $vesa_glob_info, %di
movw $0x4f00, %ax
int $0x10
cmpw $0x004f, %ax
@@ -692,7 +692,7 @@ set_current:
cmpw $0x004f, %ax
jne .Lsetc_done
- leaw vesa_mode_info, %di # Get mode information structure
+ movw $vesa_mode_info, %di # Get mode information structure
movw %bx, %cx
movw $0x4f01, %ax
int $0x10
@@ -715,7 +715,7 @@ set_current:
# Detect VESA modes.
vesa_modes:
movw %di, %bp # BP=original mode table end
- leaw vesa_glob_info, %di
+ movw $vesa_glob_info, %di
movw $0x4f00, %ax # VESA Get card info call
int $0x10
movw %di, %si
@@ -772,7 +772,7 @@ vesa2: pushw %cx
addw $8, %di # The mode is valid. Store it.
vesan: loop vesa1 # Next mode. Limit exceeded => error
-vesae: leaw bootsym(vesaer), %si
+vesae: movw $bootsym(vesaer), %si
call prtstr
movw %bp, %di # Discard already found modes.
vesar: popw %gs
@@ -917,7 +917,7 @@ store_edid:
cmpb $1, %al # EDID disabled on cmdline (edid=no)?
je .Lno_edid
- leaw vesa_glob_info, %di
+ movw $vesa_glob_info, %di
movw $0x4f00, %ax
int $0x10
cmpw $0x004f, %ax
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v4 8/8] x86/boot: fold two MOVs into an ADD
2022-03-31 9:42 [PATCH v4 0/8] x86/boot: (mostly) video mode handling adjustments Jan Beulich
` (6 preceding siblings ...)
2022-03-31 9:50 ` [PATCH v4 7/8] x86/boot: LEA -> MOV " Jan Beulich
@ 2022-03-31 9:51 ` Jan Beulich
2022-04-05 11:29 ` Roger Pau Monné
7 siblings, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2022-03-31 9:51 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné
There's no point going through %ax; the addition can be done directly in
%di.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.
--- a/xen/arch/x86/boot/mem.S
+++ b/xen/arch/x86/boot/mem.S
@@ -24,9 +24,7 @@ get_memory_map:
cmpw $E820_BIOS_MAX, bootsym(bios_e820nr) # up to this many entries
jae .Ldone
- movw %di,%ax
- addw $20,%ax
- movw %ax,%di
+ addw $20,%di
testl %ebx,%ebx # check to see if
jnz 1b # %ebx is set to EOF
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 3/8] x86/EFI: retrieve EDID
2022-03-31 9:45 ` [PATCH v4 3/8] x86/EFI: retrieve EDID Jan Beulich
@ 2022-04-01 10:15 ` Luca Fancellu
2022-04-05 10:27 ` Roger Pau Monné
` (2 subsequent siblings)
3 siblings, 0 replies; 38+ messages in thread
From: Luca Fancellu @ 2022-04-01 10:15 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel, Andrew Cooper, Wei Liu, Roger Pau Monné,
Julien Grall, Stefano Stabellini, Volodymyr Babchuk,
Bertrand Marquis
> On 31 Mar 2022, at 10:45, Jan Beulich <jbeulich@suse.com> wrote:
>
> When booting directly from EFI, obtaining this information from EFI is
> the only possible way. And even when booting with a boot loader
> interposed, it's more clean not to use legacy BIOS calls for this
> purpose. (The downside being that there are no "capabilities" that we
> can retrieve the EFI way.)
>
> To achieve this we need to propagate the handle used to obtain the
> EFI_GRAPHICS_OUTPUT_PROTOCOL instance for further obtaining an
> EFI_EDID_*_PROTOCOL instance, which has been part of the spec since 2.5.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Hi Jan,
For the arm and common part, the changes looks good to me.
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
I tested also the whole serie in the sense that it boots properly on arm,
unfortunately I could not test the functionality.
Cheers,
Luca
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 1/8] x86/boot: make "vga=current" work with graphics modes
2022-03-31 9:44 ` [PATCH v4 1/8] x86/boot: make "vga=current" work with graphics modes Jan Beulich
@ 2022-04-04 14:49 ` Roger Pau Monné
2022-04-04 15:50 ` Jan Beulich
2022-04-05 8:45 ` Roger Pau Monné
1 sibling, 1 reply; 38+ messages in thread
From: Roger Pau Monné @ 2022-04-04 14:49 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel, Andrew Cooper, Wei Liu, community.manager, Henry Wang
On Thu, Mar 31, 2022 at 11:44:10AM +0200, Jan Beulich wrote:
> GrUB2 can be told to leave the screen in the graphics mode it has been
> using (or any other one), via "set gfxpayload=keep" (or suitable
> variants thereof). In this case we can avoid doing another mode switch
> ourselves. This in particular avoids possibly setting the screen to a
> less desirable mode: On one of my test systems the set of modes
> reported available by the VESA BIOS depends on whether the interposed
> KVM switch has that machine set as the active one. If it's not active,
> only modes up to 1024x768 get reported, while when active 1280x1024
> modes are also included. For things to always work with an explicitly
> specified mode (via the "vga=" option), that mode therefore needs be a
> 1024x768 one.
>
> For some reason this only works for me with "multiboot2" (and
> "module2"); "multiboot" (and "module") still forces the screen into text
> mode, despite my reading of the sources suggesting otherwise.
>
> For starters I'm limiting this to graphics modes; I do think this ought
> to also work for text modes, but
> - I can't tell whether GrUB2 can set any text mode other than 80x25
> (I've only found plain "text" to be valid as a "gfxpayload" setting),
> - I'm uncertain whether supporting that is worth it, since I'm uncertain
> how many people would be running their systems/screens in text mode,
> - I'd like to limit the amount of code added to the realmode trampoline.
>
> For starters I'm also limiting mode information retrieval to raw BIOS
> accesses. This will allow things to work (in principle) also with other
> boot environments where a graphics mode can be left in place. The
> downside is that this then still is dependent upon switching back to
> real mode, so retrieving the needed information from multiboot info is
> likely going to be desirable down the road.
I'm unsure, what's the benefit from retrieving this information from
the VESA blob rather than from multiboot(2) structures?
Is it because we require a VESA mode to be set before we parse the
multiboot information?
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> I'm not convinced boot_vid_mode really needs setting here; I'm doing so
> mainly because setvesabysize also does.
> ---
> v4: Add changelog entry.
> v2: Use 0x9b instead of 0x99 for attributes check: I think the value
> used by check_vesa also wants to be converted, to match vesa2's.
> (Perhaps the value wants to become a #define, albeit before doing so
> I'd question the requirement of the mode to be a color one.)
>
> --- a/CHANGELOG.md
> +++ b/CHANGELOG.md
> @@ -6,6 +6,10 @@ The format is based on [Keep a Changelog
>
> ## [unstable UNRELEASED](https://xenbits.xen.org/gitweb/?p=xen.git;a=shortlog;h=staging) - TBD
>
> +### Changed
> + - On x86 "vga=current" can now be used together with GrUB2's gfxpayload setting. Note that
> + this requires use of "multiboot2" (and "module2") as the GrUB commands loading Xen.
> +
> ### Removed / support downgraded
> - dropped support for the (x86-only) "vesa-mtrr" and "vesa-remap" command line options
>
> --- a/xen/arch/x86/boot/video.S
> +++ b/xen/arch/x86/boot/video.S
> @@ -575,7 +575,6 @@ set14: movw $0x1111, %ax
> movb $0x01, %ah # Define cursor scan lines 11-12
> movw $0x0b0c, %cx
> int $0x10
> -set_current:
> stc
> ret
>
> @@ -693,6 +692,39 @@ vga_modes:
> .word VIDEO_80x60, 0x50,0x3c,0 # 80x60
> vga_modes_end:
>
> +# If the current mode is a VESA graphics one, obtain its parameters.
> +set_current:
> + leaw vesa_glob_info, %di
> + movw $0x4f00, %ax
> + int $0x10
> + cmpw $0x004f, %ax
> + jne .Lsetc_done
You don't seem to make use of the information fetched here? I guess
this is somehow required to access the other functions?
> + movw $0x4f03, %ax
It would help readability to have defines for those values, ie:
VESA_GET_CURRENT_MODE or some such (not that you need to do it here,
just a comment).
> + int $0x10
> + cmpw $0x004f, %ax
> + jne .Lsetc_done
> +
> + leaw vesa_mode_info, %di # Get mode information structure
> + movw %bx, %cx
> + movw $0x4f01, %ax
> + int $0x10
> + cmpw $0x004f, %ax
> + jne .Lsetc_done
> +
> + movb (%di), %al # Check mode attributes
> + andb $0x9b, %al
> + cmpb $0x9b, %al
So you also check that the reserved D1 bit is set to 1 as mandated by
the spec. This is slightly different than what's done in check_vesa,
would you mind adding a define for this an unifying with check_vesa?
Thanks, Roger.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 1/8] x86/boot: make "vga=current" work with graphics modes
2022-04-04 14:49 ` Roger Pau Monné
@ 2022-04-04 15:50 ` Jan Beulich
2022-04-05 8:24 ` Roger Pau Monné
0 siblings, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2022-04-04 15:50 UTC (permalink / raw)
To: Roger Pau Monné; +Cc: xen-devel, Andrew Cooper, Wei Liu
(reducing Cc list some)
On 04.04.2022 16:49, Roger Pau Monné wrote:
> On Thu, Mar 31, 2022 at 11:44:10AM +0200, Jan Beulich wrote:
>> GrUB2 can be told to leave the screen in the graphics mode it has been
>> using (or any other one), via "set gfxpayload=keep" (or suitable
>> variants thereof). In this case we can avoid doing another mode switch
>> ourselves. This in particular avoids possibly setting the screen to a
>> less desirable mode: On one of my test systems the set of modes
>> reported available by the VESA BIOS depends on whether the interposed
>> KVM switch has that machine set as the active one. If it's not active,
>> only modes up to 1024x768 get reported, while when active 1280x1024
>> modes are also included. For things to always work with an explicitly
>> specified mode (via the "vga=" option), that mode therefore needs be a
>> 1024x768 one.
>>
>> For some reason this only works for me with "multiboot2" (and
>> "module2"); "multiboot" (and "module") still forces the screen into text
>> mode, despite my reading of the sources suggesting otherwise.
>>
>> For starters I'm limiting this to graphics modes; I do think this ought
>> to also work for text modes, but
>> - I can't tell whether GrUB2 can set any text mode other than 80x25
>> (I've only found plain "text" to be valid as a "gfxpayload" setting),
>> - I'm uncertain whether supporting that is worth it, since I'm uncertain
>> how many people would be running their systems/screens in text mode,
>> - I'd like to limit the amount of code added to the realmode trampoline.
>>
>> For starters I'm also limiting mode information retrieval to raw BIOS
>> accesses. This will allow things to work (in principle) also with other
>> boot environments where a graphics mode can be left in place. The
>> downside is that this then still is dependent upon switching back to
>> real mode, so retrieving the needed information from multiboot info is
>> likely going to be desirable down the road.
>
> I'm unsure, what's the benefit from retrieving this information from
> the VESA blob rather than from multiboot(2) structures?
As said - it allows things to work even when that data isn't provided.
Note also how I say "for starters" - patch 2 adds logic to retrieve
the information from MB.
> Is it because we require a VESA mode to be set before we parse the
> multiboot information?
No, I don't think so.
>> --- a/xen/arch/x86/boot/video.S
>> +++ b/xen/arch/x86/boot/video.S
>> @@ -575,7 +575,6 @@ set14: movw $0x1111, %ax
>> movb $0x01, %ah # Define cursor scan lines 11-12
>> movw $0x0b0c, %cx
>> int $0x10
>> -set_current:
>> stc
>> ret
>>
>> @@ -693,6 +692,39 @@ vga_modes:
>> .word VIDEO_80x60, 0x50,0x3c,0 # 80x60
>> vga_modes_end:
>>
>> +# If the current mode is a VESA graphics one, obtain its parameters.
>> +set_current:
>> + leaw vesa_glob_info, %di
>> + movw $0x4f00, %ax
>> + int $0x10
>> + cmpw $0x004f, %ax
>> + jne .Lsetc_done
>
> You don't seem to make use of the information fetched here? I guess
> this is somehow required to access the other functions?
See the similar logic at check_vesa. The information is used later, by
mode_params (half way into mopar_gr). Quite likely this could be done
just in a single place, but that would require some restructuring of
the code, which I'd like to avoid doing here.
>> + movw $0x4f03, %ax
>
> It would help readability to have defines for those values, ie:
> VESA_GET_CURRENT_MODE or some such (not that you need to do it here,
> just a comment).
Right - this applies to all of our BIOS interfacing code, I guess.
>> + int $0x10
>> + cmpw $0x004f, %ax
>> + jne .Lsetc_done
>> +
>> + leaw vesa_mode_info, %di # Get mode information structure
>> + movw %bx, %cx
>> + movw $0x4f01, %ax
>> + int $0x10
>> + cmpw $0x004f, %ax
>> + jne .Lsetc_done
>> +
>> + movb (%di), %al # Check mode attributes
>> + andb $0x9b, %al
>> + cmpb $0x9b, %al
>
> So you also check that the reserved D1 bit is set to 1 as mandated by
> the spec. This is slightly different than what's done in check_vesa,
> would you mind adding a define for this an unifying with check_vesa?
Well, see the v2 changelog comment. I'm somewhat hesitant to do that
here; I'd prefer to consolidate this in a separate patch.
Jan
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 1/8] x86/boot: make "vga=current" work with graphics modes
2022-04-04 15:50 ` Jan Beulich
@ 2022-04-05 8:24 ` Roger Pau Monné
2022-04-05 8:36 ` Jan Beulich
0 siblings, 1 reply; 38+ messages in thread
From: Roger Pau Monné @ 2022-04-05 8:24 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu
On Mon, Apr 04, 2022 at 05:50:57PM +0200, Jan Beulich wrote:
> (reducing Cc list some)
>
> On 04.04.2022 16:49, Roger Pau Monné wrote:
> > On Thu, Mar 31, 2022 at 11:44:10AM +0200, Jan Beulich wrote:
> >> GrUB2 can be told to leave the screen in the graphics mode it has been
> >> using (or any other one), via "set gfxpayload=keep" (or suitable
> >> variants thereof). In this case we can avoid doing another mode switch
> >> ourselves. This in particular avoids possibly setting the screen to a
> >> less desirable mode: On one of my test systems the set of modes
> >> reported available by the VESA BIOS depends on whether the interposed
> >> KVM switch has that machine set as the active one. If it's not active,
> >> only modes up to 1024x768 get reported, while when active 1280x1024
> >> modes are also included. For things to always work with an explicitly
> >> specified mode (via the "vga=" option), that mode therefore needs be a
> >> 1024x768 one.
So this patch helps you by not having to set a mode and just relying
on the mode set by GrUB?
> >>
> >> For some reason this only works for me with "multiboot2" (and
> >> "module2"); "multiboot" (and "module") still forces the screen into text
> >> mode, despite my reading of the sources suggesting otherwise.
> >>
> >> For starters I'm limiting this to graphics modes; I do think this ought
> >> to also work for text modes, but
> >> - I can't tell whether GrUB2 can set any text mode other than 80x25
> >> (I've only found plain "text" to be valid as a "gfxpayload" setting),
> >> - I'm uncertain whether supporting that is worth it, since I'm uncertain
> >> how many people would be running their systems/screens in text mode,
> >> - I'd like to limit the amount of code added to the realmode trampoline.
> >>
> >> For starters I'm also limiting mode information retrieval to raw BIOS
> >> accesses. This will allow things to work (in principle) also with other
> >> boot environments where a graphics mode can be left in place. The
> >> downside is that this then still is dependent upon switching back to
> >> real mode, so retrieving the needed information from multiboot info is
> >> likely going to be desirable down the road.
> >
> > I'm unsure, what's the benefit from retrieving this information from
> > the VESA blob rather than from multiboot(2) structures?
>
> As said - it allows things to work even when that data isn't provided.
> Note also how I say "for starters" - patch 2 adds logic to retrieve
> the information from MB.
>
> > Is it because we require a VESA mode to be set before we parse the
> > multiboot information?
>
> No, I don't think so.
>
> >> --- a/xen/arch/x86/boot/video.S
> >> +++ b/xen/arch/x86/boot/video.S
> >> @@ -575,7 +575,6 @@ set14: movw $0x1111, %ax
> >> movb $0x01, %ah # Define cursor scan lines 11-12
> >> movw $0x0b0c, %cx
> >> int $0x10
> >> -set_current:
> >> stc
> >> ret
> >>
> >> @@ -693,6 +692,39 @@ vga_modes:
> >> .word VIDEO_80x60, 0x50,0x3c,0 # 80x60
> >> vga_modes_end:
> >>
> >> +# If the current mode is a VESA graphics one, obtain its parameters.
> >> +set_current:
> >> + leaw vesa_glob_info, %di
> >> + movw $0x4f00, %ax
> >> + int $0x10
> >> + cmpw $0x004f, %ax
> >> + jne .Lsetc_done
> >
> > You don't seem to make use of the information fetched here? I guess
> > this is somehow required to access the other functions?
>
> See the similar logic at check_vesa. The information is used later, by
> mode_params (half way into mopar_gr). Quite likely this could be done
> just in a single place, but that would require some restructuring of
> the code, which I'd like to avoid doing here.
I didn't realize check_vesa and set_current where mutually
exclusive.
> >> + movw $0x4f03, %ax
> >
> > It would help readability to have defines for those values, ie:
> > VESA_GET_CURRENT_MODE or some such (not that you need to do it here,
> > just a comment).
>
> Right - this applies to all of our BIOS interfacing code, I guess.
>
> >> + int $0x10
> >> + cmpw $0x004f, %ax
> >> + jne .Lsetc_done
> >> +
> >> + leaw vesa_mode_info, %di # Get mode information structure
> >> + movw %bx, %cx
> >> + movw $0x4f01, %ax
> >> + int $0x10
> >> + cmpw $0x004f, %ax
> >> + jne .Lsetc_done
> >> +
> >> + movb (%di), %al # Check mode attributes
> >> + andb $0x9b, %al
> >> + cmpb $0x9b, %al
> >
> > So you also check that the reserved D1 bit is set to 1 as mandated by
> > the spec. This is slightly different than what's done in check_vesa,
> > would you mind adding a define for this an unifying with check_vesa?
>
> Well, see the v2 changelog comment. I'm somewhat hesitant to do that
> here; I'd prefer to consolidate this in a separate patch.
Sorry, didn't notice that v2 comment before.
It's my understanding that the main difference this patch introduces
is that set_current now fetches the currently set mode, so that we
avoid further mode changes if the mode set already matches the
selected one, or if Xen is to use the already set mode?
Thanks, Roger.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 1/8] x86/boot: make "vga=current" work with graphics modes
2022-04-05 8:24 ` Roger Pau Monné
@ 2022-04-05 8:36 ` Jan Beulich
0 siblings, 0 replies; 38+ messages in thread
From: Jan Beulich @ 2022-04-05 8:36 UTC (permalink / raw)
To: Roger Pau Monné; +Cc: xen-devel, Andrew Cooper, Wei Liu
On 05.04.2022 10:24, Roger Pau Monné wrote:
> On Mon, Apr 04, 2022 at 05:50:57PM +0200, Jan Beulich wrote:
>> (reducing Cc list some)
>>
>> On 04.04.2022 16:49, Roger Pau Monné wrote:
>>> On Thu, Mar 31, 2022 at 11:44:10AM +0200, Jan Beulich wrote:
>>>> GrUB2 can be told to leave the screen in the graphics mode it has been
>>>> using (or any other one), via "set gfxpayload=keep" (or suitable
>>>> variants thereof). In this case we can avoid doing another mode switch
>>>> ourselves. This in particular avoids possibly setting the screen to a
>>>> less desirable mode: On one of my test systems the set of modes
>>>> reported available by the VESA BIOS depends on whether the interposed
>>>> KVM switch has that machine set as the active one. If it's not active,
>>>> only modes up to 1024x768 get reported, while when active 1280x1024
>>>> modes are also included. For things to always work with an explicitly
>>>> specified mode (via the "vga=" option), that mode therefore needs be a
>>>> 1024x768 one.
>
> So this patch helps you by not having to set a mode and just relying
> on the mode set by GrUB?
Yes, but it goes beyond that: The modes offered by VESA on the particular
system don't include the higher resolution one under certain circumstances,
so I cannot tell Xen to switch to that mode. By not having to tell Xen a
specific mode (but rather inherit that set / left active by the boot
loader), I can leverage the better mode in most cases, but things will
still work if I turn on (or reset) the system with another machine being
the presently selected one at the KVM switch.
But yes, beyond the particular quirk on this system the benefit is one
less mode switch and hence less screen flickering and slightly faster
boot.
>>>> --- a/xen/arch/x86/boot/video.S
>>>> +++ b/xen/arch/x86/boot/video.S
>>>> @@ -575,7 +575,6 @@ set14: movw $0x1111, %ax
>>>> movb $0x01, %ah # Define cursor scan lines 11-12
>>>> movw $0x0b0c, %cx
>>>> int $0x10
>>>> -set_current:
>>>> stc
>>>> ret
>>>>
>>>> @@ -693,6 +692,39 @@ vga_modes:
>>>> .word VIDEO_80x60, 0x50,0x3c,0 # 80x60
>>>> vga_modes_end:
>>>>
>>>> +# If the current mode is a VESA graphics one, obtain its parameters.
>>>> +set_current:
>>>> + leaw vesa_glob_info, %di
>>>> + movw $0x4f00, %ax
>>>> + int $0x10
>>>> + cmpw $0x004f, %ax
>>>> + jne .Lsetc_done
>>>
>>> You don't seem to make use of the information fetched here? I guess
>>> this is somehow required to access the other functions?
>>
>> See the similar logic at check_vesa. The information is used later, by
>> mode_params (half way into mopar_gr). Quite likely this could be done
>> just in a single place, but that would require some restructuring of
>> the code, which I'd like to avoid doing here.
>
> I didn't realize check_vesa and set_current where mutually
> exclusive.
>
>>>> + movw $0x4f03, %ax
>>>
>>> It would help readability to have defines for those values, ie:
>>> VESA_GET_CURRENT_MODE or some such (not that you need to do it here,
>>> just a comment).
>>
>> Right - this applies to all of our BIOS interfacing code, I guess.
>>
>>>> + int $0x10
>>>> + cmpw $0x004f, %ax
>>>> + jne .Lsetc_done
>>>> +
>>>> + leaw vesa_mode_info, %di # Get mode information structure
>>>> + movw %bx, %cx
>>>> + movw $0x4f01, %ax
>>>> + int $0x10
>>>> + cmpw $0x004f, %ax
>>>> + jne .Lsetc_done
>>>> +
>>>> + movb (%di), %al # Check mode attributes
>>>> + andb $0x9b, %al
>>>> + cmpb $0x9b, %al
>>>
>>> So you also check that the reserved D1 bit is set to 1 as mandated by
>>> the spec. This is slightly different than what's done in check_vesa,
>>> would you mind adding a define for this an unifying with check_vesa?
>>
>> Well, see the v2 changelog comment. I'm somewhat hesitant to do that
>> here; I'd prefer to consolidate this in a separate patch.
>
> Sorry, didn't notice that v2 comment before.
>
> It's my understanding that the main difference this patch introduces
> is that set_current now fetches the currently set mode, so that we
> avoid further mode changes if the mode set already matches the
> selected one, or if Xen is to use the already set mode?
Not exactly: You either tell Xen to use the current mode ("vga=current")
or you tell Xen to use a specific mode ("vga=<mode>"). Checking whether
the present mode is the (specific) one Xen was told to switch to would
require yet more work. But skipping a requested mode switch can also
have unintended consequences, so I wouldn't even be certain we would
want to go such a route.
Jan
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 1/8] x86/boot: make "vga=current" work with graphics modes
2022-03-31 9:44 ` [PATCH v4 1/8] x86/boot: make "vga=current" work with graphics modes Jan Beulich
2022-04-04 14:49 ` Roger Pau Monné
@ 2022-04-05 8:45 ` Roger Pau Monné
2022-04-06 14:23 ` Jan Beulich
1 sibling, 1 reply; 38+ messages in thread
From: Roger Pau Monné @ 2022-04-05 8:45 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel, Andrew Cooper, Wei Liu, community.manager, Henry Wang
On Thu, Mar 31, 2022 at 11:44:10AM +0200, Jan Beulich wrote:
> GrUB2 can be told to leave the screen in the graphics mode it has been
> using (or any other one), via "set gfxpayload=keep" (or suitable
> variants thereof). In this case we can avoid doing another mode switch
> ourselves. This in particular avoids possibly setting the screen to a
> less desirable mode: On one of my test systems the set of modes
> reported available by the VESA BIOS depends on whether the interposed
> KVM switch has that machine set as the active one. If it's not active,
> only modes up to 1024x768 get reported, while when active 1280x1024
> modes are also included. For things to always work with an explicitly
> specified mode (via the "vga=" option), that mode therefore needs be a
> 1024x768 one.
>
> For some reason this only works for me with "multiboot2" (and
> "module2"); "multiboot" (and "module") still forces the screen into text
> mode, despite my reading of the sources suggesting otherwise.
>
> For starters I'm limiting this to graphics modes; I do think this ought
> to also work for text modes, but
> - I can't tell whether GrUB2 can set any text mode other than 80x25
> (I've only found plain "text" to be valid as a "gfxpayload" setting),
> - I'm uncertain whether supporting that is worth it, since I'm uncertain
> how many people would be running their systems/screens in text mode,
> - I'd like to limit the amount of code added to the realmode trampoline.
>
> For starters I'm also limiting mode information retrieval to raw BIOS
> accesses. This will allow things to work (in principle) also with other
> boot environments where a graphics mode can be left in place. The
> downside is that this then still is dependent upon switching back to
> real mode, so retrieving the needed information from multiboot info is
> likely going to be desirable down the road.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
Thanks, Roger.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 2/8] x86/boot: obtain video info from boot loader
2022-03-31 9:45 ` [PATCH v4 2/8] x86/boot: obtain video info from boot loader Jan Beulich
@ 2022-04-05 9:35 ` Roger Pau Monné
2022-04-05 10:57 ` Jan Beulich
0 siblings, 1 reply; 38+ messages in thread
From: Roger Pau Monné @ 2022-04-05 9:35 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel, Andrew Cooper, George Dunlap, Julien Grall,
Stefano Stabellini, Wei Liu
On Thu, Mar 31, 2022 at 11:45:02AM +0200, Jan Beulich wrote:
> With MB2 the boot loader may provide this information, allowing us to
> obtain it without needing to enter real mode (assuming we don't need to
> set a new mode from "vga=", but can instead inherit the one the
> bootloader may have established).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v4: Re-base.
> v3: Re-base.
> v2: New.
>
> --- a/xen/arch/x86/boot/defs.h
> +++ b/xen/arch/x86/boot/defs.h
> @@ -53,6 +53,7 @@ typedef unsigned int u32;
> typedef unsigned long long u64;
> typedef unsigned int size_t;
> typedef u8 uint8_t;
> +typedef u16 uint16_t;
> typedef u32 uint32_t;
> typedef u64 uint64_t;
>
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -562,12 +562,18 @@ trampoline_setup:
> mov %esi, sym_esi(xen_phys_start)
> mov %esi, sym_esi(trampoline_xen_phys_start)
>
> - mov sym_esi(trampoline_phys), %ecx
> -
> /* Get bottom-most low-memory stack address. */
> + mov sym_esi(trampoline_phys), %ecx
> add $TRAMPOLINE_SPACE,%ecx
Just for my understanding, since you are already touching the
instruction, why not switch it to a lea like you do below?
Is that because you would also like to take the opportunity to fold
the add into the lea and that would be too much of a change?
>
> +#ifdef CONFIG_VIDEO
> + lea sym_esi(boot_vid_info), %edx
> +#else
> + xor %edx, %edx
> +#endif
> +
> /* Save Multiboot / PVH info struct (after relocation) for later use. */
> + push %edx /* Boot video info to be filled from MB2. */
> push %ecx /* Bottom-most low-memory stack address. */
> push %ebx /* Multiboot / PVH information address. */
> push %eax /* Magic number. */
> --- a/xen/arch/x86/boot/reloc.c
> +++ b/xen/arch/x86/boot/reloc.c
> @@ -14,9 +14,10 @@
>
> /*
> * This entry point is entered from xen/arch/x86/boot/head.S with:
> - * - 0x4(%esp) = MAGIC,
> - * - 0x8(%esp) = INFORMATION_ADDRESS,
> - * - 0xc(%esp) = TOPMOST_LOW_MEMORY_STACK_ADDRESS.
> + * - 0x04(%esp) = MAGIC,
> + * - 0x08(%esp) = INFORMATION_ADDRESS,
> + * - 0x0c(%esp) = TOPMOST_LOW_MEMORY_STACK_ADDRESS.
> + * - 0x10(%esp) = BOOT_VIDEO_INFO_ADDRESS.
> */
> asm (
> " .text \n"
> @@ -32,6 +33,39 @@ asm (
> #include "../../../include/xen/kconfig.h"
> #include <public/arch-x86/hvm/start_info.h>
>
> +#ifdef CONFIG_VIDEO
> +# include "video.h"
> +
> +/* VESA control information */
> +struct __packed vesa_ctrl_info {
> + uint8_t signature[4];
> + uint16_t version;
> + uint32_t oem_name;
> + uint32_t capabilities;
> + uint32_t mode_list;
> + uint16_t mem_size;
> + /* We don't use any further fields. */
> +};
> +
> +/* VESA 2.0 mode information */
> +struct vesa_mode_info {
Should we add __packed here just in case further added fields are no
longer naturally aligned? (AFAICT all field right now are aligned to
it's size so there's no need for it).
> + uint16_t attrib;
> + uint8_t window[14]; /* We don't use the individual fields. */
> + uint16_t bytes_per_line;
> + uint16_t width;
> + uint16_t height;
> + uint8_t cell_width;
> + uint8_t cell_height;
> + uint8_t nr_planes;
> + uint8_t depth;
> + uint8_t memory[5]; /* We don't use the individual fields. */
> + struct boot_video_colors colors;
> + uint8_t direct_color;
> + uint32_t base;
> + /* We don't use any further fields. */
> +};
Would it make sense to put those struct definitions in boot/video.h
like you do for boot_video_info?
I also wonder whether you could then hide the #ifdef CONFIG_VIDEO
check inside of the header itself.
> +#endif /* CONFIG_VIDEO */
> +
> #define get_mb2_data(tag, type, member) (((multiboot2_tag_##type##_t *)(tag))->member)
> #define get_mb2_string(tag, type, member) ((u32)get_mb2_data(tag, type, member))
>
> @@ -146,7 +180,7 @@ static multiboot_info_t *mbi_reloc(u32 m
> return mbi_out;
> }
>
> -static multiboot_info_t *mbi2_reloc(u32 mbi_in)
> +static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, uint32_t video_out)
> {
> const multiboot2_fixed_t *mbi_fix = _p(mbi_in);
> const multiboot2_memory_map_t *mmap_src;
> @@ -154,6 +188,9 @@ static multiboot_info_t *mbi2_reloc(u32
> module_t *mbi_out_mods = NULL;
> memory_map_t *mmap_dst;
> multiboot_info_t *mbi_out;
> +#ifdef CONFIG_VIDEO
> + struct boot_video_info *video = NULL;
> +#endif
> u32 ptr;
> unsigned int i, mod_idx = 0;
>
> @@ -254,17 +291,64 @@ static multiboot_info_t *mbi2_reloc(u32
> ++mod_idx;
> break;
>
> +#ifdef CONFIG_VIDEO
> + case MULTIBOOT2_TAG_TYPE_VBE:
> + if ( video_out )
> + {
> + const struct vesa_ctrl_info *ci;
> + const struct vesa_mode_info *mi;
> +
> + video = _p(video_out);
> + ci = (void *)get_mb2_data(tag, vbe, vbe_control_info);
> + mi = (void *)get_mb2_data(tag, vbe, vbe_mode_info);
> +
> + if ( ci->version >= 0x0200 && (mi->attrib & 0x9b) == 0x9b )
> + {
> + video->capabilities = ci->capabilities;
> + video->lfb_linelength = mi->bytes_per_line;
> + video->lfb_width = mi->width;
> + video->lfb_height = mi->height;
> + video->lfb_depth = mi->depth;
> + video->lfb_base = mi->base;
> + video->lfb_size = ci->mem_size;
> + video->colors = mi->colors;
> + video->vesa_attrib = mi->attrib;
> + }
> +
> + video->vesapm.seg = get_mb2_data(tag, vbe, vbe_interface_seg);
> + video->vesapm.off = get_mb2_data(tag, vbe, vbe_interface_off);
> + }
> + break;
> +
> + case MULTIBOOT2_TAG_TYPE_FRAMEBUFFER:
> + if ( (get_mb2_data(tag, framebuffer, framebuffer_type) !=
> + MULTIBOOT2_FRAMEBUFFER_TYPE_RGB) )
> + {
> + video_out = 0;
> + video = NULL;
> + }
I'm confused, don't you need to store the information in the
framebuffer tag for use after relocation?
> + break;
> +#endif /* CONFIG_VIDEO */
> +
> case MULTIBOOT2_TAG_TYPE_END:
> - return mbi_out;
> + goto end; /* Cannot "break;" here. */
>
> default:
> break;
> }
>
> + end:
> +
> +#ifdef CONFIG_VIDEO
> + if ( video )
> + video->orig_video_isVGA = 0x23;
I see we use this elsewhere, what's the meaning of this (magic) 0x23?
Thanks, Roger.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 3/8] x86/EFI: retrieve EDID
2022-03-31 9:45 ` [PATCH v4 3/8] x86/EFI: retrieve EDID Jan Beulich
2022-04-01 10:15 ` Luca Fancellu
@ 2022-04-05 10:27 ` Roger Pau Monné
2022-04-05 14:36 ` Jan Beulich
2022-04-06 14:24 ` Jan Beulich
2022-04-06 14:34 ` Bertrand Marquis
3 siblings, 1 reply; 38+ messages in thread
From: Roger Pau Monné @ 2022-04-05 10:27 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel, Andrew Cooper, Wei Liu, Julien Grall,
Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis
On Thu, Mar 31, 2022 at 11:45:36AM +0200, Jan Beulich wrote:
> When booting directly from EFI, obtaining this information from EFI is
> the only possible way. And even when booting with a boot loader
> interposed, it's more clean not to use legacy BIOS calls for this
> purpose. (The downside being that there are no "capabilities" that we
> can retrieve the EFI way.)
>
> To achieve this we need to propagate the handle used to obtain the
> EFI_GRAPHICS_OUTPUT_PROTOCOL instance for further obtaining an
> EFI_EDID_*_PROTOCOL instance, which has been part of the spec since 2.5.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Setting boot_edid_caps to zero isn't desirable, but arbitrarily setting
> one or both of the two low bits also doesn't seem appropriate.
>
> GrUB also checks an "agp-internal-edid" variable. As I haven't been able
> to find any related documentation, and as GrUB being happy about the
> variable being any size (rather than at least / precisely 128 bytes),
> I didn't follow that route.
> ---
> v3: Re-base.
> v2: New.
>
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -464,6 +464,10 @@ static void __init efi_arch_edd(void)
> {
> }
>
> +static void __init efi_arch_edid(EFI_HANDLE gop_handle)
> +{
> +}
> +
> static void __init efi_arch_memory_setup(void)
> {
> }
> --- a/xen/arch/x86/boot/video.S
> +++ b/xen/arch/x86/boot/video.S
> @@ -922,7 +922,14 @@ store_edid:
> pushw %dx
> pushw %di
>
> - cmpb $1, bootsym(opt_edid) # EDID disabled on cmdline (edid=no)?
> + movb bootsym(opt_edid), %al
> + cmpw $0x1313, bootsym(boot_edid_caps) # Data already retrieved?
> + je .Lcheck_edid
> + cmpb $2, %al # EDID forced on cmdline (edid=force)?
> + jne .Lno_edid
> +
> +.Lcheck_edid:
> + cmpb $1, %al # EDID disabled on cmdline (edid=no)?
> je .Lno_edid
>
> leaw vesa_glob_info, %di
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -568,6 +568,49 @@ static void __init efi_arch_video_init(E
> #endif
> }
>
> +#ifdef CONFIG_VIDEO
> +static bool __init copy_edid(const void *buf, unsigned int size)
> +{
> + /*
> + * Be conservative - for both undersized and oversized blobs it is unclear
> + * what to actually do with them. The more that unlike the VESA BIOS
> + * interface we also have no associated "capabilities" value (which might
> + * carry a hint as to possible interpretation).
> + */
> + if ( size != ARRAY_SIZE(boot_edid_info) )
> + return false;
> +
> + memcpy(boot_edid_info, buf, size);
> + boot_edid_caps = 0;
> +
> + return true;
> +}
> +#endif
> +
> +static void __init efi_arch_edid(EFI_HANDLE gop_handle)
> +{
> +#ifdef CONFIG_VIDEO
> + static EFI_GUID __initdata active_guid = EFI_EDID_ACTIVE_PROTOCOL_GUID;
> + static EFI_GUID __initdata discovered_guid = EFI_EDID_DISCOVERED_PROTOCOL_GUID;
Is there a need to make those static?
I think this function is either called from efi_start or
efi_multiboot, but there aren't multiple calls to it? (also both
parameters are IN only, so not to be changed by the EFI method?
I have the feeling setting them to static is done because they can't
be set to const?
> + EFI_EDID_ACTIVE_PROTOCOL *active_edid;
> + EFI_EDID_DISCOVERED_PROTOCOL *discovered_edid;
> + EFI_STATUS status;
> +
> + status = efi_bs->OpenProtocol(gop_handle, &active_guid,
> + (void **)&active_edid, efi_ih, NULL,
> + EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> + if ( status == EFI_SUCCESS &&
> + copy_edid(active_edid->Edid, active_edid->SizeOfEdid) )
> + return;
Isn't it enough to just call EFI_EDID_ACTIVE_PROTOCOL_GUID?
From my reading of the UEFI spec this will either return
EFI_EDID_OVERRIDE_PROTOCOL_GUID or EFI_EDID_DISCOVERED_PROTOCOL_GUID.
If EFI_EDID_OVERRIDE_PROTOCOL is set it must be used, and hence
falling back to EFI_EDID_DISCOVERED_PROTOCOL_GUID if
EFI_EDID_ACTIVE_PROTOCOL_GUID cannot be parsed would likely mean
ignoring EFI_EDID_OVERRIDE_PROTOCOL?
> + status = efi_bs->OpenProtocol(gop_handle, &discovered_guid,
> + (void **)&discovered_edid, efi_ih, NULL,
> + EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> + if ( status == EFI_SUCCESS )
> + copy_edid(discovered_edid->Edid, discovered_edid->SizeOfEdid);
> +#endif
> +}
> +
> static void __init efi_arch_memory_setup(void)
> {
> unsigned int i;
> @@ -729,6 +772,7 @@ static void __init efi_arch_flush_dcache
> void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
> {
> EFI_GRAPHICS_OUTPUT_PROTOCOL *gop;
> + EFI_HANDLE gop_handle;
> UINTN cols, gop_mode = ~0, rows;
>
> __set_bit(EFI_BOOT, &efi_flags);
> @@ -742,11 +786,15 @@ void __init efi_multiboot2(EFI_HANDLE Im
> &cols, &rows) == EFI_SUCCESS )
> efi_arch_console_init(cols, rows);
>
> - gop = efi_get_gop();
> + gop = efi_get_gop(&gop_handle);
>
> if ( gop )
> + {
> gop_mode = efi_find_gop_mode(gop, 0, 0, 0);
>
> + efi_arch_edid(gop_handle);
> + }
> +
> efi_arch_edd();
> efi_arch_cpu();
>
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -118,7 +118,7 @@ static bool read_section(const EFI_LOADE
>
> static void efi_init(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable);
> static void efi_console_set_mode(void);
> -static EFI_GRAPHICS_OUTPUT_PROTOCOL *efi_get_gop(void);
> +static EFI_GRAPHICS_OUTPUT_PROTOCOL *efi_get_gop(EFI_HANDLE *gop_handle);
> static UINTN efi_find_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
> UINTN cols, UINTN rows, UINTN depth);
> static void efi_tables(void);
> @@ -758,7 +758,7 @@ static void __init efi_console_set_mode(
> StdOut->SetMode(StdOut, best);
> }
>
> -static EFI_GRAPHICS_OUTPUT_PROTOCOL __init *efi_get_gop(void)
> +static EFI_GRAPHICS_OUTPUT_PROTOCOL __init *efi_get_gop(EFI_HANDLE *gop_handle)
> {
> EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info;
> EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
> @@ -783,7 +783,10 @@ static EFI_GRAPHICS_OUTPUT_PROTOCOL __in
> continue;
> status = gop->QueryMode(gop, gop->Mode->Mode, &info_size, &mode_info);
> if ( !EFI_ERROR(status) )
> + {
> + *gop_handle = handles[i];
> break;
> + }
> }
> if ( handles )
> efi_bs->FreePool(handles);
> @@ -1222,6 +1225,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
> if ( use_cfg_file )
> {
> EFI_FILE_HANDLE dir_handle;
> + EFI_HANDLE gop_handle;
> UINTN depth, cols, rows, size;
>
> size = cols = rows = depth = 0;
> @@ -1230,7 +1234,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
> &cols, &rows) == EFI_SUCCESS )
> efi_arch_console_init(cols, rows);
>
> - gop = efi_get_gop();
> + gop = efi_get_gop(&gop_handle);
>
> /* Get the file system interface. */
> dir_handle = get_parent_handle(loaded_image, &file_name);
> @@ -1360,7 +1364,11 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
> dir_handle->Close(dir_handle);
>
> if ( gop && !base_video )
> + {
> gop_mode = efi_find_gop_mode(gop, cols, rows, depth);
> +
> + efi_arch_edid(gop_handle);
> + }
> }
>
> /* Get the number of boot modules specified on the DT or an error (<0) */
> @@ -1387,7 +1395,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
>
> efi_arch_edd();
>
> - /* XXX Collect EDID info. */
> efi_arch_cpu();
>
> efi_tables();
> --- a/xen/include/efi/efiprot.h
> +++ b/xen/include/efi/efiprot.h
> @@ -724,5 +724,52 @@ struct _EFI_GRAPHICS_OUTPUT_PROTOCOL {
> EFI_GRAPHICS_OUTPUT_PROTOCOL_BLT Blt;
> EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE *Mode;
> };
> +
> +/*
> + * EFI EDID Discovered Protocol
> + * UEFI Specification Version 2.5 Section 11.9
> + */
> +#define EFI_EDID_DISCOVERED_PROTOCOL_GUID \
> + { 0x1C0C34F6, 0xD380, 0x41FA, { 0xA0, 0x49, 0x8a, 0xD0, 0x6C, 0x1A, 0x66, 0xAA} }
> +
> +typedef struct _EFI_EDID_DISCOVERED_PROTOCOL {
> + UINT32 SizeOfEdid;
> + UINT8 *Edid;
> +} EFI_EDID_DISCOVERED_PROTOCOL;
> +
> +/*
> + * EFI EDID Active Protocol
> + * UEFI Specification Version 2.5 Section 11.9
> + */
> +#define EFI_EDID_ACTIVE_PROTOCOL_GUID \
> + { 0xBD8C1056, 0x9F36, 0x44EC, { 0x92, 0xA8, 0xA6, 0x33, 0x7F, 0x81, 0x79, 0x86} }
> +
> +typedef struct _EFI_EDID_ACTIVE_PROTOCOL {
> + UINT32 SizeOfEdid;
> + UINT8 *Edid;
> +} EFI_EDID_ACTIVE_PROTOCOL;
> +
> +/*
> + * EFI EDID Override Protocol
> + * UEFI Specification Version 2.5 Section 11.9
> + */
> +#define EFI_EDID_OVERRIDE_PROTOCOL_GUID \
> + { 0x48ECB431, 0xFB72, 0x45C0, { 0xA9, 0x22, 0xF4, 0x58, 0xFE, 0x04, 0x0B, 0xD5} }
> +
> +INTERFACE_DECL(_EFI_EDID_OVERRIDE_PROTOCOL);
> +
> +typedef
> +EFI_STATUS
> +(EFIAPI *EFI_EDID_OVERRIDE_PROTOCOL_GET_EDID) (
> + IN struct _EFI_EDID_OVERRIDE_PROTOCOL *This,
> + IN EFI_HANDLE *ChildHandle,
> + OUT UINT32 *Attributes,
> + IN OUT UINTN *EdidSize,
> + IN OUT UINT8 **Edid);
> +
> +typedef struct _EFI_EDID_OVERRIDE_PROTOCOL {
> + EFI_EDID_OVERRIDE_PROTOCOL_GET_EDID GetEdid;
> +} EFI_EDID_OVERRIDE_PROTOCOL;
> +
> #endif
FWIW, EFI_EDID_OVERRIDE_PROTOCOL_GUID is not used by the patch, so I
guess it's introduced for completeness (or because it's used by
further patches).
Thanks, Roger.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 4/8] x86/boot: simplify mode_table
2022-03-31 9:48 ` [PATCH v4 4/8] x86/boot: simplify mode_table Jan Beulich
@ 2022-04-05 10:44 ` Roger Pau Monné
0 siblings, 0 replies; 38+ messages in thread
From: Roger Pau Monné @ 2022-04-05 10:44 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu
On Thu, Mar 31, 2022 at 11:48:51AM +0200, Jan Beulich wrote:
> There's no point in writing 80x25 text mode information via multiple
> insns all storing immediate values. The data can simply be included
> first thing in the vga_modes table, allowing the already present
> REP MOVSB to take care of everything in one go.
>
> While touching this also correct a related but stale comment.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Thanks, Roger.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 5/8] x86/boot: fold branches in video handling code
2022-03-31 9:49 ` [PATCH v4 5/8] x86/boot: fold branches in video handling code Jan Beulich
@ 2022-04-05 10:55 ` Roger Pau Monné
0 siblings, 0 replies; 38+ messages in thread
From: Roger Pau Monné @ 2022-04-05 10:55 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu
On Thu, Mar 31, 2022 at 11:49:24AM +0200, Jan Beulich wrote:
> Using Jcc to branch around a JMP is necessary only in pre-386 code,
> where Jcc is limited to disp8. Use the opposite Jcc directly in two
> places. Since it's adjacent, also convert an ORB to TESTB.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Thanks, Roger.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 2/8] x86/boot: obtain video info from boot loader
2022-04-05 9:35 ` Roger Pau Monné
@ 2022-04-05 10:57 ` Jan Beulich
2022-04-05 11:34 ` Roger Pau Monné
0 siblings, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2022-04-05 10:57 UTC (permalink / raw)
To: Roger Pau Monné
Cc: xen-devel, Andrew Cooper, George Dunlap, Julien Grall,
Stefano Stabellini, Wei Liu
On 05.04.2022 11:35, Roger Pau Monné wrote:
> On Thu, Mar 31, 2022 at 11:45:02AM +0200, Jan Beulich wrote:
>> --- a/xen/arch/x86/boot/head.S
>> +++ b/xen/arch/x86/boot/head.S
>> @@ -562,12 +562,18 @@ trampoline_setup:
>> mov %esi, sym_esi(xen_phys_start)
>> mov %esi, sym_esi(trampoline_xen_phys_start)
>>
>> - mov sym_esi(trampoline_phys), %ecx
>> -
>> /* Get bottom-most low-memory stack address. */
>> + mov sym_esi(trampoline_phys), %ecx
>> add $TRAMPOLINE_SPACE,%ecx
>
> Just for my understanding, since you are already touching the
> instruction, why not switch it to a lea like you do below?
>
> Is that because you would also like to take the opportunity to fold
> the add into the lea and that would be too much of a change?
No. This MOV cannot be converted, as its source operand isn't an
immediate (or register); such a conversion would also be undesirable,
for increasing insn size. See the later patch doing conversions in
the other direction, to reduce code size. Somewhat similarly ...
>> +#ifdef CONFIG_VIDEO
>> + lea sym_esi(boot_vid_info), %edx
... this LEA also cannot be expressed by a single MOV.
>> @@ -32,6 +33,39 @@ asm (
>> #include "../../../include/xen/kconfig.h"
>> #include <public/arch-x86/hvm/start_info.h>
>>
>> +#ifdef CONFIG_VIDEO
>> +# include "video.h"
>> +
>> +/* VESA control information */
>> +struct __packed vesa_ctrl_info {
>> + uint8_t signature[4];
>> + uint16_t version;
>> + uint32_t oem_name;
>> + uint32_t capabilities;
>> + uint32_t mode_list;
>> + uint16_t mem_size;
>> + /* We don't use any further fields. */
>> +};
>> +
>> +/* VESA 2.0 mode information */
>> +struct vesa_mode_info {
>
> Should we add __packed here just in case further added fields are no
> longer naturally aligned? (AFAICT all field right now are aligned to
> it's size so there's no need for it).
I think we should avoid __packed whenever possible.
>> + uint16_t attrib;
>> + uint8_t window[14]; /* We don't use the individual fields. */
>> + uint16_t bytes_per_line;
>> + uint16_t width;
>> + uint16_t height;
>> + uint8_t cell_width;
>> + uint8_t cell_height;
>> + uint8_t nr_planes;
>> + uint8_t depth;
>> + uint8_t memory[5]; /* We don't use the individual fields. */
>> + struct boot_video_colors colors;
>> + uint8_t direct_color;
>> + uint32_t base;
>> + /* We don't use any further fields. */
>> +};
>
> Would it make sense to put those struct definitions in boot/video.h
> like you do for boot_video_info?
Personally I prefer to expose things in headers only when multiple
other files want to consume what is being declared/defined.
>> @@ -254,17 +291,64 @@ static multiboot_info_t *mbi2_reloc(u32
>> ++mod_idx;
>> break;
>>
>> +#ifdef CONFIG_VIDEO
>> + case MULTIBOOT2_TAG_TYPE_VBE:
>> + if ( video_out )
>> + {
>> + const struct vesa_ctrl_info *ci;
>> + const struct vesa_mode_info *mi;
>> +
>> + video = _p(video_out);
>> + ci = (void *)get_mb2_data(tag, vbe, vbe_control_info);
>> + mi = (void *)get_mb2_data(tag, vbe, vbe_mode_info);
>> +
>> + if ( ci->version >= 0x0200 && (mi->attrib & 0x9b) == 0x9b )
>> + {
>> + video->capabilities = ci->capabilities;
>> + video->lfb_linelength = mi->bytes_per_line;
>> + video->lfb_width = mi->width;
>> + video->lfb_height = mi->height;
>> + video->lfb_depth = mi->depth;
>> + video->lfb_base = mi->base;
>> + video->lfb_size = ci->mem_size;
>> + video->colors = mi->colors;
>> + video->vesa_attrib = mi->attrib;
>> + }
>> +
>> + video->vesapm.seg = get_mb2_data(tag, vbe, vbe_interface_seg);
>> + video->vesapm.off = get_mb2_data(tag, vbe, vbe_interface_off);
>> + }
>> + break;
>> +
>> + case MULTIBOOT2_TAG_TYPE_FRAMEBUFFER:
>> + if ( (get_mb2_data(tag, framebuffer, framebuffer_type) !=
>> + MULTIBOOT2_FRAMEBUFFER_TYPE_RGB) )
>> + {
>> + video_out = 0;
>> + video = NULL;
>> + }
>
> I'm confused, don't you need to store the information in the
> framebuffer tag for use after relocation?
If there was a consumer - yes. Right now this tag is used only to
invalidate the information taken from the other tag (or to suppress
taking values from there if that other tag came later) in case the
framebuffer type doesn't match what we support.
>> + break;
>> +#endif /* CONFIG_VIDEO */
>> +
>> case MULTIBOOT2_TAG_TYPE_END:
>> - return mbi_out;
>> + goto end; /* Cannot "break;" here. */
>>
>> default:
>> break;
>> }
>>
>> + end:
>> +
>> +#ifdef CONFIG_VIDEO
>> + if ( video )
>> + video->orig_video_isVGA = 0x23;
>
> I see we use this elsewhere, what's the meaning of this (magic) 0x23?
This is a value Linux uses (also as a plain number without any #define
iirc; at least it was that way when we first inherited that value).
Short of knowing where they took it from or what meaning they associate
with the value it would be hard for us to give this a (meaningful) name
and hence use a #define. And hence it's equally hard to properly answer
your question.
Jan
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 6/8] x86/boot: fold/replace moves in video handling code
2022-03-31 9:50 ` [PATCH v4 6/8] x86/boot: fold/replace moves " Jan Beulich
@ 2022-04-05 11:13 ` Roger Pau Monné
0 siblings, 0 replies; 38+ messages in thread
From: Roger Pau Monné @ 2022-04-05 11:13 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu
On Thu, Mar 31, 2022 at 11:50:00AM +0200, Jan Beulich wrote:
> Replace (mainly) MOV forms with shorter insns (or sequences thereof).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
Thanks, Roger.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 7/8] x86/boot: LEA -> MOV in video handling code
2022-03-31 9:50 ` [PATCH v4 7/8] x86/boot: LEA -> MOV " Jan Beulich
@ 2022-04-05 11:28 ` Roger Pau Monné
0 siblings, 0 replies; 38+ messages in thread
From: Roger Pau Monné @ 2022-04-05 11:28 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu
On Thu, Mar 31, 2022 at 11:50:20AM +0200, Jan Beulich wrote:
> Replace most LEA instances with (one byte shorter) MOV.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
Thanks, Roger.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 8/8] x86/boot: fold two MOVs into an ADD
2022-03-31 9:51 ` [PATCH v4 8/8] x86/boot: fold two MOVs into an ADD Jan Beulich
@ 2022-04-05 11:29 ` Roger Pau Monné
0 siblings, 0 replies; 38+ messages in thread
From: Roger Pau Monné @ 2022-04-05 11:29 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu
On Thu, Mar 31, 2022 at 11:51:02AM +0200, Jan Beulich wrote:
> There's no point going through %ax; the addition can be done directly in
> %di.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
Thanks, Roger.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 2/8] x86/boot: obtain video info from boot loader
2022-04-05 10:57 ` Jan Beulich
@ 2022-04-05 11:34 ` Roger Pau Monné
0 siblings, 0 replies; 38+ messages in thread
From: Roger Pau Monné @ 2022-04-05 11:34 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel, Andrew Cooper, George Dunlap, Julien Grall,
Stefano Stabellini, Wei Liu
On Tue, Apr 05, 2022 at 12:57:51PM +0200, Jan Beulich wrote:
> On 05.04.2022 11:35, Roger Pau Monné wrote:
> > On Thu, Mar 31, 2022 at 11:45:02AM +0200, Jan Beulich wrote:
> >> --- a/xen/arch/x86/boot/head.S
> >> +++ b/xen/arch/x86/boot/head.S
> >> @@ -562,12 +562,18 @@ trampoline_setup:
> >> mov %esi, sym_esi(xen_phys_start)
> >> mov %esi, sym_esi(trampoline_xen_phys_start)
> >>
> >> - mov sym_esi(trampoline_phys), %ecx
> >> -
> >> /* Get bottom-most low-memory stack address. */
> >> + mov sym_esi(trampoline_phys), %ecx
> >> add $TRAMPOLINE_SPACE,%ecx
> >
> > Just for my understanding, since you are already touching the
> > instruction, why not switch it to a lea like you do below?
> >
> > Is that because you would also like to take the opportunity to fold
> > the add into the lea and that would be too much of a change?
>
> No. This MOV cannot be converted, as its source operand isn't an
> immediate (or register); such a conversion would also be undesirable,
> for increasing insn size. See the later patch doing conversions in
> the other direction, to reduce code size. Somewhat similarly ...
>
> >> +#ifdef CONFIG_VIDEO
> >> + lea sym_esi(boot_vid_info), %edx
>
> ... this LEA also cannot be expressed by a single MOV.
>
> >> @@ -32,6 +33,39 @@ asm (
> >> #include "../../../include/xen/kconfig.h"
> >> #include <public/arch-x86/hvm/start_info.h>
> >>
> >> +#ifdef CONFIG_VIDEO
> >> +# include "video.h"
> >> +
> >> +/* VESA control information */
> >> +struct __packed vesa_ctrl_info {
> >> + uint8_t signature[4];
> >> + uint16_t version;
> >> + uint32_t oem_name;
> >> + uint32_t capabilities;
> >> + uint32_t mode_list;
> >> + uint16_t mem_size;
> >> + /* We don't use any further fields. */
> >> +};
> >> +
> >> +/* VESA 2.0 mode information */
> >> +struct vesa_mode_info {
> >
> > Should we add __packed here just in case further added fields are no
> > longer naturally aligned? (AFAICT all field right now are aligned to
> > it's size so there's no need for it).
>
> I think we should avoid __packed whenever possible.
>
> >> + uint16_t attrib;
> >> + uint8_t window[14]; /* We don't use the individual fields. */
> >> + uint16_t bytes_per_line;
> >> + uint16_t width;
> >> + uint16_t height;
> >> + uint8_t cell_width;
> >> + uint8_t cell_height;
> >> + uint8_t nr_planes;
> >> + uint8_t depth;
> >> + uint8_t memory[5]; /* We don't use the individual fields. */
> >> + struct boot_video_colors colors;
> >> + uint8_t direct_color;
> >> + uint32_t base;
> >> + /* We don't use any further fields. */
> >> +};
> >
> > Would it make sense to put those struct definitions in boot/video.h
> > like you do for boot_video_info?
>
> Personally I prefer to expose things in headers only when multiple
> other files want to consume what is being declared/defined.
>
> >> @@ -254,17 +291,64 @@ static multiboot_info_t *mbi2_reloc(u32
> >> ++mod_idx;
> >> break;
> >>
> >> +#ifdef CONFIG_VIDEO
> >> + case MULTIBOOT2_TAG_TYPE_VBE:
> >> + if ( video_out )
> >> + {
> >> + const struct vesa_ctrl_info *ci;
> >> + const struct vesa_mode_info *mi;
> >> +
> >> + video = _p(video_out);
> >> + ci = (void *)get_mb2_data(tag, vbe, vbe_control_info);
> >> + mi = (void *)get_mb2_data(tag, vbe, vbe_mode_info);
> >> +
> >> + if ( ci->version >= 0x0200 && (mi->attrib & 0x9b) == 0x9b )
> >> + {
> >> + video->capabilities = ci->capabilities;
> >> + video->lfb_linelength = mi->bytes_per_line;
> >> + video->lfb_width = mi->width;
> >> + video->lfb_height = mi->height;
> >> + video->lfb_depth = mi->depth;
> >> + video->lfb_base = mi->base;
> >> + video->lfb_size = ci->mem_size;
> >> + video->colors = mi->colors;
> >> + video->vesa_attrib = mi->attrib;
> >> + }
> >> +
> >> + video->vesapm.seg = get_mb2_data(tag, vbe, vbe_interface_seg);
> >> + video->vesapm.off = get_mb2_data(tag, vbe, vbe_interface_off);
> >> + }
> >> + break;
> >> +
> >> + case MULTIBOOT2_TAG_TYPE_FRAMEBUFFER:
> >> + if ( (get_mb2_data(tag, framebuffer, framebuffer_type) !=
> >> + MULTIBOOT2_FRAMEBUFFER_TYPE_RGB) )
> >> + {
> >> + video_out = 0;
> >> + video = NULL;
> >> + }
> >
> > I'm confused, don't you need to store the information in the
> > framebuffer tag for use after relocation?
>
> If there was a consumer - yes. Right now this tag is used only to
> invalidate the information taken from the other tag (or to suppress
> taking values from there if that other tag came later) in case the
> framebuffer type doesn't match what we support.
>
> >> + break;
> >> +#endif /* CONFIG_VIDEO */
> >> +
> >> case MULTIBOOT2_TAG_TYPE_END:
> >> - return mbi_out;
> >> + goto end; /* Cannot "break;" here. */
> >>
> >> default:
> >> break;
> >> }
> >>
> >> + end:
> >> +
> >> +#ifdef CONFIG_VIDEO
> >> + if ( video )
> >> + video->orig_video_isVGA = 0x23;
> >
> > I see we use this elsewhere, what's the meaning of this (magic) 0x23?
>
> This is a value Linux uses (also as a plain number without any #define
> iirc; at least it was that way when we first inherited that value).
> Short of knowing where they took it from or what meaning they associate
> with the value it would be hard for us to give this a (meaningful) name
> and hence use a #define. And hence it's equally hard to properly answer
> your question.
OK, so it's a magic value. I'm happy with the rest, so:
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
Thanks, Roger.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 3/8] x86/EFI: retrieve EDID
2022-04-05 10:27 ` Roger Pau Monné
@ 2022-04-05 14:36 ` Jan Beulich
2022-04-05 15:47 ` Roger Pau Monné
0 siblings, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2022-04-05 14:36 UTC (permalink / raw)
To: Roger Pau Monné
Cc: xen-devel, Andrew Cooper, Wei Liu, Julien Grall,
Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis
On 05.04.2022 12:27, Roger Pau Monné wrote:
> On Thu, Mar 31, 2022 at 11:45:36AM +0200, Jan Beulich wrote:
>> --- a/xen/arch/x86/efi/efi-boot.h
>> +++ b/xen/arch/x86/efi/efi-boot.h
>> @@ -568,6 +568,49 @@ static void __init efi_arch_video_init(E
>> #endif
>> }
>>
>> +#ifdef CONFIG_VIDEO
>> +static bool __init copy_edid(const void *buf, unsigned int size)
>> +{
>> + /*
>> + * Be conservative - for both undersized and oversized blobs it is unclear
>> + * what to actually do with them. The more that unlike the VESA BIOS
>> + * interface we also have no associated "capabilities" value (which might
>> + * carry a hint as to possible interpretation).
>> + */
>> + if ( size != ARRAY_SIZE(boot_edid_info) )
>> + return false;
>> +
>> + memcpy(boot_edid_info, buf, size);
>> + boot_edid_caps = 0;
>> +
>> + return true;
>> +}
>> +#endif
>> +
>> +static void __init efi_arch_edid(EFI_HANDLE gop_handle)
>> +{
>> +#ifdef CONFIG_VIDEO
>> + static EFI_GUID __initdata active_guid = EFI_EDID_ACTIVE_PROTOCOL_GUID;
>> + static EFI_GUID __initdata discovered_guid = EFI_EDID_DISCOVERED_PROTOCOL_GUID;
>
> Is there a need to make those static?
>
> I think this function is either called from efi_start or
> efi_multiboot, but there aren't multiple calls to it? (also both
> parameters are IN only, so not to be changed by the EFI method?
>
> I have the feeling setting them to static is done because they can't
> be set to const?
Even if they could be const, they ought to also be static. They don't
strictly need to be, but without "static" code will be generated to
populate the on-stack variables; quite possibly the compiler would
even allocate an unnamed static variable and memcpy() from there onto
the stack.
>> + EFI_EDID_ACTIVE_PROTOCOL *active_edid;
>> + EFI_EDID_DISCOVERED_PROTOCOL *discovered_edid;
>> + EFI_STATUS status;
>> +
>> + status = efi_bs->OpenProtocol(gop_handle, &active_guid,
>> + (void **)&active_edid, efi_ih, NULL,
>> + EFI_OPEN_PROTOCOL_GET_PROTOCOL);
>> + if ( status == EFI_SUCCESS &&
>> + copy_edid(active_edid->Edid, active_edid->SizeOfEdid) )
>> + return;
>
> Isn't it enough to just call EFI_EDID_ACTIVE_PROTOCOL_GUID?
>
> From my reading of the UEFI spec this will either return
> EFI_EDID_OVERRIDE_PROTOCOL_GUID or EFI_EDID_DISCOVERED_PROTOCOL_GUID.
> If EFI_EDID_OVERRIDE_PROTOCOL is set it must be used, and hence
> falling back to EFI_EDID_DISCOVERED_PROTOCOL_GUID if
> EFI_EDID_ACTIVE_PROTOCOL_GUID cannot be parsed would likely mean
> ignoring EFI_EDID_OVERRIDE_PROTOCOL?
That's the theory. As per one of the post-commit-message remarks I had
looked at what GrUB does, and I decided to follow its behavior in this
regard, assuming they do what they do to work around quirks. As said
in the remark, I didn't want to go as far as also cloning their use of
the undocumented (afaik) "agp-internal-edid" variable.
>> --- a/xen/include/efi/efiprot.h
>> +++ b/xen/include/efi/efiprot.h
>> @@ -724,5 +724,52 @@ struct _EFI_GRAPHICS_OUTPUT_PROTOCOL {
>> EFI_GRAPHICS_OUTPUT_PROTOCOL_BLT Blt;
>> EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE *Mode;
>> };
>> +
>> +/*
>> + * EFI EDID Discovered Protocol
>> + * UEFI Specification Version 2.5 Section 11.9
>> + */
>> +#define EFI_EDID_DISCOVERED_PROTOCOL_GUID \
>> + { 0x1C0C34F6, 0xD380, 0x41FA, { 0xA0, 0x49, 0x8a, 0xD0, 0x6C, 0x1A, 0x66, 0xAA} }
>> +
>> +typedef struct _EFI_EDID_DISCOVERED_PROTOCOL {
>> + UINT32 SizeOfEdid;
>> + UINT8 *Edid;
>> +} EFI_EDID_DISCOVERED_PROTOCOL;
>> +
>> +/*
>> + * EFI EDID Active Protocol
>> + * UEFI Specification Version 2.5 Section 11.9
>> + */
>> +#define EFI_EDID_ACTIVE_PROTOCOL_GUID \
>> + { 0xBD8C1056, 0x9F36, 0x44EC, { 0x92, 0xA8, 0xA6, 0x33, 0x7F, 0x81, 0x79, 0x86} }
>> +
>> +typedef struct _EFI_EDID_ACTIVE_PROTOCOL {
>> + UINT32 SizeOfEdid;
>> + UINT8 *Edid;
>> +} EFI_EDID_ACTIVE_PROTOCOL;
>> +
>> +/*
>> + * EFI EDID Override Protocol
>> + * UEFI Specification Version 2.5 Section 11.9
>> + */
>> +#define EFI_EDID_OVERRIDE_PROTOCOL_GUID \
>> + { 0x48ECB431, 0xFB72, 0x45C0, { 0xA9, 0x22, 0xF4, 0x58, 0xFE, 0x04, 0x0B, 0xD5} }
>> +
>> +INTERFACE_DECL(_EFI_EDID_OVERRIDE_PROTOCOL);
>> +
>> +typedef
>> +EFI_STATUS
>> +(EFIAPI *EFI_EDID_OVERRIDE_PROTOCOL_GET_EDID) (
>> + IN struct _EFI_EDID_OVERRIDE_PROTOCOL *This,
>> + IN EFI_HANDLE *ChildHandle,
>> + OUT UINT32 *Attributes,
>> + IN OUT UINTN *EdidSize,
>> + IN OUT UINT8 **Edid);
>> +
>> +typedef struct _EFI_EDID_OVERRIDE_PROTOCOL {
>> + EFI_EDID_OVERRIDE_PROTOCOL_GET_EDID GetEdid;
>> +} EFI_EDID_OVERRIDE_PROTOCOL;
>> +
>> #endif
>
> FWIW, EFI_EDID_OVERRIDE_PROTOCOL_GUID is not used by the patch, so I
> guess it's introduced for completeness (or because it's used by
> further patches).
Indeed (the former; there's no use in later patches).
Jan
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 3/8] x86/EFI: retrieve EDID
2022-04-05 14:36 ` Jan Beulich
@ 2022-04-05 15:47 ` Roger Pau Monné
2022-04-06 8:44 ` Jan Beulich
0 siblings, 1 reply; 38+ messages in thread
From: Roger Pau Monné @ 2022-04-05 15:47 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel, Andrew Cooper, Wei Liu, Julien Grall,
Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis
On Tue, Apr 05, 2022 at 04:36:53PM +0200, Jan Beulich wrote:
> On 05.04.2022 12:27, Roger Pau Monné wrote:
> > On Thu, Mar 31, 2022 at 11:45:36AM +0200, Jan Beulich wrote:
> >> --- a/xen/arch/x86/efi/efi-boot.h
> >> +++ b/xen/arch/x86/efi/efi-boot.h
> >> @@ -568,6 +568,49 @@ static void __init efi_arch_video_init(E
> >> #endif
> >> }
> >>
> >> +#ifdef CONFIG_VIDEO
> >> +static bool __init copy_edid(const void *buf, unsigned int size)
> >> +{
> >> + /*
> >> + * Be conservative - for both undersized and oversized blobs it is unclear
> >> + * what to actually do with them. The more that unlike the VESA BIOS
> >> + * interface we also have no associated "capabilities" value (which might
> >> + * carry a hint as to possible interpretation).
> >> + */
> >> + if ( size != ARRAY_SIZE(boot_edid_info) )
> >> + return false;
> >> +
> >> + memcpy(boot_edid_info, buf, size);
> >> + boot_edid_caps = 0;
> >> +
> >> + return true;
> >> +}
> >> +#endif
> >> +
> >> +static void __init efi_arch_edid(EFI_HANDLE gop_handle)
> >> +{
> >> +#ifdef CONFIG_VIDEO
> >> + static EFI_GUID __initdata active_guid = EFI_EDID_ACTIVE_PROTOCOL_GUID;
> >> + static EFI_GUID __initdata discovered_guid = EFI_EDID_DISCOVERED_PROTOCOL_GUID;
> >
> > Is there a need to make those static?
> >
> > I think this function is either called from efi_start or
> > efi_multiboot, but there aren't multiple calls to it? (also both
> > parameters are IN only, so not to be changed by the EFI method?
> >
> > I have the feeling setting them to static is done because they can't
> > be set to const?
>
> Even if they could be const, they ought to also be static. They don't
> strictly need to be, but without "static" code will be generated to
> populate the on-stack variables; quite possibly the compiler would
> even allocate an unnamed static variable and memcpy() from there onto
> the stack.
I thought that making those const (and then annotate with __initconst)
would already have the same effect as having it static, as there will
be no memcpy in that case either.
> >> + EFI_EDID_ACTIVE_PROTOCOL *active_edid;
> >> + EFI_EDID_DISCOVERED_PROTOCOL *discovered_edid;
> >> + EFI_STATUS status;
> >> +
> >> + status = efi_bs->OpenProtocol(gop_handle, &active_guid,
> >> + (void **)&active_edid, efi_ih, NULL,
> >> + EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> >> + if ( status == EFI_SUCCESS &&
> >> + copy_edid(active_edid->Edid, active_edid->SizeOfEdid) )
> >> + return;
> >
> > Isn't it enough to just call EFI_EDID_ACTIVE_PROTOCOL_GUID?
> >
> > From my reading of the UEFI spec this will either return
> > EFI_EDID_OVERRIDE_PROTOCOL_GUID or EFI_EDID_DISCOVERED_PROTOCOL_GUID.
> > If EFI_EDID_OVERRIDE_PROTOCOL is set it must be used, and hence
> > falling back to EFI_EDID_DISCOVERED_PROTOCOL_GUID if
> > EFI_EDID_ACTIVE_PROTOCOL_GUID cannot be parsed would likely mean
> > ignoring EFI_EDID_OVERRIDE_PROTOCOL?
>
> That's the theory. As per one of the post-commit-message remarks I had
> looked at what GrUB does, and I decided to follow its behavior in this
> regard, assuming they do what they do to work around quirks. As said
> in the remark, I didn't want to go as far as also cloning their use of
> the undocumented (afaik) "agp-internal-edid" variable.
Could you add this as a comment here? So it's not lost on commit as
being just a post-commit log remark. With that:
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
Thanks, Roger.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 3/8] x86/EFI: retrieve EDID
2022-04-05 15:47 ` Roger Pau Monné
@ 2022-04-06 8:44 ` Jan Beulich
2022-04-06 9:34 ` Roger Pau Monné
0 siblings, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2022-04-06 8:44 UTC (permalink / raw)
To: Roger Pau Monné
Cc: xen-devel, Andrew Cooper, Wei Liu, Julien Grall,
Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis
On 05.04.2022 17:47, Roger Pau Monné wrote:
> On Tue, Apr 05, 2022 at 04:36:53PM +0200, Jan Beulich wrote:
>> On 05.04.2022 12:27, Roger Pau Monné wrote:
>>> On Thu, Mar 31, 2022 at 11:45:36AM +0200, Jan Beulich wrote:
>>>> --- a/xen/arch/x86/efi/efi-boot.h
>>>> +++ b/xen/arch/x86/efi/efi-boot.h
>>>> @@ -568,6 +568,49 @@ static void __init efi_arch_video_init(E
>>>> #endif
>>>> }
>>>>
>>>> +#ifdef CONFIG_VIDEO
>>>> +static bool __init copy_edid(const void *buf, unsigned int size)
>>>> +{
>>>> + /*
>>>> + * Be conservative - for both undersized and oversized blobs it is unclear
>>>> + * what to actually do with them. The more that unlike the VESA BIOS
>>>> + * interface we also have no associated "capabilities" value (which might
>>>> + * carry a hint as to possible interpretation).
>>>> + */
>>>> + if ( size != ARRAY_SIZE(boot_edid_info) )
>>>> + return false;
>>>> +
>>>> + memcpy(boot_edid_info, buf, size);
>>>> + boot_edid_caps = 0;
>>>> +
>>>> + return true;
>>>> +}
>>>> +#endif
>>>> +
>>>> +static void __init efi_arch_edid(EFI_HANDLE gop_handle)
>>>> +{
>>>> +#ifdef CONFIG_VIDEO
>>>> + static EFI_GUID __initdata active_guid = EFI_EDID_ACTIVE_PROTOCOL_GUID;
>>>> + static EFI_GUID __initdata discovered_guid = EFI_EDID_DISCOVERED_PROTOCOL_GUID;
>>>
>>> Is there a need to make those static?
>>>
>>> I think this function is either called from efi_start or
>>> efi_multiboot, but there aren't multiple calls to it? (also both
>>> parameters are IN only, so not to be changed by the EFI method?
>>>
>>> I have the feeling setting them to static is done because they can't
>>> be set to const?
>>
>> Even if they could be const, they ought to also be static. They don't
>> strictly need to be, but without "static" code will be generated to
>> populate the on-stack variables; quite possibly the compiler would
>> even allocate an unnamed static variable and memcpy() from there onto
>> the stack.
>
> I thought that making those const (and then annotate with __initconst)
> would already have the same effect as having it static, as there will
> be no memcpy in that case either.
You cannot annotate non-static variables with __initconst.
>>>> + EFI_EDID_ACTIVE_PROTOCOL *active_edid;
>>>> + EFI_EDID_DISCOVERED_PROTOCOL *discovered_edid;
>>>> + EFI_STATUS status;
>>>> +
>>>> + status = efi_bs->OpenProtocol(gop_handle, &active_guid,
>>>> + (void **)&active_edid, efi_ih, NULL,
>>>> + EFI_OPEN_PROTOCOL_GET_PROTOCOL);
>>>> + if ( status == EFI_SUCCESS &&
>>>> + copy_edid(active_edid->Edid, active_edid->SizeOfEdid) )
>>>> + return;
>>>
>>> Isn't it enough to just call EFI_EDID_ACTIVE_PROTOCOL_GUID?
>>>
>>> From my reading of the UEFI spec this will either return
>>> EFI_EDID_OVERRIDE_PROTOCOL_GUID or EFI_EDID_DISCOVERED_PROTOCOL_GUID.
>>> If EFI_EDID_OVERRIDE_PROTOCOL is set it must be used, and hence
>>> falling back to EFI_EDID_DISCOVERED_PROTOCOL_GUID if
>>> EFI_EDID_ACTIVE_PROTOCOL_GUID cannot be parsed would likely mean
>>> ignoring EFI_EDID_OVERRIDE_PROTOCOL?
>>
>> That's the theory. As per one of the post-commit-message remarks I had
>> looked at what GrUB does, and I decided to follow its behavior in this
>> regard, assuming they do what they do to work around quirks. As said
>> in the remark, I didn't want to go as far as also cloning their use of
>> the undocumented (afaik) "agp-internal-edid" variable.
Actually it's a little different, as I realized while re-checking in
order to reply to your request below. While GrUB looks to use this
only "just in case", our use is actually to also cope with failure
in copy_edid(): In case the overridden EDID doesn't match the size
constraint (which is more strict than GrUB's), we would retry with
the "discovered" one in the hope that its size is okay.
> Could you add this as a comment here? So it's not lost on commit as
> being just a post-commit log remark.
As a result I'm unsure of the value of a comment here going beyond
what the comment in copy_edid() already says. For now I've added
/*
* In case an override is in place which doesn't fit copy_edid(), also try
* obtaining the discovered EDID in the hope that it's better than nothing.
*/
In turn ...
> With that:
>
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
... I'll wait with applying this (thanks) until we've reached
agreement.
Jan
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 3/8] x86/EFI: retrieve EDID
2022-04-06 8:44 ` Jan Beulich
@ 2022-04-06 9:34 ` Roger Pau Monné
2022-04-06 12:40 ` Jan Beulich
0 siblings, 1 reply; 38+ messages in thread
From: Roger Pau Monné @ 2022-04-06 9:34 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel, Andrew Cooper, Wei Liu, Julien Grall,
Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis
On Wed, Apr 06, 2022 at 10:44:12AM +0200, Jan Beulich wrote:
> On 05.04.2022 17:47, Roger Pau Monné wrote:
> > On Tue, Apr 05, 2022 at 04:36:53PM +0200, Jan Beulich wrote:
> >> On 05.04.2022 12:27, Roger Pau Monné wrote:
> >>> On Thu, Mar 31, 2022 at 11:45:36AM +0200, Jan Beulich wrote:
> >>>> --- a/xen/arch/x86/efi/efi-boot.h
> >>>> +++ b/xen/arch/x86/efi/efi-boot.h
> >>>> @@ -568,6 +568,49 @@ static void __init efi_arch_video_init(E
> >>>> #endif
> >>>> }
> >>>>
> >>>> +#ifdef CONFIG_VIDEO
> >>>> +static bool __init copy_edid(const void *buf, unsigned int size)
> >>>> +{
> >>>> + /*
> >>>> + * Be conservative - for both undersized and oversized blobs it is unclear
> >>>> + * what to actually do with them. The more that unlike the VESA BIOS
> >>>> + * interface we also have no associated "capabilities" value (which might
> >>>> + * carry a hint as to possible interpretation).
> >>>> + */
> >>>> + if ( size != ARRAY_SIZE(boot_edid_info) )
> >>>> + return false;
> >>>> +
> >>>> + memcpy(boot_edid_info, buf, size);
> >>>> + boot_edid_caps = 0;
> >>>> +
> >>>> + return true;
> >>>> +}
> >>>> +#endif
> >>>> +
> >>>> +static void __init efi_arch_edid(EFI_HANDLE gop_handle)
> >>>> +{
> >>>> +#ifdef CONFIG_VIDEO
> >>>> + static EFI_GUID __initdata active_guid = EFI_EDID_ACTIVE_PROTOCOL_GUID;
> >>>> + static EFI_GUID __initdata discovered_guid = EFI_EDID_DISCOVERED_PROTOCOL_GUID;
> >>>
> >>> Is there a need to make those static?
> >>>
> >>> I think this function is either called from efi_start or
> >>> efi_multiboot, but there aren't multiple calls to it? (also both
> >>> parameters are IN only, so not to be changed by the EFI method?
> >>>
> >>> I have the feeling setting them to static is done because they can't
> >>> be set to const?
> >>
> >> Even if they could be const, they ought to also be static. They don't
> >> strictly need to be, but without "static" code will be generated to
> >> populate the on-stack variables; quite possibly the compiler would
> >> even allocate an unnamed static variable and memcpy() from there onto
> >> the stack.
> >
> > I thought that making those const (and then annotate with __initconst)
> > would already have the same effect as having it static, as there will
> > be no memcpy in that case either.
>
> You cannot annotate non-static variables with __initconst.
Oh, I guess I've never realized.
> >>>> + EFI_EDID_ACTIVE_PROTOCOL *active_edid;
> >>>> + EFI_EDID_DISCOVERED_PROTOCOL *discovered_edid;
> >>>> + EFI_STATUS status;
> >>>> +
> >>>> + status = efi_bs->OpenProtocol(gop_handle, &active_guid,
> >>>> + (void **)&active_edid, efi_ih, NULL,
> >>>> + EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> >>>> + if ( status == EFI_SUCCESS &&
> >>>> + copy_edid(active_edid->Edid, active_edid->SizeOfEdid) )
> >>>> + return;
> >>>
> >>> Isn't it enough to just call EFI_EDID_ACTIVE_PROTOCOL_GUID?
> >>>
> >>> From my reading of the UEFI spec this will either return
> >>> EFI_EDID_OVERRIDE_PROTOCOL_GUID or EFI_EDID_DISCOVERED_PROTOCOL_GUID.
> >>> If EFI_EDID_OVERRIDE_PROTOCOL is set it must be used, and hence
> >>> falling back to EFI_EDID_DISCOVERED_PROTOCOL_GUID if
> >>> EFI_EDID_ACTIVE_PROTOCOL_GUID cannot be parsed would likely mean
> >>> ignoring EFI_EDID_OVERRIDE_PROTOCOL?
> >>
> >> That's the theory. As per one of the post-commit-message remarks I had
> >> looked at what GrUB does, and I decided to follow its behavior in this
> >> regard, assuming they do what they do to work around quirks. As said
> >> in the remark, I didn't want to go as far as also cloning their use of
> >> the undocumented (afaik) "agp-internal-edid" variable.
>
> Actually it's a little different, as I realized while re-checking in
> order to reply to your request below. While GrUB looks to use this
> only "just in case", our use is actually to also cope with failure
> in copy_edid(): In case the overridden EDID doesn't match the size
> constraint (which is more strict than GrUB's), we would retry with
> the "discovered" one in the hope that its size is okay.
Hm, the specification states in EFI_EDID_OVERRIDE_PROTOCOL.GetEdid that:
"Returns EDID values and attributes that the Video BIOS must use"
And since EFI_EDID_ACTIVE_PROTOCOL will return
EFI_EDID_OVERRIDE_PROTOCOL if present it makes me wonder whether it's
fine to resort to EFI_EDID_DISCOVERED_PROTOCOL if the problem is not
the call itself failing, but Xen failing to parse the result (because
of the usage of must in the sentence).
I think it's fine to resort to EFI_EDID_DISCOVERED_PROTOCOL if
EFI_EDID_ACTIVE_PROTOCOL fails, but it's likely not if the call
succeeds but it's Xen the one failing to parse the result.
> > Could you add this as a comment here? So it's not lost on commit as
> > being just a post-commit log remark.
>
> As a result I'm unsure of the value of a comment here going beyond
> what the comment in copy_edid() already says. For now I've added
>
> /*
> * In case an override is in place which doesn't fit copy_edid(), also try
> * obtaining the discovered EDID in the hope that it's better than nothing.
> */
I think the comment is fine, but as mentioned above I wonder whether
by failing to parse the EDID from EFI_EDID_ACTIVE_PROTOCOL and
resorting to EFI_EDID_DISCOVERED_PROTOCOL we could be screwing the
system more than by simply failing to get video output, hence I
think a more conservative approach might be to just use
EFI_EDID_DISCOVERED_PROTOCOL if EFI_EDID_ACTIVE_PROTOCOL fails.
As with firmware, this should mostly mimic what others do in order to
be on the safe side, so if GrUB does so we should likely follow suit.
Thanks, Roger.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 3/8] x86/EFI: retrieve EDID
2022-04-06 9:34 ` Roger Pau Monné
@ 2022-04-06 12:40 ` Jan Beulich
2022-04-06 14:14 ` Roger Pau Monné
0 siblings, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2022-04-06 12:40 UTC (permalink / raw)
To: Roger Pau Monné
Cc: xen-devel, Andrew Cooper, Wei Liu, Julien Grall,
Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis
On 06.04.2022 11:34, Roger Pau Monné wrote:
> On Wed, Apr 06, 2022 at 10:44:12AM +0200, Jan Beulich wrote:
>> On 05.04.2022 17:47, Roger Pau Monné wrote:
>>> On Tue, Apr 05, 2022 at 04:36:53PM +0200, Jan Beulich wrote:
>>>> On 05.04.2022 12:27, Roger Pau Monné wrote:
>>>>> On Thu, Mar 31, 2022 at 11:45:36AM +0200, Jan Beulich wrote:
>>>>>> + EFI_EDID_ACTIVE_PROTOCOL *active_edid;
>>>>>> + EFI_EDID_DISCOVERED_PROTOCOL *discovered_edid;
>>>>>> + EFI_STATUS status;
>>>>>> +
>>>>>> + status = efi_bs->OpenProtocol(gop_handle, &active_guid,
>>>>>> + (void **)&active_edid, efi_ih, NULL,
>>>>>> + EFI_OPEN_PROTOCOL_GET_PROTOCOL);
>>>>>> + if ( status == EFI_SUCCESS &&
>>>>>> + copy_edid(active_edid->Edid, active_edid->SizeOfEdid) )
>>>>>> + return;
>>>>>
>>>>> Isn't it enough to just call EFI_EDID_ACTIVE_PROTOCOL_GUID?
>>>>>
>>>>> From my reading of the UEFI spec this will either return
>>>>> EFI_EDID_OVERRIDE_PROTOCOL_GUID or EFI_EDID_DISCOVERED_PROTOCOL_GUID.
>>>>> If EFI_EDID_OVERRIDE_PROTOCOL is set it must be used, and hence
>>>>> falling back to EFI_EDID_DISCOVERED_PROTOCOL_GUID if
>>>>> EFI_EDID_ACTIVE_PROTOCOL_GUID cannot be parsed would likely mean
>>>>> ignoring EFI_EDID_OVERRIDE_PROTOCOL?
>>>>
>>>> That's the theory. As per one of the post-commit-message remarks I had
>>>> looked at what GrUB does, and I decided to follow its behavior in this
>>>> regard, assuming they do what they do to work around quirks. As said
>>>> in the remark, I didn't want to go as far as also cloning their use of
>>>> the undocumented (afaik) "agp-internal-edid" variable.
>>
>> Actually it's a little different, as I realized while re-checking in
>> order to reply to your request below. While GrUB looks to use this
>> only "just in case", our use is actually to also cope with failure
>> in copy_edid(): In case the overridden EDID doesn't match the size
>> constraint (which is more strict than GrUB's), we would retry with
>> the "discovered" one in the hope that its size is okay.
>
> Hm, the specification states in EFI_EDID_OVERRIDE_PROTOCOL.GetEdid that:
>
> "Returns EDID values and attributes that the Video BIOS must use"
I'm tempted to say "We're not the Video BIOS." ;-)
> And since EFI_EDID_ACTIVE_PROTOCOL will return
> EFI_EDID_OVERRIDE_PROTOCOL if present it makes me wonder whether it's
> fine to resort to EFI_EDID_DISCOVERED_PROTOCOL if the problem is not
> the call itself failing, but Xen failing to parse the result (because
> of the usage of must in the sentence).
>
> I think it's fine to resort to EFI_EDID_DISCOVERED_PROTOCOL if
> EFI_EDID_ACTIVE_PROTOCOL fails, but it's likely not if the call
> succeeds but it's Xen the one failing to parse the result.
>
>>> Could you add this as a comment here? So it's not lost on commit as
>>> being just a post-commit log remark.
>>
>> As a result I'm unsure of the value of a comment here going beyond
>> what the comment in copy_edid() already says. For now I've added
>>
>> /*
>> * In case an override is in place which doesn't fit copy_edid(), also try
>> * obtaining the discovered EDID in the hope that it's better than nothing.
>> */
>
> I think the comment is fine, but as mentioned above I wonder whether
> by failing to parse the EDID from EFI_EDID_ACTIVE_PROTOCOL and
> resorting to EFI_EDID_DISCOVERED_PROTOCOL we could be screwing the
> system more than by simply failing to get video output, hence I
> think a more conservative approach might be to just use
> EFI_EDID_DISCOVERED_PROTOCOL if EFI_EDID_ACTIVE_PROTOCOL fails.
>
> As with firmware, this should mostly mimic what others do in order to
> be on the safe side, so if GrUB does so we should likely follow suit.
But they're careless in other ways; I don't want to mimic that. I would
assume (or at least hope) that a discovered EDID still fits the system,
perhaps not as optimally as a subsequently installed override. But then
I also lack sufficient knowledge on all aspects which EDID may be
relevant for, so it's all guesswork anyway afaic.
Jan
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 3/8] x86/EFI: retrieve EDID
2022-04-06 12:40 ` Jan Beulich
@ 2022-04-06 14:14 ` Roger Pau Monné
2022-04-06 14:21 ` Jan Beulich
0 siblings, 1 reply; 38+ messages in thread
From: Roger Pau Monné @ 2022-04-06 14:14 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel, Andrew Cooper, Wei Liu, Julien Grall,
Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis
On Wed, Apr 06, 2022 at 02:40:50PM +0200, Jan Beulich wrote:
> On 06.04.2022 11:34, Roger Pau Monné wrote:
> > On Wed, Apr 06, 2022 at 10:44:12AM +0200, Jan Beulich wrote:
> >> On 05.04.2022 17:47, Roger Pau Monné wrote:
> >>> On Tue, Apr 05, 2022 at 04:36:53PM +0200, Jan Beulich wrote:
> >>>> On 05.04.2022 12:27, Roger Pau Monné wrote:
> >>>>> On Thu, Mar 31, 2022 at 11:45:36AM +0200, Jan Beulich wrote:
> >>>>>> + EFI_EDID_ACTIVE_PROTOCOL *active_edid;
> >>>>>> + EFI_EDID_DISCOVERED_PROTOCOL *discovered_edid;
> >>>>>> + EFI_STATUS status;
> >>>>>> +
> >>>>>> + status = efi_bs->OpenProtocol(gop_handle, &active_guid,
> >>>>>> + (void **)&active_edid, efi_ih, NULL,
> >>>>>> + EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> >>>>>> + if ( status == EFI_SUCCESS &&
> >>>>>> + copy_edid(active_edid->Edid, active_edid->SizeOfEdid) )
> >>>>>> + return;
> >>>>>
> >>>>> Isn't it enough to just call EFI_EDID_ACTIVE_PROTOCOL_GUID?
> >>>>>
> >>>>> From my reading of the UEFI spec this will either return
> >>>>> EFI_EDID_OVERRIDE_PROTOCOL_GUID or EFI_EDID_DISCOVERED_PROTOCOL_GUID.
> >>>>> If EFI_EDID_OVERRIDE_PROTOCOL is set it must be used, and hence
> >>>>> falling back to EFI_EDID_DISCOVERED_PROTOCOL_GUID if
> >>>>> EFI_EDID_ACTIVE_PROTOCOL_GUID cannot be parsed would likely mean
> >>>>> ignoring EFI_EDID_OVERRIDE_PROTOCOL?
> >>>>
> >>>> That's the theory. As per one of the post-commit-message remarks I had
> >>>> looked at what GrUB does, and I decided to follow its behavior in this
> >>>> regard, assuming they do what they do to work around quirks. As said
> >>>> in the remark, I didn't want to go as far as also cloning their use of
> >>>> the undocumented (afaik) "agp-internal-edid" variable.
> >>
> >> Actually it's a little different, as I realized while re-checking in
> >> order to reply to your request below. While GrUB looks to use this
> >> only "just in case", our use is actually to also cope with failure
> >> in copy_edid(): In case the overridden EDID doesn't match the size
> >> constraint (which is more strict than GrUB's), we would retry with
> >> the "discovered" one in the hope that its size is okay.
> >
> > Hm, the specification states in EFI_EDID_OVERRIDE_PROTOCOL.GetEdid that:
> >
> > "Returns EDID values and attributes that the Video BIOS must use"
>
> I'm tempted to say "We're not the Video BIOS." ;-)
I think UEFI expects this to be exclusively used by legacy Video BIOS
implementations but not OSes?
> > And since EFI_EDID_ACTIVE_PROTOCOL will return
> > EFI_EDID_OVERRIDE_PROTOCOL if present it makes me wonder whether it's
> > fine to resort to EFI_EDID_DISCOVERED_PROTOCOL if the problem is not
> > the call itself failing, but Xen failing to parse the result (because
> > of the usage of must in the sentence).
> >
> > I think it's fine to resort to EFI_EDID_DISCOVERED_PROTOCOL if
> > EFI_EDID_ACTIVE_PROTOCOL fails, but it's likely not if the call
> > succeeds but it's Xen the one failing to parse the result.
> >
> >>> Could you add this as a comment here? So it's not lost on commit as
> >>> being just a post-commit log remark.
> >>
> >> As a result I'm unsure of the value of a comment here going beyond
> >> what the comment in copy_edid() already says. For now I've added
> >>
> >> /*
> >> * In case an override is in place which doesn't fit copy_edid(), also try
> >> * obtaining the discovered EDID in the hope that it's better than nothing.
> >> */
> >
> > I think the comment is fine, but as mentioned above I wonder whether
> > by failing to parse the EDID from EFI_EDID_ACTIVE_PROTOCOL and
> > resorting to EFI_EDID_DISCOVERED_PROTOCOL we could be screwing the
> > system more than by simply failing to get video output, hence I
> > think a more conservative approach might be to just use
> > EFI_EDID_DISCOVERED_PROTOCOL if EFI_EDID_ACTIVE_PROTOCOL fails.
> >
> > As with firmware, this should mostly mimic what others do in order to
> > be on the safe side, so if GrUB does so we should likely follow suit.
>
> But they're careless in other ways; I don't want to mimic that. I would
> assume (or at least hope) that a discovered EDID still fits the system,
> perhaps not as optimally as a subsequently installed override. But then
> I also lack sufficient knowledge on all aspects which EDID may be
> relevant for, so it's all guesswork anyway afaic.
Yes, I'm afraid I don't have any more insight. I'm slightly concerned
about this, but I guess not so much as in to block the change:
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
I would word the comment as:
/*
* In case an override is in place which doesn't fit copy_edid(), also
* try obtaining the discovered EDID in the hope that it's better than
* nothing.
*
* Note that attempting to use the information in
* EFI_EDID_DISCOVERED_PROTOCOL when there's an override provided by
* EFI_EDID_ACTIVE_PROTOCOL could lead to issues.
*/
Thanks, Roger.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 3/8] x86/EFI: retrieve EDID
2022-04-06 14:14 ` Roger Pau Monné
@ 2022-04-06 14:21 ` Jan Beulich
0 siblings, 0 replies; 38+ messages in thread
From: Jan Beulich @ 2022-04-06 14:21 UTC (permalink / raw)
To: Roger Pau Monné
Cc: xen-devel, Andrew Cooper, Wei Liu, Julien Grall,
Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis
On 06.04.2022 16:14, Roger Pau Monné wrote:
> On Wed, Apr 06, 2022 at 02:40:50PM +0200, Jan Beulich wrote:
>> On 06.04.2022 11:34, Roger Pau Monné wrote:
>>> On Wed, Apr 06, 2022 at 10:44:12AM +0200, Jan Beulich wrote:
>>>> On 05.04.2022 17:47, Roger Pau Monné wrote:
>>>>> On Tue, Apr 05, 2022 at 04:36:53PM +0200, Jan Beulich wrote:
>>>>>> On 05.04.2022 12:27, Roger Pau Monné wrote:
>>>>>>> On Thu, Mar 31, 2022 at 11:45:36AM +0200, Jan Beulich wrote:
>>>>>>>> + EFI_EDID_ACTIVE_PROTOCOL *active_edid;
>>>>>>>> + EFI_EDID_DISCOVERED_PROTOCOL *discovered_edid;
>>>>>>>> + EFI_STATUS status;
>>>>>>>> +
>>>>>>>> + status = efi_bs->OpenProtocol(gop_handle, &active_guid,
>>>>>>>> + (void **)&active_edid, efi_ih, NULL,
>>>>>>>> + EFI_OPEN_PROTOCOL_GET_PROTOCOL);
>>>>>>>> + if ( status == EFI_SUCCESS &&
>>>>>>>> + copy_edid(active_edid->Edid, active_edid->SizeOfEdid) )
>>>>>>>> + return;
>>>>>>>
>>>>>>> Isn't it enough to just call EFI_EDID_ACTIVE_PROTOCOL_GUID?
>>>>>>>
>>>>>>> From my reading of the UEFI spec this will either return
>>>>>>> EFI_EDID_OVERRIDE_PROTOCOL_GUID or EFI_EDID_DISCOVERED_PROTOCOL_GUID.
>>>>>>> If EFI_EDID_OVERRIDE_PROTOCOL is set it must be used, and hence
>>>>>>> falling back to EFI_EDID_DISCOVERED_PROTOCOL_GUID if
>>>>>>> EFI_EDID_ACTIVE_PROTOCOL_GUID cannot be parsed would likely mean
>>>>>>> ignoring EFI_EDID_OVERRIDE_PROTOCOL?
>>>>>>
>>>>>> That's the theory. As per one of the post-commit-message remarks I had
>>>>>> looked at what GrUB does, and I decided to follow its behavior in this
>>>>>> regard, assuming they do what they do to work around quirks. As said
>>>>>> in the remark, I didn't want to go as far as also cloning their use of
>>>>>> the undocumented (afaik) "agp-internal-edid" variable.
>>>>
>>>> Actually it's a little different, as I realized while re-checking in
>>>> order to reply to your request below. While GrUB looks to use this
>>>> only "just in case", our use is actually to also cope with failure
>>>> in copy_edid(): In case the overridden EDID doesn't match the size
>>>> constraint (which is more strict than GrUB's), we would retry with
>>>> the "discovered" one in the hope that its size is okay.
>>>
>>> Hm, the specification states in EFI_EDID_OVERRIDE_PROTOCOL.GetEdid that:
>>>
>>> "Returns EDID values and attributes that the Video BIOS must use"
>>
>> I'm tempted to say "We're not the Video BIOS." ;-)
>
> I think UEFI expects this to be exclusively used by legacy Video BIOS
> implementations but not OSes?
>
>>> And since EFI_EDID_ACTIVE_PROTOCOL will return
>>> EFI_EDID_OVERRIDE_PROTOCOL if present it makes me wonder whether it's
>>> fine to resort to EFI_EDID_DISCOVERED_PROTOCOL if the problem is not
>>> the call itself failing, but Xen failing to parse the result (because
>>> of the usage of must in the sentence).
>>>
>>> I think it's fine to resort to EFI_EDID_DISCOVERED_PROTOCOL if
>>> EFI_EDID_ACTIVE_PROTOCOL fails, but it's likely not if the call
>>> succeeds but it's Xen the one failing to parse the result.
>>>
>>>>> Could you add this as a comment here? So it's not lost on commit as
>>>>> being just a post-commit log remark.
>>>>
>>>> As a result I'm unsure of the value of a comment here going beyond
>>>> what the comment in copy_edid() already says. For now I've added
>>>>
>>>> /*
>>>> * In case an override is in place which doesn't fit copy_edid(), also try
>>>> * obtaining the discovered EDID in the hope that it's better than nothing.
>>>> */
>>>
>>> I think the comment is fine, but as mentioned above I wonder whether
>>> by failing to parse the EDID from EFI_EDID_ACTIVE_PROTOCOL and
>>> resorting to EFI_EDID_DISCOVERED_PROTOCOL we could be screwing the
>>> system more than by simply failing to get video output, hence I
>>> think a more conservative approach might be to just use
>>> EFI_EDID_DISCOVERED_PROTOCOL if EFI_EDID_ACTIVE_PROTOCOL fails.
>>>
>>> As with firmware, this should mostly mimic what others do in order to
>>> be on the safe side, so if GrUB does so we should likely follow suit.
>>
>> But they're careless in other ways; I don't want to mimic that. I would
>> assume (or at least hope) that a discovered EDID still fits the system,
>> perhaps not as optimally as a subsequently installed override. But then
>> I also lack sufficient knowledge on all aspects which EDID may be
>> relevant for, so it's all guesswork anyway afaic.
>
> Yes, I'm afraid I don't have any more insight. I'm slightly concerned
> about this, but I guess not so much as in to block the change:
>
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
Thanks.
> I would word the comment as:
>
> /*
> * In case an override is in place which doesn't fit copy_edid(), also
> * try obtaining the discovered EDID in the hope that it's better than
> * nothing.
> *
> * Note that attempting to use the information in
> * EFI_EDID_DISCOVERED_PROTOCOL when there's an override provided by
> * EFI_EDID_ACTIVE_PROTOCOL could lead to issues.
> */
I've extended it like this.
Jan
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 1/8] x86/boot: make "vga=current" work with graphics modes
2022-04-05 8:45 ` Roger Pau Monné
@ 2022-04-06 14:23 ` Jan Beulich
2022-04-11 9:50 ` Ping: " Jan Beulich
0 siblings, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2022-04-06 14:23 UTC (permalink / raw)
To: Henry Wang
Cc: xen-devel, Andrew Cooper, Wei Liu, community.manager,
Roger Pau Monné
On 05.04.2022 10:45, Roger Pau Monné wrote:
> On Thu, Mar 31, 2022 at 11:44:10AM +0200, Jan Beulich wrote:
>> GrUB2 can be told to leave the screen in the graphics mode it has been
>> using (or any other one), via "set gfxpayload=keep" (or suitable
>> variants thereof). In this case we can avoid doing another mode switch
>> ourselves. This in particular avoids possibly setting the screen to a
>> less desirable mode: On one of my test systems the set of modes
>> reported available by the VESA BIOS depends on whether the interposed
>> KVM switch has that machine set as the active one. If it's not active,
>> only modes up to 1024x768 get reported, while when active 1280x1024
>> modes are also included. For things to always work with an explicitly
>> specified mode (via the "vga=" option), that mode therefore needs be a
>> 1024x768 one.
>>
>> For some reason this only works for me with "multiboot2" (and
>> "module2"); "multiboot" (and "module") still forces the screen into text
>> mode, despite my reading of the sources suggesting otherwise.
>>
>> For starters I'm limiting this to graphics modes; I do think this ought
>> to also work for text modes, but
>> - I can't tell whether GrUB2 can set any text mode other than 80x25
>> (I've only found plain "text" to be valid as a "gfxpayload" setting),
>> - I'm uncertain whether supporting that is worth it, since I'm uncertain
>> how many people would be running their systems/screens in text mode,
>> - I'd like to limit the amount of code added to the realmode trampoline.
>>
>> For starters I'm also limiting mode information retrieval to raw BIOS
>> accesses. This will allow things to work (in principle) also with other
>> boot environments where a graphics mode can be left in place. The
>> downside is that this then still is dependent upon switching back to
>> real mode, so retrieving the needed information from multiboot info is
>> likely going to be desirable down the road.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
May I ask for an ack or otherwise for the changelog entry, please?
Thanks, Jan
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 3/8] x86/EFI: retrieve EDID
2022-03-31 9:45 ` [PATCH v4 3/8] x86/EFI: retrieve EDID Jan Beulich
2022-04-01 10:15 ` Luca Fancellu
2022-04-05 10:27 ` Roger Pau Monné
@ 2022-04-06 14:24 ` Jan Beulich
2022-04-06 14:34 ` Bertrand Marquis
3 siblings, 0 replies; 38+ messages in thread
From: Jan Beulich @ 2022-04-06 14:24 UTC (permalink / raw)
To: Julien Grall, Stefano Stabellini, Bertrand Marquis
Cc: Andrew Cooper, Wei Liu, Roger Pau Monné,
Volodymyr Babchuk, xen-devel
On 31.03.2022 11:45, Jan Beulich wrote:
> When booting directly from EFI, obtaining this information from EFI is
> the only possible way. And even when booting with a boot loader
> interposed, it's more clean not to use legacy BIOS calls for this
> purpose. (The downside being that there are no "capabilities" that we
> can retrieve the EFI way.)
>
> To achieve this we need to propagate the handle used to obtain the
> EFI_GRAPHICS_OUTPUT_PROTOCOL instance for further obtaining an
> EFI_EDID_*_PROTOCOL instance, which has been part of the spec since 2.5.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Setting boot_edid_caps to zero isn't desirable, but arbitrarily setting
> one or both of the two low bits also doesn't seem appropriate.
>
> GrUB also checks an "agp-internal-edid" variable. As I haven't been able
> to find any related documentation, and as GrUB being happy about the
> variable being any size (rather than at least / precisely 128 bytes),
> I didn't follow that route.
> ---
> v3: Re-base.
> v2: New.
>
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -464,6 +464,10 @@ static void __init efi_arch_edd(void)
> {
> }
>
> +static void __init efi_arch_edid(EFI_HANDLE gop_handle)
> +{
> +}
> +
> static void __init efi_arch_memory_setup(void)
> {
> }
May I ask for an Arm side ack here, please?
Thanks, Jan
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 3/8] x86/EFI: retrieve EDID
2022-03-31 9:45 ` [PATCH v4 3/8] x86/EFI: retrieve EDID Jan Beulich
` (2 preceding siblings ...)
2022-04-06 14:24 ` Jan Beulich
@ 2022-04-06 14:34 ` Bertrand Marquis
3 siblings, 0 replies; 38+ messages in thread
From: Bertrand Marquis @ 2022-04-06 14:34 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel, Andrew Cooper, Wei Liu, Roger Pau Monné,
Julien Grall, Stefano Stabellini, Volodymyr Babchuk
Hi Jan,
> On 31 Mar 2022, at 10:45, Jan Beulich <jbeulich@suse.com> wrote:
>
> When booting directly from EFI, obtaining this information from EFI is
> the only possible way. And even when booting with a boot loader
> interposed, it's more clean not to use legacy BIOS calls for this
> purpose. (The downside being that there are no "capabilities" that we
> can retrieve the EFI way.)
>
> To achieve this we need to propagate the handle used to obtain the
> EFI_GRAPHICS_OUTPUT_PROTOCOL instance for further obtaining an
> EFI_EDID_*_PROTOCOL instance, which has been part of the spec since 2.5.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Bertrand Marquis <bertrand.marquis@arm.com> #arm
Cheers
Bertrand
^ permalink raw reply [flat|nested] 38+ messages in thread
* Ping: [PATCH v4 1/8] x86/boot: make "vga=current" work with graphics modes
2022-04-06 14:23 ` Jan Beulich
@ 2022-04-11 9:50 ` Jan Beulich
2022-04-11 10:11 ` Henry Wang
0 siblings, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2022-04-11 9:50 UTC (permalink / raw)
To: Henry Wang
Cc: xen-devel, Andrew Cooper, Wei Liu, community.manager,
Roger Pau Monné
On 06.04.2022 16:23, Jan Beulich wrote:
> On 05.04.2022 10:45, Roger Pau Monné wrote:
>> On Thu, Mar 31, 2022 at 11:44:10AM +0200, Jan Beulich wrote:
>>> GrUB2 can be told to leave the screen in the graphics mode it has been
>>> using (or any other one), via "set gfxpayload=keep" (or suitable
>>> variants thereof). In this case we can avoid doing another mode switch
>>> ourselves. This in particular avoids possibly setting the screen to a
>>> less desirable mode: On one of my test systems the set of modes
>>> reported available by the VESA BIOS depends on whether the interposed
>>> KVM switch has that machine set as the active one. If it's not active,
>>> only modes up to 1024x768 get reported, while when active 1280x1024
>>> modes are also included. For things to always work with an explicitly
>>> specified mode (via the "vga=" option), that mode therefore needs be a
>>> 1024x768 one.
>>>
>>> For some reason this only works for me with "multiboot2" (and
>>> "module2"); "multiboot" (and "module") still forces the screen into text
>>> mode, despite my reading of the sources suggesting otherwise.
>>>
>>> For starters I'm limiting this to graphics modes; I do think this ought
>>> to also work for text modes, but
>>> - I can't tell whether GrUB2 can set any text mode other than 80x25
>>> (I've only found plain "text" to be valid as a "gfxpayload" setting),
>>> - I'm uncertain whether supporting that is worth it, since I'm uncertain
>>> how many people would be running their systems/screens in text mode,
>>> - I'd like to limit the amount of code added to the realmode trampoline.
>>>
>>> For starters I'm also limiting mode information retrieval to raw BIOS
>>> accesses. This will allow things to work (in principle) also with other
>>> boot environments where a graphics mode can be left in place. The
>>> downside is that this then still is dependent upon switching back to
>>> real mode, so retrieving the needed information from multiboot info is
>>> likely going to be desirable down the road.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
>
> May I ask for an ack or otherwise for the changelog entry, please?
Ping? This is the only thing missing for me to commit the remaining
parts of this series.
Thanks, Jan
^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: Ping: [PATCH v4 1/8] x86/boot: make "vga=current" work with graphics modes
2022-04-11 9:50 ` Ping: " Jan Beulich
@ 2022-04-11 10:11 ` Henry Wang
2022-04-11 10:14 ` Jan Beulich
0 siblings, 1 reply; 38+ messages in thread
From: Henry Wang @ 2022-04-11 10:11 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel, Andrew Cooper, Wei Liu, community.manager,
Roger Pau Monné
Hi Jan,
> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>
> >> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> >
> > May I ask for an ack or otherwise for the changelog entry, please?
>
> Ping? This is the only thing missing for me to commit the remaining
> parts of this series.
Sorry for the late response, the previous e-mail that you directly
"to"ed me fell through the cracks somehow.
Acked-by: Henry Wang <Henry.Wang@arm.com>
Since you also mentioned the changelog entry, I will take a note of this
series and we can have a discussion about adding it when we do the
next Xen release (4.17). Would that sound ok with you?
Kind regards,
Henry
>
> Thanks, Jan
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: Ping: [PATCH v4 1/8] x86/boot: make "vga=current" work with graphics modes
2022-04-11 10:11 ` Henry Wang
@ 2022-04-11 10:14 ` Jan Beulich
2022-04-11 10:30 ` Henry Wang
0 siblings, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2022-04-11 10:14 UTC (permalink / raw)
To: Henry Wang
Cc: xen-devel, Andrew Cooper, Wei Liu, community.manager,
Roger Pau Monné
On 11.04.2022 12:11, Henry Wang wrote:
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
>>>
>>> May I ask for an ack or otherwise for the changelog entry, please?
>>
>> Ping? This is the only thing missing for me to commit the remaining
>> parts of this series.
>
> Sorry for the late response, the previous e-mail that you directly
> "to"ed me fell through the cracks somehow.
>
> Acked-by: Henry Wang <Henry.Wang@arm.com>
Thanks.
> Since you also mentioned the changelog entry, I will take a note of this
> series and we can have a discussion about adding it when we do the
> next Xen release (4.17). Would that sound ok with you?
"Adding it" where? Maybe you mean to the release notes, but that's not
entirely clear.
Jan
^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: Ping: [PATCH v4 1/8] x86/boot: make "vga=current" work with graphics modes
2022-04-11 10:14 ` Jan Beulich
@ 2022-04-11 10:30 ` Henry Wang
0 siblings, 0 replies; 38+ messages in thread
From: Henry Wang @ 2022-04-11 10:30 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel, Andrew Cooper, Wei Liu, community.manager,
Roger Pau Monné
Hi Jan,
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> > Since you also mentioned the changelog entry, I will take a note of this
> > series and we can have a discussion about adding it when we do the
> > next Xen release (4.17). Would that sound ok with you?
>
> "Adding it" where? Maybe you mean to the release notes, but that's not
> entirely clear.
Yeah, I meant the release notes. Thanks for pointing out that I did not use
accurate wording.
Kind regards,
Henry
>
> Jan
^ permalink raw reply [flat|nested] 38+ messages in thread
end of thread, other threads:[~2022-04-11 10:30 UTC | newest]
Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-31 9:42 [PATCH v4 0/8] x86/boot: (mostly) video mode handling adjustments Jan Beulich
2022-03-31 9:44 ` [PATCH v4 1/8] x86/boot: make "vga=current" work with graphics modes Jan Beulich
2022-04-04 14:49 ` Roger Pau Monné
2022-04-04 15:50 ` Jan Beulich
2022-04-05 8:24 ` Roger Pau Monné
2022-04-05 8:36 ` Jan Beulich
2022-04-05 8:45 ` Roger Pau Monné
2022-04-06 14:23 ` Jan Beulich
2022-04-11 9:50 ` Ping: " Jan Beulich
2022-04-11 10:11 ` Henry Wang
2022-04-11 10:14 ` Jan Beulich
2022-04-11 10:30 ` Henry Wang
2022-03-31 9:45 ` [PATCH v4 2/8] x86/boot: obtain video info from boot loader Jan Beulich
2022-04-05 9:35 ` Roger Pau Monné
2022-04-05 10:57 ` Jan Beulich
2022-04-05 11:34 ` Roger Pau Monné
2022-03-31 9:45 ` [PATCH v4 3/8] x86/EFI: retrieve EDID Jan Beulich
2022-04-01 10:15 ` Luca Fancellu
2022-04-05 10:27 ` Roger Pau Monné
2022-04-05 14:36 ` Jan Beulich
2022-04-05 15:47 ` Roger Pau Monné
2022-04-06 8:44 ` Jan Beulich
2022-04-06 9:34 ` Roger Pau Monné
2022-04-06 12:40 ` Jan Beulich
2022-04-06 14:14 ` Roger Pau Monné
2022-04-06 14:21 ` Jan Beulich
2022-04-06 14:24 ` Jan Beulich
2022-04-06 14:34 ` Bertrand Marquis
2022-03-31 9:48 ` [PATCH v4 4/8] x86/boot: simplify mode_table Jan Beulich
2022-04-05 10:44 ` Roger Pau Monné
2022-03-31 9:49 ` [PATCH v4 5/8] x86/boot: fold branches in video handling code Jan Beulich
2022-04-05 10:55 ` Roger Pau Monné
2022-03-31 9:50 ` [PATCH v4 6/8] x86/boot: fold/replace moves " Jan Beulich
2022-04-05 11:13 ` Roger Pau Monné
2022-03-31 9:50 ` [PATCH v4 7/8] x86/boot: LEA -> MOV " Jan Beulich
2022-04-05 11:28 ` Roger Pau Monné
2022-03-31 9:51 ` [PATCH v4 8/8] x86/boot: fold two MOVs into an ADD Jan Beulich
2022-04-05 11:29 ` Roger Pau Monné
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.