Changeset View
Standalone View
sys/dev/nvdimm/nvdimm.c
Show First 20 Lines • Show All 177 Lines • ▼ Show 20 Lines | ||||||||||||||||||||||||||||||||||||||||
} | } | |||||||||||||||||||||||||||||||||||||||
static bool | static bool | |||||||||||||||||||||||||||||||||||||||
label_index_is_valid(struct nvdimm_label_index *index, uint32_t max_labels, | label_index_is_valid(struct nvdimm_label_index *index, uint32_t max_labels, | |||||||||||||||||||||||||||||||||||||||
size_t size, size_t offset) | size_t size, size_t offset) | |||||||||||||||||||||||||||||||||||||||
{ | { | |||||||||||||||||||||||||||||||||||||||
uint64_t checksum; | uint64_t checksum; | |||||||||||||||||||||||||||||||||||||||
index = (struct nvdimm_label_index *)((uint8_t *)index + offset); | index = (struct nvdimm_label_index *)((uint8_t *)index + size * offset); | |||||||||||||||||||||||||||||||||||||||
if (strcmp(index->signature, NVDIMM_INDEX_BLOCK_SIGNATURE) != 0) | if (strcmp(index->signature, NVDIMM_INDEX_BLOCK_SIGNATURE) != 0) | |||||||||||||||||||||||||||||||||||||||
return false; | return false; | |||||||||||||||||||||||||||||||||||||||
checksum = index->checksum; | checksum = index->checksum; | |||||||||||||||||||||||||||||||||||||||
index->checksum = 0; | index->checksum = 0; | |||||||||||||||||||||||||||||||||||||||
if (checksum != fletcher64(index, size) || | if (checksum != fletcher64(index, size) || | |||||||||||||||||||||||||||||||||||||||
index->this_offset != size * offset || index->this_size != size || | index->this_offset != size * offset || index->this_size != size || | |||||||||||||||||||||||||||||||||||||||
index->other_offset != size * (offset == 0 ? 1 : 0) || | index->other_offset != size * (offset == 0 ? 1 : 0) || | |||||||||||||||||||||||||||||||||||||||
index->seq == 0 || index->seq > 3 || index->slot_cnt > max_labels || | index->seq == 0 || index->seq > 3 || index->slot_cnt > max_labels || | |||||||||||||||||||||||||||||||||||||||
▲ Show 20 Lines • Show All 42 Lines • ▼ Show 20 Lines | SLIST_FOREACH_SAFE(i, &nv->labels, link, next) { | |||||||||||||||||||||||||||||||||||||||
} | } | |||||||||||||||||||||||||||||||||||||||
} | } | |||||||||||||||||||||||||||||||||||||||
__unreachable(); | __unreachable(); | |||||||||||||||||||||||||||||||||||||||
} | } | |||||||||||||||||||||||||||||||||||||||
static int | static int | |||||||||||||||||||||||||||||||||||||||
read_labels(struct nvdimm_dev *nv) | read_labels(struct nvdimm_dev *nv) | |||||||||||||||||||||||||||||||||||||||
{ | { | |||||||||||||||||||||||||||||||||||||||
struct nvdimm_label_index *indices; | struct nvdimm_label_index *index0, *index1; | |||||||||||||||||||||||||||||||||||||||
size_t bitfield_size, index_size, num_labels; | size_t bitfield_size, index_size, num_labels; | |||||||||||||||||||||||||||||||||||||||
int error, n; | int error, n; | |||||||||||||||||||||||||||||||||||||||
bool index_0_valid, index_1_valid; | bool index_0_valid, index_1_valid; | |||||||||||||||||||||||||||||||||||||||
for (index_size = 256; ; index_size += 256) { | for (index_size = 256; ; index_size += 256) { | |||||||||||||||||||||||||||||||||||||||
num_labels = 8 * (index_size - | num_labels = 8 * (index_size - | |||||||||||||||||||||||||||||||||||||||
sizeof(struct nvdimm_label_index)); | sizeof(struct nvdimm_label_index)); | |||||||||||||||||||||||||||||||||||||||
if (index_size + num_labels * sizeof(struct nvdimm_label) >= | if (index_size + num_labels * sizeof(struct nvdimm_label) >= | |||||||||||||||||||||||||||||||||||||||
nv->label_area_size) | nv->label_area_size) | |||||||||||||||||||||||||||||||||||||||
break; | break; | |||||||||||||||||||||||||||||||||||||||
} | } | |||||||||||||||||||||||||||||||||||||||
num_labels = (nv->label_area_size - index_size) / | num_labels = (nv->label_area_size - index_size) / | |||||||||||||||||||||||||||||||||||||||
sizeof(struct nvdimm_label); | sizeof(struct nvdimm_label); | |||||||||||||||||||||||||||||||||||||||
bitfield_size = roundup2(num_labels, 8) / 8; | bitfield_size = roundup2(num_labels, 8) / 8; | |||||||||||||||||||||||||||||||||||||||
indices = malloc(2 * index_size, M_NVDIMM, M_WAITOK); | index0 = malloc(2 * index_size, M_NVDIMM, M_WAITOK); | |||||||||||||||||||||||||||||||||||||||
error = read_label_area(nv, (void *)indices, 0, 2 * index_size); | index1 = (void *)((uint8_t *)index0 + index_size); | |||||||||||||||||||||||||||||||||||||||
error = read_label_area(nv, (void *)index0, 0, 2 * index_size); | ||||||||||||||||||||||||||||||||||||||||
if (error != 0) { | if (error != 0) { | |||||||||||||||||||||||||||||||||||||||
free(indices, M_NVDIMM); | free(index0, M_NVDIMM); | |||||||||||||||||||||||||||||||||||||||
return (error); | return (error); | |||||||||||||||||||||||||||||||||||||||
} | } | |||||||||||||||||||||||||||||||||||||||
index_0_valid = label_index_is_valid(indices, num_labels, index_size, | index_0_valid = label_index_is_valid(index0, num_labels, index_size, | |||||||||||||||||||||||||||||||||||||||
0); | 0); | |||||||||||||||||||||||||||||||||||||||
index_1_valid = label_index_is_valid(indices, num_labels, index_size, | index_1_valid = label_index_is_valid(index0, num_labels, index_size, | |||||||||||||||||||||||||||||||||||||||
1); | 1); | |||||||||||||||||||||||||||||||||||||||
if (!index_0_valid && !index_1_valid) { | if (!index_0_valid && !index_1_valid) { | |||||||||||||||||||||||||||||||||||||||
free(indices, M_NVDIMM); | free(index0, M_NVDIMM); | |||||||||||||||||||||||||||||||||||||||
return (ENXIO); | return (ENXIO); | |||||||||||||||||||||||||||||||||||||||
} | } | |||||||||||||||||||||||||||||||||||||||
if (index_0_valid && index_1_valid && | if (index_0_valid && index_1_valid && | |||||||||||||||||||||||||||||||||||||||
(indices[1].seq > indices[0].seq || | (index1->seq - index0->seq == 1 || | |||||||||||||||||||||||||||||||||||||||
(indices[1].seq == 1 && indices[0].seq == 3))) | (int)index1->seq - index0->seq == -2)) | |||||||||||||||||||||||||||||||||||||||
index_0_valid = false; | index_0_valid = false; | |||||||||||||||||||||||||||||||||||||||
nv->label_index = malloc(index_size, M_NVDIMM, M_WAITOK); | nv->label_index = malloc(index_size, M_NVDIMM, M_WAITOK); | |||||||||||||||||||||||||||||||||||||||
bcopy(indices + (index_0_valid ? 0 : 1), nv->label_index, index_size); | bcopy(index_0_valid ? index0 : index1, nv->label_index, index_size); | |||||||||||||||||||||||||||||||||||||||
free(indices, M_NVDIMM); | free(index0, M_NVDIMM); | |||||||||||||||||||||||||||||||||||||||
cem: The portion above here seems not to have any functional change? Just refactoring? It's not… | ||||||||||||||||||||||||||||||||||||||||
scottphAuthorUnsubmitted Done Inline ActionsThere 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. scottph: There are a few functional changes above. The important thing to note is that index_size !=… | ||||||||||||||||||||||||||||||||||||||||
cemUnsubmitted Not Done Inline ActionsI 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].) [¹]:
cem: I see, thanks!
I think it might make the logical bugfix clearer to leave `indices` alone… | ||||||||||||||||||||||||||||||||||||||||
for (bit_ffc_at((bitstr_t *)nv->label_index->free, 0, num_labels, &n); | bit_ffc_at((bitstr_t *)nv->label_index->free, 0, | |||||||||||||||||||||||||||||||||||||||
n >= 0; | nv->label_index->slot_cnt, &n); | |||||||||||||||||||||||||||||||||||||||
bit_ffc_at((bitstr_t *)nv->label_index->free, n + 1, num_labels, | while (n >= 0) { | |||||||||||||||||||||||||||||||||||||||
&n)) { | ||||||||||||||||||||||||||||||||||||||||
read_label(nv, n); | read_label(nv, n); | |||||||||||||||||||||||||||||||||||||||
bit_ffc_at((bitstr_t *)nv->label_index->free, n + 1, | ||||||||||||||||||||||||||||||||||||||||
nv->label_index->slot_cnt, &n); | ||||||||||||||||||||||||||||||||||||||||
cemUnsubmitted Not Done Inline ActionsThis part (and label_index_is_valid above) seem to be the functional change? cem: This part (and `label_index_is_valid` above) seem to be the functional change? | ||||||||||||||||||||||||||||||||||||||||
scottphAuthorUnsubmitted Done Inline Actionsyes, if the free bitfield is smaller than it possibly could be, we go with the smaller size in the index block. scottph: yes, if the `free` bitfield is smaller than it possibly could be, we go with the smaller size… | ||||||||||||||||||||||||||||||||||||||||
cemUnsubmitted Not Done Inline ActionsThis part LGTM. Do we sanity check slot_cnt from ACPI is <= num_labels (i.e., it's sane and within bounds)? Should we? cem: This part LGTM. Do we sanity check `slot_cnt` from ACPI is `<= num_labels` (i.e., it's sane… | ||||||||||||||||||||||||||||||||||||||||
} | } | |||||||||||||||||||||||||||||||||||||||
return (0); | return (0); | |||||||||||||||||||||||||||||||||||||||
} | } | |||||||||||||||||||||||||||||||||||||||
struct nvdimm_dev * | struct nvdimm_dev * | |||||||||||||||||||||||||||||||||||||||
nvdimm_find_by_handle(nfit_handle_t nv_handle) | nvdimm_find_by_handle(nfit_handle_t nv_handle) | |||||||||||||||||||||||||||||||||||||||
{ | { | |||||||||||||||||||||||||||||||||||||||
struct nvdimm_dev *res; | struct nvdimm_dev *res; | |||||||||||||||||||||||||||||||||||||||
▲ Show 20 Lines • Show All 107 Lines • Show Last 20 Lines |
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.