Re: [PATCH v4 2/3] arm/efi: Use dom0less configuration when using EFI boot

[Luca Fancellu <luca.fancellu@xxxxxxx>]

> On 7 Oct 2021, at 08:15, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 01.10.2021 17:13, Luca Fancellu wrote:
>> 
>> 
>>> On 1 Oct 2021, at 15:22, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>> 
>>> On 01.10.2021 15:55, Luca Fancellu wrote:
>>>>> On 1 Oct 2021, at 12:02, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>>> On 30.09.2021 16:28, Luca Fancellu wrote:
>>>>>> @@ -1361,12 +1361,30 @@ efi_start(EFI_HANDLE ImageHandle, 
>>>>>> EFI_SYSTEM_TABLE *SystemTable)
>>>>>>       efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
>>>>>>       cfg.addr = 0;
>>>>>> 
>>>>>> -        dir_handle->Close(dir_handle);
>>>>>> -
>>>>>>       if ( gop && !base_video )
>>>>>>           gop_mode = efi_find_gop_mode(gop, cols, rows, depth);
>>>>>>   }
>>>>>> 
>>>>>> +#ifdef CONFIG_HAS_DEVICE_TREE
>>>>>> +    /* Get the number of boot modules specified on the DT or an error 
>>>>>> (<0) */
>>>>>> +    dt_modules_found = efi_arch_check_dt_boot(dir_handle);
>>>>>> +#endif
>>>>> 
>>>>> So I had asked to add a stub enclosed in such an #ifdef, to avoid the
>>>>> #ifdef here. I may be willing to let you keep things as you have them
>>>>> now, but I'd like to understand why you've picked that different
>>>>> approach despite the prior discussion.
>>>> 
>>>> There must be a misunderstanding, your message in the v3 was:
>>>> 
>>>> "Every time I see this addition I'm getting puzzled. As a result I'm
>>>> afraid I now need to finally ask you to do something about this (and
>>>> I'm sorry for doing so only now). There would better be no notion of
>>>> DT in x86 code, and there would better also not be a need for
>>>> architectures not supporting DT to each supply such a stub. Instead
>>>> I think you want to put this stub in xen/common/efi/boot.c, inside a
>>>> suitable #ifdef.”
>>>> 
>>>> So I thought you wanted me to remove the stub in x86 (since it doesn’t 
>>>> support DT)
>>>> and put this call under #ifdef so it won’t be compiled for arch not 
>>>> supporting DT.
>>> 
>>> So FTAOD I'll repeat the crucial part: "I think you want to put this
>>> stub in xen/common/efi/boot.c". There was nothing about removing the
>>> stub altogether.
>> 
>> Oh ok, now I see, so in your opinion this is a better idea:
>> 
>> #ifndef CONFIG_HAS_DEVICE_TREE
>> static inline int __init efi_arch_check_dt_boot(EFI_FILE_HANDLE dir_handle)
>> {
>>    return 0;
>> }
>> #endif
>> 
>> But I would like to understand the advantage respect of my approach, could 
>> you
>> explain me?
> 
> Well, to a degree it's a matter of taste. Your approach may lead to a long
> series of various #ifdef sections in a single function, harming readability.
> Having stubs instead (usually placed in headers, albeit not in this case)
> allows the main bodies of code to remain more tidy.

Yes right, in this case I did in another way because declaring the stub in the 
.c file
was (in my opinion) not the right thing to do, since also the name (efi_arch_*) 
recalls
something arch oriented and so not to be put in the common code.

In this way any future architecture supporting DT, can just provide the 
function (or a 
stub) and we don’t have stubs in architectures that won’t ever support DT.

In your opinion that solution could be acceptable?

Cheers,
Luca

> 
> Jan