Re: [PATCH v7 12/48] nvme: add temperature threshold feature

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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;
>