Added NBFT table support #1791

johnmeneghini · 2023-02-02T22:33:17Z

Added support for parsing and printing the contents of the NBFT table (per NVMe-oF boot specification v1.0).

 $ .build/nvme nbft --help
 nvme-2.4
 usage: nvme nbft  command [device] [args]
 
 The  <device> may be either an NVMe character device (ex: /dev/nvme0), an 
 nvme block device (ex: /dev/nvme0n1), or a mctp address in the form
 mctp net,eid ctrl-id 

 ACPI NBFT table extensions

 The following are all implemented sub-commands:
   show            Show contents of ACPI NBFT tables
   version         Shows the program version
   help            Display this help

 See nvme nbft help <command> for more information on a specific command

 $ .build/nvme nbft show --help
 Usage: nvme nbft show <device> [OPTIONS]

 Display contents of the ACPI NBFT files.

 Options:
   [  --output-format=<FMT>, -o <FMT> ]  --- Output format: normal|json
   [  --subsystem, -s ]                  --- show NBFT subsystems
   [  --hfi, -H ]                        --- show NBFT HFIs
   [  --discovery, -d ]                  --- show NBFT discovery controllers
   [  --nbft-path=<STR> ]                --- user-defined path for NBFT tables

Signed-off-by: Stuart Hayes stuart_hayes@dell.com
Signed-off-by: Martin Belanger martin.belanger@dell.com
Signed-off-by: Tomas Bzatek tbzatek@redhat.com
Signed-off-by: Martin Wilck mwilck@suse.com
Signed-off-by: John Meneghini jmeneghi@redhat.com

igaw · 2023-02-03T10:39:19Z

Just two quick remarks as we have to agree on this first (the implementation can be changed update/changed without breaking any users):

I think this should be a plugin and not a top level command (e.g. like zns).
The next thing is the name, nbft stand for NVMe Boot Firmware Table. Is there any plans to extend this in future? Would it make sense to name the plugin firmware and have a subcommand called nbft?

mwilck · 2023-02-03T11:26:33Z

Would it make sense to name the plugin firmware and have a subcommand called nbft?

This would result in 3-level subcommands like nvme firmware nbft show . I don't think we need this complexity. Currently the "firmware" command would support NBFT only. If any time in the future we get a different FW API, firmware supporting that API will most likely not support NBFT, and the kind of API used by the firmware could be an implementation detail hidden by the nvme firmware command.

Let's summarize the functionality we need, independent of the FW implementation:

print configuration from firmware in machine-readable format (JSON), today nvme show-nbft -o json;
print configuration from firmware in human-readable format, currently nvme show-nbft -o normal;
connect to subsystems configured by the firmware, currently nvme connect-nbft;
connect to everything, including firmware-configured subsystems;
connect to everything, excluding firmware-configured subsystems (for troubleshooting).

No. 4) and 5) would need integration of NBFT support in the standard subcommands connect and/or connect-all. 3) could be implemented by nvme firmware connect, or e.g. using nvme connect --firmware.

igaw · 2023-02-03T11:54:15Z

a) I was wondering if nbtf is a good name (== plugin name), e.g. nvme firmware show and not nvme firmware nbtf show.
b) We currently have two main connect user interfaces, nvme connect and nvme connect-all. Introducing yet another connect call is confusing from the user point of view, e.g how does it interact with the existing connect and connect-all command and why does connect-all not do what it says in the name?

I think a) is not really controversial just the question of the name. I don't mind going with nbft.

The bigger issue for me is b). This needs a strong argument why it should exists in the first place. This includes documentation how to use it correctly (if we agree it should exist).

igaw · 2023-02-03T14:43:44Z

Just to make it clear: Why do we need another 'connect' command? This needs to be explained. What are the use cases?

mwilck · 2023-02-03T22:24:37Z

I've pondered this some more. The main difference between connect and connect-all is that the former takes an NQN argument while the latter does not. connect-all does take other options that limit the selection of discovery controllers to retrieve discovery information from, though. The NBFT could be seen as a source of "discovery" information. Thus it would make sense to use nvme connect-all --nbft rather than nvme connect --nbft. connect-all, without arguments, would include the NBFT. For troubleshooting, we might also need nvme connect-all --no-nbft. Thus we would end up with the following new commands:

nvme nbft show 
nvme connect-all --nbft
nvme connect-all --no-nbft

Idea: we could also add nvme discover --nbft, which would print the subsystem information from the NBFT in nvme discover format. This would allow us to keep the analogy between discover and connect-all. We could add this in a later PR if we agree on it.

igaw · 2023-02-06T08:55:28Z

About the name of the plugin: I agree firmware is not a good name as it is more associated with actually firmware on a device. As I said nbft is okay just a bit hard to type and remember what it is.
nvme connect and nvme connect-all are only taking options, no command line arguments. In fact both accept the same options.
Given the argument, if there is need for --nbtf and --no-nbtf, in this case I think both commands need to handle them. I am not sure, because no exact description what they are doing and how the interact with any of the existing options is explained. Maybe it's just me understanding things wrong. But I rather have these thing explicitly discussed and documented now because changing it later is more pain.

igaw · 2023-02-08T14:35:32Z

So the argument is that we only need either nvme connect --nbtf or nvme connect-all --nbtf and your preference is that nvme connect-all --nbtf makes more sense because it connects all connection listed in the table. Alright, then do this and let's how this goes. I don't care too much anyway, except I don't want another connect command and that is what I tried to say from the beginning on.

hreinecke · 2023-02-08T15:28:30Z

Actually, I would do it exactly the other way round.
'nvme connect-all' really should be doing what it advertised, namely connecting to all sources. And all sources should include nbft one.
'nvme connect', OTOH, requires the user to specify where to connect to, so having an '--nbft' argument here is far more sensible.

mwilck · 2023-02-08T15:35:31Z

nbft.c

+				}
+
+				if (ret)
+					fprintf(stderr, "no controller found\n");


We should handle the case errno == ENVME_CONNECT_ALREADY here. Dracut may need to do multiple connection attempts. "no controller found" is not a helpful error message if the subsystem at hand is already connected.

There is nvme_errno_to_string() which translates the error codes into something more useful.

We might actually consider not returning an error if we find that (all?) connections specified in the NBFT are already set up.

FWIW nvme connect-all ignores already connected targets. It only logs the warning.

@mwilck are we good here? Do you still want to make this change?

mwilck · 2023-02-08T15:38:25Z

nbft.c

+	nbft_json = json_create_object();
+	if (!nbft_json)
+		return NULL;
+


We should add a "version" field to the json output. This way, if the format of the json changes in the future, consumers will be able to detect it easily.

In this case the schema and parser in libnvme needs to be updated accordingly.

I was referring to the format of the JSON only here, i.e. the API presented to consumers of the JSON by nvme-cli. Currently the code for converting the NBFT to JSON is only in nvme-cli, not in libnvme. So I'm unsure what changes in schema and parser you're referring to.

You are right that we should reflect possible changes in the NBFT format itself, too. So maybe we need to version fields, or a 2-digit version, one part indicating the version of the NBFT, and the other the version of the JSON output. When the NBFT format changes, the JSON will almost certainly change as well, but not vice versa.

I was referring the JSON config file. But this seems to be a different one. Still a schema for it wouldn't hurt, I suppose.

Still a schema for it wouldn't hurt, I suppose.

ack.

I think this is the last remaining open issues.

mwilck · 2023-02-08T15:44:18Z

Actually, I would do it exactly the other way round. 'nvme connect-all' really should be doing what it advertised, namely connecting to all sources.

This applies only to connect-all without arguments / options. connect-all supports plenty of options that would restrict the "all" to a subset of connections only offered by a certain discovery controller.

And all sources should include nbft one. 'nvme connect', OTOH, requires the user to specify where to connect to, so having an '--nbft' argument here is far more sensible.

Hm. AFAICS, the distinctive difference between connect -t $transport -a $traddr -s $trsvcid and connect-all -t $transport -a $traddr -s $trsvcid is that the former requires a -n $subsysnqn option, whereas the latter doesn't. Connecting to the NBFT would not require a subsysnqn option. Therefore I think it's more like connect-all (with options).

I don't want to turn this into a religious matter. If everyone else agrees connect is the way to go, fine with me. I just wouldn't want to have to implement both.

stuarthayes · 2023-02-13T17:26:20Z

I don't have any objections to "connect-all --nbft".

igaw · 2023-02-14T08:54:33Z

As I understood @mwilck, he wants to keep the effort limited hence the wish only to do one connect command. As far I can tell the newly nbtf connect code will life in a separate function and connect or connect-all call it. Given that, it would be simple to add/remove the calls. This feels a bit like bike shedding :)

mwilck · 2023-02-14T09:30:12Z

This feels a bit like bike shedding

I just want clarification how the command line API should look like. And I believe this decision should be made now.

To me, the important thing is that the final command line API provides commands to execute any of the following:

connect everything, including NBFT,
connect everything, except NBFT,
connect NBFT targets only.

igaw · 2023-02-14T12:57:50Z

Let me suggest the following:

To me, the important thing is that the final command line API provides commands to execute any of the following:
1. connect everything, including NBFT,

nvme connect-all

2. connect everything, except NBFT,

nvme connect-all --no-nbft

3. connect NBFT targets only.

nvme connect-all --nbft

edit: I updated the last proposal. So it's what @mwilck suggested in the beginning using the connect-all command. Is everyone happy with this now?

mwilck · 2023-03-25T15:00:44Z

I realize that the code in this PR doesn't build because it doesn't contain the changes to libnvme.wrap that would be necessary to make it build against the Timberland-enhanced libnvnme (basically, cc8f744). I suggest that we add these changes back. We can (and will) still remove them once the Timberland libnvme changes have been merged upstream.

igaw · 2023-03-27T11:48:02Z

FWIW, we can also progress quickly on the libnvme part and then look/update this PR.

johnmeneghini · 2023-03-29T05:24:47Z

I have squashed and rebased both linux-nvme/libnvme#572 and #1791 onto the HEAD of the master branch. After some work on the libnvme.spec file in my rh_linux_poc here I was able to get my fedora RPMs to compile and install. However, testing these new rebased bits in my QEMU POC fails. After rebasing everything to the HEAD of the master branch in libnvme and nvme-cli, these changes no longer work with dracutdevs/dracut#2184 and with timberland-sig/edk2@9e63dc0. I am no longer able to boot my QEMU host with NVMe/TCP using these bits.

johnmeneghini · 2023-04-07T07:34:53Z

Pushed a major update which should address all comments.

Please review.

johnmeneghini · 2023-04-12T14:14:12Z

I have squashed and rebased both linux-nvme/libnvme#572 and #1791 onto the HEAD of the master branch. After some work on the libnvme.spec file in my rh_linux_poc here I was able to get my fedora RPMs to compile and install. However, testing these new rebased bits in my QEMU POC fails. After rebasing everything to the HEAD of the master branch in libnvme and nvme-cli, these changes no longer work with dracutdevs/dracut#2184 and with timberland-sig/edk2@9e63dc0. I am no longer able to boot my QEMU host with NVMe/TCP using these bits.

All issues have been resolved. The newest version of the rh_linux_poc which includes all the needed prebuilt rpms and artifacts used to test this latest nvme-cli changes can be seen here. The dracut patches used with this version of nvme-cli can be seen here. The prebuilt Fedora RPMs are all publicly available in my COPR repository

igaw

Only minor change requests/question

just splitting the patch slightly
questions on some comments in the connect algorithm code.
updating of the man pages.

Looks in great shape

fabrics.c

nbft.c

nvme-builtin.h

plugins/meson.build

To prepare for the addition of nbft functionality fixup the fabrics declarations. Signed-off-by: John Meneghini <jmeneghi@redhat.com>

johnmeneghini · 2023-04-14T18:46:54Z

I've addressed all of @igaw s comments. There are now three commits. Documentation has been added, and checkpatch.pl has been run over everything. Note that I opened #1905 to demonstrait that I have introduced no regression in fabric.c. (The file needs some attention). Everything here runs and passes the nvme/tcp boot sniff test with the rh_linux_poc. The prebuilt Fedora RPMs are all publicly available in my COPR repository here.

[root@host-vm ~]# nvme connect-all --help
Usage: nvme connect-all <device> [OPTIONS]

Discover NVMeoF subsystems and connect to them

Options:
.
.
.
  [  --force ]                          --- Force persistent discovery
                                            controller creation
  [  --nbft ]                           --- Only look at NBFT tables
  [  --no-nbft ]                        --- Do not look at NBFT tables
  [  --nbft-path=<STR> ]                --- user-defined path for NBFT tables

[root@host-vm ~]# nvme nbft --help
nvme-2.4
usage: nvme nbft <command> [<device>] [<args>]

The '<device>' may be either an NVMe character device (ex: /dev/nvme0), an
nvme block device (ex: /dev/nvme0n1), or a mctp address in the form
mctp:<net>,<eid>[:ctrl-id]

ACPI NBFT table extensions

The following are all implemented sub-commands:
  show            Show contents of ACPI NBFT tables
  version         Shows the program version
  help            Display this help

See 'nvme nbft help <command>' for more information on a specific command

[root@host-vm ~]# nvme nbft show
/sys/firmware/acpi/tables/NBFT:

NBFT Subsystems:

Idx|NQN                                                                 |Trsp|Address       |SvcId|HFIs
---+--------------------------------------------------------------------+----+--------------+-----+----
1  |nqn.2014-08.org.nvmexpress:uuid:0c468c4d-a385-47e0-8299-6e95051277db|tcp |192.168.101.20|4420 |1   

NBFT HFIs:

Idx|Trsp|PCI Addr  |MAC Addr         |DHCP|IP Addr       |Mask|Gateway |DNS     
---+----+----------+-----------------+----+--------------+----+--------+--------
1  |tcp |0:0:4.0   |ea:eb:d3:58:89:58|no  |192.168.101.30|24  |0.0.0.0 |0.0.0.0

johnmeneghini · 2023-04-14T18:55:01Z

I realize that the code in this PR doesn't build because it doesn't contain the changes to libnvme.wrap that would be necessary to make it build against the Timberland-enhanced libnvnme (basically, cc8f744). I suggest that we add these changes back. We can (and will) still remove them once the Timberland libnvme changes have been merged upstream.

To build the Timberland sig nvme-cli repository you need to clone the repository, make (which will fail to compile), and then cd subprojects/libnvme; git checkout master. This will git you the updated libnvme library you need to compile and test this pull request.

Added support for parsing the contents of the NBFT table (per NVMe-oF boot specification v1.0) with the connect-all and discover commands. nvme discover/connect-all --nbft ignore /etc/nvme config and use NBFT tables nvme discover/connect-all --no-nbft ignore NBFT tables and use /etc/nvme config nvme discover/connect-all --nbft-path=<STR> user-defined path for NBFT tables Signed-off-by: Stuart Hayes <stuart_hayes@dell.com> Signed-off-by: Martin Belanger <martin.belanger@dell.com> Use the new nbft public API. Signed-off-by: Tomas Bzatek <tbzatek@redhat.com> Signed-off-by: Martin Wilck <mwilck@suse.com> Signed-off-by: John Meneghini <jmeneghi@redhat.com>

Display contents of the ACPI NBFT files. Usage: nvme nbft show <device> [OPTIONS] Options: [ --output-format=<FMT>, -o <FMT> ] --- Output format: normal|json [ --subsystem, -s ] --- show NBFT subsystems [ --hfi, -H ] --- show NBFT HFIs [ --discovery, -d ] --- show NBFT discovery controllers [ --nbft-path=<STR> ] --- user-defined path for NBFT tables Signed-off-by: Stuart Hayes <stuart_hayes@dell.com> Signed-off-by: Martin Wilck <mwilck@suse.com> Signed-off-by: John Meneghini <jmeneghi@redhat.com>

igaw · 2023-04-17T08:08:31Z

Thanks a lot!

We have some time to stabilize or change things without any concerns of breaking stuff (inside this project, obviously only), since we still have time until the next release. But I suspect you really want to keep the command line options now as it is :)

mwilck mentioned this pull request Feb 3, 2023

NVMeoF / TCP boot support dracutdevs/dracut#2184

Merged

1 task

johnmeneghini mentioned this pull request Feb 3, 2023

Added NBFT table support timberland-sig/nvme-cli#1

Closed

This comment was marked as resolved.

Sign in to view

mwilck reviewed Feb 8, 2023

View reviewed changes

tbzatek mentioned this pull request Feb 27, 2023

nbft: added NBFT table support linux-nvme/libnvme#572

Merged

johnmeneghini force-pushed the timberlandmaster_final_v23 branch from 3198db6 to d13c14a Compare March 29, 2023 05:01

johnmeneghini force-pushed the timberlandmaster_final_v23 branch 2 times, most recently from 9cfa0ec to 84e4bbc Compare April 7, 2023 07:28

igaw requested changes Apr 12, 2023

View reviewed changes

fabrics.c Show resolved Hide resolved

fabrics.c Show resolved Hide resolved

nbft.c Outdated Show resolved Hide resolved

nbft.c Outdated Show resolved Hide resolved

nvme-builtin.h Outdated Show resolved Hide resolved

plugins/meson.build Show resolved Hide resolved

johnmeneghini force-pushed the timberlandmaster_final_v23 branch from 84e4bbc to c94829b Compare April 13, 2023 17:08

nbft: make lookup_ctrl function public

24da32d

To prepare for the addition of nbft functionality fixup the fabrics declarations. Signed-off-by: John Meneghini <jmeneghi@redhat.com>

johnmeneghini force-pushed the timberlandmaster_final_v23 branch from c94829b to 7cc9231 Compare April 14, 2023 18:31

johnmeneghini requested a review from igaw April 14, 2023 18:47

johnmeneghini force-pushed the timberlandmaster_final_v23 branch 2 times, most recently from 80ab18a to 7979dfd Compare April 15, 2023 02:29

stuarthayes added 2 commits April 14, 2023 22:52

johnmeneghini force-pushed the timberlandmaster_final_v23 branch from 7979dfd to f41dea3 Compare April 15, 2023 02:55

igaw merged commit 23daeed into linux-nvme:master Apr 17, 2023

teclator mentioned this pull request Apr 3, 2024

Fix call for connecting all discovered NVMe-over-Fabrics subsystems (bsc#1222246) yast/yast-installation#1112

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added NBFT table support #1791

Added NBFT table support #1791

johnmeneghini commented Feb 2, 2023 •

edited

Loading

igaw commented Feb 3, 2023

mwilck commented Feb 3, 2023

igaw commented Feb 3, 2023

igaw commented Feb 3, 2023 •

edited

Loading

This comment was marked as resolved.

mwilck commented Feb 3, 2023

igaw commented Feb 6, 2023

igaw commented Feb 8, 2023

hreinecke commented Feb 8, 2023

mwilck Feb 8, 2023 •

edited

Loading

igaw Feb 9, 2023 •

edited

Loading

mwilck Feb 9, 2023

igaw Feb 13, 2023

johnmeneghini Apr 14, 2023

mwilck Feb 8, 2023

igaw Feb 9, 2023

mwilck Feb 9, 2023

igaw Feb 9, 2023

mwilck Feb 9, 2023

johnmeneghini Apr 14, 2023

mwilck commented Feb 8, 2023

stuarthayes commented Feb 13, 2023

igaw commented Feb 14, 2023

mwilck commented Feb 14, 2023

igaw commented Feb 14, 2023 •

edited

Loading

mwilck commented Mar 25, 2023

igaw commented Mar 27, 2023

johnmeneghini commented Mar 29, 2023

johnmeneghini commented Apr 7, 2023

johnmeneghini commented Apr 12, 2023 •

edited

Loading

igaw left a comment

johnmeneghini commented Apr 14, 2023

johnmeneghini commented Apr 14, 2023

igaw commented Apr 17, 2023

Added NBFT table support #1791

Added NBFT table support #1791

Conversation

johnmeneghini commented Feb 2, 2023 • edited Loading

igaw commented Feb 3, 2023

mwilck commented Feb 3, 2023

igaw commented Feb 3, 2023

igaw commented Feb 3, 2023 • edited Loading

This comment was marked as resolved.

mwilck commented Feb 3, 2023

igaw commented Feb 6, 2023

igaw commented Feb 8, 2023

hreinecke commented Feb 8, 2023

mwilck Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

igaw Feb 9, 2023 • edited Loading

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

mwilck commented Feb 8, 2023

stuarthayes commented Feb 13, 2023

igaw commented Feb 14, 2023

mwilck commented Feb 14, 2023

igaw commented Feb 14, 2023 • edited Loading

mwilck commented Mar 25, 2023

igaw commented Mar 27, 2023

johnmeneghini commented Mar 29, 2023

johnmeneghini commented Apr 7, 2023

johnmeneghini commented Apr 12, 2023 • edited Loading

igaw left a comment

Choose a reason for hiding this comment

johnmeneghini commented Apr 14, 2023

johnmeneghini commented Apr 14, 2023

igaw commented Apr 17, 2023

johnmeneghini commented Feb 2, 2023 •

edited

Loading

igaw commented Feb 3, 2023 •

edited

Loading

mwilck Feb 8, 2023 •

edited

Loading

igaw Feb 9, 2023 •

edited

Loading

igaw commented Feb 14, 2023 •

edited

Loading

johnmeneghini commented Apr 12, 2023 •

edited

Loading