Re: [PATCH 04/17] hw/block/nvme: add temperature threshold feature
On Jul 3 00:44, Dmitry Fomichev wrote:
> On Mon, 2020-06-29 at 20:26 +0200, Klaus Jensen wrote:
> > From: Klaus Jensen
> >
> > It might seem weird to implement this feature for an emulated device,
> > 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.
> >
> > Signed-off-by: Klaus Jensen
> > Acked-by: Keith Busch
> > Reviewed-by: Maxim Levitsky
> > ---
> > 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 b7037a7d3504..5ca50646369e 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -59,6 +59,9 @@
> > #define NVME_DB_SIZE 4
> > #define NVME_CMB_BIR 2
> > #define NVME_PMR_BIR 2
> > +#define NVME_TEMPERATURE 0x143
> > +#define NVME_TEMPERATURE_WARNING 0x157
> > +#define NVME_TEMPERATURE_CRITICAL 0x175
> >
> > #define NVME_GUEST_ERR(trace, fmt, ...) \
> > do { \
> > @@ -827,9 +830,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_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
> > @@ -874,6 +899,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;
> > @@ -1454,6 +1496,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_blk(NvmeCtrl *n, Error **errp)
> > @@ -1611,6 +1654,11 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice
> > *pci_dev)
> > 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 1bf5c80ed843..3acde10e1d2a 100644
> > --- a/hw/block/nvme.h
> > +++ b/hw/block/nvme.h
> > @@ -107,6 +107,7 @@ typedef struct NvmeCtrl {
> > NvmeSQueue admin_sq;
> > NvmeCQueue admin_cq;
> > NvmeIdCtrl id_ctrl;
> > +NvmeFeatureVal features;
> > } NvmeCtrl;
> >
> > /* calculate the number of LBAs that the namespace can accomodate */
> > diff --git a/include/block/nvme.h b/include/block/nvme.h
> > index 6d1fa6ff2228..bb651d0cbf5a 100644
> > --- a/include/block/nvme.h
> > +++ b/include/block/nvme.h
> > @@ -860,7 +860,13 @@ enum NvmeIdCtrlOncs {
> > typedef struct NvmeFeatureVal {
> > uint32_tarbitration;
> > uint32_tpower_mgmt;
> > -uint32_ttemp_thresh;
> > +union {
> > +struct {
> > +
Re: [PATCH 04/17] hw/block/nvme: add temperature threshold feature
On Mon, 2020-06-29 at 20:26 +0200, Klaus Jensen wrote:
> From: Klaus Jensen
>
> It might seem weird to implement this feature for an emulated device,
> 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.
>
> Signed-off-by: Klaus Jensen
> Acked-by: Keith Busch
> Reviewed-by: Maxim Levitsky
> ---
> 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 b7037a7d3504..5ca50646369e 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -59,6 +59,9 @@
> #define NVME_DB_SIZE 4
> #define NVME_CMB_BIR 2
> #define NVME_PMR_BIR 2
> +#define NVME_TEMPERATURE 0x143
> +#define NVME_TEMPERATURE_WARNING 0x157
> +#define NVME_TEMPERATURE_CRITICAL 0x175
>
> #define NVME_GUEST_ERR(trace, fmt, ...) \
> do { \
> @@ -827,9 +830,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_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
> @@ -874,6 +899,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;
> @@ -1454,6 +1496,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_blk(NvmeCtrl *n, Error **errp)
> @@ -1611,6 +1654,11 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice
> *pci_dev)
> 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 1bf5c80ed843..3acde10e1d2a 100644
> --- a/hw/block/nvme.h
> +++ b/hw/block/nvme.h
> @@ -107,6 +107,7 @@ typedef struct NvmeCtrl {
> NvmeSQueue admin_sq;
> NvmeCQueue admin_cq;
> NvmeIdCtrl id_ctrl;
> +NvmeFeatureVal features;
> } NvmeCtrl;
>
> /* calculate the number of LBAs that the namespace can accomodate */
> diff --git a/include/block/nvme.h b/include/block/nvme.h
> index 6d1fa6ff2228..bb651d0cbf5a 100644
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -860,7 +860,13 @@ enum NvmeIdCtrlOncs {
> typedef struct NvmeFeatureVal {
> uint32_tarbitration;
> uint32_tpower_mgmt;
> -uint32_ttemp_thresh;
> +union {
> +struct {
> +uint16_t temp_thresh_hi;
> +uint16_t temp_thresh_low;
> +};
> +uint32_t temp_thresh;
temp_thresh seems unused, is this union really needed?
> +};
> uint32_terr_rec;
> uint32_tvolatile_wc;
> uint32_tnum_queues;
