Wip/hughsie/nvme #666

hughsie · 2018-08-16T14:36:02Z

No description provided.

hughsie · 2018-08-16T14:36:16Z

UNTESTED!

superm1 · 2018-08-16T14:40:54Z

plugins/nvme/fu-plugin-nvme.c

+	/* create device */
+	fu_device_add_flag (dev, FWUPD_DEVICE_FLAG_INTERNAL);
+	fu_device_add_flag (dev, FWUPD_DEVICE_FLAG_UPDATABLE);
+	fu_device_add_flag (dev, FWUPD_DEVICE_FLAG_REQUIRE_AC);


This collection of flags I don't think is necessarily true. Since NVMe drives are PCIe device, it's certainly possible to have them in a Thunderbolt device or thunderbolt PCIe enclosure. To really prove they are internal/require-ac you'd have to look at the topology to see if they go through a thunderbolt bridge.

I don't think that's worth it though.

Well, we use INTERNAL to decide some security stuff, so maybe it is worth discovering.

Are there any good heuristics for the topology walking? I assume we just need to look at the bridges and root controllers.

Maybe @gicmo or @YehezkelShB have some ideas here. My beset guess for a heuristic would be to match the thunderbolt NHI uevent to find it's parent and then see if the parent is in common.

superm1

Well that was quick. I guess since you have a json dump in the directory it would be good to read that in and with a self test script (and maybe call /bin/true with it to test "flash process" rather than nvme)

superm1 · 2018-08-16T14:41:24Z

plugins/nvme/fu-plugin-nvme.c

+		return FALSE;
+	}
+	nvme_frmw = json_object_get_int_member (obj, "frmw");
+	if ((nvme_frmw & 0x08) == 0)


This is a magic number, maybe a #define ?

superm1 · 2018-08-16T14:43:24Z

plugins/nvme/fu-plugin-nvme.c

+	if (g_strv_length (nvme_mn_split) == 2) {
+		if (g_strcmp0 (nvme_mn_split[1], "TOSHIBA") == 0 ||
+			   g_strcmp0 (nvme_mn_split[1], "TOS") == 0 ||
+			   g_strcmp0 (nvme_mn_split[1], "TO") == 0) {


This sorta feels like it's going to grow into a pretty big list, how about just storing this in quirk data so it's easy to match?

It's really only Toshiba that does it the wrong way around, but I do agree this kind of thing needs to go to the quirk database. Will fix.

superm1 · 2018-08-16T14:47:37Z

plugins/nvme/fu-plugin-nvme.c

+
+	/* write blob to temp location */
+	tmpdir = g_dir_make_tmp ("fwupd-XXXXXX", error);
+	if (tmpdir == NULL)


It's not obvious to me what permissions this temp directory gets created with g_dir_make_tmp. Is it only writable by the user ID of the process that created it? If not, then I think you'll need to adjust that before the file is written.

I guess we don't actually need a directory, just a file. I suppose we could even write this to TMPDIR directly. It's a shame we can't pass an fd and just back the fd using a blob of memory.

Maybe they would take a patch for that though for later and then this plugin can change behavior based on nvme --version

superm1 · 2018-08-16T14:48:41Z

plugins/nvme/fu-plugin-nvme.c

+	fu_device_set_progress (device, 100);
+
+	/* delete temp location */
+	if (!fu_common_rmtree (tmpdir, error))


I think you'll want to do this with a device locker instead to make sure that temp directory gets cleared even if the flash process failed.

superm1 · 2018-08-16T14:53:20Z

Assuming this works I guess we'll want to enact getting it installed by more packages automatically too.
I think that can be followed up later though.

superm1 · 2018-08-16T18:05:52Z

Have you examined any firmware binaries to check how they're constructed? I inquire because I notice this:

The firmware image may be constructed of multiple pieces that
are individually downloaded with separate Firmware Image Download
commands. Each Firmware Image Download command includes a Dword
Offset and Number of Dwords that specify a Dword range. The host
software shall ensure that firmware pieces do not have Dword ranges
that overlap. Firmware portions may be submitted out of order to the
controller.

If that's a common situation then this might be difficult to represent in the plugin.

hughsie · 2018-08-16T19:19:18Z

If that's a common situation then this might be difficult to represent in the plugin.

I've never seen an image with more than one chunk, and I've never seen an image that had an offset that wasn't 0. If we did have to support such a thing I'd urge the vendor to ship a DfuSe binary, that can have multiple images, each with a 32bit offset, and additional metadata. This could then be parsed in the plugin and each image applied.

If you think that's a sensible thing to support out of the box (or if you know of a NVMe binary with more than one chunk) just say and I'll add it to the plugin.

superm1 · 2018-08-16T19:23:20Z

If you think that's a sensible thing to support out of the box (or if you know of a NVMe binary with more than one chunk) just say and I'll add it to the plugin.

If it's not common I don't think it's sensible to support. I haven't analyzed any of the binaries we use yet, so I don't know if that's something we have to worry about.

hughsie · 2018-08-20T19:37:31Z

It wasn't hard to use the ioctl to read the CNS without using nvme-cli. I think this version is much better. Could you re-review please? Thanks.

superm1 · 2018-08-21T17:13:57Z

plugins/nvme/fu-nvme-device.c

+
+#include "dfu-chunked.h"
+
+#include "fu-nvme-device.h"


I think you want to re-order these headers, wasn't the convention we were going for to have "local" first followed by system?

I think it's config.h, system, local.

Hmm, I guess that matches what we discussed previously (#568 (review)) I just remembered backwards.

superm1 · 2018-08-21T17:16:48Z

plugins/nvme/fu-nvme-device.c

+
+#include <glib/gstdio.h>
+
+#include <linux/nvme_ioctl.h>


So this adds an implicit dependency of kernel 4.4 right? Unless you end up wanting to run this plugin on RHEL 7.x it would be good to check at plugin startup for at least this kernel version and fail otherwise.

Ahh, I should check for this in meson.build rather relying on a specific kernel version -- good catch, thanks.

superm1 · 2018-08-21T17:24:31Z

plugins/nvme/fu-nvme-device.c

+fu_nvme_device_init (FuNvmeDevice *self)
+{
+	fu_device_add_flag (FU_DEVICE (self), FWUPD_DEVICE_FLAG_REQUIRE_AC);
+	fu_device_add_flag (FU_DEVICE (self), FWUPD_DEVICE_FLAG_UPDATABLE);


Weren't we talking about making this updatable flag a whitelist (eg quirked) until OEM vs aftermarket is sorted out?

Yes, I still think that's best. Having it as a quirk still makes it hard to test tho. Maybe we can use --force if something isn't in the whitelist?

Yeah I think that's good for now.

I've done the force thing. Do you think it makes sense to whitelist based on model, vendor or guid? Or leave the whitelist to later?

superm1 · 2018-08-21T17:28:21Z

Looks like the remaining CI failures are all real things (small though).

superm1 · 2018-08-21T17:54:26Z

plugins/nvme/fu-nvme-device.c

+	}
+	if (!fu_nvme_device_parse_cns (self, buf, sizeof(buf), error))
+		return FALSE;
+


I think it would be good to add a function that at least dumps the vendor block into debug output until we know what to do with it.

All 1024 bytes of it?

Maybe keep the printing of it behind FU_NVME_VERBOSE or similar. I think it would be good when we have some parsing methods to try to throw at it though to have the plumbing around it.

superm1 · 2018-08-23T17:32:45Z

plugins/nvme/fu-nvme-device.c

+	g_autofree gchar *guid_id = NULL;
+
+	/* add extra component ID if set */
+	component_id = fu_nvme_device_get_string_safe (buf, 0xc36, 0xc3d);


The start offset is right, but this should be 8 long not 7 long.

Yup, it is. It's "inclusive" --hence the + 1 in the fu_nvme_device_get_string_safe() length calculation. I did it that way to match the constants in the NVMe spec. If it's confusing I can add a comment.

Ah yes, a comment would be good.

superm1 · 2018-08-23T17:33:43Z

Tested it on a Dell disk that does export both an EFI GUID and Component ID, it did build the GUIDs properly. Haven't tried flashing yet though, waiting for a new binary to do that.

superm1 · 2018-08-23T17:35:49Z

plugins/nvme/fu-nvme-device.c

+	/* check the GUID is plausible */
+	for (guint i = 0; i < 16; i++)
+		c_cnt += buf[addr_start + i];
+	if (c_cnt < 0xff)


Is this really true? all EFI guids add up to something smaller than 255? I'd never heard that, but it's great to know if it is.

Nope, that's completely a made up rule. I figured statistically the GUID would have an average summed value of 2032, and that GUIDs like 01000200-0000-0300-0010-080020000a00 would be statistically unlikely and it probably indicates it's not actually a GUID.

Sounds pretty good.
What would you think about putting in a warning for this situation that it found some data but that it doesn't look like a GUID? That way at least if we find this happening in the wild we can see warnings in fwupd.log to go with it.

Well, as I understand it there are quite a few drives with component IDs and not GUIDs, so wouldn't a lot of drives have warnings? Are the early drives going to have zeros for the GUID? I guess we could avoid a warning for that.

Yeah they should have 0's.

Okay, fixed.

superm1 · 2018-08-23T17:38:31Z

plugins/nvme/fu-nvme-device.c

+	fu_nvme_device_parse_cns_maybe_dell (self, buf);
+
+	/* fall back to the device description */
+	if (fu_device_get_guids (FU_DEVICE (self))->len == 0) {


In your data set were you able to check if any other vendors 'vs' blobs set off the Dell creation of a Dell GUID? With this approach I would just wonder if another vendor vs section has a set of bytes that could represent an EFI guid at that offset) (but doesn't actually) what happens.

It takes more than the plausible GUID, it also takes a plausible component ID string too.

OK, I guess that makes sense then for this. It would only apply to "newer" disks (that only have more than one field), but it's safer.

hughsie · 2018-08-26T18:44:37Z

Fixed to actually make updates work, and merged, rebasing against wip/hughsie/gudev. Thanks for all the reviews.

superm1 reviewed Aug 16, 2018

View reviewed changes

hughsie force-pushed the wip/hughsie/nvme branch from d5bc8c7 to 255535c Compare August 20, 2018 19:35

superm1 reviewed Aug 21, 2018

View reviewed changes

hughsie force-pushed the wip/hughsie/nvme branch 6 times, most recently from 45ba5f6 to 705789a Compare August 23, 2018 16:30

nvme: Add a new plugin to update NVMe hardware

8d4eae4

hughsie force-pushed the wip/hughsie/nvme branch from 705789a to 8d4eae4 Compare August 23, 2018 16:55

superm1 reviewed Aug 23, 2018

View reviewed changes

hughsie and others added 3 commits August 23, 2018 21:48

f

b3658f1

disable nvme plugin for debian too

e5df680

f

92e3e08

hughsie closed this Aug 26, 2018

hughsie deleted the wip/hughsie/nvme branch August 26, 2018 18:44


		#include "dfu-chunked.h"

		#include "fu-nvme-device.h"


		#include <glib/gstdio.h>

		#include <linux/nvme_ioctl.h>

Wip/hughsie/nvme #666

Wip/hughsie/nvme #666

Conversation

hughsie commented Aug 16, 2018

hughsie commented Aug 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superm1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superm1 commented Aug 16, 2018

superm1 commented Aug 16, 2018

hughsie commented Aug 16, 2018

superm1 commented Aug 16, 2018

hughsie commented Aug 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superm1 commented Aug 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superm1 commented Aug 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superm1 Aug 23, 2018 • edited

Choose a reason for hiding this comment

hughsie commented Aug 26, 2018

superm1 Aug 23, 2018 •

edited