Re: [PATCH 1/7] multiboot2: Add load type header and support for the PE binary type
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
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
Re: [PATCH 1/7] multiboot2: Add load type header and support for the PE binary type
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: multiboot2 and module2 boot issues via GRUB2
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: multiboot2 and module2 boot issues via GRUB2
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: [Xen-devel] Uniform commands for booting xen
>>> On 11.01.16 at 15:58, wrote: > On 11.01.2016 15:32, Jan Beulich wrote: >>>>> On 11.01.16 at 15:06, wrote: >>> On 13.11.2015 10:50, Ian Campbell wrote: >>>> On Fri, 2015-11-13 at 12:04 +0300, Andrei Borzenkov wrote: >>>>>> 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? >>>>>> >>>>> >>>>> Are modules used by Xen self-identifying? Is it enough to simply pass >>>>> Xen kernel list of binary blobs or Xen kernel must be told what these >>>>> binary blobs are? If they are self identifying, why arm needs to be >>>>> passed module type in the first place? >>>> >>>> At first Xen/ARM required the bootloader to identify, but that was since >>>> identified as causing madness and fixed by having Xen/ARM do as Xen/x86 >>>> does and figure things out for itself, but I failed to communicate this >>>> clearly and things got implemented on the grub side under the old >>>> assumptions. >>>> >>> This changes a lot. This removes most of hurdles towards uniformity. Are >>> you ok with replacing xen_kernel/xen_xsm/... with just xen_module and >>> dropping type altogether? >>> Do you think that it makes sense to have xen_initrd in order to have >>> in-memory initrd concatenation like baremetal counterpart? In either >>> case we can add it later. I'd rather not have a command than to change >>> its meaning later. >>> Jan, does it address your concerns? >> >> It improves things a bit, but I'd really like to not see any xen_ >> prefixed commands at all in grub2 - after all Xen should just be >> an ordinary multiboot client. >> > This is true for x86 but on ARM64 the protocol xen expects is quite > different and not really multiboot. How would we avoid xen_ prefixed > commands for ARM64? And when we have them it makes sense to have them on > x86 as well so that the same set of commands works on both arm64 and x86 Well, if they're unavoidable on ARM then I can see avoiding them on x86 as not as relevant. Otoh avoiding divergence from other x86 clients might still call for them to be omitted. In the end you're the maintainer, so you should know best. 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
>>> On 11.01.16 at 15:06, wrote: > On 13.11.2015 10:50, Ian Campbell wrote: >> On Fri, 2015-11-13 at 12:04 +0300, Andrei Borzenkov wrote: 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? >>> >>> Are modules used by Xen self-identifying? Is it enough to simply pass >>> Xen kernel list of binary blobs or Xen kernel must be told what these >>> binary blobs are? If they are self identifying, why arm needs to be >>> passed module type in the first place? >> >> At first Xen/ARM required the bootloader to identify, but that was since >> identified as causing madness and fixed by having Xen/ARM do as Xen/x86 >> does and figure things out for itself, but I failed to communicate this >> clearly and things got implemented on the grub side under the old >> assumptions. >> > This changes a lot. This removes most of hurdles towards uniformity. Are > you ok with replacing xen_kernel/xen_xsm/... with just xen_module and > dropping type altogether? > Do you think that it makes sense to have xen_initrd in order to have > in-memory initrd concatenation like baremetal counterpart? In either > case we can add it later. I'd rather not have a command than to change > its meaning later. > Jan, does it address your concerns? It improves things a bit, but I'd really like to not see any xen_ prefixed commands at all in grub2 - after all Xen should just be an ordinary multiboot client. 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
>>> On 12.11.15 at 18:09, wrote:
> On Thu, 2015-11-12 at 08:44 -0700, Jan Beulich wrote:
>> > > > 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.
>
> 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
>>> On 12.11.15 at 17:58, wrote: > 12.11.2015 18:44, 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. >> > > 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: [Xen-devel] Uniform commands for booting xen
>>> 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: [PATCH v2 21/23] x86/boot: implement early command line parser in C
>>> On 22.09.15 at 19:03, wrote:
> On Thu, Aug 27, 2015 at 06:43:39AM -0600, Jan Beulich wrote:
>> >>> On 20.07.15 at 16:29, 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
>>> On 22.09.15 at 17:21, wrote: > On Thu, Aug 27, 2015 at 06:01:26AM -0600, Jan Beulich wrote: >> >>> On 20.07.15 at 16:29, 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: [Xen-devel] [PATCH v2 22/23] x86: make Xen early boot code relocatable
>>> On 31.08.15 at 21:49, wrote: > On Fri, Aug 28, 2015 at 08:16:05AM -0600, Jan Beulich wrote: >> >>> On 28.08.15 at 15:42, 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: [Xen-devel] [PATCH v2 22/23] x86: make Xen early boot code relocatable
>>> On 27.08.15 at 17:29, 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: [Xen-devel] [PATCH v2 22/23] x86: make Xen early boot code relocatable
>>> On 28.08.15 at 15:42, 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
>>> 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: [PATCH v2 22/23] x86: make Xen early boot code relocatable
>>> On 27.08.15 at 20:04, wrote: > On 27/08/15 16:29, Jan Beulich wrote: >>>>> On 27.08.15 at 17:10, wrote: >>> On Thu, Aug 27, 2015 at 07:12:38AM -0600, Jan Beulich wrote: >>>>>>> On 20.07.15 at 16:29, 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: [PATCH v2 22/23] x86: make Xen early boot code relocatable
>>> On 27.08.15 at 17:10, wrote: > On Thu, Aug 27, 2015 at 07:12:38AM -0600, Jan Beulich wrote: >> >>> On 20.07.15 at 16:29, 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
>>> On 20.07.15 at 16:29, wrote: > - %fs register is filled with segment descriptor which describes memory > region > with Xen image (it could be relocated or not); This is too fuzzy. Please be very precise which region it is that %fs is supposed to point to (so that reviewers have a chance to validate uses). > --- a/xen/arch/x86/Makefile > +++ b/xen/arch/x86/Makefile > @@ -72,8 +72,10 @@ efi-$(x86_64) := $(shell if [ ! -r > $(BASEDIR)/include/xen/compile.h -o \ > echo '$(TARGET).efi'; fi) > > $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32 > - ./boot/mkelf32 $(TARGET)-syms $(TARGET) 0x10 \ > - `$(NM) -nr $(TARGET)-syms | head -n 1 | sed -e 's/^\([^ ]*\).*/0x\1/'` > +#THIS IS UGLY HACK! PLEASE DO NOT COMPLAIN. I WILL FIX IT IN NEXT > RELEASE. > + ./boot/mkelf32 $(TARGET)-syms $(TARGET) $(XEN_IMG_PHYS_START) > 0x82d08100 > +#./boot/mkelf32 $(TARGET)-syms $(TARGET) 0x10 \ > +#`$(NM) -nr $(TARGET)-syms | head -n 1 | sed -e 's/^\([^ ]*\).*/0x\1/'` I do complain nevertheless: Such a patch should be tagged RFC. And with such a hack in place I'm not even sure it's worth reviewing. > --- a/xen/arch/x86/boot/head.S > +++ b/xen/arch/x86/boot/head.S > @@ -12,13 +12,15 @@ > .text > .code32 > > -#define sym_phys(sym) ((sym) - __XEN_VIRT_START) > +#define sym_phys(sym) ((sym) - __XEN_VIRT_START + XEN_IMG_PHYS_START - > XEN_IMG_OFFSET) > +#define sym_offset(sym) ((sym) - __XEN_VIRT_START) With XEN_IMG_PHYS_START == XEN_IMG_OFFSET - what's the point? (If there is a point, then there's obviously a comment missing here explaining things, including when to use which.) > @@ -105,12 +107,13 @@ multiboot1_header_end: > > .word 0 > gdt_boot_descr: > -.word 6*8-1 > -.long sym_phys(trampoline_gdt) > +.word 7*8-1 > +gdt_boot_descr_addr: gdt_boot_base would seem a better name; with C in mind what you use currently could be easily mistaken for a variable holding &gdt_boot_descr. > @@ -263,12 +271,8 @@ __start: > cld > cli > > -/* Initialise GDT and basic data segments. */ > -lgdt%cs:sym_phys(gdt_boot_descr) > -mov $BOOT_DS,%ecx > -mov %ecx,%ds > -mov %ecx,%es > -mov %ecx,%ss > +/* Load default Xen image base address. */ > +mov $sym_phys(__image_base__),%ebp Isn't this always zero? I.e. wouldn't you better use "xor %ebp,%ebp"? > /* Save the Multiboot info struct (after relocation) for later use. > */ > -mov $sym_phys(cpu0_stack)+1024,%esp > +lea (sym_offset(cpu0_stack)+1024)(%ebp),%esp Please avoid obfuscating the code by adding pointless parentheses. > @@ -390,72 +432,111 @@ trampoline_setup: > > /* Stash TSC to calculate a good approximation of time-since-boot */ > rdtsc > -mov %eax,sym_phys(boot_tsc_stamp) > -mov %edx,sym_phys(boot_tsc_stamp+4) > +mov %eax,%fs:sym_offset(boot_tsc_stamp) > +mov %edx,%fs:sym_offset(boot_tsc_stamp+4) > > -/* Initialise L2 boot-map page table entries (16MB). */ > -mov $sym_phys(l2_bootmap),%edx > -mov $PAGE_HYPERVISOR|_PAGE_PSE,%eax > -mov $8,%ecx > +/* Update frame addreses in page tables. */ > +lea sym_offset(__page_tables_start)(%ebp),%edx > +mov $((__page_tables_end-__page_tables_start)/8),%ecx > +1: testl $_PAGE_PRESENT,(%edx) > +jz 2f > +add %ebp,(%edx) > +2: add $8,%edx > +loop1b > + > +/* Initialise L2 boot-map page table entries (14MB). */ > +lea sym_offset(l2_bootmap)(%ebp),%edx > +lea sym_offset(start)(%ebp),%eax > +and $~((1< +mov %eax,%ebx > +shr $(L2_PAGETABLE_SHIFT-3),%ebx > +and $(L2_PAGETABLE_ENTRIES*4*8-1),%ebx > +add %ebx,%edx > +add $(PAGE_HYPERVISOR|_PAGE_PSE),%eax > +mov $7,%ecx > 1: mov %eax,(%edx) > add $8,%edx > add $(1< loop1b > + > /* Initialise L3 boot-map page directory entry. */ > -mov $sym_phys(l2_bootmap)+__PAGE_HYPERVISOR,%eax > -mov %eax,sym_phys(l3_bootmap) + 0*8 > +lea (sym_offset(l2_bootmap)+__PAGE_HYPERVISOR)(%ebp),%eax > +lea sym_offset(l3_bootmap)(%ebp),%ebx > +mov $4,%ecx > +1: mov %eax,(%ebx) > +add $8,%ebx > +add $(L2_PAGETABLE_ENTRIES*8),%eax > +loop1b > + > +/* Initialise L2 direct map page table entries (14MB). */ > +lea sym_offset(l2_identmap)(%ebp),%edx > +lea sym_offset(start)(%ebp),%eax > +and $~((1< +mov %eax,%ebx > +shr $(L2_PAGETABLE_SHIFT-3),%ebx > +and $(L2_PAGETABLE_ENTRIES*4*8-1),%ebx > +
Re: [PATCH v2 21/23] x86/boot: implement early command line parser in C
>>> On 20.07.15 at 16:29, 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
Re: [PATCH v2 20/23] x86: add multiboot2 protocol support for EFI platforms
>>> On 20.07.15 at 16:29, wrote:
> Signed-off-by: Daniel Kiper
For a patch of this size, no description at all seems rather
problematic.
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -89,6 +89,13 @@ multiboot1_header_end:
> 0, /* Number of the lines - no preference. */ \
> 0 /* Number of bits per pixel - no preference. */
>
> +/* Do not disable EFI boot services. */
> +mb2ht_init MB2_HT(EFI_BS), MB2_HT(OPTIONAL)
> +
> +/* EFI64 entry point. */
> +mb2ht_init MB2_HT(ENTRY_ADDRESS_EFI64), MB2_HT(OPTIONAL), \
> + sym_phys(__efi64_start)
Iirc at least one of those two is what upstream doesn't have yet,
or not all earlier grub2 versions might have. If so - what happens
if the resulting Xen gets booted on an incapable grub? Silent death
(with black screen)? Or at least some note to the user that the
grub2 version is not suitable?
> @@ -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
> +run_bs:
Again.
> +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.)
> @@ -182,7 +318,7 @@ multiboot2_proto:
> and $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
> jmp 0b
>
> -trampoline_setup:
> +trampoline_bios_setup:
Another .L candidate.
> /* Set up trampoline segment 64k below EBDA */
> movzwl 0x40e,%ecx /* EBDA segment */
> cmp $0xa000,%ecx/* sanity check (high) */
> @@ -198,12 +334,13 @@ trampoline_setup:
> * multiboot structure (if available) and use the smallest.
> */
> cmp $0x100,%edx /* is the multiboot value too small? */
> -jb 2f /* if so, do not use it */
> +jb trampoline_setup/* if so, do not use it */
> shl $10-4,%edx
> cmp %ecx,%edx /* compare with BDA value */
> cmovb %edx,%ecx /* and use the smaller */
>
> -2: /* Reserve 64kb for the trampoline */
> +trampoline_setup:
> +/* Reserve 64kb for the trampoline. */
And one more.
> @@ -220,6 +357,13 @@ trampoline_setup:
> add $12,%esp/* Remove reloc() args from stack. */
> mov %eax,sym_phys(multiboot_ptr)
>
> +/*
> + * Do not zero BSS on EFI platform here.
> + * It was initialized earlier.
> + */
> +cmpb$1,sym_phys(skip_realmode)
> +je 1f
So why can't this be done uniformly here? I didn't see you write any
variable in .bss by now. And if there was any, moving it to .data
would avoid zeroing .bss in two different places. Or wait - I see, it's
the C code in efi_multiboot2() that you need to take care of. Well,
probably okay then this way, but please extend the comment at the
new zeroing site to say _why_ it needs to be done differently/earlier.
> +paddr_t __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE
> *SystemTable)
> +{
> +EFI_GRAPHICS_OUTPUT_PROTOCOL *gop;
> +UINTN cols, gop_mode = ~0, rows;
> +
> +set_bit(EFI_PLATFORM, &efi.flags);
> +
> +efi_init(ImageHandle, SystemTable);
> +
> +efi_console_set_mode();
> +
> +if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode,
> + &cols, &rows) == EFI_SUCCESS )
> +efi_arch_console_init(cols, rows);
> +
> +gop = efi_get_gop();
> +gop_mode = efi_find_gop_mode(gop, 0, 0, 0);
> +efi_arch_edd();
> +efi_tables();
> +setup_efi_pci();
> +efi_variables();
> +efi_set_gop_mode(gop, gop_mode);
> +efi_exit_boot(ImageHandle, SystemTable);
> +
> +return cfg.addr;
With all the refactoring in the earlier patches it is now almost impossible
to know what cfg.addr holds at this point - it looks to me as if this is
entirely unrelated to the mem_lower value the caller wants to derive
from this. Pl
Re: [PATCH v2 19/23] x86/efi: create new early memory allocator
>>> On 20.07.15 at 16:29, 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: [PATCH v2 10/23] efi: build xen.gz with EFI code
>>> On 26.08.15 at 14:33, 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 10/23] efi: build xen.gz with EFI code
>>> On 25.08.15 at 18:31, wrote: > On Tue, Aug 25, 2015 at 06:09:09AM -0600, Jan Beulich wrote: >> >>> On 24.08.15 at 22:54, wrote: >> > On Mon, Aug 24, 2015 at 05:35:21AM -0600, Jan Beulich wrote: >> >> >>> On 22.08.15 at 15:59, wrote: >> >> > On Thu, Aug 20, 2015 at 09:39:39AM -0600, Jan Beulich wrote: >> >> >> >>> On 20.07.15 at 16:29, 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
Re: [PATCH v2 10/23] efi: build xen.gz with EFI code
>>> On 24.08.15 at 22:54, wrote: > On Mon, Aug 24, 2015 at 05:35:21AM -0600, Jan Beulich wrote: >> >>> On 22.08.15 at 15:59, wrote: >> > On Thu, Aug 20, 2015 at 09:39:39AM -0600, Jan Beulich wrote: >> >> >>> On 20.07.15 at 16:29, 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 2>disabled && echo y)) >> >> > efi := $(if $(efi),$(shell $(LD) -mi386pep --subsystem=10 -o check.efi >> >> > heck.o >disabled && e
Re: [PATCH v2 09/23] efi: create efi_enabled()
>>> On 22.08.15 at 14:33, wrote:
> On Thu, Aug 20, 2015 at 09:18:17AM -0600, Jan Beulich wrote:
>> >>> On 20.07.15 at 16:29, wrote:
>> > --- a/xen/arch/x86/efi/stub.c
>> > +++ b/xen/arch/x86/efi/stub.c
>> > @@ -4,9 +4,14 @@
>> > #include
>> > #include
>> >
>> > -#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
>>> On 22.08.15 at 15:59, wrote: > On Thu, Aug 20, 2015 at 09:39:39AM -0600, Jan Beulich wrote: >> >>> On 20.07.15 at 16:29, 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 2>disabled && 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 14/23] efi: split out efi_find_gop_mode()
>>> On 20.07.15 at 16:29, 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
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
>>> On 20.07.15 at 16:29, 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 2>disabled && echo y)) > efi := $(if $(efi),$(shell $(LD) -mi386pep --subsystem=10 -o check.efi > check.o 2>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)? > -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: [PATCH v2 09/23] efi: create efi_enabled()
>>> On 20.07.15 at 16:29, wrote:
> --- a/xen/arch/x86/efi/stub.c
> +++ b/xen/arch/x86/efi/stub.c
> @@ -4,9 +4,14 @@
> #include
> #include
>
> -#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
> # include
> # include
> -
> -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 08/23] x86: add multiboot2 protocol support
>>> On 18.08.15 at 14:00, wrote:
> On Tue, Aug 18, 2015 at 02:12:58AM -0600, Jan Beulich wrote:
>> >>> On 20.07.15 at 16:29, 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 08/23] x86: add multiboot2 protocol support
>>> On 20.07.15 at 16:29, 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
Re: [PATCH v2 07/23] x86/boot/reloc: Rename some variables and rearrange code a bit
>>> On 20.07.15 at 16:29, 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 Acked-by: Jan Beulich ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2 05/23] x86/boot/reloc: create generic alloc and copy functions
>>> On 20.07.15 at 16:29, 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 > Reviewed-by: Andrew Cooper > --- > 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 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
>>> On 20.07.15 at 16:28, 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 > */ > > -/* 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: [Xen-devel] [PATCH v2 22/23] x86: make Xen early boot code relocatable
>>> On 14.08.15 at 16:37, wrote: > On Fri, Aug 14, 2015 at 08:32:05AM -0600, Jan Beulich wrote: >> >>> On 14.08.15 at 15:59, wrote: >> > On Fri, Aug 14, 2015 at 06:49:18AM -0600, Jan Beulich wrote: >> >> >>> On 14.08.15 at 13:52, 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
>>> On 14.08.15 at 15:59, wrote: > On Fri, Aug 14, 2015 at 06:49:18AM -0600, Jan Beulich wrote: >> >>> On 14.08.15 at 13:52, 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
>>> On 14.08.15 at 13:52, 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
>>> On 13.08.15 at 21:22, 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
>>> On 20.07.15 at 16:28, 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: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms
>>> On 27.03.15 at 15:57, wrote: > On Fri, Mar 27, 2015 at 02:34:19PM +, Jan Beulich wrote: >> >>> On 27.03.15 at 15:26, wrote: >> > On Fri, Mar 27, 2015 at 01:36:32PM +, Jan Beulich wrote: >> >> >>> On 27.03.15 at 14:06, wrote: >> >> > On Tue, Mar 17, 2015 at 10:32:01AM +, Jan Beulich wrote: >> >> >> >>> On 30.01.15 at 18:54, 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 18/18] x86: add multiboot2 protocol support for EFI platforms
>>> On 27.03.15 at 15:26, wrote: > On Fri, Mar 27, 2015 at 01:36:32PM +, Jan Beulich wrote: >> >>> On 27.03.15 at 14:06, wrote: >> > On Tue, Mar 17, 2015 at 10:32:01AM +, Jan Beulich wrote: >> >> >>> On 30.01.15 at 18:54, 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
>>> On 27.03.15 at 15:09, wrote: > On Fri, Mar 27, 2015 at 02:04:22PM +, Jan Beulich wrote: >> >>> On 27.03.15 at 14:53, wrote: >> > On 27/03/15 13:43, Jan Beulich wrote: >> >>>>> On 27.03.15 at 14:32, wrote: >> >>> On Fri, Feb 20, 2015 at 04:17:35PM +, Jan Beulich wrote: >> >>>>>>> On 30.01.15 at 18:54, 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 05/18] efi: split efi_enabled to efi_platform and efi_loader
>>> On 27.03.15 at 14:53, wrote: > On 27/03/15 13:43, Jan Beulich wrote: >>>>> On 27.03.15 at 14:32, wrote: >>> On Fri, Feb 20, 2015 at 04:17:35PM +, Jan Beulich wrote: >>>>>>> On 30.01.15 at 18:54, 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 05/18] efi: split efi_enabled to efi_platform and efi_loader
>>> On 27.03.15 at 14:32, wrote: > On Fri, Feb 20, 2015 at 04:17:35PM +, Jan Beulich wrote: >> >>> On 30.01.15 at 18:54, 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
>>> On 27.03.15 at 14:06, wrote: > On Tue, Mar 17, 2015 at 10:32:01AM +, Jan Beulich wrote: >> >>> On 30.01.15 at 18:54, 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 17/18] x86/efi: create new early memory allocator
>>> On 27.03.15 at 13:57, wrote:
> On Mon, Mar 02, 2015 at 05:23:49PM +, Jan Beulich wrote:
>> >>> On 30.01.15 at 18:54, 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(L"Out 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 04/18] xen/x86: add multiboot2 protocol support
>>> On 27.03.15 at 13:22, wrote:
> On Fri, Mar 27, 2015 at 11:20:10AM +, Jan Beulich wrote:
>> >>> On 27.03.15 at 11:56, wrote:
>> > On Fri, Feb 20, 2015 at 04:06:26PM +, Jan Beulich wrote:
>> >> >>> On 30.01.15 at 18:54, 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 16/18] efi: create efi_exit_boot()
>>> On 27.03.15 at 13:00, 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 08/18] efi: build xen.gz with EFI code
>>> On 27.03.15 at 12:14, 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 04/18] xen/x86: add multiboot2 protocol support
>>> On 27.03.15 at 11:56, wrote:
> On Fri, Feb 20, 2015 at 04:06:26PM +, Jan Beulich wrote:
>> >>> On 30.01.15 at 18:54, 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 18/18] x86: add multiboot2 protocol support for EFI platforms
>>> On 30.01.15 at 18:54, 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 .L 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
Re: [PATCH 05/18] efi: split efi_enabled to efi_platform and efi_loader
>>> On 03.03.15 at 00:40, 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 17/18] x86/efi: create new early memory allocator
>>> On 02.03.15 at 21:25, wrote:
> On Mon, Mar 2, 2015 at 9:23 AM, Jan Beulich wrote:
>>>>> On 30.01.15 at 18:54, 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 17/18] x86/efi: create new early memory allocator
>>> On 30.01.15 at 18:54, 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(L"Out 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 16/18] efi: create efi_exit_boot()
>>> On 30.01.15 at 18:54, 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 08/18] efi: build xen.gz with EFI code
>>> On 30.01.15 at 18:54, 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 07/18] efi: run EFI specific code on EFI platform only
>>> On 30.01.15 at 18:54, 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 05/18] efi: split efi_enabled to efi_platform and efi_loader
>>> On 30.01.15 at 18:54, 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
> #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 04/18] xen/x86: add multiboot2 protocol support
>>> On 30.01.15 at 18:54, 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
Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms
>>> On 10.02.15 at 22:27, 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 00/18] x86: multiboot2 protocol support
>>> On 09.02.15 at 18:59, 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 00/18] x86: multiboot2 protocol support
>>> On 05.02.15 at 12:50, wrote: > Le 2015-02-04 10:51, "Jan Beulich" a écrit : >> >> >>> On 04.02.15 at 10:04, 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, 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 00/18] x86: multiboot2 protocol support
>>> On 04.02.15 at 10:04, 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, 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 02/18] x86/boot/reloc: create generic alloc and copy functions
>>> On 30.01.15 at 18:54, 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
>>> On 30.01.15 at 18:54, 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
>>> On 30.01.15 at 18:54, 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: [Xen-devel] Loading a 32 bit kernel from 64 bit grub-xen
>>> On 01.07.14 at 20:27, 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 >> пишет: >> >>> -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: EFI and multiboot2 devlopment work for Xen
>>> On 28.10.13 at 19:01, Vladimir 'f-coder/phcoder' >>> Serbinenko 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] EFI and multiboot2 devlopment work for Xen
>>> Vladimir 'φ-coder/phcoder' Serbinenko 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: [Xen-devel] EFI and multiboot2 devlopment work for Xen
>>> Konrad Rzeszutek Wilk 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
>>> Ian Campbell 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: EFI and multiboot2 devlopment work for Xen
>>> On 22.10.13 at 16:51, Konrad Rzeszutek Wilk 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: EFI and multiboot2 devlopment work for Xen
>>> On 22.10.13 at 11:45, Ian Campbell wrote: > On Tue, 2013-10-22 at 10:31 +0100, Jan Beulich wrote: >> >>> On 22.10.13 at 11:26, Ian Campbell 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
>>> On 22.10.13 at 15:53, Ian Campbell 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
>>> On 22.10.13 at 11:26, Ian Campbell 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
>>> On 21.10.13 at 20:39, Daniel Kiper wrote: > On Mon, Oct 21, 2013 at 02:36:38PM +0100, Jan Beulich wrote: >> >>> On 21.10.13 at 14:57, Daniel Kiper 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
>>> On 21.10.13 at 20:46, Daniel Kiper wrote: > On Mon, Oct 21, 2013 at 03:37:21PM +0100, Jan Beulich wrote: >> >>> On 21.10.13 at 16:23, Konrad Rzeszutek Wilk >> >>> wrote: >> > However my understanding is that the general distro approach is >> > to use GRUB2 and I think we want to follow the mainstream on this. >> > Which means using GRUB2 and making sense of the myrid of patches >> > that each distro has. >> >> As does ours - and we simply use the chain loading mechanism as >> I'm told (and as I suggested - I'm only occasionally involved in the >> secure boot stuff). > > Do you think about GRUB2 chainloader command? Sure. 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
>>> On 21.10.13 at 16:23, Konrad Rzeszutek Wilk wrote: > On Mon, Oct 21, 2013 at 02:36:38PM +0100, Jan Beulich wrote: >> >>> On 21.10.13 at 14:57, Daniel Kiper 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.) > > I see this: > > From: Daniel Kiper > To: boris.ostrov...@oracle.com, david.woodho...@intel.com, > ian.campb...@citrix.com, jbeul...@suse.com, k...@xen.org, > > > You are on the 'To' instead of the 'CC'. That should make the email > arrive at your mailbox much quicker than through the mailing list? Indeed - I was clearly looking at the wrong place. I'm very sorry. >> > 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. > > However my understanding is that the general distro approach is > to use GRUB2 and I think we want to follow the mainstream on this. > Which means using GRUB2 and making sense of the myrid of patches > that each distro has. As does ours - and we simply use the chain loading mechanism as I'm told (and as I suggested - I'm only occasionally involved in the secure boot stuff). 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
>>> On 21.10.13 at 14:57, Daniel Kiper 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
