Re: [XEN PATCH] build: Fix x86 build without EFI

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 16/08/2022 17:29, Jan Beulich wrote:
> On 16.08.2022 17:43, Anthony PERARD wrote:
> > On Tue, Aug 16, 2022 at 03:02:10PM +0200, Jan Beulich wrote:
> > > On 16.08.2022 12:30, Anthony PERARD wrote:
> > > > We can't have a source file with the same name that exist in both the
> > > > common code and in the arch specific code for efi/. This can lead to
> > > > comfusion in make and it can pick up the wrong source file. This issue
> > > > lead to a failure to build a pv-shim for x86 out-of-tree, as this is
> > > > one example of an x86 build using the efi/stub.c.
> > > >
> > > > The issue is that in out-of-tree, make might find x86/efi/stub.c via
> > > > VPATH, but as the target needs to be rebuilt due to FORCE, make
> > > > actually avoid changing the source tree and rebuilt the target with
> > > > VPATH ignored, so $@ lead to the build tree where "stub.c" dosen't
> > > > exist yet so a link is made to "common/stub.c".
> > > >
> > > > Rework the new common/stub.c file to have a different name than the
> > > > already existing one. And build both *stub.c as two different objects.
> > > > This mean we have to move some efi_compat_* aliases which are probably
> > > > useless for Arm.
> > > These useless aliases want avoiding there imo. Already when the original
> > > series was discussed, I requested to avoid introduction of a file named
> > > common-stub.c or alike.
> > Yeah, I've notice that. This is why the build is broken under
> > specific condition.
> >
> > > If names need to be different, can't we follow
> > > boot.c's model and introduce a per-arch efi/stub.h which stub.c would
> > > include at a suitable position (and which right now would be empty for
> > > Arm)?
> > That seems to be possible. But how is it better than having two
> > different source file? The only thing is to avoid exporting the
> > efi_compat_* symbol aliases.
> As said - I think they're wrong to have in Arm. But if Arm maintainers
> don't care about them being there, so be it. As long as they don't
> voice a view, I guess as the EFI maintainer I can sensibly ask for
> them to be avoided in a reasonably clean way.
AFAIU, the two aliases are using by the compat code. So how about 
protecting it with CONFIG_COMPAT (like we do for other compat code in 
common code)?
> > The downside is we would have another weird
> > looking not really header which is actually just part of a source file.
> > At least, "stub.c" and "stub.h" would be two different names, we just
> > change the extension rather than the basename.
> Whether that's "weird" is certainly a matter of taste ... To me,
> common-stub.c also comes close  to "weird", fwiw. But as I've tried
> to express, if I'm the only one disliking common-stub.c, then please
> ignore my view and I'll nevertheless ack the resulting patch. (That
> said, I view the vpath issue causing the problem as really the one
> that would want tackling. There shouldn't be a requirement for
> files to have different names as long as they live in different
> directories.)
So I agree with Anthony here. I think the approach we use for 
boot.c/efi-boot.h should not be promoted.
I also agree that "common-stub.c" sounds weird. But it would require 
some change in the build system (I always find a bit string we are using 
linking) which is likely too late for 4.17.
So I would be fine with stub-common.c and then protect the alias with 
#ifdef CONFIG_COMPAT.
Cheers,

--
Julien Grall