nvdimm(4): Fix various problems when the using the second label index block

[scottph]

Summary

struct nvdimm_label_index is dynamically sized, with the free
bitfield expanding to hold slot_cnt entries. Fix a few places
where we were treating the struct as though it had a fixed sized.

[cem]

Thanks!

Some initial tentative feedback and CCing kib@. Caveat, I haven't yet consulted any official documentation nor have I looked at the adjacent code to understand what it is doing, so my comments may just be uninformed. But, provisionally, I don't understand the motivation for some of this changeset.

sys/dev/nvdimm/nvdimm.c

262–297

The portion above here seems not to have any functional change? Just refactoring? It's not clear to me why these changes were made; in particular, they seem less clear to me, and they don't seem to change any aspect of how label_index's dynamic-sizing is handled. I might be missing something, it's late Pacific time.

294–299

This part (and label_index_is_valid above) seem to be the functional change?

[scottph]

In D22253#486458, @cem wrote:

Thanks!

Some initial tentative feedback and CCing kib@. Caveat, I haven't yet consulted any official documentation nor have I looked at the adjacent code to understand what it is doing, so my comments may just be uninformed. But, provisionally, I don't understand the motivation for some of this changeset.

No worries, the relevant spec material to look at for NVDIMM labels is the UEFI
spec, sec 13.19. The description of the label index block begins
unceremoniously after the listing of return codes for LabelStorageWrite(). A
few pages later, under the heading of Label Storage Area Description, it says:

The size of an Index Block depends on how many label slots fit into the Label
Storage Area. The minimum size of an Index Block is 256 bytes and the size
must be a multiple of EFI_NVDIMM_LABEL_INDEX_ALIGN bytes. As necessary,
padding with zero bytes at the end of the structure is used to meet these
size requirements. The minimum size of the Label Storage Area is large
enough to hold 2 index blocks and 2 labels.

sys/dev/nvdimm/nvdimm.c

262–297

There are a few functional changes above. The important thing to note is that index_size != sizeof(struct nvdimm_label_index). index_size is required to be a multiple of EFI_NVDIMM_LABEL_INDEX_ALIGN (= 256), where sizeof(struct nvdimm_label_index) == 72. So before, where index[1] meant to index to the second label index block, it actually was indexing to the address of index[0].free

Also, in the seq comparison code above, the check of index1.seq > index0.seq would incorrectly assign the priority to index1 in the index0.seq==1 && index1.seq==3 case, where priority should go to index0.

294–299

yes, if the free bitfield is smaller than it possibly could be, we go with the smaller size in the index block.

[cem]

sys/dev/nvdimm/nvdimm.c

262–297

I see, thanks!

I think it might make the logical bugfix clearer to leave indices alone, adding the new index1 with the careful arithmetic and removing references to indices[1]. (Much like how we refer to getdirentries buffers with struct dirent despite a trailing variable-length member.)

As far as the sequence number comparison, I think the update wheel algorithm described in the UEFI spec (1 -> 2 -> 3 -> 1) might be more clearly expressed with subtraction mod 3. I believe this is correct (it's easy to verify exhaustively¹):

if (index_0_valid && index_1_valid) {
    if (((int)index1->seq - (int)index0->seq + 3) % 3 == 1) {
        /* index 1 was mostly recently updated */
        index_0_valid = false;
    } else {
        /* index 0 was mostly recently updated */
        index_1_valid = false;
    }
}

(The explicit +3 is needed because C's % operator isn't really modulus and doesn't work on negative integers. Before + 3, the result must be in the range [-2, 2].)

[¹]:

idx1->seq	idx0->seq	difference plus 3	mod 3	newer index
1	2	2	2	0
1	3	1	1	1
2	1	4	1	1
2	3	2	2	0
3	1	5	2	0
3	2	4	1	1

294–299

This part LGTM. Do we sanity check slot_cnt from ACPI is <= num_labels (i.e., it's sane and within bounds)? Should we?

[cem]

Change looks functionally correct to me, my suggestions are all stylistic.

[scottph]

Updated with @cem's suggestions. Reposting for mentor approval.