linux-kernel - Re: [Xen-devel] [PATCH 3/5] EFI: add efi driver for Xen efi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <4F354AB6020000780007267B@nat28.tlf.novell.com>
Date:	Fri, 10 Feb 2012 15:49:58 +0000
From:	"Jan Beulich" <JBeulich@...e.com>
To:	<liang.tang@...cle.com>
Cc:	<xen-devel@...ts.xensource.com>, <konrad.wilk@...cle.com>,
	<mjg59@...f.ucam.org>, <linux-acpi@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [Xen-devel] [PATCH 3/5] EFI: add efi driver for Xen efi

>>> On 09.02.12 at 04:33, Tang Liang <liang.tang@...cle.com> wrote:

As noted in a private mail already, this lack a From:, as large parts of
this are quite obviously derived from what we have in our SLE and
openSUSE trees. Feel free to also stick my Signed-off-by in further
down.

> The efi memory is owned by Xen and efi run-time service can not be called
> directly,so a new efi driver is needed by Xen efi.
> We call efi run-time service through the Xen in Xen efi driver.
> 
> Signed-off-by: Tang Liang <liang.tang@...cle.com>
> 
> --- a/arch/x86/platform/efi/Makefile
> +++ b/arch/x86/platform/efi/Makefile
> @@ -1 +1,4 @@
>  obj-$(CONFIG_EFI) 		+= efi.o efi_$(BITS).o efi_stub_$(BITS).o
> +ifdef CONFIG_XEN
> +obj-$(CONFIG_EFI)		+= efi-xen.o

Calling this just xen.c would seem more natural to me. The *-xen.c
naming convention really is necessary only in the legacy (and forward
ported) trees.

> +endif
> diff --git a/arch/x86/platform/efi/efi-xen.c b/arch/x86/platform/efi/efi-xen.c
> new file mode 100644
> index 0000000..f4f6235
> --- /dev/null
> +++ b/arch/x86/platform/efi/efi-xen.c
> @@ -0,0 +1,432 @@
> +/*
> + * Common EFI (Extensible Firmware Interface) support functions
> + * Based on Extensible Firmware Interface Specification version 1.0
> + *
> + * Copyright (C) 1999 VA Linux Systems
> + * Copyright (C) 1999 Walt Drummond <drummond@...inux.com>
> + * Copyright (C) 1999-2002 Hewlett-Packard Co.
> + *	David Mosberger-Tang <davidm@....hp.com>
> + *	Stephane Eranian <eranian@....hp.com>
> + * Copyright (C) 2005-2008 Intel Co.
> + *	Fenghua Yu <fenghua.yu@...el.com>
> + *	Bibo Mao <bibo.mao@...el.com>
> + *	Chandramouli Narayanan <mouli@...ux.intel.com>
> + *	Huang Ying <ying.huang@...el.com>
> + * Copyright (C) 2011-2012 Oracle Co.
> + *	Liang Tang <liang.tang@...cle.com>
> + *

I think everything between here ...

> + * Copied from efi_32.c to eliminate the duplicated code between EFI
> + * 32/64 support code. --ying 2007-10-26
> + *
> + * All EFI Runtime Services are not implemented yet as EFI only
> + * supports physical mode addressing on SoftSDV. This is to be fixed
> + * in a future version.  --drummond 1999-07-20
> + *
> + * Implemented EFI runtime services and virtual mode calls.  --davidm
> + *
> + * Goutham Rao: <goutham.rao@...el.com>
> + *	Skip non-WB memory and ignore empty memory ranges.

... and here is meaningless in this file.

> + */
>...
> +struct efi __read_mostly efi_xen = {

With this just being copied in an __init function below, this can be
both static and __initdata (or even const __initconst).

> +	.mps                      = EFI_INVALID_TABLE_ADDR,
> +	.acpi                     = EFI_INVALID_TABLE_ADDR,
> +	.acpi20                   = EFI_INVALID_TABLE_ADDR,
> +	.smbios                   = EFI_INVALID_TABLE_ADDR,
> +	.sal_systab               = EFI_INVALID_TABLE_ADDR,
> +	.boot_info                = EFI_INVALID_TABLE_ADDR,
> +	.hcdp                     = EFI_INVALID_TABLE_ADDR,
> +	.uga                      = EFI_INVALID_TABLE_ADDR,
> +	.uv_systab                = EFI_INVALID_TABLE_ADDR,
> +	.get_time                 = xen_efi_get_time,
> +	.set_time                 = xen_efi_set_time,
> +	.get_wakeup_time          = xen_efi_get_wakeup_time,
> +	.set_wakeup_time          = xen_efi_set_wakeup_time,
> +	.get_variable             = xen_efi_get_variable,
> +	.get_next_variable        = xen_efi_get_next_variable,
> +	.set_variable             = xen_efi_set_variable,
> +	.get_next_high_mono_count = xen_efi_get_next_high_mono_count,
> +	.query_variable_info      = xen_efi_query_variable_info,
> +	.update_capsule           = xen_efi_update_capsule,
> +	.query_capsule_caps       = xen_efi_query_capsule_caps,
> +};
>...
> +static void efi_enter_virtual_mode_xen(void)  { };
> +static void efi_reserve_boot_services_xen(void) { };

With the generic wrapper checking for NULL pointers I don't see why you
need empty placeholders here.

> +
> +struct efi_init_funcs xen_efi_funcs = {

static const.

> +	.__efi_init		     = efi_init_xen,
> +	.__efi_reserve_boot_services = efi_reserve_boot_services_xen,
> +	.__efi_enter_virtual_mode    = efi_enter_virtual_mode_xen,
> +	.__efi_mem_type		     = efi_mem_type_xen,
> +	.__efi_mem_attributes	     = efi_mem_attributes_xen
> +};
> +
> +static void register_xen_efi_function(void)
> +{
> +	efi_init_function_register(&xen_efi_funcs);
> +}
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -476,6 +476,7 @@ extern struct efi_memory_map memmap;
>  extern void efi_init_function_register(struct efi_init_funcs *funcs);
>  extern void get_efi_table_info(efi_config_table_t *config_tables, int 
> nr_tables,
>  			       struct efi *efi_t);
> +extern void __init xen_efi_probe(void);

No __init in declarations.

Jan

>  
>  /**
>   * efi_range_is_wc - check the WC bit on an address range

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/