nvme: Add support for get-reg command to output single register

[ikegami-t]

Note: Currently only stdout print supported.

[igaw]

• Please avoid mixing white space cleanup patches with feature patches
• Documentation is missing

How does the kernel figure out that some of the register values have changes?

[ikegami-t]

Fixed for some comments but still remaining comments to fix so will update later.

[calebsander]

The verbose output is less verbose?

Also why use %02x? There are already some offsets that take 3 hex digits.

[ikegami-t]

The output example is below also the human-readable option is for readable format output but not for verbose output.

tokunori@tokunori-desktop:~/nvme-cli$ sudo .build/nvme get-reg /dev/nvme0 -c
register: 0x00 (Controller Capabilities), value: 3028033fff
tokunori@tokunori-desktop:~/nvme-cli$ sudo .build/nvme get-reg /dev/nvme0 -c -H
Controller Capabilities: 3028033fff
        Controller Ready With Media Support (CRWMS): Not Supported
        Controller Ready Independent of Media Support (CRIMS): Not Supported
        NVM Subsystem Shutdown Supported   (NSSS): Not Supported
        Controller Memory Buffer Supported (CMBS): The Controller Memory Buffer is Not Supported
        Persistent Memory Region Supported (PMRS): The Persistent Memory Region is Not Supported
        Memory Page Size Maximum         (MPSMAX): 4096 bytes
        Memory Page Size Minimum         (MPSMIN): 4096 bytes
        Controller Power Scope              (CPS): Not Reported
        Boot Partition Support              (BPS): No
        Command Sets Supported              (CSS): NVM command set is Supported
                                                   One or more I/O Command Sets are Not Supported
                                                   I/O Command Set is Supported
        NVM Subsystem Reset Supported     (NSSRS): Yes
        Doorbell Stride                   (DSTRD): 4 bytes
        Timeout                              (TO): 20000 ms
        Arbitration Mechanism Supported     (AMS): Weighted Round Robin with Urgent Priority Class is Not supported
        Contiguous Queues Required          (CQR): Yes
        Maximum Queue Entries Supported    (MQES): 16384

tokunori@tokunori-desktop:~/nvme-cli$ sudo .build/nvme get-reg /dev/nvme0 -c -h
Usage: nvme get-reg <device> [OPTIONS]

Reads and shows the defined NVMe controller register.
Register offset must be one of:
CAP=0x0, VS=0x8, INTMS=0xc, INTMC=0x10, CC=0x14, CSTS=0x1c,
NSSR=0x20, AQA=0x24, ASQ=0x28, ACQ=0x30, CMBLOC=0x38,
CMBSZ=0x3c, BPINFO=0x40, BPRSEL=0x44, BPMBL=0x48, CMBMSC=0x50,
CMBSTS=0x58, CRTO=0x68, PMRCAP=0xe00, PMRCTL=0xe04,
PMRSTS=0xe08, PMREBS=0xe0c, PMRSWTP=0xe10, PMRMSCL=0xe14, PMRMSCU=0xe18

Options:
  [  --verbose, -v ]                    --- Increase output verbosity
  [  --output-format=<FMT>, -o <FMT> ]  --- Output format: normal|json|binary
  [  --offset=<NUM>, -O <NUM> ]         --- offset of the requested register
  [  --human-readable, -H ]             --- show register in readable format

[ikegami-t]

Also why use %02x? There are already some offsets that take 3 hex digits.

Fixed by the commit 24f79e3.

[calebsander]

But in this specific line (register: 0x00 (Controller Capabilities), value: 3028033fff vs. Controller Capabilities: 3028033fff), the verbose output doesn't have the register number. It seems like "verbose" should at least contain all the information that "not verbose" does.

[ikegami-t]

To make sure let me reconfirm as the "verbose" mentioned is not actually set by the command verbose option but set by the human-readable so not necessary to contain the register number for the option output.

static int get_register(int argc, char **argv, struct command *cmd, struct plugin *plugin)
{
...
	bool human = stdout_print_ops.flags & VERBOSE;
...
static void stdout_ctrl_register(int offset, uint64_t value64)
{
	if (cfg.human_readable)
		flags |= VERBOSE;

[igaw]

I think stdout_ctrl_register_common still has this issue.

+static void stdout_ctrl_register_common(int offset, uint64_t value, bool fabrics)
+{
+       bool human = !!(stdout_print_ops.flags & VERBOSE);
+       const char *name = nvme_register_to_string(offset);
+       const char *type = fabrics ? "property" : "register";
+
+       if (human) {
+               printf("%s: %#"PRIx64"\n", name, value);
+               stdout_ctrl_register_human(offset, value, true);
+               return;
+       }
+
+       printf("%s: %#04x (%s), value: %#"PRIx64"\n", type, offset,
+              name, value);
+}

While stdout_ctrl_register_support handle the verbose flag differently, that is it prints the normal output and then the verbose. This is what I would expect from the previous output as well

+static void stdout_ctrl_register_support(void *bar, bool fabrics, int offset, bool human,
+                                        bool support)
+{
+       uint64_t value = nvme_is_64bit_reg(offset) ? mmio_read64(bar + offset) :
+           mmio_read32(bar + offset);
+
+       if (fabrics && value == -1)
+               return;
+
+       printf("%-8s: ", nvme_register_symbol_to_string(offset));
+
+       printf("%#"PRIx64"\n", value);
+
+       if (human)
+               stdout_ctrl_register_human(offset, value, support);
+}

[ikegami-t]

No stdout_ctrl_register_common() returns after the verbose output and before it outputs also the normal output. So the order itself same with the stdout_ctrl_register_support().
Note: The normal outputs below are different since depended on the current show-regs command outputs.

stdout_ctrl_register_support
static void stdout_ctrl_register_common(int offset, uint64_t value, bool fabrics)
{
...
	if (human) {
		printf("%s: %#"PRIx64"\n", name, value);
...
	printf("%s: %#04x (%s), value: %#"PRIx64"\n", type, offset,
	       name, value);
}

[calebsander]

offset and nvme_register_to_string(offset) have already been printed. Is it necessary to do it again here?

[ikegami-t]

Those only printed if the human-readable option not set but this output is only for the option so not duplicated to output actually. By the way fixed the offset output to take 4 hex digits by the commit 227ce4e.

[igaw]

The human flag should only annotate information. If the flag is actually influence the control flow, than this doesn't sound right.

[ikegami-t]

Yes the human flag flow not changed as same with the current show-regs command and the new get-reg command follows the output I think.

[ikegami-t]

Just rebased with the linux-nvme:master.

[ikegami-t]

Still to do belows.

1. Add documentation
2. Have a flag to fallback to 32 bit writes
3. Consider how to figure out the register values changes by the kernel
4. Enforce arguments mutual exclusivity
5. Resolve PR run failures
6. Add support for json and binary print outputs
7. Rebase changes to combine review comment fix patches
8. Add SQ/CQ 0..y tail/head doorbell registers

[igaw]

Please squash your fixes for new code into the new code. There is no need to see the development history in the final version.

[ikegami-t]

Just squashed the changes.

[ikegami-t]

The review comments almost fixed so could you please review again? Still the following items to do but will do separately if you okay.

3. Consider how to figure out the register values changes by the kernel
6. Add support for json and binary print outputs
8. Add SQ/CQ 0..y tail/head doorbell registers
9. Add completions
10. Support for fabrics by set-reg

[ikegami-t]

Just confirmed as the access issue by CC.EN zero caused by the nvme identify namespace to scan NVMe topology sysfs ns nuse attribute and this can be avoided by stopping nvme identify namespace since the attirute not necesary for the PCI register access.

Just considered this but seems no way to do it.. By the way confirmed if CC.EN set to zero then the device not able to be accessed and reset by the timeout error. Also to avoid the error tried to reset the device after the CC.EN zero setting before the access but the CC.EN setting is reset to one by the reset. So seems difficult to let the kernel figure out the command settings.

[igaw]

Thanks!