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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.