Re: [PATCH v2 13/15] hw/nvme: Add support for the Virtualization Management command

[Łukasz Gieryk]

On Wed, Nov 24, 2021 at 09:06:23AM +0100, Klaus Jensen wrote:
> On Nov 16 16:34, Łukasz Gieryk wrote:
> > With the new Virtualization Management command one can:
> >  - assign flexible resources (queues, interrupts) to primary and
> >    secondary controllers,
> >  - toggle the online/offline state of given controller.
> > 
> > Signed-off-by: Łukasz Gieryk <lukasz.gieryk@linux.intel.com>
> > ---
> >  hw/nvme/ctrl.c       | 204 +++++++++++++++++++++++++++++++++++++++++++
> >  hw/nvme/nvme.h       |  16 ++++
> >  hw/nvme/trace-events |   3 +
> >  include/block/nvme.h |  17 ++++
> >  4 files changed, 240 insertions(+)
> > 
> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > index f589ffde59..9d0432a2e5 100644
> > --- a/hw/nvme/ctrl.c
> > +++ b/hw/nvme/ctrl.c
> 
> [... snip]
> 
> > +static uint16_t nvme_assign_virt_res_to_sec(NvmeCtrl *n, NvmeRequest *req,
> > +                                            uint16_t cntlid, uint8_t rt, 
> > int nr)
> > +{
> > +    int limit = rt ? n->params.sriov_max_vi_per_vf :
> > +                     n->params.sriov_max_vq_per_vf;
> 
> If these parameters are left at the default, limit is 0 and the check
> below fails.
> 
> [... snip]
> 
> > +    if (nr > limit) {
> > +        return NVME_INVALID_NUM_RESOURCES | NVME_DNR;
> > +    }

Indeed, my bad.

Al the tests I have at hand set the parameters explicitly, so the
problem has slipped through. I’ve manually tested only the negative
scenarios, where Qemu refuses to start.