[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 02/10] xen/x86: move reusable EFI stub functions from x86 to common
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |