Add support for Dell TPM 1.2/2.0 updates and initial support for dock information #49

superm1 · 2016-05-20T22:46:47Z

Dell systems from 2015 support field update-able TPMs that also can switch modes. This adds support for them including switching modes via the newly added unlock mechanisms.

Dell TB15 and WD15 docks can also be updated on Dell systems with Type-C. This is the initial work necessary to be able to support polling information about their updates. Currently a virtual dock device is created when a USB device on the dock (Realtek NIC specific VID/PID) is detected.

This isn't really the ideal end approach for a variety of reasons. The main one is there are multiple individual firmware components that can be updated on the dock via the same payload mechanism and flash mechanism. This means that you really want to query all the components to evaluate whether an update should be offered.

What would be most ideal is creating 3-4 devices that represent the different pieces of the dock, but then only allowing "updates" to a single component of the dock at a given time. If this is the approach taken, I would like to specify the version of each component and match it to a GUID but put them all in the same .CAB file and point them all to the same binary in that .CAB file.

To make matters more complicated, the dock EC's can't communicate the versions of all of these components properly until after the first field update. I think this corner case just needs to hardcode the version of one of the other dock devices to 0.0.0.0 (as it's currently doing with the single dock device) to ensure it gets the first update.

superm1 · 2016-05-20T22:47:55Z

Oh it's worth mentioning - this is all dependent upon rhboot/fwupdate#52 as well.

hughsie · 2016-05-23T11:43:11Z

I think this looks good in concept, and I've pushed a few cleanup patches. Good stuff, but a bit more to do before we can merge I guess.

superm1 · 2016-05-23T15:23:51Z

Thanks. Something else that was recently discussed that should be sorted before merging:
The way the TPM device is built is not future proofed.
Before gen7 no system supported this TPM mode switch and flashability. I'm not very concerned about invalid older devices showing the TPM nodes when they have a discrete TPM since the DACI call should return -2 on older systems (but it should be confirmed at some point).

The main concern is that for some future system if the TPM chip model or vendor changed in a future generation that the wrong device nodes get created on a new system with older distro code and the wrong thing offered. Since the TPM payload GUIDs for this model/vendor combination can't be queried from something like the ESRT this needs to be checked. There's 3 ways I'm exploring.

Don't create devices with the TPM payload GUID, but rather with the system ID and something informative about the TPM mode. Something like ("%x-%d", system_id, mode)
Then on the Dell side when we build the CAB for TPM updates we build 15 or so entries in the XML the same way. We would maintain the list of supported devices on the Dell side. The big advantage here is no fwupd code changes needed when new systems come out. If it's just new systems supported by same payload GUIDs, we spin the CAB again. if it's a new CAB we put new systems in the new CAB.

The validation of the payload's GUID happens later on after it's unpacked (and the embedded GUID is pulled out) rather than checking the capsule header against the ESRT when UpdateCapsule() is called.
So we might be able to drop the need for rhboot/fwupdate#52 and just stuff this into the existing ESRT GUID of the system instead, but that might cause confusion if a UEFI system payload update and a Dell TPM update both came down at the same time. Not so sure how fwupdate would handle staging both payloads if they are both using the same ESRT GUID. That might need to be checked.

In any case this approach is my preference if it works and it sounds workable on your end too.

DACI call to query a GUID's flashability. This does exist, but it's been historically used for the system payload. I have to investigate if it's results can be trusted for TPM. If this was used, newer systems would just say that the GUID's that are hardcoded don't work on this system. The code would be needed to be changed for any new GUID's - but that is what I would expect.

This means fwupd changes when a new GUID is created, but at least no one would see the wrong thing on new systems. Second choice.

Hardcode a whitelist. This isn't really ideal to me as it means maintaining the whitelist at all times in fwupd code.

It also means fwupd changes when new devices are released. Last choice.

superm1 · 2016-05-25T06:27:31Z

Those commits hopefully handle the TPM case I referred to above nicely. The dependency upon rhboot/fwupdate#52 is now optional. When it's not present the update is stuffed into the ESRT GUID through libfwup.

This does bring up a corner case that a UEFI update and TPM or dock update can't be delivered simultaneously when compiled without that support to set a different GUID in fwupdate.

superm1 · 2016-05-27T14:11:14Z

Alright so with this latest set of commits, all the child components for the dock are created independently. The behavior before the docks are flashed for the first time (invalid EC version) will be a bit different than future flashes.

In my mind TPM is complete now. If there is anything else you see missing, please let me know.

Remaining dock items from my perspective:

Actually apply an update to a dock with an invalid EC version using this interface, make sure it works properly.
Verify child interfaces continue to populate properly after an update is done.
Update test suite to show another test for a dock that has a valid EC version.
Lock down all dock other devices while applying an update for a single component. Or maybe keep track of the payload that was applied and just don't apply it twice? I'm not sure the right way to do this. Comments welcome.

hughsie · 2016-05-31T09:43:19Z

On 27 May 2016 at 15:11, Mario Limonciello notifications@github.com wrote:

In my mind TPM is complete now. If there is anything else you see missing, please let me know.

I'm wondering if we should have a SIMULATE flag in FwupdInstallFlags,
and maybe we need a way to test the providers in a general way, but I
guess that's more of a "me" problem.

I do like the fact you added so many self tests, good job.

Remaining dock items from my perspective:

Actually apply an update to a dock with an invalid EC version using this interface, make sure it works properly.

Right! :)

Lock down all dock other devices while applying an update for a single component.

I think that's the most logical thing to do; I hope gnome-software
will do the right thing when devices go from unlocked->locked. I guess
we could communicate that better in the UI.

In related news, I've pushed a few fixes to the LVFS that mean you can
upload the dock.cab file -- the fix was
https://github.com/hughsie/lvfs-website/commit/10fc5fb225de003fb5b934fd56cb4d83a0739233

If you could restrict any uploads of this kind of file to embargo
that'd be great; I don't think gnome-software will blow up but I'd
rather test this carefully. Thanks!

Richard.

superm1 · 2016-05-31T14:39:45Z

On Tue, May 31, 2016 at 4:43 AM Richard Hughes notifications@github.com
wrote:

I'm wondering if we should have a SIMULATE flag in FwupdInstallFlags,
and maybe we need a way to test the providers in a general way, but I
guess that's more of a "me" problem.

Since most of the complexity in the Dell provider and it's tests is in the
device creation and interactions with virtual devices (around unlocking),
maybe InstallFlags isn't the right way for a general simulation
infrastructure.

I do like the fact you added so many self tests, good job.

Thanks.

Remaining dock items from my perspective:

Actually apply an update to a dock with an invalid EC version using
this interface, make sure it works properly.

Right! :)

So I did this already with a CAB that didn't have so many entries in it.
In general the interfaces worked properly and the dock updated.

Lock down all dock other devices while applying an update for a
single component.

I think that's the most logical thing to do; I hope gnome-software
will do the right thing when devices go from unlocked->locked. I guess
we could communicate that better in the UI.

I think this will come down to how things are changed to handle when
"Install" is called on a .CAB that has a bunch of matching devices too. As
you know right now Install can't handle that case. I think right now the
update helper method will return an error if the first device found in the
CAB is locked or the version doesn't match something valid. I think errors
like that should be returned if "none" of the devices matching in the CAB
can be updated. They should just be warnings output "Skipping device
update %s due to problem %s".

I don't think that information about skipping devices is even valuable in
gnome-software's UI. It should really only be output when using fwupdmgr.

In related news, I've pushed a few fixes to the LVFS that mean you can
upload the dock.cab file -- the fix was

https://github.com/hughsie/lvfs-website/commit/10fc5fb225de003fb5b934fd56cb4d83a0739233

If you could restrict any uploads of this kind of file to embargo
that'd be great; I don't think gnome-software will blow up but I'd
rather test this carefully. Thanks!

Sure thing. It looks like it uploads to embargo fine now, but something a
bit peculiar I noticed is get-details outputs in the order of the XML
files, but LVFS organizes it in some other order I can't figure out. Eg the
first thing that shows up is the "Dell WD15 Dock EC update" even though the
first XML file shows "Dell package version update". This means when you
look at https://secure-lvfs.rhcloud.com/lvfs/firmware the EC update is the
child node that everything from that CAB is categorized under.

hughsie · 2016-05-31T18:57:45Z

On 31 May 2016 at 15:39, Mario Limonciello notifications@github.com wrote:

Since most of the complexity in the Dell provider and it's tests is in the
device creation and interactions with virtual devices (around unlocking),
maybe InstallFlags isn't the right way for a general simulation
infrastructure.

Agree.

I think this will come down to how things are changed to handle when
"Install" is called on a .CAB that has a bunch of matching devices too. As
you know right now Install can't handle that case.

What does it do now exactly? What do you think should happen exactly?

I don't think that information about skipping devices is even valuable in
gnome-software's UI. It should really only be output when using fwupdmgr.

Right; I guess fwupd should know about how devices are dependent on
one another. I guess kinda like equivalent_id but exposed on the
interface.

Sure thing. It looks like it uploads to embargo fine now, but something a
bit peculiar I noticed is get-details outputs in the order of the XML
files, but LVFS organizes it in some other order I can't figure out.

I'd imagine in the order the files were packed into the .cab file. Do
you think we should do what we're doing now (i.e. show a parent device
at random), sort the components more predictably, or just show all the
components for each GUID. I actually think it's more of a bug than any
conscious design decision.

Richard.

superm1 · 2016-05-31T19:52:08Z

I think this will come down to how things are changed to handle when
"Install" is called on a .CAB that has a bunch of matching devices too.
As
you know right now Install can't handle that case.

What does it do now exactly? What do you think should happen exactly?

Last week when I tried this it was matching the first device in the cab.
It just so happened the first device in the CAB had no version (it was the
invalid EC case) so it said something along the lines the device doesn't
have a defined version so the update can't be applied.
I did
6a8c18c
to
work around that issue and just not create virtual devices unless there is
a defined version.
This meant it matched the next device which happened to have the same
version as what I was trying to update and fell over.

I'd expect keep trying all matching devices until something is updatable
(unlocked, has a version, and new enough version or --allow-older was
set). If that updatable one fails, return an error. If that one passes
keep trying the rest that are available and match. That would mean the
provider should lock out the others during the update.

I don't think that information about skipping devices is even valuable in
gnome-software's UI. It should really only be output when using fwupdmgr.

Right; I guess fwupd should know about how devices are dependent on
one another. I guess kinda like equivalent_id but exposed on the
interface.

Well I don't think you need to go this far necessarily. A simple loop
around the Install method to check all the devices in the CAB that is being
installed to those on the system. On valid devices store any error found
in the middle to display as a warning. If nothing is successful return an
error that is a summary of what happened with the various components.
If something is successful just output the warnings on the console, but
return success.

At least with the Dell provider I'd expect it to work:

that it probably matches the same version on some devices. Those get
skipped by a version check
It finds something valid and applies an update. During the update the
dell provider locks all the other devices
Install is still looping and skips the newly locked devices.

That means i'll need to make the Dell provider still:

Lock those devices on install (and internally know associations).
Display an error if they're attempted to be unlocked before system is
rebooted to install.

Sure thing. It looks like it uploads to embargo fine now, but something a

bit peculiar I noticed is get-details outputs in the order of the XML
files, but LVFS organizes it in some other order I can't figure out.

I'd imagine in the order the files were packed into the .cab file. Do
you think we should do what we're doing now (i.e. show a parent device
at random), sort the components more predictably, or just show all the
components for each GUID. I actually think it's more of a bug than any
conscious design decision.

Richard.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#49 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAeLvjsoO6zTmTWcfGn-zaV3uejN1wFzks5qHISpgaJpZM4Ijnr4
.

superm1 · 2016-06-22T21:20:32Z

Once #53 (or some form of this) is merged, the only remaining thing from my perspective on the Dell provider being merged is to have it lock common devices upon installation and display an error if you try to unlock.

superm1 · 2016-07-12T20:57:11Z

Since #53 is closed, I checked on the locking common devices. This actually doesn't appear necessary. Calling into fwupdate again doesn't cause any problems since it's the same payload again.

The MST and TBT NVM will be features to be added later dependent upon other userspace and kernel work.

So Dell provider should be complete now and ready to merge.

hughsie · 2016-07-13T08:06:25Z

Can you rebase, merge some of the fixup commits down, and force push this branch to update the PR please. Then I'll review one last time and we can merge. Thanks!

superm1 · 2016-07-13T14:01:56Z

OK, just did that.

On Wed, Jul 13, 2016 at 3:06 AM Richard Hughes notifications@github.com
wrote:

Can you rebase, merge some of the fixup commits down, and force push this
branch to update the PR please. Then I'll review one last time and we can
merge. Thanks!

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#49 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAeLvqgxUtfG9MlykLCMt4mwGpSDz_9eks5qVJyBgaJpZM4Ijnr4
.

hughsie · 2016-07-13T15:27:14Z

configure.ac

@@ -181,6 +181,26 @@ if test x$enable_uefi != xno; then
 fi
 AM_CONDITIONAL(HAVE_UEFI, test x$enable_uefi = xyes)

+# Dell Non ESRT capsule support
+AC_ARG_ENABLE(rbu, AS_HELP_STRING([--enable-dell],[Enable Dell non-esrt capsule support]),


probably best with caps, i.e. ESRT

hughsie · 2016-07-13T15:33:09Z

src/fu-provider-dell.c

+	 * Querying the version is also not available at this time
+	 */
+	/* TB15 NVM */
+	if (buf.record->dock_info_header.dock_type == DOCK_TYPE_TB15){


space after bracket

hughsie · 2016-07-13T15:38:39Z

Quite a few nits to deal with, but when all those are fixed up I think it's time to push to master. Thanks!

This provider will provide support for items that can be flashed as capsules but aren't present in the ESRT table. The MST hub and TBT NVM are not yet updatable, but GUIDs are created to represent them when they are.

superm1 · 2016-07-13T17:32:09Z

OK hopefully that should cover all those nits.

side note, I need a plugin for my editor that alerts me about style type stuff. Get mixed up easily on different projects I contribute to :)

hughsie · 2016-07-18T08:05:00Z

So moving out those structs from the .h broke the self tests :( Any chance you could export the bits required to the .h file please? Thanks.

superm1 · 2016-07-18T14:37:47Z

Ah darn, sorry should have ran make check again after shuffling it all around. Moved the relevant important ones back over.

superm1 force-pushed the wip/superm1/dell-support branch from e85e8e0 to a1b7862 Compare May 22, 2016 00:43

superm1 mentioned this pull request Jun 1, 2016

tracking thunderbolt support for docks intel/thunderbolt-software-user-space#2

Closed

superm1 force-pushed the wip/superm1/dell-support branch from 4636ccf to d8965bc Compare July 13, 2016 13:56

superm1 force-pushed the wip/superm1/dell-support branch from d8965bc to 102b6c8 Compare July 13, 2016 14:58

hughsie reviewed Jul 13, 2016
View reviewed changes

superm1 force-pushed the wip/superm1/dell-support branch from 102b6c8 to 3d11bba Compare July 13, 2016 17:27

Mario Limonciello and others added 2 commits July 13, 2016 12:30

Add Dell TPM and TB15/WD15 support via new Dell provider.

958ead6

This provider will provide support for items that can be flashed as capsules but aren't present in the ESRT table. The MST hub and TBT NVM are not yet updatable, but GUIDs are created to represent them when they are.

trivial: Add libsmbios-devel to the BRs in the example spec file

ffb99c6

superm1 force-pushed the wip/superm1/dell-support branch from 3d11bba to ffb99c6 Compare July 13, 2016 17:30

hughsie merged commit 0a89b61 into master Jul 14, 2016

hughsie deleted the wip/superm1/dell-support branch July 14, 2016 07:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Dell TPM 1.2/2.0 updates and initial support for dock information #49

Add support for Dell TPM 1.2/2.0 updates and initial support for dock information #49

superm1 commented May 20, 2016

superm1 commented May 20, 2016

hughsie commented May 23, 2016

superm1 commented May 23, 2016 •

edited

Loading

superm1 commented May 25, 2016

superm1 commented May 27, 2016

hughsie commented May 31, 2016

superm1 commented May 31, 2016

hughsie commented May 31, 2016

superm1 commented May 31, 2016

superm1 commented Jun 22, 2016

superm1 commented Jul 12, 2016

hughsie commented Jul 13, 2016

superm1 commented Jul 13, 2016

hughsie Jul 13, 2016

hughsie Jul 13, 2016

hughsie commented Jul 13, 2016

superm1 commented Jul 13, 2016

hughsie commented Jul 18, 2016

superm1 commented Jul 18, 2016

Add support for Dell TPM 1.2/2.0 updates and initial support for dock information #49

Add support for Dell TPM 1.2/2.0 updates and initial support for dock information #49

Conversation

superm1 commented May 20, 2016

superm1 commented May 20, 2016

hughsie commented May 23, 2016

superm1 commented May 23, 2016 • edited Loading

superm1 commented May 25, 2016

superm1 commented May 27, 2016

hughsie commented May 31, 2016

superm1 commented May 31, 2016

hughsie commented May 31, 2016

superm1 commented May 31, 2016

superm1 commented Jun 22, 2016

superm1 commented Jul 12, 2016

hughsie commented Jul 13, 2016

superm1 commented Jul 13, 2016

hughsie Jul 13, 2016

Choose a reason for hiding this comment

hughsie Jul 13, 2016

Choose a reason for hiding this comment

hughsie commented Jul 13, 2016

superm1 commented Jul 13, 2016

hughsie commented Jul 18, 2016

superm1 commented Jul 18, 2016

superm1 commented May 23, 2016 •

edited

Loading