| Message ID | 20210222184759.65041-6-its@irrelevant.dk |
|---|---|
| State | New |
| Headers | show |
| Series | hw/block/nvme: misc fixes | expand |
On Mon, Feb 22, 2021 at 07:47:59PM +0100, Klaus Jensen wrote: > +typedef struct NvmeIdCtrlNvm { > + uint8_t vsl; > + uint8_t wzsl; > + uint8_t wusl; > + uint8_t dmrl; > + uint32_t dmrsl; > + uint64_t dmsl; > + uint8_t rsvd16[4080]; > +} NvmeIdCtrlNvm; TP 4040a still displays these fields with preceding '...' indicating something comes before this. Is that just left-over from the integration for TBD offsets, or is there something that still hasn't been accounted for?
On Feb 23 05:55, Keith Busch wrote: > On Mon, Feb 22, 2021 at 07:47:59PM +0100, Klaus Jensen wrote: > > +typedef struct NvmeIdCtrlNvm { > > + uint8_t vsl; > > + uint8_t wzsl; > > + uint8_t wusl; > > + uint8_t dmrl; > > + uint32_t dmrsl; > > + uint64_t dmsl; > > + uint8_t rsvd16[4080]; > > +} NvmeIdCtrlNvm; > > TP 4040a still displays these fields with preceding '...' indicating > something comes before this. Is that just left-over from the integration > for TBD offsets, or is there something that still hasn't been accounted > for? Good question. But since the TBDs have been assigned I believe it is just a left-over. I must admit that I have not cross checked this with all other TPs, but AFAIK this is the only ratified TP that adds something to the NVM-specific identify controller data structure.
On Feb 22 22:12, Klaus Jensen wrote: > On Feb 23 05:55, Keith Busch wrote: > > On Mon, Feb 22, 2021 at 07:47:59PM +0100, Klaus Jensen wrote: > > > +typedef struct NvmeIdCtrlNvm { > > > + uint8_t vsl; > > > + uint8_t wzsl; > > > + uint8_t wusl; > > > + uint8_t dmrl; > > > + uint32_t dmrsl; > > > + uint64_t dmsl; > > > + uint8_t rsvd16[4080]; > > > +} NvmeIdCtrlNvm; > > > > TP 4040a still displays these fields with preceding '...' indicating > > something comes before this. Is that just left-over from the integration > > for TBD offsets, or is there something that still hasn't been accounted > > for? > > Good question. > > But since the TBDs have been assigned I believe it is just a left-over. > I must admit that I have not cross checked this with all other TPs, but > AFAIK this is the only ratified TP that adds something to the > NVM-specific identify controller data structure. Are you otherwise OK with this?
On Mon, Mar 01, 2021 at 12:15:26PM +0100, Klaus Jensen wrote: > On Feb 22 22:12, Klaus Jensen wrote: > > On Feb 23 05:55, Keith Busch wrote: > > > On Mon, Feb 22, 2021 at 07:47:59PM +0100, Klaus Jensen wrote: > > > > +typedef struct NvmeIdCtrlNvm { > > > > + uint8_t vsl; > > > > + uint8_t wzsl; > > > > + uint8_t wusl; > > > > + uint8_t dmrl; > > > > + uint32_t dmrsl; > > > > + uint64_t dmsl; > > > > + uint8_t rsvd16[4080]; > > > > +} NvmeIdCtrlNvm; > > > > > > TP 4040a still displays these fields with preceding '...' indicating > > > something comes before this. Is that just left-over from the integration > > > for TBD offsets, or is there something that still hasn't been accounted > > > for? > > > > Good question. > > > > But since the TBDs have been assigned I believe it is just a left-over. > > I must admit that I have not cross checked this with all other TPs, but > > AFAIK this is the only ratified TP that adds something to the > > NVM-specific identify controller data structure. > > Are you otherwise OK with this? Yes, I compared other TP's and it appears to be set for good once an actual numeric value is assigned, so okay to go here. Reviewed-by: Keith Busch <kbusch@kernel.org>
diff --git a/hw/block/nvme.h b/hw/block/nvme.h index cb2b5175f1a1..3046b82b3da1 100644 --- a/hw/block/nvme.h +++ b/hw/block/nvme.h @@ -172,6 +172,7 @@ typedef struct NvmeCtrl { int aer_queued; uint8_t zasl; + uint32_t dmrsl; NvmeSubsystem *subsys; diff --git a/include/block/nvme.h b/include/block/nvme.h index b23f3ae2279f..16d8c4c90f7e 100644 --- a/include/block/nvme.h +++ b/include/block/nvme.h @@ -1041,6 +1041,16 @@ typedef struct NvmeIdCtrlZoned { uint8_t rsvd1[4095]; } NvmeIdCtrlZoned; +typedef struct NvmeIdCtrlNvm { + uint8_t vsl; + uint8_t wzsl; + uint8_t wusl; + uint8_t dmrl; + uint32_t dmrsl; + uint64_t dmsl; + uint8_t rsvd16[4080]; +} NvmeIdCtrlNvm; + enum NvmeIdCtrlOacs { NVME_OACS_SECURITY = 1 << 0, NVME_OACS_FORMAT = 1 << 1, @@ -1396,6 +1406,7 @@ static inline void _nvme_check_size(void) QEMU_BUILD_BUG_ON(sizeof(NvmeEffectsLog) != 4096); QEMU_BUILD_BUG_ON(sizeof(NvmeIdCtrl) != 4096); QEMU_BUILD_BUG_ON(sizeof(NvmeIdCtrlZoned) != 4096); + QEMU_BUILD_BUG_ON(sizeof(NvmeIdCtrlNvm) != 4096); QEMU_BUILD_BUG_ON(sizeof(NvmeLBAF) != 4); QEMU_BUILD_BUG_ON(sizeof(NvmeLBAFE) != 16); QEMU_BUILD_BUG_ON(sizeof(NvmeIdNs) != 4096); diff --git a/hw/block/nvme.c b/hw/block/nvme.c index b641c7a45a2f..4013bf53d4ff 100644 --- a/hw/block/nvme.c +++ b/hw/block/nvme.c @@ -1775,6 +1775,10 @@ static uint16_t nvme_dsm(NvmeCtrl *n, NvmeRequest *req) trace_pci_nvme_dsm_deallocate(nvme_cid(req), nvme_nsid(ns), slba, nlb); + if (nlb > n->dmrsl) { + trace_pci_nvme_dsm_single_range_limit_exceeded(nlb, n->dmrsl); + } + offset = nvme_l2b(ns, slba); len = nvme_l2b(ns, nlb); @@ -3200,21 +3204,27 @@ static uint16_t nvme_identify_ctrl(NvmeCtrl *n, NvmeRequest *req) static uint16_t nvme_identify_ctrl_csi(NvmeCtrl *n, NvmeRequest *req) { NvmeIdentify *c = (NvmeIdentify *)&req->cmd; - NvmeIdCtrlZoned id = {}; + uint8_t id[NVME_IDENTIFY_DATA_SIZE] = {}; trace_pci_nvme_identify_ctrl_csi(c->csi); - if (c->csi == NVME_CSI_NVM) { - return nvme_rpt_empty_id_struct(n, req); - } else if (c->csi == NVME_CSI_ZONED) { + switch (c->csi) { + case NVME_CSI_NVM: + ((NvmeIdCtrlNvm *)&id)->dmrsl = cpu_to_le32(n->dmrsl); + break; + + case NVME_CSI_ZONED: if (n->params.zasl_bs) { - id.zasl = n->zasl; + ((NvmeIdCtrlZoned *)&id)->zasl = n->zasl; } - return nvme_dma(n, (uint8_t *)&id, sizeof(id), - DMA_DIRECTION_FROM_DEVICE, req); + + break; + + default: + return NVME_INVALID_FIELD | NVME_DNR; } - return NVME_INVALID_FIELD | NVME_DNR; + return nvme_dma(n, id, sizeof(id), DMA_DIRECTION_FROM_DEVICE, req); } static uint16_t nvme_identify_ns(NvmeCtrl *n, NvmeRequest *req) @@ -4668,6 +4678,9 @@ int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp) n->namespaces[nsid - 1] = ns; + n->dmrsl = MIN_NON_ZERO(n->dmrsl, + BDRV_REQUEST_MAX_BYTES / nvme_l2b(ns, 1)); + return 0; } diff --git a/hw/block/trace-events b/hw/block/trace-events index 1f958d09d2a9..27940fe2e98a 100644 --- a/hw/block/trace-events +++ b/hw/block/trace-events @@ -51,6 +51,7 @@ pci_nvme_copy_cb(uint16_t cid) "cid %"PRIu16"" pci_nvme_block_status(int64_t offset, int64_t bytes, int64_t pnum, int ret, bool zeroed) "offset %"PRId64" bytes %"PRId64" pnum %"PRId64" ret 0x%x zeroed %d" pci_nvme_dsm(uint16_t cid, uint32_t nsid, uint32_t nr, uint32_t attr) "cid %"PRIu16" nsid %"PRIu32" nr %"PRIu32" attr 0x%"PRIx32"" pci_nvme_dsm_deallocate(uint16_t cid, uint32_t nsid, uint64_t slba, uint32_t nlb) "cid %"PRIu16" nsid %"PRIu32" slba %"PRIu64" nlb %"PRIu32"" +pci_nvme_dsm_single_range_limit_exceeded(uint32_t nlb, uint32_t dmrsl) "nlb %"PRIu32" dmrsl %"PRIu32"" pci_nvme_compare(uint16_t cid, uint32_t nsid, uint64_t slba, uint32_t nlb) "cid %"PRIu16" nsid %"PRIu32" slba 0x%"PRIx64" nlb %"PRIu32"" pci_nvme_compare_cb(uint16_t cid) "cid %"PRIu16"" pci_nvme_aio_discard_cb(uint16_t cid) "cid %"PRIu16""