Changes to support TP 4068 and TP 4084 #358
Conversation
igaw
left a comment
igaw
left a comment
There was a problem hiding this comment.
Unfortunately, this PR breaks the stable API guarantee we gave with the initial 1.0 release. You need to add these changes in a way that it is backwards compatible. E.g. when a struct *_args is extended we have to support the old version as the new one. So there needs some sort of compatibility code introduced:
if (args->size == v1_size) {
/* access members of v1 */
} else if (args->size = v2_size) {
/* access members up to v2 */
}
Signed-off-by: Jeff Lien <jeff.lien@wdc.com>
|
I see the packed attribute is causing CI jobs to fail because of warning messages. I'll have to figure out a different fix to determine the structure size of for each version. |
Maybe this could work: |
|
@igaw Looks like I can fix the warning messages in the CI process by removing the packed attribute from the nvme_io_args structure. The nvme_copy_args structure also does not need the packed attribute. The nvme_format_nvm_args and nvme_zns_append_args structures still need it though. This should resolve the failures from the CI jobs - they were all from references to pointers to specific fields in the nvme_io_args structure. Does that sound acceptable to you? This option doesn't appear to work either. The problem is the nvme_format_nvm_args struct is only 1 byte difference in length from version 1 and 2 and it's padded out either 7 or 6 bytes. The version 1 nvme_zns_append_args struct is padded out 4 bytes. The version 2 struct is 4 bytes longer then version 1 so it uses up the 4 padded bytes and ends up the exact same length. Here's the lengths of the structs in both cases: V2 format nvm args size - 42 (packed, not packed it would be 48) |
|
I think you need to add explicit padding for the new member you want to add if it doesn't align naturally. Let's say you have this here v1: struct foo_args {
u64 bar;
u32 baz;
};and now you want to add another u64 member. v2: struct foo_args {
u64 bar;
u32 baz;
u8 rsvd6[4];
u64 bax;
};The compiler naturally adds padding to the v1 version of the struct, hence we need to add this to our calculation as well. Something like does the trick: #include <stddef.h>
#include <stdio.h>
#include <stdint.h>
typedef uint64_t u64;
typedef uint32_t u32;
typedef uint8_t u8;
struct foo_args_v1 {
u64 bar;
u32 baz;
};
struct foo_args_v2 {
u64 bar;
u32 baz;
u8 rsvd6[4];
u64 box;
};
int main(int argc, char *argv[])
{
int pad;
int baz;
baz = offsetof(struct foo_args_v1, baz) + sizeof(u32);
pad = (sizeof(u64) - (baz%sizeof(u64))) % sizeof(u64);
printf("v1: baz %d pad %d\n", baz, pad);
printf("v1: sizeof(struct foo_args) = %ld\n", sizeof(struct foo_args_v1));
printf("v1: offsetof(struct foo_args, baz) + sizeof(baz) + pad = %ld\n", offsetof(struct foo_args_v1, baz) + sizeof(u32) + pad);
baz = offsetof(struct foo_args_v2, baz) + sizeof(u32);
pad = (sizeof(u64) - (baz%sizeof(u64))) % sizeof(u64);
printf("v2: baz %d pad %d\n", baz, pad);
printf("v2: sizeof(struct foo_args) = %ld\n", sizeof(struct foo_args_v2));
printf("v2: offsetof(struct foo_args, baz) + sizeof(baz) + pad = %ld\n", offsetof(struct foo_args_v2, baz) + sizeof(u32) + pad);
printf("v2: offsetof(struct foo_args, box) + sizeof(box) = %ld\n", offsetof(struct foo_args_v2, box) + sizeof(u64));
} We want probably to introduce a helper function for this kind of calculation #define sizeof_args(type, member, align) \
({ \
type s; \
size_t t = offsetof(type, member) + sizeof(s.member); \
size_t p = (sizeof(align) - (t % sizeof(align))) % sizeof(align); \
t + p; \
})
printf("v1: sizeof_args(struct foo_args, baz, u64) = %ld\n", sizeof_args(struct foo_args_v1, baz, u64));
printf("v2: sizeof_args(struct foo_args, baz, u64) = %ld\n", sizeof_args(struct foo_args_v2, baz, u64));
printf("v2: sizeof_args(struct foo_args, box, u64) = %ld\n", sizeof_args(struct foo_args_v2, box, u64)); |
|
Thanks for the feedback @igaw. I was hoping we could make a simpler fix but don't see how. I'll make changes based on this design and update the PR. |
|
@igaw I think I have all the issues resolved now and have verified nvme cli with both libnvme 1.0 and 1.1 versions. Let me know if you have any comments or find any issues I missed. Thanks |
igaw
left a comment
igaw
left a comment
There was a problem hiding this comment.
Almost there! Sorry for being pedantic but I want to get the versioning handling into good shape so that we have a blueprint for future changes.
There was a problem hiding this comment.
What about this:
int nvme_io(struct nvme_io_args *args, __u8 opcode)
{
const size_t size_v1 = sizeof_args(struct nvme_io_args, dsm, __u64);
const size_t size_v2 = sizeof_args(struct nvme_io_args, pif, __u64);
__u32 cdw2, cdw3, cdw10, cdw11, cdw12, cdw13, cdw14, cdw15;
if (args->args_size < size_v1 || args->args_size > size_v2) {
errno = EINVAL;
return -1;
}
cdw10 = args->slba & 0xffffffff;
cdw11 = args->slba >> 32;
cdw12 = args->nlb | (args->control << 16);
cdw13 = args->dsm | (args->dspec << 16);
cdw15 = args->apptag | (args->appmask << 16);
if (args->args_size == size_v1) {
cdw2 = args->storage_tag & 0xffffffff;
cdw3 = (args->storage_tag >> 32) & 0xffff;
cdw14 = args->reftag;
} else {
if (nvme_set_var_size_tags(&cdw2, &cdw3, &cdw14,
args->pif,
args->sts,
args->reftag_u64,
args->storage_tag)) {
errno = EINVAL;
return -1;
}
}
struct nvme_passthru_cmd cmd = {
.opcode = opcode,
.nsid = args->nsid,
.cdw2 = cdw2,
.cdw3 = cdw3,
.cdw10 = cdw10,
.cdw11 = cdw11,
.cdw12 = cdw12,
.cdw13 = cdw13,
.cdw14 = cdw14,
.cdw15 = cdw15,
.data_len = args->data_len,
.metadata_len = args->metadata_len,
.addr = (__u64)(uintptr_t)args->data,
.metadata = (__u64)(uintptr_t)args->metadata,
.timeout_ms = args->timeout,
};
return nvme_submit_io_passthru(args->fd, &cmd, args->result);
}This follows the code order the nvme_format_nvm version above.
There was a problem hiding this comment.
Also the original changes were addressing a bug in the current handling of storage_tag, in that the spec defines both variable-sized tags as able to take the back 16 bits of cdw2, the full cdw3, and full cdw14.
Calling it a "storage tag" feels a bit wrong when it could be entirely part of a larger reference tag, but in any case it shouldn't be setting the full bits of cdw2 and should be setting cdw3.
There was a problem hiding this comment.
Changed the code to the suggested way above. Also fixed the original leg to be correct based on @bjpaupor comment.
Adds support for expanded reference and storage tags, automatically placing them into the appropriate bits based on Protection Information Format and Storage Tag Size. This includes updates to nvme_io_args expanding the size of the tags and adding these STS and PIF values. Also adds a new copy range format as defined by the NVMe 2.0 spec, as it determines the reference/storage tags associated to the range being copied. Signed-off-by: Brandon Paupore <brandon.paupore@wdc.com> Signed-off-by: Jeff Lien <jeff.lien@wdc.com>
Signed-off-by: Jeff Lien <jeff.lien@wdc.com>
|
I've fixed a few whitespace and docs issues. No code changes. |
4 participants
TP 4068 - Protection Information Enchancement
TP 4084 - Time-to-Ready Enhancements