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

[Wei Chen <Wei.Chen@arm.com>]

Hi Jan,

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?

>> --- /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.
I will merge patch3 with it, indicating that this movement is to
share the EFI stub for Arm.

Thanks,
Wei Che

> Jan
>