Re: [PATCH v3 10/16] efi: create efi_enabled()

From: Daniel Kiper <daniel.kiper@oracle.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Juergen Gross <JGross@suse.com>,
	stefano.stabellini@eu.citrix.com, 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, richard.l.maliszewski@intel.com,
	gang.wei@intel.com, fu.wei@linaro.org
Subject: Re: [PATCH v3 10/16] efi: create efi_enabled()
Date: Wed, 25 May 2016 19:15:35 +0200	[thread overview]
Message-ID: <20160525171535.GM5490@olila.local.net-space.pl> (raw)
In-Reply-To: <57456E5702000078000EE897@prv-mh.provo.novell.com>

On Wed, May 25, 2016 at 01:20:23AM -0600, Jan Beulich wrote:
> >>> On 15.04.16 at 14:33, <daniel.kiper@oracle.com> wrote:
> > --- a/xen/arch/x86/efi/stub.c
> > +++ b/xen/arch/x86/efi/stub.c
> > @@ -4,11 +4,8 @@
> >  #include <xen/lib.h>
> >  #include <asm/page.h>
> >
> > -#ifndef efi_enabled
> > -const bool_t efi_enabled = 0;
> > -#endif
> > -
> >  struct efi __read_mostly efi = {
> > +	.flags   = 0, /* Initialized later. */
>
> This is pointless to add - the field will get zero-initialized anyway.

Sure thing. However, I think that we should be clear here that
there is no default value for .flags (well, it is 0). Though if
you wish I can remove that.

> > --- a/xen/common/efi/boot.c
> > +++ b/xen/common/efi/boot.c
> > @@ -934,6 +934,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
>
> Surely this can be __set_bit()? It's also hard to see what setting this

OK.

> flag has got to do with runtime services. But more on this below.

Well, comment is not the best one here... I will fix it.

> > @@ -42,11 +38,12 @@ UINT64 __read_mostly efi_boot_remain_var_store_size;
> >  UINT64 __read_mostly efi_boot_max_var_size;
> >
> >  struct efi __read_mostly efi = {
> > -	.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,
> > +	.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
> >  };
>
> This, again, is an unnecessary hunk. And in no case should you drop

Ditto.

> the trailing comma - that's there for a reason.

What is the reason for trailing comma?

> > --- a/xen/include/xen/efi.h
> > +++ b/xen/include/xen/efi.h
> > @@ -2,15 +2,17 @@
> >  #define __XEN_EFI_H__
> >
> >  #ifndef __ASSEMBLY__
> > +#include <xen/bitops.h>
> >  #include <xen/types.h>
> >  #endif
> >
> > -extern const bool_t efi_enabled;
> > -
> >  #define EFI_INVALID_TABLE_ADDR (~0UL)
> >
> > +#define EFI_PLATFORM	0
>
> So what does "platform" mean? Did you consider using the more fine

It means "EFI platform". It differentiates from "legacy BIOS platform".

> grained set of flags Linux uses nowadays? That would also eliminate

I wish to use just basic idea. However, I am not going to copy all
stuff from Linux. We do not need that.

> the odd connection to runtime services mentioned earlier.

That is good point. I will think how to solve that in good way.

> And please add a comment making clear that these values are bit
> positions to be used in the flags field below. I might also help to
> move this right next to the structure field.

OK.

Daniel

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