libnvme/cmds: change copy desc format 0h and 2h elbt to big-endian

[ikegami-t]

Since the format 1h and 3h elbt values are set as big-endian. Also change the field name eilbrt to elbt as following spec.

[ikegami-t]

@igaw @keithbusch @brandon-paupore-sndk @zwxdg @chenghong085 Could you please review the changes? For the issue: #2975 I have rejected the PR: #2977 then the issue also closed. To make consistent just created this PR changes but for the changes I considered again so still the issue: #2975 mentioned seems correct also.. To make sure should we ask to NVMe org about this specification?
Note: I am thinking to change elbat and elbatm fileds also to big-endian to make consistent (but not sire sure if correct or not).

[igaw]

Would it be possible to have a unit tests for this? Given the amount of discussion around this API I think it would be great to have one. Maybe we could get a binary copy of an existing log page and decode it then?

[ikegami-t]

Okay will do try it later. Thank you.

[ikegami-t]

Just created the separated PR: #3185 for the unit test.
Note: The test expected data is depended on the current API: nvme_init_copy_range if the API behavior changed then the data also needed to be changed.

[igaw]

I wonder if we should rename this struct to nvme_copy_range_f0 because also the spec names this f0.

[igaw]

Looks good. I saw that there are a handful of new fields defined in the newest spec. We might want to add them too. WDYT?

BTW, the commit message doesn't match anymore. This change updates the names of the fields.

[ikegami-t]

Yes I will do rename the struct and add new fields by additional commits. Thank you.

[igaw]

Sorry for my recent changes and you had to rebase your work. I'll just have to read up on the little to big endian change in your explanation. It strikes me a bit odd that one field is big endian, but that might just be my OCD...

[ikegami-t]

No problem. The changes to move the nvme_init_* implementation to header look good. Just changed the odd big endian field to array as same with the copy range format 1 and 3.
Note: The x390x s930x cross build error will be fixed later.

[ikegami-t]

Just changed the copy desc format elbat and elabatm fields as big endian and refixed for the s930x crosss build error.
Note: Still not usre if the copy desc format elbt fields are really big endian as not described clearly and the specification is desribed as below as Unless otherwise specified, this specification specifies data in a little endian format..

[igaw]

Thanks for your effort in getting this sorted out. I’ve spent some time trying to figure out what we should do here, and I hope I understand it correctly now.

As Keith pointed out, the command word endianness is handled by the driver. The application is responsible for getting the payload right.

Figure 39 – Source Range Entries Copy Descriptor Format 0h and Format 2:

ELBT, ELBAT, and ELBATM all reference Section 5.3, which contains the diagrams showing the byte ordering as big-endian.

So, the series here gets it right, albeit after a bit of meandering. I think it would be beneficial to reorganize the series to make the review easier for everyone involved in this discussion.

Could you rearrange the series so that:

• the first patches handle renaming and the addition of new fields from the spec,
• followed by a single patch that converts all fields from little-endian to big-endian in one go?

Also, please rename nvme_init_copy_range_elbt to nvme_init_copy_range_elbts or even better to nvme_init_copy_range_tags, since it is used for all tag types, not just ELBT.

Thanks!

[ikegami-t]

Sure will do. Thank you.

ELBT, ELBAT, and ELBATM all reference Section 5.3, which contains the diagrams showing the byte ordering as big-endian.

But the diagram itself seems not described about the copy command descriptor itself cleary clearly for me. I will do try to understand this again later.

[ikegami-t]

As Keith pointed out, the command word endianness is handled by the driver. The application is responsible for getting the payload right.

About the Dword values fields below also desribed to refert refer the diagram but the values are handled as the little endian by the driver but not the big endian so not handled as same with the copy command descriptor fields ELBT, ELBAT and ELBATM.

1. Dword 2 and 3: LBTU

2. Dword 14 and 15: LBTL, LBATM and LBAT

[ikegami-t]

Just pushed the patches updated as mentioned. Thank you.
Note: Anyway the copy command desc ELBT, ELBAT and ELBATM byte ordrering can be changed to the little endian by the function: nvme_init_copy_range_tags() if needed in future.

[igaw]

Just realized that these are not defined in the public headers of libnvme and I don't think we should.

I think we need to use htobe16 and htobe32 instead.

Would something like

	__be32			elbt;

copy[i].elbt = htobe32(elbts[i]);

not work?

[ikegami-t]

Yes just done as mentioned. Thank you.

[igaw]

yes this looks really good!

[igaw]

Thanks a lot! Sorry that this took so long. But hopefully this code doesn't have to be touched again :)