On 10/7/21 18:24, Lukasz Maniak wrote:
> From: Łukasz Gieryk <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 */
>
