Re: [Qemu-devel] [PATCH] nvme: do not advertise support for unsupported arbitration mechanism

[Klaus Birkelund <klaus@birkelund.eu>]

On Fri, Jun 14, 2019 at 10:39:27PM +0200, Max Reitz wrote:
> On 06.06.19 11:25, Klaus Birkelund Jensen wrote:
> > The device mistakenly reports that the Weighted Round Robin with Urgent
> > Priority Class arbitration mechanism is supported.
> > 
> > It is not.
> 
> I believe you based on the fact that there is no “weight” or “priority”
> anywhere in nvme.c, and that it does not evaluate the Arbitration
> Mechanism Selected field.
> 

Not sure if you want me to change the commit message? Feel free to
change it if you want to ;)

> > Signed-off-by: Klaus Birkelund Jensen <klaus.jensen@cnexlabs.com>
> > ---
> >  hw/block/nvme.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 30e50f7a3853..415b4641d6b4 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -1383,7 +1383,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
> >      n->bar.cap = 0;
> >      NVME_CAP_SET_MQES(n->bar.cap, 0x7ff);
> >      NVME_CAP_SET_CQR(n->bar.cap, 1);
> > -    NVME_CAP_SET_AMS(n->bar.cap, 1);
> 
> I suppose the better way would be to pass 0, so it is more explicit, I
> think.
> 
> (Just removing it looks like it may have just been forgotten.)
> 

Not explicitly setting it to zero aligns with how the other fields in
CAP are also left out if kept at zero. If we explicitly set it to zero I
think we should also set all the other fields that way (DSTRD, NSSRS,
etc.).


Klaus