| Message ID | 20200609094508.32412-1-its@irrelevant.dk (mailing list archive) |
|---|---|
| Headers | show |
| Series | hw/block/nvme: fix assert on invalid irq vector | expand |
On 6/9/20 11:45 AM, Klaus Jensen wrote: > From: Klaus Jensen <k.jensen@samsung.com> > > I goofed up with commit c09794fe40e3 ("hw/block/nvme: allow use of any > valid msix vector"). Kevin, since your queue isn't merged, can you directly squash the fix? > > This fixes the goof by adding a new msix_qsize parameter. As a nice > side-effect this allows a device with less interrupt vectors available > than supported queues. Also, improve the error handling in > nvme_init_pci(). > > Kevin, please consider picking this up for the block branch when > reviewed. > > Cc: qemu-devel@nongnu.org > Cc: Keith Busch <kbusch@kernel.org> > Cc: Max Reitz <mreitz@redhat.com> > Cc: Kevin Wolf <kwolf@redhat.com> > Cc: Klaus Jensen <its@irrelevant.dk> > Cc: Javier Gonzalez <javier.gonz@samsung.com> > Cc: Maxim Levitsky <mlevitsk@redhat.com> > Cc: Philippe Mathieu-Daudé <philmd@redhat.com> > > Klaus Jensen (2): > hw/block/nvme: add msix_qsize parameter > hw/block/nvme: verify msix_init_exclusive_bar() return value > > hw/block/nvme.c | 28 ++++++++++++++++++++++------ > hw/block/nvme.h | 1 + > 2 files changed, 23 insertions(+), 6 deletions(-) >
On Jun 9 13:17, Philippe Mathieu-Daudé wrote: > On 6/9/20 11:45 AM, Klaus Jensen wrote: > > From: Klaus Jensen <k.jensen@samsung.com> > > > > I goofed up with commit c09794fe40e3 ("hw/block/nvme: allow use of any > > valid msix vector"). > > Kevin, since your queue isn't merged, can you directly squash the fix? The commit (c09794fe40e3) can just be dropped without conflicts, but it leaves a use of n->params.num_queues in nvme_create_cq() which commit cde74bfd4b87 ("hw/block/nvme: add max_ioqpairs device parameter") must fix.
Am 09.06.2020 um 13:46 hat Klaus Jensen geschrieben: > On Jun 9 13:17, Philippe Mathieu-Daudé wrote: > > On 6/9/20 11:45 AM, Klaus Jensen wrote: > > > From: Klaus Jensen <k.jensen@samsung.com> > > > > > > I goofed up with commit c09794fe40e3 ("hw/block/nvme: allow use of any > > > valid msix vector"). > > > > Kevin, since your queue isn't merged, can you directly squash the fix? > > The commit (c09794fe40e3) can just be dropped without conflicts, but it > leaves a use of n->params.num_queues in nvme_create_cq() which commit > cde74bfd4b87 ("hw/block/nvme: add max_ioqpairs device parameter") must > fix. Hm, so it seems this isn't easy to squash in without conflicts (and I would have to rewrite the whole commit message), so I think it's better to just apply the series on top. One problem with the commit message is that it references commit IDs which aren't stable yet. Maybe it's best if I apply these patches, manually fix up the commit ID references and then immediately do a pull request so that they become stable. It would be good to have at least one review, though. Kevin
On 6/9/20 4:14 PM, Kevin Wolf wrote: > Am 09.06.2020 um 13:46 hat Klaus Jensen geschrieben: >> On Jun 9 13:17, Philippe Mathieu-Daudé wrote: >>> On 6/9/20 11:45 AM, Klaus Jensen wrote: >>>> From: Klaus Jensen <k.jensen@samsung.com> >>>> >>>> I goofed up with commit c09794fe40e3 ("hw/block/nvme: allow use of any >>>> valid msix vector"). >>> >>> Kevin, since your queue isn't merged, can you directly squash the fix? >> >> The commit (c09794fe40e3) can just be dropped without conflicts, but it >> leaves a use of n->params.num_queues in nvme_create_cq() which commit >> cde74bfd4b87 ("hw/block/nvme: add max_ioqpairs device parameter") must >> fix. > > Hm, so it seems this isn't easy to squash in without conflicts (and I > would have to rewrite the whole commit message), so I think it's better > to just apply the series on top. > > One problem with the commit message is that it references commit IDs > which aren't stable yet. Maybe it's best if I apply these patches, > manually fix up the commit ID references and then immediately do a pull > request so that they become stable. This is the friendlier way. Less friendly way is to drop Klaus's patches and ask him to respin. While this is a valid outcome, if we can avoid it it will save all of us review time. > > It would be good to have at least one review, though. Maxim catched this issue, I'd feel safer if he acks your pre-merge queue. > > Kevin >
Am 09.06.2020 um 16:18 hat Philippe Mathieu-Daudé geschrieben: > On 6/9/20 4:14 PM, Kevin Wolf wrote: > > Am 09.06.2020 um 13:46 hat Klaus Jensen geschrieben: > >> On Jun 9 13:17, Philippe Mathieu-Daudé wrote: > >>> On 6/9/20 11:45 AM, Klaus Jensen wrote: > >>>> From: Klaus Jensen <k.jensen@samsung.com> > >>>> > >>>> I goofed up with commit c09794fe40e3 ("hw/block/nvme: allow use of any > >>>> valid msix vector"). > >>> > >>> Kevin, since your queue isn't merged, can you directly squash the fix? > >> > >> The commit (c09794fe40e3) can just be dropped without conflicts, but it > >> leaves a use of n->params.num_queues in nvme_create_cq() which commit > >> cde74bfd4b87 ("hw/block/nvme: add max_ioqpairs device parameter") must > >> fix. > > > > Hm, so it seems this isn't easy to squash in without conflicts (and I > > would have to rewrite the whole commit message), so I think it's better > > to just apply the series on top. > > > > One problem with the commit message is that it references commit IDs > > which aren't stable yet. Maybe it's best if I apply these patches, > > manually fix up the commit ID references and then immediately do a pull > > request so that they become stable. > > This is the friendlier way. > > Less friendly way is to drop Klaus's patches and ask him to respin. > While this is a valid outcome, if we can avoid it it will save all of us > review time. If Klaus wants to do that, fine with me. I'm just trying to find the easiest solution for all of us. > > It would be good to have at least one review, though. > > Maxim catched this issue, I'd feel safer if he acks your pre-merge queue. Ok. Maxim, can you please review this series then? Kevin
On Jun 9 17:32, Kevin Wolf wrote: > Am 09.06.2020 um 16:18 hat Philippe Mathieu-Daudé geschrieben: > > On 6/9/20 4:14 PM, Kevin Wolf wrote: > > > Am 09.06.2020 um 13:46 hat Klaus Jensen geschrieben: > > >> On Jun 9 13:17, Philippe Mathieu-Daudé wrote: > > >>> On 6/9/20 11:45 AM, Klaus Jensen wrote: > > >>>> From: Klaus Jensen <k.jensen@samsung.com> > > >>>> > > >>>> I goofed up with commit c09794fe40e3 ("hw/block/nvme: allow use of any > > >>>> valid msix vector"). > > >>> > > >>> Kevin, since your queue isn't merged, can you directly squash the fix? > > >> > > >> The commit (c09794fe40e3) can just be dropped without conflicts, but it > > >> leaves a use of n->params.num_queues in nvme_create_cq() which commit > > >> cde74bfd4b87 ("hw/block/nvme: add max_ioqpairs device parameter") must > > >> fix. > > > > > > Hm, so it seems this isn't easy to squash in without conflicts (and I > > > would have to rewrite the whole commit message), so I think it's better > > > to just apply the series on top. > > > > > > One problem with the commit message is that it references commit IDs > > > which aren't stable yet. Maybe it's best if I apply these patches, > > > manually fix up the commit ID references and then immediately do a pull > > > request so that they become stable. > > > > This is the friendlier way. > > > > Less friendly way is to drop Klaus's patches and ask him to respin. > > While this is a valid outcome, if we can avoid it it will save all of us > > review time. > > If Klaus wants to do that, fine with me. I'm just trying to find the > easiest solution for all of us. > Sure, I can respin it. I would like to include this series as well though since I think it's a nice addition. I'll post a v7 that includes Philippes's return value verification patch as well as the patches in this series. We should only need a review or two on those patches then.
On Tue, 2020-06-09 at 17:32 +0200, Kevin Wolf wrote: > Am 09.06.2020 um 16:18 hat Philippe Mathieu-Daudé geschrieben: > > On 6/9/20 4:14 PM, Kevin Wolf wrote: > > > Am 09.06.2020 um 13:46 hat Klaus Jensen geschrieben: > > > > On Jun 9 13:17, Philippe Mathieu-Daudé wrote: > > > > > On 6/9/20 11:45 AM, Klaus Jensen wrote: > > > > > > From: Klaus Jensen <k.jensen@samsung.com> > > > > > > > > > > > > I goofed up with commit c09794fe40e3 ("hw/block/nvme: allow use of any > > > > > > valid msix vector"). > > > > > > > > > > Kevin, since your queue isn't merged, can you directly squash the fix? > > > > > > > > The commit (c09794fe40e3) can just be dropped without conflicts, but it > > > > leaves a use of n->params.num_queues in nvme_create_cq() which commit > > > > cde74bfd4b87 ("hw/block/nvme: add max_ioqpairs device parameter") must > > > > fix. > > > > > > Hm, so it seems this isn't easy to squash in without conflicts (and I > > > would have to rewrite the whole commit message), so I think it's better > > > to just apply the series on top. > > > > > > One problem with the commit message is that it references commit IDs > > > which aren't stable yet. Maybe it's best if I apply these patches, > > > manually fix up the commit ID references and then immediately do a pull > > > request so that they become stable. > > > > This is the friendlier way. > > > > Less friendly way is to drop Klaus's patches and ask him to respin. > > While this is a valid outcome, if we can avoid it it will save all of us > > review time. > > If Klaus wants to do that, fine with me. I'm just trying to find the > easiest solution for all of us. > > > > It would be good to have at least one review, though. > > > > Maxim catched this issue, I'd feel safer if he acks your pre-merge queue. > > Ok. Maxim, can you please review this series then? > > Kevin I am slowly getting through the heap of the patches trying to understand the current state of things. I will start reviewing all these patches today. Best regards, Maxim Levitsky
On Jul 7 12:10, Maxim Levitsky wrote: > On Tue, 2020-06-09 at 17:32 +0200, Kevin Wolf wrote: > > Am 09.06.2020 um 16:18 hat Philippe Mathieu-Daudé geschrieben: > > > On 6/9/20 4:14 PM, Kevin Wolf wrote: > > > > Am 09.06.2020 um 13:46 hat Klaus Jensen geschrieben: > > > > > On Jun 9 13:17, Philippe Mathieu-Daudé wrote: > > > > > > On 6/9/20 11:45 AM, Klaus Jensen wrote: > > > > > > > From: Klaus Jensen <k.jensen@samsung.com> > > > > > > > > > > > > > > I goofed up with commit c09794fe40e3 ("hw/block/nvme: allow use of any > > > > > > > valid msix vector"). > > > > > > > > > > > > Kevin, since your queue isn't merged, can you directly squash the fix? > > > > > > > > > > The commit (c09794fe40e3) can just be dropped without conflicts, but it > > > > > leaves a use of n->params.num_queues in nvme_create_cq() which commit > > > > > cde74bfd4b87 ("hw/block/nvme: add max_ioqpairs device parameter") must > > > > > fix. > > > > > > > > Hm, so it seems this isn't easy to squash in without conflicts (and I > > > > would have to rewrite the whole commit message), so I think it's better > > > > to just apply the series on top. > > > > > > > > One problem with the commit message is that it references commit IDs > > > > which aren't stable yet. Maybe it's best if I apply these patches, > > > > manually fix up the commit ID references and then immediately do a pull > > > > request so that they become stable. > > > > > > This is the friendlier way. > > > > > > Less friendly way is to drop Klaus's patches and ask him to respin. > > > While this is a valid outcome, if we can avoid it it will save all of us > > > review time. > > > > If Klaus wants to do that, fine with me. I'm just trying to find the > > easiest solution for all of us. > > > > > > It would be good to have at least one review, though. > > > > > > Maxim catched this issue, I'd feel safer if he acks your pre-merge queue. > > > > Ok. Maxim, can you please review this series then? > > > > Kevin > I am slowly getting through the heap of the patches trying to understand the current state of things. > I will start reviewing all these patches today. > Hi Maxim, Yeah, I bombed it again; sorry! ;) "[PATCH v3 00/18] hw/block/nvme: bump to v1.3" is the series currently under review. I also posted: [PATCH 00/17] hw/block/nvme: AIO and address mapping refactoring, [PATCH 0/2] hw/block/nvme: handle transient dma errors [PATCH 0/3] hw/block/nvme: support scatter gather lists [PATCH 0/4] hw/block/nvme: support multiple namespaces [PATCH] hw/block/nvme: make lba data size configurable [PATCH] hw/block/nvme: add support for dulbe [PATCH 0/3] hw/block/nvme: bump to v1.4 [PATCH 00/10] hw/block/nvme: namespace types and zoned namespaces I really appreciate you reviewing! Your R-b's are on a lot of the patches already, thanks for that!
From: Klaus Jensen <k.jensen@samsung.com> I goofed up with commit c09794fe40e3 ("hw/block/nvme: allow use of any valid msix vector"). This fixes the goof by adding a new msix_qsize parameter. As a nice side-effect this allows a device with less interrupt vectors available than supported queues. Also, improve the error handling in nvme_init_pci(). Kevin, please consider picking this up for the block branch when reviewed. Cc: qemu-devel@nongnu.org Cc: Keith Busch <kbusch@kernel.org> Cc: Max Reitz <mreitz@redhat.com> Cc: Kevin Wolf <kwolf@redhat.com> Cc: Klaus Jensen <its@irrelevant.dk> Cc: Javier Gonzalez <javier.gonz@samsung.com> Cc: Maxim Levitsky <mlevitsk@redhat.com> Cc: Philippe Mathieu-Daudé <philmd@redhat.com> Klaus Jensen (2): hw/block/nvme: add msix_qsize parameter hw/block/nvme: verify msix_init_exclusive_bar() return value hw/block/nvme.c | 28 ++++++++++++++++++++++------ hw/block/nvme.h | 1 + 2 files changed, 23 insertions(+), 6 deletions(-)