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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Wed, May 25, 2016 at 01:20:23AM -0600, Jan Beulich wrote:
> >>> On 15.04.16 at 14:33, <daniel.kiper@xxxxxxxxxx> 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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel