Re: [PATCH v3 1/5] efi: Add efi_init_ops variable

2014-03-26 Thread Matt Fleming 
On Wed, 26 Mar, at 03:02:17PM, Daniel Kiper wrote:
> On Wed, Mar 26, 2014 at 12:56:23PM +, Matt Fleming wrote:
> >
> > Please don't create another struct of EFI function pointers.
> >
> > After this we'll have 3 global efi structures defintions and 4 global
> > efi objects on x86,
> >
> >   - struct efi_early [in the boot stub]
> >   - struct efi ['efi_phys' before/'efi' after SetVirtualAddressMap()]
> >   - struct efi_init_ops [for the benefit of xen]
> 
> What do you suggest? Should we fill all EFI related structures
> in xen_efi_probe() (called from xen_start_kernel()) and later
> they should be parsed by generic/x86 EFI initialization code?
> I suppose that many variables will have NULL (or something relevant)
> in Xen dom0 and it will require some changes in EFI initialization code.
 
Yeah, that is what I was thinking. And if we have to make some changes
to the generic code to accomodate NULL fields, etc, I don't think it's a
big deal.

> > I have a big problem with exposing .efi_reserve_boot_services as an API.
> 
> Why? efi_reserve_boot_services() is public right now.

Sure it's public, but it isn't implemented on ia64 at all, not even as a
stub function, and it's only called inside x86 code (the fact that the
prototype is in include/linux/efi.h is a bit of a bug).

My concern is that if in we bake into our APIs it's going to be
difficult in the future to get rid of it or disable it, or at least
allow people to write code for EFI without caring about it.

Afterall, it's a workaround for buggy firmware.

-- 
Matt Fleming, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/5] efi: Add efi_init_ops variable

2014-03-26 Thread Daniel Kiper 
On Wed, Mar 26, 2014 at 12:56:23PM +, Matt Fleming wrote:
> On Tue, 25 Mar, at 09:57:52PM, Daniel Kiper wrote:
> > Add efi_init_ops variable which allows us to replace
> > EFI init functions on Xen with its specific stuff.
> >
> > This patch is based on Jan Beulich and Tang Liang work.
> >
> > Signed-off-by: Daniel Kiper 
> > Signed-off-by: Tang Liang 
> > ---
> >  arch/ia64/kernel/efi.c  |   30 +-
> >  arch/x86/platform/efi/efi.c |   18 +-
> >  drivers/firmware/efi/efi.c  |   26 ++
> >  include/linux/efi.h |8 
> >  4 files changed, 68 insertions(+), 14 deletions(-)
>
> [...]
>
> > @@ -1118,6 +1118,14 @@ u64 efi_mem_attributes(unsigned long phys_addr)
> > return 0;
> >  }
> >
> > +struct efi_init_ops efi_init_ops = {
> > +   .efi_init = efi_init_generic,
> > +   .efi_reserve_boot_services = efi_reserve_boot_services_generic,
> > +   .efi_enter_virtual_mode = efi_enter_virtual_mode_generic,
> > +   .efi_mem_type = efi_mem_type_generic,
> > +   .efi_mem_attributes = efi_mem_attributes_generic
> > +};
>
> Please don't create another struct of EFI function pointers.
>
> After this we'll have 3 global efi structures defintions and 4 global
> efi objects on x86,
>
>   - struct efi_early [in the boot stub]
>   - struct efi ['efi_phys' before/'efi' after SetVirtualAddressMap()]
>   - struct efi_init_ops [for the benefit of xen]

What do you suggest? Should we fill all EFI related structures
in xen_efi_probe() (called from xen_start_kernel()) and later
they should be parsed by generic/x86 EFI initialization code?
I suppose that many variables will have NULL (or something relevant)
in Xen dom0 and it will require some changes in EFI initialization code.

> I have a big problem with exposing .efi_reserve_boot_services as an API.

Why? efi_reserve_boot_services() is public right now.

Daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/5] efi: Add efi_init_ops variable

2014-03-26 Thread Matt Fleming 
On Tue, 25 Mar, at 09:57:52PM, Daniel Kiper wrote:
> Add efi_init_ops variable which allows us to replace
> EFI init functions on Xen with its specific stuff.
> 
> This patch is based on Jan Beulich and Tang Liang work.
> 
> Signed-off-by: Daniel Kiper 
> Signed-off-by: Tang Liang 
> ---
>  arch/ia64/kernel/efi.c  |   30 +-
>  arch/x86/platform/efi/efi.c |   18 +-
>  drivers/firmware/efi/efi.c  |   26 ++
>  include/linux/efi.h |8 
>  4 files changed, 68 insertions(+), 14 deletions(-)

[...]

> @@ -1118,6 +1118,14 @@ u64 efi_mem_attributes(unsigned long phys_addr)
>   return 0;
>  }
>  
> +struct efi_init_ops efi_init_ops = {
> + .efi_init = efi_init_generic,
> + .efi_reserve_boot_services = efi_reserve_boot_services_generic,
> + .efi_enter_virtual_mode = efi_enter_virtual_mode_generic,
> + .efi_mem_type = efi_mem_type_generic,
> + .efi_mem_attributes = efi_mem_attributes_generic
> +};

Please don't create another struct of EFI function pointers.

After this we'll have 3 global efi structures defintions and 4 global
efi objects on x86,

  - struct efi_early [in the boot stub]
  - struct efi ['efi_phys' before/'efi' after SetVirtualAddressMap()]
  - struct efi_init_ops [for the benefit of xen]

I have a big problem with exposing .efi_reserve_boot_services as an API.

Also, the fact that in later patches you're copying chunks of the x86
virt_*() funtions suggests to me that this has been implemented at the
wrong layer, because it actively discourages sharing code between x86
and Xen.

-- 
Matt Fleming, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 1/5] efi: Add efi_init_ops variable

2014-03-25 Thread Daniel Kiper 
Add efi_init_ops variable which allows us to replace
EFI init functions on Xen with its specific stuff.

This patch is based on Jan Beulich and Tang Liang work.

Signed-off-by: Daniel Kiper 
Signed-off-by: Tang Liang 
---
 arch/ia64/kernel/efi.c  |   30 +-
 arch/x86/platform/efi/efi.c |   18 +-
 drivers/firmware/efi/efi.c  |   26 ++
 include/linux/efi.h |8 
 4 files changed, 68 insertions(+), 14 deletions(-)

diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
index da5b462..f646374 100644
--- a/arch/ia64/kernel/efi.c
+++ b/arch/ia64/kernel/efi.c
@@ -468,8 +468,8 @@ efi_map_pal_code (void)
ia64_set_psr(psr);  /* restore psr */
 }
 
-void __init
-efi_init (void)
+static void __init
+efi_init_generic (void)
 {
void *efi_map_start, *efi_map_end;
efi_char16_t *c16;
@@ -593,8 +593,13 @@ efi_init (void)
efi_enter_virtual_mode();
 }
 
-void
-efi_enter_virtual_mode (void)
+static void
+efi_reserve_boot_services_generic (void)
+{
+}
+
+static void
+efi_enter_virtual_mode_generic (void)
 {
void *efi_map_start, *efi_map_end, *p;
efi_memory_desc_t *md;
@@ -751,8 +756,8 @@ efi_memmap_intersects (unsigned long phys_addr, unsigned 
long size)
return 0;
 }
 
-u32
-efi_mem_type (unsigned long phys_addr)
+static u32
+efi_mem_type_generic (unsigned long phys_addr)
 {
efi_memory_desc_t *md = efi_memory_descriptor(phys_addr);
 
@@ -761,8 +766,8 @@ efi_mem_type (unsigned long phys_addr)
return 0;
 }
 
-u64
-efi_mem_attributes (unsigned long phys_addr)
+static u64
+efi_mem_attributes_generic (unsigned long phys_addr)
 {
efi_memory_desc_t *md = efi_memory_descriptor(phys_addr);
 
@@ -770,7 +775,14 @@ efi_mem_attributes (unsigned long phys_addr)
return md->attribute;
return 0;
 }
-EXPORT_SYMBOL(efi_mem_attributes);
+
+struct efi_init_ops efi_init_ops = {
+   .efi_init = efi_init_generic,
+   .efi_reserve_boot_services = efi_reserve_boot_services_generic,
+   .efi_enter_virtual_mode = efi_enter_virtual_mode_generic,
+   .efi_mem_type = efi_mem_type_generic,
+   .efi_mem_attributes = efi_mem_attributes_generic
+};
 
 u64
 efi_mem_attribute (unsigned long phys_addr, unsigned long size)
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index b97acec..e4af217 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -422,7 +422,7 @@ static void __init print_efi_memmap(void)
 #endif  /*  EFI_DEBUG  */
 }
 
-void __init efi_reserve_boot_services(void)
+static void __init efi_reserve_boot_services_generic(void)
 {
void *p;
 
@@ -700,7 +700,7 @@ out:
return ret;
 }
 
-void __init efi_init(void)
+static void __init efi_init_generic(void)
 {
efi_char16_t *c16;
char vendor[100] = "unknown";
@@ -998,7 +998,7 @@ out:
  * be passed in via setup_data. In that case runtime ranges will be mapped
  * to the same virtual addresses as the first kernel.
  */
-void __init efi_enter_virtual_mode(void)
+static void __init efi_enter_virtual_mode_generic(void)
 {
efi_status_t status;
void *new_memmap = NULL;
@@ -1085,7 +1085,7 @@ void __init efi_enter_virtual_mode(void)
 /*
  * Convenience functions to obtain memory types and attributes
  */
-u32 efi_mem_type(unsigned long phys_addr)
+static u32 efi_mem_type_generic(unsigned long phys_addr)
 {
efi_memory_desc_t *md;
void *p;
@@ -1103,7 +1103,7 @@ u32 efi_mem_type(unsigned long phys_addr)
return 0;
 }
 
-u64 efi_mem_attributes(unsigned long phys_addr)
+static u64 efi_mem_attributes_generic(unsigned long phys_addr)
 {
efi_memory_desc_t *md;
void *p;
@@ -1118,6 +1118,14 @@ u64 efi_mem_attributes(unsigned long phys_addr)
return 0;
 }
 
+struct efi_init_ops efi_init_ops = {
+   .efi_init = efi_init_generic,
+   .efi_reserve_boot_services = efi_reserve_boot_services_generic,
+   .efi_enter_virtual_mode = efi_enter_virtual_mode_generic,
+   .efi_mem_type = efi_mem_type_generic,
+   .efi_mem_attributes = efi_mem_attributes_generic
+};
+
 /*
  * Some firmware has serious problems when using more than 50% of the EFI
  * variable store, i.e. it triggers bugs that can brick machines. Ensure that
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 4753bac..5ce75cf 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -137,6 +137,32 @@ static void generic_ops_unregister(void)
efivars_unregister(&generic_efivars);
 }
 
+void __init efi_init(void)
+{
+   efi_init_ops.efi_init();
+}
+
+void __init efi_reserve_boot_services(void)
+{
+   efi_init_ops.efi_reserve_boot_services();
+}
+
+void __init efi_enter_virtual_mode(void)
+{
+   efi_init_ops.efi_enter_virtual_mode();
+}
+
+u32 efi_mem_type(unsigned long phys_addr)
+{
+   return efi_init_ops.efi_mem_type(phys_addr);
+}