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

2016-08-31 Thread Jan Beulich 
>>> On 30.08.16 at 19:19,  wrote:
> On Thu, Aug 25, 2016 at 06:16:35AM -0600, Jan Beulich wrote:
>> >>> On 20.08.16 at 00:43,  wrote:
>> > --- a/xen/arch/x86/efi/stub.c
>> > +++ b/xen/arch/x86/efi/stub.c
>> > @@ -4,9 +4,10 @@
>> >  #include 
>> >  #include 
>> >
>> > -#ifndef efi_enabled
>> > -const bool_t efi_enabled = 0;
>> > -#endif
>> > +bool_t efi_enabled(int feature)
>> > +{
>> > +return false;
>> > +}
>>
>> Plain bool please, the more that you use false. And I'm relatively
> 
> What is wrong with bool_t? bool is equal to bool_t.

With the (recent) introduction of bool we want to switch away from
bool_t.

>> certain I did ask for the parameter type to be unsigned int, as you
>> don't mean to pass negative values.
> 
> Sure. However, full blown efi_enabled() calls test_bit(). Latter calls
> constant_test_bit() or variable_test_bit() which take bit position as
> int. So, I do not think that efi_enabled() should take unsigned int. Or
> we should change constant_test_bit() and variable_test_bit() respectively.
> Hmmm... No, we should not. It looks that negative offsets are valid.

But in the context here you mean an unsigned value, so you should
use an unsigned type. Feel free to ASSERT(... <= INT_MAX) if you
really care.

>> > @@ -739,8 +739,10 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>> >  l3_bootmap[l3_table_offset(BOOTSTRAP_MAP_BASE)] =
>> >  l3e_from_paddr(__pa(l2_bootmap), __PAGE_HYPERVISOR);
>> >
>> > -memmap_type = loader;
>> > +memmap_type = "EFI";
>> >  }
>> > +else if ( efi_enabled(EFI_BOOT) )
>> > +memmap_type = "EFI";
>>
>> Is the change ahead of the "else" really necessary?
> 
> No. However, in this case memmap_type is always "EFI" and we do not need
> to get this value from loader variable. Which, of course, is always also
> "EFI" (to be precise pointer to) here but IMO "memmap_type = loader" suggests
> that memmap_type could be anything else.

Well - it's been okay so far, so I see no reason to change it, unless
you want to do this in a separate patch with an acceptable explanation.

Jan


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

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

2016-08-30 Thread Daniel Kiper 
On Thu, Aug 25, 2016 at 06:16:35AM -0600, Jan Beulich wrote:
> >>> On 20.08.16 at 00:43,  wrote:
> > --- a/xen/arch/x86/efi/stub.c
> > +++ b/xen/arch/x86/efi/stub.c
> > @@ -4,9 +4,10 @@
> >  #include 
> >  #include 
> >
> > -#ifndef efi_enabled
> > -const bool_t efi_enabled = 0;
> > -#endif
> > +bool_t efi_enabled(int feature)
> > +{
> > +return false;
> > +}
>
> Plain bool please, the more that you use false. And I'm relatively

What is wrong with bool_t? bool is equal to bool_t.

> certain I did ask for the parameter type to be unsigned int, as you
> don't mean to pass negative values.

Sure. However, full blown efi_enabled() calls test_bit(). Latter calls
constant_test_bit() or variable_test_bit() which take bit position as
int. So, I do not think that efi_enabled() should take unsigned int. Or
we should change constant_test_bit() and variable_test_bit() respectively.
Hmmm... No, we should not. It looks that negative offsets are valid.

> > @@ -739,8 +739,10 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >  l3_bootmap[l3_table_offset(BOOTSTRAP_MAP_BASE)] =
> >  l3e_from_paddr(__pa(l2_bootmap), __PAGE_HYPERVISOR);
> >
> > -memmap_type = loader;
> > +memmap_type = "EFI";
> >  }
> > +else if ( efi_enabled(EFI_BOOT) )
> > +memmap_type = "EFI";
>
> Is the change ahead of the "else" really necessary?

No. However, in this case memmap_type is always "EFI" and we do not need
to get this value from loader variable. Which, of course, is always also
"EFI" (to be precise pointer to) here but IMO "memmap_type = loader" suggests
that memmap_type could be anything else.

> > @@ -1078,7 +1080,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >
> >  if ( !xen_phys_start )
> >  panic("Not enough memory to relocate Xen.");
> > -reserve_e820_ram(_e820, efi_enabled ? mbi->mem_upper : 
> > __pa(&_start),
> > +reserve_e820_ram(_e820, efi_enabled(EFI_BOOT) ? mbi->mem_upper : 
> > __pa(&_start),
>
> Is this really EFI_BOOT and not EFI_LOADER?

Yep, it should be EFI_LOADER. I am not sure why it has changed to EFI_BOOT.
Or it did not... Anyway, it is my fault. I will fix it.

Daniel

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

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

2016-08-25 Thread Jan Beulich 
>>> On 20.08.16 at 00:43,  wrote:
> --- a/xen/arch/x86/efi/stub.c
> +++ b/xen/arch/x86/efi/stub.c
> @@ -4,9 +4,10 @@
>  #include 
>  #include 
>  
> -#ifndef efi_enabled
> -const bool_t efi_enabled = 0;
> -#endif
> +bool_t efi_enabled(int feature)
> +{
> +return false;
> +}

Plain bool please, the more that you use false. And I'm relatively
certain I did ask for the parameter type to be unsigned int, as you
don't mean to pass negative values.

> @@ -739,8 +739,10 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>  l3_bootmap[l3_table_offset(BOOTSTRAP_MAP_BASE)] =
>  l3e_from_paddr(__pa(l2_bootmap), __PAGE_HYPERVISOR);
>  
> -memmap_type = loader;
> +memmap_type = "EFI";
>  }
> +else if ( efi_enabled(EFI_BOOT) )
> +memmap_type = "EFI";

Is the change ahead of the "else" really necessary?

> @@ -1078,7 +1080,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>  
>  if ( !xen_phys_start )
>  panic("Not enough memory to relocate Xen.");
> -reserve_e820_ram(_e820, efi_enabled ? mbi->mem_upper : 
> __pa(&_start),
> +reserve_e820_ram(_e820, efi_enabled(EFI_BOOT) ? mbi->mem_upper : 
> __pa(&_start),

Is this really EFI_BOOT and not EFI_LOADER?

> @@ -1153,7 +1160,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
> *SystemTable)
>  
>  #ifndef CONFIG_ARM /* TODO - runtime service support */
>  
> -static bool_t __initdata efi_rs_enable = 1;
>  static bool_t __initdata efi_map_uc;
>  
>  static void __init parse_efi_param(char *s)
> @@ -1171,7 +1177,10 @@ static void __init parse_efi_param(char *s)
>  *ss = '\0';
>  
>  if ( !strcmp(s, "rs") )
> -efi_rs_enable = val;
> +{
> +if ( !val )
> +__clear_bit(EFI_RS, _flags);

"else __set_bit(...)" to continue to allow to override an earlier
"efi=no-rs".

> @@ -43,6 +37,9 @@ UINT64 __read_mostly efi_boot_max_var_store_size;
>  UINT64 __read_mostly efi_boot_remain_var_store_size;
>  UINT64 __read_mostly efi_boot_max_var_size;
>  
> +/* Bit fields representing available EFI features/properties. */

Bit field ...

> +unsigned long efi_flags;

An unsigned int would suffice for now, afaict.

> @@ -53,6 +50,12 @@ struct efi __read_mostly efi = {
>  
>  const struct efi_pci_rom *__read_mostly efi_pci_roms;
>  
> +/* Test whether EFI_* bits are enabled. */
> +bool_t efi_enabled(int feature)
> +{
> +return test_bit(feature, _flags);
> +}

Please either make the comment useful, or drop it.

> --- a/xen/include/xen/efi.h
> +++ b/xen/include/xen/efi.h
> @@ -2,13 +2,17 @@
>  #define __XEN_EFI_H__
>  
>  #ifndef __ASSEMBLY__
> +#include 

If efi_enabled() doesn't get inlined here I don't see what you need
this include for.

Jan


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

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

2016-08-19 Thread Daniel Kiper 
First of all we need to differentiate between legacy BIOS
and EFI platforms during runtime, not during build, because
one image will have legacy and EFI code and can be executed
on both platforms. Additionally, we need more fine grained
knowledge about EFI environment and check for EFI platform
and EFI loader separately to properly support multiboot2
protocol. In general Xen loaded by this protocol uses memory
mappings and loaded modules in similar way to Xen loaded by
multiboot (v1) protocol. Hence, create efi_enabled() which
checks available features in efi_flags. This patch defines
EFI_BOOT, EFI_LOADER and EFI_RS features. EFI_BOOT is equal
to old efi_enabled == 1. EFI_RS ease control on runtime
services usage. EFI_LOADER tells that Xen was loaded
directly from EFI as PE executable.

Suggested-by: Jan Beulich 
Signed-off-by: Daniel Kiper 
---
v5 - suggestions/fixes:
   - squash three patches into one
 (suggested by Jan Beulich),
   - introduce all features at once
 (suggested by Jan Beulich),
   - efi_enabled() returns bool_t
 instead of unsigned int
 (suggested by Jan Beulich),
   - update commit message.

v4 - suggestions/fixes:
   - rename EFI_PLATFORM to EFI_BOOT
 (suggested by Jan Beulich),
   - move EFI_BOOT definition to efi struct definition
 (suggested by Jan Beulich),
   - remove unneeded efi.flags initialization
 (suggested by Jan Beulich),
   - use __set_bit() instead of set_bit() if possible
 (suggested by Jan Beulich),
   - do efi_enabled() cleanup
 (suggested by Jan Beulich),
   - improve comments
 (suggested by Jan Beulich),
   - improve commit message.

v3 - suggestions/fixes:
   - define efi struct in xen/arch/x86/efi/stub.c
 in earlier patch
 (suggested by Jan Beulich),
   - improve comments
 (suggested by Jan Beulich),
   - improve commit message
 (suggested by Jan Beulich).
---
 xen/arch/x86/dmi_scan.c|4 ++--
 xen/arch/x86/domain_page.c |2 +-
 xen/arch/x86/efi/stub.c|7 ---
 xen/arch/x86/mpparse.c |4 ++--
 xen/arch/x86/setup.c   |   14 --
 xen/arch/x86/shutdown.c|4 ++--
 xen/arch/x86/time.c|2 +-
 xen/common/efi/boot.c  |   17 +
 xen/common/efi/runtime.c   |   15 +--
 xen/drivers/acpi/osl.c |2 +-
 xen/include/xen/efi.h  |9 +++--
 11 files changed, 50 insertions(+), 30 deletions(-)

diff --git a/xen/arch/x86/dmi_scan.c b/xen/arch/x86/dmi_scan.c
index b049e31..8dcb640 100644
--- a/xen/arch/x86/dmi_scan.c
+++ b/xen/arch/x86/dmi_scan.c
@@ -238,7 +238,7 @@ const char *__init dmi_get_table(paddr_t *base, u32 *len)
 {
static unsigned int __initdata instance;
 
-   if (efi_enabled) {
+   if (efi_enabled(EFI_BOOT)) {
if (efi_smbios3_size && !(instance & 1)) {
*base = efi_smbios3_address;
*len = efi_smbios3_size;
@@ -696,7 +696,7 @@ static void __init dmi_decode(struct dmi_header *dm)
 
 void __init dmi_scan_machine(void)
 {
-   if ((!efi_enabled ? dmi_iterate(dmi_decode) :
+   if ((!efi_enabled(EFI_BOOT) ? dmi_iterate(dmi_decode) :
dmi_efi_iterate(dmi_decode)) == 0)
dmi_check_system(dmi_blacklist);
else
diff --git a/xen/arch/x86/domain_page.c b/xen/arch/x86/domain_page.c
index d86f8fe..7541b91 100644
--- a/xen/arch/x86/domain_page.c
+++ b/xen/arch/x86/domain_page.c
@@ -36,7 +36,7 @@ static inline struct vcpu *mapcache_current_vcpu(void)
  * domain's page tables but current may point at another domain's VCPU.
  * Return NULL as though current is not properly set up yet.
  */
-if ( efi_enabled && efi_rs_using_pgtables() )
+if ( efi_enabled(EFI_RS) && efi_rs_using_pgtables() )
 return NULL;
 
 /*
diff --git a/xen/arch/x86/efi/stub.c b/xen/arch/x86/efi/stub.c
index 07c2bd0..f087023 100644
--- a/xen/arch/x86/efi/stub.c
+++ b/xen/arch/x86/efi/stub.c
@@ -4,9 +4,10 @@
 #include 
 #include 
 
-#ifndef efi_enabled
-const bool_t efi_enabled = 0;
-#endif
+bool_t efi_enabled(int feature)
+{
+return false;
+}
 
 void __init efi_init_memory(void) { }
 
diff --git a/xen/arch/x86/mpparse.c b/xen/arch/x86/mpparse.c
index ef6557c..c3d5bdc 100644
--- a/xen/arch/x86/mpparse.c
+++ b/xen/arch/x86/mpparse.c
@@ -564,7 +564,7 @@ static inline void __init construct_default_ISA_mptable(int 
mpc_default_type)
 
 static __init void efi_unmap_mpf(void)
 {
-   if (efi_enabled)
+   if (efi_enabled(EFI_BOOT))
clear_fixmap(FIX_EFI_MPF);
 }
 
@@ -722,7 +722,7 @@ void __init find_smp_config (void)
 {
unsigned int address;
 
-   if (efi_enabled) {
+   if (efi_enabled(EFI_BOOT)) {
efi_check_config();
return;
}
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 8ae897a..360ca59 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -439,8