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

[Luis R. Rodriguez]

On Fri, May 04, 2018 at 07:54:28AM +0200, Ard Biesheuvel wrote:
> On 4 May 2018 at 01:29, Luis R. Rodriguez  wrote:
> > On Sun, Apr 29, 2018 at 11:35:55AM +0200, Hans de Goede wrote:
> [...]
> >> diff --git a/Documentation/driver-api/firmware/request_firmware.rst 
> >> b/Documentation/driver-api/firmware/request_firmware.rst
> >> index c8bddbdcfd10..560dfed76e38 100644
> >> --- a/Documentation/driver-api/firmware/request_firmware.rst
> >> +++ b/Documentation/driver-api/firmware/request_firmware.rst
> >> @@ -73,3 +73,69 @@ If something went wrong firmware_request() returns 
> >> non-zero and fw_entry
> >>  is set to NULL. Once your driver is done with processing the firmware it
> >>  can call call firmware_release(fw_entry) to release the firmware image
> >>  and any related resource.
> >> +
> >> +EFI embedded firmware support
> >> +=
> >
> > This is a new fallback mechanism, please see:
> >
> > Documentation/driver-api/firmware/fallback-mechanisms.rst
> >
> > Refer to the section "Types of fallback mechanisms", augument the list there
> > and then move the section "Firmware sysfs loading facility" to a new file, 
> > and
> > then add a new file for your own.
> >
> >> +
> >> +On some devices the system's EFI code / ROM may contain an embedded copy
> >> +of firmware for some of the system's integrated peripheral devices and
> >> +the peripheral's Linux device-driver needs to access this firmware.
> >
> > You in no way indicate this is a just an invented scheme, a custom solution 
> > and
> > nothing standard.  I realize Ard criticized that the EFI Firmware Volume 
> > Protocol
> > is not part of the UEFI spec -- however it is a bit more widely used right?
> > Why can't Linux support it instead?
> >
> 
> Most implementations of UEFI are based on PI,

That seems to be the UEFI Platform Initialization specification:

http://www.uefi.org/sites/default/files/resources/PI_Spec_1_6.pdf

> and so it is likely that
> the protocols are available. However, the PI spec does not cover
> firmware blobs,

Indeed, I cannot find anything about it on the PI Spec, but I *can* easily
find a few documents referring to the Firmware Volume Protocol:

http://wiki.phoenix.com/wiki/index.php/EFI_FIRMWARE_VOLUME_PROTOCOL

But this has no references at all...

I see stupid patents over some of this and authentication mechanisms for it:

https://patents.google.com/patent/US20170098084

> and so it is undefined whether such blobs are self
> contained (i.e., in separate files in the firmware volume), statically
> linked into the driver or maybe even encrypted or otherwise
> encapsulated, and the actual loadable image only lives in memory.

Got it, thanks this helps! There are two things then:

 1) The "EFI Firmware Volume Protocol" ("FV" for short in your descriptions
below), and whether to support it or not in the future and recommend it
for future use cases.

 b) Han's EFI scraper to help support 2 drivers, and whether or not to
recommend it for future use cases.

> Hans's case is the second one, i.e., the firmware is at an arbitrary
> offset in the driver image. Using the FV protocol in this case would
> result in a mix of both approaches: look up the driver file by GUID
> [which could change btw between different versions of the system
> firmware, although this is unlikely] and then still use the prefix/crc
> based approach to sift through the image itself.

Got it. And to be clear its a reversed engineered solution to what
two vendors decided to do.

> But my main objection is simply that from the UEFI forum point of
> view, there is a clear distinction between the OS visible interfaces
> in the UEFI spec and the internal interfaces in the PI spec (which for
> instance are not subject to the same rules when it comes to backward
> compatibility), and so I think we should not depend on PI at all.

Ah I see.

> This
> is all the more important considering that we are trying to encourage
> the creation of other implementations of UEFI that are not based on PI
> (e.g., uboot for arm64 implements the required UEFI interfaces for
> booting the kernel via GRUB), and adding dependencies on PI protocols
> makes that a moving target.

Got it!

> So in my view, we either take a ad-hoc approach which works for the
> few platforms we expect to support, in which case Hans's approach is
> sufficient, 

Modulo it needs some work for ARM as it only works for x86 right now ;)

> or we architect it properly, in which case we shouldn't
> depend on PI because it does not belong in a properly architected
> OS<->firmware exchange.

OK, it sounds to me like we have room to then implement our own de-facto
standard for letting vendors stuff firmware into EFI as we in the Linux
community see fit.

We can start out by supporting existing drivers, but also consider customizing
this in the future for our own needs, so long as we document it and set
expectations well.

So we need to support what Hans is 

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

[Luis R. Rodriguez]

On Tue, May 08, 2018 at 03:38:05PM +, Luis R. Rodriguez wrote:
> On Fri, May 04, 2018 at 12:44:37PM -0700, Martijn Coenen wrote:
> > On Wed, Apr 25, 2018 at 10:55 AM, Luis R. Rodriguez  
> > wrote:
> > > Android became the primary user of CONFIG_FW_LOADER_USER_HELPER_FALLBACK.
> > >
> > > It would be good for us to hear from Android folks if their current use of
> > > request_firmware_into_buf() is designed in practice to *never* use the 
> > > direct
> > > filesystem firmware loading interface, and always rely instead on the
> > > fallback mechanism.
> > 
> > It's hard to answer this question for Android in general. As far as I
> > can tell the reasons we use CONFIG_FW_LOADER_USER_HELPER(_FALLBACK)
> > are:
> > 1) We have multiple different paths on our devices where firmware can
> > be located, and the direct loader only supports one custom path

FWIW I'd love to consider patches to address this, if this is something
you may find a need for in the future to *avoid* the fallback, however
would like a clean solution.

> > 2) Most of those paths are not mounted by the time the corresponding
> > drivers are loaded, because pretty much all Android kernels today are
> > built without module support, and therefore drivers are loaded well
> > before the firmware partition is mounted

I've given this some more thought and you can address this with initramfs,
this is how other Linux distributions are addressing this. One way to
address this automatically is to scrape the drivers built-in or needed early on
boot in initamfs and if the driver has a MODULE_FIRMWARE() its respective
firmware is added to initramfs as well.

If you *don't* use initramfs, then yes you can obviously run into issues
where your firmware may not be accessible if the driver is somehow loaded
early.

> > 3) I think we use _FALLBACK because doing this with uevents is just
> > the easiest thing to do; our init code has a firmware helper that
> > deals with this and searches the paths that we care about
> > 
> > 2) will change at some point, because Android is moving towards a
> > model where device-specific peripheral drivers will be loaded as
> > modules, and since those modules would likely come from the same
> > partition as the firmware, it's possible that the direct load would
> > succeed (depending on whether the custom path is configured there or
> > not). But I don't think we can rely on the direct loader even in those
> > cases, unless we could configure it with multiple custom paths.

Using initramfs will help, but because of the custom path needs -- you're
right, we don't have anything for that yet, its also a bit unclear if
something nice and clean can be drawn up for it. So perhaps dealing with
the fallback mechanism is the way to go for this for sure, since we already
have support for it.

Just keep in mind that the fallback mechanism costs you about ~13436 bytes.

So, if someone comes up with a clean interface for custom paths I'd love
to consider it to avoid those 13436 bytes.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Luis R. Rodriguez]

On Fri, May 04, 2018 at 12:44:37PM -0700, Martijn Coenen wrote:
> On Wed, Apr 25, 2018 at 10:55 AM, Luis R. Rodriguez  wrote:
> > Android became the primary user of CONFIG_FW_LOADER_USER_HELPER_FALLBACK.
> >
> > It would be good for us to hear from Android folks if their current use of
> > request_firmware_into_buf() is designed in practice to *never* use the 
> > direct
> > filesystem firmware loading interface, and always rely instead on the
> > fallback mechanism.
> 
> It's hard to answer this question for Android in general. As far as I
> can tell the reasons we use CONFIG_FW_LOADER_USER_HELPER(_FALLBACK)
> are:
> 1) We have multiple different paths on our devices where firmware can
> be located, and the direct loader only supports one custom path
> 2) Most of those paths are not mounted by the time the corresponding
> drivers are loaded, because pretty much all Android kernels today are
> built without module support, and therefore drivers are loaded well
> before the firmware partition is mounted
> 3) I think we use _FALLBACK because doing this with uevents is just
> the easiest thing to do; our init code has a firmware helper that
> deals with this and searches the paths that we care about
> 
> 2) will change at some point, because Android is moving towards a
> model where device-specific peripheral drivers will be loaded as
> modules, and since those modules would likely come from the same
> partition as the firmware, it's possible that the direct load would
> succeed (depending on whether the custom path is configured there or
> not). But I don't think we can rely on the direct loader even in those
> cases, unless we could configure it with multiple custom paths.
> 
> I have no reason to believe request_firmware_into_buf() is special in
> this regard; drivers that depend on it may have their corresponding
> firmware in different locations, so just depending on the direct
> loader would not be good enough.

Thanks! This is very useful! This provides yet-another justification and use
case to document for the fallback mechanism. I'll go and extend it.

> >
> > Is ptr below
> >
> > ret = request_firmware_into_buf(_fw, fw_name, dev,
> > ptr, phdr->p_filesz);
> >
> > Also part of the DMA buffer allocated earlier via:
> >
> > ret = qcom_scm_pas_init_image(pas_id, fw->data, fw->size);
> >
> > Android folks?
> 
> I think the Qualcomm folks owning this (Andy, David, Bjorn, already
> cc'd here) are better suited to answer that question.

Andy, David, Bjorn?

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] device property: Get rid of union aliasing

[Andy Shevchenko]

The commit

  318a19718261 ("device property: refactor built-in properties support")

went way too far and brought a union aliasing. Partially revert it here
to get rid of union aliasing.

Note, Apple properties support is still utilizing this trick.

What union aliasing is?
~~~
The C99 standard in section 6.2.5 paragraph 20 defines union type as
"an overlapping nonempty set of member objects". It also states in
section 6.7.2.1 paragraph 14 that "the value of at most one of the
members can be stored in a union object at any time'.

Union aliasing is a type punning mechanism using union members to store
as one type and read back as another.

Why it's not good?
~~
Section 6.2.6.1 paragraph 6 says that a union object may not be a trap
representation, although its member objects may be.

Meanwhile annex J.1 says that "the value of a union member other than
the last one stored into" is unspecified [removed in C11].

In TC3, a footnote is added which specifies that accessing a member of a
union other than the last one stored causes "the object representation"
to be re-interpreted in the new type and specifically refers to this as
"type punning". This conflicts to some degree with Annex J.1.

While it's working in Linux with GCC, the use of union members to do
type punning is not clear area in the C standard and might lead to
unspecified behaviour.

More information is available in this [1] blog post.

[1]: https://davmac.wordpress.com/2010/02/26/c99-revisited/

Signed-off-by: Andy Shevchenko 
---

- extend commit message to explain what union aliasing is and why it's not good

 drivers/base/property.c | 99 -
 drivers/firmware/efi/apple-properties.c |  8 +-
 include/linux/property.h| 52 +++--
 3 files changed, 112 insertions(+), 47 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 8f205f6461ed..de19d7cc073b 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -56,6 +56,67 @@ pset_prop_get(const struct property_set *pset, const char 
*name)
return NULL;
 }
 
+static const void *property_get_pointer(const struct property_entry *prop)
+{
+   switch (prop->type) {
+   case DEV_PROP_U8:
+   if (prop->is_array)
+   return prop->pointer.u8_data;
+   return >value.u8_data;
+   case DEV_PROP_U16:
+   if (prop->is_array)
+   return prop->pointer.u16_data;
+   return >value.u16_data;
+   case DEV_PROP_U32:
+   if (prop->is_array)
+   return prop->pointer.u32_data;
+   return >value.u32_data;
+   case DEV_PROP_U64:
+   if (prop->is_array)
+   return prop->pointer.u64_data;
+   return >value.u64_data;
+   default:
+   if (prop->is_array)
+   return prop->pointer.str;
+   return >value.str;
+   }
+}
+
+static void property_set_pointer(struct property_entry *prop, const void 
*pointer)
+{
+   switch (prop->type) {
+   case DEV_PROP_U8:
+   if (prop->is_array)
+   prop->pointer.u8_data = pointer;
+   else
+   prop->value.u8_data = *((u8 *)pointer);
+   break;
+   case DEV_PROP_U16:
+   if (prop->is_array)
+   prop->pointer.u16_data = pointer;
+   else
+   prop->value.u16_data = *((u16 *)pointer);
+   break;
+   case DEV_PROP_U32:
+   if (prop->is_array)
+   prop->pointer.u32_data = pointer;
+   else
+   prop->value.u32_data = *((u32 *)pointer);
+   break;
+   case DEV_PROP_U64:
+   if (prop->is_array)
+   prop->pointer.u64_data = pointer;
+   else
+   prop->value.u64_data = *((u64 *)pointer);
+   break;
+   default:
+   if (prop->is_array)
+   prop->pointer.str = pointer;
+   else
+   prop->value.str = pointer;
+   }
+}
+
 static const void *pset_prop_find(const struct property_set *pset,
  const char *propname, size_t length)
 {
@@ -65,10 +126,7 @@ static const void *pset_prop_find(const struct property_set 
*pset,
prop = pset_prop_get(pset, propname);
if (!prop)
return ERR_PTR(-EINVAL);
-   if (prop->is_array)
-   pointer = prop->pointer.raw_data;
-   else
-   pointer = >value.raw_data;
+   pointer = property_get_pointer(prop);
if (!pointer)
return ERR_PTR(-ENODATA);
if (length > prop->length)
@@ -698,16 +756,17 @@ EXPORT_SYMBOL_GPL(fwnode_property_get_reference_args);