Re: [PATCH V3 7/8] hw/block/nvme: support changed namespace asyncrohous event

[Klaus Jensen]

On Mar  1 01:10, Minwoo Im wrote:
> If namespace inventory is changed due to some reasons (e.g., namespace
> attachment/detachment), controller can send out event notifier to the
> host to manage namespaces.
> 
> This patch sends out the AEN to the host after either attach or detach
> namespaces from controllers.  To support clear of the event from the
> controller, this patch also implemented Get Log Page command for Changed
> Namespace List log type.  To return namespace id list through the
> command, when namespace inventory is updated, id is added to the
> per-controller list (changed_ns_list).
> 
> To indicate the support of this async event, this patch set
> OAES(Optional Asynchronous Events Supported) in Identify Controller data
> structure.
> 
> Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
> ---
>  hw/block/nvme.c      | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  hw/block/nvme.h      |  7 +++++++
>  include/block/nvme.h |  7 +++++++
>  3 files changed, 58 insertions(+)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 68c2e63d9412..fc06f806e58e 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -2980,6 +2980,32 @@ static uint16_t nvme_error_info(NvmeCtrl *n, uint8_t 
> rae, uint32_t buf_len,
>                      DMA_DIRECTION_FROM_DEVICE, req);
>  }
>  
> +static uint16_t nvme_changed_nslist(NvmeCtrl *n, uint8_t rae, uint32_t 
> buf_len,
> +                                    uint64_t off, NvmeRequest *req)
> +{
> +    uint32_t nslist[1024];
> +    uint32_t trans_len;
> +    NvmeChangedNs *ns, *next;
> +    int i = 0;
> +
> +    memset(nslist, 0x0, sizeof(nslist));
> +    trans_len = MIN(sizeof(nslist) - off, buf_len);
> +
> +    QTAILQ_FOREACH_SAFE(ns, &n->changed_ns_list, entry, next) {
> +        nslist[i++] = ns->nsid;
> +
> +        QTAILQ_REMOVE(&n->changed_ns_list, ns, entry);
> +        g_free(ns);
> +    }
> +
> +    if (!rae) {
> +        nvme_clear_events(n, NVME_AER_TYPE_NOTICE);
> +    }
> +
> +    return nvme_dma(n, ((uint8_t *)nslist) + off, trans_len,
> +                    DMA_DIRECTION_FROM_DEVICE, req);
> +}
> +
>  static uint16_t nvme_cmd_effects(NvmeCtrl *n, uint8_t csi, uint32_t buf_len,
>                                   uint64_t off, NvmeRequest *req)
>  {
> @@ -3064,6 +3090,8 @@ static uint16_t nvme_get_log(NvmeCtrl *n, NvmeRequest 
> *req)
>          return nvme_smart_info(n, rae, len, off, req);
>      case NVME_LOG_FW_SLOT_INFO:
>          return nvme_fw_log_info(n, len, off, req);
> +    case NVME_LOG_CHANGED_NSLIST:
> +        return nvme_changed_nslist(n, rae, len, off, req);
>      case NVME_LOG_CMD_EFFECTS:
>          return nvme_cmd_effects(n, csi, len, off, req);
>      default:
> @@ -3882,6 +3910,7 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n, 
> NvmeRequest *req)
>      uint16_t *ids = &list[1];
>      uint16_t ret;
>      int i;
> +    NvmeChangedNs *changed_nsid;
>  
>      trace_pci_nvme_ns_attachment(nvme_cid(req), dw10 & 0xf);
>  
> @@ -3920,6 +3949,18 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n, 
> NvmeRequest *req)
>  
>              nvme_ns_detach(ctrl, ns);
>          }
> +
> +        /*
> +         * Add namespace id to the changed namespace id list for event 
> clearing
> +         * via Get Log Page command.
> +         */
> +        changed_nsid = g_new(NvmeChangedNs, 1);
> +        changed_nsid->nsid = nsid;
> +        QTAILQ_INSERT_TAIL(&ctrl->changed_ns_list, changed_nsid, entry);
> +
> +        nvme_enqueue_event(ctrl, NVME_AER_TYPE_NOTICE,
> +                           NVME_AER_INFO_NOTICE_NS_ATTR_CHANGED,
> +                           NVME_LOG_CHANGED_NSLIST);
>      }

If one just keeps attaching/detaching we end up with more than 1024
entries here and go out of bounds in nvme_changed_nslist.

How about having the QTAILQ_ENTRY directly on the NvmeNamespace struct
and use QTAILQ_IN_USE to check if the namespace is already in the list?

>  
>      return NVME_SUCCESS;
> @@ -4714,6 +4755,7 @@ static void nvme_init_state(NvmeCtrl *n)
>      n->features.temp_thresh_hi = NVME_TEMPERATURE_WARNING;
>      n->starttime_ms = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
>      n->aer_reqs = g_new0(NvmeRequest *, n->params.aerl + 1);
> +    QTAILQ_INIT(&n->changed_ns_list);
>  }
>  
>  static int nvme_attach_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error 
> **errp)
> @@ -4910,6 +4952,8 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice 
> *pci_dev)
>  
>      id->cntlid = cpu_to_le16(n->cntlid);
>  
> +    id->oaes = cpu_to_le32(NVME_OAES_NS_ATTR);
> +
>      id->rab = 6;
>  
>      if (n->params.use_intel_id) {
> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> index 74a00ab21a55..d5eaea003ea5 100644
> --- a/hw/block/nvme.h
> +++ b/hw/block/nvme.h
> @@ -132,6 +132,11 @@ typedef struct NvmeFeatureVal {
>      uint32_t    async_config;
>  } NvmeFeatureVal;
>  
> +typedef struct NvmeChangedNs {
> +    uint32_t nsid;
> +    QTAILQ_ENTRY(NvmeChangedNs) entry;
> +} NvmeChangedNs;
> +
>  typedef struct NvmeCtrl {
>      PCIDevice    parent_obj;
>      MemoryRegion bar0;
> @@ -177,6 +182,8 @@ typedef struct NvmeCtrl {
>      QTAILQ_HEAD(, NvmeAsyncEvent) aer_queue;
>      int         aer_queued;
>  
> +    QTAILQ_HEAD(, NvmeChangedNs) changed_ns_list;   /* Changed NS list log */
> +
>      NvmeSubsystem   *subsys;
>  
>      NvmeNamespace   namespace;
> diff --git a/include/block/nvme.h b/include/block/nvme.h
> index 339784d9c23a..eb0b31e949c2 100644
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -760,6 +760,7 @@ typedef struct QEMU_PACKED NvmeCopySourceRange {
>  enum NvmeAsyncEventRequest {
>      NVME_AER_TYPE_ERROR                     = 0,
>      NVME_AER_TYPE_SMART                     = 1,
> +    NVME_AER_TYPE_NOTICE                    = 2,
>      NVME_AER_TYPE_IO_SPECIFIC               = 6,
>      NVME_AER_TYPE_VENDOR_SPECIFIC           = 7,
>      NVME_AER_INFO_ERR_INVALID_DB_REGISTER   = 0,
> @@ -771,6 +772,7 @@ enum NvmeAsyncEventRequest {
>      NVME_AER_INFO_SMART_RELIABILITY         = 0,
>      NVME_AER_INFO_SMART_TEMP_THRESH         = 1,
>      NVME_AER_INFO_SMART_SPARE_THRESH        = 2,
> +    NVME_AER_INFO_NOTICE_NS_ATTR_CHANGED    = 0,
>  };
>  
>  typedef struct QEMU_PACKED NvmeAerResult {
> @@ -940,6 +942,7 @@ enum NvmeLogIdentifier {
>      NVME_LOG_ERROR_INFO     = 0x01,
>      NVME_LOG_SMART_INFO     = 0x02,
>      NVME_LOG_FW_SLOT_INFO   = 0x03,
> +    NVME_LOG_CHANGED_NSLIST = 0x04,
>      NVME_LOG_CMD_EFFECTS    = 0x05,
>  };
>  
> @@ -1046,6 +1049,10 @@ typedef struct NvmeIdCtrlZoned {
>      uint8_t     rsvd1[4095];
>  } NvmeIdCtrlZoned;
>  
> +enum NvmeIdCtrlOaes {
> +    NVME_OAES_NS_ATTR   = 1 << 8,
> +};
> +
>  enum NvmeIdCtrlOacs {
>      NVME_OACS_SECURITY  = 1 << 0,
>      NVME_OACS_FORMAT    = 1 << 1,
> -- 
> 2.25.1
> 

-- 
One of us - No more doubt, silence or taboo about mental illness.

signature.asc
Description: PGP signature