Re: EFI and multiboot2 devlopment work for Xen

[Jan Beulich]

 On 21.10.13 at 14:57, Daniel Kiper daniel.ki...@oracle.com wrote:

(Looking at the Cc list it's quite interesting that you copied a
whole lot of people, but not me as the maintainer of the EFI
bits in Xen.)

 Separate multiboot2efi module should be established. It should verify system
 kernel and all loaded modules using shim on EFI platforms with enabled 
 secure boot

Each involved component verifies only the next image. I.e. the
shim verifies the Xen image, and Xen verifies the Dom0 kernel
binary. The Dom0 kernel (assuming it to be Linux) will then be
responsible for dealing with its initrd. (One open question is how
Xen ought to deal with an eventual XSM module; I take it that
the CPUs themselves take care of the microcode blob.) This can't
be different because the shim provided verification protocol
assumes that it's being handed a PE image (hence the need for
Linux to package itself as a fake PE image), and hence can't be
used for verifying other than the Xen and Dom0 kernel binaries.

 At first I am going to prepare multiboot2 protocol implementation for Xen 
 (there
 is about 80% of code ready) with above mentioned workaround.

Is that really worthwhile as long as it's not clear whether ...

 Later I am going to work on multiboot2efi module.

... is going to be accepted?

 What do you think about that?
 Any comments, suggestions, objections?

The complications here make it pretty clear to me that the
GrUB2-less solution (or, if GruB2 absolutely has to be involved,
its chain loading capability) I have been advocating continues
to be the better (and, as said before, conceptually correct)
model.

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: EFI and multiboot2 devlopment work for Xen

[Jan Beulich]

 On 21.10.13 at 20:39, Daniel Kiper daniel.ki...@oracle.com wrote:
 On Mon, Oct 21, 2013 at 02:36:38PM +0100, Jan Beulich wrote:
  On 21.10.13 at 14:57, Daniel Kiper daniel.ki...@oracle.com wrote:
  Separate multiboot2efi module should be established. It should verify 
  system
  kernel and all loaded modules using shim on EFI platforms with enabled
  secure boot

 Each involved component verifies only the next image. I.e. the
 shim verifies the Xen image, and Xen verifies the Dom0 kernel
 binary. The Dom0 kernel (assuming it to be Linux) will then be
 responsible for dealing with its initrd. (One open question is how
 
 Currently Linux Kernel is only verified. Sorry, my fault.
 As I know Matthew Garrett would like to verify Linux Kernel
 modules too. However, I do not know details now. I think that
 we should take into account his work.

Sure, Linux modules are to be verified. But that's a Linux thing
we can be entirely unconcerned about. In the context of GrUB,
module can only have the meaning of GrUB modules.

 Xen ought to deal with an eventual XSM module; I take it that
 
 Could you tell me more about that? What issues do you expect here?

We obviously need to have a way to verify the integrity of an
XSM module. Otherwise - as with any unverified component -
its presence would break the integrity of the supposedly secure
system.

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: EFI and multiboot2 devlopment work for Xen

[Jan Beulich]

 On 22.10.13 at 11:26, Ian Campbell ian.campb...@citrix.com wrote:
 AIUI efilinux is somewhat badly named and does not use the Linux Boot
 Protocol (i.e. the (b)zImage stuff with real mode entry point) either.
 It actually loads and executes the kernel binary as a PE/COFF executable
 (the native UEFI binary executable format). xen.efi is a PE/COFF binary
 too and could equally well be launched by linuxefi in this way.

Except that unless I'm mistaken linuxefi still expects to find certain
Linux-specific internal data structures inside the PE image, which I
don't see us wanting to be emulating. That's the main difference to
chainloader afaict.

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: EFI and multiboot2 devlopment work for Xen

[Jan Beulich]

 On 22.10.13 at 15:53, Ian Campbell ian.campb...@citrix.com wrote:
 On Tue, 2013-10-22 at 09:42 -0400, Konrad Rzeszutek Wilk wrote:
 
 Looking at the Fedora GRUB2 source, the 'struct linux_kernel_header' is 
 defined
 in the linux/Documentation/x86/boot.txt and hpa is pretty strict
 about making it backwards compatible. It also seems to support Xen!
 
 (Interestingly enough we do have this structure in the code: see
 setup_header in arch/x86/bzimage.c)
 
 There will be another usage in tools/libxc/...bzimage too
 
 FWIW I think we only use this stuff for the magic number/version and the
 payload_offset/length fields, which we do in order to extract the
 payload (ELF file) for booting dom0 and domU. It's not AFAIK used for
 booting Xen itself or lets say, that's not why I added it ;-)).

Indeed, we only use this to handle bzImage type kernels.

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: EFI and multiboot2 devlopment work for Xen

[Jan Beulich]

 On 22.10.13 at 11:45, Ian Campbell ian.campb...@citrix.com wrote:
 On Tue, 2013-10-22 at 10:31 +0100, Jan Beulich wrote:
  On 22.10.13 at 11:26, Ian Campbell ian.campb...@citrix.com wrote:
  AIUI efilinux is somewhat badly named and does not use the Linux Boot
  Protocol (i.e. the (b)zImage stuff with real mode entry point) either.
  It actually loads and executes the kernel binary as a PE/COFF executable
  (the native UEFI binary executable format). xen.efi is a PE/COFF binary
  too and could equally well be launched by linuxefi in this way.
 
 Except that unless I'm mistaken linuxefi still expects to find certain
 Linux-specific internal data structures inside the PE image, which I
 don't see us wanting to be emulating. That's the main difference to
 chainloader afaict.
 
 Ah, I'd been led to believe it was just the lack of a call to
 ExitBootServices, but I didn't check. What you say sounds completely
 plausible.
 
 Do you know what sort of Linux specific data structures are we talking
 about?

The setup header I would assume (i.e. the bits surrounding the
HdrS signature). But I'm only guessing anyway.

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: EFI and multiboot2 devlopment work for Xen

[Jan Beulich]

 On 22.10.13 at 16:51, Konrad Rzeszutek Wilk konrad.w...@oracle.com wrote:
 And I still haven't found the module that can launch any PE/COFF
 image from GRUB2. Maybe that is a myth.

I can't exclude that this is a custom a patch as the linuxefi support.

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [Xen-devel] EFI and multiboot2 devlopment work for Xen

[Jan Beulich]

 Ian Campbell ian.campb...@citrix.com 10/23/13 10:32 AM 
The second (standard PE/COFF entry point) can be launched using the UEFI
chainloader call. AIUI this should work with xen.efi today. There are
some limitations however, firstly there is no way to pass additional
blobs and so the launched image must load e.g. dom0 and initrd itself,
which Linux does by processing the initrd= option in the command line
and using UEFI functionality to load the blobs from disk. Xen does by
reading its own config file and loading dom0 + initrd based on that. I
assume this means that chainload doesn't call ExitBootServices (anyone
can confirm?). Another limitation of this is that the UEFI functionality
to load stuff from disk can only read from FAT partitions.

Again that's only a pseudo limitation: Rather than implementing support
for other file systems in GrUB, it should be implemented as EFI module.
Then any subsequent EFI binary would benefit from this. (Obviously, as
a half hearted intermediate solution, GrUB - which iiuc stays in memory
after transferring control - could export its file system support to its
descendants).

 It's not
clear to me if this mechanism limits only the loading of additional
blobs from FAT or if that applies to the kernel image itself.

Only the additional blobs would be affected.

 The
kernel is PIC on entry so no relocations are actually needed. In theory
the Linux EFI stub could instead have included a NULL relocation table
but doesn't. (I expect Xen is in the same boat WRT relocations).

No, Xen definitely needs its relocations processed.

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [Xen-devel] EFI and multiboot2 devlopment work for Xen

[Jan Beulich]

 Konrad Rzeszutek Wilk konrad.w...@oracle.com 10/23/13 3:15 PM 
On Wed, Oct 23, 2013 at 09:32:30AM +0100, Ian Campbell wrote:
 Am I correct that xen.efi today can be loaded from grub today using the
 chainload command? Whereupon it will parse the xen.cfg and load the dom0
 kernel and load things from FAT etc and everything just works. IOW
 UEFI - chainload(Xen)
 and
 UEFI - chainload(grub) - chainload(Xen)
 work equivalently from the POV of Xen?

Yes. However it does require the user to know the magic values in the Xen
configuration file and setup the chainload stanze correctly. That means
if a user wishes to modify some of the bootup options they have to modify
the Xen configuration file. No runtime changes.

Not necessarily - there's no reason for it being impossible to specify options
in the GrUB entry, nor for option added on the GrUB prompt (or its graphical
equivalent) to be passed to the chain loaded binary. If that doesn't already
work, it can't be really hard to implement.

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [Xen-devel] EFI and multiboot2 devlopment work for Xen

[Jan Beulich]

 Vladimir 'φ-coder/phcoder' Serbinenkophco...@gmail.com 10/23/13 7:02 PM 
 
 GrUB - which iiuc stays in memory
 after transferring control - could export its file system support to its
 descendants).

Xen shouldn't need to load any file after multiboot2 entry point. The
needed files would already be in memory with pointers to them passed.

I should have said to its chainloaded descendants.

If you insist on being able to load directly from EFI, then IMO the best
way is to have a PE executable with one of sections containing Xen and
code which would load remaining files to memory and call common entry point.

I think you've been told before - this is what has been working already for 
quite
some time.

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: EFI and multiboot2 devlopment work for Xen

[Jan Beulich]

 On 28.10.13 at 19:01, Vladimir 'f-coder/phcoder' 
 Serbinenkophco...@gmail.com wrote:
 Will a multiboot2 tag with whole EFI memory map solve your problem?
 I added such a tag in documentation and wrote a patch for it (attached).
 Awaiting for someone to test it to commit
 
 Great! I think from Xen perspective we first need to have Xen be able
 to understand multiboot2 - that is something Daniel had been working on.
 I will let Daniel talk more about it.
 
 Seth, would you have any time to test the patch against Solaris to
 make sure it works?
 
 I've committed that patch. BTW do you want protected mode or long mode
 entry point for x86_64 variant? Currently it's protected mode but I
 planned to add long mode possibility but it wasn't a priority.

32-bit protected mode is obviously the more consistent model
with multiboot1. But since we'll want to avoid switching back to
real mode when booted from UEFI, long mode would certainly
be an option too (just that the code paths would need to
diverge more).

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [Xen-devel] Loading a 32 bit kernel from 64 bit grub-xen

[Jan Beulich]

 On 01.07.14 at 20:27, ps...@ubuntu.com wrote:
 -BEGIN PGP SIGNED MESSAGE-
 Hash: SHA1
 
 On 7/1/2014 12:18 PM, Andrey Borzenkov wrote:
 В Tue, 01 Jul 2014 12:06:08 -0400 Phillip Susi ps...@ubuntu.com
 пишет:
 
 -BEGIN PGP SIGNED MESSAGE- Hash: SHA1
 
 I have been trying to fix grub to load a 32 bit kernel from the
 64 bit xen build.  After fixing up one or two minor issues with
 the elf loader, I believe I now have it to the point where it
 jumps correctly to the 32 bit kernel and it crashes there, since
 it is 32 bit code still executing on a cpu in 64 bit mode.  The
 question is how to return the cpu to 32 bit mode *under xen*?
 
 
 IIRC it was already discussed not long ago and it seems to be Xen 
 limitation. You probably need to ask on xen-devel to be sure.
 
 Is it not more simple to use 32 bit grub with 32 bit kernel to
 start with?
 
 The problem with that is that you have to know in advance which kernel
 you are going to boot.  That makes configuring virtual hosts harder;
 they just want one grub image they can use to chainload whatever the
 guest wants to install in their domain.
 
 Also there must be a way to do this otherwise a 64 bit kernel running
 under xen wouldn't be able to execute a 32 bit binary.  I suppose I'll
 Cc xen-devel.

Perhaps there's some confusion about what kernel here means:
The thing that's the kernel from GrUB's perspective is the hypervisor,
i.e. xen.gz or xen.efi. While the former expects to be entered in 32-bit
mode, on the Xen side we all agree that this isn't the way things
should work under UEFI; Daniel already proposed how to handle this
case (Xen should be entered in 64-bit mode, with ExitBootServices()
not having got called yet).

The thing that you'd traditionally call kernel (e.g. Linux) can be 32-
or 64-bit irrespective of GrUB's bitness: Xen knows how to load either
(both for Dom0 and, likely irrelevant here, DomU).

Jan

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 00/18] x86: multiboot2 protocol support

[Jan Beulich]

 On 04.02.15 at 10:04, andrew.coop...@citrix.com wrote:
 On 03/02/2015 17:14, Daniel Kiper wrote:
 On Mon, Feb 02, 2015 at 09:28:49AM +, Jan Beulich wrote:
 On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote:
   - xen.efi build will not so strongly depend
 on a given GCC and binutils version.
 While I can see the possibility of making the binutils version
 dependency go away (by manually creating the PE header), I can't
 see how you'd overcome the gcc one: The MS calling convention
 support is still going to be needed (not having looked at the patches
 Right, I forgot about that one.

 themselves yet, I can't see myself accepting the introduction of
 stubs to convert between calling conventions).
 
 How about __attribute__((ms_abi)) ?  It would appear to exist for this
 purpose.

But that's the point: Older compilers don't support it. And with
compilers supporting it we need no stubs.

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 00/18] x86: multiboot2 protocol support

[Jan Beulich]

 On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote:
   - xen.efi build will not so strongly depend
 on a given GCC and binutils version.

While I can see the possibility of making the binutils version
dependency go away (by manually creating the PE header), I can't
see how you'd overcome the gcc one: The MS calling convention
support is still going to be needed (not having looked at the patches
themselves yet, I can't see myself accepting the introduction of
stubs to convert between calling conventions).

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 00/18] x86: multiboot2 protocol support

[Jan Beulich]

 On 05.02.15 at 12:50, phco...@gmail.com wrote:
 Le 2015-02-04 10:51, Jan Beulich jbeul...@suse.com a écrit :

  On 04.02.15 at 10:04, andrew.coop...@citrix.com wrote:
  On 03/02/2015 17:14, Daniel Kiper wrote:
  On Mon, Feb 02, 2015 at 09:28:49AM +, Jan Beulich wrote:
  On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote:
- xen.efi build will not so strongly depend
  on a given GCC and binutils version.
  While I can see the possibility of making the binutils version
  dependency go away (by manually creating the PE header), I can't
  see how you'd overcome the gcc one: The MS calling convention
  support is still going to be needed (not having looked at the patches
  Right, I forgot about that one.
 
  themselves yet, I can't see myself accepting the introduction of
  stubs to convert between calling conventions).
 
  How about __attribute__((ms_abi)) ?  It would appear to exist for this
  purpose.

 But that's the point: Older compilers don't support it. And with
 compilers supporting it we need no stubs.

 ms_abi has been around for years. What's your minimum compiler requirement?

4.1

Jan

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 02/18] x86/boot/reloc: create generic alloc and copy functions

[Jan Beulich]

 On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote:
 --- a/xen/arch/x86/boot/reloc.c
 +++ b/xen/arch/x86/boot/reloc.c
 @@ -33,9 +33,10 @@ asm (
  typedef unsigned int u32;
  #include ../../../include/xen/multiboot.h
  
 -static void *reloc_mbi_struct(void *old, unsigned int bytes)
 +static u32 alloc_struct(u32 bytes)

The generalization calls for the naming to change. Especially with the
use from copy_string(), both of the functions no longer deal with
struct-s alone. Please name them ..._block() or some other more
neutral way.

 +static u32 copy_string(u32 src)
  {
  char *p;
 -for ( p = old; *p != '\0'; p++ )
 +
 +if ( src == 0 )
 +return 0;
 +
 +for ( p = (char *)src; *p != '\0'; p++ )
  continue;
 -return reloc_mbi_struct(old, p - old + 1);
 +
 +return copy_struct(src, p - (char *)src + 1);
  }

As few casts as possible please: You can get away with one if you type
p u32.

  multiboot_info_t *reloc(multiboot_info_t *mbi_old)
  {
 -multiboot_info_t *mbi = reloc_mbi_struct(mbi_old, sizeof(*mbi));
 +multiboot_info_t *mbi = (multiboot_info_t *)copy_struct((u32)mbi_old, 
 sizeof(*mbi));

Please get rid of the cast on the first argument by converting
reloc()'s parameter type accordingly.

  int i;
  
  if ( mbi-flags  MBI_CMDLINE )
 -mbi-cmdline = (u32)reloc_mbi_string((char *)mbi-cmdline);
 +mbi-cmdline = copy_string(mbi-cmdline);
  
  if ( mbi-flags  MBI_MODULES )
  {
 -module_t *mods = reloc_mbi_struct(
 -(module_t *)mbi-mods_addr, mbi-mods_count * sizeof(module_t));
 +module_t *mods = (module_t *)copy_struct(
 +mbi-mods_addr, mbi-mods_count * sizeof(module_t));
  
  mbi-mods_addr = (u32)mods;

And again you can get away with one less cast if you store the
result of copy_struct() here and then assign mods.

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 03/18] x86/boot: use %ecx instead of %eax

[Jan Beulich]

 On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote:
  /* Save the Multiboot info struct (after relocation) for later use. 
 */
  mov $sym_phys(cpu0_stack)+1024,%esp
 -push%ebx
 -callreloc
 +mov %ecx,%eax
 +push%ebx/* Multiboot information address */
 +callreloc   /* %eax contains trampoline address */

This last part looks completely unrelated to the change made here
(and contrary to the description, as here you clobber %eax while
the description says reloc() needs it unclobbered); afaict it belongs
in whatever patch add the consumption of this value in reloc().
That said - passing parameters to reloc() by two different means
looks very odd too. I'm clearly of the opinion that parameter
passing should follow an existing convention unless entirely
unfeasible. Which then raises the question whether this patch is
really needed: Rather than fiddling with a lot of code, can't you
just copy the incoming %eax into some other register, making this
a single line change that can again simply be done in the patch
where you actually consume the new information?

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 00/18] x86: multiboot2 protocol support

[Jan Beulich]

 On 09.02.15 at 18:59, daniel.ki...@oracle.com wrote:
 On Fri, Jan 30, 2015 at 06:54:04PM +0100, Daniel Kiper wrote:
 I am sending, long awaited, first version of multiboot2 protocol
 support for legacy BIOS and EFI platforms.

 The final goal is xen.efi binary file which could be loaded by EFI
 loader, multiboot (v1) protocol (only on legacy BIOS platforms) and
 multiboot2 protocol. This way we will have:
   - smaller Xen code base,
   - one code base for xen.gz and xen.efi,
   - one build method for xen.gz and xen.efi;
 xen.efi will be extracted from xen file
 using objcopy; PE header will be contained
 in ELF file and will precede Xen code,
   - xen.efi build will not so strongly depend
 on a given GCC and binutils version.
 
 I have not received so many comments on this patch series.
 Is there no interest in having this feature in Xen? Is there
 anything wrong? Should I fix something?

I didn't get to look at patches 4 and onwards yet, there's too much
other stuff I need to take care of right now; I'll get to this eventually.

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms

[Jan Beulich]

 On 10.02.15 at 22:27, daniel.ki...@oracle.com wrote:
 After some testing we have found at least one machine on which this thing
 does not work. It is Dell PowerEdge R820 with latest firmware. Machine
 crashes/stops because early 32-bit code is not relocatable and must live
 under 0x10 address. (side note: I am surprised how it worked without
 any issue until now; Multiboot protocol, any version, does not guarantee
 that OS image will be loaded at specified/requested address;

How does it not? It's an ELF binary without relocations that's being
loaded - I can't see how such could be validly loaded anywhere but
at the virtual address(es) its program header states (and I don't
know whether grub [1 or 2] would correctly process relocations if
there were any, but I doubt it).

 Now I see two solutions for these issues:
 
 1) We can make early 32-bit code relocatable. We may use something similar
to xen/arch/x86/boot/trampoline.S:bootsym_rel(). Additionally, I think
that early code should not blindly map first 16 MiB of memory. It should
map first 1 MiB of memory and then 16 MiB of memory starting from
xen_phys_start. This way we also fix long standing bug in early code
which I described earlier.
 
 2) We can jump from EFI x86-64 mode directly into Xen x86-64 mode like
it is done in case of EFI loader. However, then we must duplicate 
 multiboot2
protocol implementation in x86-64 mode (if we wish that multiboot2 
 protocol
can be used on legacy BIOS and EFI platforms; I think that we should 
 support
this protocol on both for users convenience). Additionally, we must use
a workaround to relocate trampoline if boot services uses memory below 1 
 MiB
(please check commit c1f2dfe8f6a559bc28935f24e31bb33d17d9713d, x86/EFI: 
 make
trampoline allocation more flexible, for more details).
 
 I prefer #1 because this way we do not duplicate multiboot2 protocol 
 implementation
 (one for legacy BIOS and EFI) and we avoid issues with trampoline relocation 
 when
 low memory is occupied by boot services and/or 1:1 EFI page tables.

Between the two, 1 is certainly the preferable option.

 PS I have just realized that commit c1f2dfe8f6a559bc28935f24e31bb33d17d9713d
will not work if trampoline code will overwrite some of EFI 1:1 page 
 tables.
Dell PowerEdge R820 store part of 1:1 page tables below 1 MiB. Xen loaded
by native EFI loader boots but it is only lucky coincidence that it does
not overwrite used entries. So, I tend to go and choose #1 even more.

How awful a firmware implementation! On PC-like systems, _nothing_
that _absolutely_ has to be below 1Mb should be placed there.

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms

[Jan Beulich]

 On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote:
 @@ -94,6 +111,17 @@ ENTRY(start)
  gdt_boot_descr:
  .word   6*8-1
  .long   sym_phys(trampoline_gdt)
 +.long   0 /* Needed for 64-bit lgdt */
 +
 +cs32_switch_addr:
 +.long   sym_phys(cs32_switch)
 +.long   BOOT_CS32
 +
 +efi_st:
 +.quad   0
 +
 +efi_ih:
 +.quad   0
  
  .Lbad_cpu_msg: .asciz ERR: Not a 64-bit CPU!
  .Lbad_ldr_msg: .asciz ERR: Not a Multiboot bootloader!
 @@ -124,6 +152,133 @@ print_err:
  .Lhalt: hlt
  jmp .Lhalt
  
 +.code64
 +
 +__efi64_start:
 +cld
 +
 +/* Bootloaders may set multiboot[12].mem_lower to a nonzero value */
 +xor %edx,%edx
 +
 +/* Check for Multiboot2 bootloader */
 +cmp $MULTIBOOT2_BOOTLOADER_MAGIC,%eax
 +je  efi_multiboot2_proto
 +
 +jmp not_multiboot
 +
 +efi_multiboot2_proto:

jne not_multiboot (and efi_multiboot2_proto dropped altogether)

 +/* Skip Multiboot2 information fixed part */
 +lea MB2_fixed_sizeof(%ebx),%ecx

Let's please not add more assumptions than necessary about stuff
being below 4G.

 +
 +0:
 +/* Get mem_lower from Multiboot2 information */
 +cmpl$MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO,(%ecx)
 +jne 1f
 +
 +mov MB2_mem_lower(%ecx),%edx
 +jmp 4f
 +
 +1:
 +/* Get EFI SystemTable address from Multiboot2 information */
 +cmpl$MULTIBOOT2_TAG_TYPE_EFI64,(%ecx)
 +jne 2f
 +
 +lea MB2_efi64_st(%ecx),%esi
 +lea efi_st(%rip),%edi
 +movsq

A simple pair of mov-s please, assuming all of this really needs to be
done in assembly in the first place. Also please use .Lname labels
in this assembly coded switch statement to ease reading.

 +
 +/* Do not go into real mode on EFI platform */
 +movb$1,skip_realmode(%rip)
 +
 +jmp 4f
 +
 +2:
 +/* Get EFI ImageHandle address from Multiboot2 information */
 +cmpl$MULTIBOOT2_TAG_TYPE_EFI64_IH,(%ecx)
 +jne 3f
 +
 +lea MB2_efi64_ih(%ecx),%esi
 +lea efi_ih(%rip),%edi
 +movsq
 +jmp 4f
 +
 +3:
 +/* Is it the end of Multiboot2 information? */
 +cmpl$MULTIBOOT2_TAG_TYPE_END,(%ecx)
 +je  run_bs
 +
 +4:

Please switch the order so that 2 can fall through into 4 (and you
then won't need the run_bs label, which otherwise should also
becom .Lrun_bs).

 +/* Go to next Multiboot2 information tag */
 +add MB2_tag_size(%ecx),%ecx
 +add $(MULTIBOOT2_TAG_ALIGN-1),%ecx
 +and $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
 +jmp 0b
 +
 +run_bs:
 +push%rax
 +push%rdx
 +
 +/* Initialize BSS (no nasty surprises!) */
 +lea __bss_start(%rip),%rdi
 +lea _end(%rip),%rcx
 +sub %rdi,%rcx
 +xor %rax,%rax
 +rep stosb

rep stosb

 +
 +mov efi_ih(%rip),%rdi   /* EFI ImageHandle */
 +mov efi_st(%rip),%rsi   /* EFI SystemTable */
 +callefi_multiboot2

With efi_multiboot2 being a C function it really looks like much of the
above doesn't need to be done in assembly.

 +
 +pop %rcx
 +pop %rax
 +
 +shl $10-4,%rcx  /* Convert multiboot2.mem_lower to 
 bytes/16 */
 +
 +cli
 +
 +/* Initialise GDT */
 +lgdtgdt_boot_descr(%rip)
 +
 +/* Reload code selector */
 +ljmpl   *cs32_switch_addr(%rip)
 +
 +.code32
 +
 +cs32_switch:
 +/* Initialise basic data segments */
 +mov $BOOT_DS,%edx
 +mov %edx,%ds
 +mov %edx,%es
 +mov %edx,%fs
 +mov %edx,%gs
 +mov %edx,%ss
 +
 +mov $sym_phys(cpu0_stack)+1024,%esp
 +
 +/* Disable paging */
 +mov %cr0,%edx
 +and $(~X86_CR0_PG),%edx
 +mov %edx,%cr0
 +
 +push%eax
 +push%ecx
 +
 +/* Disable Long Mode */
 +mov $MSR_EFER,%ecx
 +rdmsr
 +and $(~EFER_LME),%eax
 +wrmsr

I don't think this is really needed.

 +
 +pop %ecx
 +pop %eax
 +
 +/* Turn off PAE */
 +mov %cr4,%edx
 +and $(~X86_CR4_PAE),%edx
 +mov %edx,%cr4

Nor this.

 @@ -179,7 +334,7 @@ multiboot2_proto:
  and $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
  jmp 0b
  
 -trampoline_setup:
 +bios_platform:
  /* Set up trampoline segment 64k below EBDA */

This is still trampoline setup code, so it's not clear why you rename
the label. If you need another named label ...

 @@ -195,12 +350,13 @@ trampoline_setup:
   * multiboot structure (if available) and use the smallest.
   */
  cmp $0x100,%edx /* is the multiboot value too small? */
 -

Re: [PATCH 04/18] xen/x86: add multiboot2 protocol support

[Jan Beulich]

 On 27.03.15 at 11:56, daniel.ki...@oracle.com wrote:
 On Fri, Feb 20, 2015 at 04:06:26PM +, Jan Beulich wrote:
  On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote:
  --- a/xen/arch/x86/boot/Makefile
  +++ b/xen/arch/x86/boot/Makefile
  @@ -1,6 +1,7 @@
   obj-bin-y += head.o
 
  -RELOC_DEPS = $(BASEDIR)/include/asm-x86/config.h 
  $(BASEDIR)/include/xen/multiboot.h
  +RELOC_DEPS = $(BASEDIR)/include/asm-x86/config.h 
  $(BASEDIR)/include/xen/compiler.h \
  +   $(BASEDIR)/include/xen/multiboot.h 
  $(BASEDIR)/include/xen/multiboot2.h

 Perhaps we should rather have the compiler generate the
 dependencies for us when compiling reloc.o?
 
 Hmmm... I am a bit confused. Here 
 http://lists.xen.org/archives/html/xen-devel/2014-10/msg02094.html 
 you told a bit different thing and relevant patch was accepted as commit 
 c42070df66c9fcedf477959b8371b85aa4ac4933
 (x86/boot: fix reloc.S build dependencies). I can try to do what you 
 suggest, this is not a problem
 for me, however, I would like to be sure what is your final decision in that 
 case.

First of all I wasn't anticipating that this list of dependencies would
further grow. And then I didn't say don't do this, I only pointed
out (albeit maybe a little too implicitly) that this would be a more
complex patch.

  +.long   MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO
  +.long   MULTIBOOT2_TAG_TYPE_MMAP
  +.Lmultiboot2_info_req_end:
  +
  +.align  MULTIBOOT2_TAG_ALIGN
  +.short  MULTIBOOT2_HEADER_TAG_MODULE_ALIGN
  +.short  MULTIBOOT2_HEADER_TAG_REQUIRED
  +.long   8 /* Tag size. */

 ... here and further down you hard-code it. Please be consistent
 (and if you go the calculation route, perhaps introduce some
 redundancy reducing macro).
 
 I did this deliberately. I calculate size using labels when it is really
 needed, e.g. in tags which size depends on number of elements. However,
 most tags have strictly defined sizes. So, that is why I dropped labels
 and entered simple numbers. Though I agree that it can be improved.
 I think that we can define relevant tag structures in multiboot2.h and
 generate relevant constants using asm-offsets.c. Is it OK for you?

That would mean exposing stuff to other parts of the tree which
doesn't need to be exposed. I don't see why you can't just do
proper size calculations here.

  @@ -81,10 +135,51 @@ __start:
   mov %ecx,%es
   mov %ecx,%ss
 
  +/* Bootloaders may set multiboot[12].mem_lower to a nonzero value 
  */

 Above you meet coding style requirements, but here you stop to do
 so (ongoing below)? Even if pre-existing neighboring comments aren't
 well formed, please don't make the situation worse.
 
 Do you ask about ending fullstops? Am I right?

Yes.

  @@ -31,7 +38,16 @@ asm (
   );
 
   typedef unsigned int u32;
  +typedef unsigned long long u64;
  +
  +#include ../../../include/xen/compiler.h

 Why?
 
 static multiboot_info_t __used *reloc(void *mbi_in, u32 mb_magic)
 Because of this -- ^^

And what keeps you from leaving out the static, making the
__used unnecessary?

  +
  +typedef struct {
  +u32 type;
  +u32 size;
  +u32 mem_lower;
  +u32 mem_upper;
  +} multiboot2_tag_basic_meminfo_t;
  +
  +typedef struct __packed {

 Why __packed when all the others aren't?
 
 Ha! This thing was taken from GRUB2 source.
 I was asking this question myself many times.
 I have not found real explanation for this
 but if you wish I can dive into code and
 try to find one (if it exists).

Or even better just drop the __packed.

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 16/18] efi: create efi_exit_boot()

[Jan Beulich]

 On 27.03.15 at 13:00, daniel.ki...@oracle.com wrote:
 Additionally, efi_start()
 is architecture independent and efi_multiboot2() is x86 only and it should
 live in x86 files.

Is that really the case? Looking at the grub2 sources I see support
for other than x86...

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 04/18] xen/x86: add multiboot2 protocol support

[Jan Beulich]

 On 27.03.15 at 13:22, daniel.ki...@oracle.com wrote:
 On Fri, Mar 27, 2015 at 11:20:10AM +, Jan Beulich wrote:
  On 27.03.15 at 11:56, daniel.ki...@oracle.com wrote:
  On Fri, Feb 20, 2015 at 04:06:26PM +, Jan Beulich wrote:
   On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote:
   @@ -31,7 +38,16 @@ asm (
);
  
typedef unsigned int u32;
   +typedef unsigned long long u64;
   +
   +#include ../../../include/xen/compiler.h
 
  Why?
 
  static multiboot_info_t __used *reloc(void *mbi_in, u32 mb_magic)
  Because of this -- ^^

 And what keeps you from leaving out the static, making the
 __used unnecessary?
 
 This func is clearly static. Why do not mark it as is and use __used.
 What is wrong with that?

#include-s of the above kind generally indicate badness, so we
should try to limit them to the absolute minimum.

   +
   +typedef struct {
   +u32 type;
   +u32 size;
   +u32 mem_lower;
   +u32 mem_upper;
   +} multiboot2_tag_basic_meminfo_t;
   +
   +typedef struct __packed {
 
  Why __packed when all the others aren't?
 
  Ha! This thing was taken from GRUB2 source.
  I was asking this question myself many times.
  I have not found real explanation for this
  but if you wish I can dive into code and
  try to find one (if it exists).

 Or even better just drop the __packed.
 
 Should not we be in line with GRUB2 source?
 Even it sounds strange. Anyway, I will check
 GRUB2 source and maybe we can also remove __packed
 from it. This way everything will be OK.

Now that's the right course of action.

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 08/18] efi: build xen.gz with EFI code

[Jan Beulich]

 On 27.03.15 at 12:14, daniel.ki...@oracle.com wrote:
 IIRC, MS ABI is supported starting from GCC v4.0.

Where did you find that? From all I know __attribute__((__ms_abi__))
is being supported only by 4.5 and newer. The mere support of the
MS ABI via command line option doesn't help us, as we need to be
able to mix ABIs within a single source file.

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 17/18] x86/efi: create new early memory allocator

[Jan Beulich]

 On 27.03.15 at 13:57, daniel.ki...@oracle.com wrote:
 On Mon, Mar 02, 2015 at 05:23:49PM +, Jan Beulich wrote:
  On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote:
  +{
  +void *ptr;
  +
  +/*
  + * Init __malloc_free on runtime. Static initialization
  + * will not work because it puts virtual address there.
  + */
  +if ( __malloc_free == NULL )
  +__malloc_free = __malloc_mem;
  +
  +ptr = __malloc_free;
  +
  +__malloc_free += size;
  +
  +if ( __malloc_free - __malloc_mem  sizeof(__malloc_mem) )
  +blexit(LOut of static memory\r\n);
  +
  +return ptr;
  +}

 You're ignoring alignment requirements here altogether.
 
 I can understand why __malloc_mem should be let's say page aligned
 but I am not sure why we should care here about alignment inside
 of __malloc_mem array like...
 
  @@ -192,12 +218,7 @@ static void __init
  efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
 
   static void *__init efi_arch_allocate_mmap_buffer(UINTN *map_size)
   {
  -place_string(mbi.mem_upper, NULL);
  -mbi.mem_upper -= *map_size;
  -mbi.mem_upper = -__alignof__(EFI_MEMORY_DESCRIPTOR);
 
 ...here...

Specifically with the later mentioned potential for sharing this with
ARM I think you have to make sure that things are suitably aligned,
or else you cause aborts.

  -if ( mbi.mem_upper  xen_phys_start )
  -return NULL;
  -return (void *)(long)mbi.mem_upper;
  +return __malloc(*map_size);
   }

 Which then even suggests that _if_ we go this route, this could be
 shared with ARM (and hence become common code again).
 
 So, go or no go this route?

As long as it's being done properly, I'm not wildly opposed.

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 05/18] efi: split efi_enabled to efi_platform and efi_loader

[Jan Beulich]

 On 27.03.15 at 14:32, daniel.ki...@oracle.com wrote:
 On Fri, Feb 20, 2015 at 04:17:35PM +, Jan Beulich wrote:
  On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote:
  We need more fine grained knowledge about EFI environment and check
  for EFI platform and EFI loader separately to properly support
  multiboot2 protocol.

 ... because of ... (i.e. I can't see from the description what the
 separation is good for). Looking at the comments you placed
 aside the variables doesn't help me either.

  In general Xen loaded by this protocol uses
  memory mappings and loaded modules in simliar way to Xen loaded
  by multiboot (v1) protocol. Hence, split efi_enabled to efi_platform
  and efi_loader.

 And if I'm guessing things right, then introducing efi_loader but
 leaving efi_enabled alone (only converting where needed) would
 
 efi_enabled is not fortunate name for new usage. Currently it means
 that Xen binary have or does not have EFI support build in. However,
 if we build in multiboot2 protocol into xen.gz then it means that
 it can ran on legacy BIOS or EFI platform. So, I think that we
 should rename efi_enabled to efi_platform because it will mean
 that Xen runs on EFI platform or not.

I disagree here.

 efi_loader is used to differentiate between EFI native loader
 and multiboot2 protocol.

And I agree here.

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms

[Jan Beulich]

 On 27.03.15 at 14:06, daniel.ki...@oracle.com wrote:
 On Tue, Mar 17, 2015 at 10:32:01AM +, Jan Beulich wrote:
  On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote:
  +/* Skip Multiboot2 information fixed part */
  +lea MB2_fixed_sizeof(%ebx),%ecx

 Let's please not add more assumptions than necessary about stuff
 being below 4G.
 
 I am not sure what do you mean by that.

See the 32-bit register used for addressing here (and in many more
places)?

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 05/18] efi: split efi_enabled to efi_platform and efi_loader

[Jan Beulich]

 On 27.03.15 at 14:53, andrew.coop...@citrix.com wrote:
 On 27/03/15 13:43, Jan Beulich wrote:
 On 27.03.15 at 14:32, daniel.ki...@oracle.com wrote:
 On Fri, Feb 20, 2015 at 04:17:35PM +, Jan Beulich wrote:
 On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote:
 We need more fine grained knowledge about EFI environment and check
 for EFI platform and EFI loader separately to properly support
 multiboot2 protocol.
 ... because of ... (i.e. I can't see from the description what the
 separation is good for). Looking at the comments you placed
 aside the variables doesn't help me either.

 In general Xen loaded by this protocol uses
 memory mappings and loaded modules in simliar way to Xen loaded
 by multiboot (v1) protocol. Hence, split efi_enabled to efi_platform
 and efi_loader.
 And if I'm guessing things right, then introducing efi_loader but
 leaving efi_enabled alone (only converting where needed) would
 efi_enabled is not fortunate name for new usage. Currently it means
 that Xen binary have or does not have EFI support build in. However,
 if we build in multiboot2 protocol into xen.gz then it means that
 it can ran on legacy BIOS or EFI platform. So, I think that we
 should rename efi_enabled to efi_platform because it will mean
 that Xen runs on EFI platform or not.
 I disagree here.

 efi_loader is used to differentiate between EFI native loader
 and multiboot2 protocol.
 And I agree here.
 
 I suppose built with efi support is known because of CONFIG_EFI or 
 not, and doesn't need a variable.
 
 However, booted legacy vs booted EFI does need distinguishing at 
 runtime, as either is possible.

Right, but that's what efi_enabled is supposed to express after
the change; there's no need to express built with EFI support.
It just so happens that right now, without all these changes,
built-with-EFI-support == runs-on-EFI.

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms

[Jan Beulich]

 On 27.03.15 at 15:26, daniel.ki...@oracle.com wrote:
 On Fri, Mar 27, 2015 at 01:36:32PM +, Jan Beulich wrote:
  On 27.03.15 at 14:06, daniel.ki...@oracle.com wrote:
  On Tue, Mar 17, 2015 at 10:32:01AM +, Jan Beulich wrote:
   On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote:
   +/* Skip Multiboot2 information fixed part */
   +lea MB2_fixed_sizeof(%ebx),%ecx
 
  Let's please not add more assumptions than necessary about stuff
  being below 4G.
 
  I am not sure what do you mean by that.

 See the 32-bit register used for addressing here (and in many more
 places)?
 
 This is what I expected but I was confused because you were referring only
 here to this problem. Anyway, is it possible to do this in different way?
 Should we care if image is always loaded at 0x10 right now? Even with
 Xen early boot code being relocatable loader could not load image higher
 than 0x - 14 MiB.

I don't understand what you're alluding to. Just use 64-bit registers
for memory accesses and LEAs, and be done. This will result in smaller
code as a benefit.

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [Xen-devel] [PATCH 05/18] efi: split efi_enabled to efi_platform and efi_loader

[Jan Beulich]

 On 27.03.15 at 15:09, lsore...@csclub.uwaterloo.ca wrote:
 On Fri, Mar 27, 2015 at 02:04:22PM +, Jan Beulich wrote:
  On 27.03.15 at 14:53, andrew.coop...@citrix.com wrote:
  On 27/03/15 13:43, Jan Beulich wrote:
  On 27.03.15 at 14:32, daniel.ki...@oracle.com wrote:
  On Fri, Feb 20, 2015 at 04:17:35PM +, Jan Beulich wrote:
  On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote:
  We need more fine grained knowledge about EFI environment and check
  for EFI platform and EFI loader separately to properly support
  multiboot2 protocol.
  ... because of ... (i.e. I can't see from the description what the
  separation is good for). Looking at the comments you placed
  aside the variables doesn't help me either.
 
  In general Xen loaded by this protocol uses
  memory mappings and loaded modules in simliar way to Xen loaded
  by multiboot (v1) protocol. Hence, split efi_enabled to efi_platform
  and efi_loader.
  And if I'm guessing things right, then introducing efi_loader but
  leaving efi_enabled alone (only converting where needed) would
  efi_enabled is not fortunate name for new usage. Currently it means
  that Xen binary have or does not have EFI support build in. However,
  if we build in multiboot2 protocol into xen.gz then it means that
  it can ran on legacy BIOS or EFI platform. So, I think that we
  should rename efi_enabled to efi_platform because it will mean
  that Xen runs on EFI platform or not.
  I disagree here.
 
  efi_loader is used to differentiate between EFI native loader
  and multiboot2 protocol.
  And I agree here.
  
  I suppose built with efi support is known because of CONFIG_EFI or 
  not, and doesn't need a variable.
  
  However, booted legacy vs booted EFI does need distinguishing at 
  runtime, as either is possible.
 
 Right, but that's what efi_enabled is supposed to express after
 the change; there's no need to express built with EFI support.
 It just so happens that right now, without all these changes,
 built-with-EFI-support == runs-on-EFI.
 
 Then how about 'efi_booted' as the variable name.

Why should we rename a variable that's perfectly suitable for the
purposes we have? Even more so that we don't just want to
express that we were booted from EFI, but also that we're running
in a respective environment, including using tables coming from
EFI and using runtime services (unless specifically disabled). If
anything we could follow Linux and make efi_enabled a bit mask.

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms

[Jan Beulich]

 On 27.03.15 at 15:57, daniel.ki...@oracle.com wrote:
 On Fri, Mar 27, 2015 at 02:34:19PM +, Jan Beulich wrote:
  On 27.03.15 at 15:26, daniel.ki...@oracle.com wrote:
  On Fri, Mar 27, 2015 at 01:36:32PM +, Jan Beulich wrote:
   On 27.03.15 at 14:06, daniel.ki...@oracle.com wrote:
   On Tue, Mar 17, 2015 at 10:32:01AM +, Jan Beulich wrote:
On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote:
+/* Skip Multiboot2 information fixed part */
+lea MB2_fixed_sizeof(%ebx),%ecx
  
   Let's please not add more assumptions than necessary about stuff
   being below 4G.
  
   I am not sure what do you mean by that.
 
  See the 32-bit register used for addressing here (and in many more
  places)?
 
  This is what I expected but I was confused because you were referring only
  here to this problem. Anyway, is it possible to do this in different way?
  Should we care if image is always loaded at 0x10 right now? Even with
  Xen early boot code being relocatable loader could not load image higher
  than 0x - 14 MiB.

 I don't understand what you're alluding to. Just use 64-bit registers
 for memory accesses and LEAs, and be done. This will result in smaller
 code as a benefit.
 
 Well... How can I do that here if processor is in 32-bit mode? Maybe,
 we could that things after switching to 64-bit mode. However, I think
 this requires separate patch to do these changes.

No, if the processor is in 32-bit mode, then using 32-bit registers is
fine of course. But I'm pretty certain I spotted at least some cases
where it looked like you used 32-bit registers in 64-bit mode.

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 17/18] x86/efi: create new early memory allocator

[Jan Beulich]

 On 02.03.15 at 21:25, roy.fr...@linaro.org wrote:
 On Mon, Mar 2, 2015 at 9:23 AM, Jan Beulich jbeul...@suse.com wrote:
 On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote:
 @@ -192,12 +218,7 @@ static void __init
 efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,

  static void *__init efi_arch_allocate_mmap_buffer(UINTN *map_size)
  {
 -place_string(mbi.mem_upper, NULL);
 -mbi.mem_upper -= *map_size;
 -mbi.mem_upper = -__alignof__(EFI_MEMORY_DESCRIPTOR);
 -if ( mbi.mem_upper  xen_phys_start )
 -return NULL;
 -return (void *)(long)mbi.mem_upper;
 +return __malloc(*map_size);
  }

 Which then even suggests that _if_ we go this route, this could be
 shared with ARM (and hence become common code again).
 
 We could do the same thing on ARM.  For ARM, 2 allocations are done: 1
 for the FDT, and
 this one for the EFI memory map.  Both of these are currently
 allocated with EFI allocation
 functions, so don't have fixed size limits.  If we go with the fixed
 size pool, I don't think that 64k
 will be enough for the ARM case, as FDTs can be 20-50k in size.

The 64k size here is to be debated anyway I think. We currently have
about 1Mb in the x86 variant, and I'd much rather see the pool be
this size initially, properly taking care of releasing to the allocator any
unused portions of it.

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 08/18] efi: build xen.gz with EFI code

[Jan Beulich]

 On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote:
 --- a/xen/arch/x86/efi/Makefile
 +++ b/xen/arch/x86/efi/Makefile
 @@ -1,14 +1,14 @@
  CFLAGS += -fshort-wchar
  
 -obj-y += stub.o
 +obj-y += boot.o
 +obj-y += compat.o
 +obj-y += runtime.o

So how is this going to work with a compiler not new enough to
support the MS ABI (via function attribute)?

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 16/18] efi: create efi_exit_boot()

[Jan Beulich]

 On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote:
 ..which gets memory map and calls ExitBootServices(). We need this
 to support multiboot2 protocol on EFI platforms.

Patches from 9 up to here all make sense on the basis that patch 18
does and assuming that you really need all this code moved out to
separate functions. How much different is efi_multiboot2() introduced
in #18 from what is left of efi_start() at this point? I.e. is splitting out
all of this code really needed?

If it is, please don't title all these patches create ... but split out
... or some such - you don't really create the code. Similarly the
second sentence above is too imprecise for my taste - we want to
re-use this code to support ... would seem more to the point.

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 17/18] x86/efi: create new early memory allocator

[Jan Beulich]

 On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote:
 --- a/xen/arch/x86/efi/efi-boot.h
 +++ b/xen/arch/x86/efi/efi-boot.h
 @@ -103,9 +103,35 @@ static void __init relocate_trampoline(unsigned long 
 phys)
  *(u16 *)(*trampoline_ptr + (long)trampoline_ptr) = phys  4;
  }
  
 +#define __MALLOC_SIZE65536
 +
 +static char __malloc_mem[__MALLOC_SIZE];
 +static char *__malloc_free = NULL;
 +
 +static void __init *__malloc(size_t size)

Please do away with all these prefixing underscores.

 +{
 +void *ptr;
 +
 +/*
 + * Init __malloc_free on runtime. Static initialization
 + * will not work because it puts virtual address there.
 + */
 +if ( __malloc_free == NULL )
 +__malloc_free = __malloc_mem;
 +
 +ptr = __malloc_free;
 +
 +__malloc_free += size;
 +
 +if ( __malloc_free - __malloc_mem  sizeof(__malloc_mem) )
 +blexit(LOut of static memory\r\n);
 +
 +return ptr;
 +}

You're ignoring alignment requirements here altogether.

 @@ -192,12 +218,7 @@ static void __init 
 efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
  
  static void *__init efi_arch_allocate_mmap_buffer(UINTN *map_size)
  {
 -place_string(mbi.mem_upper, NULL);
 -mbi.mem_upper -= *map_size;
 -mbi.mem_upper = -__alignof__(EFI_MEMORY_DESCRIPTOR);
 -if ( mbi.mem_upper  xen_phys_start )
 -return NULL;
 -return (void *)(long)mbi.mem_upper;
 +return __malloc(*map_size);
  }

Which then even suggests that _if_ we go this route, this could be
shared with ARM (and hence become common code again).

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 05/18] efi: split efi_enabled to efi_platform and efi_loader

[Jan Beulich]

 On 03.03.15 at 00:40, roy.fr...@linaro.org wrote:
 Reviewing the #ifndef CONFIG_ARM in EFI code, and the efi_enabled
 usage elsewhere,
 the remaining EFI tasks on ARM look like:
 * Support for SetVirtualAddressMap
 * Runtime service support - looks like just time function used by x86
 in get_cmos_time()

* Provide runtime service access for Dom0

Jan

 Not strictly EFI, but related:
 * Lookup of ACPI tables
 * Lookup of SMBIOS tables
 
 Roy




___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 04/18] xen/x86: add multiboot2 protocol support

[Jan Beulich]

 On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote:
 --- a/xen/arch/x86/boot/Makefile
 +++ b/xen/arch/x86/boot/Makefile
 @@ -1,6 +1,7 @@
  obj-bin-y += head.o
  
 -RELOC_DEPS = $(BASEDIR)/include/asm-x86/config.h 
 $(BASEDIR)/include/xen/multiboot.h
 +RELOC_DEPS = $(BASEDIR)/include/asm-x86/config.h 
 $(BASEDIR)/include/xen/compiler.h \
 +  $(BASEDIR)/include/xen/multiboot.h 
 $(BASEDIR)/include/xen/multiboot2.h

Perhaps we should rather have the compiler generate the
dependencies for us when compiling reloc.o?

 @@ -32,6 +33,59 @@ ENTRY(start)
  .long   MULTIBOOT_HEADER_FLAGS
  /* Checksum: must be the negated sum of the first two fields. */
  .long   -(MULTIBOOT_HEADER_MAGIC + MULTIBOOT_HEADER_FLAGS)
 + 
 +/*** MULTIBOOT2 HEADER /
 +/* Some ideas are taken from grub-2.00/grub-core/tests/boot/kernel-i386.S 
 file. */
 +.align  MULTIBOOT2_HEADER_ALIGN
 +
 +.Lmultiboot2_header:
 +/* Magic number indicating a Multiboot2 header. */
 +.long   MULTIBOOT2_HEADER_MAGIC
 +/* Architecture: i386. */
 +.long   MULTIBOOT2_ARCHITECTURE_I386
 +/* Multiboot2 header length. */
 +.long   .Lmultiboot2_header_end - .Lmultiboot2_header
 +/* Multiboot2 header checksum. */
 +.long   -(MULTIBOOT2_HEADER_MAGIC + MULTIBOOT2_ARCHITECTURE_I386 + \
 +(.Lmultiboot2_header_end - .Lmultiboot2_header))

So here ...

 +/* Multiboot2 tags... */
 +.Lmultiboot2_info_req:
 +/* Multiboot2 information request tag. */
 +.short  MULTIBOOT2_HEADER_TAG_INFORMATION_REQUEST
 +.short  MULTIBOOT2_HEADER_TAG_REQUIRED
 +.long   .Lmultiboot2_info_req_end - .Lmultiboot2_info_req

.. and here you properly calculate the length, while ...

 +.long   MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO
 +.long   MULTIBOOT2_TAG_TYPE_MMAP
 +.Lmultiboot2_info_req_end:
 +
 +.align  MULTIBOOT2_TAG_ALIGN
 +.short  MULTIBOOT2_HEADER_TAG_MODULE_ALIGN
 +.short  MULTIBOOT2_HEADER_TAG_REQUIRED
 +.long   8 /* Tag size. */

... here and further down you hard-code it. Please be consistent
(and if you go the calculation route, perhaps introduce some
redundancy reducing macro).

 +
 +/* Console flags tag. */
 +.align  MULTIBOOT2_TAG_ALIGN
 +.short  MULTIBOOT2_HEADER_TAG_CONSOLE_FLAGS
 +.short  MULTIBOOT2_HEADER_TAG_OPTIONAL
 +.long   12 /* Tag size. */
 +.long   MULTIBOOT2_CONSOLE_FLAGS_EGA_TEXT_SUPPORTED
 +
 +/* Framebuffer tag. */
 +.align  MULTIBOOT2_TAG_ALIGN
 +.short  MULTIBOOT2_HEADER_TAG_FRAMEBUFFER
 +.short  MULTIBOOT2_HEADER_TAG_OPTIONAL
 +.long   20 /* Tag size. */
 +.long   0 /* Number of the columns - no preference. */
 +.long   0 /* Number of the lines - no preference. */
 +.long   0 /* Number of bits per pixel - no preference. */
 +
 +/* Multiboot2 header end tag. */
 +.align  MULTIBOOT2_TAG_ALIGN
 +.short  MULTIBOOT2_HEADER_TAG_END
 +.short  0
 +.long   8 /* Tag size. */
 +.Lmultiboot2_header_end:
  
  .section .init.rodata, a, @progbits
  .align 4
 @@ -81,10 +135,51 @@ __start:
  mov %ecx,%es
  mov %ecx,%ss
  
 +/* Bootloaders may set multiboot[12].mem_lower to a nonzero value */

Above you meet coding style requirements, but here you stop to do
so (ongoing below)? Even if pre-existing neighboring comments aren't
well formed, please don't make the situation worse.

Also please don't say 12 here - I first even mistook this as an array
index, but you seem to mean 1 or 2. Please use {1,2} instead.

 +xor %edx,%edx
 +
  /* Check for Multiboot bootloader */
  cmp $MULTIBOOT_BOOTLOADER_MAGIC,%eax
 -jne not_multiboot
 +je  multiboot1_proto
  
 +/* Check for Multiboot2 bootloader */
 +cmp $MULTIBOOT2_BOOTLOADER_MAGIC,%eax
 +je  multiboot2_proto
 +
 +jmp not_multiboot
 +
 +multiboot1_proto:
 +/* Get mem_lower from Multiboot information */
 +testb   $MBI_MEMLIMITS,(%ebx)

MB_flags(%ebx)

 +jz  trampoline_setup/* not available? BDA value will be fine 
 */
 +
 +mov MB_mem_lower(%ebx),%edx

Please use cmovnz or or instead of jz and mov.

 +jmp trampoline_setup
 +
 +multiboot2_proto:
 +/* Skip Multiboot2 information fixed part */
 +lea MB2_fixed_sizeof(%ebx),%ecx
 +
 +0:
 +/* Get mem_lower from Multiboot2 information */
 +cmpl$MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO,(%ecx)
 +jne 1f
 +
 +mov MB2_mem_lower(%ecx),%edx
 +jmp trampoline_setup
 +
 +1:
 +/* Is it the end of Multiboot2 information? */
 +cmpl$MULTIBOOT2_TAG_TYPE_END,(%ecx)

This lacks the use a suitable MB2_* definition (even if that ends up
being zero).

 --- 

Re: [PATCH 05/18] efi: split efi_enabled to efi_platform and efi_loader

[Jan Beulich]

 On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote:
 We need more fine grained knowledge about EFI environment and check
 for EFI platform and EFI loader separately to properly support
 multiboot2 protocol.

... because of ... (i.e. I can't see from the description what the
separation is good for). Looking at the comments you placed
aside the variables doesn't help me either.

 In general Xen loaded by this protocol uses
 memory mappings and loaded modules in simliar way to Xen loaded
 by multiboot (v1) protocol. Hence, split efi_enabled to efi_platform
 and efi_loader.

And if I'm guessing things right, then introducing efi_loader but
leaving efi_enabled alone (only converting where needed) would
seem the right approach.

 --- a/xen/arch/x86/setup.c
 +++ b/xen/arch/x86/setup.c
 @@ -431,7 +431,7 @@ static void __init parse_video_info(void)
  struct boot_video_info *bvi = bootsym(boot_vid_info);
  
  /* The EFI loader fills vga_console_info directly. */
 -if ( efi_enabled )
 +if ( efi_platform )

This looks wrong considering the comment.

 @@ -663,7 +663,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
  if ( ((unsigned long)cpu0_stack  (STACK_SIZE-1)) != 0 )
  panic(Misaligned CPU0 stack.);
  
 -if ( efi_enabled )
 +if ( efi_loader )
  {
  set_pdx_range(xen_phys_start  PAGE_SHIFT,
(xen_phys_start + BOOTSTRAP_MAP_BASE)  PAGE_SHIFT);
 @@ -774,7 +774,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
   * we can relocate the dom0 kernel and other multiboot modules. Also, on
   * x86/64, we relocate Xen to higher memory.
   */
 -for ( i = 0; !efi_enabled  i  mbi-mods_count; i++ )
 +for ( i = 0; !efi_loader  i  mbi-mods_count; i++ )
  {
  if ( mod[i].mod_start  (PAGE_SIZE - 1) )
  panic(Bootloader didn't honor module alignment request.);
 @@ -962,7 +962,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
  
  if ( !xen_phys_start )
  panic(Not enough memory to relocate Xen.);
 -reserve_e820_ram(boot_e820, efi_enabled ? mbi-mem_upper : 
 __pa(_start),
 +reserve_e820_ram(boot_e820, efi_loader ? mbi-mem_upper : __pa(_start),

For all of these it's really hard to tell whether they're right without
knowing which parts of the current EFI loading code you intend to
re-use. Perhaps switching these would better be done along with
adding the re-using code (or splitting out the not re-used parts)?

 --- a/xen/include/xen/efi.h
 +++ b/xen/include/xen/efi.h
 @@ -5,7 +5,11 @@
  #include xen/types.h
  #endif
  
 -extern const bool_t efi_enabled;
 +/* If true then Xen runs on EFI platform. */
 +extern bool_t efi_platform;
 +
 +/* If true then Xen was loaded by native EFI loader as PE application. */
 +extern bool_t efi_loader;

I don't see why you're losing the constness.

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 07/18] efi: run EFI specific code on EFI platform only

[Jan Beulich]

 On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote:
 --- a/xen/arch/x86/shutdown.c
 +++ b/xen/arch/x86/shutdown.c
 @@ -504,7 +504,8 @@ void machine_restart(unsigned int delay_millisecs)
  tboot_shutdown(TB_SHUTDOWN_REBOOT);
  }
  
 -efi_reset_system(reboot_mode != 0);
 +if ( efi_platform )
 +efi_reset_system(reboot_mode != 0);

Please sync with Konrad on this one - it's supposed to get moved
and conditionalized anyway.

 --- a/xen/common/efi/boot.c
 +++ b/xen/common/efi/boot.c
 @@ -1154,6 +1154,11 @@ void __init efi_init_memory(void)
  } *extra, *extra_head = NULL;
  #endif
  
 +#ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */
 +if ( !efi_platform )
 +return;
 +#endif

At least for this one I'd rather see the call site to have the
conditional.

 --- a/xen/common/efi/runtime.c
 +++ b/xen/common/efi/runtime.c
 @@ -164,6 +164,9 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info)
  {
  unsigned int i, n;
  
 +if ( !efi_platform )
 +return -ENOSYS;
 +
  switch ( idx )
  {
  case XEN_FW_EFI_VERSION:
 @@ -298,6 +301,9 @@ int efi_runtime_call(struct xenpf_efi_runtime_call *op)
  EFI_STATUS status = EFI_NOT_STARTED;
  int rc = 0;
  
 +if ( !efi_platform )
 +return -ENOSYS;

EOPNOTSUPP in both cases please, consistent with when runtime
service use is disabled.

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2 08/23] x86: add multiboot2 protocol support

[Jan Beulich]

 On 20.07.15 at 16:29, daniel.ki...@oracle.com wrote:
 v2 - not fixed yet:
- dynamic dependency generation for xen/arch/x86/boot/reloc.S;
  this requires more work; I am not sure that it pays because
  potential patch requires more changes than addition of just
  multiboot2.h to Makefile
  (suggested by Jan Beulich),

With the compiler.h inclusion done I don't think this is as necessary
anymore as it was previously.

- isolated/stray __packed attribute usage for multiboot2_memory_map_t
  (suggested by Jan Beulich).

This one I don't, however, see why you didn't address. And with
un-addressed issues I think you should have tagged this RFC).

 @@ -19,6 +20,28 @@
  #define BOOT_PSEUDORM_CS 0x0020
  #define BOOT_PSEUDORM_DS 0x0028
  
 +#define MB2_HT(name)  (MULTIBOOT2_HEADER_TAG_##name)
 +#define MB2_TT(name)  (MULTIBOOT2_TAG_TYPE_##name)
 +
 +.macro mb2ht_args arg, args:vararg

Please make sure the oldest binutils we support allow for this
(introduced in 2.17; the oldest supported SLE seems to have it,
but I'm not sure about RHEL).

 +.long \arg
 +.ifnb \args
 +mb2ht_args \args
 +.endif
 +.endm
 +
 +.macro mb2ht_init type, req, args:vararg
 +.align MULTIBOOT2_TAG_ALIGN
 +0:

Labels belong in the first column. Also I generally recommend against
using numeric labels in macros (due to the risk of conflict with label
uses around the macro use sites) - use \@ instead to make labels
unique (again provided the oldest supported binutils handle this).

 @@ -119,10 +213,11 @@ __start:
  
  /* Save the Multiboot info struct (after relocation) for later use. 
 */
  mov $sym_phys(cpu0_stack)+1024,%esp
 +push%eax/* Multiboot magic. */
  push%ebx/* Multiboot information address. */
  push%ecx/* Boot trampoline address. */
  callreloc
 -add $8,%esp /* Remove reloc() args from stack. */
 +add $12,%esp/* Remove reloc() args from stack. */

The latest now it becomes clear that the arguments passed are
kind of backwards: One would expect the qualifying value (i.e.
the magic) to come first, then the info pointer, and last the
trampoline address.

 @@ -86,12 +104,12 @@ static u32 copy_string(u32 src)
  return copy_mem(src, p - src + 1);
  }
  
 -multiboot_info_t *reloc(u32 mbi_in)
 +static multiboot_info_t *mbi_mbi(void *mbi_in)

I don't see why this needs to become a pointer all of the sudden, ...

  {
  int i;
  multiboot_info_t *mbi_out;
  
 -mbi_out = (multiboot_info_t *)copy_mem(mbi_in, sizeof(*mbi_out));
 +mbi_out = (multiboot_info_t *)copy_mem((u32)mbi_in, sizeof(*mbi_out));

... adding back a cast that we'd better avoid here.

 @@ -127,3 +145,123 @@ multiboot_info_t *reloc(u32 mbi_in)
  
  return mbi_out;
  }
 +
 +static multiboot_info_t *mbi2_mbi(void *mbi_in)

Why not const multiboot2_fixed_t *?

 +{
 +/* Do not complain that mbi_out_mods is not initialized. */
 +module_t *mbi_out_mods = (module_t *)0;
 +memory_map_t *mmap_dst;
 +multiboot2_memory_map_t *mmap_src;

const

 +multiboot2_tag_t *tag;

const?

 +multiboot_info_t *mbi_out;
 +u32 ptr;
 +unsigned int i, mod_idx = 0;
 +
 +mbi_out = (multiboot_info_t *)alloc_mem(sizeof(*mbi_out));
 +zero_mem((u32)mbi_out, sizeof(*mbi_out));

Please avoid one of the two casts (e.g. by using ptr as intermediate
storage for the alloc_mem() result.

 +
 +/* Skip Multiboot2 information fixed part. */
 +tag = mbi_in + sizeof(multiboot2_fixed_t);
 +
 +for ( ; ; )
 +{
 +if ( tag-type == MULTIBOOT2_TAG_TYPE_MODULE )
 +++mbi_out-mods_count;
 +else if ( tag-type == MULTIBOOT2_TAG_TYPE_END )
 +{
 +mbi_out-flags = MBI_MODULES;
 +mbi_out-mods_addr = alloc_mem(mbi_out-mods_count * 
 sizeof(module_t));
 +mbi_out_mods = (module_t *)mbi_out-mods_addr;

Even if we don't expect Xen to be booted without any modules,
wouldn't you better do this only when mbi_out-mods_count  0?

 +break;
 +}
 +
 +/* Go to next Multiboot2 information tag. */
 +tag = (multiboot2_tag_t *)(ALIGN_UP((u32)tag + tag-size, 
 MULTIBOOT2_TAG_ALIGN));

Please drop the stray pair of parentheses around the invocation of
ALIGN_UP(). Also - long line.

 +/* Check Multiboot2 information total size just in case. */
 + if ( (void *)tag - mbi_in = ((multiboot2_fixed_t *)mbi_in)-total_size 
 )

Hard tab.

 +break;
 +}
 +
 +/* Skip Multiboot2 information fixed part. */
 +tag = mbi_in + sizeof(multiboot2_fixed_t);
 +
 +for ( ; ; )
 +{
 +switch ( tag-type )
 +{
 +case MULTIBOOT2_TAG_TYPE_BOOT_LOADER_NAME:
 +mbi_out-flags |= MBI_LOADERNAME;
 +ptr = (u32)get_mb2_data(tag, multiboot2_tag_string_t

Re: [PATCH v2 05/23] x86/boot/reloc: create generic alloc and copy functions

[Jan Beulich]

 On 20.07.15 at 16:29, daniel.ki...@oracle.com wrote:
 Create generic alloc and copy functions. We need
 separate tools for memory allocation and copy to
 provide multiboot2 protocol support.
 
 Signed-off-by: Daniel Kiper daniel.ki...@oracle.com
 Reviewed-by: Andrew Cooper andrew.coop...@citrix.com
 ---
 v2 - suggestions/fixes:
- generalize new functions names
  (suggested by Jan Beulich),
- reduce number of casts
  (suggested by Jan Beulich).

This contradicts retaining Andrew's R-b tag. Please remember to
drop tags for everything you make non-trivial changes to.

 @@ -55,50 +56,64 @@ static void *reloc_mbi_struct(void *old, unsigned int 
 bytes)
  sub  %1,%0   \n
  and  $~15,%0 \n
  mov  %0,alloc-1b(%%edx)  \n
 -mov  %0,%%edi\n
 -rep  movsb   \n
 -   : =r (new), +c (bytes), +S (old)
 - : : edx, edi, memory);
 -return new;
 +   : =r (s) : r (bytes) : edx, memory);

Can't bytes use a simple g constraint now?

Preferably (i.e. if correct) this changed
Acked-by: Jan Beulich jbeul...@suse.com

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2 07/23] x86/boot/reloc: Rename some variables and rearrange code a bit

[Jan Beulich]

 On 20.07.15 at 16:29, daniel.ki...@oracle.com wrote:
 Rename mbi and mbi_old variables and rearrange code a bit to make
 it more readable. Additionally, this way multiboot (v1) protocol
 implementation and future multiboot2 protocol implementation will
 use the same variable naming convention.
 
 Signed-off-by: Daniel Kiper daniel.ki...@oracle.com

Acked-by: Jan Beulich jbeul...@suse.com


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2 09/23] efi: create efi_enabled()

[Jan Beulich]

 On 22.08.15 at 14:33, daniel.ki...@oracle.com wrote:
 On Thu, Aug 20, 2015 at 09:18:17AM -0600, Jan Beulich wrote:
  On 20.07.15 at 16:29, daniel.ki...@oracle.com wrote:
  --- a/xen/arch/x86/efi/stub.c
  +++ b/xen/arch/x86/efi/stub.c
  @@ -4,9 +4,14 @@
   #include xen/lib.h
   #include asm/page.h
 
  -#ifndef efi_enabled
  -const bool_t efi_enabled = 0;
  -#endif
  +struct efi __read_mostly efi = {
  +  .flags   = 0, /* Initialized later. */
  +  .acpi= EFI_INVALID_TABLE_ADDR,
  +  .acpi20  = EFI_INVALID_TABLE_ADDR,
  +  .mps = EFI_INVALID_TABLE_ADDR,
  +  .smbios  = EFI_INVALID_TABLE_ADDR,
  +  .smbios3 = EFI_INVALID_TABLE_ADDR
  +};

 How is this change related to the subject of the patch?
 
 I need to add this struct because...
 
  --- a/xen/arch/x86/xen.lds.S
  +++ b/xen/arch/x86/xen.lds.S
  @@ -191,8 +191,6 @@ SECTIONS
 .pad : {
   . = ALIGN(MB(16));
 } :text
  -#else
  -  efi = .;
   #endif

 Same here.
 
 ...this creates efi symbol to just satisfy linker and I am removing it.
 However, existing solution does not allocate space for this symbol and
 any references to acpi20, etc. does not make sense. As I saw any efi.*
 references are protected by relevant ifs but we should not do that because
 it makes code very fragile. If somebody does not know how efi symbol is
 created he/she may assume that it always represent valid structure and
 do invalid references somewhere. So, I still think that stub.c should
 define efi struct properly even if we assume that flags should not be
 there. However, I agree that this could be separate patch.
 
 By the way why did you choose so strange way to satisfy liker needs?

To me there's nothing strange about it: I want a symbol that
occupies no space in memory.

  --- a/xen/common/efi/boot.c
  +++ b/xen/common/efi/boot.c
  @@ -717,6 +717,10 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE
  *SystemTable)
   char *option_str;
   bool_t use_cfg_file;
 
  +#ifndef CONFIG_ARM /* Disabled until runtime services implemented. */
  +set_bit(EFI_PLATFORM, efi.flags);
  +#endif

 Just for this to work? I don't see the need for all the pointers in
 the stub case - why can't this be a separate variable? We don't
 
 Could be but if we create struct with so generic name like just simple
 efi it suggest that this is good place to put flags there. If it is not
 how to call it? efi_flags? Or maybe we should rename efi to efi_tables
 too. Then everything will be clear.

I agree that this may be matter of taste, but to me the current
naming looks quite fine. And yes, efi_flags of efi_state would be
a fine name. In general I wouldn't even mind it to be a field in
the structure, if only that resulted in the _full_ structure to be
allocated even in the no-EFI build case. I admit though that with
the goal of always building EFI code (unless the tool chain
doesn't support doing so) this becomes less of an issue; otoh us
probably wanting some Kconfig-like mechanism sooner or later to
{en,dis}able certain features would call for this to remain separable.
And yes, at that point it could be done by #ifdef-ing out everything
by the flags member.

So based on the above I'm withdrawing my implied objection, but
please make sure you write better patch descriptions explaining
what is done and _why_.

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2 10/23] efi: build xen.gz with EFI code

[Jan Beulich]

 On 22.08.15 at 15:59, daniel.ki...@oracle.com wrote:
 On Thu, Aug 20, 2015 at 09:39:39AM -0600, Jan Beulich wrote:
  On 20.07.15 at 16:29, daniel.ki...@oracle.com wrote:
  Build xen.gz with EFI code. We need this to support multiboot2
  protocol on EFI platforms.
 
  If we wish to load not ELF file using multiboot (v1) or multiboot2 then

 DYM a non-ELF file?

  it must contain linear (or flat) representation of code and data.

 Why? Please don't just put out statements, but also reasons (i.e.
 at least which component is unable to deal with the current [valid
 afaict] PE image we have).
 
 This is a requirement of multiboot (v1) or multiboot2 protocol. They both
 know nothing about PE image format.

And hence how specifically we arrange data inside the image should
be benign to them, as they won't be able to load the file _anyway_.

  Currently, PE file contains many sections which are not linear (one
  after another without any holes) or even do not have representation
  in a file (e.g. BSS). In theory there is a chance that we could build
  proper PE file using current build system. However, it means that

 What is improper about the currently built PE file? And if there is
 anything improper, did you inform the binutils maintainers of the
 problem?
 
 From PE loader point of view everything is OK. However, current Xen PE
 image (at least build on my machines) is not usable by multiboot (v1)
 or multiboot2 protocol compatible loader because it is not linear (one
 section does not live immediately after another without any voids).

Again - either I'm missing something (and then your explanation is
not good enough) or this is (as said above) a pointless adjustment.

  --- a/xen/arch/x86/efi/Makefile
  +++ b/xen/arch/x86/efi/Makefile
  @@ -1,14 +1,16 @@
   CFLAGS += -fshort-wchar
 
  -obj-y += stub.o
  -
  -create = test -e $(1) || touch -t 19990101 $(1)
  -
   efi := $(filter y,$(x86_64)$(shell rm -f disabled))
   efi := $(if $(efi),$(shell $(CC) $(filter-out $(CFLAGS-y) .%.d,$(CFLAGS)) 
  -c check.c 2disabled  echo y))
   efi := $(if $(efi),$(shell $(LD) -mi386pep --subsystem=10 -o check.efi 
  check.o disabled  echo y))
  -efi := $(if $(efi),$(shell rm disabled)y,$(shell $(call 
  create,boot.init.o); $(call create,runtime.o)))
  +efi := $(if $(efi),$(shell rm disabled)y)
 
  -extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o
  +extra-y += relocs-dummy.o

 Why is this no longer extra-$(efi)?
 
 Because we need proper EFI code in xen.gz to support boot
 via multiboot2 on EFI platforms.

What would we need that for when not building an EFI-capable
binary anyway?

  -stub.o: $(extra-y)

 With this dependency removed (instead of perhaps replaced or
 extended) - what will trigger relocs-dummy.o to be (re)built?
 
 It is triggered by prelink.o build rule in xen/arch/x86/Makefile.

No. Or better - if it is, then this is wrong. With the way our build
logic works, unless there are exceptional circumstances things in
a certain directory should be built _only_ when recursion reaches
that particular directory (i.e. not from any of its parents).

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2 04/23] x86/boot: call reloc() using cdecl calling convention

[Jan Beulich]

 On 20.07.15 at 16:28, daniel.ki...@oracle.com wrote:

An empty description leaves us guess at the why.

 --- a/xen/arch/x86/boot/reloc.c
 +++ b/xen/arch/x86/boot/reloc.c
 @@ -10,15 +10,27 @@
   *Keir Fraser k...@xen.org
   */
  
 -/* entered with %eax = BOOT_TRAMPOLINE */
 +/*
 + * This entry point is entered from xen/arch/x86/boot/head.S with:
 + *   - 0x4(%esp) = BOOT_TRAMPOLINE_ADDRESS,
 + *   - 0x8(%esp) = MULTIBOOT_INFORMATION_ADDRESS.
 + */
  asm (
  .text \n
  .globl _start \n
  _start:   \n
 +push %ebp \n
 +mov  %esp,%ebp\n

What do you need this for?

  call 1f   \n
 -1:  pop  %ebx \n
 -mov  %eax,alloc-1b(%ebx)  \n
 -jmp  reloc\n
 +1:  pop  %ecx \n
 +mov  0x8(%ebp),%eax   \n
 +mov  %eax,alloc-1b(%ecx)  \n
 +mov  0xc(%ebp),%eax   \n
 +push %eax \n

push 12(%ebp)

 +call reloc\n
 +add  $4,%esp  \n

If you already copy %esp to %ebp at the top,

mov %ebp, %esp

would be the better alternative.

 +pop  %ebp \n

Or even just

leave

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2 08/23] x86: add multiboot2 protocol support

[Jan Beulich]

 On 18.08.15 at 14:00, daniel.ki...@oracle.com wrote:
 On Tue, Aug 18, 2015 at 02:12:58AM -0600, Jan Beulich wrote:
  On 20.07.15 at 16:29, daniel.ki...@oracle.com wrote:
  @@ -119,10 +213,11 @@ __start:
 
   /* Save the Multiboot info struct (after relocation) for later 
  use. */
   mov $sym_phys(cpu0_stack)+1024,%esp
  +push%eax/* Multiboot magic. */
   push%ebx/* Multiboot information address. */
   push%ecx/* Boot trampoline address. */
   callreloc
  -add $8,%esp /* Remove reloc() args from stack. */
  +add $12,%esp/* Remove reloc() args from stack. */

 The latest now it becomes clear that the arguments passed are
 kind of backwards: One would expect the qualifying value (i.e.
 the magic) to come first, then the info pointer, and last the
 trampoline address.
 
 Yep, you are right. However, I wanted to avoid changing order of arguments
 to not confuse reader. If you wish I can change that thing.

Yes please: Between confusing the reader of the patch and the reader
of the resulting code, the former is the less problematic one. Furthermore
with the function having just one argument before your series you have
all options to avoid confusing the reader of the patches - just insert the
new arguments at the right slot in each patch adding one.

  +case MULTIBOOT2_TAG_TYPE_MMAP:
  +mbi_out-flags |= MBI_MEMMAP;
  +mbi_out-mmap_length = get_mb2_data(tag, 
  multiboot2_tag_mmap_t, size);
  +mbi_out-mmap_length -= sizeof(multiboot2_tag_mmap_t);
  +mbi_out-mmap_length += 
  sizeof(((multiboot2_tag_mmap_t){0}).entries);

 Afaict this sizeof() evaluates to zero. And even if it didn't I can't see
 what the line is good for.
 
 Right, I have missed that. However, if it does not evaluate to zero then
 you must add back relevant number of entries which you subtracted one
 line above. Otherwise count of entries will be incorrect.

There's no count being subtracted afaict - what is being subtracted
is the size of the rest of the structure.

 Dropping the static would permit to also drop the used attribute.
 Since it was that way before, why didn't you keep it that way?
 
 Yep, but I do not like this solution. Lack of static suggests that this 
 function
 is used elsewhere. I prefer to explicitly say that there are not external 
 users
 of that function and silence compiler warnings with __used.

If you want to do this, then not in the middle of some already big
patch doing something completely different, but in a separate
cleanup patch explaining what you explained above (which is not
to say that I'm convinced, and hence I can't promise such a change
to ultimately go in).

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2 14/23] efi: split out efi_find_gop_mode()

[Jan Beulich]

 On 20.07.15 at 16:29, daniel.ki...@oracle.com wrote:
 --- a/xen/common/efi/boot.c
 +++ b/xen/common/efi/boot.c
 @@ -665,6 +665,58 @@ static EFI_GRAPHICS_OUTPUT_PROTOCOL __init 
 *efi_get_gop(void)
  return gop;
  }
  
 +static UINTN __init efi_find_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
 +  UINTN cols, UINTN rows, UINTN depth)
 +{
 +EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info;
 +EFI_STATUS status;
 +UINTN gop_mode = ~0, info_size, size;
 +unsigned int i;
 +
 +if ( !gop )
 +return gop_mode;

Please drop this in favor of ...

 @@ -978,46 +1030,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
 *SystemTable)
  
  dir_handle-Close(dir_handle);
  
 -if ( gop  !base_video )

... keeping the original check here.

Similarly in patch 17.

With these minor adjustments, all the efi: split out efi_... patches
Acked-by: Jan Beulich jbeul...@suse.com

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2 09/23] efi: create efi_enabled()

[Jan Beulich]

 On 20.07.15 at 16:29, daniel.ki...@oracle.com wrote:
 --- a/xen/arch/x86/efi/stub.c
 +++ b/xen/arch/x86/efi/stub.c
 @@ -4,9 +4,14 @@
  #include xen/lib.h
  #include asm/page.h
  
 -#ifndef efi_enabled
 -const bool_t efi_enabled = 0;
 -#endif
 +struct efi __read_mostly efi = {
 + .flags   = 0, /* Initialized later. */
 + .acpi= EFI_INVALID_TABLE_ADDR,
 + .acpi20  = EFI_INVALID_TABLE_ADDR,
 + .mps = EFI_INVALID_TABLE_ADDR,
 + .smbios  = EFI_INVALID_TABLE_ADDR,
 + .smbios3 = EFI_INVALID_TABLE_ADDR
 +};

How is this change related to the subject of the patch?

 --- a/xen/arch/x86/xen.lds.S
 +++ b/xen/arch/x86/xen.lds.S
 @@ -191,8 +191,6 @@ SECTIONS
.pad : {
  . = ALIGN(MB(16));
} :text
 -#else
 -  efi = .;
  #endif

Same here.

 --- a/xen/common/efi/boot.c
 +++ b/xen/common/efi/boot.c
 @@ -717,6 +717,10 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
 *SystemTable)
  char *option_str;
  bool_t use_cfg_file;
  
 +#ifndef CONFIG_ARM /* Disabled until runtime services implemented. */
 +set_bit(EFI_PLATFORM, efi.flags);
 +#endif

Just for this to work? I don't see the need for all the pointers in
the stub case - why can't this be a separate variable? We don't
need to follow Linux with where the flags actually live...

 --- a/xen/common/efi/runtime.c
 +++ b/xen/common/efi/runtime.c
 @@ -10,14 +10,10 @@ DEFINE_XEN_GUEST_HANDLE(CHAR16);
  
  #ifndef COMPAT
  
 -#ifdef CONFIG_ARM  /* Disabled until runtime services implemented */
 -const bool_t efi_enabled = 0;
 -#else
 +#ifndef CONFIG_ARM
  # include asm/i387.h
  # include asm/xstate.h
  # include public/platform.h
 -
 -const bool_t efi_enabled = 1;
  #endif

Afaict the proper replacement (at least at this point in the series) for
this would be to statically initialize the flag set in this xen.efi case.

 @@ -40,6 +42,16 @@ int efi_runtime_call(struct xenpf_efi_runtime_call *);
  int efi_compat_get_info(uint32_t idx, union compat_pf_efi_info *);
  int efi_compat_runtime_call(struct compat_pf_efi_runtime_call *);
  
 +/*
 + * Test whether the above EFI_* bits are enabled.
 + *
 + * Stolen from Linux Kernel.

I don't think this second part of the comment makes a lot of sense
for a minor piece of functionality like this.

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2 10/23] efi: build xen.gz with EFI code

[Jan Beulich]

 On 20.07.15 at 16:29, daniel.ki...@oracle.com wrote:
 Build xen.gz with EFI code. We need this to support multiboot2
 protocol on EFI platforms.
 
 If we wish to load not ELF file using multiboot (v1) or multiboot2 then

DYM a non-ELF file?

 it must contain linear (or flat) representation of code and data.

Why? Please don't just put out statements, but also reasons (i.e.
at least which component is unable to deal with the current [valid
afaict] PE image we have).

 Currently, PE file contains many sections which are not linear (one
 after another without any holes) or even do not have representation
 in a file (e.g. BSS). In theory there is a chance that we could build
 proper PE file using current build system. However, it means that

What is improper about the currently built PE file? And if there is
anything improper, did you inform the binutils maintainers of the
problem?

 --- a/xen/arch/x86/efi/Makefile
 +++ b/xen/arch/x86/efi/Makefile
 @@ -1,14 +1,16 @@
  CFLAGS += -fshort-wchar
  
 -obj-y += stub.o
 -
 -create = test -e $(1) || touch -t 19990101 $(1)
 -
  efi := $(filter y,$(x86_64)$(shell rm -f disabled))
  efi := $(if $(efi),$(shell $(CC) $(filter-out $(CFLAGS-y) .%.d,$(CFLAGS)) -c 
 check.c 2disabled  echo y))
  efi := $(if $(efi),$(shell $(LD) -mi386pep --subsystem=10 -o check.efi 
 check.o 2disabled  echo y))
 -efi := $(if $(efi),$(shell rm disabled)y,$(shell $(call create,boot.init.o); 
 $(call create,runtime.o)))
 +efi := $(if $(efi),$(shell rm disabled)y)
  
 -extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o
 +extra-y += relocs-dummy.o

Why is this no longer extra-$(efi)?

 -stub.o: $(extra-y)

With this dependency removed (instead of perhaps replaced or
extended) - what will trigger relocs-dummy.o to be (re)built?

 +ifeq ($(efi),y)
 +obj-y += boot.init.o
 +obj-y += compat.o
 +obj-y += runtime.o
 +else
 +obj-y += stub.o
 +endif

obj-y := stub.o
obj-$(efi) := boot.init.o compat.o runtime.o

 --- a/xen/arch/x86/mm.c
 +++ b/xen/arch/x86/mm.c
 @@ -344,7 +344,8 @@ void __init arch_init_memory(void)
  
  subarch_init_memory();
  
 -efi_init_memory();
 +if ( efi_enabled(EFI_PLATFORM) )
 +efi_init_memory();

I think I'd prefer such checks to be constrained to EFI code where
possible, i.e. in this case do the check inside efi_init_memory().

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [Xen-devel] [PATCH v2 22/23] x86: make Xen early boot code relocatable

[Jan Beulich]

 On 14.08.15 at 15:59, daniel.ki...@oracle.com wrote:
 On Fri, Aug 14, 2015 at 06:49:18AM -0600, Jan Beulich wrote:
  On 14.08.15 at 13:52, daniel.ki...@oracle.com wrote:
  On Tue, Aug 11, 2015 at 12:48:06PM -0400, Konrad Rzeszutek Wilk wrote:
  On Mon, Jul 20, 2015 at 04:29:17PM +0200, Daniel Kiper wrote:
   diff --git a/xen/include/asm-x86/page.h b/xen/include/asm-x86/page.h
   index 87b3341..27481ac 100644
   --- a/xen/include/asm-x86/page.h
   +++ b/xen/include/asm-x86/page.h
   @@ -283,7 +283,7 @@ extern root_pgentry_t 
 idle_pg_table[ROOT_PAGETABLE_ENTRIES];
extern l2_pgentry_t  *compat_idle_pg_table_l2;
extern unsigned int   m2p_compat_vstart;
extern l2_pgentry_t l2_xenmap[L2_PAGETABLE_ENTRIES],
   -l2_bootmap[L2_PAGETABLE_ENTRIES];
   +l2_bootmap[4*L2_PAGETABLE_ENTRIES];
 
  ? Why do we need to expand this to be 16kB?
 
  TBH, we need 8 KiB in the worst case. The worst case is when
  next GiB starts (e.g. 1 GiB ends and 2 GiB starts) in the middle
  of Xen image. In this situation we must hook up lower l2_bootmap
  table with lower l3_bootmap entry, higher l2_bootmap table with
  higher l3_bootmap entry and finally fill l2_bootmap relevant
  tables in proper way. Sadly, this method requires more calculations.
  To avoid that I have added 3 l2_bootmap tables and simply hook up
  one after another with relevant l3_bootmap entries. However, if
  you wish we can reduce number of l2_bootmap tables to two. This
  way code will be more complicated but we will save about 8 KiB.

 Wouldn't it be better (simpler) to enforce, say, 16Mb alignment
 in the PE32+ header (which the EFI loader would then honor)?
 
 Good idea but then we must enforce this for multiboot protocol (v1 and v2) 
 too.
 multiboot2 with my patches supports that solution. However, multiboot (v1) 
 could
 be a bit problematic because it means that we must set load address to 16 
 MiB.
 Are we sure that this region is available on all machines like region 
 starting
 at 1 MiB?

This region being which one?

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [Xen-devel] [PATCH v2 22/23] x86: make Xen early boot code relocatable

[Jan Beulich]

 On 14.08.15 at 16:37, daniel.ki...@oracle.com wrote:
 On Fri, Aug 14, 2015 at 08:32:05AM -0600, Jan Beulich wrote:
  On 14.08.15 at 15:59, daniel.ki...@oracle.com wrote:
  On Fri, Aug 14, 2015 at 06:49:18AM -0600, Jan Beulich wrote:
   On 14.08.15 at 13:52, daniel.ki...@oracle.com wrote:
   On Tue, Aug 11, 2015 at 12:48:06PM -0400, Konrad Rzeszutek Wilk wrote:
   On Mon, Jul 20, 2015 at 04:29:17PM +0200, Daniel Kiper wrote:
diff --git a/xen/include/asm-x86/page.h b/xen/include/asm-x86/page.h
index 87b3341..27481ac 100644
--- a/xen/include/asm-x86/page.h
+++ b/xen/include/asm-x86/page.h
@@ -283,7 +283,7 @@ extern root_pgentry_t
  idle_pg_table[ROOT_PAGETABLE_ENTRIES];
 extern l2_pgentry_t  *compat_idle_pg_table_l2;
 extern unsigned int   m2p_compat_vstart;
 extern l2_pgentry_t l2_xenmap[L2_PAGETABLE_ENTRIES],
-l2_bootmap[L2_PAGETABLE_ENTRIES];
+l2_bootmap[4*L2_PAGETABLE_ENTRIES];
  
   ? Why do we need to expand this to be 16kB?
  
   TBH, we need 8 KiB in the worst case. The worst case is when
   next GiB starts (e.g. 1 GiB ends and 2 GiB starts) in the middle
   of Xen image. In this situation we must hook up lower l2_bootmap
   table with lower l3_bootmap entry, higher l2_bootmap table with
   higher l3_bootmap entry and finally fill l2_bootmap relevant
   tables in proper way. Sadly, this method requires more calculations.
   To avoid that I have added 3 l2_bootmap tables and simply hook up
   one after another with relevant l3_bootmap entries. However, if
   you wish we can reduce number of l2_bootmap tables to two. This
   way code will be more complicated but we will save about 8 KiB.
 
  Wouldn't it be better (simpler) to enforce, say, 16Mb alignment
  in the PE32+ header (which the EFI loader would then honor)?
 
  Good idea but then we must enforce this for multiboot protocol (v1 and v2)
  too.
  multiboot2 with my patches supports that solution. However, multiboot (v1)
  could
  be a bit problematic because it means that we must set load address to 16
  MiB.
  Are we sure that this region is available on all machines like region
  starting
  at 1 MiB?

 This region being which one?
 
 16 MiB - 32 MiB.

While I think all systems where Xen can reasonably run on would
have memory in that range, I'd really prefer not to touch the MB1
case (i.e. find a way for it to continue to load at 1Mb). Perhaps
the 16Mb alignment could be specified only in the PE32+ header,
but not enforced in the ELF one?

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [Xen-devel] [PATCH v2 22/23] x86: make Xen early boot code relocatable

[Jan Beulich]

 On 14.08.15 at 13:52, daniel.ki...@oracle.com wrote:
 On Tue, Aug 11, 2015 at 12:48:06PM -0400, Konrad Rzeszutek Wilk wrote:
 On Mon, Jul 20, 2015 at 04:29:17PM +0200, Daniel Kiper wrote:
  diff --git a/xen/include/asm-x86/page.h b/xen/include/asm-x86/page.h
  index 87b3341..27481ac 100644
  --- a/xen/include/asm-x86/page.h
  +++ b/xen/include/asm-x86/page.h
  @@ -283,7 +283,7 @@ extern root_pgentry_t 
  idle_pg_table[ROOT_PAGETABLE_ENTRIES];
   extern l2_pgentry_t  *compat_idle_pg_table_l2;
   extern unsigned int   m2p_compat_vstart;
   extern l2_pgentry_t l2_xenmap[L2_PAGETABLE_ENTRIES],
  -l2_bootmap[L2_PAGETABLE_ENTRIES];
  +l2_bootmap[4*L2_PAGETABLE_ENTRIES];

 ? Why do we need to expand this to be 16kB?
 
 TBH, we need 8 KiB in the worst case. The worst case is when
 next GiB starts (e.g. 1 GiB ends and 2 GiB starts) in the middle
 of Xen image. In this situation we must hook up lower l2_bootmap
 table with lower l3_bootmap entry, higher l2_bootmap table with
 higher l3_bootmap entry and finally fill l2_bootmap relevant
 tables in proper way. Sadly, this method requires more calculations.
 To avoid that I have added 3 l2_bootmap tables and simply hook up
 one after another with relevant l3_bootmap entries. However, if
 you wish we can reduce number of l2_bootmap tables to two. This
 way code will be more complicated but we will save about 8 KiB.

Wouldn't it be better (simpler) to enforce, say, 16Mb alignment
in the PE32+ header (which the EFI loader would then honor)?

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [Xen-devel] [PATCH v2 08/23] x86: add multiboot2 protocol support

[Jan Beulich]

 On 13.08.15 at 21:22, daniel.ki...@oracle.com wrote:
 On Mon, Aug 10, 2015 at 03:17:48PM -0400, Konrad Rzeszutek Wilk wrote:
 On Mon, Jul 20, 2015 at 04:29:03PM +0200, Daniel Kiper wrote:
  @@ -34,6 +57,42 @@ multiboot1_header_start:   /*** MULTIBOOT1 HEADER 
  /
   .long   -(MULTIBOOT_HEADER_MAGIC + MULTIBOOT_HEADER_FLAGS)
   multiboot1_header_end:
 
  +/*** MULTIBOOT2 HEADER /
  +/* Some ideas are taken from grub-2.00/grub-core/tests/boot/kernel-i386.S 
  file. */
  +.align  MULTIBOOT2_HEADER_ALIGN
  +
  +.Lmultiboot2_header:

 How come you use .L? It makes this hidden while the multiboot1 headers
 are visible? Makes it a bit harder to see the contents of this under
 an debugger.
 
 Good point. IIRC, Jan asked about that. I will remove .L if he does not 
 object.

For this particular one I think it's okay to drop the .L, but generally
I'd like to see .L used more widely for any auxiliary labels (i.e. only
main labels - function entry points and data objects - should have
their labels present in the final symbol table).

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2 00/23] x86: multiboot2 protocol support

[Jan Beulich]

 On 20.07.15 at 16:28, daniel.ki...@oracle.com wrote:
 This series, in general, is not targeted to Xen 4.6. However, there are
 some fixes at the beginning of it which are worth considering, I think.

I looked at the first few, and didn't spot any fixes. If you meant
just cleanup or other cosmetic adjustments, then I don't view them
as candidates for going in now. If I overlooked some aspect, then
please be more precise.

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [Xen-devel] Uniform commands for booting xen

[Jan Beulich]

>>> On 12.11.15 at 18:09, <ian.campb...@citrix.com> wrote:
> On Thu, 2015-11-12 at 08:44 -0700, Jan Beulich wrote:
>> > > > On 12.11.15 at 14:41, <phco...@gmail.com> wrote:
>> > Hello, all. I'd like to have set of commands that would boot xen on all
>> > platforms. I thought of following set:
>> > 
>> > xen_hypervisor FILE XEN_OPTIONS
>> > xen_kernel FILE KERNEL_OPTIONS
>> > xen_initrd INITRD INITRD INITRD
>> > all initrds are concatenated.
>> > xen_xsm ???
>> 
>> xen_ucode (and we might add more going forward). I don't see
>> why the multiboot mechanism (kernel plus any number of modules)
>> can't be used, without adding any Xen-specific directives.
> 
> You likely aren't aware that on ARM Xen doesn't boot via multiboot, but via
> a protocol which involves passing modules in an fdt[0].
> 
> I had originally hoped that this would use the same command names in the
> grub cfg, such that things would just work, however the grub maintainers
> didn't like that (and I appreciate why).
> 
> Hence on grub/ARM we already have xen_{hypervisor,kernel,initrd,...}.
> 
> The question then is what grub-mkconfig (or more precisely
> /etc/grub.d/20_linux_xen) ought to emit so that things just work on all
> architectures.
> 
> The author of the grub/ARM/Xen patches initially made it generate the xen_*
> namas for arm and the multiboot names for x86, here is Vladimir's feedback
> on that: http://lists.gnu.org/archive/html/grub-devel/2015-10/msg00133.html 
> 
> Which I think gets us to approximately today and Vladimir's question.

Now that makes the situation really ugly (and supports my
reservations regarding grub2 as a uniform solution for everything).
How do you express modules other than kernel+initrd in that
scheme, without grub needing to be aware of any new addition we
may find necessary going forward?

I think any architecture following a well defined, cross-arch
protocol (like multiboot) should not require any special xen_*
directives. If ARM64 needs Xen to be treated specially, special
directives are maybe warranted for this particular case, but I don't
see why all architectures supporting Xen should then automatically
have to use those too. But yes, it's not my call decide this...

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [Xen-devel] Uniform commands for booting xen

[Jan Beulich]

>>> On 12.11.15 at 14:41,  wrote:
> Hello, all. I'd like to have set of commands that would boot xen on all
> platforms. I thought of following set:
> 
> xen_hypervisor FILE XEN_OPTIONS
> xen_kernel FILE KERNEL_OPTIONS
> xen_initrd INITRD INITRD INITRD
> all initrds are concatenated.
> xen_xsm ???

xen_ucode (and we might add more going forward). I don't see
why the multiboot mechanism (kernel plus any number of modules)
can't be used, without adding any Xen-specific directives.

Jan

> On arm64 it would use the arm64 xen FDT protocol but on x86 should we
> use multiboot2 if multiboot2 header is present and multiboot otherwise?
> Or do xen devs have other preferences?




___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [Xen-devel] Uniform commands for booting xen

[Jan Beulich]

>>> On 12.11.15 at 17:58, <arvidj...@gmail.com> wrote:
> 12.11.2015 18:44, Jan Beulich пишет:
>>>>> On 12.11.15 at 14:41, <phco...@gmail.com> wrote:
>>> Hello, all. I'd like to have set of commands that would boot xen on all
>>> platforms. I thought of following set:
>>>
>>> xen_hypervisor FILE XEN_OPTIONS
>>> xen_kernel FILE KERNEL_OPTIONS
>>> xen_initrd INITRD INITRD INITRD
>>> all initrds are concatenated.
>>> xen_xsm ???
>>
>> xen_ucode (and we might add more going forward). I don't see
>> why the multiboot mechanism (kernel plus any number of modules)
>> can't be used, without adding any Xen-specific directives.
>>
> 
> Loader commands traditionally reflected underlying protocol. Using 
> multiboot command in this case would be confusing as this is not 
> multiboot.

Why would it not be multiboot?

Jan

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2 21/23] x86/boot: implement early command line parser in C

[Jan Beulich]

 On 20.07.15 at 16:29, daniel.ki...@oracle.com wrote:
 Current early command line parser implementation in assembler
 is very difficult to change to relocatable stuff using segment
 registers. This requires a lot of changes in very weird and
 fragile code. So, reimplement this functionality in C. This
 way code will be relocatable out of the box and much easier
 to maintain.

All appreciated and nice, but the goal of making the code
relocatable by playing with segment registers sounds fragile:
This breaks assumptions the compiler may validly make.

  xen/arch/x86/boot/cmdline.S|  367 -
  xen/arch/x86/boot/cmdline.c|  396 
 

A fundamental expectation I would have had is for the C file to be
noticeably smaller than the assembly file.

 --- /dev/null
 +++ b/xen/arch/x86/boot/cmdline.c
[...]
 +#define VESA_WIDTH   0
 +#define VESA_HEIGHT  1
 +#define VESA_DEPTH   2
 +
 +#define VESA_SIZE3

These should go away in favor of using individual (sub)structure fields.

 +#define __cdecl  __attribute__((__cdecl__))

???

 +#define __packed __attribute__((__packed__))
 +#define __text   __attribute__((__section__(.text)))
 +#define __used   __attribute__((__used__))

Likely better to include compiler.h instead.

 +#define max(x,y) ({ \
 +const typeof(x) _x = (x);   \
 +const typeof(y) _y = (y);   \
 +(void) (_x == _y);\
 +_x  _y ? _x : _y; })

I also wonder whether -imacros .../xen/kernel.h wouldn't be a better
approach here. Please really think hard on how to avoid duplications
like these.

 +#define strlen_static(s) (sizeof(s) - 1)

What is this good for? A decent compiler should be able to deal with
strlen(...). Plus your macro is longer that what it tries to abbreviate.

 +static const char empty_chars[] __text =  \n\r\t;

What is empty about them? DYM blank or (white) space or separator
or delimiter? I also wonder whether \n and \r are actually usefully here,
as they should (if at all) only end the line.

 +/**
 + * strlen - Find the length of a string
 + * @s: The string to be sized
 + */
 +static size_t strlen(const char *s)

Comments are certainly nice, but in this special case I'd rather suggest
against bloating the code by commenting standard library functions.

 +static int strtoi(const char *s, const char *stop, const char **next)
 +{
 +int base = 10, i, ores = 0, res = 0;
 +
 +if ( *s == '0' )
 +  base = (tolower(*++s) == 'x') ? (++s, 16) : 8;
 +
 +for ( ; *s != '\0'; ++s )
 +{
 +for ( i = 0; stop  stop[i] != '\0'; ++i )
 +if ( *s == stop[i] )
 +goto out;
 +
 +if ( *s  '0' || (*s  '7'  base == 8) )
 +{
 +res = -1;
 +goto out;
 +}
 +
 +if ( *s  '9'  (base != 16 || tolower(*s)  'a' || tolower(*s)  
 'f') )
 +{
 +res = -1;
 +goto out;
 +}
 +
 +res *= base;
 +res += (tolower(*s) = 'a') ? (tolower(*s) - 'a' + 10) : (*s - '0');
 +
 +if ( ores  res )
 +{
 +res = -1;
 +goto out;
 +}
 +
 +ores = res;
 +}
 +
 +out:

C labels intended by at least one space please.

 +static const char *find_opt(const char *cmdline, const char *opt, int arg)
 +{
 +size_t lc, lo;
 +static const char mm[] __text = --;

I'd be surprised if there weren't compiler/assembler versions
complaining about a section type conflict here. I can see why you
want everything in one section, but I'd rather suggest achieving
this at the linking step (which I would suppose to already be taking
care of this).

 +static u8 skip_realmode(const char *cmdline)
 +{
 +static const char nrm[] __text = no-real-mode;
 +static const char tboot[] __text = tboot=;
 +
 +if ( find_opt(cmdline, nrm, 0) || find_opt(cmdline, tboot, 1) )
 +return 1;
 +
 +return 0;

return find_opt(cmdline, nrm, 0) || find_opt(cmdline, tboot, 1);

 +static u8 edd_parse(const char *cmdline)
 +{
 +const char *c;
 +size_t la;
 +static const char edd[] __text = edd=;
 +static const char edd_off[] __text = off;
 +static const char edd_skipmbr[] __text = skipmbr;
 +
 +c = find_opt(cmdline, edd, 1);
 +
 +if ( !c )
 +return 0;
 +
 +c += strlen_static(edd);
 +la = strcspn(c, empty_chars);
 +
 +if ( !strncmp(c, edd_off, max(la, strlen_static(edd_off))) )
 +return 2;
 +else if ( !strncmp(c, edd_skipmbr, max(la, strlen_static(edd_skipmbr))) )

Pointless else.

 +return 1;
 +
 +return 0;

And the last two returns can be folded again anyway.

 +static void __cdecl __used cmdline_parse_early(const char *cmdline, 
 early_boot_opts_t *ebo)

I don't see the point of the __cdecl, and (as said before) dislike the
static __used pair.

 --- a/xen/arch/x86/boot/head.S
 +++ b/xen/arch/x86/boot/head.S
 @@ 

Re: [PATCH v2 10/23] efi: build xen.gz with EFI code

[Jan Beulich]

 On 25.08.15 at 18:31, daniel.ki...@oracle.com wrote:
 On Tue, Aug 25, 2015 at 06:09:09AM -0600, Jan Beulich wrote:
  On 24.08.15 at 22:54, daniel.ki...@oracle.com wrote:
  On Mon, Aug 24, 2015 at 05:35:21AM -0600, Jan Beulich wrote:
   On 22.08.15 at 15:59, daniel.ki...@oracle.com wrote:
   On Thu, Aug 20, 2015 at 09:39:39AM -0600, Jan Beulich wrote:
On 20.07.15 at 16:29, daniel.ki...@oracle.com wrote:
Currently, PE file contains many sections which are not linear (one
after another without any holes) or even do not have representation
in a file (e.g. BSS). In theory there is a chance that we could build
proper PE file using current build system. However, it means that
  
   What is improper about the currently built PE file? And if there is
   anything improper, did you inform the binutils maintainers of the
   problem?
  
   From PE loader point of view everything is OK. However, current Xen PE
   image (at least build on my machines) is not usable by multiboot (v1)
   or multiboot2 protocol compatible loader because it is not linear (one
   section does not live immediately after another without any voids).
 
  Again - either I'm missing something (and then your explanation is
  not good enough) or this is (as said above) a pointless adjustment.
 
  Let's focus on multiboot2 protocol (multiboot (v1) is similar to multiboot2
  in discussed case). In general multiboot2 is able to load any file which
  has:
1. proper multiboot2 header in first 32 KiB of a given file,
2. the text and data segments must be consecutive in the OS image
   (The Multiboot Specification version 1.6).
 
  This implies that we can e.g. build valid ELF file which is also multiboot2
  protocol compatible image. And we does. However, we can go further.
  Potentially we can build valid PE image which is also valid multiboot2
  protocol image. Although current build method does not satisfy requirement
  number 2 because, e.g.:
 
  Sections:
  Idx Name  Size  VMA   LMA   File off
  Algn
0 .text 001513d0  82d08020  82d08020  1000
  2**12
^^
CONTENTS, ALLOC, LOAD, CODE
1 .rodata   0004de12  82d0803513e0  82d0803513e0  00153000
  2**5
^^
CONTENTS, ALLOC, LOAD, READONLY, DATA
 
  Hence, we must use special method to build PE image (I discussed that in
  my earlier email in that topic) to do it compatible with multiboot2
  protocol.

 And you realize that we use a special method for building the
 current flat ELF image too?
 
 Yes, I know about that.

And with that I wonder ...

  This way one file could be loaded by native PE loader, mulitboot (v1)
  protocol
  (it requires relevant header but it does not interfere with PE and
  multiboot2
  protocol stuff) and mutliboot2 protocol compatible loaders. Additionally,
  if it is signed with Secure Boot signature then potentially signature could
  be verified by UEFI itself and e.g. GRUB2. However, as I said earlier this
  requires more work and this is next step which I am going to do after
  applying
  this series. Currently I am going to embed EFI support into ELF file 
  because
  it is easy (less changes; currently used ELF file has required properties
  because multiboot (v1) which we use has similar requirements like 
  multiboot2
  protocol) to make it compatible with multiboot2 protocol.

 I think whether what you do now makes sense depends on the
 ultimate goal: If we want a single binary usable for all three cases,
 then while yes, having EFI code available in the ELF image makes
 sense, using an ELF Image won't work. And we can't have an
 image being both ELF and PE. Hence the goal ought to be to have
 a single PE image, and with that the direction you move seems
 wrong.
 
 It depends how we want to generate proper PE file. There are two options.
 
 We can put manually proper PE header into xen/arch/x86/boot/head.S (maybe
 with some additional needed stuff). Then after build we will have ELF file
 which is loadable by multiboot protocols and has extra PE header. Of course
 it is unusable directly by EFI loader. However, using simple objcopy we can
 extract all interesting stuff from ELF file. This way we get proper PE file
 which is usable by three different boot protocols. Going that way we can
 also remove strict dependency on exact version of binutils which must have
 enabled i386pep support if we wish to build PE image.
 
 Potentially we can choose second way and build proper PE image using ld and
 objcopy/objdump tools with proper options. However, this require more work
 (maybe we will be forced to build something similar to mkelf32) and we bind
 Xen build machinery more tightly with exact version of binutils which is
 not nice.
 
 So, I decided to choose option #1.

... why there's

Re: [PATCH v2 22/23] x86: make Xen early boot code relocatable

[Jan Beulich]

 On 27.08.15 at 17:10, daniel.ki...@oracle.com wrote:
 On Thu, Aug 27, 2015 at 07:12:38AM -0600, Jan Beulich wrote:
  On 20.07.15 at 16:29, daniel.ki...@oracle.com wrote:
   /* Copy bootstrap trampoline to low memory, below 1MB. */
  -mov $sym_phys(trampoline_start),%esi
  +lea sym_offset(trampoline_start)(%ebp),%esi
   mov $trampoline_end - trampoline_start,%ecx
   rep movsb
 

 The latest at this final hunk I'm getting tired (and upset). I'd much
 rather not touch all this code in these fragile ways, and instead
 require Xen to be booted without grub2 on badly written firmware
 like the one on the machine you quote in the description.
 
 Let's start discussion from here. My further work on this patch series
 is pointless until we do not agree details in this case.
 
 So, I agree that any software (including firmware) should not use
 precious resources (i.e. low memory in that case) if it is not strictly
 required. And I do not think so that latest UEFI implementations
 on new hardware really need this low memory to survive (at least page
 tables could be put anywhere above low memory). However, in case of
 UEFI it is a wish of smart people not requirement set by spec. I was
 checking UEFI docs a few times but I was not able to find anything
 which says that e.g. ...developer MUST allocate memory outside of low
 memory  Even I have not found any suggestion about that. However,
 I must admit that I do not know whole UEFI spec, so, if you know something
 which is similar to above please tell me where it is (title, revision,
 page, paragraph, etc.). Hence, if there is not strict requirement in
 regards to memory allocation in specs we are lost. Of course we can
 encourage people to not do nasty things but I do not believe that many
 will listen. So, sadly, I suppose that we will see more and more machines
 in the wild which are broken (well, let's say do not align to good 
 practices).
 
 On the other hand I think that we should not assume that a given memory
 region (in our case starting at 1 MiB) is (or will be) available in every
 machine. I have not seen anything which says that it is true. We should
 stop doing that even if it works right now. I think that it works by
 chance and there is a chance that it will stop working one day because
 somebody will discover that he or she can put there e.g. device or hole.
 
 Last but not least. I suppose that Xen users and especially distros will
 complain when they are not able to use GRUB2 with Xen on every platform
 just because somebody (i.e. platform designers, developers, or whatever)
 do not accept our beliefs or we require that platform must obey rules
 (i.e. memory map requirements) which are specified nowhere.

You're right, there's no such requirement on memory use in the spec.
But you're missing the point. Supporting grub2 on UEFI is already a
hack (ignoring all intentions EFI had from its first days). And now
you've found an environment where that hack needs another hack
(in Xen) to actually work. That's too much hackery for my taste, the
more that things on this system can (afaict) work quite okay (without
grub2, or with using its chainloader mechanism).

So no, I'm still not convinced of the need for this patch.

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2 22/23] x86: make Xen early boot code relocatable

[Jan Beulich]

 On 27.08.15 at 20:04, andrew.coop...@citrix.com wrote:
 On 27/08/15 16:29, Jan Beulich wrote:
 On 27.08.15 at 17:10, daniel.ki...@oracle.com wrote:
 On Thu, Aug 27, 2015 at 07:12:38AM -0600, Jan Beulich wrote:
 On 20.07.15 at 16:29, daniel.ki...@oracle.com wrote:
  /* Copy bootstrap trampoline to low memory, below 1MB. */
 -mov $sym_phys(trampoline_start),%esi
 +lea sym_offset(trampoline_start)(%ebp),%esi
  mov $trampoline_end - trampoline_start,%ecx
  rep movsb

 The latest at this final hunk I'm getting tired (and upset). I'd much
 rather not touch all this code in these fragile ways, and instead
 require Xen to be booted without grub2 on badly written firmware
 like the one on the machine you quote in the description.
 Let's start discussion from here. My further work on this patch series
 is pointless until we do not agree details in this case.

 So, I agree that any software (including firmware) should not use
 precious resources (i.e. low memory in that case) if it is not strictly
 required. And I do not think so that latest UEFI implementations
 on new hardware really need this low memory to survive (at least page
 tables could be put anywhere above low memory). However, in case of
 UEFI it is a wish of smart people not requirement set by spec. I was
 checking UEFI docs a few times but I was not able to find anything
 which says that e.g. ...developer MUST allocate memory outside of low
 memory  Even I have not found any suggestion about that. However,
 I must admit that I do not know whole UEFI spec, so, if you know something
 which is similar to above please tell me where it is (title, revision,
 page, paragraph, etc.). Hence, if there is not strict requirement in
 regards to memory allocation in specs we are lost. Of course we can
 encourage people to not do nasty things but I do not believe that many
 will listen. So, sadly, I suppose that we will see more and more machines
 in the wild which are broken (well, let's say do not align to good 
 practices).

 On the other hand I think that we should not assume that a given memory
 region (in our case starting at 1 MiB) is (or will be) available in every
 machine. I have not seen anything which says that it is true. We should
 stop doing that even if it works right now. I think that it works by
 chance and there is a chance that it will stop working one day because
 somebody will discover that he or she can put there e.g. device or hole.

 Last but not least. I suppose that Xen users and especially distros will
 complain when they are not able to use GRUB2 with Xen on every platform
 just because somebody (i.e. platform designers, developers, or whatever)
 do not accept our beliefs or we require that platform must obey rules
 (i.e. memory map requirements) which are specified nowhere.
 You're right, there's no such requirement on memory use in the spec.
 But you're missing the point. Supporting grub2 on UEFI is already a
 hack (ignoring all intentions EFI had from its first days). And now
 you've found an environment where that hack needs another hack
 (in Xen) to actually work. That's too much hackery for my taste, the
 more that things on this system can (afaict) work quite okay (without
 grub2, or with using its chainloader mechanism).

 So no, I'm still not convinced of the need for this patch.
 
 I disagree.  There are many things a grub environment gives us which
 plain EFI doesn't, most notably a familiar booting environment and
 recovery facilities for users (which vastly trumps how EFI expects to
 run things).

Familiar booting environment? I knew of EFI's much earlier than
I saw grub for the first time, so to me EFI's is more familiar if you
so will.

 Furthermore, it offers common management of /boot in the dom0 filesystem
 between legacy and EFI boots.

Which is an argument for porting suitable file system drivers to EFI,
not an argument to layer multiple boot loaders. Plus booting _is_
(naturally) different between platforms. Plus if EFI was ported to
architectures of interest (see ARM), the difference between
platforms would decrease too (which btw was one of the original
goals of EFI). Are you btw saying that ARM should use grub2 too?
I didn't hear that mentioned as a goal anywhere so far...

 Therefore I am very much +1 get grub working.

Then you kind of misunderstood: I'm not against getting grub2
working (i.e. patches prior to this one are fine in principle). What
I'm against is hacking around firmware+grub2 combinations not
suitable for Xen's purposes (where one could argue which of the
three is really at fault, and I'm far from convinced it's Xen).

Jan

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [Xen-devel] [PATCH v2 22/23] x86: make Xen early boot code relocatable

[Jan Beulich]

 On 27.08.15 at 19:56, 426...@gmail.com wrote:
 If you advocate direct booting ( no boot loader)  on production machines I
 wont argue much, as long as there is good recovery tools to deal with
 failed boots (grub does this very well, I am not aware of anything
 comparable that is pure efi). however the other use case which care more
 about is dual booting. I am a novice when it comes to Xen, although
 otherwise competent. The test machines I have for playing with Xen are also
 used for other tests, some of which require raw hardware support, so I want
 the ability to use a boot menu.

All EFI systems that I've seen so far have a boot manager. Are you
saying there are vendors who remove that?

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [Xen-devel] [PATCH v2 22/23] x86: make Xen early boot code relocatable

[Jan Beulich]

 On 28.08.15 at 15:42, konrad.w...@oracle.com wrote:
 And I am not comfortable to say 'GRUB2+Xen cannot run on this hardware
 because your firmware vendor is not following the EFI spec in spirit.'

Well, not the least since I don't really agree with this (albeit I can
see where you're coming from) ...

 Now that said - do you have suggestions on how to make this work
 with GRUB in the picture?

... I don't think I'm the one to make suggestions on how to make
things work with grub in the picture when I continue to be of the
opinion that it shouldn't have been brought into the picture in the
first place.

But for the purely technical (patch) aspect: Anything (e.g.
macroization such that at least some sym_phys() uses can remain
untouched) allowing to limit the impact of said patch on the source
code (thus helping review and perhaps also long term
maintainability) would be a step towards talking me into
withdrawing my objection.

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [Xen-devel] [PATCH v2 22/23] x86: make Xen early boot code relocatable

[Jan Beulich]

 On 27.08.15 at 17:29, jbeul...@suse.com wrote:
 You're right, there's no such requirement on memory use in the spec.
 But you're missing the point. Supporting grub2 on UEFI is already a
 hack (ignoring all intentions EFI had from its first days). And now
 you've found an environment where that hack needs another hack
 (in Xen) to actually work. That's too much hackery for my taste, the
 more that things on this system can (afaict) work quite okay (without
 grub2, or with using its chainloader mechanism).

It has been brought to my attention that the use of the work hack
above could have been understood as an offense. It certainly
wasn't meant so, and I'd like to apologize if it came over that way. I
simply used it not finding a better term while writing; perhaps I could
have used misguided and workaround respectively, but I'm not
sure that would have been received much better.

In any event - I didn't mean to insult you or anyone else.

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2 10/23] efi: build xen.gz with EFI code

[Jan Beulich]

 On 26.08.15 at 14:33, daniel.ki...@oracle.com wrote:
 Do you suggest that I should put this functionality (PE with multiboot
 headers) on top of this patch series? Well, it is possible but this
 series is big and I would like to avoid to make it bigger. I prefer to
 get current patches into Xen tree and then work on new features (it
 should not take so long because as I can see we almost agreed most
 of stuff in that case). Or if at least half patches of current series
 will be accepted (as I can see there is a chance to do that with v3)
 then I can work on this functionality and put it on top of second not
 applied part. Does it make sense?

Yes. I'm not objecting to deferring that part. All I want is you to make
sure not to submit any changes potentially conflicting with the end
goal of having a single binary (which as I understand it can only be a
PE one).

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2 19/23] x86/efi: create new early memory allocator

[Jan Beulich]

 On 20.07.15 at 16:29, daniel.ki...@oracle.com wrote:
 There is a problem with place_string() which is used as early memory
 allocator. It gets memory chunks starting from start symbol and
 going down. Sadly this does not work when Xen is loaded using multiboot2
 protocol because start lives on 1 MiB address. So, I tried to use
 mem_lower address calculated by GRUB2. However, it works only on some
 machines. There are machines in the wild (e.g. Dell PowerEdge R820)
 which uses first ~640 KiB for boot services code or data... :-(((
 
 In case of multiboot2 protocol we need that place_string() only allocate
 memory chunk for EFI memory map. However, I think that it should be fixed
 instead of making another function used just in one case. I thought about
 two solutions.
 
 1) We could use native EFI allocation functions (e.g. AllocatePool()
or AllocatePages()) to get memory chunk. However, later (somewhere
in __start_xen()) we must copy its contents to safe place or reserve
this in e820 memory map and map it in Xen virtual address space.
In later case we must also care about conflicts with e.g. crash
kernel regions which could be quite difficult.

1b) Do away with efi_arch_allocate_mmap_buffer() and use 
AllocatePages() uniformly, perhaps with a per-arch specified
memory type (by means of which you can control whether the
memory contents will remain preserved until the time you want
to look at it). That will eliminate the only place_string() you're
concerned about, with a patch with better diffstat (largely due
to the questionable arch hook gone).

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [Xen-devel] [PATCH v2 22/23] x86: make Xen early boot code relocatable

[Jan Beulich]

>>> On 31.08.15 at 21:49, <daniel.ki...@oracle.com> wrote:
> On Fri, Aug 28, 2015 at 08:16:05AM -0600, Jan Beulich wrote:
>> >>> On 28.08.15 at 15:42, <konrad.w...@oracle.com> wrote:
>> > Now that said - do you have suggestions on how to make this work
>> > with GRUB in the picture?
>>
>> ... I don't think I'm the one to make suggestions on how to make
>> things work with grub in the picture when I continue to be of the
>> opinion that it shouldn't have been brought into the picture in the
>> first place.
> 
> Could you be more specific what is wrong with this patch or at least last
> hunk which you reviewed? What is real technical reason that it could not
> be accepted? If idea is wrong in general please tell me where and why.
> Otherwise I am not able to work out other better solution.

The patch may not be wrong technically (and I never said so), it is
just that the way you carry things out is too intrusive for my taste.
Since Andrew is happy with the change in principle, I think I wouldn't
veto it going in with his R-b.

> By the way, once I have put 3 (IIRC) proposals for this problem on the 
> table.
> Even we discussed this issue in Shanghai. You and Andrew approved more or
> less this one. So, I am a bit disappointed that you withdraw your approval
> (yes, partial but still the approval) at this stage with just vague 
> explanations.

I've never approved of anything here. At the hackathon we've only
hashed out possible options.

>> But for the purely technical (patch) aspect: Anything (e.g.
>> macroization such that at least some sym_phys() uses can remain
>> untouched) allowing to limit the impact of said patch on the source
>> code (thus helping review and perhaps also long term
>> maintainability) would be a step towards talking me into
>> withdrawing my objection.
> 
> Ditto. This is too vague. So, I will be very grateful if you review this
> patch until the end or at least tell me what (if you add why it will be
> nice) exactly should be fixed.

How is this to vague? I gave a possible direction (macroization) as
well as the criteria (less impact on existing code; to be slightly more
precise I'd specifically like to see most of the open coded %fs: uses
gone).

Again - if Andrew thinks this is the right thing to do, I'll defer to him
for reviewing these final few patches of the series, since as said
before to me this is a workaround for a misguided design, and as
such I'm not willing to accept as intrusive a patch as this one is. (And
no, I don't really buy his argument of Xen boot code needing to be
relocatable anyway - the legacy boot path doesn't really need to
fear there not being free memory right above 1Mb. At least I have
seen no proof of there being any system [supporting legacy boot]
where Xen can't boot that way. And even if there was, it would
still need to be determined whether this is legitimately so, or again
due to poorly written firmware.)

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2 10/23] efi: build xen.gz with EFI code

[Jan Beulich]

 On 24.08.15 at 22:54, daniel.ki...@oracle.com wrote:
 On Mon, Aug 24, 2015 at 05:35:21AM -0600, Jan Beulich wrote:
  On 22.08.15 at 15:59, daniel.ki...@oracle.com wrote:
  On Thu, Aug 20, 2015 at 09:39:39AM -0600, Jan Beulich wrote:
   On 20.07.15 at 16:29, daniel.ki...@oracle.com wrote:
   Currently, PE file contains many sections which are not linear (one
   after another without any holes) or even do not have representation
   in a file (e.g. BSS). In theory there is a chance that we could build
   proper PE file using current build system. However, it means that
 
  What is improper about the currently built PE file? And if there is
  anything improper, did you inform the binutils maintainers of the
  problem?
 
  From PE loader point of view everything is OK. However, current Xen PE
  image (at least build on my machines) is not usable by multiboot (v1)
  or multiboot2 protocol compatible loader because it is not linear (one
  section does not live immediately after another without any voids).

 Again - either I'm missing something (and then your explanation is
 not good enough) or this is (as said above) a pointless adjustment.
 
 Let's focus on multiboot2 protocol (multiboot (v1) is similar to multiboot2
 in discussed case). In general multiboot2 is able to load any file which 
 has:
   1. proper multiboot2 header in first 32 KiB of a given file,
   2. the text and data segments must be consecutive in the OS image
  (The Multiboot Specification version 1.6).
 
 This implies that we can e.g. build valid ELF file which is also multiboot2
 protocol compatible image. And we does. However, we can go further.
 Potentially we can build valid PE image which is also valid multiboot2
 protocol image. Although current build method does not satisfy requirement
 number 2 because, e.g.:
 
 Sections:
 Idx Name  Size  VMA   LMA   File off  
 Algn
   0 .text 001513d0  82d08020  82d08020  1000  
 2**12
   ^^
   CONTENTS, ALLOC, LOAD, CODE
   1 .rodata   0004de12  82d0803513e0  82d0803513e0  00153000  
 2**5
   ^^
   CONTENTS, ALLOC, LOAD, READONLY, DATA
 
 Hence, we must use special method to build PE image (I discussed that in
 my earlier email in that topic) to do it compatible with multiboot2 
 protocol.

And you realize that we use a special method for building the
current flat ELF image too?

 This way one file could be loaded by native PE loader, mulitboot (v1) 
 protocol
 (it requires relevant header but it does not interfere with PE and 
 multiboot2
 protocol stuff) and mutliboot2 protocol compatible loaders. Additionally,
 if it is signed with Secure Boot signature then potentially signature could
 be verified by UEFI itself and e.g. GRUB2. However, as I said earlier this
 requires more work and this is next step which I am going to do after 
 applying
 this series. Currently I am going to embed EFI support into ELF file because
 it is easy (less changes; currently used ELF file has required properties
 because multiboot (v1) which we use has similar requirements like multiboot2
 protocol) to make it compatible with multiboot2 protocol.

I think whether what you do now makes sense depends on the
ultimate goal: If we want a single binary usable for all three cases,
then while yes, having EFI code available in the ELF image makes
sense, using an ELF Image won't work. And we can't have an
image being both ELF and PE. Hence the goal ought to be to have
a single PE image, and with that the direction you move seems
wrong.

   --- a/xen/arch/x86/efi/Makefile
   +++ b/xen/arch/x86/efi/Makefile
   @@ -1,14 +1,16 @@
CFLAGS += -fshort-wchar
  
   -obj-y += stub.o
   -
   -create = test -e $(1) || touch -t 19990101 $(1)
   -
efi := $(filter y,$(x86_64)$(shell rm -f disabled))
efi := $(if $(efi),$(shell $(CC) $(filter-out $(CFLAGS-y) 
   .%.d,$(CFLAGS)) -c check.c 2disabled  echo y))
efi := $(if $(efi),$(shell $(LD) -mi386pep --subsystem=10 -o check.efi 
   heck.o disabled  echo y))
   -efi := $(if $(efi),$(shell rm disabled)y,$(shell $(call 
   create,boot.init.o); $(call create,runtime.o)))
   +efi := $(if $(efi),$(shell rm disabled)y)
  
   -extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o
   +extra-y += relocs-dummy.o
 
  Why is this no longer extra-$(efi)?
 
  Because we need proper EFI code in xen.gz to support boot
  via multiboot2 on EFI platforms.

 What would we need that for when not building an EFI-capable
 binary anyway?
 
 xen/arch/x86/efi/stub.c

This is still too unspecific: I can't see any reference from that file
to any of the symbols relocs-dummy.S provides.

Jan

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2 21/23] x86/boot: implement early command line parser in C

[Jan Beulich]

>>> On 22.09.15 at 19:03, <daniel.ki...@oracle.com> wrote:
> On Thu, Aug 27, 2015 at 06:43:39AM -0600, Jan Beulich wrote:
>> >>> On 20.07.15 at 16:29, <daniel.ki...@oracle.com> wrote:
>> > +#define __packed  __attribute__((__packed__))
>> > +#define __text__attribute__((__section__(".text")))
>> > +#define __used__attribute__((__used__))
>>
>> Likely better to include compiler.h instead.
> 
> As I know you do not like to include such headers in early C files
> because it makes code fragile and it looks strange. I agree with you
> to some extent. So, I decided to define needed constants ourselves.
> Whatever we do we should be consistent. Hence, if we include compiler.h
> here we should do the same in reloc.c too if it is required.

I disagree - reloc.c serves a completely different purpose, and
hence may have different considerations applied when it comes
to what to (not) include.

>> > +#define max(x,y) ({ \
>> > +const typeof(x) _x = (x);   \
>> > +const typeof(y) _y = (y);   \
>> > +(void) (&_x == &_y);\
>> > +_x > _y ? _x : _y; })
>>
>> I also wonder whether -imacros .../xen/kernel.h wouldn't be a better
>> approach here. Please really think hard on how to avoid duplications
>> like these.
> 
> Ditto. So, what is your decision? Include or define? If include then
> we should think how to generate relevant dependencies automatically.

I think the question should rather be whether we can't make cmdline.c
build the normal way, not the reloc.c one.

>> > +#define strlen_static(s) (sizeof(s) - 1)
>>
>> What is this good for? A decent compiler should be able to deal with
>> strlen("..."). Plus your macro is longer that what it tries to "abbreviate".
> 
> I thought that it is true but it is not. Sadly, without this binary is 
> bigger... :-(((

Perhaps as a result of some (missing) option(s)?

> However, you are right that the name could be better.

Or just drop the macro.

>> > +static const char empty_chars[] __text = " \n\r\t";
>>
>> What is empty about them? DYM blank or (white) space or separator
>> or delimiter? I also wonder whether \n and \r are actually usefully here,
> 
> Yep, delimiter or something like that looks better.
> 
>> as they should (if at all) only end the line.
> 
> Yes, I included them just in case and they should not appear in command 
> line.

If you mean to retain characters in that array that aren't obviously
needed, please explain such in a comment so people won't have to
guess whether to delete them, or will know it is (relatively) safe to
delete them in case their presence causes problems. But even better
would be to just have the obvious blank characters (space and tab)
there.

>> > +static void __cdecl __used cmdline_parse_early(const char *cmdline, 
> early_boot_opts_t *ebo)
>>
>> I don't see the point of the __cdecl, and (as said before) dislike the
>> static __used pair.
> 
> Are you sure that without __cdecl compiler will not try to optimize
> cmdline_parse_early() call and try to pass arguments using registers
> or anything else not conforming to cdecl calling convention?

Yes, I am - the moment you drop the "static". At that point the
compiler has no choice, unless it is being told on the command line
to use a different calling convention

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2 20/23] x86: add multiboot2 protocol support for EFI platforms

[Jan Beulich]

>>> On 22.09.15 at 17:21, <daniel.ki...@oracle.com> wrote:
> On Thu, Aug 27, 2015 at 06:01:26AM -0600, Jan Beulich wrote:
>> >>> On 20.07.15 at 16:29, <daniel.ki...@oracle.com> wrote:
>> > @@ -130,6 +146,119 @@ print_err:
>> >  .Lhalt: hlt
>> >  jmp .Lhalt
>> >
>> > +.code64
>> > +
>> > +__efi64_start:
>> > +cld
>> > +
>> > +/* Check for Multiboot2 bootloader. */
>> > +cmp $MULTIBOOT2_BOOTLOADER_MAGIC,%eax
>> > +je  efi_multiboot2_proto
>> > +
>> > +/* Jump to not_multiboot after switching CPU to x86_32 mode. */
>> > +lea not_multiboot(%rip),%rdi
>> > +jmp x86_32_switch
>> > +
>> > +efi_multiboot2_proto:
>>
>> .L
> 
> Why do you want to hide labels which could be useful during debugging?

With a few exceptions, assembly code should follow C and other
high level languages: Symbol table entries only at function
boundaries (or whatever their suitable counterparts in assembly
are).

>> > +x86_32_switch:
>> > +cli
>> > +
>> > +/* Initialise GDT. */
>> > +lgdtgdt_boot_descr(%rip)
>> > +
>> > +/* Reload code selector. */
>> > +ljmpl   *cs32_switch_addr(%rip)
>> > +
>> > +.code32
>> > +
>> > +cs32_switch:
>> > +/* Initialise basic data segments. */
>> > +mov $BOOT_DS,%edx
>> > +mov %edx,%ds
>> > +mov %edx,%es
>> > +mov %edx,%fs
>> > +mov %edx,%gs
>> > +mov %edx,%ss
>>
>> I see no point in loading %fs and %gs with other than nul selectors.
>> I also think %ss should be loaded first. Which reminds me - what
>> about %rsp? Is it guaranteed to have its upper 32 bits clear, so you
>> can re-use the stack in 32-bit mode? (If it is, saying so in a comment
>> would be very desirable.)
> 
> I am not reusing %rsp value. %esp is initialized later in 32-bit code.
> Stack is not used until %esp is not initialized.

If you load %ss without loading the stack pointer, you should imo
at least add a comment saying when/where the other half will be
done.

>> > --- a/xen/common/efi/boot.c
>> > +++ b/xen/common/efi/boot.c
>> > @@ -79,6 +79,17 @@ static size_t wstrlen(const CHAR16 * s);
>> >  static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz);
>> >  static bool_t match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2);
>> >
>> > +static void efi_init(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
>> > *SystemTable);
>> > +static void efi_console_set_mode(void);
>> > +static EFI_GRAPHICS_OUTPUT_PROTOCOL *efi_get_gop(void);
>> > +static UINTN efi_find_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
>> > +   UINTN cols, UINTN rows, UINTN depth);
>> > +static void efi_tables(void);
>> > +static void setup_efi_pci(void);
>> > +static void efi_variables(void);
>> > +static void efi_set_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, UINTN 
>> > gop_mode);
>> > +static void efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
>> > *SystemTable);
>>
>> This is ugly; I'm sure there is a way to avoid these declarations.
> 
> This probably requires play with '#include "efi-boot.h"' and move
> somewhere before efi_start(). Maybe something else. If it is not
> a problem for you I can do that.

Indeed moving an #include would seem far better than adding almost
a dozen declarations (any of which will need to get touched if the
respective definition changes, i.e. arranging for future churn).

Jan


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: multiboot2 and module2 boot issues via GRUB2

[Jan Beulich via Grub-devel]

On 01.04.2021 03:06, Roman Shaposhnik wrote:
> And the obvious next question: is my EVE usecase esoteric enough that
> I should just go ahead and do a custom GRUB patch or is there a more
> general interest in this?

Not sure if it ought to be a grub patch - the issue could as well
be dealt with in Xen, by concatenating modules to form a monolithic
initrd.

Jan

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: multiboot2 and module2 boot issues via GRUB2

[Jan Beulich via Grub-devel]

On 01.04.2021 21:43, Andrew Cooper wrote:
> On 01/04/2021 09:44, Roger Pau Monné wrote:
>> On Thu, Apr 01, 2021 at 09:31:07AM +0200, Jan Beulich wrote:
>>> On 01.04.2021 03:06, Roman Shaposhnik wrote:
>>>> And the obvious next question: is my EVE usecase esoteric enough that
>>>> I should just go ahead and do a custom GRUB patch or is there a more
>>>> general interest in this?
>>> Not sure if it ought to be a grub patch - the issue could as well
>>> be dealt with in Xen, by concatenating modules to form a monolithic
>>> initrd.
>> I would rather have it done in the loader than Xen, mostly because
>> it's a Linux boot specific format, and hence I don't think Xen should
>> have any knowledge about it.
>>
>> If it turns out to be impossible to implement on the loader side we
>> should consider doing it in Xen, but that's not my first option.
> 
> Concatenating random things which may or may not be initrds is
> absolutely not something Xen should do.  We don't have enough context to
> do it safely/sensibly.

Well, I wasn't suggesting anywhere to concatenate random things.
Instead I was envisioning a command line option giving us the
context we need (e.g. "initrd=3+5").

Jan

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 1/7] multiboot2: Add load type header and support for the PE binary type

[Jan Beulich via Grub-devel]

On 13.03.2024 16:07, Ross Lagerwall wrote:
> In addition to the existing address and ELF load types, specify a new
> optional PE binary load type. This new type is a useful addition since
> PE binaries can be signed and verified (i.e. used with Secure Boot).

And the consideration to have ELF signable (by whatever extension to
the ELF spec) went nowhere?

Jan

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 1/7] multiboot2: Add load type header and support for the PE binary type

[Jan Beulich via Grub-devel]

On 14.03.2024 15:24, Ross Lagerwall wrote:
> On Thu, Mar 14, 2024 at 1:37 PM Jan Beulich  wrote:
>> On 14.03.2024 10:30, Ross Lagerwall wrote:
>>> On Thu, Mar 14, 2024 at 7:24 AM Jan Beulich  wrote:
>>>> On 13.03.2024 16:07, Ross Lagerwall wrote:
>>>>> In addition to the existing address and ELF load types, specify a new
>>>>> optional PE binary load type. This new type is a useful addition since
>>>>> PE binaries can be signed and verified (i.e. used with Secure Boot).
>>>>
>>>> And the consideration to have ELF signable (by whatever extension to
>>>> the ELF spec) went nowhere?
>>>
>>> I'm not sure if you're referring to some ongoing work to create signable
>>> ELFs that I'm not aware of.
>>
>> Something must have been invented already to make Linux modules signable.
> 
> Linux module signatures operate outside of the ELF container. In fact,
> AFAIK the actual signed content could be anything. The file format is:
> 
> * Content (i.e. ELF binary)
> * Signature of content in PKCS7 format
> * Signature info, including signature length
> * Magic marker: "~Module signature appended~\n"
> 
> This kind of arrangement does indeed work but it is fragile. Since the
> signature is on the entire contents and tools that understand ELF don't
> parse the signature, any transformation of the binary (e.g. to
> strip out debuginfo) will cause the signature to be lost / invalidated.

This looks extremely poor to me, so ...

> Nevertheless, this could still be an option for Xen if this is
> deemed to be a preferred solution by others. It would be good to hear
> some opinions on this.

... I'd rather not see this used for Xen.

Jan

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 1/7] multiboot2: Add load type header and support for the PE binary type

[Jan Beulich via Grub-devel]

On 14.03.2024 10:30, Ross Lagerwall wrote:
> On Thu, Mar 14, 2024 at 7:24 AM Jan Beulich  wrote:
>>
>> On 13.03.2024 16:07, Ross Lagerwall wrote:
>>> In addition to the existing address and ELF load types, specify a new
>>> optional PE binary load type. This new type is a useful addition since
>>> PE binaries can be signed and verified (i.e. used with Secure Boot).
>>
>> And the consideration to have ELF signable (by whatever extension to
>> the ELF spec) went nowhere?
>>
> 
> I'm not sure if you're referring to some ongoing work to create signable
> ELFs that I'm not aware of.

Something must have been invented already to make Linux modules signable.

> I didn't choose that route because:
> 
> * Signed PE binaries are the current standard for Secure Boot.
> 
> * Having signed ELF binaries would mean that code to handle them needs
> to be added to Shim which contravenes its goals of being small and
> simple to verify.

Both true, but neither goes entirely without saying, I suppose.

> * I could be wrong on this but to my knowledge, the ELF format is not
> being actively updated nor is the standard owned/maintained by a
> specific group which makes updating it difficult.

And PE/COFF isn't under control of a public entity / group afaik, which
may be viewed as no better, if not worse.

> * Tools would need to be updated/developed to add support for signing
> ELF binaries and inspecting the signatures.

As above, yes indeed.

Jan

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel