Re: [PATCH v4 3/8] x86/EFI: retrieve EDID

["Roger Pau Monné" <roger.pau@citrix.com>]

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.