nbft: added NBFT table support #572

johnmeneghini · 2023-02-02T22:14:25Z

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

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

tbzatek

Mostly forwarding my previous remarks from the private repo.

src/nvme/nbft.h

src/libnvme.map

src/nvme/nbft.h

igaw

I think we should have a unit test for the parser. My idea would be to grab a memory dump and use it to verify that the parser works.

nbft_read could accept a descriptor instead of a filename and we could run this code with valgrind without having to setup any hardware.

Furthermore, if we modify stuff it could be easily verified nothing breaks.

src/nvme/nbft.h

src/nvme/nbft.c

igaw · 2023-02-03T12:35:10Z

The checkpatch.pl script the Linux tree helps to spot a lot of the style issues.

src/nvme/nbft.c

tbzatek · 2023-02-27T16:12:21Z

Alright, I'm going to slowly start addressing the points raised above to get things moving. As this will change the API, I'll be posting fixes to linux-nvme/nvme-cli#1791 accordingly.

tbzatek · 2023-03-09T15:58:39Z

I'm posting my fixes to side branches as the official timberland one is used as a source for PoC and I still need to properly test my changes. I do plan to add some tests as well. There's still some work to do, for now the code lives at:
https://github.com/timberland-sig/libnvme/tree/timberlandmaster_final_v13-fixes
https://github.com/timberland-sig/nvme-cli/tree/timberlandmaster_final_v23-fixes

src/nvme/nbft.h

tbzatek · 2023-03-27T17:06:39Z

Thanks @johnmeneghini for merging this. One more fix in timberland-sig#6 to be tested & merged plus this branch needs to be rebased against upstream libnvme master.

src/nvme/nbft.c

johnmeneghini · 2023-03-27T21:48:18Z

Next steps for this pull request:

rebase this pull request to master
rebase Added NBFT table support nvme-cli#1791 to master
retest everything with NVMeoF / TCP boot support dracutdevs/dracut#2184

Following this I propose we mere this into libnvme. and continue to work on linux-nvme/nvme-cli#1791.

igaw · 2023-03-28T07:43:06Z

@johnmeneghini thanks for the progress report. So I'll wait for the rebase of this PR. BTW, are you also squashing the commits too? I don't think we need to see the development history in upstream.

johnmeneghini · 2023-03-28T15:22:18Z

@johnmeneghini thanks for the progress report. So I'll wait for the rebase of this PR. BTW, are you also squashing the commits too? I don't think we need to see the development history in upstream.

OK. Will do. I will squash and rebase now.

src/nvme/nbft.c

mwilck · 2023-03-29T09:27:13Z

I am no longer able to boot my QEMU host with NVMe/TCP using these bits.

Hm, that's weird. My first thought was that you'd included Stuart's changes for the command line API, but apparently you didn't.

Have you tried just running nvme show-nbft on a system booted with the previous releases? If show-nbft still works, we probably have an issue with connect-nbft now?

tbzatek · 2023-03-29T12:44:00Z

I've tested the rebased branches and nvme show-nbft seems to be working fine, haven't noticed any difference in the output from the state of a week ago.

johnmeneghini · 2023-03-29T13:12:48Z

On 3/29/23 05:27, Martin Wilck wrote: I am no longer able to boot my QEMU host with NVMe/TCP using these bits. Hm, that's weird. Have you tried just running |nvme show-nbft| on a system booted with the previous releases?

Yes, I reported the same thing last night. Apparently the rebase process broke something. I was working on this till 1:00am lsst night and finally decided to push the broken branch upstream. A bug has been introduced somewhere... maybe an incompatibility with the dracut code. I'm looking for help to fix the problem.

…

— Reply to this email directly, view it on GitHub <#572 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADCUXXNOWCUFILW5XSN3V5DW6P57XANCNFSM6AAAAAAUPSF464>. You are receiving this because you were mentioned.Message ID: ***@***.***>

mwilck · 2023-03-29T13:32:02Z

I'm looking for help to fix the problem.

I'd love to help but I fear I won't have time for it today.

tbzatek · 2023-03-29T14:23:18Z

Another tiny change in PR timberland-sig#7 should fix the github actions checks.

johnmeneghini · 2023-04-01T02:17:49Z

I have redone my work and retested. All tests are passing my nvme/tcp boot test on the rh-linux-poc and this branch is ready for final review. Once the final few issues are resolved, I think this branch can be merged.

tbzatek · 2023-04-03T15:25:48Z

I have redone my work and retested. All tests are passing my nvme/tcp boot test on the rh-linux-poc and this branch is ready for final review. Once the final few issues are resolved, I think this branch can be merged.

The patch from timberland-sig#7 doesn't seem to be included in your last rebase. The rest of the code looks okay.

src/nvme/nbft.c

igaw

Sorry for the review delay. Was busy playing fire fighter.

doc/meson.build

src/libnvme.map

src/nvme/fabrics.h

src/nvme/nbft.c

src/meson.build

src/nvme/nbft.c

igaw · 2023-04-05T10:42:46Z

src/nvme/nbft.c

+#define verify(condition, message)					\
+	do {								\
+		if (!(condition)) {					\
+			nvme_msg(NULL, LOG_DEBUG, "file %s: " message "\n", \


I am not really happy with printing on stdout in the library. In this case it's just for debugging. The fabrics parts of the library we avoided this problem by attaching a logging fd to the nvme_root_t object. Obviously, here it's a bit difficult as there is no context we could use to pass in a fd.

I agree. But it will take some work to get rid of this one. Even if I fix this here, there are many more places where nvme_msg is called. Maybe we can open an issue for this and move forward.

Functions calling this function: nvme_msg File Function Line 0 nbft.c verify 84 nvme_msg(NULL, LOG_DEBUG, "file %s: " message "\n", \ 1 nbft.c __get_heap_obj 99 nvme_msg(NULL, LOG_DEBUG, 2 nbft.c __get_heap_obj 110 nvme_msg(NULL, LOG_DEBUG, 3 nbft.c __get_heap_obj 117 nvme_msg(NULL, LOG_DEBUG, 4 nbft.c read_ssns 231 nvme_msg(NULL, LOG_DEBUG, 5 nbft.c read_ssns 264 nvme_msg(NULL, LOG_DEBUG, 6 nbft.c read_ssns 282 nvme_msg(NULL, LOG_DEBUG, 7 nbft.c read_ssns 291 nvme_msg(NULL, LOG_DEBUG, 8 nbft.c read_hfi_info_tcp 335 nvme_msg(NULL, LOG_DEBUG, 9 nbft.c read_hfi 412 nvme_msg(NULL, LOG_DEBUG, a nbft.c read_discovery 454 nvme_msg(NULL, LOG_DEBUG, b nbft.c read_discovery 460 nvme_msg(NULL, LOG_DEBUG, c nbft.c nvme_nbft_read 695 nvme_msg(NULL, LOG_ERR, "Failed to open %s: %s\n", d nbft.c nvme_nbft_read 703 nvme_msg(NULL, LOG_ERR, "Failed to read from %s: %s\n", e nbft.c nvme_nbft_read 715 nvme_msg(NULL, LOG_ERR, "Failed to allocate memory for NBFT table"); f nbft.c nvme_nbft_read 723 nvme_msg(NULL, LOG_ERR, "Failed to read from %s: %s\n", g nbft.c nvme_nbft_read 737 nvme_msg(NULL, LOG_ERR, "Could not allocate memory for NBFT\n"); h nbft.c nvme_nbft_read 748 nvme_msg(NULL, LOG_ERR, "Failed to parse %s\n", filename);

Yes, let's move forward and address this later.

src/nvme/nbft.c

johnmeneghini · 2023-04-05T17:38:06Z

Thanks for your review Daniel. I will work on getting these updates done tonight.

…

On 4/5/23 06:55, Daniel Wagner wrote: ***@***.**** requested changes on this pull request. Sorry for the review delay. Was busy playing fire fighter. -------------------------------------------------------------------------------------------------------------------------------- In doc/meson.build <#572 (comment)>: > @@ -16,6 +16,7 @@ api_files = [ 'mi.h', 'tree.h', 'types.h', + 'nbft.h', This is supposed to be sorted alphabetically, though the |fabrics.h| is sorted in wrong too. -------------------------------------------------------------------------------------------------------------------------------- In src/libnvme.map <#572 (comment)>: > @@ -379,6 +379,8 @@ LIBNVME_1_0 { nvmf_treq_str; nvmf_trtype_str; nvmf_update_config; + nbft_read; + nbft_free; These should be prefixed with either |nvme_| and moved to right section (IIRC fixed in commit later) -------------------------------------------------------------------------------------------------------------------------------- In src/nvme/fabrics.h <#572 (comment)>: > @@ -12,6 +12,7 @@ #include <stdbool.h> #include <stdint.h> #include "tree.h" +#include "nbft.h" Looks like unnecessary change. -------------------------------------------------------------------------------------------------------------------------------- In src/nvme/nbft.c <#572 (comment)>: > + free(nbft->filename); + free(nbft); +} + +/** + * nbft_read() - Read and parse contents of an ACPI NBFT table + * + * @nbft: Parsed NBFT table data. + * @filename: Filename of the raw NBFT table to read. + * + * Read and parse the specified NBFT file into a struct nbft_info. + * Free with nbft_free(). + * + * Return: 0 on success, errno otherwise. + */ +int nbft_read(struct nbft_info **nbft, const char *filename) Prefix this function with |nvme_| -------------------------------------------------------------------------------------------------------------------------------- In src/nvme/nbft.c <#572 (comment)>: > + control->num_ssns <= le32_to_cpu(header->length), + "invalid subsystem namespace descriptor list offset"); + raw_ssns_array = (struct nbft_ssns *)(raw_nbft + + le32_to_cpu(control->ssnso)); + read_ssns_descriptors(nbft, control->num_ssns, raw_ssns_array, + le16_to_cpu(control->ssnsl)); + } + + return 0; +} + +/** + * nbft_free() - Free the struct nbft_info and its contents + * @nbft: Parsed NBFT table data. + */ +void nbft_free(struct nbft_info *nbft) Prefix this function with |nvme_| -------------------------------------------------------------------------------------------------------------------------------- In src/meson.build <#572 (comment)>: > @@ -120,6 +121,7 @@ install_headers('libnvme.h', install_mode: mode) install_headers('libnvme-mi.h', install_mode: mode) install_headers([ 'nvme/api-types.h', + 'nvme/nbft.h', This is also supposed to be alphabetical sorted. -------------------------------------------------------------------------------------------------------------------------------- In src/nvme/nbft.c <#572 (comment)>: > + +#include <errno.h> +#include <stdio.h> +#include <stdlib.h> + +#include <arpa/inet.h> +#include <ccan/endian/endian.h> + +#include "private.h" +#include "nbft.h" +#include "log.h" + + +#define MIN(a, b) (((a) < (b)) ? (a) : (b)) + +static __u8 csum(void *buffer, int length) buffer should propebly be |const __u8 *| type, this avoids the cast below and the caller is actually passing in |__u8 *| length should be type |ssize_t| because of the |raw_nbft_size| definition. -------------------------------------------------------------------------------------------------------------------------------- In src/nvme/nbft.c <#572 (comment)>: > + + for (n = 0; n < length; n++) + sum = (__u8)(sum + ((__u8 *)buffer)[n]); + return sum; +} + +static void format_ip_addr(char *buf, size_t buflen, __u8 *addr) +{ + struct in6_addr *addr_ipv6; + + addr_ipv6 = (struct in6_addr *)addr; + if (addr_ipv6->s6_addr32[0] == 0 && + addr_ipv6->s6_addr32[1] == 0 && + ntohl(addr_ipv6->s6_addr32[2]) == 0xffff) + /* ipv4 */ + inet_ntop(AF_INET, &(addr_ipv6->s6_addr32[3]), buf, buflen); No need to check if the |inet_ntop| is successful? -------------------------------------------------------------------------------------------------------------------------------- In src/nvme/nbft.c <#572 (comment)>: > + * Return transport_type string (NBFT Table 2) + */ +static char *trtype_to_string(__u8 transport_type) +{ + switch (transport_type) { + case 3: + return "tcp"; + default: + return "invalid"; + } +} + +#define verify(condition, message) \ + do { \ + if (!(condition)) { \ + nvme_msg(NULL, LOG_DEBUG, "file %s: " message "\n", \ I am not really happy with printing on stdout in the library. In this case it's just for debugging. The fabrics parts of the library we avoided this problem by attaching a logging fd to the |nvme_root_t| object. Obviously, here it's a bit difficult as there is no context we could use to pass in a fd. -------------------------------------------------------------------------------------------------------------------------------- In src/nvme/nbft.c <#572 (comment)>: > + nvme_msg(NULL, LOG_DEBUG, "file %s: " message "\n", \ + nbft->filename); \ + return -EINVAL; \ + } \ + } while (0) + +static int __get_heap_obj(struct nbft_header *header, const char *filename, + const char *descriptorname, const char *fieldname, + struct nbft_heap_obj obj, bool is_string, + char **output) +{ + if (le16_to_cpu(obj.length) == 0) + return -ENOENT; + + if (!in_heap(header, obj)) { + nvme_msg(NULL, LOG_DEBUG, Trying to print when we can't allocate memory is almost certainly also failing. Just return the error code. -------------------------------------------------------------------------------------------------------------------------------- In src/nvme/nbft.c <#572 (comment)>: > verify(raw_ssns->structure_id == NBFT_DESC_SSNS, "invalid ID in SSNS descriptor"); ssns = calloc(1, sizeof(*ssns)); - if (!ssns) + if (!ssns) { + errno = ENOMEM; The libnvme follows (unfortunatly) the POSIX API design. That means it supposed to return 1 on error with errno set. We have parts of the API though which returns -1 and sets errno. So my take is that within a subset of the libnvme API (header file) we should aim to be consistent. Note, internally we use negative returning negative error codes, this is just for the public API where we do a return 1, errno=CODE. Anyway for v2, we have plans to change this part to just return the error codes directly with the option to have more structured error messages means if necessary. Similar to the GLib APIs. — Reply to this email directly, view it on GitHub <#572 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADCUXXPVVGZUZWB3MYGUSFTW7VFRVANCNFSM6AAAAAAUPSF464>. You are receiving this because you were mentioned.Message ID: ***@***.***>

Added support for parsing and printing the contents of the NBFT table (per NVMe-oF boot specification v1.0). Signed-off-by: Stuart Hayes <stuart_hayes@dell.com> Signed-off-by: Martin Belanger <martin.belanger@dell.com> Signed-off-by: Martin Wilck <mwilck@suse.com> Signed-off-by: Tomas Bzatek <tbzatek@redhat.com> Signed-off-by: John Meneghini <jmeneghi@redhat.com>

johnmeneghini · 2023-04-07T07:36:52Z

I have squashed everything and re based onto v1.4.

igaw · 2023-04-12T13:10:16Z

Thanks everyone! Now, let's see what we have for nvme-cli :)

johnmeneghini marked this pull request as draft February 2, 2023 22:14

johnmeneghini marked this pull request as ready for review February 2, 2023 22:17

tbzatek reviewed Feb 3, 2023

View reviewed changes

igaw requested changes Feb 3, 2023

View reviewed changes

johnmeneghini mentioned this pull request Feb 3, 2023

Added NBFT table support timberland-sig/libnvme#1

Closed

martin-belanger reviewed Feb 3, 2023

View reviewed changes

src/nvme/nbft.c Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

tbzatek mentioned this pull request Mar 14, 2023

timberlandmaster_final_v13: API fixes batch #1 timberland-sig/libnvme#4

Merged

tbzatek reviewed Mar 14, 2023

View reviewed changes

src/nvme/nbft.h Show resolved Hide resolved

tbzatek reviewed Mar 14, 2023

View reviewed changes

src/nvme/nbft.h Show resolved Hide resolved

tbzatek reviewed Mar 27, 2023

View reviewed changes

src/nvme/nbft.c Show resolved Hide resolved

martin-belanger reviewed Mar 28, 2023

View reviewed changes

src/nvme/nbft.c Outdated Show resolved Hide resolved

johnmeneghini force-pushed the timberlandmaster_final_v13 branch from 123748c to 5d69e99 Compare March 29, 2023 04:59

This comment was marked as outdated.

Sign in to view

johnmeneghini mentioned this pull request Mar 29, 2023

Added NBFT table support linux-nvme/nvme-cli#1791

Merged

johnmeneghini force-pushed the timberlandmaster_final_v13 branch from 479c345 to cd122fc Compare April 1, 2023 01:48

mwilck reviewed Apr 4, 2023

View reviewed changes

src/nvme/nbft.c Outdated Show resolved Hide resolved

johnmeneghini requested a review from igaw April 4, 2023 16:37

igaw requested changes Apr 5, 2023

View reviewed changes

johnmeneghini force-pushed the timberlandmaster_final_v13 branch 2 times, most recently from 8411348 to dd89897 Compare April 6, 2023 20:10

johnmeneghini force-pushed the timberlandmaster_final_v13 branch from dd89897 to ac5cd69 Compare April 7, 2023 07:16

johnmeneghini requested a review from igaw April 7, 2023 07:37

igaw approved these changes Apr 12, 2023

View reviewed changes

igaw merged commit 28ad0e9 into linux-nvme:master Apr 12, 2023

johnmeneghini mentioned this pull request Apr 12, 2023

Refactor use of nvme_msg in nbft.c #619

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nbft: added NBFT table support #572

nbft: added NBFT table support #572

johnmeneghini commented Feb 2, 2023 •

edited

Loading

tbzatek left a comment

igaw left a comment

igaw commented Feb 3, 2023

tbzatek commented Feb 27, 2023

This comment was marked as resolved.

tbzatek commented Mar 9, 2023

tbzatek commented Mar 27, 2023 •

edited

Loading

johnmeneghini commented Mar 27, 2023

igaw commented Mar 28, 2023

johnmeneghini commented Mar 28, 2023

This comment was marked as outdated.

mwilck commented Mar 29, 2023 •

edited

Loading

tbzatek commented Mar 29, 2023 •

edited

Loading

johnmeneghini commented Mar 29, 2023 via email

mwilck commented Mar 29, 2023 •

edited

Loading

tbzatek commented Mar 29, 2023

johnmeneghini commented Apr 1, 2023

tbzatek commented Apr 3, 2023

igaw left a comment

igaw Apr 5, 2023

johnmeneghini Apr 5, 2023

igaw Apr 12, 2023

johnmeneghini commented Apr 5, 2023 via email

johnmeneghini commented Apr 7, 2023

igaw commented Apr 12, 2023

nbft: added NBFT table support #572

nbft: added NBFT table support #572

Conversation

johnmeneghini commented Feb 2, 2023 • edited Loading

tbzatek left a comment

Choose a reason for hiding this comment

igaw left a comment

Choose a reason for hiding this comment

igaw commented Feb 3, 2023

tbzatek commented Feb 27, 2023

This comment was marked as resolved.

tbzatek commented Mar 9, 2023

tbzatek commented Mar 27, 2023 • edited Loading

johnmeneghini commented Mar 27, 2023

igaw commented Mar 28, 2023

johnmeneghini commented Mar 28, 2023

This comment was marked as outdated.

mwilck commented Mar 29, 2023 • edited Loading

tbzatek commented Mar 29, 2023 • edited Loading

johnmeneghini commented Mar 29, 2023 via email

mwilck commented Mar 29, 2023 • edited Loading

tbzatek commented Mar 29, 2023

johnmeneghini commented Apr 1, 2023

tbzatek commented Apr 3, 2023

igaw left a comment

Choose a reason for hiding this comment

igaw Apr 5, 2023

Choose a reason for hiding this comment

johnmeneghini Apr 5, 2023

Choose a reason for hiding this comment

igaw Apr 12, 2023

Choose a reason for hiding this comment

johnmeneghini commented Apr 5, 2023 via email

johnmeneghini commented Apr 7, 2023

igaw commented Apr 12, 2023

johnmeneghini commented Feb 2, 2023 •

edited

Loading

tbzatek commented Mar 27, 2023 •

edited

Loading

mwilck commented Mar 29, 2023 •

edited

Loading

tbzatek commented Mar 29, 2023 •

edited

Loading

mwilck commented Mar 29, 2023 •

edited

Loading