| Message ID | c11d267f-f6b7-558f-18ce-2b081ae12427@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/boot: (mostly) video mode handling adjustments | expand |
> 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
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.
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
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.
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
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.
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
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.
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
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
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
--- 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
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.