Skip to content

Revert various related big-endian PI commits#982

Closed
brandon-paupore-sndk wants to merge 1 commit intolinux-nvme:masterfrom
brandon-paupore-sndk:le-shared-tags
Closed

Revert various related big-endian PI commits#982
brandon-paupore-sndk wants to merge 1 commit intolinux-nvme:masterfrom
brandon-paupore-sndk:le-shared-tags

Conversation

@brandon-paupore-sndk
Copy link
Copy Markdown
Contributor

This reverts a series of related changes that mapped storage and reference tags into the shared region over multiple CDWords in big-endian ordering. Keith confirmed the CDWords are always in little-endian ordering, and that the MSB-LSB representation in the spec is for the metadata payload itself.

Comment thread src/nvme/ioctl.c Outdated
cdw2 = (args->storage_tag >> 32) & 0xffff;
cdw3 = args->storage_tag & 0xffffffff;
cdw14 = args->reftag;
cdw2 = cpu_to_le32((args->storage_tag >> 32) & 0xffff);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still wrong, you should never have to do any byte swapping for the command dwords. The driver will take care of that if necessary. Just store the command values in native cpu format.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know, will update the commit message as well.

This reverts a series of related changes that mapped storage and
reference tags into the shared region over multiple CDWords in
big-endian ordering. Keith confirmed the CDWords are always in
host-endian ordering, and that the MSB-LSB representation in the spec
is for the metadata payload itself as handled by the driver.

Signed-off-by: Brandon Paupore <[email protected]>
@igaw
Copy link
Copy Markdown
Collaborator

igaw commented Apr 3, 2025

@keithbusch this PR also updates the nvme_init_copy_range_elbt function. I think I take this version (after updating the commit message slightly) if this is okay with you.

@keithbusch
Copy link
Copy Markdown
Contributor

keithbusch commented Apr 3, 2025

The copy command's elbt field is part of the payload, so that part does need the host to do whatever encoding manipulation is required. The patch will mean big vs little endian CPU's will produce different payloads, so I don't think it's right. But I am not sure exactly what the format is supposed to be here, the spec is a bit confusing on this. I'd just drop that part from the patch.

@igaw
Copy link
Copy Markdown
Collaborator

igaw commented Apr 3, 2025

Thanks, so that means your patch on the mailing list the likely the only fix needed. Gosh, I already thought I am too stupid to read the spec. This part of the spec is completely bonkers.

@brandon-paupore-sndk
Copy link
Copy Markdown
Contributor Author

Seems good to me, thanks for the details Keith.

Feel free to close this, or if you prefer I could update to just have the non-payload updates here.

@igaw
Copy link
Copy Markdown
Collaborator

igaw commented Apr 3, 2025

Okay, I'll take the version from Keith. Thanks for everyone helping out with this one. Highly appreciated!

@igaw igaw closed this Apr 3, 2025
@keithbusch
Copy link
Copy Markdown
Contributor

Gosh, I already thought I am too stupid to read the spec. This part of the spec is completely bonkers.

And we didn't even have a copy-offload talk at LSFMM this year. We're losing our touch...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants