Mi admin passthru

[ikegami-t]

No description provided.

[igaw]

Could you leave the orignal formatting of the function as it is and just add the __attribute__. This way it's clear this change is only adding the attribute. Mixing whitespace changes into code changes should best be avoided, it makes the reviewing harder than it needs to be. Thanks

[igaw]

Please do not change the formatting if there is no good reason for it. It perfectly fine to use to 80 chars limits. There is no fix rule we nee do to use 100 chars.

[igaw]

The original formatting looks saner in my opinion. Let's keep it this way, please.

In an ideal world we would have used something like clang formatter from the beginning but I think this ship has sailed and we live in an imperfect world. And I really think checkpatch.pl is really a crutch, it helps for new submissions to minimize the review work but for the existing code I'd rather leave as it is unless it is completely offensive to read (e.g. the plugin code used to be very bad). Though the mi.c file is one of the very nicely formatted files and Jeremy is really taking care it stays this way.

[igaw]

@jk-ozlabs

[jk-ozlabs]

Allowing arbitrary override of the library functions sounds like a recipe for disaster, and we don't have a good rationale for this change. The PR description is empty.

Can you provide some information about why you're trying to do this?

[igaw]

The idea with the weak symbol for the passthru is that the user is able to implement their own logging. For the non-mi passthru we already got this. nvme-cli is able to annotate the ioctl. Before that there was a lot of random printfs sprinkled in the libnvme, which I really hated. Thus the compromise was to allow the overwrite the low level wrapper around ioctl. Yes, it opens the door for other disasters but then it's not our problem :)

E.g. the -vv is implemented in nvme-cli not in libnvme

# nvme id-ctrl -vv /dev/nvme0
opcode       : 06
flags        : 00
rsvd1        : 0000
nsid         : 00000000
cdw2         : 00000000
cdw3         : 00000000
data_len     : 00001000
metadata_len : 00000000
addr         : 564b82b40000
metadata     : 0
cdw10        : 00000001
cdw11        : 00000000
cdw12        : 00000000
cdw13        : 00000000
cdw14        : 00000000
cdw15        : 00000000
timeout_ms   : 00000000
result       : 00000000
err          : 0
latency      : 2317 us

[jk-ozlabs]

Ah! thanks for the context.

In that case, I'd agree with the prior review - there's a lot of churn in this change, can we just stick to the attribute addition?

[jk-ozlabs]

We'll end up with two functions with similar names:

• nvme_mi_admin_passthru; and
• nvme_mi_admin_admin_passthru

- is there a batter way to distinguish these?

[ikegami-t]

Deleted the commits for coding style changes. Also change the function name nvme_mi_admin_passthru() to nvme_mi_submit_passthru(). Thank you.

[jk-ozlabs]

Thus the compromise was to allow the overwrite the low level wrapper around ioctl. Yes, it opens the door for other disasters but then it's not our problem

Just thinking about this a little - would it make more sense to have a weak function lower in the stack, at the point where the MI command is submitted?

The default implementation of that function would be a no-op, but users could override to get per-command logging. At present this is just for the passthrough interface, and we'd need to make a lot more changes to get full coverage.

[igaw]

The current weak functions are just a wrapper around the ioctl call. I didn't thought of using an empty default implementation. Sounds like a good idea!

[jk-ozlabs]

OK, in that case (if I understand the overall intention correctly) we would want to hook into nvme_mi_submit, which handles the command submission and completion process for all MI interactions, at a transport-independent level (our only transport is MCTP, but others are possible).

So, two options that I can see there:

• we provide two "hooks" as weak functions: one for the command, and another for the response. Default implementations would be empty:

  int nvme_mi_submit(nvme_mi_ep_t ep, struct nvme_mi_req *req,
     	   struct nvme_mi_resp *resp)
  {
     nvme_mi_submit_hook_entry(ep, req);
  
     /* .. existing mi_submit() implementation .. */
  
     nvme_mi_submit_hook_exit(ep, req, resp);
  }
  
  __attribute__((weak)) void nvme_mi_submit_hook_entry(nvme_mi_ep_t ep, struct nvme_mi_req *req) { }
  __attribute__((weak)) void nvme_mi_submit_hook_exit(nvme_mi_ep_t ep, struct nvme_mi_req *req, struct nvme_mi_resp *resp) { }

• we provide a "wrapper" as a weak function: this would see both the request and the response, but is responsible for calling a lower-level submit in order to do so (and may log before and/or after). Default implementation would just call that lower-level submit:

  __attribute__((weak)) int nvme_mi_submit_wrap(nvme_mi_ep_t ep, struct nvme_mi_req *req,
  		   struct nvme_mi_resp *resp)
  {
      return nvme_mi_submit(ep, req, resp);
  }

  (and, of course, convert all internal callers of nvme_mi_submit to nvme_mi_submit_wrap)

This would match the "hook at the ioctl" concept more closely, rather than just hooking the mi_passthrough call, which is used only for custom commands.

[That said, the MCTP transport makes this fairly easy to debug without having to modify any code; we can just tcpdump / wireshark the MCTP messages to observe the requests/responses. However, that's at a different level, so there still may be valid usecases for doing the debug in-process]

[igaw]

After a bit of pondering, I'd say the first API is the one we should do. nvme-cli can do all the logging with those hooks and it avoids duplicating any code, e.g. error handling.

Maybe it would be good to allow passing around a opaque pointer...

[ikegami-t]

Thanks for your comments and advices. The structures struct nvme_mi_req and struct nvme_mi_resp are defined in private.h so seems not used for the APIs.

[jk-ozlabs]

Maybe it would be good to allow passing around a opaque pointer...

OK, but we'd need somewhere to stash that, and set it up. Would it belong in the root, or the endpoint?

The structures struct nvme_mi_req and struct nvme_mi_resp are defined in private.h

Yes, good point. What information do you want access to for these hooks?

[igaw]

OK, but we'd need somewhere to stash that, and set it up. Would it belong in the root, or the endpoint?

Ah I remember now why I have chosen the second API approach. The scoping of the function is nice but has the downside that all users need to provide the complete implementation.

Anyway, what I had in mind was something like:

// libnvme

__attribute__((weak)) void *nvme_submit_passthru_hook_entry(struct nvme_passthru_cmd *cmd) { }
__attribute__((weak)) void nvme_submit_passthru_hook_exit(struct nvme_passthru_cmd *cmd, int err, void *user_data) { }

int nvme_submit_passthru(int fd, unsigned long ioctl_cmd,
			 struct nvme_passthru_cmd *cmd, __u32 *result)
{
	void *user_data;

	user_data = nvme_submit_passthru_hook_entry(cmd);

	int err = ioctl(fd, ioctl_cmd, cmd);

	if (err >= 0 && result)
		*result = cmd->result;

	nvme_submit_passthru_hook_exit(cmd, err, user_data);
	return err;
}

// nvme-cli

struct submit_data
{
	struct timeval start;
	struct timeval end;
};

static struct submit_data sb;

void *nvme_sumbit_passthru_hook_entry(struct nvme_passthru_cmd *cmd)
{
	sb = { 0, };

	if (log_level >= LOG_DEBUG)
		gettimeofday(&sb.start, NULL);

	return &sb;
}

void nvme_sumbit_passthru_hook_exit(struct nvme_passthru_cmd *cmd, int err, void *user_data)
{
	struct submit_data *sb = user_data;

	if (log_level >= LOG_DEBUG) {
		gettimeofday(sb->end, NULL);
		nvme_show_command(cmd, err);
		nvme_show_latency(sb->start, sb->end);
	}
}

Obviously, it's a bit stupid that there is a global data struct in nvme-cli but that could be made also a bit smarter. I just want to avoid adding things to libnvme which is way harder to change later.

[jk-ozlabs]

One question: why the reference to 'passthru' here? Do you only want logging for passthru-style commands (ie, those that are bypassing the general libnvme-mi API), or for all commands?

[jk-ozlabs]

	user_data = nvme_submit_passthru_hook_entry(cmd);

	int err = ioctl(fd, ioctl_cmd, cmd);

	if (err >= 0 && result)
		*result = cmd->result;

	nvme_submit_passthru_hook_exit(cmd, err, user_data)

ok, so the user_data can be used to pass things between a specific call, but has no special access to general context - say a log fd. Any data used in the entry hook would all have to be passed through globals, is that still okay?

[ikegami-t]

How about the changes below as the struct nvme_mi_msg_hdr can be used for API?

tokunori@tokunori-desktop:~/nvme-cli/subprojects/libnvme$ git diff
diff --git a/src/nvme/mi.c b/src/nvme/mi.c
index 4640b0a9..d4922cf9 100644
--- a/src/nvme/mi.c
+++ b/src/nvme/mi.c
@@ -414,11 +414,16 @@ static int nvme_mi_verify_resp_mic(struct nvme_mi_resp *resp)
        return resp->mic != ~crc;
 }
 
+__attribute__((weak)) void nvme_mi_submit_entry(nvme_mi_ep_t ep, struct nvme_mi_msg_hdr *req) { }
+__attribute__((weak)) void nvme_mi_submit_exit(nvme_mi_ep_t ep, struct nvme_mi_msg_hdr *resp) { }
+
 int nvme_mi_submit(nvme_mi_ep_t ep, struct nvme_mi_req *req,
                   struct nvme_mi_resp *resp)
 {
        int rc;
 
+       nvme_mi_submit_entry(ep, req->hdr);
+
        if (req->hdr_len < sizeof(struct nvme_mi_msg_hdr)) {
                errno = EINVAL;
                return -1;
@@ -502,6 +507,8 @@ int nvme_mi_submit(nvme_mi_ep_t ep, struct nvme_mi_req *req,
                return -1;
        }
 
+       nvme_mi_submit_exit(ep, resp->hdr);
+
        return 0;
 }

[igaw]

One question: why the reference to 'passthru' here? Do you only want logging for passthru-style commands (ie, those that are bypassing the general libnvme-mi API), or for all commands?

I was just using the existing code as example, how I think we should pass in an opaque pointer. Sorry should have added this.

...say a log fd. Any data used in the entry hook would all have to be passed through globals, is that still okay?

For this case I think your proposal looks 1) good just with the additional pointer passing.

[jk-ozlabs]

How about the changes below as the struct nvme_mi_msg_hdr can be used for API?

that looks nice and neat, but I don't have much info about your aims here. From my previous comment: What information do you want access from these hooks? Given that we're writing a stable API here, we'll want to get this right first-time.

When passing the header (ie struct nvme_mi_msg_hdr) you'll have access to the base MI header only - so no type-specific header (eg, the Admin command header), and no payload. Is that sufficient to implement reasonable hooks?

Also: I assume the request/response data should be const? or do you want the hooks to be able to modify them?

I would also suggest that you'll want to pass both the request and the response to the _exit hook, so you have more context to work with there.

As @igaw has brought up, there should probably also be some way to correlate a request (a call to _entry) with a response (a call to _exit) too - you could pass a void * or even a uintptr_t from the _entry to the _exit. We can sort-of tell this from the CSI value in the header - there should not be more than one command outstanding, for each of the two CSI values, but it might be nicer to provide something more ergonomic for the caller.

[ikegami-t]

Hi,
At this moment the information required below opcode and dwords data etc,.

1. https://github.com/linux-nvme/nvme-cli/blob/master/nvme.c#L9175

		printf("opcode       : %02x\n", cfg.opcode);
		printf("flags        : %02x\n", cfg.flags);
		printf("rsvd1        : %04x\n", cfg.rsvd);
		printf("nsid         : %08x\n", cfg.namespace_id);
		printf("cdw2         : %08x\n", cfg.cdw2);
		printf("cdw3         : %08x\n", cfg.cdw3);
		printf("data_len     : %08x\n", cfg.data_len);
		printf("metadata_len : %08x\n", cfg.metadata_len);
		printf("addr         : %"PRIx64"\n", (uint64_t)(uintptr_t)data);
		printf("metadata     : %"PRIx64"\n", (uint64_t)(uintptr_t)mdata);
		printf("cdw10        : %08x\n", cfg.cdw10);
		printf("cdw11        : %08x\n", cfg.cdw11);
		printf("cdw12        : %08x\n", cfg.cdw12);
		printf("cdw13        : %08x\n", cfg.cdw13);
		printf("cdw14        : %08x\n", cfg.cdw14);
		printf("cdw15        : %08x\n", cfg.cdw15);
		printf("timeout_ms   : %08x\n", nvme_cfg.timeout);

So it okay to pass the struct nvme_mi_admin_req_hdr data below.

struct nvme_mi_admin_req_hdr {
	struct nvme_mi_msg_hdr hdr;
	__u8	opcode;
	__u8	flags;
	__le16	ctrl_id;
	__le32	cdw1, cdw2, cdw3, cdw4, cdw5;
	__le32	doff;
	__le32	dlen;
	__le32	rsvd0, rsvd1;
	__le32	cdw10, cdw11, cdw12, cdw13, cdw14, cdw15;
} __attribute((packed));

Also the following information also can be parsed from the dwords information above.
2. https://github.com/linux-nvme/nvme-cli/blob/master/nvme.c#L8289

		printf("opcode       : %02x\n", opcode);
		printf("nsid         : %02x\n", cfg.namespace_id);
		printf("flags        : %02x\n", 0);
		printf("control      : %04x\n", control);
		printf("nblocks      : %04x\n", nblocks);
		printf("metadata     : %"PRIx64"\n", (uint64_t)(uintptr_t)mbuffer);
		printf("addr         : %"PRIx64"\n", (uint64_t)(uintptr_t)buffer);
		printf("slba         : %"PRIx64"\n", (uint64_t)cfg.start_block);
		printf("dsmgmt       : %08x\n", dsmgmt);
		printf("reftag       : %"PRIx64"\n", (uint64_t)cfg.ref_tag);
		printf("apptag       : %04x\n", cfg.app_tag);
		printf("appmask      : %04x\n", cfg.app_tag_mask);
		printf("storagetagcheck : %04x\n", cfg.storage_tag_check);
		printf("storagetag      : %"PRIx64"\n", (uint64_t)cfg.storage_tag);
		printf("pif             : %02x\n", pif);
		printf("sts             : %02x\n", sts);

I understand the struct nvme_mi_req hdr points the struct nvme_mi_admin_req_hdr hdr address but the header length is the size of struct nvme_mi_admin_req_hdr so includes the opcode and dwords data etc, also.

struct nvme_mi_req {
	struct nvme_mi_msg_hdr *hdr;
	size_t hdr_len;
	void *data;
	size_t data_len;
	__u32 mic;
};

It is also same for other structures for example struct nvme_mi_control_req etc,.

struct nvme_mi_control_req {
	struct nvme_mi_msg_hdr hdr;
	__u8	opcode;
	__u8	tag;
	__le16	cpsp;
} __attribute((packed));

And it can be checked by the struct nvme_mi_msg_hdr type as which structure used.

struct nvme_mi_msg_hdr {
	__u8	type;
	__u8	nmp;
	__u8	meb;
	__u8	rsvd0;
} __attribute__((packed));

Yes I will do update the changes to use const paramter then push the commit later.
By the way I am not sure if the data information is required in future.
Thank you.

[jk-ozlabs]

At this moment the information required below opcode and dwords data etc,.

OK, thanks for the info. are you looking to access the message payload data as well?

I understand the struct nvme_mi_req hdr points the struct nvme_mi_admin_req_hdr hdr address but the header length is the size of struct nvme_mi_admin_req_hdr so includes the opcode and dwords data etc, also.

That's true for now, but is an implementation detail of the internal workings of libnvme. The proposed change will codify the layout of libnvme's own buffers into a stable ABI.

For example: if we changed to a scatter-gather buffer¹, the MI header data may no longer be contiguous with the type-specific header. Just passing the nvme_mi_req_hdr header does not indicate to the function that any bytes past sizeof(struct nvme_mi_req_hdr) are valid

With the function prototypes as they are at the moment, the only thing that the function can assume access to is the struct nvme_mi_req_hdr itself.

In practice, it will be unlikely that we change the message layout in memory. But it's a bit sketchy for a hook implementation to be poking around past the end of the structure it was passed.

(Also, as you mention, it may not be an admin header that follows; it could be a MI header, a control header, or PCIe command header in future. A hook implementation would need to check the type before safely interpreting the full header data)

For making forward process here: I would think that a reasonable approach may be for us to decide (and document!) that the header data must be contiguous for all future libnvme², and include the actual header size in the hook arguments. The latter provides some indication to the hook implementation that there is valid data beyond the passed struct.

An alternative would be to pass a union of all header types, but that seems a bit messy.

1: say, for more efficient message creation - which we have already done for the header/payload/MIC split
2: or if we do change it, we would need to make a backwards-compatible contiguous copy for the hooks to consume

[igaw]

So just for the sake of logging, I don't want to expose currently internal data structures. From what we've seen in the last few years, there's always someone using them, and we can't really change them easily without breaking things afterwards.

[jk-ozlabs]

Sure, that's a reasonable approach. Just to clarify though - this isn't exposing anything outside of the (currently public) API - and in this case those definitions are just the MI spec's message layout.

What this is exposing is a "contract" in how we're layout-out the buffers with those messages, and specifically that the MI header and the rest of the command header (MI, Admin, Control or PCI) is contiguous. I don't think that's too onerous to stick with.

if we later make those discontiguous, we have a backwards-compat "escape hatch" should we need one: we can linearise just for the hooks. But since we have no idea whether the hooks have been overridden, we would need to re-linearise in every invocation of submit - not just when the hooks are present, which is I assume is very infrequent. In practice, this escape hatch would mean an extra two malloc + header copy, per command.

[ikegami-t]

Changed the hook functions only for the MI message type: admin at this moment and also added the message payload data as mentioned. Thank you.

[jk-ozlabs]

Are you sure you want to take that approach? sounds like we'll need a lot of hooks there...

[ikegami-t]

@jk-ozlabs Yes right so fixed the patch then please review again. Thank you.

[jk-ozlabs]

That looks like the right approach to me, in terms of the data we're passing to the hook (and the implicit ABI around the header / data layouts).

I can't speak much for the consumer side though, @igaw mentioned facilities for maintaining context across those too calls - is that still desirable?

[igaw]

Yes, I still like to have to possibility to pass a pointer around from the call side. #983 (comment)

[ikegami-t]

Yes, I still like to have to possibility to pass a pointer around from the call side. #983 (comment)

About the suggestion above there is are issues as the comment below so could please confirm the comment?
linux-nvme/nvme-cli#2751 (comment)

@igaw Thanks for your review comments. Basically look good but let me confirm below before the fix.

1. The suggestion will change the some logging behaviors so are the changes okay for now? Or still should we keep the logging behavior basically?
2. Basically the struct nvme_passthru_cmd values opcode and cdw, etc. can be converted to nvme_show_io_command() outputs values nlb and slba, etc. but only the value storage_tag needed pif and sts values to pass and also 'args_size' needed probably. The nvme-cli converts struct config to struct nvme_io_args and the libnvme converts struct nvme_io_args to struct nvme_passthru_cmd then the logging feature converts the struct nvme_passthru_cmd to struct nvme_io_args again. Is this really required?

[ikegami-t]

Very sorry about the above comment #983 (comment) I misunderstood as the PR mention #983 (comment) and the nvme-cli PR #2751 comments are same but actually those related but not same. So I will do consder this PR comment above again to fix. Thank you.

[ikegami-t]

Fix for the comment so please review again. Thank you.

[jk-ozlabs]

Why the void * here? it's always at least a struct nvme_mi_msg_hdr, right?

(hook implementations can then cast according to type, if necessary...)

[ikegami-t]

Yes so fixed the void * to struct nvme_mi_msg_hdr *. Thank you.

[jk-ozlabs]

Yes so fixed the void * to struct nvme_mi_msg_hdr *. Thank you.

but now we've lost the const; is that intentional?

[ikegami-t]

No missed to remain the const so fixed the patch again.

[jk-ozlabs]

struct nvme_mi_msg_resp for hdr here too?

[ikegami-t]

This also fixed. Thank you.

[jk-ozlabs]

Looks good to me!

[igaw]

Thanks!