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

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 2022年4月26日 22:31
> To: Wei Chen <Wei.Chen@xxxxxxx>
> Cc: nd <nd@xxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; Roger Pau
> Monné <roger.pau@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> 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