Re: [Xen-devel] [PATCH v5 13/16] x86: add multiboot2 protocol support for EFI platforms

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 31.08.16 at 19:07, <daniel.kiper@xxxxxxxxxx> wrote:
> On Wed, Aug 31, 2016 at 06:49:51AM -0600, Jan Beulich wrote:
>> >>> On 30.08.16 at 21:32, <daniel.kiper@xxxxxxxxxx> wrote:
>> > On Thu, Aug 25, 2016 at 06:59:54AM -0600, Jan Beulich wrote:
>> >> >>> On 20.08.16 at 00:43, <daniel.kiper@xxxxxxxxxx> wrote:
>> >> > --- a/xen/arch/x86/efi/stub.c
>> >> > +++ b/xen/arch/x86/efi/stub.c
>> >> > @@ -3,6 +3,30 @@
>> >> >  #include <xen/init.h>
>> >> >  #include <xen/lib.h>
>> >> >  #include <asm/page.h>
>> >> > +#include <asm/efibind.h>
>> >> > +#include <efi/efidef.h>
>> >> > +#include <efi/eficapsule.h>
>> >> > +#include <efi/eficon.h>
>> >> > +#include <efi/efidevp.h>
>> >> > +#include <efi/efiapi.h>
>> >> > +
>> >> > +paddr_t __init noreturn efi_multiboot2(EFI_HANDLE ImageHandle, 
>> >> > EFI_SYSTEM_TABLE *SystemTable)
>> >> > +{
>> >> > +    CHAR16 *err = L"Xen does not have EFI code build in!!!\r\nSystem 
>> >> > halted!!!\r\n";
>> >> > +    SIMPLE_TEXT_OUTPUT_INTERFACE *StdErr;
>> >> > +
>> >> > +    StdErr = SystemTable->StdErr ? SystemTable->StdErr : 
>> >> > SystemTable->ConOut;
>> >> > +
>> >> > +    /* Print error message and halt the system. */
>> >> > +    asm volatile(
>> >> > +    "    call %2                      \n"
>> >> > +    "0:  hlt                          \n"
>> >> > +    "    jmp  0b                      \n"
>> >> > +       : "+c" (StdErr), "+d" (err) : "g" (StdErr->OutputString)
>> >> > +       : "rax", "r8", "r9", "r10", "r11", "cc", "memory");
>> >>
>> >> There are explanations missing here: First, a warning should be added
>> >> alongside the EFI header inclusions, making clear that no services
>> >> whatsoever can be called. And then the asm() here needs to explain
>> >
>> > I am not convinced but if you wish...
>>
>> Not convinced of what?
> 
> About "... a warning should be added alongside the EFI header inclusions,
> making clear that no services whatsoever can be called". AIUI, "warning" ==
> "comment" here. However, I think that everybody who reads this file is
> aware that "no services whatsoever can be called". So, I am not sure
> where is the point.

Odd - you do an EFI call here (in the asm()) and talk about reader's
awareness?

>> >> need for an explicit "cc" clobber on x86.
>> >
>> > Why?
>>
>> Because such a clobber gets added to every asm() by the compiler,
>> unless it uses the (new in gcc 6) flag output. I've actually suggested
>> to upstream a patch making it possible to avoid that automatic
>> addition, but there hadn't been a whole lot of useful feedback.
> 
> So, when somebody uses this new flag then "cc" will not be add here.
> This is not big deal but I think that extra "cc" clobbers does not
> hurt too.

It surely doesn't hurt, but it makes code bigger and hence results in
it taking longer to be read and parsed (for all of these - even if just
slightly). I'm sorry, but I'm opposed to adding unnecessary stuff.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel