Re: [PATCH v2 02/10] xen/x86: move reusable EFI stub functions from x86 to common

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.04.2022 12:37, Wei Chen wrote:
> On 2022/4/26 16:53, Jan Beulich wrote:
>> On 18.04.2022 11:07, Wei Chen wrote:
>>> diff --git a/xen/arch/x86/efi/stub.c b/xen/arch/x86/efi/stub-x86.c
>>> similarity index 71%
>>> rename from xen/arch/x86/efi/stub.c
>>> rename to xen/arch/x86/efi/stub-x86.c
>>> index 9984932626..2cd5c8d4dc 100644
>>> --- a/xen/arch/x86/efi/stub.c
>>> +++ b/xen/arch/x86/efi/stub-x86.c
>>
>> I'm not happy to see a file named *x86*.[ch] under x86/. I think the
>> x86 file wants to simply include the common one (and the symlinking
>> be suppressed when a real file already exists). Naming the common
>> file stub-common.c wouldn't help, as a similar anomaly would result.
>>
> 
> How about using stub-arch.c to indicate this stub file only contains
> the arch specific contents? However, we cannot predict what link files 
> will be created in this directory in the future. If someone needs to
> create a stub-arch.c link file in the future, can we solve it at that
> time?  Or do you have any suggestions?

I did provide my suggestion. I do not like stub-arch.c any better than
stub-x86.c or stub-common.c.

>>> --- /dev/null
>>> +++ b/xen/common/efi/stub.c
>>> @@ -0,0 +1,38 @@
>>> +#include <xen/efi.h>
>>> +#include <xen/errno.h>
>>> +#include <xen/lib.h>
>>> +
>>> +bool efi_enabled(unsigned int feature)
>>> +{
>>> +    return false;
>>> +}
>>> +
>>> +bool efi_rs_using_pgtables(void)
>>> +{
>>> +    return false;
>>> +}
>>> +
>>> +unsigned long efi_get_time(void)
>>> +{
>>> +    BUG();
>>> +    return 0;
>>> +}
>>> +
>>> +void efi_halt_system(void) { }
>>> +void efi_reset_system(bool warm) { }
>>> +
>>> +int efi_get_info(uint32_t idx, union xenpf_efi_info *info)
>>> +{
>>> +    return -ENOSYS;
>>> +}
>>> +
>>> +int efi_compat_get_info(uint32_t idx, union compat_pf_efi_info *)
>>> +    __attribute__((__alias__("efi_get_info")));
>>
>> I doubt you need this outside of x86.
>>
>>> +int efi_runtime_call(struct xenpf_efi_runtime_call *op)
>>> +{
>>> +    return -ENOSYS;
>>> +}
>>> +
>>> +int efi_compat_runtime_call(struct compat_pf_efi_runtime_call *)
>>> +    __attribute__((__alias__("efi_runtime_call")));
>>
>> Same here.
>>
> 
> You're correct, I check the code, Arm doesn't need above two
> compat functions. I will restore them to x86 specific file.
> 
>> Even for the non-compat variants the need is un-obvious: Are you
>> intending to wire these up anywhere in Arm or common code? This
>> of course is once again pointing out that such code movement would
>> better be done when the new consumers actually appear, such that
>> it's clear why the movement is done - for every individual item.
>>
> 
> Yes, but I didn't deliberately ignore your comment from the last
> series. I also hesitated for a while when constructing this patch.
> I felt that this independent work, maybe it would be better to use
> an independent patch.

Well, it of course depends on further aspects. If it had been clear
that what is moved is actually going to be wired up, this being a
standalone patch would be okay-ish. But with this unclear (and, as
per above, actually having gone too far) it's imo better to move
things as they get re-used.

Jan