hw/block/nvme.c | 1 - 1 file changed, 1 deletion(-)
The device mistakenly reports that the Weighted Round Robin with Urgent
Priority Class arbitration mechanism is supported.
It is not.
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);
NVME_CAP_SET_TO(n->bar.cap, 0xf);
NVME_CAP_SET_CSS(n->bar.cap, 1);
NVME_CAP_SET_MPSMAX(n->bar.cap, 4);
--
2.21.0
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. > 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.) Max > NVME_CAP_SET_TO(n->bar.cap, 0xf); > NVME_CAP_SET_CSS(n->bar.cap, 1); > NVME_CAP_SET_MPSMAX(n->bar.cap, 4); >
On Fri, 2019-06-14 at 22:39 +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. > > > 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.) > > Max > > > NVME_CAP_SET_TO(n->bar.cap, 0xf); > > NVME_CAP_SET_CSS(n->bar.cap, 1); > > NVME_CAP_SET_MPSMAX(n->bar.cap, 4); > > > > Yea. no way that this driver supports WRRU and I haven't noticed it. Just checked again to be sure. To be honest, after some quick look, that driver doesn' even really support the regular Round-Robin as it uses a timer to process a specific queue, kicked up by a doorbell write :-) Acked-by: Maxim Levitsky <mlevitsk@redhat.com> Best regards, Maxim Levitsky
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
On 17.06.19 08:54, Klaus Birkelund wrote: > 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 ;) Oh, no, no. It’s just that I have no idea how the NVMe interface works (well, I did download the spec then, but...), so this was basically just my excuse for “I can review this!!”. >>> 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.). OK then. Thanks, applied to my block branch: https://git.xanclic.moe/XanClic/qemu/commits/branch/block Max
© 2016 - 2024 Red Hat, Inc.