[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v3 1/2] x86/efi: move the logic to detect PE build support

On Wed, Jul 25, 2018 at 03:38:43PM +0200, Roger Pau Monné wrote:
> On Wed, Jul 25, 2018 at 02:48:28PM +0200, Daniel Kiper wrote:
> > On Wed, Jul 18, 2018 at 12:27:32PM +0200, Roger Pau Monne wrote:
> > > So that it can be used by other components apart from the efi specific
> > > code. By moving the detection code creating a dummy efi/disabled file
> > > can be avoided.
> > >
> > > This is required so that the conditional used to define the efi symbol
> > > in the linker script can be removed and instead the definition of the
> > > efi symbol can be guarded using the preprocessor.
> > >
> > > The motivation behind this change is to be able to build Xen using lld
> > > (the LLVM linker), that at least on version 6.0.0 doesn't work
> > > properly with a DEFINED being used in a conditional expression:
> > >
> > > ld    -melf_x86_64_fbsd  -T xen.lds -N prelink.o --build-id=sha1 \
> > >     /root/src/xen/xen/common/symbols-dummy.o -o 
> > > /root/src/xen/xen/.xen-syms.0
> > > ld: error: xen.lds:233: symbol not found: efi
> > >
> > > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > > Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> >
> > This patch does more than subject says. So, please split this patch into
> > 2 parts. One which really moves the detection logic and the second which
> > replaces DEFINED() with #ifdef.
> It's not possible to place the whole contents of the patch in a
> subject line, that's why there's also a commit message that I think
> correctly explains everything done in the patch.

OK, but the subject suggests that the main goal is the PE build support
detection code realignment but AIUI the main goal is DEFINED() drop. So,
this should be in subject. And current commit message is confusing too.
It discuss realignment first and then mentions DEFINED() drop. I think
that it should be done other way around.

> IMO just having a patch to change the DEFINED to an ifdef is quite
> pointless, it's better to switch it here together with the change that
> actually introduces the define itself.

I would do this split. If at some point we have to revert DEFINED()
change then we have to revert whole patch and the following one. If
we do the split we will avoid that problem. And it can be useful for


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.