Re: [PATCH] tpm: Require that all digests are present in TCG_PCR_EVENT2 structures

[Peter Jones]

On Tue, Jun 30, 2020 at 02:23:22PM -0500, Tyler Hicks wrote:
> > > I am all for stringent checks, but this could potentially break
> > > measured boot on systems that are working fine today, right?
> > 
> > Seems like in that case our measurement is unreliable and can't really
> > be trusted.  That said, having things that were using the measurements
> > before this suddenly stop being able to access sealed secrets is not a
> > great experience for the user who unwittingly bought the junk hardware.
> 
> I haven't seen where anyone has suggested that such junk hardware
> exists. Do you know of or expect any firmware that has mismatched
> TCG_PCR_EVENT2.digests.count and TCG_EfiSpecIdEvent.numberOfAlgorithms
> values?

If nobody has seen any hardware that actually produces the values you're
excluding, then I don't have a strong objection.
 
> I would think that the userspace code that's parsing
> /sys/kernel/security/tpm0/binary_bios_measurements would also have
> issues with such an event log.
> 
> > Same with the zero-supported-hashes case.
> 
> Small but important correction: it is a zero-hashes case, not a
> zero-supported-hashes case
> 
> There's no handshake involved or anything like that. This would only
> cause problems if the firmware provided no hashes, which means the
> firmware event log is unusable, anyways.

Indeed.

-- 
Peter

Re: [PATCH] tpm: Require that all digests are present in TCG_PCR_EVENT2 structures

[Peter Jones]

On Tue, Jun 16, 2020 at 11:08:38AM +0200, Ard Biesheuvel wrote:
> (cc Matthew and Peter)
> 
> On Tue, 16 Jun 2020 at 01:28, Tyler Hicks  wrote:
> >
> > Require that the TCG_PCR_EVENT2.digests.count value strictly matches the
> > value of TCG_EfiSpecIdEvent.numberOfAlgorithms in the event field of the
> > TCG_PCClientPCREvent event log header. Also require that
> > TCG_EfiSpecIdEvent.numberOfAlgorithms is non-zero.
> >
> > The TCG PC Client Platform Firmware Profile Specification section 9.1
> > (Family "2.0", Level 00 Revision 1.04) states:
> >
> >  For each Hash algorithm enumerated in the TCG_PCClientPCREvent entry,
> >  there SHALL be a corresponding digest in all TCG_PCR_EVENT2 structures.
> >  Note: This includes EV_NO_ACTION events which do not extend the PCR.
> >
> > Section 9.4.5.1 provides this description of
> > TCG_EfiSpecIdEvent.numberOfAlgorithms:
> >
> >  The number of Hash algorithms in the digestSizes field. This field MUST
> >  be set to a value of 0x01 or greater.
> >
> > Enforce these restrictions, as required by the above specification, in
> > order to better identify and ignore invalid sequences of bytes at the
> > end of an otherwise valid TPM2 event log. Firmware doesn't always have
> > the means necessary to inform the kernel of the actual event log size so
> > the kernel's event log parsing code should be stringent when parsing the
> > event log for resiliency against firmware bugs. This is true, for
> > example, when firmware passes the event log to the kernel via a reserved
> > memory region described in device tree.
> >
> 
> When does this happen? Do we have code in mainline that does this?
> 
> > Prior to this patch, a single bit set in the offset corresponding to
> > either the TCG_PCR_EVENT2.eventType or TCG_PCR_EVENT2.eventSize fields,
> > after the last valid event log entry, could confuse the parser into
> > thinking that an additional entry is present in the event log. This
> > patch raises the bar on how difficult it is for stale memory to confuse
> > the kernel's event log parser but there's still a reliance on firmware
> > to properly initialize the remainder of the memory region reserved for
> > the event log as the parser cannot be expected to detect a stale but
> > otherwise properly formatted firmware event log entry.
> >
> > Fixes: fd5c78694f3f ("tpm: fix handling of the TPM 2.0 event logs")
> > Signed-off-by: Tyler Hicks 
> > ---
> 
> I am all for stringent checks, but this could potentially break
> measured boot on systems that are working fine today, right?

Seems like in that case our measurement is unreliable and can't really
be trusted.  That said, having things that were using the measurements
before this suddenly stop being able to access sealed secrets is not a
great experience for the user who unwittingly bought the junk hardware.
Same with the zero-supported-hashes case.  It would be nice to at log it
and have a way for them to opt-in to allowing the old measurement to go
through, so they can recover their data, though I don't know what that
method would be if the measured command line is one of their
dependencies.

-- 
Peter

[tip: efi/urgent] efi: Make it possible to disable efivar_ssdt entirely

[tip-bot2 for Peter Jones]

The following commit has been merged into the efi/urgent branch of tip:

Commit-ID: 435d1a471598752446a72ad1201b3c980526d869
Gitweb:
https://git.kernel.org/tip/435d1a471598752446a72ad1201b3c980526d869
Author:Peter Jones 
AuthorDate:Mon, 15 Jun 2020 16:24:08 -04:00
Committer: Ard Biesheuvel 
CommitterDate: Tue, 16 Jun 2020 11:01:07 +02:00

efi: Make it possible to disable efivar_ssdt entirely

In most cases, such as CONFIG_ACPI_CUSTOM_DSDT and
CONFIG_ACPI_TABLE_UPGRADE, boot-time modifications to firmware tables
are tied to specific Kconfig options.  Currently this is not the case
for modifying the ACPI SSDT via the efivar_ssdt kernel command line
option and associated EFI variable.

This patch adds CONFIG_EFI_CUSTOM_SSDT_OVERLAYS, which defaults
disabled, in order to allow enabling or disabling that feature during
the build.

Cc: 
Signed-off-by: Peter Jones 
Link: https://lore.kernel.org/r/20200615202408.2242614-1-pjo...@redhat.com
Signed-off-by: Ard Biesheuvel 
---
 drivers/firmware/efi/Kconfig | 11 +++
 drivers/firmware/efi/efi.c   |  2 +-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index e6fc022..3939699 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -278,3 +278,14 @@ config EFI_EARLYCON
depends on SERIAL_EARLYCON && !ARM && !IA64
select FONT_SUPPORT
select ARCH_USE_MEMREMAP_PROT
+
+config EFI_CUSTOM_SSDT_OVERLAYS
+   bool "Load custom ACPI SSDT overlay from an EFI variable"
+   depends on EFI_VARS && ACPI
+   default ACPI_TABLE_UPGRADE
+   help
+ Allow loading of an ACPI SSDT overlay from an EFI variable specified
+ by a kernel command line option.
+
+ See Documentation/admin-guide/acpi/ssdt-overlays.rst for more
+ information.
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index edc5d36..5114cae 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -189,7 +189,7 @@ static void generic_ops_unregister(void)
efivars_unregister(_efivars);
 }
 
-#if IS_ENABLED(CONFIG_ACPI)
+#ifdef CONFIG_EFI_CUSTOM_SSDT_OVERLAYS
 #define EFIVAR_SSDT_NAME_MAX   16
 static char efivar_ssdt[EFIVAR_SSDT_NAME_MAX] __initdata;
 static int __init efivar_ssdt_setup(char *str)

[PATCH] Make it possible to disable efivar_ssdt entirely

[Peter Jones]

In most cases, such as CONFIG_ACPI_CUSTOM_DSDT and
CONFIG_ACPI_TABLE_UPGRADE, boot-time modifications to firmware tables
are tied to specific Kconfig options.  Currently this is not the case
for modifying the ACPI SSDT via the efivar_ssdt kernel command line
option and associated EFI variable.

This patch adds CONFIG_EFI_CUSTOM_SSDT_OVERLAYS, which defaults
disabled, in order to allow enabling or disabling that feature during
the build.

Signed-off-by: Peter Jones 
---
 drivers/firmware/efi/efi.c   |  2 +-
 drivers/firmware/efi/Kconfig | 11 +++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 48d0188936c..4b12a598ccf 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -192,7 +192,7 @@ static void generic_ops_unregister(void)
efivars_unregister(_efivars);
 }
 
-#if IS_ENABLED(CONFIG_ACPI)
+#if IS_ENABLED(CONFIG_EFI_CUSTOM_SSDT_OVERLAYS)
 #define EFIVAR_SSDT_NAME_MAX   16
 static char efivar_ssdt[EFIVAR_SSDT_NAME_MAX] __initdata;
 static int __init efivar_ssdt_setup(char *str)
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 6b38f9e5d20..fe433f76b03 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -278,3 +278,14 @@ config EFI_EARLYCON
depends on SERIAL_EARLYCON && !ARM && !IA64
select FONT_SUPPORT
select ARCH_USE_MEMREMAP_PROT
+
+config EFI_CUSTOM_SSDT_OVERLAYS
+   bool "Load custom ACPI SSDT overlay from an EFI variable"
+   depends on EFI_VARS
+   default ACPI_TABLE_UPGRADE
+   help
+ Allow loading of an ACPI SSDT overlay from an EFI variable specified
+ by a kernel command line option.
+
+ See Documentation/admin-guide/acpi/ssdt-overlays.rst for more
+ information.
-- 
2.26.2

Re: [PATCH] efi/fb: Convert PCI bus address to resource if translated by the bridge

[Peter Jones]

On Thu, May 17, 2018 at 09:22:23AM -0400, Sinan Kaya wrote:
> A host bridge is allowed to remap BAR addresses using _TRA attribute in
> _CRS windows.
> 
> pci_bus :00: root bus resource [mem 0x8010010-0x8011fff window] 
> (bus address [0x0010-0x1fff])
> pci :02:00.0: reg 0x10: [mem 0x8011e00-0x8011eff]
> 
> When a VGA device is behind such a host bridge and the resource is
> translated efifb driver is trying to do ioremap against bus address
> rather than the resource address and is failing to probe.
> 
> efifb driver is having difficulty locating the base address from BAR
> address when
> 
> efifb: probing for efifb
> efifb: cannot reserve video memory at 0x1e00
> efifb: framebuffer at 0x1e00, using 1920k, total 1875k
> efifb: mode is 800x600x32, linelength=3200, pages=1
> efifb: scrolling: redraw
> efifb: Truecolor: size=8:8:8:8, shift=24:16:8:0
> 
> Use the host bridge offset information to convert bus address to
> resource address in the fixup.
> 
> Signed-off-by: Sinan Kaya <ok...@codeaurora.org>

Looks reasonable to me - Ard, do you want to take this up through the
EFI tree?

Signed-off-by: Peter Jones <pjo...@redhat.com>

> ---
>  drivers/video/fbdev/efifb.c | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index 46a4484..ea68d5c 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -428,6 +428,8 @@ static void efifb_fixup_resources(struct pci_dev *dev)
>  {
>   u64 base = screen_info.lfb_base;
>   u64 size = screen_info.lfb_size;
> + struct pci_bus_region region;
> + struct resource res;
>   int i;
>  
>   if (efifb_pci_dev || screen_info.orig_video_isVGA != VIDEO_TYPE_EFI)
> @@ -439,6 +441,14 @@ static void efifb_fixup_resources(struct pci_dev *dev)
>   if (!base)
>   return;
>  
> + region.start = base;
> + region.end = base + size - 1;
> + res.start = 0;
> + res.flags = IORESOURCE_MEM;
> + pcibios_bus_to_resource(dev->bus, , );
> + if (res.start)
> + base = res.start;
> +
>   for (i = 0; i <= PCI_STD_RESOURCE_END; i++) {
>   struct resource *res = >resource[i];
>  
> -- 
> 2.7.4
> 

-- 
  Peter

Re: [PATCH] efi/fb: Convert PCI bus address to resource if translated by the bridge

[Peter Jones]

On Thu, May 17, 2018 at 09:22:23AM -0400, Sinan Kaya wrote:
> A host bridge is allowed to remap BAR addresses using _TRA attribute in
> _CRS windows.
> 
> pci_bus :00: root bus resource [mem 0x8010010-0x8011fff window] 
> (bus address [0x0010-0x1fff])
> pci :02:00.0: reg 0x10: [mem 0x8011e00-0x8011eff]
> 
> When a VGA device is behind such a host bridge and the resource is
> translated efifb driver is trying to do ioremap against bus address
> rather than the resource address and is failing to probe.
> 
> efifb driver is having difficulty locating the base address from BAR
> address when
> 
> efifb: probing for efifb
> efifb: cannot reserve video memory at 0x1e00
> efifb: framebuffer at 0x1e00, using 1920k, total 1875k
> efifb: mode is 800x600x32, linelength=3200, pages=1
> efifb: scrolling: redraw
> efifb: Truecolor: size=8:8:8:8, shift=24:16:8:0
> 
> Use the host bridge offset information to convert bus address to
> resource address in the fixup.
> 
> Signed-off-by: Sinan Kaya 

Looks reasonable to me - Ard, do you want to take this up through the
EFI tree?

Signed-off-by: Peter Jones 

> ---
>  drivers/video/fbdev/efifb.c | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index 46a4484..ea68d5c 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -428,6 +428,8 @@ static void efifb_fixup_resources(struct pci_dev *dev)
>  {
>   u64 base = screen_info.lfb_base;
>   u64 size = screen_info.lfb_size;
> + struct pci_bus_region region;
> + struct resource res;
>   int i;
>  
>   if (efifb_pci_dev || screen_info.orig_video_isVGA != VIDEO_TYPE_EFI)
> @@ -439,6 +441,14 @@ static void efifb_fixup_resources(struct pci_dev *dev)
>   if (!base)
>   return;
>  
> + region.start = base;
> + region.end = base + size - 1;
> + res.start = 0;
> + res.flags = IORESOURCE_MEM;
> + pcibios_bus_to_resource(dev->bus, , );
> + if (res.start)
> + base = res.start;
> +
>   for (i = 0; i <= PCI_STD_RESOURCE_END; i++) {
>   struct resource *res = >resource[i];
>  
> -- 
> 2.7.4
> 

-- 
  Peter

Re: [PATCH 2/2] efi: Add embedded peripheral firmware support

[Peter Jones]

On Thu, Apr 05, 2018 at 07:43:49AM +0200, Lukas Wunner wrote:
> On Wed, Apr 04, 2018 at 01:18:36PM -0400, Peter Jones wrote:
> > > On Tue, Apr 03, 2018 at 08:07:11PM +0200, Lukas Wunner wrote:
> > > > * Add the EFI Firmware Volume Protocol to include/linux/efi.h:
> > > >   
> > > > https://www.intel.com/content/dam/doc/reference-guide/efi-firmware-file-volume-specification.pdf
> > > > 
> > > > * Amend arch/x86/boot/compressed/eboot.c to read the files with the
> > > >   GUIDs you're interested in into memory and pass the files to the
> > > >   kernel as setup_data payloads.
> > 
> > To be honest, I'm a bit skeptical about the firmware volume approach.
> > Tools like UEFITool[0] and uefi-firmware-parser[1] have existed for
> > years, still don't seem to reliably parse firmware images I see in the
> > wild, and have a fairly regular need for fixes.  These are tools
> > maintained by smart people who are making a real effort, and it still
> > looks pretty hard to do a good job that applies across a lot of
> > platforms.
> > 
> > So I'd rather use Hans's existing patches, at least for now, and if
> > someone is interested in hacking on making an efi firmware volume parser
> > for the kernel, switch them to that when such a thing is ready.
> 
> Hello?  As I've written in the above-quoted e-mail the kernel should
> read the files using EFI_FIRMWARE_VOLUME_PROTOCOL.ReadFile().
> 
> *Not* by parsing the firmware volume!

Okay, that's a fair point that I'd missed reading your first mail.  It's
worth a shot.  That said, EFI_FIRMWARE_VOLUME_PROTOCOL is part of the
PI spec, not the UEFI spec.  It's not required in order to implement UEFI,
and some implementations don't use it.  Even when the implementation
does include PI, there's no guarantee PI protocols are exposed after the
"End of DXE" event (though edk2 will expose this, I think).

> Parsing the firmware volume is only necessary to find out the GUIDs
> of the files you're looking for.  You only do that *once*.
> 
> I habitually extract Apple's EFI Firmware Volumes and found the
> existing tools always did a sufficiently good job to get the
> information I need (such as UEFIExtract, which I've linked to,
> and which is part of the UEFITool suite, which you've linked to
> once more).

Yeah, it's probably worth implementing your way and using it when we
can.

> On Wed, Apr 04, 2018 at 10:25:05PM +0200, Hans de Goede wrote:
> > Lukas, thank you for your suggestions on this, but I doubt that these
> > devices use the Firmware Volume stuff.
> 
> If they're using EFI, they're using a Firmware Volume and you should
> be able to use the Firmware Volume Protocol to read files from it.

This is not actually the case.  There is more than one implementation of
UEFI, and they do not all implement PEI/DXE/etc from the PI spec.  For
two examples, there are still vendors that ship EDK-I implementations on
some platforms, and current u-boot can be built to export a UEFI API.
(It's a subset, but so is every other implementation.)

> If that protocol wasn't supported, the existing EFI drivers wouldn't
> be able to function as they couldn't load files from the volume.

This is not correct.  Not all UEFI implementations implement *any* of
PI.  Also, AFAIK, nothing we use in Linux so far directly depends on
anything in PI.

> > What you are describing sounds like significantly more work then
> > the vendor just embedding the firmware as a char firmware[] in their
> > EFI mouse driver.
> 
> In such cases, read the EFI mouse driver from the firmware volume and
> extract the firmware from the offset you've pre-determined.  That's
> never going to change since the devices never receive updates, as
> you're saying.  So basically you'll have a struct with GUID, offset,
> length, and the name under which the firmware is made available to
> Linux drivers.

No, he's saying that it's really easy to implement a driver with the
device firmware in a char [] in the code, so cheap ODMs who supply a
UEFI driver with their hardware part are very, very likely to have done
that instead of providing two things to go into the FV - a UEFI driver
*and* a separate image for the device firmware.  This also cuts out a
point of failure between the OEM and the ODM.

> > That combined with Peter's worries about difficulties parsing the
> > Firmware Volume stuff, makes me believe that it is best to just
> > stick with my current approach as Peter suggests.
> 
> I don't know why you insist on a hackish solution (your own words)
> even though a cleaner solution is suggested, but fortunately it's
> not for me to decide what goes in and what doesn't. ;-)

It already works...

-- 
  Peter

Re: [PATCH 2/2] efi: Add embedded peripheral firmware support

[Peter Jones]

On Thu, Apr 05, 2018 at 07:43:49AM +0200, Lukas Wunner wrote:
> On Wed, Apr 04, 2018 at 01:18:36PM -0400, Peter Jones wrote:
> > > On Tue, Apr 03, 2018 at 08:07:11PM +0200, Lukas Wunner wrote:
> > > > * Add the EFI Firmware Volume Protocol to include/linux/efi.h:
> > > >   
> > > > https://www.intel.com/content/dam/doc/reference-guide/efi-firmware-file-volume-specification.pdf
> > > > 
> > > > * Amend arch/x86/boot/compressed/eboot.c to read the files with the
> > > >   GUIDs you're interested in into memory and pass the files to the
> > > >   kernel as setup_data payloads.
> > 
> > To be honest, I'm a bit skeptical about the firmware volume approach.
> > Tools like UEFITool[0] and uefi-firmware-parser[1] have existed for
> > years, still don't seem to reliably parse firmware images I see in the
> > wild, and have a fairly regular need for fixes.  These are tools
> > maintained by smart people who are making a real effort, and it still
> > looks pretty hard to do a good job that applies across a lot of
> > platforms.
> > 
> > So I'd rather use Hans's existing patches, at least for now, and if
> > someone is interested in hacking on making an efi firmware volume parser
> > for the kernel, switch them to that when such a thing is ready.
> 
> Hello?  As I've written in the above-quoted e-mail the kernel should
> read the files using EFI_FIRMWARE_VOLUME_PROTOCOL.ReadFile().
> 
> *Not* by parsing the firmware volume!

Okay, that's a fair point that I'd missed reading your first mail.  It's
worth a shot.  That said, EFI_FIRMWARE_VOLUME_PROTOCOL is part of the
PI spec, not the UEFI spec.  It's not required in order to implement UEFI,
and some implementations don't use it.  Even when the implementation
does include PI, there's no guarantee PI protocols are exposed after the
"End of DXE" event (though edk2 will expose this, I think).

> Parsing the firmware volume is only necessary to find out the GUIDs
> of the files you're looking for.  You only do that *once*.
> 
> I habitually extract Apple's EFI Firmware Volumes and found the
> existing tools always did a sufficiently good job to get the
> information I need (such as UEFIExtract, which I've linked to,
> and which is part of the UEFITool suite, which you've linked to
> once more).

Yeah, it's probably worth implementing your way and using it when we
can.

> On Wed, Apr 04, 2018 at 10:25:05PM +0200, Hans de Goede wrote:
> > Lukas, thank you for your suggestions on this, but I doubt that these
> > devices use the Firmware Volume stuff.
> 
> If they're using EFI, they're using a Firmware Volume and you should
> be able to use the Firmware Volume Protocol to read files from it.

This is not actually the case.  There is more than one implementation of
UEFI, and they do not all implement PEI/DXE/etc from the PI spec.  For
two examples, there are still vendors that ship EDK-I implementations on
some platforms, and current u-boot can be built to export a UEFI API.
(It's a subset, but so is every other implementation.)

> If that protocol wasn't supported, the existing EFI drivers wouldn't
> be able to function as they couldn't load files from the volume.

This is not correct.  Not all UEFI implementations implement *any* of
PI.  Also, AFAIK, nothing we use in Linux so far directly depends on
anything in PI.

> > What you are describing sounds like significantly more work then
> > the vendor just embedding the firmware as a char firmware[] in their
> > EFI mouse driver.
> 
> In such cases, read the EFI mouse driver from the firmware volume and
> extract the firmware from the offset you've pre-determined.  That's
> never going to change since the devices never receive updates, as
> you're saying.  So basically you'll have a struct with GUID, offset,
> length, and the name under which the firmware is made available to
> Linux drivers.

No, he's saying that it's really easy to implement a driver with the
device firmware in a char [] in the code, so cheap ODMs who supply a
UEFI driver with their hardware part are very, very likely to have done
that instead of providing two things to go into the FV - a UEFI driver
*and* a separate image for the device firmware.  This also cuts out a
point of failure between the OEM and the ODM.

> > That combined with Peter's worries about difficulties parsing the
> > Firmware Volume stuff, makes me believe that it is best to just
> > stick with my current approach as Peter suggests.
> 
> I don't know why you insist on a hackish solution (your own words)
> even though a cleaner solution is suggested, but fortunately it's
> not for me to decide what goes in and what doesn't. ;-)

It already works...

-- 
  Peter

Re: [GIT PULL] Kernel lockdown for secure boot

[Peter Jones]

On Tue, Apr 03, 2018 at 02:51:23PM -0700, Andy Lutomirski wrote:
> On Tue, Apr 3, 2018 at 12:29 PM, Matthew Garrett  wrote:
> > On Tue, Apr 3, 2018 at 9:46 AM Andy Lutomirski  wrote:
> >> On Tue, Apr 3, 2018 at 9:29 AM, Matthew Garrett  wrote:
> >> > A kernel that allows users arbitrary access to ring 0 is just an
> >> > overfeatured bootloader. Why would you want secure boot in that case?
> >
> >> To get a chain of trust.  I can provision a system with some public
> >> keys, stored in UEFI authenticated variables, such that the system
> >> will only boot a signed image.  That signed image, can, in turn, load
> >> a signed (or hashed or otherwise verfified) kernel and a verified
> >> initramfs.  The initramfs can run a full system from a verified (using
> >> dm-verity or similar) filesystem, for example.  Now it's very hard to
> >> persistently attack this system.  Chromium OS does something very much
> >> like this, except that it doesn't use UEFI as far as I know.  So does
> >> iOS, and so do some Android versions.  None of this requires lockdown,
> >> or even a separation between usermode and kernelmode, to work
> >> correctly.  One could even do this on an MMU-less system if one really
> >> cared to.  More usefully, someone probably has done this using a
> >> unikernel.
> >
> > That's only viable if you're the only person with the ability to sign stuff
> > for your machine - the moment there are generic distributions that your
> > machine trusts, an attacker can use one as a bootloader to compromise your
> > trust chain.
> 
> 
> If you removed "as a bootloader", then I agree with that sentence.
> 
> Can someone please explain why the UEFI crowd cares so much about "as
> a bootloader"?  Once I'm able to install an OS (Linux kernel +
> bootloader, Windows embedded doodad, OpenBSD, whatever) on your
> machine, I can use your peripherals, read your data, write your data,
> see your keystrokes, use your network connection, re-flash your BIOS
> (at least as well as any OS can), run VMs, and generally own your
> system.  Somehow you all seem fine with all of this, except that the
> fact that I can chainload something else gives UEFI people the
> willies.
> 
> Can someone explain why?

There's no inherent difference, in terms of the trust chain, between
compromising it to use the machine as a toaster or to run a botnet - the
trust chain is compromised either way.  But you're much more likely to
notice if your desktop starts producing bread products than if it hides
some malware and keeps on booting, and the second one is much more
attractive to attackers anyway.

The reason we talk about it as a bootloader is because of the model
employed by malware.  I'm sure you know that one kind of malware that
exists in the wild, a so-called "boot kit", operates by modifying a
kernel during load (or on disk before loading) so that it has some
malicious payload, like exfiltrating user data or allowing a way to
install software that the kernel hides or *whatever*, and incorporating
some way to achieve relative persistence on the system - for example
hiding the real boot settings and loading a kernel with a different than
normal initramfs that loads an exploit before continuing with a normal
looking boot.

As Kees has pointed out, the lockdown portion of this is about
separating uid-0 from ring-0.  There are a lot of reasons to want to do
that, of course.  But the reason Secure Boot exists, and ultimately the
reason we started trying to do this, is so you can't build the
persistence mechanism for a boot kit by using a trusted kernel to
springboard into a modified one, even if it's the same kernel just
modified before kexec.  If you can do that, you can use that to build
the persistence mechanism in a boot kit.

That is to say, as a result of the way malware has been written, our way
of thinking about it is often that it's a way to build a boot loader for
a malicious kernel, so that's how we wind up talking about it.  Are we
concerned with malware stealing your data?  Yes, but Secure Boot is only
indirectly about that.  It's primarily about denying the malware easy
mechanisms to build a persistence mechanism.  The uid-0 != ring-0 aspect
is useful independent of Secure Boot, but Secure Boot without it falls
way short of accomplishing its goal.

--
  Peter

Re: [GIT PULL] Kernel lockdown for secure boot

[Peter Jones]

On Tue, Apr 03, 2018 at 02:51:23PM -0700, Andy Lutomirski wrote:
> On Tue, Apr 3, 2018 at 12:29 PM, Matthew Garrett  wrote:
> > On Tue, Apr 3, 2018 at 9:46 AM Andy Lutomirski  wrote:
> >> On Tue, Apr 3, 2018 at 9:29 AM, Matthew Garrett  wrote:
> >> > A kernel that allows users arbitrary access to ring 0 is just an
> >> > overfeatured bootloader. Why would you want secure boot in that case?
> >
> >> To get a chain of trust.  I can provision a system with some public
> >> keys, stored in UEFI authenticated variables, such that the system
> >> will only boot a signed image.  That signed image, can, in turn, load
> >> a signed (or hashed or otherwise verfified) kernel and a verified
> >> initramfs.  The initramfs can run a full system from a verified (using
> >> dm-verity or similar) filesystem, for example.  Now it's very hard to
> >> persistently attack this system.  Chromium OS does something very much
> >> like this, except that it doesn't use UEFI as far as I know.  So does
> >> iOS, and so do some Android versions.  None of this requires lockdown,
> >> or even a separation between usermode and kernelmode, to work
> >> correctly.  One could even do this on an MMU-less system if one really
> >> cared to.  More usefully, someone probably has done this using a
> >> unikernel.
> >
> > That's only viable if you're the only person with the ability to sign stuff
> > for your machine - the moment there are generic distributions that your
> > machine trusts, an attacker can use one as a bootloader to compromise your
> > trust chain.
> 
> 
> If you removed "as a bootloader", then I agree with that sentence.
> 
> Can someone please explain why the UEFI crowd cares so much about "as
> a bootloader"?  Once I'm able to install an OS (Linux kernel +
> bootloader, Windows embedded doodad, OpenBSD, whatever) on your
> machine, I can use your peripherals, read your data, write your data,
> see your keystrokes, use your network connection, re-flash your BIOS
> (at least as well as any OS can), run VMs, and generally own your
> system.  Somehow you all seem fine with all of this, except that the
> fact that I can chainload something else gives UEFI people the
> willies.
> 
> Can someone explain why?

There's no inherent difference, in terms of the trust chain, between
compromising it to use the machine as a toaster or to run a botnet - the
trust chain is compromised either way.  But you're much more likely to
notice if your desktop starts producing bread products than if it hides
some malware and keeps on booting, and the second one is much more
attractive to attackers anyway.

The reason we talk about it as a bootloader is because of the model
employed by malware.  I'm sure you know that one kind of malware that
exists in the wild, a so-called "boot kit", operates by modifying a
kernel during load (or on disk before loading) so that it has some
malicious payload, like exfiltrating user data or allowing a way to
install software that the kernel hides or *whatever*, and incorporating
some way to achieve relative persistence on the system - for example
hiding the real boot settings and loading a kernel with a different than
normal initramfs that loads an exploit before continuing with a normal
looking boot.

As Kees has pointed out, the lockdown portion of this is about
separating uid-0 from ring-0.  There are a lot of reasons to want to do
that, of course.  But the reason Secure Boot exists, and ultimately the
reason we started trying to do this, is so you can't build the
persistence mechanism for a boot kit by using a trusted kernel to
springboard into a modified one, even if it's the same kernel just
modified before kexec.  If you can do that, you can use that to build
the persistence mechanism in a boot kit.

That is to say, as a result of the way malware has been written, our way
of thinking about it is often that it's a way to build a boot loader for
a malicious kernel, so that's how we wind up talking about it.  Are we
concerned with malware stealing your data?  Yes, but Secure Boot is only
indirectly about that.  It's primarily about denying the malware easy
mechanisms to build a persistence mechanism.  The uid-0 != ring-0 aspect
is useful independent of Secure Boot, but Secure Boot without it falls
way short of accomplishing its goal.

--
  Peter

Re: [PATCH 2/2] efi: Add embedded peripheral firmware support

[Peter Jones]

On Tue, Apr 03, 2018 at 06:58:48PM +, Luis R. Rodriguez wrote:
> On Tue, Apr 03, 2018 at 08:07:11PM +0200, Lukas Wunner wrote:
> > On Tue, Apr 03, 2018 at 10:33:25AM +0200, Hans de Goede wrote:
> > > I asked Peter Jones for suggestions how to extract this during boot and
> > > he suggested seeing if there was a copy of the firmware in the
> > > EFI_BOOT_SERVICES_CODE memory segment, which it turns out there is.
> > > 
> > > My patch to add support for this contains a table of device-model (dmi
> > > strings), firmware header (first 64 bits), length and crc32 and then if
> > > we boot on a device-model which is in the table the code scans the
> > > EFI_BOOT_SERVICES_CODE for the prefix, if found checks the crc and
> > > caches the firmware for later use by request-firmware.
> > > 
> > > So I just do a brute-force search for the firmware, this really is hack,
> > > nothing standard about it I'm afraid. But it works on 4 different x86
> > > tablets I have and makes the touchscreen work OOTB on them, so I believe
> > > it is a worthwhile hack to have.
> > 
> > The EFI Firmware Volume contains a kind of filesystem with files
> > identified by GUIDs.  Those files include EFI drivers, ACPI tables,
> > DMI data and so on.  It is actually quite common for vendors to
> > also include device firmware on the Firmware Volume.  Apple is doing
> > this to ship firmware updates e.g. for the GMUX controller found on
> > dual GPU MacBook Pros.  If they want to update the controller's
> > firmware, they include it in a BIOS update, and an EFI driver checks
> > on boot if the firmware update for the controller is necessary and
> > if so, flashes it.
> > 
> > The firmware files you're looking for are almost certainly included
> > on the Firmware Volume as individual files.
> 
> What Hans implemented seems to have been for a specific x86 hack, best if we
> confirm if indeed they are present on the Firmware Volume.

To be honest, I'm a bit skeptical about the firmware volume approach.
Tools like UEFITool[0] and uefi-firmware-parser[1] have existed for
years, still don't seem to reliably parse firmware images I see in the
wild, and have a fairly regular need for fixes.  These are tools
maintained by smart people who are making a real effort, and it still
looks pretty hard to do a good job that applies across a lot of
platforms.

So I'd rather use Hans's existing patches, at least for now, and if
someone is interested in hacking on making an efi firmware volume parser
for the kernel, switch them to that when such a thing is ready.

[0] g...@github.com:LongSoft/UEFITool.git
[1] g...@github.com:theopolis/uefi-firmware-parser.git

> > Rather than scraping
> > the EFI memory for firmware, I think it would be cleaner and more
> > elegant if you just retrieve the files you're interested in from
> > the Firmware Volume.
> > 
> > We're doing something similar with Apple EFI properties, see
> > 58c5475aba67 and c9cc3aaa0281.
> > 
> > Basically what you need to do to implement this approach is:
> > 
> > * Determine the GUIDs used by vendors for the files you're interested
> >   in.  Either dump the Firmware Volume or take an EFI update as
> >   shipped by the vendor, then feed it to UEFIExtract:
> >   https://github.com/LongSoft/UEFITool
> >   
> > * Add the EFI Firmware Volume Protocol to include/linux/efi.h:
> >   
> > https://www.intel.com/content/dam/doc/reference-guide/efi-firmware-file-volume-specification.pdf
> > 
> > * Amend arch/x86/boot/compressed/eboot.c to read the files with the
> >   GUIDs you're interested in into memory and pass the files to the
> >   kernel as setup_data payloads.
> > 
> > * Once the kernel has booted, make the files you've retrieved
> >   available to device drivers as firmware blobs.
> 
> Happen to know if devices using Firmware Volumes also sign their firmware
> and if hw checks the firmware at load time?

It varies on a per-device basis, of course.  Most new Intel machines as
of Haswell *should* be verifying their system firmware via Boot Guard,
which both checks an RSA signature and measures the firmware into the
TPM, but as with everything of this nature, there are certainly vendors
that screw it up. (I think AMD has something similar, but I'm really not
sure.)

-- 
  Peter

Re: [PATCH 2/2] efi: Add embedded peripheral firmware support

[Peter Jones]

On Tue, Apr 03, 2018 at 06:58:48PM +, Luis R. Rodriguez wrote:
> On Tue, Apr 03, 2018 at 08:07:11PM +0200, Lukas Wunner wrote:
> > On Tue, Apr 03, 2018 at 10:33:25AM +0200, Hans de Goede wrote:
> > > I asked Peter Jones for suggestions how to extract this during boot and
> > > he suggested seeing if there was a copy of the firmware in the
> > > EFI_BOOT_SERVICES_CODE memory segment, which it turns out there is.
> > > 
> > > My patch to add support for this contains a table of device-model (dmi
> > > strings), firmware header (first 64 bits), length and crc32 and then if
> > > we boot on a device-model which is in the table the code scans the
> > > EFI_BOOT_SERVICES_CODE for the prefix, if found checks the crc and
> > > caches the firmware for later use by request-firmware.
> > > 
> > > So I just do a brute-force search for the firmware, this really is hack,
> > > nothing standard about it I'm afraid. But it works on 4 different x86
> > > tablets I have and makes the touchscreen work OOTB on them, so I believe
> > > it is a worthwhile hack to have.
> > 
> > The EFI Firmware Volume contains a kind of filesystem with files
> > identified by GUIDs.  Those files include EFI drivers, ACPI tables,
> > DMI data and so on.  It is actually quite common for vendors to
> > also include device firmware on the Firmware Volume.  Apple is doing
> > this to ship firmware updates e.g. for the GMUX controller found on
> > dual GPU MacBook Pros.  If they want to update the controller's
> > firmware, they include it in a BIOS update, and an EFI driver checks
> > on boot if the firmware update for the controller is necessary and
> > if so, flashes it.
> > 
> > The firmware files you're looking for are almost certainly included
> > on the Firmware Volume as individual files.
> 
> What Hans implemented seems to have been for a specific x86 hack, best if we
> confirm if indeed they are present on the Firmware Volume.

To be honest, I'm a bit skeptical about the firmware volume approach.
Tools like UEFITool[0] and uefi-firmware-parser[1] have existed for
years, still don't seem to reliably parse firmware images I see in the
wild, and have a fairly regular need for fixes.  These are tools
maintained by smart people who are making a real effort, and it still
looks pretty hard to do a good job that applies across a lot of
platforms.

So I'd rather use Hans's existing patches, at least for now, and if
someone is interested in hacking on making an efi firmware volume parser
for the kernel, switch them to that when such a thing is ready.

[0] g...@github.com:LongSoft/UEFITool.git
[1] g...@github.com:theopolis/uefi-firmware-parser.git

> > Rather than scraping
> > the EFI memory for firmware, I think it would be cleaner and more
> > elegant if you just retrieve the files you're interested in from
> > the Firmware Volume.
> > 
> > We're doing something similar with Apple EFI properties, see
> > 58c5475aba67 and c9cc3aaa0281.
> > 
> > Basically what you need to do to implement this approach is:
> > 
> > * Determine the GUIDs used by vendors for the files you're interested
> >   in.  Either dump the Firmware Volume or take an EFI update as
> >   shipped by the vendor, then feed it to UEFIExtract:
> >   https://github.com/LongSoft/UEFITool
> >   
> > * Add the EFI Firmware Volume Protocol to include/linux/efi.h:
> >   
> > https://www.intel.com/content/dam/doc/reference-guide/efi-firmware-file-volume-specification.pdf
> > 
> > * Amend arch/x86/boot/compressed/eboot.c to read the files with the
> >   GUIDs you're interested in into memory and pass the files to the
> >   kernel as setup_data payloads.
> > 
> > * Once the kernel has booted, make the files you've retrieved
> >   available to device drivers as firmware blobs.
> 
> Happen to know if devices using Firmware Volumes also sign their firmware
> and if hw checks the firmware at load time?

It varies on a per-device basis, of course.  Most new Intel machines as
of Haswell *should* be verifying their system firmware via Boot Guard,
which both checks an RSA signature and measures the firmware into the
TPM, but as with everything of this nature, there are certainly vendors
that screw it up. (I think AMD has something similar, but I'm really not
sure.)

-- 
  Peter

Re: [PATCH 2/2] efi: Add embedded peripheral firmware support

[Peter Jones]

On Sat, Mar 31, 2018 at 02:19:44PM +0200, Hans de Goede wrote:
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index fddc5f706fd2..1a5ea950f58f 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -455,6 +455,7 @@ int __init efi_mem_desc_lookup(u64 phys_addr, 
> efi_memory_desc_t *out_md)
>   u64 end;
>  
>   if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
> + md->type != EFI_BOOT_SERVICES_CODE &&
>   md->type != EFI_BOOT_SERVICES_DATA &&
>   md->type != EFI_RUNTIME_SERVICES_DATA) {
>   continue;

Might be worth adding a comment here to ensure nobody comes along later
and adds something like EFI_BOOT_LOADER_DATA or other stuff that's
allocated later here.  I don't want to accidentally patch our way into
having the ability to stumble across a firmware blob somebody dumped
into the middle of a grub config file, especially since you only need to
collide crc32 (within the same length) to pre-alias a match.

...
> +static int __init efi_check_md_for_embedded_firmware(
> + efi_memory_desc_t *md, const struct embedded_fw_desc *desc)
> +{
...
> + if (found_fw_count >= MAX_EMBEDDED_FIRMWARES) {
> + pr_err("Error already have %d embedded firmwares\n",
> +MAX_EMBEDDED_FIRMWARES);
> + return -ENOSPC;
> + }

Doesn't seem like this needs to be pr_err(); after all we have already
found a valid match, so the firmware vendor has done something
moderately stupid, but we have a firmware that will probably work.  Of
course it still needs to return != 0, but pr_warn() or even pr_info()
seems more reasonable.

Aside from those nits, looks good to me.

Reviewed-by: Peter Jones <pjo...@redhat.com>

-- 
  Peter

Re: [PATCH 2/2] efi: Add embedded peripheral firmware support

[Peter Jones]

On Sat, Mar 31, 2018 at 02:19:44PM +0200, Hans de Goede wrote:
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index fddc5f706fd2..1a5ea950f58f 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -455,6 +455,7 @@ int __init efi_mem_desc_lookup(u64 phys_addr, 
> efi_memory_desc_t *out_md)
>   u64 end;
>  
>   if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
> + md->type != EFI_BOOT_SERVICES_CODE &&
>   md->type != EFI_BOOT_SERVICES_DATA &&
>   md->type != EFI_RUNTIME_SERVICES_DATA) {
>   continue;

Might be worth adding a comment here to ensure nobody comes along later
and adds something like EFI_BOOT_LOADER_DATA or other stuff that's
allocated later here.  I don't want to accidentally patch our way into
having the ability to stumble across a firmware blob somebody dumped
into the middle of a grub config file, especially since you only need to
collide crc32 (within the same length) to pre-alias a match.

...
> +static int __init efi_check_md_for_embedded_firmware(
> + efi_memory_desc_t *md, const struct embedded_fw_desc *desc)
> +{
...
> + if (found_fw_count >= MAX_EMBEDDED_FIRMWARES) {
> + pr_err("Error already have %d embedded firmwares\n",
> +MAX_EMBEDDED_FIRMWARES);
> + return -ENOSPC;
> + }

Doesn't seem like this needs to be pr_err(); after all we have already
found a valid match, so the firmware vendor has done something
moderately stupid, but we have a firmware that will probably work.  Of
course it still needs to return != 0, but pr_warn() or even pr_info()
seems more reasonable.

Aside from those nits, looks good to me.

Reviewed-by: Peter Jones 

-- 
  Peter

Re: [PATCH] efivarfs: Limit the rate for non-root to read files

[Peter Jones]

On Thu, Feb 22, 2018 at 06:11:14AM +, Luck, Tony wrote:
>> On Feb 21, 2018, at 21:52, Linus Torvalds wrote:
>> 
>> Does the error return actually break real users? Not "I can do did
>> things and it acts differently" things, but actual users...
> 
> Probably not. Peter Jones said that efibootmgr might access up to 20
> files. Assuming it is sanely reading in big chunks, it won’t hit the
> rate limit that I set at 100.

Typically each read looks like:

openat(AT_FDCWD, 
"/sys/firmware/efi/efivars/Boot0001-8be4df61-93ca-11d2-aa0d-00e098032b8c", 
O_RDONLY) = 3
read(3, "\7\0\0\0", 4)  = 4
read(3, "\1\0\0\0b\0t\0e\0s\0t\0\0\0\4\1*\0\1\0\0\0\0\10\0\0\0\0\0\0"..., 4096) 
= 114
read(3, "", 3982)   = 0
close(3)= 0

For each multiple of 4k, you'll see one more read() call.  (It's two
reads because libefivar's efi_get_variable() returns the attributes
separately from the data, which goes in an allocation the caller is
responsible for freeing, so doing it as one read means it would
introduce an extra copy.)  Looking at that code path, if it *does* get
tripped up by EAGAIN, it should handle it fine, though maybe I should
add a short randomized delay (or just sched_yield()) in that case.

I don't think the 3rd read there to detect EOF hits the efivarfs code,
so that's 2 reads per variable until you go over 4k, which most never
do.

That pattern is true of everything that uses libefivar to do its EFI
variable manipulation.  On my moderately typical laptop with 2 boot
variables set, it looks something like:

trillian:~$ strace -o efibootmgr.strace efibootmgr -v 
BootCurrent: 0001
Timeout: 0 seconds
BootOrder: 0001,
Boot* Fedora
HD(1,GPT,2cf5261b-7b98-48c0-ae54-463dbd23e65b,0x800,0x64000)/File(\EFI\fedora\shimx64.efi)
Boot0001* test  
HD(1,GPT,2cf5261b-7b98-48c0-ae54-463dbd23e65b,0x800,0x64000)/File(\EFI\fedora\shimx64.efi)
trillian:~$ grep '^\(open\|read\)' efibootmgr.strace | grep -A100 sys/firmware  
| grep -c ^read
15

Which, if I'm write about VFS eating that last read, means 10 calls.
Many machines have some default boot variables; my desktop at home has 5
completely useless variables the firmware sets.  So there it winds up
being 27 calls to read(2), and thus 18 calls to count towards our limit.

Your limit at 100 looks sufficiently large to me.

-- 
  Peter

Re: [PATCH] efivarfs: Limit the rate for non-root to read files

[Peter Jones]

On Thu, Feb 22, 2018 at 06:11:14AM +, Luck, Tony wrote:
>> On Feb 21, 2018, at 21:52, Linus Torvalds wrote:
>> 
>> Does the error return actually break real users? Not "I can do did
>> things and it acts differently" things, but actual users...
> 
> Probably not. Peter Jones said that efibootmgr might access up to 20
> files. Assuming it is sanely reading in big chunks, it won’t hit the
> rate limit that I set at 100.

Typically each read looks like:

openat(AT_FDCWD, 
"/sys/firmware/efi/efivars/Boot0001-8be4df61-93ca-11d2-aa0d-00e098032b8c", 
O_RDONLY) = 3
read(3, "\7\0\0\0", 4)  = 4
read(3, "\1\0\0\0b\0t\0e\0s\0t\0\0\0\4\1*\0\1\0\0\0\0\10\0\0\0\0\0\0"..., 4096) 
= 114
read(3, "", 3982)   = 0
close(3)= 0

For each multiple of 4k, you'll see one more read() call.  (It's two
reads because libefivar's efi_get_variable() returns the attributes
separately from the data, which goes in an allocation the caller is
responsible for freeing, so doing it as one read means it would
introduce an extra copy.)  Looking at that code path, if it *does* get
tripped up by EAGAIN, it should handle it fine, though maybe I should
add a short randomized delay (or just sched_yield()) in that case.

I don't think the 3rd read there to detect EOF hits the efivarfs code,
so that's 2 reads per variable until you go over 4k, which most never
do.

That pattern is true of everything that uses libefivar to do its EFI
variable manipulation.  On my moderately typical laptop with 2 boot
variables set, it looks something like:

trillian:~$ strace -o efibootmgr.strace efibootmgr -v 
BootCurrent: 0001
Timeout: 0 seconds
BootOrder: 0001,
Boot* Fedora
HD(1,GPT,2cf5261b-7b98-48c0-ae54-463dbd23e65b,0x800,0x64000)/File(\EFI\fedora\shimx64.efi)
Boot0001* test  
HD(1,GPT,2cf5261b-7b98-48c0-ae54-463dbd23e65b,0x800,0x64000)/File(\EFI\fedora\shimx64.efi)
trillian:~$ grep '^\(open\|read\)' efibootmgr.strace | grep -A100 sys/firmware  
| grep -c ^read
15

Which, if I'm write about VFS eating that last read, means 10 calls.
Many machines have some default boot variables; my desktop at home has 5
completely useless variables the firmware sets.  So there it winds up
being 27 calls to read(2), and thus 18 calls to count towards our limit.

Your limit at 100 looks sufficiently large to me.

-- 
  Peter

Re: [PATCH 1/2] fs/efivarfs: restrict inode permissions

[Peter Jones]

On Tue, Feb 20, 2018 at 02:01:51PM -0800, Linus Torvalds wrote:

> Which variables are actually used by user space tools? It sounds like
> it may be exactly those boot order things that we already have flags
> and special behavior for.

Not that many are really part of a non-root workflow.  The biggest
consumer that there's a real /user/ workflow for is Matthew's tpmtotp,
which lets you pick your own when you set it up as root and merely read
from it as a user in perpetuity.  Most workflows are of the same form as
"I run efibootmgr as a user to list things and then use sudo when I
actually need to make some changes."

> And no, I don't believe in the "SMI can trigger a DoS" argument. Those
> garbage efivars that *do* trigger SMI's should me marked and shamed,
> and maybe _they_ need to be added to the list of things that you
> should look out for. Root or not.

It's all of them except the very few that are generated during bootup.
It's worth noting, though, that EFI does not actually require this
implementation behavior in any way.  This is all about Intel's
choice to use SMI to protect the variable store.

> And just on general principlies, I don't want to see weasel-wordy
> commit messages like
> 
>  "Reading certain EFI variables trigger SMIs"
> 
> I understand *writing* them causing SMI's due to some flash protection
> scheme. What's the reading thing? And why aren't we calling that
> garbage out?

Assuming most Intel CPUs work more or less the same as Galileo in this
particular regard, what's going on is the SPI part is connected to the
North Cluster, which presents an MMIO interface to program it, and SMM
is configured so that touching those addresses triggers an SMI.  This
way they can protect the the "authenticated" variables by checking
signatures inside SMM.

So basically it's not "Reading certain variables trigger SMIs", it's
"reading any variable that's not completely fake causes SMI".  The
result is any user can not merely trigger an SMI, but really more like
they can hold it asserted.

> So even if we do need to limit reading, I want out comments (in core
> or commits) to actually explain *Why*., instead of this kind of
> non-specific fear-mongering that nobody can really say yay or nay
> about.

Yeah, makes sense.

There are other options, but they're not great either.  For example, On
most systems the total data is <100kB, so we could make a default-on
config option to just cache it all during boot by default.  Like the
options suggested so far, it has downsides, but it's not the end of the
world for most systems.

-- 
Peter

Re: [PATCH 1/2] fs/efivarfs: restrict inode permissions

[Peter Jones]

On Tue, Feb 20, 2018 at 02:01:51PM -0800, Linus Torvalds wrote:

> Which variables are actually used by user space tools? It sounds like
> it may be exactly those boot order things that we already have flags
> and special behavior for.

Not that many are really part of a non-root workflow.  The biggest
consumer that there's a real /user/ workflow for is Matthew's tpmtotp,
which lets you pick your own when you set it up as root and merely read
from it as a user in perpetuity.  Most workflows are of the same form as
"I run efibootmgr as a user to list things and then use sudo when I
actually need to make some changes."

> And no, I don't believe in the "SMI can trigger a DoS" argument. Those
> garbage efivars that *do* trigger SMI's should me marked and shamed,
> and maybe _they_ need to be added to the list of things that you
> should look out for. Root or not.

It's all of them except the very few that are generated during bootup.
It's worth noting, though, that EFI does not actually require this
implementation behavior in any way.  This is all about Intel's
choice to use SMI to protect the variable store.

> And just on general principlies, I don't want to see weasel-wordy
> commit messages like
> 
>  "Reading certain EFI variables trigger SMIs"
> 
> I understand *writing* them causing SMI's due to some flash protection
> scheme. What's the reading thing? And why aren't we calling that
> garbage out?

Assuming most Intel CPUs work more or less the same as Galileo in this
particular regard, what's going on is the SPI part is connected to the
North Cluster, which presents an MMIO interface to program it, and SMM
is configured so that touching those addresses triggers an SMI.  This
way they can protect the the "authenticated" variables by checking
signatures inside SMM.

So basically it's not "Reading certain variables trigger SMIs", it's
"reading any variable that's not completely fake causes SMI".  The
result is any user can not merely trigger an SMI, but really more like
they can hold it asserted.

> So even if we do need to limit reading, I want out comments (in core
> or commits) to actually explain *Why*., instead of this kind of
> non-specific fear-mongering that nobody can really say yay or nay
> about.

Yeah, makes sense.

There are other options, but they're not great either.  For example, On
most systems the total data is <100kB, so we could make a default-on
config option to just cache it all during boot by default.  Like the
options suggested so far, it has downsides, but it's not the end of the
world for most systems.

-- 
Peter

Re: [PATCH 0/2] efivars: reading variables can generate SMIs

[Peter Jones]

On Fri, Feb 16, 2018 at 09:09:30PM +, Luck, Tony wrote:
> > That said, I'm not sure how many non-root users run the toolkit to
> > extract their EFI certificates or check on the secure boot status of
> > the system, but I suspect it might be non-zero: I can see the tinfoil
> > hat people wanting at least to check the secure boot status when they
> > log in.
> 
> Another fix option might be to rate limit EFI calls for non-root users (on X86
> since only we have the SMI problem). That would:
> 
> 1) Avoid using memory to cache all the variables
> 2) Catch any other places where non-root users can call EFI

I could get behind that as well.  Currently the things I maintain do
approximately this many normal accesses with invocations you can do as a
user:

"efibootmgr -v" - six files we always try to read, plus one per Boot
  entry.
"fwupdate --info" - one file it always tries to read, one file for each
ESRT entry.
"dbxtool -l" - one file it always reads.
"mokutil --sb-state" - reads the same file twice.  I don't maintain
   this, but I'll send a patch to Gary to make it
   only read it once.  AFAICS all of the other
   invocations you can currently do as a user
   /legitimately/ read two files, though.

Some systems seem to *love* making a pile of Boot entries; I think
the most I've seen is something like 16.  So on that machine, one
"efibootmgr -v" invocation is ~22 efivars files read.  I've never seen a
machine that advertised more than 2 ESRT entries, but maybe we'll get
there some day.

-- 
  Peter

Re: [PATCH 0/2] efivars: reading variables can generate SMIs

[Peter Jones]

On Fri, Feb 16, 2018 at 09:09:30PM +, Luck, Tony wrote:
> > That said, I'm not sure how many non-root users run the toolkit to
> > extract their EFI certificates or check on the secure boot status of
> > the system, but I suspect it might be non-zero: I can see the tinfoil
> > hat people wanting at least to check the secure boot status when they
> > log in.
> 
> Another fix option might be to rate limit EFI calls for non-root users (on X86
> since only we have the SMI problem). That would:
> 
> 1) Avoid using memory to cache all the variables
> 2) Catch any other places where non-root users can call EFI

I could get behind that as well.  Currently the things I maintain do
approximately this many normal accesses with invocations you can do as a
user:

"efibootmgr -v" - six files we always try to read, plus one per Boot
  entry.
"fwupdate --info" - one file it always tries to read, one file for each
ESRT entry.
"dbxtool -l" - one file it always reads.
"mokutil --sb-state" - reads the same file twice.  I don't maintain
   this, but I'll send a patch to Gary to make it
   only read it once.  AFAICS all of the other
   invocations you can currently do as a user
   /legitimately/ read two files, though.

Some systems seem to *love* making a pile of Boot entries; I think
the most I've seen is something like 16.  So on that machine, one
"efibootmgr -v" invocation is ~22 efivars files read.  I've never seen a
machine that advertised more than 2 ESRT entries, but maybe we'll get
there some day.

-- 
  Peter

Re: [PATCH 0/2] efivars: reading variables can generate SMIs

[Peter Jones]

On Fri, Feb 16, 2018 at 07:32:17PM +, Luck, Tony wrote:
> > tl;dr: I think changing everything to 0600 is probably completely fine,
> > and whitelisting is probably pointless.  
> 
> But do you speak for all users?

No, I just write their tools :)

> It will just take one person complaining that efibootmgr no longer
> shows them what it used to show to bring down the wrath of Linus on
> our (specifically Joe's) head for breaking user space.

The userland use case is gazing idly at the values without intending to
do anything about them.  And most of this is firmware config and
firmware/OS interaction.  Honestly it should never have been user
readable to begin with.

But also, we had a bug for quite some time where efibootmgr created
everything as 0600, and as a result non-root users couldn't do e.g.
"efibootmgr -v" and see the paths of new entries until a reboot
occurred.  Nobody ever reported it in bugzilla.redhat.com or efibootmgr
or efivar's github issues pages.  One person noticed it while commenting
about another issue, but didn't see it as related to his actual issue or
being a bug so much as "weird" that listing worked as non-root before
changing something but not after.

Another user asked about getting permission denied while setting the
boot order on askubuntu here:
https://askubuntu.com/questions/688317/getting-permission-denied-errors-from-efibootmgr
The response was exactly that you have to run it as root.  I think it's
telling that nobody said anything about reading vs writing.

> I've got someone about to start looking at making efivarfs read and save
> the values during mount, so all the read-only options can continue to work
> without making EFI calls.
> 
> This will cost some memory (say 20-30 variables at up to 1K each).

71 variables on my laptop, and the 1K restriction went away a *lng*
time ago.  It was fully excised from the userland tools in 2013.  On my
laptop, 4 of those 71 variables are >5000 bytes.  The total storage of
all of the data in the variables is 38kB.

I still think changing it to 0600 and calling this done is the right
thing to do.

-- 
  Peter

Re: [PATCH 0/2] efivars: reading variables can generate SMIs

[Peter Jones]

On Fri, Feb 16, 2018 at 07:32:17PM +, Luck, Tony wrote:
> > tl;dr: I think changing everything to 0600 is probably completely fine,
> > and whitelisting is probably pointless.  
> 
> But do you speak for all users?

No, I just write their tools :)

> It will just take one person complaining that efibootmgr no longer
> shows them what it used to show to bring down the wrath of Linus on
> our (specifically Joe's) head for breaking user space.

The userland use case is gazing idly at the values without intending to
do anything about them.  And most of this is firmware config and
firmware/OS interaction.  Honestly it should never have been user
readable to begin with.

But also, we had a bug for quite some time where efibootmgr created
everything as 0600, and as a result non-root users couldn't do e.g.
"efibootmgr -v" and see the paths of new entries until a reboot
occurred.  Nobody ever reported it in bugzilla.redhat.com or efibootmgr
or efivar's github issues pages.  One person noticed it while commenting
about another issue, but didn't see it as related to his actual issue or
being a bug so much as "weird" that listing worked as non-root before
changing something but not after.

Another user asked about getting permission denied while setting the
boot order on askubuntu here:
https://askubuntu.com/questions/688317/getting-permission-denied-errors-from-efibootmgr
The response was exactly that you have to run it as root.  I think it's
telling that nobody said anything about reading vs writing.

> I've got someone about to start looking at making efivarfs read and save
> the values during mount, so all the read-only options can continue to work
> without making EFI calls.
> 
> This will cost some memory (say 20-30 variables at up to 1K each).

71 variables on my laptop, and the 1K restriction went away a *lng*
time ago.  It was fully excised from the userland tools in 2013.  On my
laptop, 4 of those 71 variables are >5000 bytes.  The total storage of
all of the data in the variables is 38kB.

I still think changing it to 0600 and calling this done is the right
thing to do.

-- 
  Peter

Re: [PATCH 0/2] efivars: reading variables can generate SMIs

[Peter Jones]

On Fri, Feb 16, 2018 at 10:48:32AM -0800, Joe Konno wrote:
> On Fri, Feb 16, 2018 at 11:18:12AM +, Ard Biesheuvel wrote:
> > On 16 February 2018 at 11:08, Borislav Petkov  wrote:
> > > On Fri, Feb 16, 2018 at 10:58:47AM +, Ard Biesheuvel wrote:
> > >> By your own reasoning above, that's a no-no as well.
> > >
> > > I'm sure we can come up with some emulation - the same way we did the
> > > BIOS emulation.
> > >
> > >> But thanks for your input. Anyone else got something constructive to 
> > >> contribute?
> > >
> > > The not-breaking userspace is constructive contribution. The last
> > > paragraph is my usual rant.
> > >
> > 
> > Fair enough. And I am not disagreeing with you either.
> > 
> > So question to Joe: is it well defined which variables may exhibit
> > this behavior?
> 
> For brevity's sake, "not yet." Folks-- e.g. firmware writers and
> equipment makers-- may use SMIs more (or less) than others. So, how many
> SMIs generated by the reference shell script can, and will, vary
> platform to platform. I and my colleagues are digging into this.

As a first guess: anything generated during boot is probably not an
SMI.  Everything else is probably an SMI.  In fact, I would expect that
for most systems, the entire list of things that *don't* generate an SMI
(aside from a few IBV specific variables) is all the variables in Table
10 of the UEFI spec that don't have the NV flag.

> > Given that UEFI variables are GUID scoped, would whitelisting certain
> > GUIDs (the ones userland currently relies on to be readable my
> > non-privileged users) and making everything else user-only solve this
> > problem as well?
> 
> Perhaps. I don't want us chasing white rabbits just yet, but the
> whitelist is but one approach under consideration. We may see some other
> patches or RFCs about caching and/or shadowing variable values in
> efivarfs to reduce the number of direct EFI reads, with the goal of
> reducing how many SMIs are generated.
> 
> Any obvious EFI variables that userspace tools have come to depend on--
> those which normal, unprivileged users need to read-- are helpful inputs
> to this discussion.

So, our big userland consumers are efibootmgr, fwupdate, and
"systemctl reboot --firmware-setup".  efibootmgr and fwupdate can do the
"show the current status" half of their job as a user right now, but
they rely on root for the other half anyway.  I don't think we normally
use libfwup as non-root even for reading status.  So basically, the use
case from userland that this will effect looks like:

efibootmgr -v
*scratch head*
sudo su -
efibootmgr -b  -B
efibootmgr -b  -c -L "fixed entry" ...
exit

I don't feel really bad about people having to move the third line up to
the top of that.  It's also not a use case you can have very much of: it
means you manually booted without any valid boot entries.  fallback.efi
would have created a valid boot entry if you didn't do it manually.

"systemctl reboot --firmware-setup" effectively runs as root in all
cases.

The only thing we really must ensure to preserve the other workflows
is making sure creat() and open() with 0644 doesn't return an error (it
obviously won't be preserved across a reboot), because that would break
the existing tools.  But I don't see anything in this patchset which
will cause that.

tl;dr: I think changing everything to 0600 is probably completely fine,
and whitelisting is probably pointless.  
-- 
  Peter

Re: [PATCH 0/2] efivars: reading variables can generate SMIs

[Peter Jones]

On Fri, Feb 16, 2018 at 10:48:32AM -0800, Joe Konno wrote:
> On Fri, Feb 16, 2018 at 11:18:12AM +, Ard Biesheuvel wrote:
> > On 16 February 2018 at 11:08, Borislav Petkov  wrote:
> > > On Fri, Feb 16, 2018 at 10:58:47AM +, Ard Biesheuvel wrote:
> > >> By your own reasoning above, that's a no-no as well.
> > >
> > > I'm sure we can come up with some emulation - the same way we did the
> > > BIOS emulation.
> > >
> > >> But thanks for your input. Anyone else got something constructive to 
> > >> contribute?
> > >
> > > The not-breaking userspace is constructive contribution. The last
> > > paragraph is my usual rant.
> > >
> > 
> > Fair enough. And I am not disagreeing with you either.
> > 
> > So question to Joe: is it well defined which variables may exhibit
> > this behavior?
> 
> For brevity's sake, "not yet." Folks-- e.g. firmware writers and
> equipment makers-- may use SMIs more (or less) than others. So, how many
> SMIs generated by the reference shell script can, and will, vary
> platform to platform. I and my colleagues are digging into this.

As a first guess: anything generated during boot is probably not an
SMI.  Everything else is probably an SMI.  In fact, I would expect that
for most systems, the entire list of things that *don't* generate an SMI
(aside from a few IBV specific variables) is all the variables in Table
10 of the UEFI spec that don't have the NV flag.

> > Given that UEFI variables are GUID scoped, would whitelisting certain
> > GUIDs (the ones userland currently relies on to be readable my
> > non-privileged users) and making everything else user-only solve this
> > problem as well?
> 
> Perhaps. I don't want us chasing white rabbits just yet, but the
> whitelist is but one approach under consideration. We may see some other
> patches or RFCs about caching and/or shadowing variable values in
> efivarfs to reduce the number of direct EFI reads, with the goal of
> reducing how many SMIs are generated.
> 
> Any obvious EFI variables that userspace tools have come to depend on--
> those which normal, unprivileged users need to read-- are helpful inputs
> to this discussion.

So, our big userland consumers are efibootmgr, fwupdate, and
"systemctl reboot --firmware-setup".  efibootmgr and fwupdate can do the
"show the current status" half of their job as a user right now, but
they rely on root for the other half anyway.  I don't think we normally
use libfwup as non-root even for reading status.  So basically, the use
case from userland that this will effect looks like:

efibootmgr -v
*scratch head*
sudo su -
efibootmgr -b  -B
efibootmgr -b  -c -L "fixed entry" ...
exit

I don't feel really bad about people having to move the third line up to
the top of that.  It's also not a use case you can have very much of: it
means you manually booted without any valid boot entries.  fallback.efi
would have created a valid boot entry if you didn't do it manually.

"systemctl reboot --firmware-setup" effectively runs as root in all
cases.

The only thing we really must ensure to preserve the other workflows
is making sure creat() and open() with 0644 doesn't return an error (it
obviously won't be preserved across a reboot), because that would break
the existing tools.  But I don't see anything in this patchset which
will cause that.

tl;dr: I think changing everything to 0600 is probably completely fine,
and whitelisting is probably pointless.  
-- 
  Peter

Re: [PATCH] efifb: allow user to disable write combined mapping.

[Peter Jones]

On Tue, Jul 18, 2017 at 04:09:09PM +1000, Dave Airlie wrote:
> This patch allows the user to disable write combined mapping
> of the efifb framebuffer console using an nowc option.
> 
> A customer noticed major slowdowns while logging to the console
> with write combining enabled, on other tasks running on the same
> CPU. (10x or greater slow down on all other cores on the same CPU
> as is doing the logging).
> 
> I reproduced this on a machine with dual CPUs.
> Intel(R) Xeon(R) CPU E5-2609 v3 @ 1.90GHz (6 core)
> 
> I wrote a test that just mmaps the pci bar and writes to it in
> a loop, while this was running in the background one a single
> core with (taskset -c 1), building a kernel up to init/version.o
> (taskset -c 8) went from 13s to 133s or so. I've yet to explain
> why this occurs or what is going wrong I haven't managed to find
> a perf command that in any way gives insight into this.
> 
> 11,885,070,715  instructions  #1.39  insns per cycle
> vs
> 12,082,592,342  instructions  #0.13  insns per cycle
> 
> is the only thing I've spotted of interest, I've tried at least:
> dTLB-stores,dTLB-store-misses,L1-dcache-stores,LLC-store,LLC-store-misses,LLC-load-misses,LLC-loads,\mem-loads,mem-stores,iTLB-loads,iTLB-load-misses,cache-references,cache-misses
> 
> For now it seems at least a good idea to allow a user to disable write
> combining if they see this until we can figure it out.

Well, that's kind of amazing, given 3c004b4f7eab239e switched us /to/
using ioremap_wc() for the exact same reason.  I'm not against letting
the user force one way or the other if it helps, though it sure would be
nice to know why.

Anyway,

Acked-By: Peter Jones <pjo...@redhat.com>

Bartlomiej, do you want to handle this in your devel tree?

-- 
  Peter

Re: [PATCH] efifb: allow user to disable write combined mapping.

[Peter Jones]

On Tue, Jul 18, 2017 at 04:09:09PM +1000, Dave Airlie wrote:
> This patch allows the user to disable write combined mapping
> of the efifb framebuffer console using an nowc option.
> 
> A customer noticed major slowdowns while logging to the console
> with write combining enabled, on other tasks running on the same
> CPU. (10x or greater slow down on all other cores on the same CPU
> as is doing the logging).
> 
> I reproduced this on a machine with dual CPUs.
> Intel(R) Xeon(R) CPU E5-2609 v3 @ 1.90GHz (6 core)
> 
> I wrote a test that just mmaps the pci bar and writes to it in
> a loop, while this was running in the background one a single
> core with (taskset -c 1), building a kernel up to init/version.o
> (taskset -c 8) went from 13s to 133s or so. I've yet to explain
> why this occurs or what is going wrong I haven't managed to find
> a perf command that in any way gives insight into this.
> 
> 11,885,070,715  instructions  #1.39  insns per cycle
> vs
> 12,082,592,342  instructions  #0.13  insns per cycle
> 
> is the only thing I've spotted of interest, I've tried at least:
> dTLB-stores,dTLB-store-misses,L1-dcache-stores,LLC-store,LLC-store-misses,LLC-load-misses,LLC-loads,\mem-loads,mem-stores,iTLB-loads,iTLB-load-misses,cache-references,cache-misses
> 
> For now it seems at least a good idea to allow a user to disable write
> combining if they see this until we can figure it out.

Well, that's kind of amazing, given 3c004b4f7eab239e switched us /to/
using ioremap_wc() for the exact same reason.  I'm not against letting
the user force one way or the other if it helps, though it sure would be
nice to know why.

Anyway,

Acked-By: Peter Jones 

Bartlomiej, do you want to handle this in your devel tree?

-- 
  Peter

Re: [PATCH 08/29] efi: Allow drivers to reserve boot services forever

[Peter Jones]

On Tue, Jan 03, 2017 at 06:48:43PM -0800, Dan Williams wrote:
> On Fri, Sep 9, 2016 at 8:18 AM, Matt Fleming <m...@codeblueprint.co.uk> wrote:
> > Today, it is not possible for drivers to reserve EFI boot services for
> > access after efi_free_boot_services() has been called on x86. For
> > ARM/arm64 it can be done simply by calling memblock_reserve().
> >
> > Having this ability for all three architectures is desirable for a
> > couple of reasons,
> >
> >   1) It saves drivers copying data out of those regions
> >   2) kexec reboot can now make use of things like ESRT
> >
> > Instead of using the standard memblock_reserve() which is insufficient
> > to reserve the region on x86 (see efi_reserve_boot_services()), a new
> > API is introduced in this patch; efi_mem_reserve().
> >
> > efi.memmap now always represents which EFI memory regions are
> > available. On x86 the EFI boot services regions that have not been
> > reserved via efi_mem_reserve() will be removed from efi.memmap during
> > efi_free_boot_services().
> >
> > This has implications for kexec, since it is not possible for a newly
> > kexec'd kernel to access the same boot services regions that the
> > initial boot kernel had access to unless they are reserved by every
> > kexec kernel in the chain.
> >
> > Tested-by: Dave Young <dyo...@redhat.com> [kexec/kdump]
> > Tested-by: Ard Biesheuvel <ard.biesheu...@linaro.org> [arm]
> > Acked-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
> > Cc: Leif Lindholm <leif.lindh...@linaro.org>
> > Cc: Peter Jones <pjo...@redhat.com>
> > Cc: Borislav Petkov <b...@alien8.de>
> > Cc: Mark Rutland <mark.rutl...@arm.com>
> > Signed-off-by: Matt Fleming <m...@codeblueprint.co.uk>
> 
> This commit appears to cause a boot regression between v4.8 and v4.9.

Can you verify that the memory map looks reasonable?  This looks similar
(but not identical) to the traceback I was hitting on some ThinkPads
with ESRT last month, so there's some chance, if it is caused by bad
memory map entries, it may be fixed by 1cb209f63 on efi/next .

> 
> BUG: unable to handle kernel paging request at 8830281bf1c8
> IP: [] __next_mem_range_rev+0x13a/0x1d6
> PGD 3193067 PUD 3196067 PTE 8030281bf060
> Oops:  1 SMP DEBUG_PAGEALLOC
> Modules linked in:
> CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.9.0+ #2
> task: 82011540 task.stack: 8200
> RIP: 0010:[] []
> __next_mem_range_rev+0x13a/0x1d6
> RSP: :82003dd8 EFLAGS: 00010202
> RAX: 8830281bf1e0 RBX: 82003e60 RCX: 82167490
> RDX:  RSI:  RDI: 00184000
> RBP: 82003e18 R08: 821674b0 R09: 008f
> R10: 008f R11: 82011cf0 R12: 0004
> R13: 00304000 R14:  R15: 0001
> FS: () GS:8817e080() knlGS:
> CS: 0010 DS:  ES:  CR0: 80050033
> CR2: 8830281bf1c8 CR3: 0200a000 CR4: 007406f0
> DR0:  DR1:  DR2: 
> DR3:  DR6: fffe0ff0 DR7: 0400
> PKRU: 
> Stack:
> ea001700 82003e50 
> 10304000 82003e58 0180
> ffc0
> 82003e98 81a21395 82003e58 
> Call Trace:
> [] memblock_find_in_range_node+0x93/0x13a
> [] memblock_alloc_range_nid+0x1b/0x3e
> [] __memblock_alloc_base+0x15/0x17
> [] memblock_alloc_base+0x12/0x2e
> [] memblock_alloc+0xb/0xd
> [] efi_free_boot_services+0x46/0x180
> [] start_kernel+0x4a1/0x4cc
> [] ? set_init_arg+0x55/0x55
> [] ? early_idt_handler_array+0x120/0x120
> [] x86_64_start_reservations+0x2a/0x2c
> [] x86_64_start_kernel+0x14c/0x16f
> Code: 18 44 89 38 41 8d 44 24 ff 49 c1 e1 20 4c 09 c8 48 89 03 e9 a0
> 00 00 00 4d 63 d1 4c 89 d0 48 c1 e0 05 49 03 40 18 45 85 c9 74 28 <48>
> 8b 50 e8 48 03 50 e0 49 83 cb ff 4d 3b 10 73 03 4c 8b 18 49 ^M
> RIP [] __next_mem_range_rev+0x13a/0x1d6
> 
> I also see that Petr may have run into it as well [1]? Petr is this
> the same signature you are seeing? Can you post a boot log with
> "efi=debug" on the kernel command line?
> 
> It also fails on 4.10-rc2.  However, if I revert the following commits
> it boots fine:
> 
> 4bc9f92e64c8 x86/efi-bgrt: Use efi_mem_reserve() to avoid copying image data
> 8e80632fb23f efi/esrt: Use efi_mem_reserve() and avoid a kmalloc()
> 816e76129ed5 efi: Allow drivers to reserve boot services forever
> 
> [1]: https://lkml.org/lkml/2016/12/21/197

-- 
Peter

Re: [PATCH 08/29] efi: Allow drivers to reserve boot services forever

[Peter Jones]

On Tue, Jan 03, 2017 at 06:48:43PM -0800, Dan Williams wrote:
> On Fri, Sep 9, 2016 at 8:18 AM, Matt Fleming  wrote:
> > Today, it is not possible for drivers to reserve EFI boot services for
> > access after efi_free_boot_services() has been called on x86. For
> > ARM/arm64 it can be done simply by calling memblock_reserve().
> >
> > Having this ability for all three architectures is desirable for a
> > couple of reasons,
> >
> >   1) It saves drivers copying data out of those regions
> >   2) kexec reboot can now make use of things like ESRT
> >
> > Instead of using the standard memblock_reserve() which is insufficient
> > to reserve the region on x86 (see efi_reserve_boot_services()), a new
> > API is introduced in this patch; efi_mem_reserve().
> >
> > efi.memmap now always represents which EFI memory regions are
> > available. On x86 the EFI boot services regions that have not been
> > reserved via efi_mem_reserve() will be removed from efi.memmap during
> > efi_free_boot_services().
> >
> > This has implications for kexec, since it is not possible for a newly
> > kexec'd kernel to access the same boot services regions that the
> > initial boot kernel had access to unless they are reserved by every
> > kexec kernel in the chain.
> >
> > Tested-by: Dave Young  [kexec/kdump]
> > Tested-by: Ard Biesheuvel  [arm]
> > Acked-by: Ard Biesheuvel 
> > Cc: Leif Lindholm 
> > Cc: Peter Jones 
> > Cc: Borislav Petkov 
> > Cc: Mark Rutland 
> > Signed-off-by: Matt Fleming 
> 
> This commit appears to cause a boot regression between v4.8 and v4.9.

Can you verify that the memory map looks reasonable?  This looks similar
(but not identical) to the traceback I was hitting on some ThinkPads
with ESRT last month, so there's some chance, if it is caused by bad
memory map entries, it may be fixed by 1cb209f63 on efi/next .

> 
> BUG: unable to handle kernel paging request at 8830281bf1c8
> IP: [] __next_mem_range_rev+0x13a/0x1d6
> PGD 3193067 PUD 3196067 PTE 8030281bf060
> Oops:  1 SMP DEBUG_PAGEALLOC
> Modules linked in:
> CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.9.0+ #2
> task: 82011540 task.stack: 8200
> RIP: 0010:[] []
> __next_mem_range_rev+0x13a/0x1d6
> RSP: :82003dd8 EFLAGS: 00010202
> RAX: 8830281bf1e0 RBX: 82003e60 RCX: 82167490
> RDX:  RSI:  RDI: 00184000
> RBP: 82003e18 R08: 821674b0 R09: 008f
> R10: 008f R11: 82011cf0 R12: 0004
> R13: 00304000 R14:  R15: 0001
> FS: () GS:8817e080() knlGS:
> CS: 0010 DS:  ES:  CR0: 80050033
> CR2: 8830281bf1c8 CR3: 0200a000 CR4: 007406f0
> DR0:  DR1:  DR2: 
> DR3:  DR6: fffe0ff0 DR7: 0400
> PKRU: 
> Stack:
> ea001700 82003e50 
> 10304000 82003e58 0180
> ffc0
> 82003e98 81a21395 82003e58 
> Call Trace:
> [] memblock_find_in_range_node+0x93/0x13a
> [] memblock_alloc_range_nid+0x1b/0x3e
> [] __memblock_alloc_base+0x15/0x17
> [] memblock_alloc_base+0x12/0x2e
> [] memblock_alloc+0xb/0xd
> [] efi_free_boot_services+0x46/0x180
> [] start_kernel+0x4a1/0x4cc
> [] ? set_init_arg+0x55/0x55
> [] ? early_idt_handler_array+0x120/0x120
> [] x86_64_start_reservations+0x2a/0x2c
> [] x86_64_start_kernel+0x14c/0x16f
> Code: 18 44 89 38 41 8d 44 24 ff 49 c1 e1 20 4c 09 c8 48 89 03 e9 a0
> 00 00 00 4d 63 d1 4c 89 d0 48 c1 e0 05 49 03 40 18 45 85 c9 74 28 <48>
> 8b 50 e8 48 03 50 e0 49 83 cb ff 4d 3b 10 73 03 4c 8b 18 49 ^M
> RIP [] __next_mem_range_rev+0x13a/0x1d6
> 
> I also see that Petr may have run into it as well [1]? Petr is this
> the same signature you are seeing? Can you post a boot log with
> "efi=debug" on the kernel command line?
> 
> It also fails on 4.10-rc2.  However, if I revert the following commits
> it boots fine:
> 
> 4bc9f92e64c8 x86/efi-bgrt: Use efi_mem_reserve() to avoid copying image data
> 8e80632fb23f efi/esrt: Use efi_mem_reserve() and avoid a kmalloc()
> 816e76129ed5 efi: Allow drivers to reserve boot services forever
> 
> [1]: https://lkml.org/lkml/2016/12/21/197

-- 
Peter

Re: [PATCH 9/9] MODSIGN: Allow the "db" UEFI variable to be suppressed

[Peter Jones]

On Mon, Nov 21, 2016 at 08:06:44PM +0100, Ard Biesheuvel wrote:
> On 21 November 2016 at 20:05, Peter Jones <pjo...@redhat.com> wrote:
> > On Mon, Nov 21, 2016 at 04:42:45PM +, Ard Biesheuvel wrote:
> >> On 21 November 2016 at 16:26, Josh Boyer <jwbo...@fedoraproject.org> wrote:
> >> > On Mon, Nov 21, 2016 at 11:18 AM, Ard Biesheuvel
> >> > <ard.biesheu...@linaro.org> wrote:
> >> >> On 16 November 2016 at 18:11, David Howells <dhowe...@redhat.com> wrote:
> >> >>> From: Josh Boyer <jwbo...@fedoraproject.org>
> >> >>>
> >> >>> If a user tells shim to not use the certs/hashes in the UEFI db 
> >> >>> variable
> >> >>> for verification purposes, shim will set a UEFI variable called
> >> >>> MokIgnoreDB.  Have the uefi import code look for this and ignore the db
> >> >>> variable if it is found.
> >> >>>
> >> >>
> >> >> Similar concern as in the previous patch: it appears to me that you
> >> >> can DoS a machine by setting MokIgnoreDB if, e.g., its modules are
> >> >> signed against a cert that resides in db, and shim/mokmanager are not
> >> >> being used.
> >> >
> >> > If shim/mokmanager aren't used, then you can't actually modify
> >> > MokIgnoreDB.  Again, it requires physical access and a reboot into
> >> > mokmanager to actually take effect.
> >> >
> >>
> >> This does the trick as well
> >>
> >> printf "\x07\x00\x00\x00\x01" >
> >> /sys/firmware/efi/efivars/MokIgnoreDB-605dab50-e046-4300-abb6-3dd810dd8b23
> >
> > So that really means two things.  First, kernel should only honor any of
> > the Mok* variables if they're Boot Services-only variables.  Second, to
> > avoid the DoS, shim should create them all as Boot Services-only the
> > first time it boots.  That'll prevent them from being created post-boot.
> >
> 
> All of that assumes you are using shim and mokmanager in the first place.

No, it doesn't.  If you're not using shim, there's no DoS problem,
because what would you be DoSing?  And likewise, if you're not using
Secure Boot at all, you have no guarantee of anything about your boot
environment, starting with the idea that the boot loader isn't hostile.
If you're not using Secure Boot, a hostile pre-boot driver could easily
add DB entries just as easily as MokList entries, or any other
variables.

The fact that keys can be injected is true with or without this patch,
though it does make it easier.  But making a boot loader that injects
keys into the kernel's built-in keyring isn't actually very difficult.

If you're not using firmware enforced SB and shim, you do not have
security against this.

-- 
Peter

Re: [PATCH 9/9] MODSIGN: Allow the "db" UEFI variable to be suppressed

[Peter Jones]

On Mon, Nov 21, 2016 at 08:06:44PM +0100, Ard Biesheuvel wrote:
> On 21 November 2016 at 20:05, Peter Jones  wrote:
> > On Mon, Nov 21, 2016 at 04:42:45PM +, Ard Biesheuvel wrote:
> >> On 21 November 2016 at 16:26, Josh Boyer  wrote:
> >> > On Mon, Nov 21, 2016 at 11:18 AM, Ard Biesheuvel
> >> >  wrote:
> >> >> On 16 November 2016 at 18:11, David Howells  wrote:
> >> >>> From: Josh Boyer 
> >> >>>
> >> >>> If a user tells shim to not use the certs/hashes in the UEFI db 
> >> >>> variable
> >> >>> for verification purposes, shim will set a UEFI variable called
> >> >>> MokIgnoreDB.  Have the uefi import code look for this and ignore the db
> >> >>> variable if it is found.
> >> >>>
> >> >>
> >> >> Similar concern as in the previous patch: it appears to me that you
> >> >> can DoS a machine by setting MokIgnoreDB if, e.g., its modules are
> >> >> signed against a cert that resides in db, and shim/mokmanager are not
> >> >> being used.
> >> >
> >> > If shim/mokmanager aren't used, then you can't actually modify
> >> > MokIgnoreDB.  Again, it requires physical access and a reboot into
> >> > mokmanager to actually take effect.
> >> >
> >>
> >> This does the trick as well
> >>
> >> printf "\x07\x00\x00\x00\x01" >
> >> /sys/firmware/efi/efivars/MokIgnoreDB-605dab50-e046-4300-abb6-3dd810dd8b23
> >
> > So that really means two things.  First, kernel should only honor any of
> > the Mok* variables if they're Boot Services-only variables.  Second, to
> > avoid the DoS, shim should create them all as Boot Services-only the
> > first time it boots.  That'll prevent them from being created post-boot.
> >
> 
> All of that assumes you are using shim and mokmanager in the first place.

No, it doesn't.  If you're not using shim, there's no DoS problem,
because what would you be DoSing?  And likewise, if you're not using
Secure Boot at all, you have no guarantee of anything about your boot
environment, starting with the idea that the boot loader isn't hostile.
If you're not using Secure Boot, a hostile pre-boot driver could easily
add DB entries just as easily as MokList entries, or any other
variables.

The fact that keys can be injected is true with or without this patch,
though it does make it easier.  But making a boot loader that injects
keys into the kernel's built-in keyring isn't actually very difficult.

If you're not using firmware enforced SB and shim, you do not have
security against this.

-- 
Peter

Re: [PATCH 9/9] MODSIGN: Allow the "db" UEFI variable to be suppressed

[Peter Jones]

On Mon, Nov 21, 2016 at 04:42:45PM +, Ard Biesheuvel wrote:
> On 21 November 2016 at 16:26, Josh Boyer  wrote:
> > On Mon, Nov 21, 2016 at 11:18 AM, Ard Biesheuvel
> >  wrote:
> >> On 16 November 2016 at 18:11, David Howells  wrote:
> >>> From: Josh Boyer 
> >>>
> >>> If a user tells shim to not use the certs/hashes in the UEFI db variable
> >>> for verification purposes, shim will set a UEFI variable called
> >>> MokIgnoreDB.  Have the uefi import code look for this and ignore the db
> >>> variable if it is found.
> >>>
> >>
> >> Similar concern as in the previous patch: it appears to me that you
> >> can DoS a machine by setting MokIgnoreDB if, e.g., its modules are
> >> signed against a cert that resides in db, and shim/mokmanager are not
> >> being used.
> >
> > If shim/mokmanager aren't used, then you can't actually modify
> > MokIgnoreDB.  Again, it requires physical access and a reboot into
> > mokmanager to actually take effect.
> >
> 
> This does the trick as well
> 
> printf "\x07\x00\x00\x00\x01" >
> /sys/firmware/efi/efivars/MokIgnoreDB-605dab50-e046-4300-abb6-3dd810dd8b23

So that really means two things.  First, kernel should only honor any of
the Mok* variables if they're Boot Services-only variables.  Second, to
avoid the DoS, shim should create them all as Boot Services-only the
first time it boots.  That'll prevent them from being created post-boot.

-- 
Peter

Re: [PATCH 9/9] MODSIGN: Allow the "db" UEFI variable to be suppressed

[Peter Jones]

On Mon, Nov 21, 2016 at 04:42:45PM +, Ard Biesheuvel wrote:
> On 21 November 2016 at 16:26, Josh Boyer  wrote:
> > On Mon, Nov 21, 2016 at 11:18 AM, Ard Biesheuvel
> >  wrote:
> >> On 16 November 2016 at 18:11, David Howells  wrote:
> >>> From: Josh Boyer 
> >>>
> >>> If a user tells shim to not use the certs/hashes in the UEFI db variable
> >>> for verification purposes, shim will set a UEFI variable called
> >>> MokIgnoreDB.  Have the uefi import code look for this and ignore the db
> >>> variable if it is found.
> >>>
> >>
> >> Similar concern as in the previous patch: it appears to me that you
> >> can DoS a machine by setting MokIgnoreDB if, e.g., its modules are
> >> signed against a cert that resides in db, and shim/mokmanager are not
> >> being used.
> >
> > If shim/mokmanager aren't used, then you can't actually modify
> > MokIgnoreDB.  Again, it requires physical access and a reboot into
> > mokmanager to actually take effect.
> >
> 
> This does the trick as well
> 
> printf "\x07\x00\x00\x00\x01" >
> /sys/firmware/efi/efivars/MokIgnoreDB-605dab50-e046-4300-abb6-3dd810dd8b23

So that really means two things.  First, kernel should only honor any of
the Mok* variables if they're Boot Services-only variables.  Second, to
avoid the DoS, shim should create them all as Boot Services-only the
first time it boots.  That'll prevent them from being created post-boot.

-- 
Peter

[tip:efi/core] efifb: Show framebuffer layout as device attributes

[tip-bot for Peter Jones]

Commit-ID:  753375a881caa01112b7cec2c796749154e0bb23
Gitweb: http://git.kernel.org/tip/753375a881caa01112b7cec2c796749154e0bb23
Author: Peter Jones <pjo...@redhat.com>
AuthorDate: Tue, 18 Oct 2016 15:33:17 +0100
Committer:  Ingo Molnar <mi...@kernel.org>
CommitDate: Tue, 18 Oct 2016 17:11:19 +0200

efifb: Show framebuffer layout as device attributes

Userland sometimes needs to know what the framebuffer configuration was
when the firmware was running. This enables us to render localized
status strings during firmware updates using the data from the ACPI BGRT
table and the protocol described at the url below:

  
https://msdn.microsoft.com/en-us/windows/hardware/drivers/bringup/boot-screen-components

This patch also fixes up efifb's printk() usage to use pr_warn() /
pr_info() / pr_err() instead.

Tested-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
Signed-off-by: Peter Jones <pjo...@redhat.com>
Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
Cc: Linus Torvalds <torva...@linux-foundation.org>
Cc: Peter Zijlstra <pet...@infradead.org>
Cc: Thomas Gleixner <t...@linutronix.de>
Cc: linux-...@vger.kernel.org
Link: http://lkml.kernel.org/r/20161018143318.15673-8-m...@codeblueprint.co.uk
Signed-off-by: Ingo Molnar <mi...@kernel.org>
---
 drivers/video/fbdev/efifb.c | 59 +++--
 1 file changed, 46 insertions(+), 13 deletions(-)

diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index 37a37c4..8c4dc1e 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -118,6 +118,31 @@ static inline bool fb_base_is_valid(void)
return false;
 }
 
+#define efifb_attr_decl(name, fmt) \
+static ssize_t name##_show(struct device *dev, \
+  struct device_attribute *attr,   \
+  char *buf)   \
+{  \
+   return sprintf(buf, fmt "\n", (screen_info.lfb_##name));\
+}  \
+static DEVICE_ATTR_RO(name)
+
+efifb_attr_decl(base, "0x%x");
+efifb_attr_decl(linelength, "%u");
+efifb_attr_decl(height, "%u");
+efifb_attr_decl(width, "%u");
+efifb_attr_decl(depth, "%u");
+
+static struct attribute *efifb_attrs[] = {
+   _attr_base.attr,
+   _attr_linelength.attr,
+   _attr_width.attr,
+   _attr_height.attr,
+   _attr_depth.attr,
+   NULL
+};
+ATTRIBUTE_GROUPS(efifb);
+
 static int efifb_probe(struct platform_device *dev)
 {
struct fb_info *info;
@@ -205,14 +230,13 @@ static int efifb_probe(struct platform_device *dev)
} else {
/* We cannot make this fatal. Sometimes this comes from magic
   spaces our resource handlers simply don't know about */
-   printk(KERN_WARNING
-  "efifb: cannot reserve video memory at 0x%lx\n",
+   pr_warn("efifb: cannot reserve video memory at 0x%lx\n",
efifb_fix.smem_start);
}
 
info = framebuffer_alloc(sizeof(u32) * 16, >dev);
if (!info) {
-   printk(KERN_ERR "efifb: cannot allocate framebuffer\n");
+   pr_err("efifb: cannot allocate framebuffer\n");
err = -ENOMEM;
goto err_release_mem;
}
@@ -230,16 +254,15 @@ static int efifb_probe(struct platform_device *dev)
 
info->screen_base = ioremap_wc(efifb_fix.smem_start, 
efifb_fix.smem_len);
if (!info->screen_base) {
-   printk(KERN_ERR "efifb: abort, cannot ioremap video memory "
-   "0x%x @ 0x%lx\n",
+   pr_err("efifb: abort, cannot ioremap video memory 0x%x @ 
0x%lx\n",
efifb_fix.smem_len, efifb_fix.smem_start);
err = -EIO;
goto err_release_fb;
}
 
-   printk(KERN_INFO "efifb: framebuffer at 0x%lx, using %dk, total %dk\n",
+   pr_info("efifb: framebuffer at 0x%lx, using %dk, total %dk\n",
   efifb_fix.smem_start, size_remap/1024, size_total/1024);
-   printk(KERN_INFO "efifb: mode is %dx%dx%d, linelength=%d, pages=%d\n",
+   pr_info("efifb: mode is %dx%dx%d, linelength=%d, pages=%d\n",
   efifb_defined.xres, efifb_defined.yres,
   efifb_defined.bits_per_pixel, efifb_fix.line_length,
   screen_info.pages);
@@ -247,7 +270,7 @@ static int efifb_probe(struct platform_device *dev)
efifb_defined.xres_virtual = efifb_defined.xres;
efifb_defined.yres_virtual = efifb_fix.smem_len /
   

[tip:efi/core] efifb: Show framebuffer layout as device attributes

[tip-bot for Peter Jones]

Commit-ID:  753375a881caa01112b7cec2c796749154e0bb23
Gitweb: http://git.kernel.org/tip/753375a881caa01112b7cec2c796749154e0bb23
Author: Peter Jones 
AuthorDate: Tue, 18 Oct 2016 15:33:17 +0100
Committer:  Ingo Molnar 
CommitDate: Tue, 18 Oct 2016 17:11:19 +0200

efifb: Show framebuffer layout as device attributes

Userland sometimes needs to know what the framebuffer configuration was
when the firmware was running. This enables us to render localized
status strings during firmware updates using the data from the ACPI BGRT
table and the protocol described at the url below:

  
https://msdn.microsoft.com/en-us/windows/hardware/drivers/bringup/boot-screen-components

This patch also fixes up efifb's printk() usage to use pr_warn() /
pr_info() / pr_err() instead.

Tested-by: Ard Biesheuvel 
Signed-off-by: Peter Jones 
Signed-off-by: Ard Biesheuvel 
Cc: Linus Torvalds 
Cc: Peter Zijlstra 
Cc: Thomas Gleixner 
Cc: linux-...@vger.kernel.org
Link: http://lkml.kernel.org/r/20161018143318.15673-8-m...@codeblueprint.co.uk
Signed-off-by: Ingo Molnar 
---
 drivers/video/fbdev/efifb.c | 59 +++--
 1 file changed, 46 insertions(+), 13 deletions(-)

diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index 37a37c4..8c4dc1e 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -118,6 +118,31 @@ static inline bool fb_base_is_valid(void)
return false;
 }
 
+#define efifb_attr_decl(name, fmt) \
+static ssize_t name##_show(struct device *dev, \
+  struct device_attribute *attr,   \
+  char *buf)   \
+{  \
+   return sprintf(buf, fmt "\n", (screen_info.lfb_##name));\
+}  \
+static DEVICE_ATTR_RO(name)
+
+efifb_attr_decl(base, "0x%x");
+efifb_attr_decl(linelength, "%u");
+efifb_attr_decl(height, "%u");
+efifb_attr_decl(width, "%u");
+efifb_attr_decl(depth, "%u");
+
+static struct attribute *efifb_attrs[] = {
+   _attr_base.attr,
+   _attr_linelength.attr,
+   _attr_width.attr,
+   _attr_height.attr,
+   _attr_depth.attr,
+   NULL
+};
+ATTRIBUTE_GROUPS(efifb);
+
 static int efifb_probe(struct platform_device *dev)
 {
struct fb_info *info;
@@ -205,14 +230,13 @@ static int efifb_probe(struct platform_device *dev)
} else {
/* We cannot make this fatal. Sometimes this comes from magic
   spaces our resource handlers simply don't know about */
-   printk(KERN_WARNING
-  "efifb: cannot reserve video memory at 0x%lx\n",
+   pr_warn("efifb: cannot reserve video memory at 0x%lx\n",
efifb_fix.smem_start);
}
 
info = framebuffer_alloc(sizeof(u32) * 16, >dev);
if (!info) {
-   printk(KERN_ERR "efifb: cannot allocate framebuffer\n");
+   pr_err("efifb: cannot allocate framebuffer\n");
err = -ENOMEM;
goto err_release_mem;
}
@@ -230,16 +254,15 @@ static int efifb_probe(struct platform_device *dev)
 
info->screen_base = ioremap_wc(efifb_fix.smem_start, 
efifb_fix.smem_len);
if (!info->screen_base) {
-   printk(KERN_ERR "efifb: abort, cannot ioremap video memory "
-   "0x%x @ 0x%lx\n",
+   pr_err("efifb: abort, cannot ioremap video memory 0x%x @ 
0x%lx\n",
efifb_fix.smem_len, efifb_fix.smem_start);
err = -EIO;
goto err_release_fb;
}
 
-   printk(KERN_INFO "efifb: framebuffer at 0x%lx, using %dk, total %dk\n",
+   pr_info("efifb: framebuffer at 0x%lx, using %dk, total %dk\n",
   efifb_fix.smem_start, size_remap/1024, size_total/1024);
-   printk(KERN_INFO "efifb: mode is %dx%dx%d, linelength=%d, pages=%d\n",
+   pr_info("efifb: mode is %dx%dx%d, linelength=%d, pages=%d\n",
   efifb_defined.xres, efifb_defined.yres,
   efifb_defined.bits_per_pixel, efifb_fix.line_length,
   screen_info.pages);
@@ -247,7 +270,7 @@ static int efifb_probe(struct platform_device *dev)
efifb_defined.xres_virtual = efifb_defined.xres;
efifb_defined.yres_virtual = efifb_fix.smem_len /
efifb_fix.line_length;
-   printk(KERN_INFO "efifb: scrolling: redraw\n");
+   pr_info("efifb: scrolling: redraw\n");
efifb_defined.yres_virtual = efifb_defined.yres;
 
/* some dummy values fo

[tip:efi/core] efi: Document #define FOO_PROTOCOL_GUID layout

[tip-bot for Peter Jones]

Commit-ID:  54fd11fee59e7d05287bc4eebccc8ec9742f2745
Gitweb: http://git.kernel.org/tip/54fd11fee59e7d05287bc4eebccc8ec9742f2745
Author: Peter Jones <pjo...@redhat.com>
AuthorDate: Sat, 25 Jun 2016 08:20:25 +0100
Committer:  Ingo Molnar <mi...@kernel.org>
CommitDate: Mon, 27 Jun 2016 13:06:55 +0200

efi: Document #define FOO_PROTOCOL_GUID layout

Add a comment documenting why EFI GUIDs are laid out like they are.

Ideally I'd like to change all the ", " to "," too, but right now the
format is such that checkpatch won't complain with new ones, and staring
at checkpatch didn't get me anywhere towards making that work.

Signed-off-by: Peter Jones <pjo...@redhat.com>
Signed-off-by: Matt Fleming <m...@codeblueprint.co.uk>
Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
Cc: Joe Perches <j...@perches.com>
Cc: Linus Torvalds <torva...@linux-foundation.org>
Cc: Peter Zijlstra <pet...@infradead.org>
Cc: Thomas Gleixner <t...@linutronix.de>
Cc: linux-...@vger.kernel.org
Link: 
http://lkml.kernel.org/r/1466839230-12781-3-git-send-email-m...@codeblueprint.co.uk
Signed-off-by: Ingo Molnar <mi...@kernel.org>
---
 include/linux/efi.h | 17 -
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/include/linux/efi.h b/include/linux/efi.h
index f196dd0..0300969 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -536,7 +536,22 @@ typedef efi_status_t efi_query_variable_store_t(u32 
attributes,
 void efi_native_runtime_setup(void);
 
 /*
- *  EFI Configuration Table and GUID definitions
+ * EFI Configuration Table and GUID definitions
+ *
+ * These should be formatted roughly like the ones in the UEFI SPEC has
+ * them.  It makes them easier to grep for, and they look the same when
+ * you're staring at them.  Here's the guide:
+ *
+ * GUID: 12345678-1234-1234-1234-123456789012
+ * Spec:
+ *  #define EFI_SOME_PROTOCOL_GUID \
+ *{0x12345678,0x1234,0x1234,\
+ *  {0x12,0x34,0x12,0x34,0x56,0x78,0x90,0x12}}
+ * Here:
+ * #define SOME_PROTOCOL_GUID \
+ * EFI_GUID(0x12345678, 0x1234,  0x1234, \
+ *  0x12, 0x34, 0x12, 0x34, 0x56, 0x78, 0x90, 0x12)
+ *  ^ tab   ^tab^ space
  */
 #define NULL_GUID \
EFI_GUID(0x, 0x, 0x, \

[tip:efi/core] efi: Document #define FOO_PROTOCOL_GUID layout

[tip-bot for Peter Jones]

Commit-ID:  54fd11fee59e7d05287bc4eebccc8ec9742f2745
Gitweb: http://git.kernel.org/tip/54fd11fee59e7d05287bc4eebccc8ec9742f2745
Author: Peter Jones 
AuthorDate: Sat, 25 Jun 2016 08:20:25 +0100
Committer:  Ingo Molnar 
CommitDate: Mon, 27 Jun 2016 13:06:55 +0200

efi: Document #define FOO_PROTOCOL_GUID layout

Add a comment documenting why EFI GUIDs are laid out like they are.

Ideally I'd like to change all the ", " to "," too, but right now the
format is such that checkpatch won't complain with new ones, and staring
at checkpatch didn't get me anywhere towards making that work.

Signed-off-by: Peter Jones 
Signed-off-by: Matt Fleming 
Cc: Ard Biesheuvel 
Cc: Joe Perches 
Cc: Linus Torvalds 
Cc: Peter Zijlstra 
Cc: Thomas Gleixner 
Cc: linux-...@vger.kernel.org
Link: 
http://lkml.kernel.org/r/1466839230-12781-3-git-send-email-m...@codeblueprint.co.uk
Signed-off-by: Ingo Molnar 
---
 include/linux/efi.h | 17 -
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/include/linux/efi.h b/include/linux/efi.h
index f196dd0..0300969 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -536,7 +536,22 @@ typedef efi_status_t efi_query_variable_store_t(u32 
attributes,
 void efi_native_runtime_setup(void);
 
 /*
- *  EFI Configuration Table and GUID definitions
+ * EFI Configuration Table and GUID definitions
+ *
+ * These should be formatted roughly like the ones in the UEFI SPEC has
+ * them.  It makes them easier to grep for, and they look the same when
+ * you're staring at them.  Here's the guide:
+ *
+ * GUID: 12345678-1234-1234-1234-123456789012
+ * Spec:
+ *  #define EFI_SOME_PROTOCOL_GUID \
+ *{0x12345678,0x1234,0x1234,\
+ *  {0x12,0x34,0x12,0x34,0x56,0x78,0x90,0x12}}
+ * Here:
+ * #define SOME_PROTOCOL_GUID \
+ * EFI_GUID(0x12345678, 0x1234,  0x1234, \
+ *  0x12, 0x34, 0x12, 0x34, 0x56, 0x78, 0x90, 0x12)
+ *  ^ tab   ^tab^ space
  */
 #define NULL_GUID \
EFI_GUID(0x, 0x, 0x, \

Re: [PATCH] ACPI: don't show an error when we're not in charge of PCIe hotplug.

[Peter Jones]

(Sorry for the slow response - it's deadline time over here.)

On Thu, Jun 16, 2016 at 04:56:57PM +0200, Rafael J. Wysocki wrote:
> On Thu, Jun 16, 2016 at 2:12 AM, Rafael J. Wysocki <raf...@kernel.org> wrote:
> > On Thu, Jun 16, 2016 at 12:15 AM, Peter Jones <pjo...@redhat.com> wrote:
> >> Right now when booting, on many laptops the firmware manages the PCIe
> >> bus.  As a result, when we call the _OSC ACPI method, it returns an
> >> error code.  Unfortunately the errors are not very articulate.
> >
> > What exactly do you mean here?
> >
> >>  As a result, we show:
> >>
> >> ACPI: PCI Root Bridge [PCI0] (domain  [bus 00-fe])
> >> acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments 
> >> MSI]
> >> \_SB_.PCI0 (33DB4D5B-1FF7-401C-9657-7441C03DD766): _OSC invalid UUID
> >> _OSC request data: 1 1f 0
> >
> > So _OSC told us that the UUID was invalid, didn't it?
> 
> BTW, the above messages are KERN_DEBUG, so at least in theory they
> shouldn't be visible in production runs.
> 
> Maybe the bug to fix is that they show up when they aren't supposed to?

No - the workflow that I am really trying to remedy is this:

 1) S3 resume sometimes isn't working on some laptop you've got.
 2) start looking at debug messages
 3) this shows an error, so it looks like it's probably the problem
 4) go fishing for red herring
 5) if you happen to know who maintains the DSDT for the platform in
question, eventually work out that this is working as intended and
the bug is someplace else.
5b) if you don't know that person, eventually work out that it /might/
 be someplace else...

So the idea was to make it look more like an indication of status, and
less like an error that's causing unrelated problems.

When I talked to Mario at Dell (Cc'd), it wasn't clear to us that
there's a way to distinguish the between the UUID being
invalid/malformed, being merely unsupported, or being supported in some
configurations but not the current one.  In this particular DSDT, the
machine doesn't support the OS controlling any of this if USB-C /
thunderbolt are enabled.  The DSDT is clearly written with the belief
that you have to completely disable the handling for that UUID in this
case, and googling for this looks like it's not the only one written
with that belief.

Reading the spec (v6.1, sections 6.2.11.3 and 6.2.11.4), it seems
plausible that you can express this instead by handling the UUID but
choosing each individual query/status bit in the way that accomplishes
the OS doing nothing with the response.  So it may well be that that's
just more code that vendors have thought wasn't necessary (or wasn't
correct for some reason.)

Mario, want to jump in on your thinking here?

-- 
  Peter

Re: [PATCH] ACPI: don't show an error when we're not in charge of PCIe hotplug.

[Peter Jones]

(Sorry for the slow response - it's deadline time over here.)

On Thu, Jun 16, 2016 at 04:56:57PM +0200, Rafael J. Wysocki wrote:
> On Thu, Jun 16, 2016 at 2:12 AM, Rafael J. Wysocki  wrote:
> > On Thu, Jun 16, 2016 at 12:15 AM, Peter Jones  wrote:
> >> Right now when booting, on many laptops the firmware manages the PCIe
> >> bus.  As a result, when we call the _OSC ACPI method, it returns an
> >> error code.  Unfortunately the errors are not very articulate.
> >
> > What exactly do you mean here?
> >
> >>  As a result, we show:
> >>
> >> ACPI: PCI Root Bridge [PCI0] (domain  [bus 00-fe])
> >> acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments 
> >> MSI]
> >> \_SB_.PCI0 (33DB4D5B-1FF7-401C-9657-7441C03DD766): _OSC invalid UUID
> >> _OSC request data: 1 1f 0
> >
> > So _OSC told us that the UUID was invalid, didn't it?
> 
> BTW, the above messages are KERN_DEBUG, so at least in theory they
> shouldn't be visible in production runs.
> 
> Maybe the bug to fix is that they show up when they aren't supposed to?

No - the workflow that I am really trying to remedy is this:

 1) S3 resume sometimes isn't working on some laptop you've got.
 2) start looking at debug messages
 3) this shows an error, so it looks like it's probably the problem
 4) go fishing for red herring
 5) if you happen to know who maintains the DSDT for the platform in
question, eventually work out that this is working as intended and
the bug is someplace else.
5b) if you don't know that person, eventually work out that it /might/
 be someplace else...

So the idea was to make it look more like an indication of status, and
less like an error that's causing unrelated problems.

When I talked to Mario at Dell (Cc'd), it wasn't clear to us that
there's a way to distinguish the between the UUID being
invalid/malformed, being merely unsupported, or being supported in some
configurations but not the current one.  In this particular DSDT, the
machine doesn't support the OS controlling any of this if USB-C /
thunderbolt are enabled.  The DSDT is clearly written with the belief
that you have to completely disable the handling for that UUID in this
case, and googling for this looks like it's not the only one written
with that belief.

Reading the spec (v6.1, sections 6.2.11.3 and 6.2.11.4), it seems
plausible that you can express this instead by handling the UUID but
choosing each individual query/status bit in the way that accomplishes
the OS doing nothing with the response.  So it may well be that that's
just more code that vendors have thought wasn't necessary (or wasn't
correct for some reason.)

Mario, want to jump in on your thinking here?

-- 
  Peter

[PATCH] ACPI: don't show an error when we're not in charge of PCIe hotplug.

[Peter Jones]

Right now when booting, on many laptops the firmware manages the PCIe
bus.  As a result, when we call the _OSC ACPI method, it returns an
error code.  Unfortunately the errors are not very articulate.  As a
result, we show:

ACPI: PCI Root Bridge [PCI0] (domain  [bus 00-fe])
acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI]
\_SB_.PCI0 (33DB4D5B-1FF7-401C-9657-7441C03DD766): _OSC invalid UUID
_OSC request data: 1 1f 0
acpi PNP0A08:00: _OSC failed (AE_ERROR); disabling ASPM

But we did get the capabilities mask back; the firmware is merely
managing this itself.  So we really should not be showing the user a
message that looks like the firmware is broken, since it is working just
fine.

This patch supresses the error message when we're calling _OSC with the
PCIe host bridge UUID, and replaces it with a relatively innocuous
looking message that you can find if you're looking for it.
---
 drivers/acpi/bus.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 262ca31..be8a91b 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -215,6 +215,8 @@ acpi_status acpi_str_to_uuid(char *str, u8 *uuid)
 }
 EXPORT_SYMBOL_GPL(acpi_str_to_uuid);
 
+static u8 pci_osc_uuid_str[] = "33DB4D5B-1FF7-401C-9657-7441C03DD766";
+
 acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context)
 {
acpi_status status;
@@ -267,9 +269,13 @@ acpi_status acpi_run_osc(acpi_handle handle, struct 
acpi_osc_context *context)
if (errors & OSC_REQUEST_ERROR)
acpi_print_osc_error(handle, context,
"_OSC request failed");
-   if (errors & OSC_INVALID_UUID_ERROR)
-   acpi_print_osc_error(handle, context,
-   "_OSC invalid UUID");
+   if (errors & OSC_INVALID_UUID_ERROR) {
+   if (!strcasecmp(context->uuid_str, pci_osc_uuid_str))
+   pr_info("PCI PME managed by ACPI.\n");
+   else
+   acpi_print_osc_error(handle, context,
+"_OSC invalid UUID");
+   }
if (errors & OSC_INVALID_REVISION_ERROR)
acpi_print_osc_error(handle, context,
"_OSC invalid revision");
-- 
2.7.4

[PATCH] ACPI: don't show an error when we're not in charge of PCIe hotplug.

[Peter Jones]

Right now when booting, on many laptops the firmware manages the PCIe
bus.  As a result, when we call the _OSC ACPI method, it returns an
error code.  Unfortunately the errors are not very articulate.  As a
result, we show:

ACPI: PCI Root Bridge [PCI0] (domain  [bus 00-fe])
acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI]
\_SB_.PCI0 (33DB4D5B-1FF7-401C-9657-7441C03DD766): _OSC invalid UUID
_OSC request data: 1 1f 0
acpi PNP0A08:00: _OSC failed (AE_ERROR); disabling ASPM

But we did get the capabilities mask back; the firmware is merely
managing this itself.  So we really should not be showing the user a
message that looks like the firmware is broken, since it is working just
fine.

This patch supresses the error message when we're calling _OSC with the
PCIe host bridge UUID, and replaces it with a relatively innocuous
looking message that you can find if you're looking for it.
---
 drivers/acpi/bus.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 262ca31..be8a91b 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -215,6 +215,8 @@ acpi_status acpi_str_to_uuid(char *str, u8 *uuid)
 }
 EXPORT_SYMBOL_GPL(acpi_str_to_uuid);
 
+static u8 pci_osc_uuid_str[] = "33DB4D5B-1FF7-401C-9657-7441C03DD766";
+
 acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context)
 {
acpi_status status;
@@ -267,9 +269,13 @@ acpi_status acpi_run_osc(acpi_handle handle, struct 
acpi_osc_context *context)
if (errors & OSC_REQUEST_ERROR)
acpi_print_osc_error(handle, context,
"_OSC request failed");
-   if (errors & OSC_INVALID_UUID_ERROR)
-   acpi_print_osc_error(handle, context,
-   "_OSC invalid UUID");
+   if (errors & OSC_INVALID_UUID_ERROR) {
+   if (!strcasecmp(context->uuid_str, pci_osc_uuid_str))
+   pr_info("PCI PME managed by ACPI.\n");
+   else
+   acpi_print_osc_error(handle, context,
+"_OSC invalid UUID");
+   }
if (errors & OSC_INVALID_REVISION_ERROR)
acpi_print_osc_error(handle, context,
"_OSC invalid revision");
-- 
2.7.4

Re: [PATCH] [efifb] Fix 16 color palette entry calculation

[Peter Jones]

On Tue, Jun 07, 2016 at 03:45:43PM +0200, Max Staudt wrote:
> When using efifb with a 16-bit (5:6:5) visual, fbcon's text is rendered
> in the wrong colors - e.g. text gray (#aa) is rendered as green
> (#50bc50) and neighboring pixels have slightly different values
> (such as #50bc78).
> 
> The reason is that fbcon loads its 16 color palette through
> efifb_setcolreg(), which in turn calculates a 32-bit value to write
> into memory for each palette index.
> Until now, this code could only handle 8-bit visuals and didn't mask
> overlapping values when ORing them.
> 
> With this patch, fbcon displays the correct colors when a qemu VM is
> booted in 16-bit mode (in GRUB: "set gfxpayload=800x600x16").
> 
> Signed-off-by: Max Staudt <msta...@suse.de>
> ---
>  drivers/video/fbdev/efifb.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index 924bad4..37a37c4 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -50,9 +50,9 @@ static int efifb_setcolreg(unsigned regno, unsigned red, 
> unsigned green,
>   return 1;
>  
>   if (regno < 16) {
> - red   >>= 8;
> - green >>= 8;
> - blue  >>= 8;
> + red   >>= 16 - info->var.red.length;
> + green >>= 16 - info->var.green.length;
> + blue  >>= 16 - info->var.blue.length;
>   ((u32 *)(info->pseudo_palette))[regno] =
>   (red   << info->var.red.offset)   |
>   (green << info->var.green.offset) |
> -- 
> 2.6.6

Looks right to me.

Acked-By: Peter Jones <pjo...@redhat.com>

-- 
  Peter

Re: [PATCH] [efifb] Fix 16 color palette entry calculation

[Peter Jones]

On Tue, Jun 07, 2016 at 03:45:43PM +0200, Max Staudt wrote:
> When using efifb with a 16-bit (5:6:5) visual, fbcon's text is rendered
> in the wrong colors - e.g. text gray (#aa) is rendered as green
> (#50bc50) and neighboring pixels have slightly different values
> (such as #50bc78).
> 
> The reason is that fbcon loads its 16 color palette through
> efifb_setcolreg(), which in turn calculates a 32-bit value to write
> into memory for each palette index.
> Until now, this code could only handle 8-bit visuals and didn't mask
> overlapping values when ORing them.
> 
> With this patch, fbcon displays the correct colors when a qemu VM is
> booted in 16-bit mode (in GRUB: "set gfxpayload=800x600x16").
> 
> Signed-off-by: Max Staudt 
> ---
>  drivers/video/fbdev/efifb.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index 924bad4..37a37c4 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -50,9 +50,9 @@ static int efifb_setcolreg(unsigned regno, unsigned red, 
> unsigned green,
>   return 1;
>  
>   if (regno < 16) {
> - red   >>= 8;
> - green >>= 8;
> - blue  >>= 8;
> + red   >>= 16 - info->var.red.length;
> + green >>= 16 - info->var.green.length;
> + blue  >>= 16 - info->var.blue.length;
>   ((u32 *)(info->pseudo_palette))[regno] =
>   (red   << info->var.red.offset)   |
>   (green << info->var.green.offset) |
> -- 
> 2.6.6

Looks right to me.

Acked-By: Peter Jones 

-- 
  Peter

Re: [PATCH] efifb: Don't show the mapping VA

[Peter Jones]

On Wed, May 11, 2016 at 04:57:47PM -0700, Andy Lutomirski wrote:
> The framebuffer mapping virtual address leaks information about the
> kernel memory layout.  Stop logging it.
> 
> Cc: Peter Jones <pjo...@redhat.com>
> Cc: Jean-Christophe Plagniol-Villard <plagn...@jcrosoft.com>
> Cc: Tomi Valkeinen <tomi.valkei...@ti.com>
> Cc: linux-fb...@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Andy Lutomirski <l...@kernel.org>

In practice it also hasn't ever helped any actual debugging.

Signed-off-by: Peter Jones <pjo...@redhat.com>

> ---
>  drivers/video/fbdev/efifb.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index 95d293b7445a..3dcaf4e82885 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -247,10 +247,8 @@ static int efifb_probe(struct platform_device *dev)
>   goto err_release_fb;
>   }
>  
> - printk(KERN_INFO "efifb: framebuffer at 0x%lx, mapped to 0x%p, "
> -"using %dk, total %dk\n",
> -efifb_fix.smem_start, info->screen_base,
> -size_remap/1024, size_total/1024);
> + printk(KERN_INFO "efifb: framebuffer at 0x%lx, using %dk, total %dk\n",
> +efifb_fix.smem_start, size_remap/1024, size_total/1024);
>   printk(KERN_INFO "efifb: mode is %dx%dx%d, linelength=%d, pages=%d\n",
>  efifb_defined.xres, efifb_defined.yres,
>  efifb_defined.bits_per_pixel, efifb_fix.line_length,
> -- 
> 2.5.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
  Peter

Re: [PATCH] efifb: Don't show the mapping VA

[Peter Jones]

On Wed, May 11, 2016 at 04:57:47PM -0700, Andy Lutomirski wrote:
> The framebuffer mapping virtual address leaks information about the
> kernel memory layout.  Stop logging it.
> 
> Cc: Peter Jones 
> Cc: Jean-Christophe Plagniol-Villard 
> Cc: Tomi Valkeinen 
> Cc: linux-fb...@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Andy Lutomirski 

In practice it also hasn't ever helped any actual debugging.

Signed-off-by: Peter Jones 

> ---
>  drivers/video/fbdev/efifb.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index 95d293b7445a..3dcaf4e82885 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -247,10 +247,8 @@ static int efifb_probe(struct platform_device *dev)
>   goto err_release_fb;
>   }
>  
> - printk(KERN_INFO "efifb: framebuffer at 0x%lx, mapped to 0x%p, "
> -"using %dk, total %dk\n",
> -efifb_fix.smem_start, info->screen_base,
> -size_remap/1024, size_total/1024);
> + printk(KERN_INFO "efifb: framebuffer at 0x%lx, using %dk, total %dk\n",
> +efifb_fix.smem_start, size_remap/1024, size_total/1024);
>   printk(KERN_INFO "efifb: mode is %dx%dx%d, linelength=%d, pages=%d\n",
>  efifb_defined.xres, efifb_defined.yres,
>  efifb_defined.bits_per_pixel, efifb_fix.line_length,
> -- 
> 2.5.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
  Peter

[tip:efi/core] efivarfs: Make efivarfs_file_ioctl() static

[tip-bot for Peter Jones]

Commit-ID:  6c5450ef66816216e574885cf8d3ddb31ef77428
Gitweb: http://git.kernel.org/tip/6c5450ef66816216e574885cf8d3ddb31ef77428
Author: Peter Jones <pjo...@redhat.com>
AuthorDate: Fri, 6 May 2016 22:39:31 +0100
Committer:  Ingo Molnar <mi...@kernel.org>
CommitDate: Sat, 7 May 2016 07:06:13 +0200

efivarfs: Make efivarfs_file_ioctl() static

There are no callers except through the file_operations struct below
this, so it should be static like everything else here.

Signed-off-by: Peter Jones <pjo...@redhat.com>
Signed-off-by: Matt Fleming <m...@codeblueprint.co.uk>
Cc: Andy Lutomirski <l...@amacapital.net>
Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
Cc: Borislav Petkov <b...@alien8.de>
Cc: Brian Gerst <brge...@gmail.com>
Cc: Denys Vlasenko <dvlas...@redhat.com>
Cc: H. Peter Anvin <h...@zytor.com>
Cc: Linus Torvalds <torva...@linux-foundation.org>
Cc: Peter Zijlstra <pet...@infradead.org>
Cc: Thomas Gleixner <t...@linutronix.de>
Cc: linux-...@vger.kernel.org
Link: 
http://lkml.kernel.org/r/1462570771-13324-6-git-send-email-m...@codeblueprint.co.uk
Signed-off-by: Ingo Molnar <mi...@kernel.org>
---
 fs/efivarfs/file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
index d48e0d2..5f22e74 100644
--- a/fs/efivarfs/file.c
+++ b/fs/efivarfs/file.c
@@ -157,7 +157,7 @@ efivarfs_ioc_setxflags(struct file *file, void __user *arg)
return 0;
 }
 
-long
+static long
 efivarfs_file_ioctl(struct file *file, unsigned int cmd, unsigned long p)
 {
void __user *arg = (void __user *)p;

[tip:efi/core] efivarfs: Make efivarfs_file_ioctl() static

[tip-bot for Peter Jones]

Commit-ID:  6c5450ef66816216e574885cf8d3ddb31ef77428
Gitweb: http://git.kernel.org/tip/6c5450ef66816216e574885cf8d3ddb31ef77428
Author: Peter Jones 
AuthorDate: Fri, 6 May 2016 22:39:31 +0100
Committer:  Ingo Molnar 
CommitDate: Sat, 7 May 2016 07:06:13 +0200

efivarfs: Make efivarfs_file_ioctl() static

There are no callers except through the file_operations struct below
this, so it should be static like everything else here.

Signed-off-by: Peter Jones 
Signed-off-by: Matt Fleming 
Cc: Andy Lutomirski 
Cc: Ard Biesheuvel 
Cc: Borislav Petkov 
Cc: Brian Gerst 
Cc: Denys Vlasenko 
Cc: H. Peter Anvin 
Cc: Linus Torvalds 
Cc: Peter Zijlstra 
Cc: Thomas Gleixner 
Cc: linux-...@vger.kernel.org
Link: 
http://lkml.kernel.org/r/1462570771-13324-6-git-send-email-m...@codeblueprint.co.uk
Signed-off-by: Ingo Molnar 
---
 fs/efivarfs/file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
index d48e0d2..5f22e74 100644
--- a/fs/efivarfs/file.c
+++ b/fs/efivarfs/file.c
@@ -157,7 +157,7 @@ efivarfs_ioc_setxflags(struct file *file, void __user *arg)
return 0;
 }
 
-long
+static long
 efivarfs_file_ioctl(struct file *file, unsigned int cmd, unsigned long p)
 {
void __user *arg = (void __user *)p;

Re: [PATCH v2] x86:sysfb_efi:efifb_set_system: fix miss valid address range in later BARs

[Peter Jones]

On Sun, May 01, 2016 at 12:21:05AM +0800, Wang YanQing wrote:
> We can't just break out when meet start is equal to zero,
> this will cause we miss valid address range in later BARs.
> 
> On the other hand, it isn't enough to test start only
> for below situation:
> 0(start) <= lfb_base < end
> 
> Note: this patch also add a trivial optimization,
>   break out after we find the address range
>   is valid without test later BARs.
> 
> Signed-off-by: Wang YanQing <udkni...@gmail.com>

This looks correct to me:

Reviewed-By: Peter Jones <pjo...@redhat.com>

Cc'ing Matt Fleming, since this should probably go through the EFI tree.

> ---
>  Due to the BUG this patch fix, I can't use video=efifb:
>  boot parameter to get efifb on my new ThinkPad E550 for
>  my old linux system hard disk with 3.10 kernel. In 3.10, 
>  efifb is the only choice due to DRM/I915 in it doesn't 
>  support the GPU.
> 
>  Changes:
>  v1-v2:
>  1: Do a trivial code optimization.
> 
>  arch/x86/kernel/sysfb_efi.c | 14 --
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/sysfb_efi.c b/arch/x86/kernel/sysfb_efi.c
> index b285d4e..5da924b 100644
> --- a/arch/x86/kernel/sysfb_efi.c
> +++ b/arch/x86/kernel/sysfb_efi.c
> @@ -106,14 +106,24 @@ static int __init efifb_set_system(const struct 
> dmi_system_id *id)
>   continue;
>   for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
>   resource_size_t start, end;
> + unsigned long flags;
> +
> + flags = pci_resource_flags(dev, i);
> + if (!(flags & IORESOURCE_MEM))
> + continue;
> +
> + if (flags & IORESOURCE_UNSET)
> + continue;
> +
> + if (pci_resource_len(dev, i) == 0)
> + continue;
>  
>   start = pci_resource_start(dev, i);
> - if (start == 0)
> - break;
>   end = pci_resource_end(dev, i);
>   if (screen_info.lfb_base >= start &&
>   screen_info.lfb_base < end) {
>   found_bar = 1;
> + break;
>   }
>   }
>   }
> -- 
> 1.8.5.6.2.g3d8a54e.dirty

-- 
  Peter

Re: [PATCH v2] x86:sysfb_efi:efifb_set_system: fix miss valid address range in later BARs

[Peter Jones]

On Sun, May 01, 2016 at 12:21:05AM +0800, Wang YanQing wrote:
> We can't just break out when meet start is equal to zero,
> this will cause we miss valid address range in later BARs.
> 
> On the other hand, it isn't enough to test start only
> for below situation:
> 0(start) <= lfb_base < end
> 
> Note: this patch also add a trivial optimization,
>   break out after we find the address range
>   is valid without test later BARs.
> 
> Signed-off-by: Wang YanQing 

This looks correct to me:

Reviewed-By: Peter Jones 

Cc'ing Matt Fleming, since this should probably go through the EFI tree.

> ---
>  Due to the BUG this patch fix, I can't use video=efifb:
>  boot parameter to get efifb on my new ThinkPad E550 for
>  my old linux system hard disk with 3.10 kernel. In 3.10, 
>  efifb is the only choice due to DRM/I915 in it doesn't 
>  support the GPU.
> 
>  Changes:
>  v1-v2:
>  1: Do a trivial code optimization.
> 
>  arch/x86/kernel/sysfb_efi.c | 14 --
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/sysfb_efi.c b/arch/x86/kernel/sysfb_efi.c
> index b285d4e..5da924b 100644
> --- a/arch/x86/kernel/sysfb_efi.c
> +++ b/arch/x86/kernel/sysfb_efi.c
> @@ -106,14 +106,24 @@ static int __init efifb_set_system(const struct 
> dmi_system_id *id)
>   continue;
>   for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
>   resource_size_t start, end;
> + unsigned long flags;
> +
> + flags = pci_resource_flags(dev, i);
> + if (!(flags & IORESOURCE_MEM))
> + continue;
> +
> + if (flags & IORESOURCE_UNSET)
> + continue;
> +
> + if (pci_resource_len(dev, i) == 0)
> + continue;
>  
>   start = pci_resource_start(dev, i);
> - if (start == 0)
> - break;
>   end = pci_resource_end(dev, i);
>   if (screen_info.lfb_base >= start &&
>   screen_info.lfb_base < end) {
>   found_bar = 1;
> + break;
>   }
>   }
>   }
> -- 
> 1.8.5.6.2.g3d8a54e.dirty

-- 
  Peter

Re: [PATCH] lib: Always NUL terminate ucs2_as_utf8

[Peter Jones]

On Thu, Apr 21, 2016 at 01:18:27PM +0100, Matt Fleming wrote:
> ( Good Lord, I hate doing string manipulation in C )

(yep)

> 
> On Wed, 20 Apr, at 03:25:32PM, Laszlo Ersek wrote:
> > 
> > So, "len" does not include the room for the terminating NUL-byte here.
> > When "len" is passed to ucs2_as_utf8(), with the proposed patch applied,
> > a NUL byte will be produced in "name", but it will be at the price of a
> > genuine character from the input variable name.
> 
> Right, and this is a problem because we're trying to keep the names
> consistent between efivarfs and the EFI variable data. Force
> NUL-terminating the string is wrong, because if you have no room for
> the NUL the caller should check for that. Sadly none do.
> 
> On the flip-side, passing around non-NUL terminated strings is just
> begging for these kinds of issues to come up.
> 
> The fact is that the callers of ucs2_as_utf8() are passing it the
> wrong 'len' argument. We want a NUL-terminated utf8 string and we're
> passing a NUL-terminated ucs2 string. We should tell ucs2_as_utf8() it
> has enough room to copy the NUL.
> 
> Wouldn't this work (minus the return value checking)?

I agree with your analysis, and your patch looks plausible.

-- 
  Peter

Re: [PATCH] lib: Always NUL terminate ucs2_as_utf8

[Peter Jones]

On Thu, Apr 21, 2016 at 01:18:27PM +0100, Matt Fleming wrote:
> ( Good Lord, I hate doing string manipulation in C )

(yep)

> 
> On Wed, 20 Apr, at 03:25:32PM, Laszlo Ersek wrote:
> > 
> > So, "len" does not include the room for the terminating NUL-byte here.
> > When "len" is passed to ucs2_as_utf8(), with the proposed patch applied,
> > a NUL byte will be produced in "name", but it will be at the price of a
> > genuine character from the input variable name.
> 
> Right, and this is a problem because we're trying to keep the names
> consistent between efivarfs and the EFI variable data. Force
> NUL-terminating the string is wrong, because if you have no room for
> the NUL the caller should check for that. Sadly none do.
> 
> On the flip-side, passing around non-NUL terminated strings is just
> begging for these kinds of issues to come up.
> 
> The fact is that the callers of ucs2_as_utf8() are passing it the
> wrong 'len' argument. We want a NUL-terminated utf8 string and we're
> passing a NUL-terminated ucs2 string. We should tell ucs2_as_utf8() it
> has enough room to copy the NUL.
> 
> Wouldn't this work (minus the return value checking)?

I agree with your analysis, and your patch looks plausible.

-- 
  Peter

[tip:efi/core] efi: Reformat GUID tables to follow the format in UEFI spec

[tip-bot for Peter Jones]

Commit-ID:  662b1d890c593673964758fe5b6f22067bffba7a
Gitweb: http://git.kernel.org/tip/662b1d890c593673964758fe5b6f22067bffba7a
Author: Peter Jones <pjo...@redhat.com>
AuthorDate: Wed, 17 Feb 2016 12:35:54 +
Committer:  Ingo Molnar <mi...@kernel.org>
CommitDate: Mon, 22 Feb 2016 08:26:25 +0100

efi: Reformat GUID tables to follow the format in UEFI spec

This makes it much easier to hunt for typos in the GUID definitions.

It also makes checkpatch complain less about efi.h GUID additions, so
that if you add another one with the same style, checkpatch won't
complain about it.

Signed-off-by: Peter Jones <pjo...@redhat.com>
Signed-off-by: Matt Fleming <m...@codeblueprint.co.uk>
Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
Cc: Linus Torvalds <torva...@linux-foundation.org>
Cc: Peter Zijlstra <pet...@infradead.org>
Cc: Thomas Gleixner <t...@linutronix.de>
Cc: linux-...@vger.kernel.org
Link: 
http://lkml.kernel.org/r/1455712566-16727-2-git-send-email-m...@codeblueprint.co.uk
Signed-off-by: Ingo Molnar <mi...@kernel.org>
---
 include/linux/efi.h | 63 +++--
 1 file changed, 42 insertions(+), 21 deletions(-)

diff --git a/include/linux/efi.h b/include/linux/efi.h
index 1acd723..42be9c9 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -536,67 +536,88 @@ void efi_native_runtime_setup(void);
  *  EFI Configuration Table and GUID definitions
  */
 #define NULL_GUID \
-EFI_GUID(  0x, 0x, 0x, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 
0x00, 0x00 )
+   EFI_GUID(0x, 0x, 0x, \
+0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00)
 
 #define MPS_TABLE_GUID\
-EFI_GUID(  0xeb9d2d2f, 0x2d88, 0x11d3, 0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 
0xc1, 0x4d )
+   EFI_GUID(0xeb9d2d2f, 0x2d88, 0x11d3, \
+0x9a, 0x16, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)
 
 #define ACPI_TABLE_GUID\
-EFI_GUID(  0xeb9d2d30, 0x2d88, 0x11d3, 0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 
0xc1, 0x4d )
+   EFI_GUID(0xeb9d2d30, 0x2d88, 0x11d3, \
+0x9a, 0x16, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)
 
 #define ACPI_20_TABLE_GUID\
-EFI_GUID(  0x8868e871, 0xe4f1, 0x11d3, 0xbc, 0x22, 0x0, 0x80, 0xc7, 0x3c, 
0x88, 0x81 )
+   EFI_GUID(0x8868e871, 0xe4f1, 0x11d3, \
+0xbc, 0x22, 0x00, 0x80, 0xc7, 0x3c, 0x88, 0x81)
 
 #define SMBIOS_TABLE_GUID\
-EFI_GUID(  0xeb9d2d31, 0x2d88, 0x11d3, 0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 
0xc1, 0x4d )
+   EFI_GUID(0xeb9d2d31, 0x2d88, 0x11d3, \
+0x9a, 0x16, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)
 
 #define SMBIOS3_TABLE_GUID\
-EFI_GUID(  0xf2fd1544, 0x9794, 0x4a2c, 0x99, 0x2e, 0xe5, 0xbb, 0xcf, 0x20, 
0xe3, 0x94 )
+   EFI_GUID(0xf2fd1544, 0x9794, 0x4a2c, \
+0x99, 0x2e, 0xe5, 0xbb, 0xcf, 0x20, 0xe3, 0x94)
 
 #define SAL_SYSTEM_TABLE_GUID\
-EFI_GUID(  0xeb9d2d32, 0x2d88, 0x11d3, 0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 
0xc1, 0x4d )
+   EFI_GUID(0xeb9d2d32, 0x2d88, 0x11d3, \
+0x9a, 0x16, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)
 
 #define HCDP_TABLE_GUID\
-EFI_GUID(  0xf951938d, 0x620b, 0x42ef, 0x82, 0x79, 0xa8, 0x4b, 0x79, 0x61, 
0x78, 0x98 )
+   EFI_GUID(0xf951938d, 0x620b, 0x42ef, \
+0x82, 0x79, 0xa8, 0x4b, 0x79, 0x61, 0x78, 0x98)
 
 #define UGA_IO_PROTOCOL_GUID \
-EFI_GUID(  0x61a4d49e, 0x6f68, 0x4f1b, 0xb9, 0x22, 0xa8, 0x6e, 0xed, 0xb, 
0x7, 0xa2 )
+   EFI_GUID(0x61a4d49e, 0x6f68, 0x4f1b, \
+0xb9, 0x22, 0xa8, 0x6e, 0xed, 0x0b, 0x07, 0xa2)
 
 #define EFI_GLOBAL_VARIABLE_GUID \
-EFI_GUID(  0x8be4df61, 0x93ca, 0x11d2, 0xaa, 0x0d, 0x00, 0xe0, 0x98, 0x03, 
0x2b, 0x8c )
+   EFI_GUID(0x8be4df61, 0x93ca, 0x11d2, \
+0xaa, 0x0d, 0x00, 0xe0, 0x98, 0x03, 0x2b, 0x8c)
 
 #define UV_SYSTEM_TABLE_GUID \
-EFI_GUID(  0x3b13a7d4, 0x633e, 0x11dd, 0x93, 0xec, 0xda, 0x25, 0x56, 0xd8, 
0x95, 0x93 )
+   EFI_GUID(0x3b13a7d4, 0x633e, 0x11dd, \
+0x93, 0xec, 0xda, 0x25, 0x56, 0xd8, 0x95, 0x93)
 
 #define LINUX_EFI_CRASH_GUID \
-EFI_GUID(  0xcfc8fc79, 0xbe2e, 0x4ddc, 0x97, 0xf0, 0x9f, 0x98, 0xbf, 0xe2, 
0x98, 0xa0 )
+   EFI_GUID(0xcfc8fc79, 0xbe2e, 0x4ddc, \
+0x97, 0xf0, 0x9f, 0x98, 0xbf, 0xe2, 0x98, 0xa0)
 
 #define LOADED_IMAGE_PROTOCOL_GUID \
-EFI_GUID(  0x5b1b31a1, 0x9562, 0x11d2, 0x8e, 0x3f, 0x00, 0xa0, 0xc9, 0x69, 
0x72, 0x3b )
+   EFI_GUID(0x5b1b31a1, 0x9562, 0x11d2, \
+0x8e, 0x3f, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b)
 
 #define EFI_GRAPHICS_OUTPUT_PROTOCOL_GUID \
-EFI_GUID(  0x9042a9de, 0x23dc, 0x4a38, 0x96, 0xfb, 0x7a, 0xde, 0xd0, 0x80, 
0x51, 0x6a )
+   EFI_GUID(0x9042a9de, 0x23dc, 0x4a38, \
+0x96, 0xfb, 0x7a, 0xde, 0xd0, 0x80, 0x51, 0x6a)
 
 #define EFI_UGA_PROTOCOL_GUID \
-EFI_GUID(  0x982c298b, 0xf4fa, 0x41cb, 0xb8, 0x38, 0x77, 0xaa, 0x68, 0x8f, 
0xb8, 0x39 )
+   EFI_GU

[tip:efi/core] efi: Reformat GUID tables to follow the format in UEFI spec

[tip-bot for Peter Jones]

Commit-ID:  662b1d890c593673964758fe5b6f22067bffba7a
Gitweb: http://git.kernel.org/tip/662b1d890c593673964758fe5b6f22067bffba7a
Author: Peter Jones 
AuthorDate: Wed, 17 Feb 2016 12:35:54 +
Committer:  Ingo Molnar 
CommitDate: Mon, 22 Feb 2016 08:26:25 +0100

efi: Reformat GUID tables to follow the format in UEFI spec

This makes it much easier to hunt for typos in the GUID definitions.

It also makes checkpatch complain less about efi.h GUID additions, so
that if you add another one with the same style, checkpatch won't
complain about it.

Signed-off-by: Peter Jones 
Signed-off-by: Matt Fleming 
Cc: Ard Biesheuvel 
Cc: Linus Torvalds 
Cc: Peter Zijlstra 
Cc: Thomas Gleixner 
Cc: linux-...@vger.kernel.org
Link: 
http://lkml.kernel.org/r/1455712566-16727-2-git-send-email-m...@codeblueprint.co.uk
Signed-off-by: Ingo Molnar 
---
 include/linux/efi.h | 63 +++--
 1 file changed, 42 insertions(+), 21 deletions(-)

diff --git a/include/linux/efi.h b/include/linux/efi.h
index 1acd723..42be9c9 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -536,67 +536,88 @@ void efi_native_runtime_setup(void);
  *  EFI Configuration Table and GUID definitions
  */
 #define NULL_GUID \
-EFI_GUID(  0x, 0x, 0x, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 
0x00, 0x00 )
+   EFI_GUID(0x, 0x, 0x, \
+0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00)
 
 #define MPS_TABLE_GUID\
-EFI_GUID(  0xeb9d2d2f, 0x2d88, 0x11d3, 0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 
0xc1, 0x4d )
+   EFI_GUID(0xeb9d2d2f, 0x2d88, 0x11d3, \
+0x9a, 0x16, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)
 
 #define ACPI_TABLE_GUID\
-EFI_GUID(  0xeb9d2d30, 0x2d88, 0x11d3, 0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 
0xc1, 0x4d )
+   EFI_GUID(0xeb9d2d30, 0x2d88, 0x11d3, \
+0x9a, 0x16, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)
 
 #define ACPI_20_TABLE_GUID\
-EFI_GUID(  0x8868e871, 0xe4f1, 0x11d3, 0xbc, 0x22, 0x0, 0x80, 0xc7, 0x3c, 
0x88, 0x81 )
+   EFI_GUID(0x8868e871, 0xe4f1, 0x11d3, \
+0xbc, 0x22, 0x00, 0x80, 0xc7, 0x3c, 0x88, 0x81)
 
 #define SMBIOS_TABLE_GUID\
-EFI_GUID(  0xeb9d2d31, 0x2d88, 0x11d3, 0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 
0xc1, 0x4d )
+   EFI_GUID(0xeb9d2d31, 0x2d88, 0x11d3, \
+0x9a, 0x16, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)
 
 #define SMBIOS3_TABLE_GUID\
-EFI_GUID(  0xf2fd1544, 0x9794, 0x4a2c, 0x99, 0x2e, 0xe5, 0xbb, 0xcf, 0x20, 
0xe3, 0x94 )
+   EFI_GUID(0xf2fd1544, 0x9794, 0x4a2c, \
+0x99, 0x2e, 0xe5, 0xbb, 0xcf, 0x20, 0xe3, 0x94)
 
 #define SAL_SYSTEM_TABLE_GUID\
-EFI_GUID(  0xeb9d2d32, 0x2d88, 0x11d3, 0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 
0xc1, 0x4d )
+   EFI_GUID(0xeb9d2d32, 0x2d88, 0x11d3, \
+0x9a, 0x16, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)
 
 #define HCDP_TABLE_GUID\
-EFI_GUID(  0xf951938d, 0x620b, 0x42ef, 0x82, 0x79, 0xa8, 0x4b, 0x79, 0x61, 
0x78, 0x98 )
+   EFI_GUID(0xf951938d, 0x620b, 0x42ef, \
+0x82, 0x79, 0xa8, 0x4b, 0x79, 0x61, 0x78, 0x98)
 
 #define UGA_IO_PROTOCOL_GUID \
-EFI_GUID(  0x61a4d49e, 0x6f68, 0x4f1b, 0xb9, 0x22, 0xa8, 0x6e, 0xed, 0xb, 
0x7, 0xa2 )
+   EFI_GUID(0x61a4d49e, 0x6f68, 0x4f1b, \
+0xb9, 0x22, 0xa8, 0x6e, 0xed, 0x0b, 0x07, 0xa2)
 
 #define EFI_GLOBAL_VARIABLE_GUID \
-EFI_GUID(  0x8be4df61, 0x93ca, 0x11d2, 0xaa, 0x0d, 0x00, 0xe0, 0x98, 0x03, 
0x2b, 0x8c )
+   EFI_GUID(0x8be4df61, 0x93ca, 0x11d2, \
+0xaa, 0x0d, 0x00, 0xe0, 0x98, 0x03, 0x2b, 0x8c)
 
 #define UV_SYSTEM_TABLE_GUID \
-EFI_GUID(  0x3b13a7d4, 0x633e, 0x11dd, 0x93, 0xec, 0xda, 0x25, 0x56, 0xd8, 
0x95, 0x93 )
+   EFI_GUID(0x3b13a7d4, 0x633e, 0x11dd, \
+0x93, 0xec, 0xda, 0x25, 0x56, 0xd8, 0x95, 0x93)
 
 #define LINUX_EFI_CRASH_GUID \
-EFI_GUID(  0xcfc8fc79, 0xbe2e, 0x4ddc, 0x97, 0xf0, 0x9f, 0x98, 0xbf, 0xe2, 
0x98, 0xa0 )
+   EFI_GUID(0xcfc8fc79, 0xbe2e, 0x4ddc, \
+0x97, 0xf0, 0x9f, 0x98, 0xbf, 0xe2, 0x98, 0xa0)
 
 #define LOADED_IMAGE_PROTOCOL_GUID \
-EFI_GUID(  0x5b1b31a1, 0x9562, 0x11d2, 0x8e, 0x3f, 0x00, 0xa0, 0xc9, 0x69, 
0x72, 0x3b )
+   EFI_GUID(0x5b1b31a1, 0x9562, 0x11d2, \
+0x8e, 0x3f, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b)
 
 #define EFI_GRAPHICS_OUTPUT_PROTOCOL_GUID \
-EFI_GUID(  0x9042a9de, 0x23dc, 0x4a38, 0x96, 0xfb, 0x7a, 0xde, 0xd0, 0x80, 
0x51, 0x6a )
+   EFI_GUID(0x9042a9de, 0x23dc, 0x4a38, \
+0x96, 0xfb, 0x7a, 0xde, 0xd0, 0x80, 0x51, 0x6a)
 
 #define EFI_UGA_PROTOCOL_GUID \
-EFI_GUID(  0x982c298b, 0xf4fa, 0x41cb, 0xb8, 0x38, 0x77, 0xaa, 0x68, 0x8f, 
0xb8, 0x39 )
+   EFI_GUID(0x982c298b, 0xf4fa, 0x41cb, \
+0xb8, 0x38, 0x77, 0xaa, 0x68, 0x8f, 0xb8, 0x39)
 
 #define EFI_PCI_IO_PROTOCOL_GUID \
-EFI_GUID(  0x4cf5b200, 0x68b8, 0x4ca5, 0x9e, 0xec, 0xb2, 0x3e, 0x3f, 0x50, 
0x2, 0x9a )
+   EFI_GUID(0x4cf5b200

Re: [PATCH RFC 1/1] x86: fix bad memory access in fb_is_primary_device()

[Peter Jones]

On Tue, Feb 16, 2016 at 01:49:18PM +, Matt Fleming wrote:
> [ Including Peter, the efifb maintainer. Original email is here,
> 
> http://marc.info/?l=linux-kernel=145552936131335=2
> 
>   I've snipped some of the quoted text ]
> 
> On Tue, 16 Feb, at 08:55:22AM, Ingo Molnar wrote:
> > 
> > (I've Cc:-ed the EFI-FB and FB gents. Mail quoted below.)
> > 
> > * Alexander Popov  wrote:
> > 
> > > Currently the code in fb_is_primary_device() contains to_pci_dev() macro
> > > which is applied to dev from struct fb_info. In some cases this causes
> > > bad memory access when fb_is_primary_device() handles fb_info of efifb.
> > > The reason is that fb dev of efifb is embedded into struct platform_device
> > > but not into struct pci_dev.
> > > 
> > > We can fix this by checking fb dev bus name in fb_is_primary_device().
> > > 
> > > It seems that this bug reveals some bigger problem with to_pci_dev(),
> > > to_platform_device() and others, which just do container_of() and
> > > don't check whether struct device is a part of the appropriate structure.
> > > Should we do something more about it?
> > > 
> > > KASan report:
> 
> [...]
> 
> > > 
> > > Signed-off-by: Alexander Popov 
> > > ---
> > >  arch/x86/video/fbdev.c | 9 +
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/arch/x86/video/fbdev.c b/arch/x86/video/fbdev.c
> > > index d5644bb..4999f78 100644
> > > --- a/arch/x86/video/fbdev.c
> > > +++ b/arch/x86/video/fbdev.c
> > > @@ -18,11 +18,12 @@ int fb_is_primary_device(struct fb_info *info)
> > >   struct pci_dev *default_device = vga_default_device();
> > >   struct resource *res = NULL;
> > >  
> > > - if (device)
> > > - pci_dev = to_pci_dev(device);
> > > -
> > > - if (!pci_dev)
> > > + if (!device || !device->bus ||
> > > + !device->bus->name || strcmp(device->bus->name, "pci")) {
> > >   return 0;
> > > + }
> > > +
> > > + pci_dev = to_pci_dev(device);
> > >  
> > >   if (default_device) {
> > >   if (pci_dev == default_device)
> > > -- 
> > > 1.9.1
> > > 
> 
> I wonder if this issue could explain some of the efifb issues we've
> seen reported on bugzilla.kernel.org in the past where switching from
> efifb to some other framebuffer device caused hangs during boot. I'm
> struggling to find the relevant bugzilla entries now, though.

It's possible it could, but I don't have them handy either.  I've also
wondered if some of them were due to bad data from the firmware - at
plugfests we've seen some cases where the actual video mode as measured
with a ruler is clearly not what the firmware claims it to be, so it's
entirely possible we're occasionally told a memory region that is not
what's actually mapped, or that's mapped but is only partially backed
by the actual frame buffer memory.

But aside from that diversion, I think Alexander has a legitimate
question about use of to_pci_dev().  If I ask the question: can we fix
this in efifb by making it live on a pci_dev, I have a couple of
fundamental problems:

1) technically it doesn't have to be a pci_dev at all (but, practically,
   so far it always is on PCI...)
2) From EFI, we can't necessarily pin it down to a single PCI device
   even if it is PCI.  Before we do EFI's ExitBootServices() call, we
   can try to find the PCI_IO handle our GOP instance is connected to,
   but not all firmware GOP drivers use that, so it doesn't always work.
   And even if it did, there can be more than one instance pointing to
   the same memory with different PCI devices - lots of laptops have
   this sort of thing.
3) Ignoring the EFI side and just focusing on PCI, if there's two
   devices configured that could do scanout, it can be mapped to one
   device's BAR but the other device be the actual device using it.  In
   this case either choice is probably wrong for something, and the
   things that have the information to resolve which one don't include
   efifb - they're the drivers we'll likely hand off to later.

So it's most likely right for efifb to be embedded in a platform_device
instead of a pci_dev.  Which leads back to Alexander's question - if it
isn't in a pci_dev, that means fb_is_primary_device() needs to not
assume it is.  So the patch appears correct, but so is the question -
should to_pci_dev() be checking this and returning NULL here?

-- 
Peter

Re: [PATCH RFC 1/1] x86: fix bad memory access in fb_is_primary_device()

[Peter Jones]

On Tue, Feb 16, 2016 at 01:49:18PM +, Matt Fleming wrote:
> [ Including Peter, the efifb maintainer. Original email is here,
> 
> http://marc.info/?l=linux-kernel=145552936131335=2
> 
>   I've snipped some of the quoted text ]
> 
> On Tue, 16 Feb, at 08:55:22AM, Ingo Molnar wrote:
> > 
> > (I've Cc:-ed the EFI-FB and FB gents. Mail quoted below.)
> > 
> > * Alexander Popov  wrote:
> > 
> > > Currently the code in fb_is_primary_device() contains to_pci_dev() macro
> > > which is applied to dev from struct fb_info. In some cases this causes
> > > bad memory access when fb_is_primary_device() handles fb_info of efifb.
> > > The reason is that fb dev of efifb is embedded into struct platform_device
> > > but not into struct pci_dev.
> > > 
> > > We can fix this by checking fb dev bus name in fb_is_primary_device().
> > > 
> > > It seems that this bug reveals some bigger problem with to_pci_dev(),
> > > to_platform_device() and others, which just do container_of() and
> > > don't check whether struct device is a part of the appropriate structure.
> > > Should we do something more about it?
> > > 
> > > KASan report:
> 
> [...]
> 
> > > 
> > > Signed-off-by: Alexander Popov 
> > > ---
> > >  arch/x86/video/fbdev.c | 9 +
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/arch/x86/video/fbdev.c b/arch/x86/video/fbdev.c
> > > index d5644bb..4999f78 100644
> > > --- a/arch/x86/video/fbdev.c
> > > +++ b/arch/x86/video/fbdev.c
> > > @@ -18,11 +18,12 @@ int fb_is_primary_device(struct fb_info *info)
> > >   struct pci_dev *default_device = vga_default_device();
> > >   struct resource *res = NULL;
> > >  
> > > - if (device)
> > > - pci_dev = to_pci_dev(device);
> > > -
> > > - if (!pci_dev)
> > > + if (!device || !device->bus ||
> > > + !device->bus->name || strcmp(device->bus->name, "pci")) {
> > >   return 0;
> > > + }
> > > +
> > > + pci_dev = to_pci_dev(device);
> > >  
> > >   if (default_device) {
> > >   if (pci_dev == default_device)
> > > -- 
> > > 1.9.1
> > > 
> 
> I wonder if this issue could explain some of the efifb issues we've
> seen reported on bugzilla.kernel.org in the past where switching from
> efifb to some other framebuffer device caused hangs during boot. I'm
> struggling to find the relevant bugzilla entries now, though.

It's possible it could, but I don't have them handy either.  I've also
wondered if some of them were due to bad data from the firmware - at
plugfests we've seen some cases where the actual video mode as measured
with a ruler is clearly not what the firmware claims it to be, so it's
entirely possible we're occasionally told a memory region that is not
what's actually mapped, or that's mapped but is only partially backed
by the actual frame buffer memory.

But aside from that diversion, I think Alexander has a legitimate
question about use of to_pci_dev().  If I ask the question: can we fix
this in efifb by making it live on a pci_dev, I have a couple of
fundamental problems:

1) technically it doesn't have to be a pci_dev at all (but, practically,
   so far it always is on PCI...)
2) From EFI, we can't necessarily pin it down to a single PCI device
   even if it is PCI.  Before we do EFI's ExitBootServices() call, we
   can try to find the PCI_IO handle our GOP instance is connected to,
   but not all firmware GOP drivers use that, so it doesn't always work.
   And even if it did, there can be more than one instance pointing to
   the same memory with different PCI devices - lots of laptops have
   this sort of thing.
3) Ignoring the EFI side and just focusing on PCI, if there's two
   devices configured that could do scanout, it can be mapped to one
   device's BAR but the other device be the actual device using it.  In
   this case either choice is probably wrong for something, and the
   things that have the information to resolve which one don't include
   efifb - they're the drivers we'll likely hand off to later.

So it's most likely right for efifb to be embedded in a platform_device
instead of a pci_dev.  Which leads back to Alexander's question - if it
isn't in a pci_dev, that means fb_is_primary_device() needs to not
assume it is.  So the patch appears correct, but so is the question -
should to_pci_dev() be checking this and returning NULL here?

-- 
Peter

[PATCH] Add support for EDD4 fields and types.

[Peter Jones]

Back in 2010, t13 EDD version 4 added a couple of storage types, some
host bridge types (that are pretty much all represented identically),
and a couple of fields on existing storage types.

This change makes the driver expose those to userland.

Signed-off-by: Peter Jones 
---
 drivers/firmware/edd.c   | 15 ++-
 include/uapi/linux/edd.h |  6 +-
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/edd.c b/drivers/firmware/edd.c
index e229576..5092b92 100644
--- a/drivers/firmware/edd.c
+++ b/drivers/firmware/edd.c
@@ -152,15 +152,15 @@ edd_show_host_bus(struct edd_device *edev, char *buf)
 info->params.interface_path.isa.base_address);
} else if (!strncmp(info->params.host_bus_type, "PCIX", 4) ||
   !strncmp(info->params.host_bus_type, "PCI", 3) ||
-  !strncmp(info->params.host_bus_type, "XPRS", 4)) {
+  !strncmp(info->params.host_bus_type, "XPRS", 4) ||
+  !strncmp(info->params.host_bus_type, "HTPT", 4)) {
p += scnprintf(p, left,
 "\t%02x:%02x.%d  channel: %u\n",
 info->params.interface_path.pci.bus,
 info->params.interface_path.pci.slot,
 info->params.interface_path.pci.function,
 info->params.interface_path.pci.channel);
-   } else if (!strncmp(info->params.host_bus_type, "IBND", 4) ||
-  !strncmp(info->params.host_bus_type, "HTPT", 4)) {
+   } else if (!strncmp(info->params.host_bus_type, "IBND", 4)) {
p += scnprintf(p, left,
 "\tTBD: %llx\n",
 info->params.interface_path.ibnd.reserved);
@@ -220,8 +220,13 @@ edd_show_interface(struct edd_device *edev, char *buf)
p += scnprintf(p, left, "\tidentity_tag: %x\n",
 info->params.device_path.raid.array_number);
} else if (!strncmp(info->params.interface_type, "SATA", 4)) {
-   p += scnprintf(p, left, "\tdevice: %u\n",
-info->params.device_path.sata.device);
+   p += scnprintf(p, left, "\tdevice: %u pmp: %u\n",
+info->params.device_path.sata.device,
+info->params.device_path.sata.pmp);
+   } else if (!strncmp(info->params.interface_type, "SAS", 4)) {
+   p += scnprintf(p, left, "\taddress: %llx lun: %llx\n",
+  info->params.device_path.sas.address,
+  info->params.device_path.sas.lun);
} else {
p += scnprintf(p, left, "\tunknown: %llx %llx\n",
 info->params.device_path.unknown.reserved1,
diff --git a/include/uapi/linux/edd.h b/include/uapi/linux/edd.h
index 89240a0..aff4573 100644
--- a/include/uapi/linux/edd.h
+++ b/include/uapi/linux/edd.h
@@ -155,12 +155,16 @@ struct edd_device_params {
} __attribute__ ((packed)) raid;
struct {
__u8 device;
-   __u8 reserved1;
+   __u8 pmp;
__u16 reserved2;
__u32 reserved3;
__u64 reserved4;
} __attribute__ ((packed)) sata;
struct {
+   __u64 address;
+   __u64 lun;
+   } __attribute__ ((packed)) sas;
+   struct {
__u64 reserved1;
__u64 reserved2;
} __attribute__ ((packed)) unknown;
-- 
2.5.0

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

[PATCH] Add support for EDD4 fields and types.

[Peter Jones]

Back in 2010, t13 EDD version 4 added a couple of storage types, some
host bridge types (that are pretty much all represented identically),
and a couple of fields on existing storage types.

This change makes the driver expose those to userland.

Signed-off-by: Peter Jones <pjo...@redhat.com>
---
 drivers/firmware/edd.c   | 15 ++-
 include/uapi/linux/edd.h |  6 +-
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/edd.c b/drivers/firmware/edd.c
index e229576..5092b92 100644
--- a/drivers/firmware/edd.c
+++ b/drivers/firmware/edd.c
@@ -152,15 +152,15 @@ edd_show_host_bus(struct edd_device *edev, char *buf)
 info->params.interface_path.isa.base_address);
} else if (!strncmp(info->params.host_bus_type, "PCIX", 4) ||
   !strncmp(info->params.host_bus_type, "PCI", 3) ||
-  !strncmp(info->params.host_bus_type, "XPRS", 4)) {
+  !strncmp(info->params.host_bus_type, "XPRS", 4) ||
+  !strncmp(info->params.host_bus_type, "HTPT", 4)) {
p += scnprintf(p, left,
 "\t%02x:%02x.%d  channel: %u\n",
 info->params.interface_path.pci.bus,
 info->params.interface_path.pci.slot,
 info->params.interface_path.pci.function,
 info->params.interface_path.pci.channel);
-   } else if (!strncmp(info->params.host_bus_type, "IBND", 4) ||
-  !strncmp(info->params.host_bus_type, "HTPT", 4)) {
+   } else if (!strncmp(info->params.host_bus_type, "IBND", 4)) {
p += scnprintf(p, left,
 "\tTBD: %llx\n",
 info->params.interface_path.ibnd.reserved);
@@ -220,8 +220,13 @@ edd_show_interface(struct edd_device *edev, char *buf)
p += scnprintf(p, left, "\tidentity_tag: %x\n",
 info->params.device_path.raid.array_number);
} else if (!strncmp(info->params.interface_type, "SATA", 4)) {
-   p += scnprintf(p, left, "\tdevice: %u\n",
-info->params.device_path.sata.device);
+   p += scnprintf(p, left, "\tdevice: %u pmp: %u\n",
+info->params.device_path.sata.device,
+info->params.device_path.sata.pmp);
+   } else if (!strncmp(info->params.interface_type, "SAS", 4)) {
+   p += scnprintf(p, left, "\taddress: %llx lun: %llx\n",
+  info->params.device_path.sas.address,
+  info->params.device_path.sas.lun);
} else {
p += scnprintf(p, left, "\tunknown: %llx %llx\n",
 info->params.device_path.unknown.reserved1,
diff --git a/include/uapi/linux/edd.h b/include/uapi/linux/edd.h
index 89240a0..aff4573 100644
--- a/include/uapi/linux/edd.h
+++ b/include/uapi/linux/edd.h
@@ -155,12 +155,16 @@ struct edd_device_params {
} __attribute__ ((packed)) raid;
struct {
__u8 device;
-   __u8 reserved1;
+   __u8 pmp;
__u16 reserved2;
__u32 reserved3;
__u64 reserved4;
} __attribute__ ((packed)) sata;
struct {
+   __u64 address;
+   __u64 lun;
+   } __attribute__ ((packed)) sas;
+   struct {
__u64 reserved1;
__u64 reserved2;
} __attribute__ ((packed)) unknown;
-- 
2.5.0

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

Re: [PATCH] drivers/firmware: make efi/esrt.c driver explicitly non-modular

[Peter Jones]

On Tue, Sep 01, 2015 at 09:57:08AM +0100, Matt Fleming wrote:
> On Mon, 31 Aug, at 09:52:02AM, Peter Jones wrote:
> > On Wed, Aug 26, 2015 at 06:11:28PM +0100, Matt Fleming wrote:
> > > 
> > > Looks good to me. I know Peter is on vacation right now, so I'm still
> > > expecting a response from him. In the meantime, I'll queue this up in
> > > the EFI 'next' branch. Thanks!
> > 
> > Looks fine to me as well.
> 
> Thanks Peter. May I add your ACK?

Yes, sorry:

Acked-by: Peter Jones 

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

Re: [PATCH] drivers/firmware: make efi/esrt.c driver explicitly non-modular

[Peter Jones]

On Tue, Sep 01, 2015 at 09:57:08AM +0100, Matt Fleming wrote:
> On Mon, 31 Aug, at 09:52:02AM, Peter Jones wrote:
> > On Wed, Aug 26, 2015 at 06:11:28PM +0100, Matt Fleming wrote:
> > > 
> > > Looks good to me. I know Peter is on vacation right now, so I'm still
> > > expecting a response from him. In the meantime, I'll queue this up in
> > > the EFI 'next' branch. Thanks!
> > 
> > Looks fine to me as well.
> 
> Thanks Peter. May I add your ACK?

Yes, sorry:

Acked-by: Peter Jones <pjo...@redhat.com>

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

Re: [PATCH] efifb: Add support for 64-bit frame buffer addresses

[Peter Jones]

On Fri, Aug 28, 2015 at 01:12:19PM +0100, Matt Fleming wrote:
> From: Matt Fleming 
> 
> The EFI Graphics Output Protocol uses 64-bit frame buffer addresses
> but these get truncated to 32-bit by the EFI boot stub when storing
> the address in the 'lfb_base' field of 'struct screen_info'.
> 
> Add a 'ext_lfb_base' field for the upper 32-bits of the frame buffer
> address and set VIDEO_TYPE_CAPABILITY_64BIT_BASE when the field is
> useable.
> 
> It turns out that the reason no one has required this support so far
> is that there's actually code in tianocore to "downgrade" PCI
> resources that have option ROMs and 64-bit BARS from 64-bit to 32-bit
> to cope with legacy option ROMs that can't handle 64-bit addresses.
> The upshot is that basically all GOP devices in the wild use a 32-bit
> frame buffer address.
> 
> Still, it is possible to build firmware that uses a full 64-bit GOP
> frame buffer address. Chad did, which led to him reporting this issue.
> 
> Add support in anticipation of GOP devices using 64-bit addresses more
> widely, and so that efifb works out of the box when that happens.
> 
> Reported-by: Chad Page 
> Cc: Pete Hawkins 
> Cc: Peter Jones 
> Cc: Matthew Garrett 
> Signed-off-by: Matt Fleming 

Looks good to me.

Acked-by: Peter Jones 

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

Re: [PATCH] drivers/firmware: make efi/esrt.c driver explicitly non-modular

[Peter Jones]

On Wed, Aug 26, 2015 at 06:11:28PM +0100, Matt Fleming wrote:
> On Tue, 25 Aug, at 07:00:48PM, Paul Gortmaker wrote:
> > The Kconfig for this driver is currently hidden with:
> > 
> > config EFI_ESRT
> > bool
> > 
> > ...meaning that it currently is not being built as a module by anyone.
> > Lets remove the modular code that is essentially orphaned, so that
> > when reading the driver there is no doubt it is builtin-only.
> > 
> > Since module_init translates to device_initcall in the non-modular
> > case, the init ordering remains unchanged with this commit.
> > 
> > We leave some tags like MODULE_AUTHOR for documentation purposes.
> > 
> > We don't replace module.h with init.h since the file already has that.
> > 
> > Cc: Matt Fleming 
> > Cc: Peter Jones 
> > Cc: linux-...@vger.kernel.org
> > Signed-off-by: Paul Gortmaker 
> 
> Looks good to me. I know Peter is on vacation right now, so I'm still
> expecting a response from him. In the meantime, I'll queue this up in
> the EFI 'next' branch. Thanks!

Looks fine to me as well.

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

Re: [PATCH] efifb: Add support for 64-bit frame buffer addresses

[Peter Jones]

On Fri, Aug 28, 2015 at 01:12:19PM +0100, Matt Fleming wrote:
> From: Matt Fleming <matt.flem...@intel.com>
> 
> The EFI Graphics Output Protocol uses 64-bit frame buffer addresses
> but these get truncated to 32-bit by the EFI boot stub when storing
> the address in the 'lfb_base' field of 'struct screen_info'.
> 
> Add a 'ext_lfb_base' field for the upper 32-bits of the frame buffer
> address and set VIDEO_TYPE_CAPABILITY_64BIT_BASE when the field is
> useable.
> 
> It turns out that the reason no one has required this support so far
> is that there's actually code in tianocore to "downgrade" PCI
> resources that have option ROMs and 64-bit BARS from 64-bit to 32-bit
> to cope with legacy option ROMs that can't handle 64-bit addresses.
> The upshot is that basically all GOP devices in the wild use a 32-bit
> frame buffer address.
> 
> Still, it is possible to build firmware that uses a full 64-bit GOP
> frame buffer address. Chad did, which led to him reporting this issue.
> 
> Add support in anticipation of GOP devices using 64-bit addresses more
> widely, and so that efifb works out of the box when that happens.
> 
> Reported-by: Chad Page <chad.p...@znyx.com>
> Cc: Pete Hawkins <pete.hawk...@znyx.com>
> Cc: Peter Jones <pjo...@redhat.com>
> Cc: Matthew Garrett <mj...@srcf.ucam.org>
> Signed-off-by: Matt Fleming <matt.flem...@intel.com>

Looks good to me.

Acked-by: Peter Jones <pjo...@redhat.com>

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

Re: [PATCH] drivers/firmware: make efi/esrt.c driver explicitly non-modular

[Peter Jones]

On Wed, Aug 26, 2015 at 06:11:28PM +0100, Matt Fleming wrote:
> On Tue, 25 Aug, at 07:00:48PM, Paul Gortmaker wrote:
> > The Kconfig for this driver is currently hidden with:
> > 
> > config EFI_ESRT
> > bool
> > 
> > ...meaning that it currently is not being built as a module by anyone.
> > Lets remove the modular code that is essentially orphaned, so that
> > when reading the driver there is no doubt it is builtin-only.
> > 
> > Since module_init translates to device_initcall in the non-modular
> > case, the init ordering remains unchanged with this commit.
> > 
> > We leave some tags like MODULE_AUTHOR for documentation purposes.
> > 
> > We don't replace module.h with init.h since the file already has that.
> > 
> > Cc: Matt Fleming <matt.flem...@intel.com>
> > Cc: Peter Jones <pjo...@redhat.com>
> > Cc: linux-...@vger.kernel.org
> > Signed-off-by: Paul Gortmaker <paul.gortma...@windriver.com>
> 
> Looks good to me. I know Peter is on vacation right now, so I'm still
> expecting a response from him. In the meantime, I'll queue this up in
> the EFI 'next' branch. Thanks!

Looks fine to me as well.

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

[PATCH] efi: Work around ia64 build problem with ESRT driver.

[Peter Jones]

So, I'm told this problem exists in the world:
--
Subject: Build error in -next due to 'efi: Add esrt support'

Building ia64:defconfig ... failed
--
Error log:

drivers/firmware/efi/esrt.c:28:31: fatal error: asm/early_ioremap.h: No
such file or directory
--

I'm not really sure how it's okay that we have things in asm-generic on
some platforms but not others - is having it the same everywhere not the
whole point of asm-generic?

That said, ia64 doesn't have early-ioremap.h .  So instead, since it's
difficult to imagine new IA64 machines with UEFI 2.5, just don't build
this code there.

To me this looks like a workaround - doing something like:

generic-y += early_ioremap.h

in arch/ia64/include/asm/Kbuild would appear to be more correct, but
ia64 has its own early_memremap() decl in arch/ia64/include/asm/io.h ,
and it's a macro.  So adding the above /and/ requiring that asm/io.h be
included /after/ asm/early_ioremap.h in all cases would fix it, but
that's pretty ugly as well.  Since I'm not going to spend the rest of my
life rectifying ia64 headers vs "generic" headers that aren't generic,
it's much simpler to just not build there.

Note that I've only actually tried to build this patch on x86_64, but
esrt.o still gets built there, and that would seem to demonstrate that
the conditional building is working correctly at all the places the code
built before.  I no longer have any ia64 machines handy to test that the
exclusion actually works there.

Signed-off-by: Peter Jones 
Acked-by: Tony Luck 
---
 drivers/firmware/efi/Kconfig  | 5 +
 drivers/firmware/efi/Makefile | 3 ++-
 include/linux/efi.h   | 4 
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 8de4da5..54071c1 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -18,6 +18,11 @@ config EFI_VARS
  Subsequent efibootmgr releases may be found at:
  <http://github.com/vathpela/efibootmgr>
 
+config EFI_ESRT
+   bool
+   depends on EFI && !IA64
+   default y
+
 config EFI_VARS_PSTORE
tristate "Register efivars backend for pstore"
depends on EFI_VARS && PSTORE
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 26eabbc..6fd3da9 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -1,8 +1,9 @@
 #
 # Makefile for linux kernel
 #
-obj-$(CONFIG_EFI)  += efi.o esrt.o vars.o reboot.o
+obj-$(CONFIG_EFI)  += efi.o vars.o reboot.o
 obj-$(CONFIG_EFI_VARS) += efivars.o
+obj-$(CONFIG_EFI_ESRT) += esrt.o
 obj-$(CONFIG_EFI_VARS_PSTORE)  += efi-pstore.o
 obj-$(CONFIG_UEFI_CPER)+= cper.o
 obj-$(CONFIG_EFI_RUNTIME_MAP)  += runtime-map.o
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 024c27e..2092965 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -879,7 +879,11 @@ static inline efi_status_t efi_query_variable_store(u32 
attributes, unsigned lon
 #endif
 extern void __iomem *efi_lookup_mapped_addr(u64 phys_addr);
 extern int efi_config_init(efi_config_table_type_t *arch_tables);
+#ifdef CONFIG_EFI_ESRT
 extern void __init efi_esrt_init(void);
+#else
+static inline void efi_esrt_init(void) { }
+#endif
 extern int efi_config_parse_tables(void *config_tables, int count, int sz,
   efi_config_table_type_t *arch_tables);
 extern u64 efi_get_iobase (void);
-- 
2.4.2

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

Re: [PATCH] efi: Work around ia64 build problem with ESRT driver.

[Peter Jones]

On Fri, Jun 05, 2015 at 02:54:35PM -0400, Peter Jones wrote:
> Note that I've only actually tried to build this patch on x86_64, but
> esrt.o still gets built there, and that would seem to demonstrate that
> the conditional building is working correctly at all the places the code
> built before.  I no longer have any ia64 machines handy to test that the
> exclusion actually works there.

This, of course, is a total lie.  I'll send a reply patch in which it's
actually true.

> Acked-by: Tony Luck 

So sorry, Tony.

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

[PATCH] efi: Work around ia64 build problem with ESRT driver.

[Peter Jones]

So, I'm told this problem exists in the world:
--
Subject: Build error in -next due to 'efi: Add esrt support'

Building ia64:defconfig ... failed
--
Error log:

drivers/firmware/efi/esrt.c:28:31: fatal error: asm/early_ioremap.h: No
such file or directory
--

I'm not really sure how it's okay that we have things in asm-generic on
some platforms but not others - is having it the same everywhere not the
whole point of asm-generic?

That said, ia64 doesn't have early-ioremap.h .  So instead, since it's
difficult to imagine new IA64 machines with UEFI 2.5, just don't build
this code there.

To me this looks like a workaround - doing something like:

generic-y += early_ioremap.h

in arch/ia64/include/asm/Kbuild would appear to be more correct, but
ia64 has its own early_memremap() decl in arch/ia64/include/asm/io.h ,
and it's a macro.  So adding the above /and/ requiring that asm/io.h be
included /after/ asm/early_ioremap.h in all cases would fix it, but
that's pretty ugly as well.  Since I'm not going to spend the rest of my
life rectifying ia64 headers vs "generic" headers that aren't generic,
it's much simpler to just not build there.

Note that I've only actually tried to build this patch on x86_64, but
esrt.o still gets built there, and that would seem to demonstrate that
the conditional building is working correctly at all the places the code
built before.  I no longer have any ia64 machines handy to test that the
exclusion actually works there.

Signed-off-by: Peter Jones 
Acked-by: Tony Luck 
---
 drivers/firmware/efi/Kconfig  | 5 +
 drivers/firmware/efi/Makefile | 3 ++-
 include/linux/efi.h   | 4 
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 8de4da5..2272ff0 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -18,6 +18,11 @@ config EFI_VARS
  Subsequent efibootmgr releases may be found at:
  <http://github.com/vathpela/efibootmgr>
 
+config EFI_ESRT
+   bool
+   depends on EFI && !IA64
+   default true
+
 config EFI_VARS_PSTORE
tristate "Register efivars backend for pstore"
depends on EFI_VARS && PSTORE
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 26eabbc..6fd3da9 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -1,8 +1,9 @@
 #
 # Makefile for linux kernel
 #
-obj-$(CONFIG_EFI)  += efi.o esrt.o vars.o reboot.o
+obj-$(CONFIG_EFI)  += efi.o vars.o reboot.o
 obj-$(CONFIG_EFI_VARS) += efivars.o
+obj-$(CONFIG_EFI_ESRT) += esrt.o
 obj-$(CONFIG_EFI_VARS_PSTORE)  += efi-pstore.o
 obj-$(CONFIG_UEFI_CPER)+= cper.o
 obj-$(CONFIG_EFI_RUNTIME_MAP)  += runtime-map.o
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 024c27e..2092965 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -879,7 +879,11 @@ static inline efi_status_t efi_query_variable_store(u32 
attributes, unsigned lon
 #endif
 extern void __iomem *efi_lookup_mapped_addr(u64 phys_addr);
 extern int efi_config_init(efi_config_table_type_t *arch_tables);
+#ifdef CONFIG_EFI_ESRT
 extern void __init efi_esrt_init(void);
+#else
+static inline void efi_esrt_init(void) { }
+#endif
 extern int efi_config_parse_tables(void *config_tables, int count, int sz,
   efi_config_table_type_t *arch_tables);
 extern u64 efi_get_iobase (void);
-- 
2.4.2

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

[PATCH] efi: Work around ia64 build problem with ESRT driver.

[Peter Jones]

So, I'm told this problem exists in the world:
--
Subject: Build error in -next due to 'efi: Add esrt support'

Building ia64:defconfig ... failed
--
Error log:

drivers/firmware/efi/esrt.c:28:31: fatal error: asm/early_ioremap.h: No
such file or directory
--

I'm not really sure how it's okay that we have things in asm-generic on
some platforms but not others - is having it the same everywhere not the
whole point of asm-generic?

That said, ia64 doesn't have early-ioremap.h .  So instead, since it's
difficult to imagine new IA64 machines with UEFI 2.5, just don't build
this code there.

To me this looks like a workaround - doing something like:

generic-y += early_ioremap.h

in arch/ia64/include/asm/Kbuild would appear to be more correct, but
ia64 has its own early_memremap() decl in arch/ia64/include/asm/io.h ,
and it's a macro.  So adding the above /and/ requiring that asm/io.h be
included /after/ asm/early_ioremap.h in all cases would fix it, but
that's pretty ugly as well.  Since I'm not going to spend the rest of my
life rectifying ia64 headers vs "generic" headers that aren't generic,
it's much simpler to just not build there.

Note that I've only actually tried to build this patch on x86_64, but
esrt.o still gets built there, and that would seem to demonstrate that
the conditional building is working correctly at all the places the code
built before.  I no longer have any ia64 machines handy to test that the
exclusion actually works there.

Signed-off-by: Peter Jones 
---
 drivers/firmware/efi/Makefile | 5 -
 include/linux/efi.h   | 4 
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 26eabbc..81c8527 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -1,7 +1,10 @@
 #
 # Makefile for linux kernel
 #
-obj-$(CONFIG_EFI)  += efi.o esrt.o vars.o reboot.o
+obj-$(CONFIG_EFI)  += efi.o vars.o reboot.o
+ifeq ($(CONFIG_IA64),)
+obj-$(CONFIG_EFI)  += esrt.o
+endif
 obj-$(CONFIG_EFI_VARS) += efivars.o
 obj-$(CONFIG_EFI_VARS_PSTORE)  += efi-pstore.o
 obj-$(CONFIG_UEFI_CPER)+= cper.o
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 024c27e..1983d17 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -879,7 +879,11 @@ static inline efi_status_t efi_query_variable_store(u32 
attributes, unsigned lon
 #endif
 extern void __iomem *efi_lookup_mapped_addr(u64 phys_addr);
 extern int efi_config_init(efi_config_table_type_t *arch_tables);
+#ifdef CONFIG_IA64
+static inline void efi_esrt_init(void) { }
+#else
 extern void __init efi_esrt_init(void);
+#endif
 extern int efi_config_parse_tables(void *config_tables, int count, int sz,
   efi_config_table_type_t *arch_tables);
 extern u64 efi_get_iobase (void);
-- 
2.4.2

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

[PATCH] efi: Work around ia64 build problem with ESRT driver.

[Peter Jones]

So, I'm told this problem exists in the world:
--
Subject: Build error in -next due to 'efi: Add esrt support'

Building ia64:defconfig ... failed
--
Error log:

drivers/firmware/efi/esrt.c:28:31: fatal error: asm/early_ioremap.h: No
such file or directory
--

I'm not really sure how it's okay that we have things in asm-generic on
some platforms but not others - is having it the same everywhere not the
whole point of asm-generic?

That said, ia64 doesn't have early-ioremap.h .  So instead, since it's
difficult to imagine new IA64 machines with UEFI 2.5, just don't build
this code there.

To me this looks like a workaround - doing something like:

generic-y += early_ioremap.h

in arch/ia64/include/asm/Kbuild would appear to be more correct, but
ia64 has its own early_memremap() decl in arch/ia64/include/asm/io.h ,
and it's a macro.  So adding the above /and/ requiring that asm/io.h be
included /after/ asm/early_ioremap.h in all cases would fix it, but
that's pretty ugly as well.  Since I'm not going to spend the rest of my
life rectifying ia64 headers vs generic headers that aren't generic,
it's much simpler to just not build there.

Note that I've only actually tried to build this patch on x86_64, but
esrt.o still gets built there, and that would seem to demonstrate that
the conditional building is working correctly at all the places the code
built before.  I no longer have any ia64 machines handy to test that the
exclusion actually works there.

Signed-off-by: Peter Jones pjo...@redhat.com
Acked-by: Tony Luck tony.l...@intel.com
---
 drivers/firmware/efi/Kconfig  | 5 +
 drivers/firmware/efi/Makefile | 3 ++-
 include/linux/efi.h   | 4 
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 8de4da5..2272ff0 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -18,6 +18,11 @@ config EFI_VARS
  Subsequent efibootmgr releases may be found at:
  http://github.com/vathpela/efibootmgr
 
+config EFI_ESRT
+   bool
+   depends on EFI  !IA64
+   default true
+
 config EFI_VARS_PSTORE
tristate Register efivars backend for pstore
depends on EFI_VARS  PSTORE
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 26eabbc..6fd3da9 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -1,8 +1,9 @@
 #
 # Makefile for linux kernel
 #
-obj-$(CONFIG_EFI)  += efi.o esrt.o vars.o reboot.o
+obj-$(CONFIG_EFI)  += efi.o vars.o reboot.o
 obj-$(CONFIG_EFI_VARS) += efivars.o
+obj-$(CONFIG_EFI_ESRT) += esrt.o
 obj-$(CONFIG_EFI_VARS_PSTORE)  += efi-pstore.o
 obj-$(CONFIG_UEFI_CPER)+= cper.o
 obj-$(CONFIG_EFI_RUNTIME_MAP)  += runtime-map.o
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 024c27e..2092965 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -879,7 +879,11 @@ static inline efi_status_t efi_query_variable_store(u32 
attributes, unsigned lon
 #endif
 extern void __iomem *efi_lookup_mapped_addr(u64 phys_addr);
 extern int efi_config_init(efi_config_table_type_t *arch_tables);
+#ifdef CONFIG_EFI_ESRT
 extern void __init efi_esrt_init(void);
+#else
+static inline void efi_esrt_init(void) { }
+#endif
 extern int efi_config_parse_tables(void *config_tables, int count, int sz,
   efi_config_table_type_t *arch_tables);
 extern u64 efi_get_iobase (void);
-- 
2.4.2

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

Re: [PATCH] efi: Work around ia64 build problem with ESRT driver.

[Peter Jones]

On Fri, Jun 05, 2015 at 02:54:35PM -0400, Peter Jones wrote:
 Note that I've only actually tried to build this patch on x86_64, but
 esrt.o still gets built there, and that would seem to demonstrate that
 the conditional building is working correctly at all the places the code
 built before.  I no longer have any ia64 machines handy to test that the
 exclusion actually works there.

This, of course, is a total lie.  I'll send a reply patch in which it's
actually true.

 Acked-by: Tony Luck tony.l...@intel.com

So sorry, Tony.

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

[PATCH] efi: Work around ia64 build problem with ESRT driver.

[Peter Jones]

So, I'm told this problem exists in the world:
--
Subject: Build error in -next due to 'efi: Add esrt support'

Building ia64:defconfig ... failed
--
Error log:

drivers/firmware/efi/esrt.c:28:31: fatal error: asm/early_ioremap.h: No
such file or directory
--

I'm not really sure how it's okay that we have things in asm-generic on
some platforms but not others - is having it the same everywhere not the
whole point of asm-generic?

That said, ia64 doesn't have early-ioremap.h .  So instead, since it's
difficult to imagine new IA64 machines with UEFI 2.5, just don't build
this code there.

To me this looks like a workaround - doing something like:

generic-y += early_ioremap.h

in arch/ia64/include/asm/Kbuild would appear to be more correct, but
ia64 has its own early_memremap() decl in arch/ia64/include/asm/io.h ,
and it's a macro.  So adding the above /and/ requiring that asm/io.h be
included /after/ asm/early_ioremap.h in all cases would fix it, but
that's pretty ugly as well.  Since I'm not going to spend the rest of my
life rectifying ia64 headers vs generic headers that aren't generic,
it's much simpler to just not build there.

Note that I've only actually tried to build this patch on x86_64, but
esrt.o still gets built there, and that would seem to demonstrate that
the conditional building is working correctly at all the places the code
built before.  I no longer have any ia64 machines handy to test that the
exclusion actually works there.

Signed-off-by: Peter Jones pjo...@redhat.com
Acked-by: Tony Luck tony.l...@intel.com
---
 drivers/firmware/efi/Kconfig  | 5 +
 drivers/firmware/efi/Makefile | 3 ++-
 include/linux/efi.h   | 4 
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 8de4da5..54071c1 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -18,6 +18,11 @@ config EFI_VARS
  Subsequent efibootmgr releases may be found at:
  http://github.com/vathpela/efibootmgr
 
+config EFI_ESRT
+   bool
+   depends on EFI  !IA64
+   default y
+
 config EFI_VARS_PSTORE
tristate Register efivars backend for pstore
depends on EFI_VARS  PSTORE
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 26eabbc..6fd3da9 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -1,8 +1,9 @@
 #
 # Makefile for linux kernel
 #
-obj-$(CONFIG_EFI)  += efi.o esrt.o vars.o reboot.o
+obj-$(CONFIG_EFI)  += efi.o vars.o reboot.o
 obj-$(CONFIG_EFI_VARS) += efivars.o
+obj-$(CONFIG_EFI_ESRT) += esrt.o
 obj-$(CONFIG_EFI_VARS_PSTORE)  += efi-pstore.o
 obj-$(CONFIG_UEFI_CPER)+= cper.o
 obj-$(CONFIG_EFI_RUNTIME_MAP)  += runtime-map.o
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 024c27e..2092965 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -879,7 +879,11 @@ static inline efi_status_t efi_query_variable_store(u32 
attributes, unsigned lon
 #endif
 extern void __iomem *efi_lookup_mapped_addr(u64 phys_addr);
 extern int efi_config_init(efi_config_table_type_t *arch_tables);
+#ifdef CONFIG_EFI_ESRT
 extern void __init efi_esrt_init(void);
+#else
+static inline void efi_esrt_init(void) { }
+#endif
 extern int efi_config_parse_tables(void *config_tables, int count, int sz,
   efi_config_table_type_t *arch_tables);
 extern u64 efi_get_iobase (void);
-- 
2.4.2

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

[PATCH] efi: Work around ia64 build problem with ESRT driver.

[Peter Jones]

So, I'm told this problem exists in the world:
--
Subject: Build error in -next due to 'efi: Add esrt support'

Building ia64:defconfig ... failed
--
Error log:

drivers/firmware/efi/esrt.c:28:31: fatal error: asm/early_ioremap.h: No
such file or directory
--

I'm not really sure how it's okay that we have things in asm-generic on
some platforms but not others - is having it the same everywhere not the
whole point of asm-generic?

That said, ia64 doesn't have early-ioremap.h .  So instead, since it's
difficult to imagine new IA64 machines with UEFI 2.5, just don't build
this code there.

To me this looks like a workaround - doing something like:

generic-y += early_ioremap.h

in arch/ia64/include/asm/Kbuild would appear to be more correct, but
ia64 has its own early_memremap() decl in arch/ia64/include/asm/io.h ,
and it's a macro.  So adding the above /and/ requiring that asm/io.h be
included /after/ asm/early_ioremap.h in all cases would fix it, but
that's pretty ugly as well.  Since I'm not going to spend the rest of my
life rectifying ia64 headers vs generic headers that aren't generic,
it's much simpler to just not build there.

Note that I've only actually tried to build this patch on x86_64, but
esrt.o still gets built there, and that would seem to demonstrate that
the conditional building is working correctly at all the places the code
built before.  I no longer have any ia64 machines handy to test that the
exclusion actually works there.

Signed-off-by: Peter Jones pjo...@redhat.com
---
 drivers/firmware/efi/Makefile | 5 -
 include/linux/efi.h   | 4 
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 26eabbc..81c8527 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -1,7 +1,10 @@
 #
 # Makefile for linux kernel
 #
-obj-$(CONFIG_EFI)  += efi.o esrt.o vars.o reboot.o
+obj-$(CONFIG_EFI)  += efi.o vars.o reboot.o
+ifeq ($(CONFIG_IA64),)
+obj-$(CONFIG_EFI)  += esrt.o
+endif
 obj-$(CONFIG_EFI_VARS) += efivars.o
 obj-$(CONFIG_EFI_VARS_PSTORE)  += efi-pstore.o
 obj-$(CONFIG_UEFI_CPER)+= cper.o
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 024c27e..1983d17 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -879,7 +879,11 @@ static inline efi_status_t efi_query_variable_store(u32 
attributes, unsigned lon
 #endif
 extern void __iomem *efi_lookup_mapped_addr(u64 phys_addr);
 extern int efi_config_init(efi_config_table_type_t *arch_tables);
+#ifdef CONFIG_IA64
+static inline void efi_esrt_init(void) { }
+#else
 extern void __init efi_esrt_init(void);
+#endif
 extern int efi_config_parse_tables(void *config_tables, int count, int sz,
   efi_config_table_type_t *arch_tables);
 extern u64 efi_get_iobase (void);
-- 
2.4.2

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

Re: [GIT PULL] EFI changes for v4.2

[Peter Jones]

On Tue, Jun 02, 2015 at 08:45:57AM +0200, Ingo Molnar wrote:
> @@ -167,7 +167,6 @@ static struct kset *esrt_kset;
>  
>  static int esre_create_sysfs_entry(void *esre, int entry_num)
>  {
> -   int rc = 0;
> struct esre_entry *entry;
> char name[20];
>  
> @@ -180,13 +179,15 @@ static int esre_create_sysfs_entry(void *esre, int 
> entry_num)
> entry->kobj.kset = esrt_kset;
>  
> if (esrt->fw_resource_version == 1) {
> +   int rc = 0;
> +
> entry->esre.esre1 = esre;
> rc = kobject_init_and_add(>kobj, _ktype, NULL,
>   "%s", name);
> -   }
> -   if (rc) {
> -   kfree(entry);
> -   return rc;
> +   if (rc) {
> +   kfree(entry);
> +   return rc;
> +   }
> }
>  
> list_add_tail(>list, _list);
> 
> How can a compiler ever have warned about 'rc' being uninitialized? It's 
> defined 
> straight at function entry, with initialization to 0. It can never be 
> uninitialized.
>
> I pulled it, because I agree with the change itself, as it's always better to 
> define and use variables in the narrowest scope possible, but I think it's a 
> cleanup, not a compiler warning fix.

Well, apparently I failed to explain it well - the warning was about
"esre" rather than "rc".  Basically before we were testing the version in
register_entries() (i.e. this function's caller) and never calling the
this function if it's not version 1.  The compiler didn't figure out
that when we set "entry->esre.esre1 = esre;", esre can not be null
because the function wouldn't be called.  Adding the explicit check
on the version here silenced the warning about entry plausibly being
NULL.

I'm guessing that this is because it's checking that the same
conditional test is involved - that the initialization is in the same
"...version == 1" test that the usage is.  But that's just a guess.

Would you like another patch to add this email to the commit message, or
do you want to add it in your tree, or what?

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

Re: [GIT PULL] EFI changes for v4.2

[Peter Jones]

On Tue, Jun 02, 2015 at 08:45:57AM +0200, Ingo Molnar wrote:
 @@ -167,7 +167,6 @@ static struct kset *esrt_kset;
  
  static int esre_create_sysfs_entry(void *esre, int entry_num)
  {
 -   int rc = 0;
 struct esre_entry *entry;
 char name[20];
  
 @@ -180,13 +179,15 @@ static int esre_create_sysfs_entry(void *esre, int 
 entry_num)
 entry-kobj.kset = esrt_kset;
  
 if (esrt-fw_resource_version == 1) {
 +   int rc = 0;
 +
 entry-esre.esre1 = esre;
 rc = kobject_init_and_add(entry-kobj, esre1_ktype, NULL,
   %s, name);
 -   }
 -   if (rc) {
 -   kfree(entry);
 -   return rc;
 +   if (rc) {
 +   kfree(entry);
 +   return rc;
 +   }
 }
  
 list_add_tail(entry-list, entry_list);
 
 How can a compiler ever have warned about 'rc' being uninitialized? It's 
 defined 
 straight at function entry, with initialization to 0. It can never be 
 uninitialized.

 I pulled it, because I agree with the change itself, as it's always better to 
 define and use variables in the narrowest scope possible, but I think it's a 
 cleanup, not a compiler warning fix.

Well, apparently I failed to explain it well - the warning was about
esre rather than rc.  Basically before we were testing the version in
register_entries() (i.e. this function's caller) and never calling the
this function if it's not version 1.  The compiler didn't figure out
that when we set entry-esre.esre1 = esre;, esre can not be null
because the function wouldn't be called.  Adding the explicit check
on the version here silenced the warning about entry plausibly being
NULL.

I'm guessing that this is because it's checking that the same
conditional test is involved - that the initialization is in the same
...version == 1 test that the usage is.  But that's just a guess.

Would you like another patch to add this email to the commit message, or
do you want to add it in your tree, or what?

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

Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware

[Peter Jones]

On Tue, Apr 21, 2015 at 06:58:58PM -0700, Andy Lutomirski wrote:
> On Tue, Apr 21, 2015 at 6:21 PM, James Bottomley
>  wrote:
> > Andy, just on the misc device idea, what about triggering the capsule
> > update from close()?  In theory close returns an error code (not sure if
> > most tools actually check this, though).  That means we can do the write
> > in chunks but pass it in atomically on the close and cat will work
> > (provided it checks the return code of close).
> 
> I thought about this but IIRC cat doesn't check the return value from close.

I checked this for the use case we'd talked about before - gnu cat
/does/ check the error code, but it's easy to miss how, because
coreutils code has some good ole' gnu-code complexity.  It'll print the
strerror() representation, but it always exits with 1 as the error
code.

Specifically the close on the output is handled by this:
---
  initialize_main (, );
  set_program_name (argv[0]);
  setlocale (LC_ALL, "");
  bindtextdomain (PACKAGE, LOCALEDIR);
  textdomain (PACKAGE);

  /* Arrange to close stdout if we exit via the
 case_GETOPT_HELP_CHAR or case_GETOPT_VERSION_CHAR code.
 Normally STDOUT_FILENO is used rather than stdout, so
 close_stdout does nothing.  */
  atexit (close_stdout);

  /* Parse command line options.  */

  while ((c = getopt_long (argc, argv, "benstuvAET", long_options, NULL))
---

Which in turn does:
---
void
close_stdout (void)
{
  if (close_stream (stdout) != 0
  && !(ignore_EPIPE && errno == EPIPE))
{
  char const *write_error = _("write error");
  if (file_name)
error (0, errno, "%s: %s", quotearg_colon (file_name),
   write_error);
  else
error (0, errno, "%s", write_error);

  _exit (exit_failure);
}

   if (close_stream (stderr) != 0)
 _exit (exit_failure);
}
---

exit_failure is a global from libcoreutils.a which cat never changes
from the default, so it's always 1.

(And of course error() is coreutils' own implementation rather than
glibc's because hey maybe you're not using glibc, but still, it's
there.)

So it's /annoying/ to propagate the error from there programatically,
but it can work.

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

Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware

[Peter Jones]

On Tue, Apr 21, 2015 at 06:58:58PM -0700, Andy Lutomirski wrote:
 On Tue, Apr 21, 2015 at 6:21 PM, James Bottomley
 james.bottom...@hansenpartnership.com wrote:
  Andy, just on the misc device idea, what about triggering the capsule
  update from close()?  In theory close returns an error code (not sure if
  most tools actually check this, though).  That means we can do the write
  in chunks but pass it in atomically on the close and cat will work
  (provided it checks the return code of close).
 
 I thought about this but IIRC cat doesn't check the return value from close.

I checked this for the use case we'd talked about before - gnu cat
/does/ check the error code, but it's easy to miss how, because
coreutils code has some good ole' gnu-code complexity.  It'll print the
strerror() representation, but it always exits with 1 as the error
code.

Specifically the close on the output is handled by this:
---
  initialize_main (argc, argv);
  set_program_name (argv[0]);
  setlocale (LC_ALL, );
  bindtextdomain (PACKAGE, LOCALEDIR);
  textdomain (PACKAGE);

  /* Arrange to close stdout if we exit via the
 case_GETOPT_HELP_CHAR or case_GETOPT_VERSION_CHAR code.
 Normally STDOUT_FILENO is used rather than stdout, so
 close_stdout does nothing.  */
  atexit (close_stdout);

  /* Parse command line options.  */

  while ((c = getopt_long (argc, argv, benstuvAET, long_options, NULL))
---

Which in turn does:
---
void
close_stdout (void)
{
  if (close_stream (stdout) != 0
   !(ignore_EPIPE  errno == EPIPE))
{
  char const *write_error = _(write error);
  if (file_name)
error (0, errno, %s: %s, quotearg_colon (file_name),
   write_error);
  else
error (0, errno, %s, write_error);

  _exit (exit_failure);
}

   if (close_stream (stderr) != 0)
 _exit (exit_failure);
}
---

exit_failure is a global from libcoreutils.a which cat never changes
from the default, so it's always 1.

(And of course error() is coreutils' own implementation rather than
glibc's because hey maybe you're not using glibc, but still, it's
there.)

So it's /annoying/ to propagate the error from there programatically,
but it can work.

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

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Peter Jones]

On Tue, Mar 10, 2015 at 08:51:59AM -0700, Andy Lutomirski wrote:
> On Tue, Mar 10, 2015 at 8:40 AM, Peter Jones  wrote:
> >
> >> >> So, for the sysfs interface, let's not allow loading from /lib. Let's
> >> >> not require a userland tool. Let's just do,
> >> >>
> >> >>   # echo /path/to/my/awesome/capsule.bin > /sys/../capsule
> >> >
> >> >>
> >> >> and be done with it.
> >> >>
> >> >> Hmmm?
> >> >
> >> > I assume you're implying a) the capsule header with the guid is embedded
> >> > in the .bin there already, and b) one contiguous write(2) with error
> >> > reporting coming through something like vars.c's efi_status_to_err()?
> >> >
> >> > If so, yes, I prefer this API.
> >> >
> >>
> >> Is using a char device really so bad?  I have a "simple_char" that
> >> makes this really easy that's pending review.
> >
> > As long as there's straightforward propagation of the EFI_STATUS return
> > from UpdateCapsule() back, sysfs file vs char device makes very little
> > difference to me.  Either way it's open(), write(), close().  Using the
> > runtime firmware upload interface designed for wifi and scsi devices is
> > the part I don't really like.
> >
> 
> I'm not 100% happy with write(2) (which is all we have in sysfs) for
> two reasons:
> 
> 1. If we write a file name, eww.  That's more complicated, requires
> temporary files, has annoying mount namespace issues, etc.
> 
> 2. If we write the full contents, we need to do it in a single call to
> write.  That means that we can't use cat, which mostly defeats the
> purpose.  In fact, using cat could be actively harmful.

So if what we wind up with is:

fd = open("/sys/.../capsule", O_RDWR);
write(fd, buf, size/N);
...
write(fd, buf + M*size/N, size/N);
close(fd);

You're suggesting the error code would post on close()?  My worry about
that is that I imagine a lot less code in the wild checks the error code
on close() than on write() - though gnu cat does do so on both.  But
there are other questions still - will it post on fdatasync()?  On
fsync()?

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

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Peter Jones]

> >> So, for the sysfs interface, let's not allow loading from /lib. Let's
> >> not require a userland tool. Let's just do,
> >>
> >>   # echo /path/to/my/awesome/capsule.bin > /sys/../capsule
> >
> >>
> >> and be done with it.
> >>
> >> Hmmm?
> >
> > I assume you're implying a) the capsule header with the guid is embedded
> > in the .bin there already, and b) one contiguous write(2) with error
> > reporting coming through something like vars.c's efi_status_to_err()?
> >
> > If so, yes, I prefer this API.
> >
> 
> Is using a char device really so bad?  I have a "simple_char" that
> makes this really easy that's pending review.

As long as there's straightforward propagation of the EFI_STATUS return
from UpdateCapsule() back, sysfs file vs char device makes very little
difference to me.  Either way it's open(), write(), close().  Using the
runtime firmware upload interface designed for wifi and scsi devices is
the part I don't really like.

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

Re: fwupdate

[Peter Jones]

On Tue, Mar 10, 2015 at 10:56:32AM -0400, Peter Jones wrote:
> That said, I haven't sent my patch to add the capsule headers to gnu-efi
> yet, so you won't get very far - I'll make sure and send those this
> week, hopefully today.

Slight correction, I did actually do that - it's in the current upstream
repo.

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

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Peter Jones]

On Tue, Mar 10, 2015 at 12:26:52PM +, Matt Fleming wrote:
> On Fri, 06 Mar, at 04:39:12PM, Peter Jones wrote:
> > 
> > So again: do we really need or want to do this?
> 
> One thing that we totally lose the ability to do is use the capsule
> interface for things *other* than firmware updates, e.g.
> 
>  https://lkml.org/lkml/2013/10/16/327 
> 
> Also, requiring embedded or custom OS to include fwupdate into their
> existing boot solutions is a bit heavy handed when literally all they
> want to do is cat a binary blob to a sysfs file.
> 
> I don't see why we can't have both solutions.

Yeah - we clearly need a kernel interface for some embedded devices, and
it'd be better for every vendor to not implement it themselves.

> Another thing is that ESRT isn't going to be supported by every
> platform.

Yeah - though I think you're *mostly* talking about the same platforms
as above.

> So, for the sysfs interface, let's not allow loading from /lib. Let's
> not require a userland tool. Let's just do,
> 
>   # echo /path/to/my/awesome/capsule.bin > /sys/../capsule

> 
> and be done with it.
> 
> Hmmm?

I assume you're implying a) the capsule header with the guid is embedded
in the .bin there already, and b) one contiguous write(2) with error
reporting coming through something like vars.c's efi_status_to_err()?

If so, yes, I prefer this API.

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

Re: fwupdate

[Peter Jones]

On Mon, Mar 09, 2015 at 06:54:12PM -0700, Roy Franz wrote:
> On Mon, Mar 9, 2015 at 2:23 PM, Borislav Petkov  wrote:
> > + pjones.
> >
> > So reportedly, there is already a capsule-loading thing which doesn't
> > need the kernel at all:
> >
> > https://github.com/rhinstaller/fwupdate
> >
> > So why are we even wasting energy with this discussion here?
> >
> > --
> > Regards/Gruss,
> > Boris.
> 
> The network boot case can be taken care of as Peter described (ie
> taking network device paths, instead of just file paths), so
> this should work for those cases as well.  There would be some extra
> setup, as for this to work the EFI firmware update program
> would need to be run at boot (via the BootNext variable), but I don't
> think this is unreasonable.
> It looks like GnuEFI now supports Aarch64, so building the firmware
> update utility shouldn't be a problem.  Peter - have you you tried
> building
> this on Aarch64 yet?  If not I'll give it a try.

I tried a very early version of the code on an Aarch64 machine where I
also wrote the firmware feature, and it basically seemed to work, modulo
my buggy firmware ;)  I haven't tried it recently, but I do have all the
right makefile goo in there to build it reasonably, and I don't know of
any reason it wouldn't work.

That said, I haven't sent my patch to add the capsule headers to gnu-efi
yet, so you won't get very far - I'll make sure and send those this
week, hopefully today.  Also note that fwupdate is still a very active
work in progress; it's not /quite/ ready for general purpose deployment
even in a "trying it out" phase, but I'm hoping to get to that point
this week or next - in particular the code on github right now assumes
an HD() device path with a specific partition guid.

That is, it has literally worked on two machines ever.  Yes, it's a
thing we intend on supporting, but it's not /there/ yet.

> I think there is some value in using runtime update capsule calls, as
> that will help make sure the feature works in the firmware.  I think
> with arm64 servers in an early stage of development, we can influence
> the firmware support of various features by using them.  I don't know
> that this is a sufficient reason, but if runtime capsules are never
> used then there is a good chance that there will be many broken
> implementations.

That's certainly potentially valid, but it doesn't necessarily yield
something we (the OS vendor deploying features to customers) can
actually depend on.  Usually firmware features like this become
generally usable when there's some market maker causing hardware vendors
to have a large interest in making sure the feature works.  That means
most often the carrot being dangled is MS Logo certification marketing
dollars, and the testing all follows from the situations they require to
work.  So far, it appears MS has been using this only from Boot
Services, so that's what works.  

It looks to me like that's probably likely to remain true.  One reason
is peripheral cards.  My expectation (though possibly unfounded) is that
MS will require peripheral cards to use this same update mechanism for
option ROMs, and even if they don't I know some vendors are working on
it.  To do so, the card's UEFI driver (supplied on the option ROM) will
be using EFI's Firmware Management Protocol (FMP) to participate in the
capsule update system.

The 2.5 spec drafts that include FMP and UpdateCapsule() support for FMP
state that capsule updates for FMP aren't required to be available
during runtime - it expressly calls out the case where
CAPSULE_FLAGS_PERSIST_ACROSS_RESET is used.  (I don't think I'm allowed
to quote language from drafts in public, but if you have a copy, I'm
talking about the last paragraph of 22.2.1)  In my mind this means most
implementations are going to always try to do updates from before
ExitBootServices() for those option types - anything else and you're
just in an error case where you have to do that anyway.

So again, I'm not dead-set against having a kernel implementation, but I
think its use cases are severely constrained, and most of the time most
implementations will want to do this before the kernel loads.  The best
case for doing it in the kernel is embedded devices where a) there's no
persistent storage except the flash part you're updating anyway, and b)
the vendor nominally controls both the firmware and the deployed OS.

-- 
Peter, heading back to the code.
--
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/

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Peter Jones]

On Tue, Mar 10, 2015 at 08:51:59AM -0700, Andy Lutomirski wrote:
 On Tue, Mar 10, 2015 at 8:40 AM, Peter Jones pjo...@redhat.com wrote:
 
   So, for the sysfs interface, let's not allow loading from /lib. Let's
   not require a userland tool. Let's just do,
  
 # echo /path/to/my/awesome/capsule.bin  /sys/../capsule
  
  
   and be done with it.
  
   Hmmm?
  
   I assume you're implying a) the capsule header with the guid is embedded
   in the .bin there already, and b) one contiguous write(2) with error
   reporting coming through something like vars.c's efi_status_to_err()?
  
   If so, yes, I prefer this API.
  
 
  Is using a char device really so bad?  I have a simple_char that
  makes this really easy that's pending review.
 
  As long as there's straightforward propagation of the EFI_STATUS return
  from UpdateCapsule() back, sysfs file vs char device makes very little
  difference to me.  Either way it's open(), write(), close().  Using the
  runtime firmware upload interface designed for wifi and scsi devices is
  the part I don't really like.
 
 
 I'm not 100% happy with write(2) (which is all we have in sysfs) for
 two reasons:
 
 1. If we write a file name, eww.  That's more complicated, requires
 temporary files, has annoying mount namespace issues, etc.
 
 2. If we write the full contents, we need to do it in a single call to
 write.  That means that we can't use cat, which mostly defeats the
 purpose.  In fact, using cat could be actively harmful.

So if what we wind up with is:

fd = open(/sys/.../capsule, O_RDWR);
write(fd, buf, size/N);
...
write(fd, buf + M*size/N, size/N);
close(fd);

You're suggesting the error code would post on close()?  My worry about
that is that I imagine a lot less code in the wild checks the error code
on close() than on write() - though gnu cat does do so on both.  But
there are other questions still - will it post on fdatasync()?  On
fsync()?

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

Re: fwupdate

[Peter Jones]

On Mon, Mar 09, 2015 at 06:54:12PM -0700, Roy Franz wrote:
 On Mon, Mar 9, 2015 at 2:23 PM, Borislav Petkov b...@alien8.de wrote:
  + pjones.
 
  So reportedly, there is already a capsule-loading thing which doesn't
  need the kernel at all:
 
  https://github.com/rhinstaller/fwupdate
 
  So why are we even wasting energy with this discussion here?
 
  --
  Regards/Gruss,
  Boris.
 
 The network boot case can be taken care of as Peter described (ie
 taking network device paths, instead of just file paths), so
 this should work for those cases as well.  There would be some extra
 setup, as for this to work the EFI firmware update program
 would need to be run at boot (via the BootNext variable), but I don't
 think this is unreasonable.
 It looks like GnuEFI now supports Aarch64, so building the firmware
 update utility shouldn't be a problem.  Peter - have you you tried
 building
 this on Aarch64 yet?  If not I'll give it a try.

I tried a very early version of the code on an Aarch64 machine where I
also wrote the firmware feature, and it basically seemed to work, modulo
my buggy firmware ;)  I haven't tried it recently, but I do have all the
right makefile goo in there to build it reasonably, and I don't know of
any reason it wouldn't work.

That said, I haven't sent my patch to add the capsule headers to gnu-efi
yet, so you won't get very far - I'll make sure and send those this
week, hopefully today.  Also note that fwupdate is still a very active
work in progress; it's not /quite/ ready for general purpose deployment
even in a trying it out phase, but I'm hoping to get to that point
this week or next - in particular the code on github right now assumes
an HD() device path with a specific partition guid.

That is, it has literally worked on two machines ever.  Yes, it's a
thing we intend on supporting, but it's not /there/ yet.

 I think there is some value in using runtime update capsule calls, as
 that will help make sure the feature works in the firmware.  I think
 with arm64 servers in an early stage of development, we can influence
 the firmware support of various features by using them.  I don't know
 that this is a sufficient reason, but if runtime capsules are never
 used then there is a good chance that there will be many broken
 implementations.

That's certainly potentially valid, but it doesn't necessarily yield
something we (the OS vendor deploying features to customers) can
actually depend on.  Usually firmware features like this become
generally usable when there's some market maker causing hardware vendors
to have a large interest in making sure the feature works.  That means
most often the carrot being dangled is MS Logo certification marketing
dollars, and the testing all follows from the situations they require to
work.  So far, it appears MS has been using this only from Boot
Services, so that's what works.  

It looks to me like that's probably likely to remain true.  One reason
is peripheral cards.  My expectation (though possibly unfounded) is that
MS will require peripheral cards to use this same update mechanism for
option ROMs, and even if they don't I know some vendors are working on
it.  To do so, the card's UEFI driver (supplied on the option ROM) will
be using EFI's Firmware Management Protocol (FMP) to participate in the
capsule update system.

The 2.5 spec drafts that include FMP and UpdateCapsule() support for FMP
state that capsule updates for FMP aren't required to be available
during runtime - it expressly calls out the case where
CAPSULE_FLAGS_PERSIST_ACROSS_RESET is used.  (I don't think I'm allowed
to quote language from drafts in public, but if you have a copy, I'm
talking about the last paragraph of 22.2.1)  In my mind this means most
implementations are going to always try to do updates from before
ExitBootServices() for those option types - anything else and you're
just in an error case where you have to do that anyway.

So again, I'm not dead-set against having a kernel implementation, but I
think its use cases are severely constrained, and most of the time most
implementations will want to do this before the kernel loads.  The best
case for doing it in the kernel is embedded devices where a) there's no
persistent storage except the flash part you're updating anyway, and b)
the vendor nominally controls both the firmware and the deployed OS.

-- 
Peter, heading back to the code.
--
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/

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Peter Jones]

On Tue, Mar 10, 2015 at 12:26:52PM +, Matt Fleming wrote:
 On Fri, 06 Mar, at 04:39:12PM, Peter Jones wrote:
  
  So again: do we really need or want to do this?
 
 One thing that we totally lose the ability to do is use the capsule
 interface for things *other* than firmware updates, e.g.
 
  https://lkml.org/lkml/2013/10/16/327 
 
 Also, requiring embedded or custom OS to include fwupdate into their
 existing boot solutions is a bit heavy handed when literally all they
 want to do is cat a binary blob to a sysfs file.
 
 I don't see why we can't have both solutions.

Yeah - we clearly need a kernel interface for some embedded devices, and
it'd be better for every vendor to not implement it themselves.

 Another thing is that ESRT isn't going to be supported by every
 platform.

Yeah - though I think you're *mostly* talking about the same platforms
as above.

 So, for the sysfs interface, let's not allow loading from /lib. Let's
 not require a userland tool. Let's just do,
 
   # echo /path/to/my/awesome/capsule.bin  /sys/../capsule

 
 and be done with it.
 
 Hmmm?

I assume you're implying a) the capsule header with the guid is embedded
in the .bin there already, and b) one contiguous write(2) with error
reporting coming through something like vars.c's efi_status_to_err()?

If so, yes, I prefer this API.

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

Re: fwupdate

[Peter Jones]

On Tue, Mar 10, 2015 at 10:56:32AM -0400, Peter Jones wrote:
 That said, I haven't sent my patch to add the capsule headers to gnu-efi
 yet, so you won't get very far - I'll make sure and send those this
 week, hopefully today.

Slight correction, I did actually do that - it's in the current upstream
repo.

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

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Peter Jones]

  So, for the sysfs interface, let's not allow loading from /lib. Let's
  not require a userland tool. Let's just do,
 
# echo /path/to/my/awesome/capsule.bin  /sys/../capsule
 
 
  and be done with it.
 
  Hmmm?
 
  I assume you're implying a) the capsule header with the guid is embedded
  in the .bin there already, and b) one contiguous write(2) with error
  reporting coming through something like vars.c's efi_status_to_err()?
 
  If so, yes, I prefer this API.
 
 
 Is using a char device really so bad?  I have a simple_char that
 makes this really easy that's pending review.

As long as there's straightforward propagation of the EFI_STATUS return
from UpdateCapsule() back, sysfs file vs char device makes very little
difference to me.  Either way it's open(), write(), close().  Using the
runtime firmware upload interface designed for wifi and scsi devices is
the part I don't really like.

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

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Peter Jones]

On Fri, Mar 06, 2015 at 01:49:20PM -0800, Roy Franz wrote:
> On Fri, Mar 6, 2015 at 1:39 PM, Peter Jones  wrote:
> > On Tue, Feb 24, 2015 at 12:49:09PM +, Kweh, Hock Leong wrote:
> >> Hi All,
> >>
> >> After some internal discussion and re-design prototyping & testing on
> >> this efi capsule interface kernel module, I would like to start a 
> >> discussion
> >> here on the new idea and wish to get input for the implementation and
> >> eventually upstream.
> >
> > So do we actually *need* a kernel interface to UpdateCapsule?  We've
> > already got an implementation in progress where we use my ESRT patch to
> > figure out what firmwares we can update, and we schedule them and do the
> > update during boot up before the normal bootloader is run.  That means
> > never having to call UpdateCapsule() after ExitBootServices() or
> > SetVirtualAddressMap() have been called, and only doing it across
> > reboots that UEFI is performing itself.  It also means never having to
> > do it with multiple CPUs running.
> 
> So this does it by writing the capsules to the EFI system partition, and 
> having
> them processed from there during the next boot?
> How would this work on diskless systems? (or boot-disk-less systems)

One of the things I'm currently writing is making the file we load be
specified correctly by UEFI device paths - and that'll include the
ability to get it from devices presented by the network drivers.  On
currently extant test machines that includes tftp support, just like
netbooting.  On UEFI 2.5 machines, where ESRT is introduced so that we
actually know something about the capsules we can apply updates for, it
also includes http support.  Admittedly that means when you're doing
deployment you'll have to have some process for putting the images
someplace, but it can be the same device you're net-booting from.
Just one example.

If we're talking about systems that are booting entirely out of their
own firmware volume, there's definitely some legitimate argument that
doing this without an ESP could be valuable - so yeah, a kernel API in
that case might be worthwhile.

That said, the extra complexity combined with the fact that it's running
after ExitBootServices() and SetVirtualAddressMap() means I'll probably
avoid using it from the userland code except on machines where there are
no other options.  My faith that we're going to see a lot of machines
where that's well tested without our address maps looking exactly like
Windows's isn't very high, and we've repeatedly run into a lot of pain
with that in the past.  That's not the only thing that has to be exactly
right, either - for instance there's no guarantee it'll work if you use
the ACPI reset vector instead of the EFI runtime services
ResetSystem(EfiResetWarm) call.  Right now we use the ACPI method
preferentially because of SetVirtualAddressMap() not working as
documented on so *many* platforms.  That means what happens upon reboot
when we do UpdateCapsule() is undefined behavior.

Of course I'm likely not the only person who will ever implement tools
in userland to orchestrate firmware updates, either :)

> What are the use cases for capsules that don't require a reboot?  I'm
> not really clear uses for those, but the kernel interface could be
> better for those, as it would allow processing without a reboot.

I'm really not sure if we know the answer to that yet.  Things like USB
devices most often won't ever be registered with firmware's FMP anyway,
so they won't have capsules exposed, and they'll use more traditional
USB commands to do firmware updates - stuff like hughsie's ColorHug
device are in that category, and he's already supporting that with
fwupd.

Things like peripheral card option ROMs are potentially in that
category, though I'm a little skeptical of the idea that I actually want
to update the firmware on my video or network card with the kernel
already running, and that when I do I'm not going to want to reboot
immediately to make sure it worked right anyway.  There are almost
certainly lots of cases I haven't thought of, though.

If we want this interface for those cases, I think we should also be
enumerating the cases we think it's supporting, as well, even if only in
broad strokes.  I don't think we need to say "support this specific
device's updates", but keeping track of the basic model of the update
we're supporting would go a long way to establishing the value of
supporting all the complexity.

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

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Peter Jones]

On Tue, Feb 24, 2015 at 12:49:09PM +, Kweh, Hock Leong wrote:
> Hi All,
> 
> After some internal discussion and re-design prototyping & testing on
> this efi capsule interface kernel module, I would like to start a discussion
> here on the new idea and wish to get input for the implementation and
> eventually upstream.

So do we actually *need* a kernel interface to UpdateCapsule?  We've
already got an implementation in progress where we use my ESRT patch to
figure out what firmwares we can update, and we schedule them and do the
update during boot up before the normal bootloader is run.  That means
never having to call UpdateCapsule() after ExitBootServices() or
SetVirtualAddressMap() have been called, and only doing it across
reboots that UEFI is performing itself.  It also means never having to
do it with multiple CPUs running.

I've posted several revisions of the ESRT patch to linux-efi , and right
now we're just waiting on the UEFI 2.5 spec to be finalized before I
send a final copy to Matt.  The command line tool and the code to apply
the firmwares during boot are at https://github.com/rhinstaller/fwupdate
and there's a GNOME-based UI in progress at
https://github.com/hughsie/fwupd (yes we apparently stink at naming
things.)

I'm not sure how making this go through the kernel will make things
better - it introduces a lot more things that can go wrong, and the only
benefit is one reboot where BootNext is set - which actually is
relatively fast even slow-POSTing machines.  After that the
UpdateCapsule+reboot to apply is *extremely* fast, because
applying capsule updates have to happen very very early in the boot-up
sequence.  (That early processing is /effectively/ a requirement,
since it has to happen before the block write locking on the SPI part is
done.)  So even on the machine that takes quite some time to POST, the
time that would be saved saved is less than 1% or so of the total update
time.

An early version of my code worked to update a machine I got from your
employer, FWIW - right now we're adding API and fixing up implementation
bits I made specifically to that one proof-of-concept scenario, and
there's some API parts that are in UEFI 2.5 draft releases that we're
not quite handling yet.  However, there's code there that's already been
checked in which has successfully applied system firmware updates
without having a kernel interface to UpdateCapsule().

So again: do we really need or want to do this?

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

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Peter Jones]

On Tue, Feb 24, 2015 at 12:49:09PM +, Kweh, Hock Leong wrote:
 Hi All,
 
 After some internal discussion and re-design prototyping  testing on
 this efi capsule interface kernel module, I would like to start a discussion
 here on the new idea and wish to get input for the implementation and
 eventually upstream.

So do we actually *need* a kernel interface to UpdateCapsule?  We've
already got an implementation in progress where we use my ESRT patch to
figure out what firmwares we can update, and we schedule them and do the
update during boot up before the normal bootloader is run.  That means
never having to call UpdateCapsule() after ExitBootServices() or
SetVirtualAddressMap() have been called, and only doing it across
reboots that UEFI is performing itself.  It also means never having to
do it with multiple CPUs running.

I've posted several revisions of the ESRT patch to linux-efi , and right
now we're just waiting on the UEFI 2.5 spec to be finalized before I
send a final copy to Matt.  The command line tool and the code to apply
the firmwares during boot are at https://github.com/rhinstaller/fwupdate
and there's a GNOME-based UI in progress at
https://github.com/hughsie/fwupd (yes we apparently stink at naming
things.)

I'm not sure how making this go through the kernel will make things
better - it introduces a lot more things that can go wrong, and the only
benefit is one reboot where BootNext is set - which actually is
relatively fast even slow-POSTing machines.  After that the
UpdateCapsule+reboot to apply is *extremely* fast, because
applying capsule updates have to happen very very early in the boot-up
sequence.  (That early processing is /effectively/ a requirement,
since it has to happen before the block write locking on the SPI part is
done.)  So even on the machine that takes quite some time to POST, the
time that would be saved saved is less than 1% or so of the total update
time.

An early version of my code worked to update a machine I got from your
employer, FWIW - right now we're adding API and fixing up implementation
bits I made specifically to that one proof-of-concept scenario, and
there's some API parts that are in UEFI 2.5 draft releases that we're
not quite handling yet.  However, there's code there that's already been
checked in which has successfully applied system firmware updates
without having a kernel interface to UpdateCapsule().

So again: do we really need or want to do this?

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

Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Peter Jones]

On Fri, Mar 06, 2015 at 01:49:20PM -0800, Roy Franz wrote:
 On Fri, Mar 6, 2015 at 1:39 PM, Peter Jones pjo...@redhat.com wrote:
  On Tue, Feb 24, 2015 at 12:49:09PM +, Kweh, Hock Leong wrote:
  Hi All,
 
  After some internal discussion and re-design prototyping  testing on
  this efi capsule interface kernel module, I would like to start a 
  discussion
  here on the new idea and wish to get input for the implementation and
  eventually upstream.
 
  So do we actually *need* a kernel interface to UpdateCapsule?  We've
  already got an implementation in progress where we use my ESRT patch to
  figure out what firmwares we can update, and we schedule them and do the
  update during boot up before the normal bootloader is run.  That means
  never having to call UpdateCapsule() after ExitBootServices() or
  SetVirtualAddressMap() have been called, and only doing it across
  reboots that UEFI is performing itself.  It also means never having to
  do it with multiple CPUs running.
 
 So this does it by writing the capsules to the EFI system partition, and 
 having
 them processed from there during the next boot?
 How would this work on diskless systems? (or boot-disk-less systems)

One of the things I'm currently writing is making the file we load be
specified correctly by UEFI device paths - and that'll include the
ability to get it from devices presented by the network drivers.  On
currently extant test machines that includes tftp support, just like
netbooting.  On UEFI 2.5 machines, where ESRT is introduced so that we
actually know something about the capsules we can apply updates for, it
also includes http support.  Admittedly that means when you're doing
deployment you'll have to have some process for putting the images
someplace, but it can be the same device you're net-booting from.
Just one example.

If we're talking about systems that are booting entirely out of their
own firmware volume, there's definitely some legitimate argument that
doing this without an ESP could be valuable - so yeah, a kernel API in
that case might be worthwhile.

That said, the extra complexity combined with the fact that it's running
after ExitBootServices() and SetVirtualAddressMap() means I'll probably
avoid using it from the userland code except on machines where there are
no other options.  My faith that we're going to see a lot of machines
where that's well tested without our address maps looking exactly like
Windows's isn't very high, and we've repeatedly run into a lot of pain
with that in the past.  That's not the only thing that has to be exactly
right, either - for instance there's no guarantee it'll work if you use
the ACPI reset vector instead of the EFI runtime services
ResetSystem(EfiResetWarm) call.  Right now we use the ACPI method
preferentially because of SetVirtualAddressMap() not working as
documented on so *many* platforms.  That means what happens upon reboot
when we do UpdateCapsule() is undefined behavior.

Of course I'm likely not the only person who will ever implement tools
in userland to orchestrate firmware updates, either :)

 What are the use cases for capsules that don't require a reboot?  I'm
 not really clear uses for those, but the kernel interface could be
 better for those, as it would allow processing without a reboot.

I'm really not sure if we know the answer to that yet.  Things like USB
devices most often won't ever be registered with firmware's FMP anyway,
so they won't have capsules exposed, and they'll use more traditional
USB commands to do firmware updates - stuff like hughsie's ColorHug
device are in that category, and he's already supporting that with
fwupd.

Things like peripheral card option ROMs are potentially in that
category, though I'm a little skeptical of the idea that I actually want
to update the firmware on my video or network card with the kernel
already running, and that when I do I'm not going to want to reboot
immediately to make sure it worked right anyway.  There are almost
certainly lots of cases I haven't thought of, though.

If we want this interface for those cases, I think we should also be
enumerating the cases we think it's supporting, as well, even if only in
broad strokes.  I don't think we need to say support this specific
device's updates, but keeping track of the basic model of the update
we're supporting would go a long way to establishing the value of
supporting all the complexity.

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

Re: [PATCH] x86, boot: Allow 64bit EFI kernel to be loaded above 4G

[Peter Jones]

On Wed, Feb 11, 2015 at 03:55:24PM +, Matt Fleming wrote:
> On Mon, 09 Feb, at 12:23:15PM, Yinghai Lu wrote:
> > On Mon, Feb 9, 2015 at 10:27 AM, Matt Fleming  
> > wrote:
> > > On Tue, 03 Feb, at 06:03:20PM, Yinghai Lu wrote:
> > >
> > > The first thing that comes to mind is the issues we experienced last
> > > year when adding support for loading initrds above 4GB to the EFI boot
> > > stub, c.f. commit 47226ad4f4cf ("x86/efi: Only load initrd above 4g on
> > > second try").
> > >
> > > Are things going to work correctly this time?
> > 
> > That should be addressed the grub2.
>  
> I vaguely remember thinking that the issue was only experienced when
> using the EFI_FILE protocol, which grub2 doesn't use. So the grub
> developers may be OK, but we should at least give them a heads up.

Looks correct to me.

> > I was thinking that we may need to add mem_limit command together with
> > linuxefi and initrdefi.
> > or add linuxefi64/initrdefi64?
>  
> No, we definitely do not want to add any more grub commands.

Definitely agree.

> > BTW, I tested loading kernel above grub2 on
> > virutalbox, qemu/kvm/OVMF, and real servers (ami ...) all work without 
> > problem.
> > 
> > wonder if we need have one black list for 64bit UEFI that does not
> > support access
> > memory above 4G.
>  
> We have been successful, so far, in not introducing these kind of
> blacklists. It would be a shame to start now.

>From grub's point of view I'm not sure why we'd care - the pages kernel
and initramfs land in are both from the Boot Services allocator, so if the
machine doesn't support high addresses, they won't be there.

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

Re: [PATCH] x86, boot: Allow 64bit EFI kernel to be loaded above 4G

[Peter Jones]

On Wed, Feb 11, 2015 at 03:55:24PM +, Matt Fleming wrote:
 On Mon, 09 Feb, at 12:23:15PM, Yinghai Lu wrote:
  On Mon, Feb 9, 2015 at 10:27 AM, Matt Fleming m...@codeblueprint.co.uk 
  wrote:
   On Tue, 03 Feb, at 06:03:20PM, Yinghai Lu wrote:
  
   The first thing that comes to mind is the issues we experienced last
   year when adding support for loading initrds above 4GB to the EFI boot
   stub, c.f. commit 47226ad4f4cf (x86/efi: Only load initrd above 4g on
   second try).
  
   Are things going to work correctly this time?
  
  That should be addressed the grub2.
  
 I vaguely remember thinking that the issue was only experienced when
 using the EFI_FILE protocol, which grub2 doesn't use. So the grub
 developers may be OK, but we should at least give them a heads up.

Looks correct to me.

  I was thinking that we may need to add mem_limit command together with
  linuxefi and initrdefi.
  or add linuxefi64/initrdefi64?
  
 No, we definitely do not want to add any more grub commands.

Definitely agree.

  BTW, I tested loading kernel above grub2 on
  virutalbox, qemu/kvm/OVMF, and real servers (ami ...) all work without 
  problem.
  
  wonder if we need have one black list for 64bit UEFI that does not
  support access
  memory above 4G.
  
 We have been successful, so far, in not introducing these kind of
 blacklists. It would be a shame to start now.

From grub's point of view I'm not sure why we'd care - the pages kernel
and initramfs land in are both from the Boot Services allocator, so if the
machine doesn't support high addresses, they won't be there.

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

Re: EFI and multiboot2 devlopment work for Xen

[Peter Jones]

On Tue, Oct 22, 2013 at 10:51:40AM -0400, Konrad Rzeszutek Wilk wrote:
> And I still haven't found the module that can launch any PE/COFF
> image from GRUB2. Maybe that is a myth.

"chainload" will do this.  In fact, it doesn't do much:

static grub_err_t
grub_chainloader_boot (void)
{
  grub_efi_boot_services_t *b;
  grub_efi_status_t status;
  grub_efi_uintn_t exit_data_size;
  grub_efi_char16_t *exit_data = NULL;

  b = grub_efi_system_table->boot_services;
  status = efi_call_3 (b->start_image, image_handle, _data_size, 
_data);
  if (status != GRUB_EFI_SUCCESS)
...

That means, of course, that it won't use shim on Secure Boot systems.

There's probably some value in merging the two, so that chainload will
use the efi stub if and only if a) it's present, and b) it's needed for
e.g. multiple initrds or bad UGA/GOP data.

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

Re: EFI and multiboot2 devlopment work for Xen

[Peter Jones]

On Tue, Oct 22, 2013 at 10:51:40AM -0400, Konrad Rzeszutek Wilk wrote:
 And I still haven't found the module that can launch any PE/COFF
 image from GRUB2. Maybe that is a myth.

chainload will do this.  In fact, it doesn't do much:

static grub_err_t
grub_chainloader_boot (void)
{
  grub_efi_boot_services_t *b;
  grub_efi_status_t status;
  grub_efi_uintn_t exit_data_size;
  grub_efi_char16_t *exit_data = NULL;

  b = grub_efi_system_table-boot_services;
  status = efi_call_3 (b-start_image, image_handle, exit_data_size, 
exit_data);
  if (status != GRUB_EFI_SUCCESS)
...

That means, of course, that it won't use shim on Secure Boot systems.

There's probably some value in merging the two, so that chainload will
use the efi stub if and only if a) it's present, and b) it's needed for
e.g. multiple initrds or bad UGA/GOP data.

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

Re: EFI and multiboot2 devlopment work for Xen

[Peter Jones]

On Mon, Oct 21, 2013 at 02:57:56PM +0200, Daniel Kiper wrote:
> Hi,
> 
> During work on multiboot2 protocol support for Xen it was discovered
> that memory map passed via relevant tag could not represent wide range
> of memory types available on EFI platforms. Additionally, GRUB2
> implementation calls ExitBootServices() on them just before jumping
> into loaded image. In this situation loaded system could not clearly
> identify reserved memory regions, EFI runtime services regions and others.

I think you'll find that many distros are shipping patches to grub2 to
add a "linuxefi" command that starts the kernel through its EFISTUB
code.  You may want to look in to that.

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

Re: EFI and multiboot2 devlopment work for Xen

[Peter Jones]

On Mon, Oct 21, 2013 at 02:57:56PM +0200, Daniel Kiper wrote:
 Hi,
 
 During work on multiboot2 protocol support for Xen it was discovered
 that memory map passed via relevant tag could not represent wide range
 of memory types available on EFI platforms. Additionally, GRUB2
 implementation calls ExitBootServices() on them just before jumping
 into loaded image. In this situation loaded system could not clearly
 identify reserved memory regions, EFI runtime services regions and others.

I think you'll find that many distros are shipping patches to grub2 to
add a linuxefi command that starts the kernel through its EFISTUB
code.  You may want to look in to that.

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

Re: [PATCH 12/12] EFI: Runtime services virtual mapping

[Peter Jones]

On Sat, Oct 12, 2013 at 10:14:39AM +0800, Dave Young wrote:
> CCing Peter Jones .., Peter, any idea about the grub related problem?

What grub problem?  As Matt was saying, grub2 isn't loading it as
EfiBootServicesCode/Data.  grub2 is loading it as EfiLoaderData .

> 
> On 10/11/13 at 09:42am, Dave Young wrote:
> > Matt,
> > 
> > The kernel I referring is the boot kernel aka the 1st kernel,
> > the boot loader is grub2 from Fedora 19.
> > 
> > [sorry for top reply because of using webmail]
> > 
> > 
> > - Original Message -
> > From: "Matt Fleming" 
> > To: "Dave Young" 
> > Cc: "Borislav Petkov" , "X86 ML" , "LKML" 
> > , "Borislav Petkov" , "Matthew 
> > Garrett" , "H. Peter Anvin" , "James 
> > Bottomley" , "Vivek Goyal" 
> > , linux-...@vger.kernel.org, fwts-de...@lists.ubuntu.com
> > Sent: Friday, October 11, 2013 6:27:06 PM
> > Subject: Re: [PATCH 12/12] EFI: Runtime services virtual mapping
> > 
> > On Fri, 11 Oct, at 02:24:37PM, Dave Young wrote:
> > > For the boot efi_reserve_boot_services code, it's mainly for the
> > > SetVirtualAddressMap callback use, so boot regions should not be reused
> > > before SetVirtualAddressMap, but the overlapping happens before the
> > > efi_reserve_boot_services, isn't it a problem?
> > 
> > Hang on, which kernel are you referring to here? The boot kernel or the
> > kexec'd kernel? I thought you were saying you noticed the overlap when
> > running in the second (kexec'd) kernel?
> > 
> > The only reason that you would see this overlap in the first (boot)
> > kernel is if the bootloader messed up and allocated the kernel text as
> > EfiBootServicesCode/Data. I'd like to believe no bootloaders are still
> > doing that.
> > 
> > -- 
> > Matt Fleming, Intel Open Source Technology Center
> > --
> > 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/

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

Re: [PATCH 12/12] EFI: Runtime services virtual mapping

[Peter Jones]

On Sat, Oct 12, 2013 at 10:14:39AM +0800, Dave Young wrote:
 CCing Peter Jones .., Peter, any idea about the grub related problem?

What grub problem?  As Matt was saying, grub2 isn't loading it as
EfiBootServicesCode/Data.  grub2 is loading it as EfiLoaderData .

 
 On 10/11/13 at 09:42am, Dave Young wrote:
  Matt,
  
  The kernel I referring is the boot kernel aka the 1st kernel,
  the boot loader is grub2 from Fedora 19.
  
  [sorry for top reply because of using webmail]
  
  
  - Original Message -
  From: Matt Fleming m...@console-pimps.org
  To: Dave Young dyo...@redhat.com
  Cc: Borislav Petkov b...@alien8.de, X86 ML x...@kernel.org, LKML 
  linux-kernel@vger.kernel.org, Borislav Petkov b...@suse.de, Matthew 
  Garrett mj...@srcf.ucam.org, H. Peter Anvin h...@zytor.com, James 
  Bottomley james.bottom...@hansenpartnership.com, Vivek Goyal 
  vgo...@redhat.com, linux-...@vger.kernel.org, fwts-de...@lists.ubuntu.com
  Sent: Friday, October 11, 2013 6:27:06 PM
  Subject: Re: [PATCH 12/12] EFI: Runtime services virtual mapping
  
  On Fri, 11 Oct, at 02:24:37PM, Dave Young wrote:
   For the boot efi_reserve_boot_services code, it's mainly for the
   SetVirtualAddressMap callback use, so boot regions should not be reused
   before SetVirtualAddressMap, but the overlapping happens before the
   efi_reserve_boot_services, isn't it a problem?
  
  Hang on, which kernel are you referring to here? The boot kernel or the
  kexec'd kernel? I thought you were saying you noticed the overlap when
  running in the second (kexec'd) kernel?
  
  The only reason that you would see this overlap in the first (boot)
  kernel is if the bootloader messed up and allocated the kernel text as
  EfiBootServicesCode/Data. I'd like to believe no bootloaders are still
  doing that.
  
  -- 
  Matt Fleming, Intel Open Source Technology Center
  --
  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/

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

Re: [PATCH] x86/efi: Add EFI framebuffer earlyprintk support

[Peter Jones]

On Thu, Oct 10, 2013 at 07:45:21PM +0200, Ingo Molnar wrote:
> 
> * Peter Jones  wrote:
> 
> > On Thu, Oct 10, 2013 at 07:28:44PM +0200, Ingo Molnar wrote:
> > > 
> > > Is a non-32-bit framebuffer a possibility? If yes then it might be nice 
> > > to 
> > > emit an informative printk() here, so that users who try to enable EFI 
> > > early-printk can at least see why it's not working. (Assuming they get to 
> > > look at regular printk output, on a safe/working kernel.)
> > 
> > Not really - the spec allows RGBx, BGRx, and for custom bit masks, but
> > they're define like:
> > 
> > typedef struct {
> > UINT32 RedMask;
> > UINT32 GreenMask;
> > UINT32 BlueMask;
> > UINT32 ReservedMask;
> > } EFI_PIXEL_BITMASK;
> 
> Hm, that structure does not show up anywhere in the kernel that I can see.

It's the thing being interpretted in arch/x86/boot/compressed/eboot.c in
setup_gop() in the code that looks like:

if (pixel_format == PIXEL_RGB_RESERVED_8BIT_PER_COLOR) {
si->lfb_depth = 32;
si->lfb_linelength = pixels_per_scan_line * 4;
...
} else if (pixel_format == PIXEL_BGR_RESERVED_8BIT_PER_COLOR) {
...
} else if (pixel_format == PIXEL_BIT_MASK) {
find_bits(pixel_info.red_mask, >red_pos, >red_size);
...
...

> How are those mask values to be interpreted? As regular bitmasks? Are bits 
> in the masks set to 1 consecutively, starting from bit 0?

So, the spec actually has some sample code in it:

INTN
GetPixelElementSize (
IN EFI_PIXEL_BITMASK *PixelBits
)
{
INTN HighestPixel = -1;
INTN BluePixel;
INTN RedPixel;
INTN GreenPixel;
INTN RsvdPixel;
BluePixel = FindHighestSetBit (PixelBits->BlueMask);
RedPixel = FindHighestSetBit (PixelBits->RedMask);
GreenPixel = FindHighestSetBit (PixelBits->GreenMask);
RsvdPixel = FindHighestSetBit (PixelBits->ReservedMask);
HighestPixel = max (BluePixel, RedPixel);
HighestPixel = max (HighestPixel, GreenPixel);
HighestPixel = max (HighestPixel, RsvdPixel);
return HighestPixel;
}
EFI_PHYSICAL_ADDRESS NewPixelAddress;
EFI_PHYSICAL_ADDRESS CurrentPixelAddress;
EFI_GRAPHICS_OUTPUT_MODE_INFORMATION OutputInfo;
INTN PixelElementSize;

switch (OutputInfo.PixelFormat) {
case PixelBitMask:
PixelElementSize = GetPixelElementSize 
();
break;
case PixelBlueGreenRedReserved8BitPerColor:
case PixelRedGreenBlueReserved8BitPerColor:
PixelElementSize = sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL);
break;
}

Which makes this painfully clear.

> Also, the main question would be, what is the typical value for 
> si->lfb_depth. 32 on almost all EFI systems? All around the map? Depends 
> on what graphics state the EFI bootloader passes us?

Yes, 32 on almost all systems that implement a framebuffer console at
all.

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

Re: [PATCH] x86/efi: Add EFI framebuffer earlyprintk support

[Peter Jones]

On Thu, Oct 10, 2013 at 07:28:44PM +0200, Ingo Molnar wrote:
> 
> Is a non-32-bit framebuffer a possibility? If yes then it might be nice to 
> emit an informative printk() here, so that users who try to enable EFI 
> early-printk can at least see why it's not working. (Assuming they get to 
> look at regular printk output, on a safe/working kernel.)

Not really - the spec allows RGBx, BGRx, and for custom bit masks, but
they're define like:

typedef struct {
UINT32 RedMask;
UINT32 GreenMask;
UINT32 BlueMask;
UINT32 ReservedMask;
} EFI_PIXEL_BITMASK;

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

Re: [PATCH] x86/efi: Add EFI framebuffer earlyprintk support

[Peter Jones]

On Thu, Oct 10, 2013 at 07:28:44PM +0200, Ingo Molnar wrote:
 
 Is a non-32-bit framebuffer a possibility? If yes then it might be nice to 
 emit an informative printk() here, so that users who try to enable EFI 
 early-printk can at least see why it's not working. (Assuming they get to 
 look at regular printk output, on a safe/working kernel.)

Not really - the spec allows RGBx, BGRx, and for custom bit masks, but
they're define like:

typedef struct {
UINT32 RedMask;
UINT32 GreenMask;
UINT32 BlueMask;
UINT32 ReservedMask;
} EFI_PIXEL_BITMASK;

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

Re: [PATCH] x86/efi: Add EFI framebuffer earlyprintk support

[Peter Jones]

On Thu, Oct 10, 2013 at 07:45:21PM +0200, Ingo Molnar wrote:
 
 * Peter Jones pjo...@redhat.com wrote:
 
  On Thu, Oct 10, 2013 at 07:28:44PM +0200, Ingo Molnar wrote:
   
   Is a non-32-bit framebuffer a possibility? If yes then it might be nice 
   to 
   emit an informative printk() here, so that users who try to enable EFI 
   early-printk can at least see why it's not working. (Assuming they get to 
   look at regular printk output, on a safe/working kernel.)
  
  Not really - the spec allows RGBx, BGRx, and for custom bit masks, but
  they're define like:
  
  typedef struct {
  UINT32 RedMask;
  UINT32 GreenMask;
  UINT32 BlueMask;
  UINT32 ReservedMask;
  } EFI_PIXEL_BITMASK;
 
 Hm, that structure does not show up anywhere in the kernel that I can see.

It's the thing being interpretted in arch/x86/boot/compressed/eboot.c in
setup_gop() in the code that looks like:

if (pixel_format == PIXEL_RGB_RESERVED_8BIT_PER_COLOR) {
si-lfb_depth = 32;
si-lfb_linelength = pixels_per_scan_line * 4;
...
} else if (pixel_format == PIXEL_BGR_RESERVED_8BIT_PER_COLOR) {
...
} else if (pixel_format == PIXEL_BIT_MASK) {
find_bits(pixel_info.red_mask, si-red_pos, si-red_size);
...
...

 How are those mask values to be interpreted? As regular bitmasks? Are bits 
 in the masks set to 1 consecutively, starting from bit 0?

So, the spec actually has some sample code in it:

INTN
GetPixelElementSize (
IN EFI_PIXEL_BITMASK *PixelBits
)
{
INTN HighestPixel = -1;
INTN BluePixel;
INTN RedPixel;
INTN GreenPixel;
INTN RsvdPixel;
BluePixel = FindHighestSetBit (PixelBits-BlueMask);
RedPixel = FindHighestSetBit (PixelBits-RedMask);
GreenPixel = FindHighestSetBit (PixelBits-GreenMask);
RsvdPixel = FindHighestSetBit (PixelBits-ReservedMask);
HighestPixel = max (BluePixel, RedPixel);
HighestPixel = max (HighestPixel, GreenPixel);
HighestPixel = max (HighestPixel, RsvdPixel);
return HighestPixel;
}
EFI_PHYSICAL_ADDRESS NewPixelAddress;
EFI_PHYSICAL_ADDRESS CurrentPixelAddress;
EFI_GRAPHICS_OUTPUT_MODE_INFORMATION OutputInfo;
INTN PixelElementSize;

switch (OutputInfo.PixelFormat) {
case PixelBitMask:
PixelElementSize = GetPixelElementSize 
(OutputInfo.PixelInformation);
break;
case PixelBlueGreenRedReserved8BitPerColor:
case PixelRedGreenBlueReserved8BitPerColor:
PixelElementSize = sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL);
break;
}

Which makes this painfully clear.

 Also, the main question would be, what is the typical value for 
 si-lfb_depth. 32 on almost all EFI systems? All around the map? Depends 
 on what graphics state the EFI bootloader passes us?

Yes, 32 on almost all systems that implement a framebuffer console at
all.

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