Re: [Patch v3 0/4] Protect against concurrent calls into UV BIOS

2019-02-14 Thread Russ Anderson 
On Thu, Feb 14, 2019 at 09:17:37AM +0100, Ard Biesheuvel wrote:
> On Wed, 13 Feb 2019 at 20:34, Hedi Berriche  wrote:
> >
> > - Changes since v2
> >   Addressed comments from Ard Biesheuvel:
> >  * expose efi_runtime_lock to UV platform only instead of globally
> >  * remove unnecessary #ifdef CONFIG_EFI from bios_uv.c
> >
> > - Changes since v1:
> >   Addressed comments from Bhupesh Sharma, Thomas Gleixner, and Ard 
> > Biesheuvel:
> >  * made __uv_bios_call() static
> >  * moved the efi_enabled() cleanup to its own patchlet
> >  * explained the reason for renaming the efi_runtime_lock semaphore
> >  * dropped the reviewed-bys as they should be given on the mailing list
> >  * Cc'ng sta...@vger.kernel.org given the nature of the problem addressed 
> > by the patches
> >
> 
> Hi Hedi,
> 
> The patches look good to me now.
> 
> For the series,
> 
> Reviewed-by: Ard Biesheuvel 
> 
> However, I don't think all the patches should go to -stable. Only 4/4
> fixes an actual bug, and the remaining patches don't look like they
> are prerequisites for that change.
> 
> Also, if your colleagues reviewed your patches, now would be the time
> to ask them to give their Reviewed-by as well.

Reviewed-by: Russ Anderson 

Thanks.

> -- 
> Ard.
> 
> 
> 
> > ---
> >
> > Calls into UV BIOS were not being serialised which is wrong as it violates 
> > EFI
> > runtime rules, and bad as it does result in all sorts of potentially hard to
> > track down hangs and panics when efi_scratch.prev_mm gets clobbered whenever
> > efi_switch_mm() gets called concurrently from two different CPUs.
> >
> > Patch #1 removes an unnecessary #ifdef CONFIG_EFI guard from bios_uv.c.
> >
> > Patch #2 removes uv_bios_call_reentrant() because it's dead code.
> >
> > Patch #3 is a cleanup that drops test_bit() in favour of the ad hoc 
> > efi_enabled().
> >
> > Patch #4 makes uv_bios_call() variants use the efi_runtime_lock semaphore to
> > protect against concurrency.
> >
> > Cc: Russ Anderson 
> > Cc: Mike Travis 
> > Cc: Dimitri Sivanich 
> > Cc: Steve Wahl 
> > Cc: sta...@vger.kernel.org # v4.9+
> >
> > Hedi Berriche (4):
> >   x86/platform/UV: remove unnecessary #ifdef CONFIG_EFI
> >   x86/platform/UV: kill uv_bios_call_reentrant() as it has no callers
> >   x86/platform/UV: use efi_enabled() instead of test_bit()
> >   x86/platform/UV: use efi_runtime_lock to serialise BIOS calls
> >
> >  arch/x86/include/asm/uv/bios.h  | 13 -
> >  arch/x86/platform/uv/bios_uv.c  | 35 ++---
> >  drivers/firmware/efi/runtime-wrappers.c |  7 +
> >  3 files changed, 34 insertions(+), 21 deletions(-)
> >
> > --
> > 2.20.1
> >

-- 
Russ Anderson,  SuperDome Flex Linux Kernel Group Manager
HPE - Hewlett Packard Enterprise (formerly SGI)  r...@hpe.com

Re: [PATCH v2] x86/efi: Correct ident mapping of efi old_map when kalsr enabled

2017-05-07 Thread Russ Anderson 
On Sat, May 06, 2017 at 01:36:20AM +0200, Borislav Petkov wrote:
> On Fri, May 05, 2017 at 09:42:14PM +0100, Matt Fleming wrote:
> > (Including the folks from SGI since this was hit on a UV system)
> 
> Wasn't there a BIOS fix supplied at some point which obviated the need
> to boot with efi=old_map on SGI boxes?

Yes, and other fixes to get new and old mapping working (except
for UV1 hardware).  The kaslr patchset broke booting with old
mapping.  That is the issue Baoquan, Bhupesh, and legacy SGI
engineers are trying to fix.


For those that want a more detailed summary:

In early 2014 upstream EFI changed the mapping, which lead to setting 
EFI_OLD_MEMMAP on all UV systems.

  
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a5d90c923bcfb9632d998ed06e9569216ad695f3

Later upstream fixes, plus a bios fix, got new mapping working.
Here are a couple of the fixes.

  
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=08914f436bdd2ed60923f49cbc402307aba20fe4
  
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/x86/platform/uv/bios_uv.c?id=f72075c9eda8a43aeea2f9dbb8d187afd4a76f0b

This patch enabled new EFI mapping on UV2+ platforms (all but UV1).
Note this is not bios version checking, it is hardware platform checking.

  
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/x86/platform/efi/quirks.c?id=d394f2d9d8e1e7b4959819344baf67b5995da9b0

One of the fixes to get new map to work broke old map.  This patch fixed it.

  
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/x86/platform/uv/bios_uv.c?id=caef78b6cdeddf4ad364f95910bba6b43b8eb9bf

So upstream with recent bios works on UV2, UV3, and UV4 hardware platforms,
both old and new mapping, with new mapping being the default.

Thanks.
-- 
Russ Anderson,  Hawks 2 Linux Kernel Group Manager
HPE - Hewlett Packard Enterprise (formerly SGI)  r...@hpe.com  (r...@sgi.com)
--
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: [BUG] Linux 3.14 fails to boot with new EFI changes

2014-01-31 Thread Russ Anderson 
On Fri, Jan 31, 2014 at 08:04:28AM +, Matt Fleming wrote:
 On Thu, 30 Jan, at 04:19:50PM, Alex Thorlton wrote:
  Re-adding lkml.
  
 Also add linux-efi.
 
  The quick answer is I think it is a virtual address, because
  it does not work in physical mode.  If you ever see virtefi
  on the RHEL bootline it is because RH switched the default
  to physical mode, which caused UV to not boot.  virtefi
  forced it back to virtual mode.
 
 Do you have details of the failure, links to bug reports? Is it a
 limitation of the firmware?

That was a non-upstream regression in the distro kernel.  The
3.13 community kernel was boots fine.  The current problem is a
regression introduced in this merge window which needs to be fixed.


-- 
Russ Anderson,  Kernel and Performance Software Team Manager
SGI - Silicon Graphics Inc  r...@sgi.com
--
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: [BUG] Linux 3.14 fails to boot with new EFI changes

2014-01-31 Thread Russ Anderson 
On Fri, Jan 31, 2014 at 11:07:22AM +0100, Borislav Petkov wrote:
 On Thu, Jan 30, 2014 at 02:23:46PM -0800, H. Peter Anvin wrote:
  On 01/30/2014 02:19 PM, Alex Thorlton wrote:
  
   The quick answer is I think it is a virtual address, because it does
   not work in physical mode. If you ever see virtefi on the RHEL
   bootline it is because RH switched the default to physical mode,
   which caused UV to not boot. virtefi forced it back to virtual
   mode.
   
  That is interesting, as it is definitely not the direction we have been
  going in within the Linux community.
 
 Right, for the new scheme to work, we'll have to map the region
 containing the code for uv_systab-function in order to do all those
 uv_bios_call()'s. Physical/virtual shouldn't matter all that much
 because we map the region *both* as a 1:1 map and in virtual space too.
 
 Can SGI please give us a reliable way to do that during boot?

I'm not sure what you are asking for.  We had a reliable way to
boot before the recent patch broke it. (commit
d2f7cbe7b26a74dbbbf8f325b2a6fd01bc34032c)


-- 
Russ Anderson,  Kernel and Performance Software Team Manager
SGI - Silicon Graphics Inc  r...@sgi.com
--
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] Modify UEFI anti-bricking code

2013-06-06 Thread Russ Anderson 
 +  * run, so check the variable info again
 +  */
 + status = efi.query_variable_info(attributes, storage_size,
 +  remaining_size, max_size);
  
 - if (!efi_no_storage_paranoia 
 - ((active_size + size + VAR_METADATA_SIZE  storage_size / 2) 
 -  (remaining_size - size  storage_size / 2)))
 - return EFI_OUT_OF_RESOURCES;
 + if (status != EFI_SUCCESS)
 + return status;
 +
 + /*
 +  * There still isn't enough room, so return an error
 +  */
 + if (remaining_size - size  EFI_MIN_RESERVE)
 + return EFI_OUT_OF_RESOURCES;
 + }
  
   return EFI_SUCCESS;
  }
 -- 
 1.8.1.4
 
 -- 
 Matt Fleming, Intel Open Source Technology Center

-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc  r...@sgi.com
--
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] Modify UEFI anti-bricking code

2013-06-06 Thread Russ Anderson 
On Thu, Jun 06, 2013 at 04:00:39PM +0100, Matt Fleming wrote:
 On Thu, 06 Jun, at 09:48:46AM, Russ Anderson wrote:
  This looks like it will try to allocate more than the remaining size.
  Is that intended?
 
 Yes, the intention is to trigger garbage collection.

OK, if that's what it takes.  It just struck me as something
that would fail initial parameter checking in the bios,
before getting far enough to trigger garbage collection.

Since I really want the rest of this patch to go in, I'm certainly
not going to object.  :-)

Thanks,
-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc  r...@sgi.com
--
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] Modify UEFI anti-bricking code

2013-06-02 Thread Russ Anderson 
   if (boot_params.efi_info.efi_systab_hi ||
 @@ -806,22 +718,6 @@ void __init efi_init(void)
   if (efi_systab_init(efi_phys.systab))
   return;
  
 - pa_data = boot_params.hdr.setup_data;
 - while (pa_data) {
 - data = early_ioremap(pa_data, sizeof(*efi_var_data));
 - if (data-type == SETUP_EFI_VARS) {
 - efi_var_data = (struct efi_var_bootdata *)data;
 -
 - efi_var_store_size = efi_var_data-store_size;
 - efi_var_remaining_size = efi_var_data-remaining_size;
 - efi_var_max_var_size = efi_var_data-max_var_size;
 - }
 - pa_data = data-next;
 - early_iounmap(data, sizeof(*efi_var_data));
 - }
 -
 - boot_used_size = efi_var_store_size - efi_var_remaining_size;
 -
   set_bit(EFI_SYSTEM_TABLES, x86_efi_facility);
  
   /*
 @@ -1141,28 +1037,53 @@ efi_status_t efi_query_variable_store(u32 attributes, 
 unsigned long size)
   if (status != EFI_SUCCESS)
   return status;
  
 - if (!max_size  remaining_size  size)
 - printk_once(KERN_ERR FW_BUG Broken EFI implementation
 -  is returning MaxVariableSize=0\n);
   /*
* Some firmware implementations refuse to boot if there's insufficient
* space in the variable store. We account for that by refusing the
* write if permitting it would reduce the available space to under
 -  * 50%. However, some firmware won't reclaim variable space until
 -  * after the used (not merely the actively used) space drops below
 -  * a threshold. We can approximate that case with the value calculated
 -  * above. If both the firmware and our calculations indicate that the
 -  * available space would drop below 50%, refuse the write.
 +  * 5KB. This figure was provided by Samsung, so should be safe.
*/
 + if ((remaining_size - size  5120)  !efi_no_storage_paranoia) {
 + /*
 +  * Triggering garbage collection may require that the firmware
 +  * generate a real EFI_OUT_OF_RESOURCES error. We can force
 +  * that by attempting to use more space than is available.
 +  */
 + unsigned long dummy_size = remaining_size + 1024;
 + void *dummy = kmalloc(dummy_size, GFP_ATOMIC);
 + efi_char16_t efi_name[6] = { 'D', 'U', 'M', 'M', 'Y', 0 };
 + efi_guid_t guid = EFI_GUID(0x4424ac57, 0xbe4b, 0x47dd, 0x9e,
 +0x97, 0xed, 0x50, 0xf0, 0x9f, 0x92,
 +0xa9);
 +
 + status = efi.set_variable(efi_name, guid, attributes,
 +   dummy_size, dummy);
 +
 + if (status == EFI_SUCCESS) {
 + /*
 +  * This should have failed, so if it didn't make sure
 +  * that we delete it...
 +  */
 + efi.set_variable(efi_name, guid, attributes, 0,
 +  dummy);
 + }
  
 - if (!storage_size || size  remaining_size ||
 - (max_size  size  max_size))
 - return EFI_OUT_OF_RESOURCES;
 + /*
 +  * The runtime code may now have triggered a garbage collection
 +  * run, so check the variable info again
 +  */
 + status = efi.query_variable_info(attributes, storage_size,
 +  remaining_size, max_size);
  
 - if (!efi_no_storage_paranoia 
 - ((active_size + size + VAR_METADATA_SIZE  storage_size / 2) 
 -  (remaining_size - size  storage_size / 2)))
 - return EFI_OUT_OF_RESOURCES;
 + if (status != EFI_SUCCESS)
 + return status;
 +
 + /*
 +  * There still isn't enough room, so return an error
 +  */
 + if (remaining_size - size  5120)
 + return EFI_OUT_OF_RESOURCES;
 + }
  
   return EFI_SUCCESS;
  }
 -- 
 1.8.1.4
 
 --
 To unsubscribe from this list: send the line unsubscribe linux-kernel in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 Please read the FAQ at  http://www.tux.org/lkml/

-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc  r...@sgi.com
--
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: [regression, bisected] x86: efi: Pass boot services variable info to runtime code

2013-05-31 Thread Russ Anderson 
On Fri, May 31, 2013 at 03:48:26PM +0100, Matthew Garrett wrote:
 On Fri, May 31, 2013 at 07:42:37AM -0700, James Bottomley wrote:
  On Fri, 2013-05-31 at 15:34 +0100, Matthew Garrett wrote:
   I agree that a revert is probably the right thing to do here, but the 
   original patch was there to permit a more accurate calculation of the 
   amount of nvram in use, not to provide additional debug information. 
   Reverting it is going to differently break a different set of systems
  
  The only ones that are broken are the Samsung ones.  Samsung claims to
  have fixed their UEFI firmware, so we could refer any problems to them.
 
 No, reverting this gets us back to the old state of refusing any writes 
 if more than 50% of the variable store *appears* to be used, regardless 
 of whether it's actually used. Which, unfortunately, makes it impossible 
 to install Linux on most UEFI machines.

When did writing EFI variables to nvram become necessary to boot
on UEFI?  And if it is necessary, why is it that only linux boot
loaders that use EFI stubs (generally grub2) need it?  The current
kernel boots using EFI/grub and EFI/elilo.  It is just when
EFI stubs are used that the boot fails.

I'm missing the background on why linux needs to write so many
EFI variables to nvram that it fills up nvram.  What is that
all about?

  In any case, Samsung clearly 
 haven't fixed this problem on a pile of machines that have already 
 shipped.

Which means the previous patch(es) that caused the bricking should
get pulled, too.


-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc  r...@sgi.com
--
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: [regression, bisected] x86: efi: Pass boot services variable info to runtime code

2013-05-31 Thread Russ Anderson 
On Fri, May 31, 2013 at 05:28:16PM +0100, Matthew Garrett wrote:
 On Fri, May 31, 2013 at 10:43:49AM -0500, Russ Anderson wrote:
 
  When did writing EFI variables to nvram become necessary to boot on 
  UEFI? And if it is necessary, why is it that only linux boot loaders 
  that use EFI stubs (generally grub2) need it?  The current kernel 
  boots using EFI/grub and EFI/elilo.  It is just when EFI stubs are 
  used that the boot fails.
 
 I think you've misunderstood the problem.

The problem for me is the system not booting.  I was not
familiar with the problem you are trying to solve, but
have now looked at the links.

   If nvram becaomes full, some 
 systems crash during firmware initialisation. So we can't let nvram 
 become full. The obvious thing to do here is to look at the values from 
 QueryVariableInfo, but many systems won't perform any garbage collection 
 until they're almost out of space and so variables that have been 
 deleted still show up as used space.

OK.  I get nvram looks full due to lack of garbage collection
on some systems.  Does QueryVariableInfo (at runtime) tell you
it is full?  Is the problem that it says it is full when it
is not, or does not tell you it is full when it is?

   We can work around that by adding 
 up the size of the variables ourselves, but that only gives us the value 
 for runtime-visible variables. We also need to know how much space is 
 used by variables that are only visible during boot,

Is it valid to assume that only the kernel writes to nvram at
runtime?  Can bios log error information to nvram on an SMM
interrupt (for example)?

  hence calling 
 QueryVariableInfo before ExitBootServices.

Correct me if I am wrong, but that is called from EFI stubs,
which is only used by some bootloaders (sometimes grub2).
Does that mean EFI/grub and EFI/elilo will not hit the problem,
or will not have your fix?


  Which means the previous patch(es) that caused the bricking should
  get pulled, too.
 
 There are no patches that cause the bricking.

I thought that was the problem you were trying to avoid.

-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc  r...@sgi.com
--
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: [regression, bisected] x86: efi: Pass boot services variable info to runtime code

2013-05-31 Thread Russ Anderson 
On Sat, Jun 01, 2013 at 01:03:11AM +0100, Matthew Garrett wrote:
 On Fri, May 31, 2013 at 05:57:31PM -0500, Russ Anderson wrote:
  On Fri, May 31, 2013 at 05:28:16PM +0100, Matthew Garrett wrote:
 If nvram becaomes full, some 
   systems crash during firmware initialisation. So we can't let nvram 
   become full. The obvious thing to do here is to look at the values from 
   QueryVariableInfo, but many systems won't perform any garbage collection 
   until they're almost out of space and so variables that have been 
   deleted still show up as used space.
  
  OK.  I get nvram looks full due to lack of garbage collection
  on some systems.  Does QueryVariableInfo (at runtime) tell you
  it is full?  Is the problem that it says it is full when it
  is not, or does not tell you it is full when it is?
 
 QueryVariableInfo reports the amount used *including garbage*. This 
 makes it useless for determining how much space is available for the 
 firmware to use.

So when QueryVariableInfo reports there is space available, the
write works and everything is fine.

And when QueryVariableInfo reports insufficient space, don't write
and everything is fine.

But when QueryVariableInfo reports insufficient space and you
write anyway, the system can get bricked.  Is that the problem?



According to the UEFI spec for QueryVariableInfo()

  RemainingVariableStorageSize
  Returns the remaining size of the storage
  space available for EFI variables associated
  with the attributes specified.

It also says:

   The returned MaximumVariableStorageSize, RemainingVariableStorageSize,
   MaximumVariableSize information may change immediately after
   the call based on other runtime activities including asynchronous
   error events. Also, these values associated with different
   attributes are not additive in nature.

Note that other runtime activities including asynchronous error
events.  That means runtime services may be using nvram without
the OS knowing about it.


Some bios implementation may be *including garbage*, but can
you definitively say ALL do?  The spec explicitly says the
implementation of variable storage is not defined in this
specification.  You can't assume any form of garbage collection.

   7.2 Variable Services

   Although the implementation of variable storage is not defined
   in this specification, variables must be persistent in most cases.
   This implies that the EFI implementation on a platform must arrange
   it so that variables passed in for storage are retained and
   available for use each time the system boots, at least until they
   are explicitly deleted or overwritten. Provision of this type of
   nonvolatile storage may be very limited on some platforms, so
   variables should be used sparingly in cases where other means of
   communicating information cannot be used.

I don't see anything in here about the OS being free to use
more nvram than QueryVariableInfo() reported as being available.
What happened to following the spec?


 We can work around that by adding 
   up the size of the variables ourselves, but that only gives us the value 
   for runtime-visible variables. We also need to know how much space is 
   used by variables that are only visible during boot,
  
  Is it valid to assume that only the kernel writes to nvram at
  runtime?  Can bios log error information to nvram on an SMM
  interrupt (for example)?
 
 There's a limited number of situations where the firmware can write to 
 nvram, but they're basically uninteresting. Practically speaking, it's 
 always via the kernel once ExitBootServices() has been called.

How can you say they're basically uninteresting???
Do you know what EVERY bios implementation does???


hence calling 
   QueryVariableInfo before ExitBootServices.
  
  Correct me if I am wrong, but that is called from EFI stubs,
  which is only used by some bootloaders (sometimes grub2).
  Does that mean EFI/grub and EFI/elilo will not hit the problem,
  or will not have your fix?
 
 Will not have the fix. Boot EFI systems via the EFI stub.

Thanks for that clarification.

 Boot EFI systems via the EFI stub.

Fortunately all our shipping systems are EFI/grub and EFI/elilo
right now, so they boot.

-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc  r...@sgi.com
--
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: [regression, bisected] x86: efi: Pass boot services variable info to runtime code

2013-05-30 Thread Russ Anderson 
On Thu, May 30, 2013 at 10:16:12AM +0800, joeyli wrote:
 於 四，2013-05-30 於 00:53 +0200，Jiri Kosina 提到：
  On Wed, 29 May 2013, Russ Anderson wrote:
  
Yes, but this call is clearly happening way before ExitBootServices() 
-- 
see the surrounding code, see for example this in efi_main():

[ ... snip ... ]
setup_efi_vars(boot_params);

setup_efi_pci(boot_params);

status = efi_call_phys3(sys_table-boottime-allocate_pool,
EFI_LOADER_DATA, sizeof(*gdt),
(void **)gdt);
if (status != EFI_SUCCESS) {
efi_printk(Failed to alloc mem for gdt structure\n);
goto fail;
}
[ ... snip ... ]
   
   Yes.  Note the failing call is sys_table-runtime while all the
   other calls are sys_table-boottime and seem to work.  Not sure
   why the sys_table-runtime call has a problem but it may be
   a clue.  Could something in the runtime path not be set up???
  
  That was my original idea early today as well. My understanding of the 
  UEFI spec is admittedly limited, but afaics calling runtime method from 
  boot environment should be a valid thing to do ... ?
 
 QueryVariableInfo() is a runtime services, all runtime services should
 available bother on boot time and runtime:
 
 UEFI spec 2.3.1 P.109:
   Runtime Services
   Functions that are available before and after any call to  
   ExitBootServices(). These functions are described in Section 7.

That's a great idea.  This patch moves the QueryVariableInfo()
call from bootime to runtime, in efi_late_init().  The attached
patch is consistent with the UEFI spec and avoids the problem.

Thanks,
-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc  r...@sgi.com
Move query_variable_info call to runtime to avoid bios issues.

Signed-off-by: Russ Anderson r...@sgi.com

---
 arch/x86/boot/compressed/eboot.c |   49 ---
 arch/x86/platform/efi/efi.c  |   35 ---
 2 files changed, 16 insertions(+), 68 deletions(-)

Index: linux/arch/x86/boot/compressed/eboot.c
===
--- linux.orig/arch/x86/boot/compressed/eboot.c	2013-05-30 11:02:19.034914824 -0500
+++ linux/arch/x86/boot/compressed/eboot.c	2013-05-30 16:53:50.512636568 -0500
@@ -251,53 +251,6 @@ static void find_bits(unsigned long mask
 	*size = len;
 }
 
-static efi_status_t setup_efi_vars(struct boot_params *params)
-{
-	struct setup_data *data;
-	struct efi_var_bootdata *efidata;
-	u64 store_size, remaining_size, var_size;
-	efi_status_t status;
-
-	if (sys_table-runtime-hdr.revision  EFI_2_00_SYSTEM_TABLE_REVISION)
-		return EFI_UNSUPPORTED;
-
-	data = (struct setup_data *)(unsigned long)params-hdr.setup_data;
-
-	while (data  data-next)
-		data = (struct setup_data *)(unsigned long)data-next;
-
-	status = efi_call_phys4((void *)sys_table-runtime-query_variable_info,
-EFI_VARIABLE_NON_VOLATILE |
-EFI_VARIABLE_BOOTSERVICE_ACCESS |
-EFI_VARIABLE_RUNTIME_ACCESS, store_size,
-remaining_size, var_size);
-
-	if (status != EFI_SUCCESS) { // RJA
-		efi_printk(RJA: setup_efi_vars FAILED\n);
-		return status;
-	}
-
-	status = efi_call_phys3(sys_table-boottime-allocate_pool,
-EFI_LOADER_DATA, sizeof(*efidata), efidata);
-
-	if (status != EFI_SUCCESS)
-		return status;
-
-	efidata-data.type = SETUP_EFI_VARS;
-	efidata-data.len = sizeof(struct efi_var_bootdata) -
-		sizeof(struct setup_data);
-	efidata-data.next = 0;
-	efidata-store_size = store_size;
-	efidata-remaining_size = remaining_size;
-	efidata-max_var_size = var_size;
-
-	if (data)
-		data-next = (unsigned long)efidata;
-	else
-		params-hdr.setup_data = (unsigned long)efidata;
-
-}
-
 static efi_status_t setup_efi_pci(struct boot_params *params)
 {
 	efi_pci_io_protocol *pci;
@@ -1204,8 +1157,6 @@ struct boot_params *efi_main(void *handl
 
 	setup_graphics(boot_params);
 
-	setup_efi_vars(boot_params);
-
 	setup_efi_pci(boot_params);
 
 	status = efi_call_phys3(sys_table-boottime-allocate_pool,
Index: linux/arch/x86/platform/efi/efi.c
===
--- linux.orig/arch/x86/platform/efi/efi.c	2013-05-30 11:02:19.034914824 -0500
+++ linux/arch/x86/platform/efi/efi.c	2013-05-30 17:05:38.140039879 -0500
@@ -786,9 +786,6 @@ void __init efi_init(void)
 	char vendor[100] = unknown;
 	int i = 0;
 	void *tmp;
-	struct setup_data *data;
-	struct efi_var_bootdata *efi_var_data;
-	u64 pa_data;
 
 #ifdef CONFIG_X86_32
 	if (boot_params.efi_info.efi_systab_hi ||
@@ -806,22 +803,6 @@ void __init efi_init(void)
 	if (efi_systab_init(efi_phys.systab))
 		return;
 
-	pa_data = boot_params.hdr.setup_data;
-	while (pa_data) {
-		data = early_ioremap(pa_data, sizeof(*efi_var_data));
-		if (data-type == SETUP_EFI_VARS) {
-			efi_var_data = (struct efi_var_bootdata *)data

Re: [regression, bisected] x86: efi: Pass boot services variable info to runtime code

2013-05-30 Thread Russ Anderson 
On Thu, May 30, 2013 at 10:21:53PM +, Matthew Garrett wrote:
 On Thu, 2013-05-30 at 17:17 -0500, Russ Anderson wrote:
 
  That's a great idea.  This patch moves the QueryVariableInfo()
  call from bootime to runtime, in efi_late_init().  The attached
  patch is consistent with the UEFI spec and avoids the problem.
 
 No, that defeats the entire point of the original patch.

How so?  It is still calling QueryVariableInfo()
before the data is used.

-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc  r...@sgi.com
--
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: [regression, bisected] x86: efi: Pass boot services variable info to runtime code

2013-05-30 Thread Russ Anderson 
On Fri, May 31, 2013 at 12:30:43AM +0200, Jiri Kosina wrote:
 On Thu, 30 May 2013, Russ Anderson wrote:
 
That's a great idea.  This patch moves the QueryVariableInfo()
call from bootime to runtime, in efi_late_init().  The attached
patch is consistent with the UEFI spec and avoids the problem.
   
   No, that defeats the entire point of the original patch.
  
  How so?  It is still calling QueryVariableInfo()
  before the data is used.
 
 You lose information provided by QueryVariableInfo() about boot-only 
 variables once the transition boot - runtime has happened.

Is that information really more important than the ability
to boot?

Correct me if I'm wrong, but linux was able to boot without
the boottime QueryVariableInfo() call up until 3.9-rc7,
and it still does on systems that do not use EFI stubs (ie
grub and elilo).  It is only when linux uses EFI stubs (ie
grub2) that linux makes the boottime QueryVariableInfo()
call.  So why is that call, or whatever is dependent on it,
more important than booting?



Thanks,
-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc  r...@sgi.com
--
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: [regression, bisected] x86: efi: Pass boot services variable info to runtime code

2013-05-30 Thread Russ Anderson 
On Thu, May 30, 2013 at 10:32:09PM +, Matthew Garrett wrote:
 On Thu, 2013-05-30 at 17:28 -0500, Russ Anderson wrote:
  On Thu, May 30, 2013 at 10:21:53PM +, Matthew Garrett wrote:
   On Thu, 2013-05-30 at 17:17 -0500, Russ Anderson wrote:
   
That's a great idea.  This patch moves the QueryVariableInfo()
call from bootime to runtime, in efi_late_init().  The attached
patch is consistent with the UEFI spec and avoids the problem.
   
   No, that defeats the entire point of the original patch.
  
  How so?  It is still calling QueryVariableInfo()
  before the data is used.
 
 We want to know how much space is used by variables that aren't visible
 at runtime.

We want to boot.  We could boot up through 3.9-rc7.

Knowing how much space is used by variables that aren't
visible at runtime it moot if you can't boot.


And again, maybe this is a bios bug - we have bios people
looking into it - and maybe that call _should_ work, but
the fact is the kernel booted without that change[1] and does
not boot with it.  


[1] commit cc5a080c5d40c36089bb08a8a16fa3fc7047fe0f

Thanks,
-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc  r...@sgi.com
--
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: [regression, bisected] x86: efi: Pass boot services variable info to runtime code

2013-05-24 Thread Russ Anderson 
On Fri, May 24, 2013 at 08:43:31AM +0100, Matt Fleming wrote:
 On Thu, 23 May, at 03:32:34PM, Russ Anderson wrote:
 efi: mem127: type=4, attr=0xf, 
  range=[0x6bb22000-0x7ca9c000) (271MB)
 
 EFI_BOOT_SERVICES_CODE
 
 efi: mem133: type=5, attr=0x800f, 
  range=[0x7daff000-0x7dbff000) (1MB)
 
 EFI_RUNTIME_SERVICES_CODE
 
 EFI Variables Facility v0.08 2004-May-17
 BUG: unable to handle kernel paging request at 7ca95b10
 IP: [88007dbf2140] 0x88007dbf213f
 
  [810499b3] ?  efi_call3+0x43/0x80
  [810492a7] ?  virt_efi_get_next_variable+0x47/0x1c0
  [814c8cc0] ?  create_efivars_bin_attributes+0x150/0x150
  [814c7b55] ?  efivar_init+0xd5/0x390
  [814c8ae0] ?  efivar_update_sysfs_entries+0x90/0x90
  [812f906b] ?  kobject_uevent+0xb/0x10
  [812f812b] ?  kset_register+0x5b/0x70
  [814c8cc0] ?  create_efivars_bin_attributes+0x150/0x150
  [814c8d47] ?  efivars_sysfs_init+0x87/0xf0
  [8100032a] ?  do_one_initcall+0x15a/0x1b0
  [81a17831] ?  do_basic_setup+0xad/0xce
  [81a17ae3] ?  kernel_init_freeable+0x291/0x291
  [81a3708a] ?  sched_init_smp+0x15b/0x162
  [81a17a5f] ?  kernel_init_freeable+0x20d/0x291
  [81601eb0] ?  rest_init+0x80/0x80
  [81601ebe] ?  kernel_init+0xe/0x180
  [8162179c] ?  ret_from_fork+0x7c/0xb0
  [81601eb0] ?  rest_init+0x80/0x80
 
 Here's the real call stack leading up to the crash.
 
 What appears to be happening is that your the EFI runtime services code
 is calling into the EFI boot services code, which is definitely a bug in
 your firmware because we're at runtime, but we've seen other machines
 that do similar things so we usually handle it just fine. However, what
 makes your case different, and the reason you see the above splat, is
 that it's using the physical address of the EFI boot services region,
 not the virtual one we setup with SetVirtualAddressMap(). Which is a
 second firmware bug. Again, we have seen other machines that access
 physical addresses after SetVirtualAddressMap(), but until now we
 haven't had any non-optional code that triggered them.
 
 The only reason I can see that the offending commit would introduce this
 problem is because it calls QueryVariableInfo() at boot time. I notice
 that your machine is an SGI UV one, is there any chance you could get a
 firmware fix for this? If possible, it would be also good to confirm
 that it's this chunk of code in setup_efi_vars(),
 
   status = efi_call_phys4(sys_table-runtime-query_variable_info,
   EFI_VARIABLE_NON_VOLATILE |
   EFI_VARIABLE_BOOTSERVICE_ACCESS |
   EFI_VARIABLE_RUNTIME_ACCESS, store_size,
   remaining_size, var_size);

This call is failing, but not returning a valid EFI_* return status.
setup_efi_vars() returns at that point.  Maybe it is not set up
to do GetNextVariable() later on???  Why call GetNextVariable() if the
earlier call failed?

 that later makes GetNextVariable() jump to the physical address of the
 EFI Boot Services region. Because if not, we need to do some more
 digging.

One other data point is if the query_variable_info call is hacked to
remove one of the EFI flags (ie comment out EFI_VARIABLE_BOOTSERVICE_ACCESS)
the efi_call_phys4() call fails with EFI_INVALID_PARAMETER and
the system boots.  Of course it does not create /sys/firmware/efivars
entries and complains [Firmware Bug]: efi: Inconsistent initial sizes.
But at least it boots.

One of the BIOS guys will build a debug bios next week to help see
what is going on in the query_variable_info() call.

 Borislav, how are your 1:1 mapping patches coming along? In theory, once
 those are merged we can gracefully workaround these kinds of issues.
 
 -- 
 Matt Fleming, Intel Open Source Technology Center

-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc  r...@sgi.com
--
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: [regression, bisected] x86: efi: Pass boot services variable info to runtime code

2013-05-24 Thread Russ Anderson 
On Fri, May 24, 2013 at 08:11:01PM +, Matthew Garrett wrote:
 On Fri, 2013-05-24 at 15:05 -0500, Russ Anderson wrote:
 
  One other data point is if the query_variable_info call is hacked to
  remove one of the EFI flags (ie comment out EFI_VARIABLE_BOOTSERVICE_ACCESS)
  the efi_call_phys4() call fails with EFI_INVALID_PARAMETER and
  the system boots.  Of course it does not create /sys/firmware/efivars
  entries and complains [Firmware Bug]: efi: Inconsistent initial sizes.
  But at least it boots.
 
 EFI_VARIABLE_RUNTIME_ACCESS is only legal if
 EFI_VARIABLE_BOOTSERVICE_ACCESS is set, so it's correct to throw
 EFI_INVALID_PARAMETER there.

Yes.  The point of the experiment was to see if it returned a
valid failure status (which it did) and see if the boot still
failed (which it didn't).  So something about going deeper
into that call seems to trigger the failure.

Why does the kernel still try to create /sys/firmware/efivars/
entries in the original failure even though efi_call_phys4()
failed?  Or does it always try to create those entries
and GetNextVariable() blows up in the original failure
but not in my experiment?

Thanks,
-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc  r...@sgi.com
--
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: [regression, bisected] x86: efi: Pass boot services variable info to runtime code

2013-05-23 Thread Russ Anderson 
On Thu, May 23, 2013 at 12:58:01PM +0100, Matt Fleming wrote:
 On Wed, 22 May, at 11:27:47AM, Russ Anderson wrote:
  [6.062157] EFI Variables Facility v0.08 2004-May-17
  [6.067731] BUG: unable to handle kernel paging request at 
  7ca95b10
  [6.075519] IP: [88007dbf2140] 0x88007dbf213f
 
 This is a bit of a head scratcher. Could you paste the EFI memmap
 entries in your dmesg for the regions that cover 0x7ca95b10 and
 0x7dbf2140?  My guess would be that they're EFI runtime code regions,
 which would at least explain why we seem to be executing code in the
 direct mapping region (0x8800).
 
 Are you booting via the EFI boot stub?

Interesting data point.  The failure is on a rhel7/grub2 root.
The identical kernel on a rhel6/grub root boots.  So maybe
grub2 brings out the failure?  I suspect Fedora19/grub2 on
EFI should hit the problem (for someone looking to reproduce
it).

In both cases the kernel boot line options are the same.

-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc  r...@sgi.com
--
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