The byte order of Expected Initial Logical Block Reference Tag in copy fmt1 source range

[chenghong085]

The function nvme_init_copy_range_elbt converts eilbrt to big-endian, I think it should be little-endian, same as eilbrt in fmt0.

static inline void nvme_init_copy_range_elbt(__u8 *elbt, __u64 eilbrt)
{
	int i;

	for (i = 0; i < 8; i++)
		elbt[9 - i] = (eilbrt >> (8 * i)) & 0xff;
	elbt[1] = 0;
	elbt[0] = 0;
}

[ikegami-t]

Just created the PR #2977 to fix this. Thank you.

[bjpaupor]

For reference, this was considered previously and intentionally left as-is based on linux-nvme/libnvme#982 (comment).

[ikegami-t]

Thanks for your comment. The PR: #982 linux-nvme/libnvme#982 changes includes for Dwords values to set CPU endian (and it is converted to little endian by the driver) and copy descriptor value to set CPU endian (but it is not converted) so seems not correct for the copy descriptor value. Also currently it is set the copy descriptor value to big endian as mentioned by the issue then the PR: #2977 changes to set little endian.

[MorningWeicheng]

I just found that the storage tag in DW2 has a similar big-endian issue in nvme-cli version 2.10. Is the fix from PR:982 included?

[ikegami-t]

The nvme-cli version 2.10 implementation below so seems cause the issue mentioned.

static int nvme_set_var_size_tags(__u32 *cmd_dw2, __u32 *cmd_dw3, __u32 *cmd_dw14,
		__u8 pif, __u8 sts, __u64 reftag, __u64 storage_tag)
{
...
	*cmd_dw2 = cpu_to_be32(cdw2);
	*cmd_dw3 = cpu_to_be32(cdw3);
	*cmd_dw14 = cpu_to_be32(cdw14);

Also currently implemented as below so looks the issue is already resolved without the PR: linux-nvme/libnvme#982 changes.

static inline int
nvme_init_var_size_tags(struct nvme_passthru_cmd64 *cmd,
		__u8 pif, __u8 sts, __u64 reftag, __u64 storage_tag)
{
...
	cmd->cdw2 = cdw2;
	cmd->cdw3 = cdw3;
	cmd->cdw14 = cdw14;

Note: The PR: linux-nvme/libnvme#982 changes closed without the changes applied.

[keithbusch]

All Command DWORDS need to be set in host-native format from user space. The kernel driver handles any architectural byte order conditions for the NVMe protocol.

The driver does not touch the data payloads, so the application creating that payload needs the byte ordering to be set as desired.

That said, the Application Tag is an opaque value, so while the spec defines the byte oder for the data integrity field as big endian, this field really doesn't matter as long as you're consistent on the host. The only time there might be a problem is if you're accessing shared storage from two different machines where one is big and the other is little.

[chenghong085]

@ikegami-t Since the Reference Tag uses the least significant bits in Storage and Reference Space, I think if we're going to change elbt to little-endian, elbt[8/9] should be set to 0, but not elbt[0/1] as before. In addition, the code assumes the cpu byte ordering is little-endian, should we use cpu_to_le64 to make it more compatible?

[keithbusch]

That said, the Application Tag is an opaque value, so while the spec defines the byte oder for the data integrity field as big endian, this field really doesn't matter as long as you're consistent on the host.

Eh, I should take that back. The ELBAT field means the controller has to interpret it. I've just never seen anything put a value other than '0' there, so I must be out of touch.

[chenghong085]

@keithbusch Yes, as an SSD developer, when implementing the copy command with PI enabled, I found that the behavior of nvme-cli is different with my understanding, that‘s why I asked this question.

[igaw]

Why I am not surprised that we are looking at this monster again.

Anyway, digging a bit into the history:

• 0d30777 ("types: Update position of elbatm in struct nvme_copy_range{_f1}") claims the tags are defined as big endian.
• update the Expected Logical Block Application Tag and Reference Tag in nvme copy command libnvme#587

I got convinced that this is big endian.

Also found linux-nvme/libnvme#455 which we could reconsider as we currently in the develompment stage for the upcoming 3.x release which allows us to break APIs

@zwxdg
@bjpaupor
@Francis-Pravin

[chenghong085]

@ikegami-t Since the Reference Tag uses the least significant bits in Storage and Reference Space, I think if we're going to change elbt to little-endian, elbt[8/9] should be set to 0, but not elbt[0/1] as before. In addition, the code assumes the cpu byte ordering is little-endian, should we use cpu_to_le64 to make it more compatible?

I should not mention the Storage and Reference Space since it's for the PI fields in the metadata payload. From my understanding, the ELBT field in the copy source range is same as the combined bits with CDW14, CDW3 and CDW2, so it should be little-endian with EILBRT in the least significant bits.

[ikegami-t]

Just fixed the PR: #2977 for the comment.