Re: [PATCH v2] x86/efi: Use generic PE/COFF structures

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Mon, Sep 23, 2024 at 9:56 AM Milan Đokić <milandjokic1995@xxxxxxxxx> wrote:
>
> Hello Frediano,
> Thank you for your feedback
> >
> > On Thu, Sep 19, 2024 at 11:42 AM Milan Đokić <milandjokic1995@xxxxxxxxx> 
> > wrote:
> > >
> > > Hello everyone,
> > >
> > > Any comments on v2 patch?
> > > Just checking if you missed this email or didn't have time for review.
> > >
> > > BR,
> > > Milan
> > >
> >
> > Hi,
> >   it does look good to me.
> > I don't like the "#include "../../../include/efi/pe.h"" line, but I
> > don't exactly know how to add a -I option.
> >
> Yes, this does look bad. Although, I don't see that we have other
> options with the current build setup. mkreloc is built as a separate
> binary and it's added to the hostprogs list, for which we are not able
> to add additional flags. Relative paths are also used for other
> hostprogs when some local xen file needs to be included. Only solution
> which I see here is to have a separate makefile for mkreloc, but I
> would say that this is also not a good option, since we will separate
> mkreloc from other hostprogs in build config. If someone has some
> better idea here we shall be happy to apply it.

Not strong about, a change like

$ git diff
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 286c003ec3..00ab091634 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -88,6 +88,8 @@ extra-y += xen.lds
hostprogs-y += boot/mkelf32
hostprogs-y += efi/mkreloc

+$(obj)/efi/mkreloc: HOSTCFLAGS += -I$(srctree)/include
+
# Allows usercopy.c to include itself
$(obj)/usercopy.o: CFLAGS-y += -iquote .

diff --git a/xen/arch/x86/efi/mkreloc.c b/xen/arch/x86/efi/mkreloc.c
index 89c525d81e..7c9aac49ed 100644
--- a/xen/arch/x86/efi/mkreloc.c
+++ b/xen/arch/x86/efi/mkreloc.c
@@ -9,7 +9,7 @@
#include <sys/mman.h>
#include <unistd.h>

-#include "../../../include/efi/pe.h"
+#include <efi/pe.h>

#define PE_PAGE_SIZE 0x1000


would work. Not sure the best place to put the option in the Makefile
or I should change a different macro.

> > There are some spurious space changes that could be removed but okay
> > for the rest.
> >
> Space changes are caused by our formatter which aligned this file
> according to coding guidelines. We can revert those if formatting
> needs to be done separately.
>

Minor for me. I personally cannot see anything so wrong that prevents a merge.

> Best regards,
> Milan

Frediano