| Message ID | 20200415055140.466900-13-its@irrelevant.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nvme: support NVMe v1.3d, SGLs and multiple namespaces | expand |
On 4/15/20 7:51 AM, Klaus Jensen wrote: > From: Klaus Jensen <k.jensen@samsung.com> > > It might seem wierd to implement this feature for an emulated device, 'weird' > but it is mandatory to support and the feature is useful for testing > asynchronous event request support, which will be added in a later > patch. Which patch? I can't find how you set the temperature in this series. > > Signed-off-by: Klaus Jensen <k.jensen@samsung.com> > Acked-by: Keith Busch <kbusch@kernel.org> > Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> > --- > hw/block/nvme.c | 48 ++++++++++++++++++++++++++++++++++++++++++++ > hw/block/nvme.h | 1 + > include/block/nvme.h | 8 +++++++- > 3 files changed, 56 insertions(+), 1 deletion(-) > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c > index d1c42ee4765c..e777cc9075c1 100644 > --- a/hw/block/nvme.c > +++ b/hw/block/nvme.c > @@ -45,6 +45,9 @@ > #include "nvme.h" > > #define NVME_CMB_BIR 2 > +#define NVME_TEMPERATURE 0x143 > +#define NVME_TEMPERATURE_WARNING 0x157 > +#define NVME_TEMPERATURE_CRITICAL 0x175 > > #define NVME_GUEST_ERR(trace, fmt, ...) \ > do { \ > @@ -798,9 +801,31 @@ static uint16_t nvme_get_feature_timestamp(NvmeCtrl *n, NvmeCmd *cmd) > static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req) > { > uint32_t dw10 = le32_to_cpu(cmd->cdw10); > + uint32_t dw11 = le32_to_cpu(cmd->cdw11); > uint32_t result; > > switch (dw10) { > + case NVME_TEMPERATURE_THRESHOLD: > + result = 0; > + > + /* > + * The controller only implements the Composite Temperature sensor, so > + * return 0 for all other sensors. > + */ > + if (NVME_TEMP_TMPSEL(dw11) != NVME_TEMP_TMPSEL_COMPOSITE) { > + break; > + } > + > + switch (NVME_TEMP_THSEL(dw11)) { > + case NVME_TEMP_THSEL_OVER: > + result = cpu_to_le16(n->features.temp_thresh_hi); > + break; > + case NVME_TEMP_THSEL_UNDER: > + result = cpu_to_le16(n->features.temp_thresh_low); > + break; > + } > + > + break; > case NVME_VOLATILE_WRITE_CACHE: > result = blk_enable_write_cache(n->conf.blk); > trace_nvme_dev_getfeat_vwcache(result ? "enabled" : "disabled"); > @@ -845,6 +870,23 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req) > uint32_t dw11 = le32_to_cpu(cmd->cdw11); > > switch (dw10) { > + case NVME_TEMPERATURE_THRESHOLD: > + if (NVME_TEMP_TMPSEL(dw11) != NVME_TEMP_TMPSEL_COMPOSITE) { > + break; > + } > + > + switch (NVME_TEMP_THSEL(dw11)) { > + case NVME_TEMP_THSEL_OVER: > + n->features.temp_thresh_hi = NVME_TEMP_TMPTH(dw11); > + break; > + case NVME_TEMP_THSEL_UNDER: > + n->features.temp_thresh_low = NVME_TEMP_TMPTH(dw11); > + break; > + default: > + return NVME_INVALID_FIELD | NVME_DNR; > + } > + > + break; > case NVME_VOLATILE_WRITE_CACHE: > blk_set_enable_write_cache(n->conf.blk, dw11 & 1); > break; > @@ -1373,6 +1415,7 @@ static void nvme_init_state(NvmeCtrl *n) > n->namespaces = g_new0(NvmeNamespace, n->num_namespaces); > n->sq = g_new0(NvmeSQueue *, n->params.max_ioqpairs + 1); > n->cq = g_new0(NvmeCQueue *, n->params.max_ioqpairs + 1); > + n->features.temp_thresh_hi = NVME_TEMPERATURE_WARNING; > } > > static void nvme_init_cmb(NvmeCtrl *n, PCIDevice *pci_dev) > @@ -1450,6 +1493,11 @@ static void nvme_init_ctrl(NvmeCtrl *n) > id->acl = 3; > id->frmw = 7 << 1; > id->lpa = 1 << 0; > + > + /* recommended default value (~70 C) */ > + id->wctemp = cpu_to_le16(NVME_TEMPERATURE_WARNING); > + id->cctemp = cpu_to_le16(NVME_TEMPERATURE_CRITICAL); > + > id->sqes = (0x6 << 4) | 0x6; > id->cqes = (0x4 << 4) | 0x4; > id->nn = cpu_to_le32(n->num_namespaces); > diff --git a/hw/block/nvme.h b/hw/block/nvme.h > index b7c465560eea..807c4ad19dcc 100644 > --- a/hw/block/nvme.h > +++ b/hw/block/nvme.h > @@ -115,6 +115,7 @@ typedef struct NvmeCtrl { > NvmeSQueue admin_sq; > NvmeCQueue admin_cq; > NvmeIdCtrl id_ctrl; > + NvmeFeatureVal features; > } NvmeCtrl; > > static inline uint64_t nvme_ns_nlbas(NvmeCtrl *n, NvmeNamespace *ns) > diff --git a/include/block/nvme.h b/include/block/nvme.h > index b30744068d46..a0519814ecec 100644 > --- a/include/block/nvme.h > +++ b/include/block/nvme.h > @@ -688,7 +688,13 @@ enum NvmeIdCtrlOncs { > typedef struct NvmeFeatureVal { > uint32_t arbitration; > uint32_t power_mgmt; > - uint32_t temp_thresh; > + union { > + struct { > + uint16_t temp_thresh_hi; > + uint16_t temp_thresh_low; > + }; > + uint32_t temp_thresh; > + }; > uint32_t err_rec; > uint32_t volatile_wc; > uint32_t num_queues; >
On Apr 15 09:19, Philippe Mathieu-Daudé wrote: > On 4/15/20 7:51 AM, Klaus Jensen wrote: > > From: Klaus Jensen <k.jensen@samsung.com> > > > > It might seem wierd to implement this feature for an emulated device, > > 'weird' Thanks, fixed :) > > > but it is mandatory to support and the feature is useful for testing > > asynchronous event request support, which will be added in a later > > patch. > > Which patch? I can't find how you set the temperature in this series. > The temperature cannot be changed, but the thresholds can with the Set Features command (and that can then trigger AERs). That is added in "nvme: add temperature threshold feature" and "nvme: add support for the asynchronous event request command" respectively. There is a test in SPDK that does this.
On Apr 15 09:24, Klaus Birkelund Jensen wrote: > On Apr 15 09:19, Philippe Mathieu-Daudé wrote: > > On 4/15/20 7:51 AM, Klaus Jensen wrote: > > > From: Klaus Jensen <k.jensen@samsung.com> > > > > > > It might seem wierd to implement this feature for an emulated device, > > > > 'weird' > > Thanks, fixed :) > > > > > > but it is mandatory to support and the feature is useful for testing > > > asynchronous event request support, which will be added in a later > > > patch. > > > > Which patch? I can't find how you set the temperature in this series. > > > > The temperature cannot be changed, but the thresholds can with the Set > Features command (and that can then trigger AERs). That is added in > "nvme: add temperature threshold feature" and "nvme: add support for the > asynchronous event request command" respectively. > > There is a test in SPDK that does this. > Oh, I think I misunderstood you. No, setting the temperature was moved to the "nvme: add support for the get log page command" patch since that is the patch that actually uses it. This was on request by Maxim in an earlier review.
On 4/15/20 9:28 AM, Klaus Birkelund Jensen wrote: > On Apr 15 09:24, Klaus Birkelund Jensen wrote: >> On Apr 15 09:19, Philippe Mathieu-Daudé wrote: >>> On 4/15/20 7:51 AM, Klaus Jensen wrote: >>>> From: Klaus Jensen <k.jensen@samsung.com> >>>> >>>> It might seem wierd to implement this feature for an emulated device, >>> >>> 'weird' >> >> Thanks, fixed :) >> >>> >>>> but it is mandatory to support and the feature is useful for testing >>>> asynchronous event request support, which will be added in a later >>>> patch. >>> >>> Which patch? I can't find how you set the temperature in this series. >>> >> >> The temperature cannot be changed, but the thresholds can with the Set >> Features command (and that can then trigger AERs). That is added in >> "nvme: add temperature threshold feature" and "nvme: add support for the >> asynchronous event request command" respectively. >> >> There is a test in SPDK that does this. >> > > Oh, I think I misunderstood you. > > No, setting the temperature was moved to the "nvme: add support for the > get log page command" patch since that is the patch that actually uses > it. This was on request by Maxim in an earlier review. I was expecting to see a QMP command to modify the device temperature at runtime.
diff --git a/hw/block/nvme.c b/hw/block/nvme.c index d1c42ee4765c..e777cc9075c1 100644 --- a/hw/block/nvme.c +++ b/hw/block/nvme.c @@ -45,6 +45,9 @@ #include "nvme.h" #define NVME_CMB_BIR 2 +#define NVME_TEMPERATURE 0x143 +#define NVME_TEMPERATURE_WARNING 0x157 +#define NVME_TEMPERATURE_CRITICAL 0x175 #define NVME_GUEST_ERR(trace, fmt, ...) \ do { \ @@ -798,9 +801,31 @@ static uint16_t nvme_get_feature_timestamp(NvmeCtrl *n, NvmeCmd *cmd) static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req) { uint32_t dw10 = le32_to_cpu(cmd->cdw10); + uint32_t dw11 = le32_to_cpu(cmd->cdw11); uint32_t result; switch (dw10) { + case NVME_TEMPERATURE_THRESHOLD: + result = 0; + + /* + * The controller only implements the Composite Temperature sensor, so + * return 0 for all other sensors. + */ + if (NVME_TEMP_TMPSEL(dw11) != NVME_TEMP_TMPSEL_COMPOSITE) { + break; + } + + switch (NVME_TEMP_THSEL(dw11)) { + case NVME_TEMP_THSEL_OVER: + result = cpu_to_le16(n->features.temp_thresh_hi); + break; + case NVME_TEMP_THSEL_UNDER: + result = cpu_to_le16(n->features.temp_thresh_low); + break; + } + + break; case NVME_VOLATILE_WRITE_CACHE: result = blk_enable_write_cache(n->conf.blk); trace_nvme_dev_getfeat_vwcache(result ? "enabled" : "disabled"); @@ -845,6 +870,23 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req) uint32_t dw11 = le32_to_cpu(cmd->cdw11); switch (dw10) { + case NVME_TEMPERATURE_THRESHOLD: + if (NVME_TEMP_TMPSEL(dw11) != NVME_TEMP_TMPSEL_COMPOSITE) { + break; + } + + switch (NVME_TEMP_THSEL(dw11)) { + case NVME_TEMP_THSEL_OVER: + n->features.temp_thresh_hi = NVME_TEMP_TMPTH(dw11); + break; + case NVME_TEMP_THSEL_UNDER: + n->features.temp_thresh_low = NVME_TEMP_TMPTH(dw11); + break; + default: + return NVME_INVALID_FIELD | NVME_DNR; + } + + break; case NVME_VOLATILE_WRITE_CACHE: blk_set_enable_write_cache(n->conf.blk, dw11 & 1); break; @@ -1373,6 +1415,7 @@ static void nvme_init_state(NvmeCtrl *n) n->namespaces = g_new0(NvmeNamespace, n->num_namespaces); n->sq = g_new0(NvmeSQueue *, n->params.max_ioqpairs + 1); n->cq = g_new0(NvmeCQueue *, n->params.max_ioqpairs + 1); + n->features.temp_thresh_hi = NVME_TEMPERATURE_WARNING; } static void nvme_init_cmb(NvmeCtrl *n, PCIDevice *pci_dev) @@ -1450,6 +1493,11 @@ static void nvme_init_ctrl(NvmeCtrl *n) id->acl = 3; id->frmw = 7 << 1; id->lpa = 1 << 0; + + /* recommended default value (~70 C) */ + id->wctemp = cpu_to_le16(NVME_TEMPERATURE_WARNING); + id->cctemp = cpu_to_le16(NVME_TEMPERATURE_CRITICAL); + id->sqes = (0x6 << 4) | 0x6; id->cqes = (0x4 << 4) | 0x4; id->nn = cpu_to_le32(n->num_namespaces); diff --git a/hw/block/nvme.h b/hw/block/nvme.h index b7c465560eea..807c4ad19dcc 100644 --- a/hw/block/nvme.h +++ b/hw/block/nvme.h @@ -115,6 +115,7 @@ typedef struct NvmeCtrl { NvmeSQueue admin_sq; NvmeCQueue admin_cq; NvmeIdCtrl id_ctrl; + NvmeFeatureVal features; } NvmeCtrl; static inline uint64_t nvme_ns_nlbas(NvmeCtrl *n, NvmeNamespace *ns) diff --git a/include/block/nvme.h b/include/block/nvme.h index b30744068d46..a0519814ecec 100644 --- a/include/block/nvme.h +++ b/include/block/nvme.h @@ -688,7 +688,13 @@ enum NvmeIdCtrlOncs { typedef struct NvmeFeatureVal { uint32_t arbitration; uint32_t power_mgmt; - uint32_t temp_thresh; + union { + struct { + uint16_t temp_thresh_hi; + uint16_t temp_thresh_low; + }; + uint32_t temp_thresh; + }; uint32_t err_rec; uint32_t volatile_wc; uint32_t num_queues;