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

[Philippe Mathieu-Daudé]

On 10/7/21 18:24, 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.
> 
> The SR-IOV feature 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->max_ioqpairs - (new) value derived from n->params.* in realize();
>                      constant through device’s lifetime.
> 
>  - 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>
> ---
>  hw/nvme/ctrl.c | 62 +++++++++++++++++++++++++++++++++-----------------
>  hw/nvme/nvme.h |  4 ++++
>  2 files changed, 45 insertions(+), 21 deletions(-)

> @@ -6322,11 +6334,17 @@ static void nvme_init_state(NvmeCtrl *n)
>      NvmeSecCtrlEntry *sctrl;
>      int i;
>  
> +    n->max_ioqpairs = n->params.max_ioqpairs;
> +    n->conf_ioqpairs = n->max_ioqpairs;
> +
> +    n->max_msix_qsize = n->params.msix_qsize;
> +    n->conf_msix_qsize = n->max_msix_qsize;

>From an developer perspective, the API becomes confusing.
Most fields from NvmeParams are exposed via QMP, such max_ioqpairs.

I'm not sure we need 2 distinct fields. Maybe simply reorganize
to not reset these values in the DeviceReset handler?

Also, with this series we should consider implementing the migration
state (nvme_vmstate).

> diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
> index 9fbb0a70b5..65383e495c 100644
> --- a/hw/nvme/nvme.h
> +++ b/hw/nvme/nvme.h
> @@ -420,6 +420,10 @@ typedef struct NvmeCtrl {
>      uint64_t    starttime_ms;
>      uint16_t    temperature;
>      uint8_t     smart_critical_warning;
> +    uint32_t    max_msix_qsize;                 /* Derived from 
> params.msix.qsize */
> +    uint32_t    conf_msix_qsize;                /* Configured limit */
> +    uint32_t    max_ioqpairs;                   /* Derived from 
> params.max_ioqpairs */
> +    uint32_t    conf_ioqpairs;                  /* Configured limit */
>