Re: [PATCH v5 09/15] hw/nvme: Make max_ioqpairs and msix_qsize configurable in runtime

[Klaus Jensen]

On Feb 17 18:44, Lukasz Maniak wrote:
> From: Łukasz Gieryk <lukasz.gieryk@linux.intel.com>
> 
> The NVMe device defines two properties: max_ioqpairs, msix_qsize. Having
> them as constants is problematic for SR-IOV support.
> 
> SR-IOV introduces virtual resources (queues, interrupts) that can be
> assigned to PF and its dependent VFs. Each device, following a reset,
> should work with the configured number of queues. A single constant is
> no longer sufficient to hold the whole state.
> 
> This patch tries to solve the problem by introducing additional
> variables in NvmeCtrl’s state. The variables for, e.g., managing queues
> are therefore organized as:
>  - n->params.max_ioqpairs – no changes, constant set by the user
>  - n->(mutable_state) – (not a part of this patch) user-configurable,
>                         specifies number of queues available _after_
>                         reset
>  - n->conf_ioqpairs - (new) used in all the places instead of the ‘old’
>                       n->params.max_ioqpairs; initialized in realize()
>                       and updated during reset() to reflect user’s
>                       changes to the mutable state
> 
> Since the number of available i/o queues and interrupts can change in
> runtime, buffers for sq/cqs and the MSIX-related structures are
> allocated big enough to handle the limits, to completely avoid the
> complicated reallocation. A helper function (nvme_update_msixcap_ts)
> updates the corresponding capability register, to signal configuration
> changes.
> 
> Signed-off-by: Łukasz Gieryk <lukasz.gieryk@linux.intel.com>

LGTM.

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>

> ---
>  hw/nvme/ctrl.c | 52 ++++++++++++++++++++++++++++++++++----------------
>  hw/nvme/nvme.h |  2 ++
>  2 files changed, 38 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 7c1dd80f21d..f1b4026e4f8 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -445,12 +445,12 @@ static bool nvme_nsid_valid(NvmeCtrl *n, uint32_t nsid)
>  
>  static int nvme_check_sqid(NvmeCtrl *n, uint16_t sqid)
>  {
> -    return sqid < n->params.max_ioqpairs + 1 && n->sq[sqid] != NULL ? 0 : -1;
> +    return sqid < n->conf_ioqpairs + 1 && n->sq[sqid] != NULL ? 0 : -1;
>  }
>  
>  static int nvme_check_cqid(NvmeCtrl *n, uint16_t cqid)
>  {
> -    return cqid < n->params.max_ioqpairs + 1 && n->cq[cqid] != NULL ? 0 : -1;
> +    return cqid < n->conf_ioqpairs + 1 && n->cq[cqid] != NULL ? 0 : -1;
>  }
>  
>  static void nvme_inc_cq_tail(NvmeCQueue *cq)
> @@ -4188,8 +4188,7 @@ static uint16_t nvme_create_sq(NvmeCtrl *n, NvmeRequest 
> *req)
>          trace_pci_nvme_err_invalid_create_sq_cqid(cqid);
>          return NVME_INVALID_CQID | NVME_DNR;
>      }
> -    if (unlikely(!sqid || sqid > n->params.max_ioqpairs ||
> -        n->sq[sqid] != NULL)) {
> +    if (unlikely(!sqid || sqid > n->conf_ioqpairs || n->sq[sqid] != NULL)) {
>          trace_pci_nvme_err_invalid_create_sq_sqid(sqid);
>          return NVME_INVALID_QID | NVME_DNR;
>      }
> @@ -4541,8 +4540,7 @@ static uint16_t nvme_create_cq(NvmeCtrl *n, NvmeRequest 
> *req)
>      trace_pci_nvme_create_cq(prp1, cqid, vector, qsize, qflags,
>                               NVME_CQ_FLAGS_IEN(qflags) != 0);
>  
> -    if (unlikely(!cqid || cqid > n->params.max_ioqpairs ||
> -        n->cq[cqid] != NULL)) {
> +    if (unlikely(!cqid || cqid > n->conf_ioqpairs || n->cq[cqid] != NULL)) {
>          trace_pci_nvme_err_invalid_create_cq_cqid(cqid);
>          return NVME_INVALID_QID | NVME_DNR;
>      }
> @@ -4558,7 +4556,7 @@ static uint16_t nvme_create_cq(NvmeCtrl *n, NvmeRequest 
> *req)
>          trace_pci_nvme_err_invalid_create_cq_vector(vector);
>          return NVME_INVALID_IRQ_VECTOR | NVME_DNR;
>      }
> -    if (unlikely(vector >= n->params.msix_qsize)) {
> +    if (unlikely(vector >= n->conf_msix_qsize)) {
>          trace_pci_nvme_err_invalid_create_cq_vector(vector);
>          return NVME_INVALID_IRQ_VECTOR | NVME_DNR;
>      }
> @@ -5155,13 +5153,12 @@ defaults:
>  
>          break;
>      case NVME_NUMBER_OF_QUEUES:
> -        result = (n->params.max_ioqpairs - 1) |
> -            ((n->params.max_ioqpairs - 1) << 16);
> +        result = (n->conf_ioqpairs - 1) | ((n->conf_ioqpairs - 1) << 16);
>          trace_pci_nvme_getfeat_numq(result);
>          break;
>      case NVME_INTERRUPT_VECTOR_CONF:
>          iv = dw11 & 0xffff;
> -        if (iv >= n->params.max_ioqpairs + 1) {
> +        if (iv >= n->conf_ioqpairs + 1) {
>              return NVME_INVALID_FIELD | NVME_DNR;
>          }
>  
> @@ -5316,10 +5313,10 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, 
> NvmeRequest *req)
>  
>          trace_pci_nvme_setfeat_numq((dw11 & 0xffff) + 1,
>                                      ((dw11 >> 16) & 0xffff) + 1,
> -                                    n->params.max_ioqpairs,
> -                                    n->params.max_ioqpairs);
> -        req->cqe.result = cpu_to_le32((n->params.max_ioqpairs - 1) |
> -                                      ((n->params.max_ioqpairs - 1) << 16));
> +                                    n->conf_ioqpairs,
> +                                    n->conf_ioqpairs);
> +        req->cqe.result = cpu_to_le32((n->conf_ioqpairs - 1) |
> +                                      ((n->conf_ioqpairs - 1) << 16));
>          break;
>      case NVME_ASYNCHRONOUS_EVENT_CONF:
>          n->features.async_config = dw11;
> @@ -5757,8 +5754,24 @@ static void nvme_process_sq(void *opaque)
>      }
>  }
>  
> +static void nvme_update_msixcap_ts(PCIDevice *pci_dev, uint32_t table_size)
> +{
> +    uint8_t *config;
> +
> +    if (!msix_present(pci_dev)) {
> +        return;
> +    }
> +
> +    assert(table_size > 0 && table_size <= pci_dev->msix_entries_nr);
> +
> +    config = pci_dev->config + pci_dev->msix_cap;
> +    pci_set_word_by_mask(config + PCI_MSIX_FLAGS, PCI_MSIX_FLAGS_QSIZE,
> +                         table_size - 1);
> +}
> +
>  static void nvme_ctrl_reset(NvmeCtrl *n, NvmeResetType rst)
>  {
> +    PCIDevice *pci_dev = &n->parent_obj;
>      NvmeNamespace *ns;
>      int i;
>  
> @@ -5788,15 +5801,17 @@ static void nvme_ctrl_reset(NvmeCtrl *n, 
> NvmeResetType rst)
>          g_free(event);
>      }
>  
> -    if (!pci_is_vf(&n->parent_obj) && n->params.sriov_max_vfs) {
> +    if (!pci_is_vf(pci_dev) && n->params.sriov_max_vfs) {
>          if (rst != NVME_RESET_CONTROLLER) {
> -            pcie_sriov_pf_disable_vfs(&n->parent_obj);
> +            pcie_sriov_pf_disable_vfs(pci_dev);
>          }
>      }
>  
>      n->aer_queued = 0;
>      n->outstanding_aers = 0;
>      n->qs_created = false;
> +
> +    nvme_update_msixcap_ts(pci_dev, n->conf_msix_qsize);
>  }
>  
>  static void nvme_ctrl_shutdown(NvmeCtrl *n)
> @@ -6507,6 +6522,9 @@ static void nvme_init_state(NvmeCtrl *n)
>      NvmeSecCtrlEntry *sctrl;
>      int i;
>  
> +    n->conf_ioqpairs = n->params.max_ioqpairs;
> +    n->conf_msix_qsize = n->params.msix_qsize;
> +
>      /* add one to max_ioqpairs to account for the admin queue pair */
>      n->reg_size = pow2ceil(sizeof(NvmeBar) +
>                             2 * (n->params.max_ioqpairs + 1) * NVME_DB_SIZE);
> @@ -6668,6 +6686,8 @@ static int nvme_init_pci(NvmeCtrl *n, PCIDevice 
> *pci_dev, Error **errp)
>          }
>      }
>  
> +    nvme_update_msixcap_ts(pci_dev, n->conf_msix_qsize);
> +
>      if (n->params.cmb_size_mb) {
>          nvme_init_cmb(n, pci_dev);
>      }
> diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
> index 5ba07b62dff..314a2894759 100644
> --- a/hw/nvme/nvme.h
> +++ b/hw/nvme/nvme.h
> @@ -434,6 +434,8 @@ typedef struct NvmeCtrl {
>      uint64_t    starttime_ms;
>      uint16_t    temperature;
>      uint8_t     smart_critical_warning;
> +    uint32_t    conf_msix_qsize;
> +    uint32_t    conf_ioqpairs;
>  
>      struct {
>          MemoryRegion mem;
> -- 
> 2.25.1
> 

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

signature.asc
Description: PGP signature