Re: [PATCH v4 1/5] efi: Introduce EFI_DIRECT flag

2014-05-20 Thread Jan Beulich 
 On 19.05.14 at 22:46, daniel.ki...@oracle.com wrote:
 On Mon, May 19, 2014 at 02:30:45PM +0100, Jan Beulich wrote:
  On 16.05.14 at 22:41, daniel.ki...@oracle.com wrote:
  @@ -457,6 +460,21 @@ void __init efi_free_boot_services(void)
 efi_unmap_memmap();
   }
 
  +static void __init __iomem *efi_early_ioremap(resource_size_t phys_addr,
  +  unsigned long size)
  +{
  +  if (efi_enabled(EFI_DIRECT))
  +  return early_ioremap(phys_addr, size);
  +
  +  return (__force void __iomem *)phys_addr;

 Now that surely needs some explanation: I can't see how this can
 ever be correct, Xen or not being completely irrelevant.
 
 I hope that efi_enabled(EFI_DIRECT) is obvious. However, in case of
 !efi_enabled(EFI_DIRECT) some structures are created artificially
 and they live in virtual address space. So that is why they should
 not be mapped. If you wish I could add relevant comment here.

That would be the very minimum I suppose. But I wonder whether
you wouldn't be better off storing their physical addresses in the
first place (and then decide whether you can stay with early_ioremap()
or want/need to use early_memremap() if !EFI_DIRECT).

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

2014-05-20 Thread David Vrabel 
On 16/05/14 21:41, Daniel Kiper wrote:
 Put EFI machinery for Xen in place.

Put what machinery to do what?

 @@ -1714,6 +1725,21 @@ asmlinkage __visible void __init xen_start_kernel(void)
  
   xen_setup_runstate_info(0);
  
 + efi_systab_xen = xen_efi_probe();
 +
 + if (efi_systab_xen) {
 + strncpy((char *)boot_params.efi_info.efi_loader_signature, 
 Xen,
 + 
 sizeof(boot_params.efi_info.efi_loader_signature));
 + boot_params.efi_info.efi_systab = 
 (__u32)((__u64)efi_systab_xen);
 + boot_params.efi_info.efi_systab_hi = 
 (__u32)((__u64)efi_systab_xen  32);
 +
 + x86_platform.get_wallclock = efi_get_time;

x86_platform.get_wallclock should always be xen_get_wallclock().

 + x86_platform.set_wallclock = efi_set_rtc_mmss;
 +
 + set_bit(EFI_BOOT, efi.flags);
 + set_bit(EFI_64BIT, efi.flags);
 + }
 +
   /* Start the world */
  #ifdef CONFIG_X86_32
   i386_start_kernel();
 --- /dev/null
 +++ b/drivers/xen/efi.c
 @@ -0,0 +1,374 @@
 + * Copyright (c) 2014 Daniel Kiper, Oracle Corporation

Is this really copyright by you personally and not Oracle?


 +#define call (op.u.efi_runtime_call)
 +#define DECLARE_CALL(what) \
 + struct xen_platform_op op; \
 + op.cmd = XENPF_efi_runtime_call; \
 + call.function = XEN_EFI_##what; \
 + call.misc = 0

Macros that declare local variables are awful.

Use what Andrew suggested and something like

struct xen_blah *call = op.u.efi_runtime_call;


 +static const struct efi efi_xen __initconst = {
 + .systab   = NULL, /* Initialized later. */
 + .runtime_version  = 0,/* Initialized later. */
 + .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,
 + .fw_vendor= EFI_INVALID_TABLE_ADDR,
 + .runtime  = EFI_INVALID_TABLE_ADDR,
 + .config_table = 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,
 + .query_variable_info  = xen_efi_query_variable_info,
 + .update_capsule   = xen_efi_update_capsule,
 + .query_capsule_caps   = xen_efi_query_capsule_caps,
 + .get_next_high_mono_count = xen_efi_get_next_high_mono_count,
 + .reset_system = NULL, /* Functionality provided by Xen. */

Xen provides functionality to reset (just maybe not via an EFI call).
Should an implementation be provided that does this?

 + .set_virtual_address_map  = NULL, /* Not used under Xen. */
 + .memmap   = NULL, /* Not used under Xen. */
 + .flags= 0 /* Initialized later. */
 +};
 +
 +efi_system_table_t __init *xen_efi_probe(void)
 +{
 + struct xen_platform_op op = {
 + .cmd = XENPF_firmware_info,
 + .u.firmware_info = {
 + .type = XEN_FW_EFI_INFO,
 + .index = XEN_FW_EFI_CONFIG_TABLE
 + }
 + };
 + union xenpf_efi_info *info = op.u.firmware_info.u.efi_info;
 +
 + if (!xen_initial_domain() || HYPERVISOR_dom0_op(op))
 + return NULL;

if (!xen_initial_domain())
return NULL;

if (HYPERVISOR_dom0_op(op)  0)
return NULL;

 +
 + /* Here we know that Xen runs on EFI platform. */
 +
 + efi = efi_xen;
 +
 + op.cmd = XENPF_firmware_info;
 + op.u.firmware_info.type = XEN_FW_EFI_INFO;
 + op.u.firmware_info.index = XEN_FW_EFI_VENDOR;
 + info-vendor.bufsz = sizeof(vendor);
 + set_xen_guest_handle(info-vendor.name, vendor);
 +
 + if (!HYPERVISOR_dom0_op(op)) {

if (HYPERVISOR_dom0_op(op) == 0)

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

2014-05-20 Thread Daniel Kiper 
On Tue, May 20, 2014 at 10:47:00AM +0100, David Vrabel wrote:
 On 16/05/14 21:41, Daniel Kiper wrote:
  Put EFI machinery for Xen in place.

 Put what machinery to do what?

  @@ -1714,6 +1725,21 @@ asmlinkage __visible void __init 
  xen_start_kernel(void)
 
  xen_setup_runstate_info(0);
 
  +   efi_systab_xen = xen_efi_probe();
  +
  +   if (efi_systab_xen) {
  +   strncpy((char *)boot_params.efi_info.efi_loader_signature, 
  Xen,
  +   
  sizeof(boot_params.efi_info.efi_loader_signature));
  +   boot_params.efi_info.efi_systab = 
  (__u32)((__u64)efi_systab_xen);
  +   boot_params.efi_info.efi_systab_hi = 
  (__u32)((__u64)efi_systab_xen  32);
  +
  +   x86_platform.get_wallclock = efi_get_time;

 x86_platform.get_wallclock should always be xen_get_wallclock().

Hmmm... Make sens... Jan, why did you replace x86_platform.get_wallclock
with efi_get_time() in your implementation?

  +   x86_platform.set_wallclock = efi_set_rtc_mmss;

Now I am not sure even about that. As I can see Linux Kernel
running on bare metal EFI platform directly sets RTC omitting
efi_set_rtc_mmss(). Am I missing something? IIRC, there was
discussion about that once. But where and when?

Anyway, now I think that this initialization should be done
in arch/x86/xen/time.c if needed.

  +
  +   set_bit(EFI_BOOT, efi.flags);
  +   set_bit(EFI_64BIT, efi.flags);
  +   }
  +
  /* Start the world */
   #ifdef CONFIG_X86_32
  i386_start_kernel();
  --- /dev/null
  +++ b/drivers/xen/efi.c
  @@ -0,0 +1,374 @@
  + * Copyright (c) 2014 Daniel Kiper, Oracle Corporation

 Is this really copyright by you personally and not Oracle?

As I know it is correct but I will double check it.



  +#define call (op.u.efi_runtime_call)
  +#define DECLARE_CALL(what) \
  +   struct xen_platform_op op; \
  +   op.cmd = XENPF_efi_runtime_call; \
  +   call.function = XEN_EFI_##what; \
  +   call.misc = 0

 Macros that declare local variables are awful.

 Use what Andrew suggested and something like

 struct xen_blah *call = op.u.efi_runtime_call;


  +static const struct efi efi_xen __initconst = {
  +   .systab   = NULL, /* Initialized later. */
  +   .runtime_version  = 0,/* Initialized later. */
  +   .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,
  +   .fw_vendor= EFI_INVALID_TABLE_ADDR,
  +   .runtime  = EFI_INVALID_TABLE_ADDR,
  +   .config_table = 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,
  +   .query_variable_info  = xen_efi_query_variable_info,
  +   .update_capsule   = xen_efi_update_capsule,
  +   .query_capsule_caps   = xen_efi_query_capsule_caps,
  +   .get_next_high_mono_count = xen_efi_get_next_high_mono_count,
  +   .reset_system = NULL, /* Functionality provided by Xen. */

 Xen provides functionality to reset (just maybe not via an EFI call).
 Should an implementation be provided that does this?

I do not think so. efi.reset_system() would not be called on Xen because
all reset related stuff is replaced by Xen reset specific functions.
Please check arch/x86/xen/enlighten.c. Additionally, I think that
situation is a bit similar to time issue. If we are running on Xen
then we should ask Xen to do reset because Xen controls platform
and it knows how to do that.

  +   .set_virtual_address_map  = NULL, /* Not used under Xen. */
  +   .memmap   = NULL, /* Not used under Xen. */
  +   .flags= 0 /* Initialized later. */
  +};
  +
  +efi_system_table_t __init *xen_efi_probe(void)
  +{
  +   struct xen_platform_op op = {
  +   .cmd = XENPF_firmware_info,
  +   .u.firmware_info = {
  +   .type = XEN_FW_EFI_INFO,
  +   .index = XEN_FW_EFI_CONFIG_TABLE
  +   }
  +   };
  +   union xenpf_efi_info *info = op.u.firmware_info.u.efi_info;
  +
  +   if (!xen_initial_domain() || HYPERVISOR_dom0_op(op))
  +   return NULL;

   if (!xen_initial_domain())
   return NULL;

   if (HYPERVISOR_dom0_op(op)  0)
   return

Re: kernel 3.14.2 oops: seems related to EFI

2014-05-20 Thread Matt Fleming 
On Mon, 19 May, at 09:09:58AM, Francis Moreau wrote:
 
 I don't know, I can't really afford to configure/compile/test this new
 kernel, sorry.

It would be useful to know whether this issue still occurs when booting
with the efi=old_map kernel parameter.

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

2014-05-20 Thread Jan Beulich 
 On 20.05.14 at 13:29, daniel.ki...@oracle.com wrote:
 On Tue, May 20, 2014 at 10:47:00AM +0100, David Vrabel wrote:
 On 16/05/14 21:41, Daniel Kiper wrote:
  @@ -1714,6 +1725,21 @@ asmlinkage __visible void __init 
  xen_start_kernel(void)
 
 xen_setup_runstate_info(0);
 
  +  efi_systab_xen = xen_efi_probe();
  +
  +  if (efi_systab_xen) {
  +  strncpy((char *)boot_params.efi_info.efi_loader_signature, 
  Xen,
  +  
  sizeof(boot_params.efi_info.efi_loader_signature));
  +  boot_params.efi_info.efi_systab = 
  (__u32)((__u64)efi_systab_xen);
  +  boot_params.efi_info.efi_systab_hi = 
  (__u32)((__u64)efi_systab_xen  32);
  +
  +  x86_platform.get_wallclock = efi_get_time;

 x86_platform.get_wallclock should always be xen_get_wallclock().
 
 Hmmm... Make sens... Jan, why did you replace x86_platform.get_wallclock
 with efi_get_time() in your implementation?

On the basis that (for Dom0 only) this is the equivalent of (and
actually also falls back to) mach_get_cmos_time(). If on Dom0
.get_wallclock doesn't get set to mach_get_cmos_time() on
pv-ops, then that line above should also be dropped.

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

2014-05-20 Thread David Vrabel 
On 20/05/14 12:29, Daniel Kiper wrote:
 
 +   if (!xen_initial_domain() || HYPERVISOR_dom0_op(op))
 +   return NULL;

  if (!xen_initial_domain())
  return NULL;

  if (HYPERVISOR_dom0_op(op)  0)
  return NULL;
 
 What is wrong with my if?

Style.  The function returns -ve on error not a boolean success/fail.

 +
 +   /* Here we know that Xen runs on EFI platform. */
 +
 +   efi = efi_xen;
 +
 +   op.cmd = XENPF_firmware_info;
 +   op.u.firmware_info.type = XEN_FW_EFI_INFO;
 +   op.u.firmware_info.index = XEN_FW_EFI_VENDOR;
 +   info-vendor.bufsz = sizeof(vendor);
 +   set_xen_guest_handle(info-vendor.name, vendor);
 +
 +   if (!HYPERVISOR_dom0_op(op)) {

 if (HYPERVISOR_dom0_op(op) == 0)
 
 Ditto?

Again, style.

David
--
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: kernel 3.14.2 oops: seems related to EFI

2014-05-20 Thread Francis Moreau 
On 05/20/2014 01:54 PM, Matt Fleming wrote:
 On Mon, 19 May, at 09:09:58AM, Francis Moreau wrote:

 I don't know, I can't really afford to configure/compile/test this new
 kernel, sorry.
 
 It would be useful to know whether this issue still occurs when booting
 with the efi=old_map kernel parameter.
 

ok I can try to boot with that parameter and see if the issue happens
again. Unfortunately if it doesn't, we couldn't tell.

Thanks
--
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: [GIT PULL] EFI changes for arm64

2014-05-20 Thread Catalin Marinas 
On Mon, May 19, 2014 at 11:50:42AM +0100, Matt Fleming wrote:
 On Wed, 30 Apr, at 09:03:32PM, Matt Fleming wrote:
  I pulled the arm64 EFI changes into the following topic branch. Catalin
  was happy for this to go through tip, which definitely makes things
  easier for the next merge window because of the dependency these patches
  have on tip/x86/efi.
  
  The following changes since commit e33655a386ed3b26ad36fb97a47ebb1c2ca1e928:
  
efivars: Add compatibility code for compat tasks (2014-04-17 13:53:53 
  +0100)
  
  are available in the git repository at:
  
git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git arm64-efi
  
  for you to fetch changes up to 345c736edd07b657a8c48190baed2719b85d0938:
  
efi/arm64: ignore dtb= when UEFI SecureBoot is enabled (2014-04-30 
  19:57:06 +0100)
 
 Ping?

Alternatively, I can merge the arm64-efi tree and wait for tip x86/efi
to go in before sending my pull request. Peter, Ingo, do you have any
preference?

Either way, I'd like to see this branch in -next to make sure there are
no (significant) conflicts before the merging window.

Thanks.

-- 
Catalin
--
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: [GIT PULL] EFI changes for v3.16

2014-05-20 Thread Matt Fleming 
On Mon, 19 May, at 03:47:31PM, H. Peter Anvin wrote:
 
 How on earth does this solve anything?  The only thing we add here is a
 WARN_ON_ONCE()... but the above text already tells us we have a problem.
 
 It seems, rather, that we need to figure out how to deal with a pstore
 in this case.  There are a few possibilities:
 
 1. We could keep an XSAVE buffer area around for this particular use.
I am *assuming* we don't let more than one CPU into EFI, because I
cannot for my life imagine that this is safe in typical CPUs.
 
Correct. This is actually prohibited by the spec, so we have a lock to
enforce it.

 2. Drop the pstore on the floor if !irq_fpu_usable().
 
 3. Allow the pstore, then die (on the assumption that we're dead
anyway.)

Personally, I'd prefer 2, but I'm open to suggestions.

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