| Message ID | 1460723596-13261-11-git-send-email-daniel.kiper@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> 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. > --- 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 flag has got to do with runtime services. But more on this below. > @@ -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 the trailing comma - that's there for a reason. > --- 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 grained set of flags Linux uses nowadays? That would also eliminate the odd connection to runtime services mentioned earlier. 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. > @@ -40,6 +42,12 @@ int efi_runtime_call(struct xenpf_efi_runtime_call *); > int efi_compat_get_info(uint32_t idx, union compat_pf_efi_info *); > int efi_compat_runtime_call(struct compat_pf_efi_runtime_call *); > > +/* Test whether the above EFI_* bits are enabled. */ The comment leaves open which EFI_* values you actually refer to. Hence another option would be to move those #define-s here. > +static inline bool_t efi_enabled(int feature) unsigned int > +{ > + return test_bit(feature, &efi.flags) != 0; > +} Please use the more conventional !! found elsewhere in our code. Jan
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
>>> @@ -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? If you put in a trailing comma, subsequent patches to add a further item become a 1 line addition, rather than a 1 subtraction and 2 addition. It makes patches for future additions smaller and more clear. ~Andrew
>>> On 25.05.16 at 19:15, <daniel.kiper@oracle.com> wrote: > 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. As you say, the initial value for flags is zero, with or without your addition. Hence the addition is pointless. >> > --- 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". Well, that's what was clear from the beginning. The question however was (taken together with the second one) what it means functionality wise. The later addition makes clear it doesn't mean "loaded directly from EFI". But looking at the various flags Linux has here, what functionality does it imply? Does it e.g. mean runtime services are to be used? If so, the flag would need to be cleared when their use if being suppressed. >> 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. We don't need all of it, sure. But some more fine grained identification of what functionality is available / to be used would surely benefit us as a whole and your patch series in particular. Jan
On Fri, May 27, 2016 at 02:22:39AM -0600, Jan Beulich wrote: > >>> On 25.05.16 at 19:15, <daniel.kiper@oracle.com> wrote: > > 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/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". > > Well, that's what was clear from the beginning. The question however > was (taken together with the second one) what it means functionality > wise. The later addition makes clear it doesn't mean "loaded directly This means that we run on EFI platform and we can use its features, e.g. runtime services, get info from it about ACPI, SMBIOS, etc. > from EFI". But looking at the various flags Linux has here, what Yep. > functionality does it imply? Does it e.g. mean runtime services are to > be used? If so, the flag would need to be cleared when their use if As above: not only. > being suppressed. If we need that (e.g. for ARM) then we should create e.g. EFI_RS. > >> 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. > > We don't need all of it, sure. But some more fine grained > identification of what functionality is available / to be used > would surely benefit us as a whole and your patch series in > particular. As above. Daniel
>>> On 01.06.16 at 17:23, <daniel.kiper@oracle.com> wrote: > On Fri, May 27, 2016 at 02:22:39AM -0600, Jan Beulich wrote: >> >>> On 25.05.16 at 19:15, <daniel.kiper@oracle.com> wrote: >> > 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/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". >> >> Well, that's what was clear from the beginning. The question however >> was (taken together with the second one) what it means functionality >> wise. The later addition makes clear it doesn't mean "loaded directly > > This means that we run on EFI platform and we can use its features, > e.g. runtime services, get info from it about ACPI, SMBIOS, etc. > >> from EFI". But looking at the various flags Linux has here, what > > Yep. > >> functionality does it imply? Does it e.g. mean runtime services are to >> be used? If so, the flag would need to be cleared when their use if > > As above: not only. I.e. we're back at me asking you to make this at least a little more fine grained. >> being suppressed. > > If we need that (e.g. for ARM) then we should create e.g. EFI_RS. Why only then? We already can suppress the use of runtime services. >> >> 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. >> >> We don't need all of it, sure. But some more fine grained >> identification of what functionality is available / to be used >> would surely benefit us as a whole and your patch series in >> particular. > > As above. Well, above you don't really reason on why this coarse granularity is good enough. Hence my response can only be: As above. Jan
On Wed, Jun 01, 2016 at 09:41:42AM -0600, Jan Beulich wrote: > >>> On 01.06.16 at 17:23, <daniel.kiper@oracle.com> wrote: > > On Fri, May 27, 2016 at 02:22:39AM -0600, Jan Beulich wrote: > >> >>> On 25.05.16 at 19:15, <daniel.kiper@oracle.com> wrote: > >> > 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/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". > >> > >> Well, that's what was clear from the beginning. The question however > >> was (taken together with the second one) what it means functionality > >> wise. The later addition makes clear it doesn't mean "loaded directly > > > > This means that we run on EFI platform and we can use its features, > > e.g. runtime services, get info from it about ACPI, SMBIOS, etc. > > > >> from EFI". But looking at the various flags Linux has here, what > > > > Yep. > > > >> functionality does it imply? Does it e.g. mean runtime services are to > >> be used? If so, the flag would need to be cleared when their use if > > > > As above: not only. > > I.e. we're back at me asking you to make this at least a little more > fine grained. You mean EFI_PLATFORM, EFI_LOADER and EFI_RS? Is it OK for you? Anything else? > >> being suppressed. > > > > If we need that (e.g. for ARM) then we should create e.g. EFI_RS. > > Why only then? We already can suppress the use of runtime services. Sorry, I forgot about that. Daniel
>>> On 01.06.16 at 21:28, <daniel.kiper@oracle.com> wrote: > On Wed, Jun 01, 2016 at 09:41:42AM -0600, Jan Beulich wrote: >> >>> On 01.06.16 at 17:23, <daniel.kiper@oracle.com> wrote: >> > On Fri, May 27, 2016 at 02:22:39AM -0600, Jan Beulich wrote: >> >> >>> On 25.05.16 at 19:15, <daniel.kiper@oracle.com> wrote: >> >> > 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/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". >> >> >> >> Well, that's what was clear from the beginning. The question however >> >> was (taken together with the second one) what it means functionality >> >> wise. The later addition makes clear it doesn't mean "loaded directly >> > >> > This means that we run on EFI platform and we can use its features, >> > e.g. runtime services, get info from it about ACPI, SMBIOS, etc. >> > >> >> from EFI". But looking at the various flags Linux has here, what >> > >> > Yep. >> > >> >> functionality does it imply? Does it e.g. mean runtime services are to >> >> be used? If so, the flag would need to be cleared when their use if >> > >> > As above: not only. >> >> I.e. we're back at me asking you to make this at least a little more >> fine grained. > > You mean EFI_PLATFORM, EFI_LOADER and EFI_RS? Is it OK for you? > Anything else? Probably that's enough for a first cut, looking at Linux'es. But please use EFI_RUNTIME_SERVICES and EFI_BOOT (the latter for one of the first two you mention, perhaps the first, as imo "platform", as mentioned above, is giving too little description). Jan
diff --git a/xen/arch/x86/dmi_scan.c b/xen/arch/x86/dmi_scan.c index 8e07f8d..4cd38c8 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_PLATFORM)) { if (efi_smbios3_size && !(instance & 1)) { *base = efi_smbios3_address; *len = efi_smbios3_size; @@ -702,7 +702,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_PLATFORM) ? 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..fdf0d8a 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_PLATFORM) && efi_rs_using_pgtables() ) return NULL; /* diff --git a/xen/arch/x86/efi/stub.c b/xen/arch/x86/efi/stub.c index e6c99b5..c5ae369 100644 --- 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. */ .acpi = EFI_INVALID_TABLE_ADDR, .acpi20 = EFI_INVALID_TABLE_ADDR, .mps = EFI_INVALID_TABLE_ADDR, diff --git a/xen/arch/x86/mpparse.c b/xen/arch/x86/mpparse.c index ef6557c..ff26b5a 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_PLATFORM)) 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_PLATFORM)) { efi_check_config(); return; } diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index c5c332d..5d80868 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -434,8 +434,8 @@ static void __init parse_video_info(void) { struct boot_video_info *bvi = &bootsym(boot_vid_info); - /* The EFI loader fills vga_console_info directly. */ - if ( efi_enabled ) + /* vga_console_info is filled directly on EFI platform. */ + if ( efi_enabled(EFI_PLATFORM) ) return; if ( (bvi->orig_video_isVGA == 1) && (bvi->orig_video_mode == 3) ) @@ -715,7 +715,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) if ( !(mbi->flags & MBI_MODULES) || (mbi->mods_count == 0) ) panic("dom0 kernel not specified. Check bootloader configuration."); - if ( efi_enabled ) + if ( efi_enabled(EFI_PLATFORM) ) { set_pdx_range(xen_phys_start >> PAGE_SHIFT, (xen_phys_start + BOOTSTRAP_MAP_BASE) >> PAGE_SHIFT); @@ -826,7 +826,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) * we can relocate the dom0 kernel and other multiboot modules. Also, on * x86/64, we relocate Xen to higher memory. */ - for ( i = 0; !efi_enabled && i < mbi->mods_count; i++ ) + for ( i = 0; !efi_enabled(EFI_PLATFORM) && i < mbi->mods_count; i++ ) { if ( mod[i].mod_start & (PAGE_SIZE - 1) ) panic("Bootloader didn't honor module alignment request."); @@ -1057,7 +1057,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) if ( !xen_phys_start ) panic("Not enough memory to relocate Xen."); - reserve_e820_ram(&boot_e820, efi_enabled ? mbi->mem_upper : __pa(&_start), + reserve_e820_ram(&boot_e820, efi_enabled(EFI_PLATFORM) ? mbi->mem_upper : __pa(&_start), __pa(&_end)); /* Late kexec reservation (dynamic start address). */ diff --git a/xen/arch/x86/shutdown.c b/xen/arch/x86/shutdown.c index 0e1499d..79dcd16 100644 --- a/xen/arch/x86/shutdown.c +++ b/xen/arch/x86/shutdown.c @@ -116,7 +116,7 @@ void machine_halt(void) static void default_reboot_type(void) { if ( reboot_type == BOOT_INVALID ) - reboot_type = efi_enabled ? BOOT_EFI + reboot_type = efi_enabled(EFI_PLATFORM) ? BOOT_EFI : acpi_disabled ? BOOT_KBD : BOOT_ACPI; } diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c index 687e39b..ffd39b2 100644 --- a/xen/arch/x86/time.c +++ b/xen/arch/x86/time.c @@ -686,7 +686,7 @@ static unsigned long get_cmos_time(void) static bool_t __read_mostly cmos_rtc_probe; boolean_param("cmos-rtc-probe", cmos_rtc_probe); - if ( efi_enabled ) + if ( efi_enabled(EFI_PLATFORM) ) { res = efi_get_time(); if ( res ) diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 125c9ce..f006575 100644 --- 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 + efi_init(ImageHandle, SystemTable); use_cfg_file = efi_arch_use_config_file(SystemTable); diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c index ae87557..aa064e7 100644 --- a/xen/common/efi/runtime.c +++ b/xen/common/efi/runtime.c @@ -10,14 +10,10 @@ DEFINE_XEN_GUEST_HANDLE(CHAR16); #ifndef COMPAT -#ifdef CONFIG_ARM /* Disabled until runtime services implemented */ -const bool_t efi_enabled = 0; -#else +#ifndef CONFIG_ARM # include <asm/i387.h> # include <asm/xstate.h> # include <public/platform.h> - -const bool_t efi_enabled = 1; #endif unsigned int __read_mostly efi_num_ct; @@ -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 }; const struct efi_pci_rom *__read_mostly efi_pci_roms; diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c index 8a28d87..eb8bc12 100644 --- a/xen/drivers/acpi/osl.c +++ b/xen/drivers/acpi/osl.c @@ -66,7 +66,7 @@ void __init acpi_os_vprintf(const char *fmt, va_list args) acpi_physical_address __init acpi_os_get_root_pointer(void) { - if (efi_enabled) { + if (efi_enabled(EFI_PLATFORM)) { if (efi.acpi20 != EFI_INVALID_TABLE_ADDR) return efi.acpi20; else if (efi.acpi != EFI_INVALID_TABLE_ADDR) diff --git a/xen/include/xen/efi.h b/xen/include/xen/efi.h index e74dad1..659c7c4 100644 --- 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 + /* Add fields here only if they need to be referenced from non-EFI code. */ struct efi { + unsigned long flags; unsigned long mps; /* MPS table */ unsigned long acpi; /* ACPI table (IA64 ext 0.71) */ unsigned long acpi20; /* ACPI table (ACPI 2.0) */ @@ -40,6 +42,12 @@ int efi_runtime_call(struct xenpf_efi_runtime_call *); int efi_compat_get_info(uint32_t idx, union compat_pf_efi_info *); int efi_compat_runtime_call(struct compat_pf_efi_runtime_call *); +/* Test whether the above EFI_* bits are enabled. */ +static inline bool_t efi_enabled(int feature) +{ + return test_bit(feature, &efi.flags) != 0; +} + #endif /* !__ASSEMBLY__ */ #endif /* __XEN_EFI_H__ */
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 only defines EFI_PLATFORM feature which is equal to old efi_enabled == 1. Following patch will define EFI_LOADER feature accordingly. Suggested-by: Jan Beulich <jbeulich@suse.com> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> --- 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 | 5 +---- xen/arch/x86/mpparse.c | 4 ++-- xen/arch/x86/setup.c | 10 +++++----- xen/arch/x86/shutdown.c | 2 +- xen/arch/x86/time.c | 2 +- xen/common/efi/boot.c | 4 ++++ xen/common/efi/runtime.c | 17 +++++++---------- xen/drivers/acpi/osl.c | 2 +- xen/include/xen/efi.h | 12 ++++++++++-- 11 files changed, 35 insertions(+), 29 deletions(-)