Re: [Xen-devel] [PATCH v3 3/5] x86: Call efi_memblock_x86_reserve_range() on native EFI platform only

2014-03-26 Thread Andrew Cooper 
On 26/03/2014 22:01, Daniel Kiper wrote:
> On Wed, Mar 26, 2014 at 01:57:23PM +, Matt Fleming wrote:
>> On Wed, 26 Mar, at 02:48:45PM, Daniel Kiper wrote:
>>> On my machine this function crashes on Xen so that is why I have changed
>>> condition. However, if you say that this issue could be solved in
>>> another way I will investigate it further.
>> Daniel, could you paste the crash? Do you get a stack trace?
> Here it is:
>
> [...]
>
> mapping kernel into physical memory
> about to get started...
> (XEN) traps.c:458:d0v0 Unhandled divide error fault/trap [#0] on VCPU 0 
> [ec=]
> (XEN) domain_crash_sync called from entry.S: fault at 82d080229d30 
> int80_direct_trap+0x200/0x210
> (XEN) Domain 0 (vcpu#0) crashed on cpu#0:
> (XEN) [ Xen-4.5-unstable  x86_64  debug=y  Tainted:C ]
> (XEN) CPU:0
> (XEN) RIP:e033:[]
> (XEN) RFLAGS: 0246   EM: 1   CONTEXT: pv guest
> (XEN) rax:    rbx: 0100   rcx: 
> (XEN) rdx:    rsi: 814d7e7e   rdi: 
> (XEN) rbp: 81601e88   rsp: 81601e48   r8:  
> (XEN) r9:  7ff0   r10: deadbeef   r11: 81601df8
> (XEN) r12: 816fe010   r13: 81601f00   r14: 
> (XEN) r15:    cr0: 80050033   cr4: 26f0
> (XEN) cr3: 7b60d000   cr2: 
> (XEN) ds:    es:    fs:    gs:    ss: e02b   cs: e033
> (XEN) Guest stack trace from rsp=81601e48:
> (XEN) 81601df8 816987c5 0001e030
> (XEN)00010046 81601e88 e02b 81601f00
> (XEN)81601ed8 8168a1a4 81601ee8 81601ea8
> (XEN)880001e16490  816fe010 
> (XEN)  81601f28 81685a3e
> (XEN)   
> (XEN)880001e16000   
> (XEN)81601f38 816854c8 81601ff8 81687442
> (XEN)0001 08000623 0789cbf580802001 03010032
> (XEN)0005   
> (XEN)   
> (XEN)   
> (XEN)   
> (XEN)   0f0060c0c748
> (XEN)c305   
> (XEN)   
> (XEN)   
> (XEN)   
> (XEN)   
> (XEN)   
> (XEN) Domain 0 crashed: rebooting machine in 5 seconds.
>
> Some addresses are solved to:
> 0x816987c5: efi_memblock_x86_reserve_range at 
> arch/x86/platform/efi/efi.c:393
> 0x8168a1a4: setup_arch at arch/x86/kernel/setup.c:940
> 0x81685a3e: setup_command_line at init/main.c:353
> 0x816854c8: x86_64_start_reservations at arch/x86/kernel/head64.c:194
> 0x81687442: xen_start_kernel at arch/x86/xen/enlighten.c:1733
>
> I am using Linus tree with latest commit 
> b098d6726bbfb94c06d6e1097466187afddae61f
> (Linux 3.14-rc8) with my patches applied excluding patch 3.
>
> Daniel

Then all you need to do is look up 816987c5 in your linux
symbols, and whichever variable is being divided on that line of source
has ether has the value 0 or -1. 

~Andrew
--
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 3/5] x86: Call efi_memblock_x86_reserve_range() on native EFI platform only

2014-03-26 Thread Daniel Kiper 
On Wed, Mar 26, 2014 at 01:57:23PM +, Matt Fleming wrote:
> On Wed, 26 Mar, at 02:48:45PM, Daniel Kiper wrote:
> >
> > On my machine this function crashes on Xen so that is why I have changed
> > condition. However, if you say that this issue could be solved in
> > another way I will investigate it further.
>
> Daniel, could you paste the crash? Do you get a stack trace?

Here it is:

[...]

mapping kernel into physical memory
about to get started...
(XEN) traps.c:458:d0v0 Unhandled divide error fault/trap [#0] on VCPU 0 
[ec=]
(XEN) domain_crash_sync called from entry.S: fault at 82d080229d30 
int80_direct_trap+0x200/0x210
(XEN) Domain 0 (vcpu#0) crashed on cpu#0:
(XEN) [ Xen-4.5-unstable  x86_64  debug=y  Tainted:C ]
(XEN) CPU:0
(XEN) RIP:e033:[]
(XEN) RFLAGS: 0246   EM: 1   CONTEXT: pv guest
(XEN) rax:    rbx: 0100   rcx: 
(XEN) rdx:    rsi: 814d7e7e   rdi: 
(XEN) rbp: 81601e88   rsp: 81601e48   r8:  
(XEN) r9:  7ff0   r10: deadbeef   r11: 81601df8
(XEN) r12: 816fe010   r13: 81601f00   r14: 
(XEN) r15:    cr0: 80050033   cr4: 26f0
(XEN) cr3: 7b60d000   cr2: 
(XEN) ds:    es:    fs:    gs:    ss: e02b   cs: e033
(XEN) Guest stack trace from rsp=81601e48:
(XEN) 81601df8 816987c5 0001e030
(XEN)00010046 81601e88 e02b 81601f00
(XEN)81601ed8 8168a1a4 81601ee8 81601ea8
(XEN)880001e16490  816fe010 
(XEN)  81601f28 81685a3e
(XEN)   
(XEN)880001e16000   
(XEN)81601f38 816854c8 81601ff8 81687442
(XEN)0001 08000623 0789cbf580802001 03010032
(XEN)0005   
(XEN)   
(XEN)   
(XEN)   
(XEN)   0f0060c0c748
(XEN)c305   
(XEN)   
(XEN)   
(XEN)   
(XEN)   
(XEN)   
(XEN) Domain 0 crashed: rebooting machine in 5 seconds.

Some addresses are solved to:
0x816987c5: efi_memblock_x86_reserve_range at 
arch/x86/platform/efi/efi.c:393
0x8168a1a4: setup_arch at arch/x86/kernel/setup.c:940
0x81685a3e: setup_command_line at init/main.c:353
0x816854c8: x86_64_start_reservations at arch/x86/kernel/head64.c:194
0x81687442: xen_start_kernel at arch/x86/xen/enlighten.c:1733

I am using Linus tree with latest commit 
b098d6726bbfb94c06d6e1097466187afddae61f
(Linux 3.14-rc8) with my patches applied excluding patch 3.

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] Add efi_early_call() macro

2014-03-26 Thread Roy Franz 
On Wed, Mar 26, 2014 at 1:59 AM, Matt Fleming  wrote:
> On Tue, 25 Mar, at 03:40:30PM, Roy Franz wrote:
>> Add the efi_early_call() macro to invoke functions in the efi_early
>> structure. Using a macro for these invocations allows the arm32/arm64
>> architectures to define the macro differently so that they can directly
>> invoke the boot services functions that are exposed in the efi_early
>> structure on x86. Prior to the introduction of the efi_early structure
>> the efi_call_physN macros were used on all architectures and allowed
>> for this differentiation.
>>
>> Signed-off-by: Roy Franz 
>> ---
>>  arch/x86/boot/compressed/eboot.c   |5 +
>>  drivers/firmware/efi/efi-stub-helper.c |   26 +-
>>  2 files changed, 18 insertions(+), 13 deletions(-)
>
> Confused.
>
> Why have you rewritten the patch that I sent Satuday morning?
>
>   https://lkml.org/lkml/2014/3/22/61
>
> I don't think your version goes far enough because you've left all the
> efi_early->call() stuff in eboot.c. So now there's two ways to invoke
> EFI functions in the x86 boot stub.
>
> What's wrong with the patch that I sent?
>
> --
> Matt Fleming, Intel Open Source Technology Center

Hi Matt,

   Sorry for the confusion - your patch is fine.  I had somehow not noticed
it on your reply.

Roy
--
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 4/5] xen: Define EFI related stuff

2014-03-26 Thread Stefano Stabellini 
On Wed, 26 Mar 2014, Jan Beulich wrote:
> >>> On 26.03.14 at 15:58,  wrote:
> >> +struct xenpf_efi_runtime_call {
> >> +  uint32_t function;
> >> +/*
> >> + * This field is generally used for per sub-function flags (defined
> >> + * below), except for the XEN_EFI_get_next_high_monotonic_count case,
> >> + * where it holds the single returned value.
> >> + */
> >> +  uint32_t misc;
> >> +  unsigned long status;
> > 
> > I realize that this is just the same as xen/include/public/platform.h,
> > but this field should be xen_ulong_t.
> 
> Care to supply a patch to the canonical header?

I will.
--
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 4/5] xen: Define EFI related stuff

2014-03-26 Thread Jan Beulich 
>>> On 26.03.14 at 15:58,  wrote:
>> +struct xenpf_efi_runtime_call {
>> +uint32_t function;
>> +/*
>> + * This field is generally used for per sub-function flags (defined
>> + * below), except for the XEN_EFI_get_next_high_monotonic_count case,
>> + * where it holds the single returned value.
>> + */
>> +uint32_t misc;
>> +unsigned long status;
> 
> I realize that this is just the same as xen/include/public/platform.h,
> but this field should be xen_ulong_t.

Care to supply a patch to the canonical header?

Jan

--
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 4/5] xen: Define EFI related stuff

2014-03-26 Thread Stefano Stabellini 
On Tue, 25 Mar 2014, Daniel Kiper wrote:
> Define EFI related stuff for Xen.
> 
> This patch is based on Jan Beulich and Tang Liang work.
> 
> Signed-off-by: Daniel Kiper 
> Signed-off-by: Tang Liang 
> Signed-off-by: Jan Beulich 
> ---
>  include/xen/interface/platform.h |  123 
> +-
>  1 file changed, 122 insertions(+), 1 deletion(-)
> 
> diff --git a/include/xen/interface/platform.h 
> b/include/xen/interface/platform.h
> index f1331e3..bb3dfad 100644
> --- a/include/xen/interface/platform.h
> +++ b/include/xen/interface/platform.h
> @@ -108,11 +108,113 @@ struct xenpf_platform_quirk {
>  };
>  DEFINE_GUEST_HANDLE_STRUCT(xenpf_platform_quirk_t);
>  
> +#define XENPF_efi_runtime_call49
> +#define XEN_EFI_get_time  1
> +#define XEN_EFI_set_time  2
> +#define XEN_EFI_get_wakeup_time   3
> +#define XEN_EFI_set_wakeup_time   4
> +#define XEN_EFI_get_next_high_monotonic_count 5
> +#define XEN_EFI_get_variable  6
> +#define XEN_EFI_set_variable  7
> +#define XEN_EFI_get_next_variable_name8
> +#define XEN_EFI_query_variable_info   9
> +#define XEN_EFI_query_capsule_capabilities   10
> +#define XEN_EFI_update_capsule   11
> +
> +struct xenpf_efi_runtime_call {
> + uint32_t function;
> +/*
> + * This field is generally used for per sub-function flags (defined
> + * below), except for the XEN_EFI_get_next_high_monotonic_count case,
> + * where it holds the single returned value.
> + */
> + uint32_t misc;
> + unsigned long status;

I realize that this is just the same as xen/include/public/platform.h,
but this field should be xen_ulong_t.


> + union {
> +#define XEN_EFI_GET_TIME_SET_CLEARS_NS 0x0001
> + struct {
> + struct xenpf_efi_time {
> + uint16_t year;
> + uint8_t month;
> + uint8_t day;
> + uint8_t hour;
> + uint8_t min;
> + uint8_t sec;
> + uint32_t ns;
> + int16_t tz;
> + uint8_t daylight;
> + } time;
> + uint32_t resolution;
> + uint32_t accuracy;
> + } get_time;
> +
> + struct xenpf_efi_time set_time;
> +
> +#define XEN_EFI_GET_WAKEUP_TIME_ENABLED 0x0001
> +#define XEN_EFI_GET_WAKEUP_TIME_PENDING 0x0002
> + struct xenpf_efi_time get_wakeup_time;
> +
> +#define XEN_EFI_SET_WAKEUP_TIME_ENABLE  0x0001
> +#define XEN_EFI_SET_WAKEUP_TIME_ENABLE_ONLY 0x0002
> + struct xenpf_efi_time set_wakeup_time;
> +
> +#define XEN_EFI_VARIABLE_NON_VOLATILE   0x0001
> +#define XEN_EFI_VARIABLE_BOOTSERVICE_ACCESS 0x0002
> +#define XEN_EFI_VARIABLE_RUNTIME_ACCESS 0x0004
> + struct {
> + GUEST_HANDLE(void) name;  /* UCS-2/UTF-16 string */
> + unsigned long size;

Same here


> + GUEST_HANDLE(void) data;
> + struct xenpf_efi_guid {
> + uint32_t data1;
> + uint16_t data2;
> + uint16_t data3;
> + uint8_t data4[8];
> + } vendor_guid;
> + } get_variable, set_variable;
> +
> + struct {
> + unsigned long size;

Same here


> + GUEST_HANDLE(void) name;  /* UCS-2/UTF-16 string */
> + struct xenpf_efi_guid vendor_guid;
> + } get_next_variable_name;
> +
> + struct {
> + uint32_t attr;
> + uint64_t max_store_size;
> + uint64_t remain_store_size;
> + uint64_t max_size;
> + } query_variable_info;
> +
> + struct {
> + GUEST_HANDLE(void) capsule_header_array;
> + unsigned long capsule_count;

Same here


> + uint64_t max_capsule_size;
> + unsigned int reset_type;

I would prefer this to be explicitly sized


> + } query_capsule_capabilities;
> +
> + struct {
> + GUEST_HANDLE(void) capsule_header_array;
> + unsigned long capsule_count;

Same here


> + uint64_t sg_list; /* machine address */
> + } update_capsule;
> + } u;
> +};
> +DEFINE_GUEST_HANDLE_STRUCT(xenpf_efi_runtime_call);
> +
> +#define  XEN_FW_EFI_VERSION0
> +#define  XEN_FW_EFI_CONFIG_TABLE   1
> +#define  XEN_FW_EFI_VENDOR 2
> +#define  XEN_FW_EFI_MEM_INFO   3
> +#define  XEN_FW_EFI_RT_VERSION 4
> +
>  #define XENPF_firmware_info   50
>  #define XEN_FW_DISK_INFO  1 /* from int 13 AH=08/41/48 */
>  #define XEN_FW_DISK_MBR_SIGNATURE 2 /* from MBR offset 0x1b8 */
>  #define XEN_FW_VBEDDC_INFO3 /* from int 10 AX=4f15 */
> +#define XEN_FW_EFI_INFO   4 /* from EFI */
>  #define XEN_FW_KBD_SHIFT_FLAGS5 /* Int16, Fn02: Get keyboard shift 
> flags. */
> +
>

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 2/5] efi: Export arch_tables variable

2014-03-26 Thread Matt Fleming 
On Wed, 26 Mar, at 03:08:00PM, Daniel Kiper wrote:
> 
> Hmmm... I am not sure which approach is better. I saw that
> in many places declarations have annotations. Could you
> point me some docs which states (and explains) that this
> is wrong idea.

Note I've had the following patch from Joe queued up for weeks. It's in
the tip tree and will be sent to Linus during the merge window.



commit 9b7d204982cf
Author: Joe Perches 
Date:   Fri Jan 3 16:08:48 2014 -0800

x86/efi: Style neatening

Coalesce formats and remove spaces before tabs.
Move __initdata after the variable declaration.

Signed-off-by: Joe Perches 
Signed-off-by: Matt Fleming 

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index dc1dd6d09dec..876dc5b9e4a3 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -67,7 +67,7 @@ struct efi_memory_map memmap;
 static struct efi efi_phys __initdata;
 static efi_system_table_t efi_systab __initdata;
 
-static __initdata efi_config_table_type_t arch_tables[] = {
+static efi_config_table_type_t arch_tables[] __initdata = {
 #ifdef CONFIG_X86_UV
{UV_SYSTEM_TABLE_GUID, "UVsystab", &efi.uv_systab},
 #endif
@@ -76,7 +76,7 @@ static __initdata efi_config_table_type_t arch_tables[] = {
 
 u64 efi_setup; /* efi setup_data physical address */
 
-static bool __initdata disable_runtime = false;
+static bool disable_runtime __initdata = false;
 static int __init setup_noefi(char *arg)
 {
disable_runtime = true;
@@ -263,9 +263,9 @@ static efi_status_t __init phys_efi_get_time(efi_time_t *tm,
 int efi_set_rtc_mmss(const struct timespec *now)
 {
unsigned long nowtime = now->tv_sec;
-   efi_status_tstatus;
-   efi_time_t  eft;
-   efi_time_cap_t  cap;
+   efi_status_tstatus;
+   efi_time_t  eft;
+   efi_time_cap_t  cap;
struct rtc_time tm;
 
status = efi.get_time(&eft, &cap);
@@ -283,9 +283,8 @@ int efi_set_rtc_mmss(const struct timespec *now)
eft.second = tm.tm_sec;
eft.nanosecond = 0;
} else {
-   printk(KERN_ERR
-  "%s: Invalid EFI RTC value: write of %lx to EFI RTC 
failed\n",
-  __FUNCTION__, nowtime);
+   pr_err("%s: Invalid EFI RTC value: write of %lx to EFI RTC 
failed\n",
+  __func__, nowtime);
return -1;
}
 
@@ -401,8 +400,7 @@ static void __init print_efi_memmap(void)
 p < memmap.map_end;
 p += memmap.desc_size, i++) {
md = p;
-   pr_info("mem%02u: type=%u, attr=0x%llx, "
-   "range=[0x%016llx-0x%016llx) (%lluMB)\n",
+   pr_info("mem%02u: type=%u, attr=0x%llx, 
range=[0x%016llx-0x%016llx) (%lluMB)\n",
i, md->type, md->attribute, md->phys_addr,
md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT),
(md->num_pages >> (20 - EFI_PAGE_SHIFT)));
@@ -434,9 +432,8 @@ void __init efi_reserve_boot_services(void)
memblock_is_region_reserved(start, size)) {
/* Could not reserve, skip it */
md->num_pages = 0;
-   memblock_dbg("Could not reserve boot range "
-   "[0x%010llx-0x%010llx]\n",
-   start, start+size-1);
+   memblock_dbg("Could not reserve boot range 
[0x%010llx-0x%010llx]\n",
+start, start+size-1);
} else
memblock_reserve(start, size);
}
@@ -572,8 +569,7 @@ static int __init efi_systab_init(void *phys)
return -EINVAL;
}
if ((efi.systab->hdr.revision >> 16) == 0)
-   pr_err("Warning: System table version "
-  "%d.%02d, expected 1.00 or greater!\n",
+   pr_err("Warning: System table version %d.%02d, expected 1.00 or 
greater!\n",
   efi.systab->hdr.revision >> 16,
   efi.systab->hdr.revision & 0x);
 
-- 
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 2/5] efi: Export arch_tables variable

2014-03-26 Thread Jan Beulich 
>>> On 26.03.14 at 15:08,  wrote:
> On Wed, Mar 26, 2014 at 01:21:19PM +, Jan Beulich wrote:
>> >>> On 25.03.14 at 21:57,  wrote:
>> > Export arch_tables variable. Xen init function calls efi_config_init()
>> > which takes it as an argument.
>> >
>> > Additionally, put __initdata in place suggested by include/linux/init.h.
>>
>> Which isn't necessarily the most appropriate place.
> 
> Why? If comments in include/linux/init.h are not valid they should be 
> changed.

Because they can't go there uniformly: While on function declarations
you can put them there, on function definitions they need to come
before the function name. And placing attributes between type and
name does - iirc - work consistently for everything.

>> > --- a/include/linux/efi.h
>> > +++ b/include/linux/efi.h
>> > @@ -583,6 +583,8 @@ extern struct efi {
>> >struct efi_memory_map *memmap;
>> >  } efi;
>> >
>> > +extern efi_config_table_type_t arch_tables[] __initdata;
>>
>> And section placement annotations are bogus on declarations.
> 
> Hmmm... I am not sure which approach is better. I saw that
> in many places declarations have annotations. Could you
> point me some docs which states (and explains) that this
> is wrong idea.

Just use common sense: Attributes that are of concern to the
caller should go on the declaration. Attributes that only affect
code generation for the function/object in question should go on
the definition.

Jan

--
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 2/5] efi: Export arch_tables variable

2014-03-26 Thread Daniel Kiper 
On Wed, Mar 26, 2014 at 01:21:19PM +, Jan Beulich wrote:
> >>> On 25.03.14 at 21:57,  wrote:
> > Export arch_tables variable. Xen init function calls efi_config_init()
> > which takes it as an argument.
> >
> > Additionally, put __initdata in place suggested by include/linux/init.h.
>
> Which isn't necessarily the most appropriate place.

Why? If comments in include/linux/init.h are not valid they should be changed.

> > --- a/arch/x86/platform/efi/efi.c
> > +++ b/arch/x86/platform/efi/efi.c
> > @@ -70,7 +70,7 @@ static efi_system_table_t efi_systab __initdata;
> >
> >  unsigned long x86_efi_facility;
> >
> > -static __initdata efi_config_table_type_t arch_tables[] = {
> > +efi_config_table_type_t arch_tables[] __initdata = {
>
> efi_config_table_type_t __initdata arch_tables[] = {
>
> would be what I'd recommend.
>
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -583,6 +583,8 @@ extern struct efi {
> > struct efi_memory_map *memmap;
> >  } efi;
> >
> > +extern efi_config_table_type_t arch_tables[] __initdata;
>
> And section placement annotations are bogus on declarations.

Hmmm... I am not sure which approach is better. I saw that
in many places declarations have annotations. Could you
point me some docs which states (and explains) that this
is wrong idea.

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 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 3/5] x86: Call efi_memblock_x86_reserve_range() on native EFI platform only

2014-03-26 Thread Matt Fleming 
On Wed, 26 Mar, at 02:48:45PM, Daniel Kiper wrote:
> 
> On my machine this function crashes on Xen so that is why I have changed
> condition. However, if you say that this issue could be solved in
> another way I will investigate it further.

Daniel, could you paste the crash? Do you get a stack trace?

-- 
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 5/5] xen: Put EFI machinery in place

2014-03-26 Thread Matt Fleming 
On Wed, 26 Mar, at 01:46:17PM, Matt Fleming wrote:
> On Wed, 26 Mar, at 01:31:04PM, Jan Beulich wrote:
> > >>> On 26.03.14 at 14:12,  wrote:
> > > 
> > > Is there a reason that you can't just populate an efi_system_table_t
> > > object, which could be used by efi_init(), so that we can save you the
> > > trouble of duplicating all of this code?
> > 
> > Would the generic function cope with all other fields being NULL (or
> > equivalent)?
>  
> I've no idea, but it shouldn't be difficult to make that work. And by
> making it work in the generic code

Guh, hit Send too soon... sorry.

What I was trying to say was that there's a real benefit to keeping all
these idiosyncrasies, such as "This field may be NULL on some platforms,
or this field, or that", in one place, because it gives you an immediate
view of what platforms Linux supports and what assumptions we can make.

It also helps immensely when the time comes to do some refactoring -
because it's all contained in one central place all the use cases are in
front of you.

Obviously this approach doesn't work for super-platform specific stuff,
but it's not clear to me that these patches fall into that category.

-- 
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 3/5] x86: Call efi_memblock_x86_reserve_range() on native EFI platform only

2014-03-26 Thread Daniel Kiper 
On Wed, Mar 26, 2014 at 01:39:42PM +, Jan Beulich wrote:
> >>> On 26.03.14 at 14:31,  wrote:
> > On Wed, 26 Mar, at 01:22:49PM, Jan Beulich wrote:
> >> >>> On 26.03.14 at 14:00,  wrote:
> >> >
> >> > This could do with a little bit more explanation. Why is it not
> >> > necessary to mark the EFI memory map that was passed to the kernel as
> >> > reserved in memblock?
> >>
> >> Because that's in memory Dom0 doesn't even see: The EFI memory
> >> map is visible to the hypervisor only.
> >
> > So where does boot_params.efi_info.efi_memmap point?
> >
> > If nowhere (i.e. it's NULL) that's no problem because memblock_reserve()
> > handles zero size regions just fine.
>
> That's a question to Daniel - in our implementation (with a separate
> Xen kernel that can't run on bare hardware) boot_params as a whole
> simply doesn't exist.

On my machine this function crashes on Xen so that is why I have changed
condition. However, if you say that this issue could be solved in
another way I will investigate it further.

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 5/5] xen: Put EFI machinery in place

2014-03-26 Thread Matt Fleming 
On Wed, 26 Mar, at 01:31:04PM, Jan Beulich wrote:
> >>> On 26.03.14 at 14:12,  wrote:
> > 
> > Is there a reason that you can't just populate an efi_system_table_t
> > object, which could be used by efi_init(), so that we can save you the
> > trouble of duplicating all of this code?
> 
> Would the generic function cope with all other fields being NULL (or
> equivalent)?
 
I've no idea, but it shouldn't be difficult to make that work. And by
making it work in the generic code

> >> +/*
> >> + * Convenience functions to obtain memory types and attributes
> >> + */
> >> +static u32 efi_mem_type_xen(unsigned long phys_addr)
> >> +{
> >> +  struct xen_platform_op op;
> >> +  union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
> >> +
> >> +  op.cmd = XENPF_firmware_info;
> >> +  op.u.firmware_info.type = XEN_FW_EFI_INFO;
> >> +  op.u.firmware_info.index = XEN_FW_EFI_MEM_INFO;
> >> +  info->mem.addr = phys_addr;
> >> +  info->mem.size = 0;
> >> +  return HYPERVISOR_dom0_op(&op) ? 0 : info->mem.type;
> >> +}
> > 
> > Same idea here. Unless you expect the EFI memory map to change at runtime
> > (and it's not clear to me whether that wouldn't cause other things to
> > explode) you'd be better off building a struct efi_memory_map and using
> > the existing generic functions.
> 
> As said in another reply to this series - the memory map isn't being
> (and shouldn't be) exposed to Dom0.
 
Right, so you might not necessarily expose the EFI memory map that was
constructed by the firmware, but it's worth looking at exposing *some*
boot memory map, e.g. the map of memory before the kernel took control
in DomU.

Whether the firmware or the kernel builds it doesn't really matter. My
point is that since we've already got a means of looking in the boot
memory map for EFI type and EFI attributes, let's use it.

-- 
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 3/5] x86: Call efi_memblock_x86_reserve_range() on native EFI platform only

2014-03-26 Thread Jan Beulich 
>>> On 26.03.14 at 14:31,  wrote:
> On Wed, 26 Mar, at 01:22:49PM, Jan Beulich wrote:
>> >>> On 26.03.14 at 14:00,  wrote:
>> > 
>> > This could do with a little bit more explanation. Why is it not
>> > necessary to mark the EFI memory map that was passed to the kernel as
>> > reserved in memblock?
>> 
>> Because that's in memory Dom0 doesn't even see: The EFI memory
>> map is visible to the hypervisor only.
> 
> So where does boot_params.efi_info.efi_memmap point?
> 
> If nowhere (i.e. it's NULL) that's no problem because memblock_reserve()
> handles zero size regions just fine.

That's a question to Daniel - in our implementation (with a separate
Xen kernel that can't run on bare hardware) boot_params as a whole
simply doesn't exist.

Jan

--
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 3/5] x86: Call efi_memblock_x86_reserve_range() on native EFI platform only

2014-03-26 Thread Matt Fleming 
On Wed, 26 Mar, at 01:22:49PM, Jan Beulich wrote:
> >>> On 26.03.14 at 14:00,  wrote:
> > 
> > This could do with a little bit more explanation. Why is it not
> > necessary to mark the EFI memory map that was passed to the kernel as
> > reserved in memblock?
> 
> Because that's in memory Dom0 doesn't even see: The EFI memory
> map is visible to the hypervisor only.

So where does boot_params.efi_info.efi_memmap point?

If nowhere (i.e. it's NULL) that's no problem because memblock_reserve()
handles zero size regions just fine.

-- 
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 5/5] xen: Put EFI machinery in place

2014-03-26 Thread Jan Beulich 
>>> On 26.03.14 at 14:12,  wrote:
> On Tue, 25 Mar, at 09:57:56PM, Daniel Kiper wrote:
>> +static void __init efi_init_xen(void)
>> +{
>> +efi_char16_t vendor_c16[100];
>> +char vendor[ARRAY_SIZE(vendor_c16)];
>> +int ret, i;
>> +struct xen_platform_op op;
>> +union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
>> +
>> +efi = efi_xen;
>> +op.cmd = XENPF_firmware_info;
>> +op.u.firmware_info.type = XEN_FW_EFI_INFO;
>> +
>> +/*
>> + * Show what we know for posterity
>> + */
>> +op.u.firmware_info.index = XEN_FW_EFI_VENDOR;
>> +info->vendor.bufsz = sizeof(vendor_c16);
>> +set_xen_guest_handle(info->vendor.name, vendor_c16);
>> +ret = HYPERVISOR_dom0_op(&op);
>> +if (!ret) {
>> +for (i = 0; i < sizeof(vendor) - 1 && vendor_c16[i]; ++i)
>> +vendor[i] = vendor_c16[i];
>> +vendor[i] = '\0';
>> +} else
>> +pr_err("Could not get the firmware vendor!\n");
>> +
> 
> Is there a reason that you can't just populate an efi_system_table_t
> object, which could be used by efi_init(), so that we can save you the
> trouble of duplicating all of this code?

Would the generic function cope with all other fields being NULL (or
equivalent)?

>> +/*
>> + * Convenience functions to obtain memory types and attributes
>> + */
>> +static u32 efi_mem_type_xen(unsigned long phys_addr)
>> +{
>> +struct xen_platform_op op;
>> +union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
>> +
>> +op.cmd = XENPF_firmware_info;
>> +op.u.firmware_info.type = XEN_FW_EFI_INFO;
>> +op.u.firmware_info.index = XEN_FW_EFI_MEM_INFO;
>> +info->mem.addr = phys_addr;
>> +info->mem.size = 0;
>> +return HYPERVISOR_dom0_op(&op) ? 0 : info->mem.type;
>> +}
> 
> Same idea here. Unless you expect the EFI memory map to change at runtime
> (and it's not clear to me whether that wouldn't cause other things to
> explode) you'd be better off building a struct efi_memory_map and using
> the existing generic functions.

As said in another reply to this series - the memory map isn't being
(and shouldn't be) exposed to Dom0.

Jan

--
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 4/5] xen: Define EFI related stuff

2014-03-26 Thread Jan Beulich 
>>> On 25.03.14 at 21:57,  wrote:
> Define EFI related stuff for Xen.
> 
> This patch is based on Jan Beulich and Tang Liang work.

And with this, ...

> Signed-off-by: Daniel Kiper 
> Signed-off-by: Tang Liang 
> Signed-off-by: Jan Beulich 

... these should be in the reverse order to reflect flow.

> +struct xenpf_efi_runtime_call {
> + uint32_t function;
> +/*
> + * This field is generally used for per sub-function flags (defined
> + * below), except for the XEN_EFI_get_next_high_monotonic_count case,
> + * where it holds the single returned value.
> + */
> + uint32_t misc;
> + unsigned long status;
> + union {

You surely would want to use consistent indentation here (i.e. a leading
hard tab)?

> @@ -143,7 +245,25 @@ struct xenpf_firmware_info {
>   /* must refer to 128-byte buffer */
>   GUEST_HANDLE(uchar) edid;
>   } vbeddc_info; /* XEN_FW_VBEDDC_INFO */
> -

Please retain blank lines like this ...

> + union xenpf_efi_info {
> + uint32_t version;
> + struct {
> + uint64_t addr;   /* EFI_CONFIGURATION_TABLE */
> + uint32_t nent;
> + } cfg;
> + struct {
> + uint32_t revision;
> + uint32_t bufsz;  /* input, in bytes */
> + GUEST_HANDLE(void) name;
> + /* UCS-2/UTF-16 string */
> + } vendor;
> + struct {
> + uint64_t addr;
> + uint64_t size;
> + uint64_t attr;
> + uint32_t type;
> + } mem;
> + } efi_info; /* XEN_FW_EFI_INFO */

... and add one here.

Jan

--
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 3/5] x86: Call efi_memblock_x86_reserve_range() on native EFI platform only

2014-03-26 Thread Jan Beulich 
>>> On 26.03.14 at 14:00,  wrote:
> On Tue, 25 Mar, at 09:57:54PM, Daniel Kiper wrote:
>> Call efi_memblock_x86_reserve_range() on native EFI platform only.
>> This is not needed and even it should not be called on platforms
>> which wraps EFI infrastructure like Xen.
>> 
>> Signed-off-by: Daniel Kiper 
>> ---
>>  arch/x86/kernel/setup.c |2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
>> index ce72964..992b67a 100644
>> --- a/arch/x86/kernel/setup.c
>> +++ b/arch/x86/kernel/setup.c
>> @@ -933,7 +933,7 @@ void __init setup_arch(char **cmdline_p)
>>  set_bit(EFI_64BIT, &x86_efi_facility);
>>  }
>>  
>> -if (efi_enabled(EFI_BOOT))
>> +if (!strncmp((char *)&boot_params.efi_info.efi_loader_signature, "EL", 
>> 2))
>>  efi_memblock_x86_reserve_range();
>>  #endif
> 
> This could do with a little bit more explanation. Why is it not
> necessary to mark the EFI memory map that was passed to the kernel as
> reserved in memblock?

Because that's in memory Dom0 doesn't even see: The EFI memory
map is visible to the hypervisor only.

Jan

--
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 2/5] efi: Export arch_tables variable

2014-03-26 Thread Jan Beulich 
>>> On 25.03.14 at 21:57,  wrote:
> Export arch_tables variable. Xen init function calls efi_config_init()
> which takes it as an argument.
> 
> Additionally, put __initdata in place suggested by include/linux/init.h.

Which isn't necessarily the most appropriate place.

> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -70,7 +70,7 @@ static efi_system_table_t efi_systab __initdata;
>  
>  unsigned long x86_efi_facility;
>  
> -static __initdata efi_config_table_type_t arch_tables[] = {
> +efi_config_table_type_t arch_tables[] __initdata = {

efi_config_table_type_t __initdata arch_tables[] = {

would be what I'd recommend.

> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -583,6 +583,8 @@ extern struct efi {
>   struct efi_memory_map *memmap;
>  } efi;
>  
> +extern efi_config_table_type_t arch_tables[] __initdata;

And section placement annotations are bogus on declarations.

Jan

--
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 5/5] xen: Put EFI machinery in place

2014-03-26 Thread Matt Fleming 
On Tue, 25 Mar, at 09:57:56PM, Daniel Kiper wrote:
> Put EFI machinery for Xen in place.
> 
> This patch is based on Jan Beulich and Tang Liang work.
> 
> Signed-off-by: Daniel Kiper 
> Signed-off-by: Tang Liang 
> Signed-off-by: Jan Beulich 
> ---
>  arch/x86/xen/enlighten.c |   10 ++
>  drivers/xen/Kconfig  |3 +
>  drivers/xen/Makefile |1 +
>  drivers/xen/efi.c|  425 
> ++
>  4 files changed, 439 insertions(+)
>  create mode 100644 drivers/xen/efi.c

[...]

> +static void __init efi_init_xen(void)
> +{
> + efi_char16_t vendor_c16[100];
> + char vendor[ARRAY_SIZE(vendor_c16)];
> + int ret, i;
> + struct xen_platform_op op;
> + union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
> +
> + efi = efi_xen;
> + op.cmd = XENPF_firmware_info;
> + op.u.firmware_info.type = XEN_FW_EFI_INFO;
> +
> + /*
> +  * Show what we know for posterity
> +  */
> + op.u.firmware_info.index = XEN_FW_EFI_VENDOR;
> + info->vendor.bufsz = sizeof(vendor_c16);
> + set_xen_guest_handle(info->vendor.name, vendor_c16);
> + ret = HYPERVISOR_dom0_op(&op);
> + if (!ret) {
> + for (i = 0; i < sizeof(vendor) - 1 && vendor_c16[i]; ++i)
> + vendor[i] = vendor_c16[i];
> + vendor[i] = '\0';
> + } else
> + pr_err("Could not get the firmware vendor!\n");
> +

Is there a reason that you can't just populate an efi_system_table_t
object, which could be used by efi_init(), so that we can save you the
trouble of duplicating all of this code?

> +/*
> + * Convenience functions to obtain memory types and attributes
> + */
> +static u32 efi_mem_type_xen(unsigned long phys_addr)
> +{
> + struct xen_platform_op op;
> + union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
> +
> + op.cmd = XENPF_firmware_info;
> + op.u.firmware_info.type = XEN_FW_EFI_INFO;
> + op.u.firmware_info.index = XEN_FW_EFI_MEM_INFO;
> + info->mem.addr = phys_addr;
> + info->mem.size = 0;
> + return HYPERVISOR_dom0_op(&op) ? 0 : info->mem.type;
> +}

Same idea here. Unless you expect the EFI memory map to change at runtime
(and it's not clear to me whether that wouldn't cause other things to
explode) you'd be better off building a struct efi_memory_map and using
the existing generic functions.

-- 
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 3/5] x86: Call efi_memblock_x86_reserve_range() on native EFI platform only

2014-03-26 Thread Matt Fleming 
On Tue, 25 Mar, at 09:57:54PM, Daniel Kiper wrote:
> Call efi_memblock_x86_reserve_range() on native EFI platform only.
> This is not needed and even it should not be called on platforms
> which wraps EFI infrastructure like Xen.
> 
> Signed-off-by: Daniel Kiper 
> ---
>  arch/x86/kernel/setup.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index ce72964..992b67a 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -933,7 +933,7 @@ void __init setup_arch(char **cmdline_p)
>   set_bit(EFI_64BIT, &x86_efi_facility);
>   }
>  
> - if (efi_enabled(EFI_BOOT))
> + if (!strncmp((char *)&boot_params.efi_info.efi_loader_signature, "EL", 
> 2))
>   efi_memblock_x86_reserve_range();
>  #endif

This could do with a little bit more explanation. Why is it not
necessary to mark the EFI memory map that was passed to the kernel as
reserved in memblock?

-- 
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 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

Re: [PATCH v2 06/13] x86/efi: Build our own EFI services pointer table

2014-03-26 Thread Matt Fleming 
On Tue, 25 Mar, at 03:47:23PM, Roy Franz wrote:
> 
> I have sent a patch (attempted to reply using git-send-email) that
> adds the macro for x86 and updates efi-stub-helper.c.  If you could
> add this to your series for 3.15 that would be great, as then we would
> not have any x86 changes in the ARM EFI stub series.

Normally my response would be "nope" because the time for getting stuff
into tip for the upcoming merge window has come and gone.

However, in this particular case I think we may be OK because it's
simple macro munging, and we can prove by looking at the generated
object code before and after that there's zero changes (which I did).

So it really comes down to what Peter and Ingo are comfortable with.

Peter, Ingo, I've attached the patch. Do you think this is something
that we could get in for the merge window, possibly the end so it gets
some time to bake in linux-next? It would help the ARM folks out for the
next cycle because it's one less dependency they need to track.

- >8 -

>From 0a64e62d455e7ff498d10d359b9cf2d136a7e1d7 Mon Sep 17 00:00:00 2001
From: Matt Fleming 
Date: Sat, 22 Mar 2014 10:09:01 +
Subject: [PATCH] efi: Abstract x86 efi_early calls

The ARM EFI boot stub doesn't need to care about the efi_early
infrastructure that x86 requires in order to do mixed mode thunking. So
wrap everything up in an efi_call_early() macro.

This allows x86 to do the necessary indirection jumps to call whatever
firmware interface is necessary (native or mixed mode), but also allows
the ARM folks to mask the fact that they don't support relocation in the
boot stub and need to pass 'sys_table_arg' to every function.

Signed-off-by: Matt Fleming 
---
 arch/x86/boot/compressed/eboot.c   | 155 -
 drivers/firmware/efi/efi-stub-helper.c |  44 +-
 2 files changed, 98 insertions(+), 101 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 5e1ba4fa3f79..1e6146137f8e 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -21,6 +21,9 @@ static efi_system_table_t *sys_table;
 
 static struct efi_config *efi_early;
 
+#define efi_call_early(f, ...) \
+   efi_early->call(efi_early->f, __VA_ARGS__);
+
 #define BOOT_SERVICES(bits)\
 static void setup_boot_services##bits(struct efi_config *c)\
 {  \
@@ -78,8 +81,8 @@ __file_size32(void *__fh, efi_char16_t *filename_16,
}
 
 grow:
-   status = efi_early->call(efi_early->allocate_pool, EFI_LOADER_DATA,
-info_sz, (void **)&info);
+   status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
+   info_sz, (void **)&info);
if (status != EFI_SUCCESS) {
efi_printk(sys_table, "Failed to alloc mem for file info\n");
return status;
@@ -88,12 +91,12 @@ grow:
status = efi_early->call((unsigned long)h->get_info, h, &info_guid,
 &info_sz, info);
if (status == EFI_BUFFER_TOO_SMALL) {
-   efi_early->call(efi_early->free_pool, info);
+   efi_call_early(free_pool, info);
goto grow;
}
 
*file_sz = info->file_size;
-   efi_early->call(efi_early->free_pool, info);
+   efi_call_early(free_pool, info);
 
if (status != EFI_SUCCESS)
efi_printk(sys_table, "Failed to get initrd info\n");
@@ -131,8 +134,8 @@ __file_size64(void *__fh, efi_char16_t *filename_16,
}
 
 grow:
-   status = efi_early->call(efi_early->allocate_pool, EFI_LOADER_DATA,
-info_sz, (void **)&info);
+   status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
+   info_sz, (void **)&info);
if (status != EFI_SUCCESS) {
efi_printk(sys_table, "Failed to alloc mem for file info\n");
return status;
@@ -141,12 +144,12 @@ grow:
status = efi_early->call((unsigned long)h->get_info, h, &info_guid,
 &info_sz, info);
if (status == EFI_BUFFER_TOO_SMALL) {
-   efi_early->call(efi_early->free_pool, info);
+   efi_call_early(free_pool, info);
goto grow;
}
 
*file_sz = info->file_size;
-   efi_early->call(efi_early->free_pool, info);
+   efi_call_early(free_pool, info);
 
if (status != EFI_SUCCESS)
efi_printk(sys_table, "Failed to get initrd info\n");
@@ -204,8 +207,8 @@ static inline efi_status_t __open_volume32(void *__image, 
void **__fh)
void *handle = (void *)(unsigned long)image->device_handle;
unsigned long func;
 
-   status = efi_early->call(efi_early->handle_protocol, handle,
-&fs_proto, (v

Re: [PATCH] Add efi_early_call() macro

2014-03-26 Thread Matt Fleming 
On Tue, 25 Mar, at 03:40:30PM, Roy Franz wrote:
> Add the efi_early_call() macro to invoke functions in the efi_early
> structure. Using a macro for these invocations allows the arm32/arm64
> architectures to define the macro differently so that they can directly
> invoke the boot services functions that are exposed in the efi_early
> structure on x86. Prior to the introduction of the efi_early structure
> the efi_call_physN macros were used on all architectures and allowed
> for this differentiation.
> 
> Signed-off-by: Roy Franz 
> ---
>  arch/x86/boot/compressed/eboot.c   |5 +
>  drivers/firmware/efi/efi-stub-helper.c |   26 +-
>  2 files changed, 18 insertions(+), 13 deletions(-)

Confused.

Why have you rewritten the patch that I sent Satuday morning?

  https://lkml.org/lkml/2014/3/22/61

I don't think your version goes far enough because you've left all the
efi_early->call() stuff in eboot.c. So now there's two ways to invoke
EFI functions in the x86 boot stub.

What's wrong with the patch that I sent?

-- 
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