[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v6 14/42] nvme: add missing mandatory features
From: |
Klaus Birkelund Jensen |
Subject: |
Re: [PATCH v6 14/42] nvme: add missing mandatory features |
Date: |
Tue, 31 Mar 2020 07:41:45 +0200 |
On Mar 25 12:41, Maxim Levitsky wrote:
> On Mon, 2020-03-16 at 07:29 -0700, Klaus Jensen wrote:
> > From: Klaus Jensen <address@hidden>
> >
> > Add support for returning a resonable response to Get/Set Features of
> > mandatory features.
> >
> > Signed-off-by: Klaus Jensen <address@hidden>
> > Acked-by: Keith Busch <address@hidden>
> > ---
> > hw/block/nvme.c | 60 ++++++++++++++++++++++++++++++++++++++++++-
> > hw/block/trace-events | 2 ++
> > include/block/nvme.h | 6 ++++-
> > 3 files changed, 66 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index ff8975cd6667..eb9c722df968 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -1058,6 +1069,19 @@ static uint16_t nvme_get_feature(NvmeCtrl *n,
> > NvmeCmd *cmd, NvmeRequest *req)
> > break;
> > case NVME_TIMESTAMP:
> > return nvme_get_feature_timestamp(n, cmd);
> > + case NVME_INTERRUPT_COALESCING:
> > + result = cpu_to_le32(n->features.int_coalescing);
> > + break;
> > + case NVME_INTERRUPT_VECTOR_CONF:
> > + if ((dw11 & 0xffff) > n->params.max_ioqpairs + 1) {
> > + return NVME_INVALID_FIELD | NVME_DNR;
> > + }
> I still think that this should be >= since the interrupt vector is not zero
> based.
> So if we have for example 3 IO queues, then we have 4 queues in total
> which translates to irq numbers 0..3.
>
Yes you are right. The device will support max_ioqpairs + 1 IVs, so
trying to access that would actually go 1 beyond the array.
Fixed.
> BTW the user of the device doesn't have to have 1:1 mapping between qid and
> msi interrupt index,
> in fact when MSI is not used, all the queues will map to the same vector,
> which will be interrupt 0
> from point of view of the device IMHO.
> So it kind of makes sense IMHO to have num_irqs or something, even if it
> technically equals to number of queues.
>
Yeah, but the device will still *support* the N IVs, so they can still
be configured even though they will not be used. So I don't think we
need to introduce an additional parameter?
> > @@ -1120,6 +1146,10 @@ static uint16_t nvme_set_feature(NvmeCtrl *n,
> > NvmeCmd *cmd, NvmeRequest *req)
> >
> > break;
> > case NVME_VOLATILE_WRITE_CACHE:
> > + if (blk_enable_write_cache(n->conf.blk)) {
> > + blk_flush(n->conf.blk);
> > + }
>
> (not your fault) but the blk_enable_write_cache function name is highly
> misleading,
> since it doesn't enable anything but just gets the flag if the write cache is
> enabled.
> It really should be called blk_get_enable_write_cache.
>
Agreed :)
> > @@ -1804,6 +1860,7 @@ static void nvme_init_ctrl(NvmeCtrl *n)
> > id->cqes = (0x4 << 4) | 0x4;
> > id->nn = cpu_to_le32(n->num_namespaces);
> > id->oncs = cpu_to_le16(NVME_ONCS_WRITE_ZEROS | NVME_ONCS_TIMESTAMP);
> > +
> Unrelated whitespace change
Fixed.
>
> Best regards,
> Maxim Levitsky
>
>
>
>
- Re: [PATCH v6 09/42] nvme: add max_ioqpairs device parameter, (continued)
[PATCH v6 11/42] nvme: add temperature threshold feature, Klaus Jensen, 2020/03/16
[PATCH v6 16/42] nvme: make sure ncqr and nsqr is valid, Klaus Jensen, 2020/03/16
[PATCH v6 14/42] nvme: add missing mandatory features, Klaus Jensen, 2020/03/16
[PATCH v6 17/42] nvme: add log specific field to trace events, Klaus Jensen, 2020/03/16
[PATCH v6 15/42] nvme: additional tracing, Klaus Jensen, 2020/03/16
[PATCH v6 12/42] nvme: add support for the get log page command, Klaus Jensen, 2020/03/16