Hi Jan,
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: 2022年4月26日 22:31
> To: Wei Chen <wei.c...@arm.com>
> Cc: nd <n...@arm.com>; Andrew Cooper <andrew.coop...@citrix.com>; Roger Pau
> Monné <roger....@citrix.com>; Wei Liu <w...@xen.org>; xen-
> de...@lists.xenproject.org
> Subject: 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.
>
With my limited English level, I can only see that you don't like this, but
I can't get what you want clearly from your comments. I can only guess:
For "x86 file wants to simply include the common one":
1. Did you mean, x86 still keeps it stub.c and includes all its original
contents. The common/efi/stub.c link behavior will be ignored, because
of x86 has a real stub.c? And common/efi/stub.c still can works for
other architectures like Arm whom doesn't have a real stub.c?
But in previous version's discussion, I had said I created a stub.c in
Arm/efi, and copied Arm required functions from x86/efi/stub.c. But
people didn't like it. If my guess is correct, I don't know what is
the essential difference between the two approaches.
2. Keeps stub.c in x86/efi, and use it to include common/stub.c.
I think this may not be the right understanding, but I can't think
of any other understanding.
And please forgive my limited reading level again!
> >>> --- /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.
>
Ok, I understand it now.
Thanks,
Wei Chen
> Jan
