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

From: Daniel Kiper <daniel.kiper@oracle.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Juergen Gross <JGross@suse.com>,
	sstabellini@kernel.org, andrew.cooper3@citrix.com,
	cardoe@cardoe.com, pgnet.dev@gmail.com, ning.sun@intel.com,
	david.vrabel@citrix.com, xen-devel@lists.xenproject.org,
	qiaowei.ren@intel.com, gang.wei@intel.com, fu.wei@linaro.org
Subject: Re: [PATCH v5 13/16] x86: add multiboot2 protocol support for EFI platforms
Date: Thu, 1 Sep 2016 22:37:37 +0200	[thread overview]
Message-ID: <20160901203737.GE20645@olila.local.net-space.pl> (raw)
In-Reply-To: <57C7F77B020000780010AB5C@prv-mh.provo.novell.com>

On Thu, Sep 01, 2016 at 01:40:11AM -0600, Jan Beulich wrote:
> >>> On 31.08.16 at 19:07, <daniel.kiper@oracle.com> wrote:
> > On Wed, Aug 31, 2016 at 06:49:51AM -0600, Jan Beulich wrote:
> >> >>> On 30.08.16 at 21:32, <daniel.kiper@oracle.com> wrote:
> >> > On Thu, Aug 25, 2016 at 06:59:54AM -0600, Jan Beulich wrote:
> >> >> >>> On 20.08.16 at 00:43, <daniel.kiper@oracle.com> 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?

I do this in quite strange way just to display clear error from file called
stub.c which contains just mostly function stubs. So, I have a feeling that
sane reader will be conscious here and will not expect code which does sensible
stuff. However, if you still think that these are insufficient then I can add
warninng/comment which assures potential reader that this is a stub file and
most functions does nothing except efi_multiboot2().

> >> >> 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.

As you wish...

Daniel

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