Hi Kunkun,
On Wed, 08 Nov 2023 09:45:51 +0000,
Kunkun Jiang <[email protected]> wrote:
>
> Hi Marc,
>
> On 2023/11/6 23:33, Marc Zyngier wrote:
> > On Mon, 06 Nov 2023 14:59:01 +0000,
> > Kunkun Jiang <[email protected]> wrote:
> >> The virtio-pci driver write entry1-6
> >> massage.data in the msix-table and trap to QEMU for processing. The
> >> massage.data is as follow:
> >>> entry-0 0
> >>> entry-1 1
> >>> entry-2 1
> >>> entry-3 1
> >>> entry-4 1
> >>> entry-5 1
> >>> entry-6 1
> > Urgh... is vp_modern_queue_vector() used in your configuration? This
> > is ... terrible.
> I encountered this problem using the 4.19 version kernel, but not the
> 5.10 version. This vp_modern_queue_vector() function does not exist
> in 4.19, but it uses 'vp_iowrite16(msix_vec, &cfg->queue_msix_vector)',
> the same as vp_modern_queue_vector().
>
> In the past two days, I learned about the virtio driver and made some
> new discoveries. When 'num_queues' is greater than maxcpus, it will
> fall back into MSI-X with one shared for queues. The two patches[1],
> submitted by Dongli, limits the number of hw queues used by
> virtio-blk/virtio-scsi by 'nr_cpu_ids'. The two patches were merged
> in 5.1-rc2. And the patch related virtio-blk was merged into the 4.19
> stable branch.The patch related virtio-scsi was not merged.
> [1]
> https://lore.kernel.org/all/[email protected]/
>
> This is the earliest discussion.
> https://lore.kernel.org/all/e4afe4c5-0262-4500-aeec-60f30734b4fc@default/
>
> I don't know if there are other circumstances that would cause it to
> fall back into MSI-X with one shared for queues. At least the hack
> method is possible.
> > I wonder if PCIe actually allows this sort of thing.
> Do you think the virtio driver should be modified?
I think the virtio driver should stop messing with the MSI-X
configuration behind the kernel's back. For example, what happens if
the kernel needs to do a disable_irq() on the "shared" interrupt? It
will mask the interrupt in *one* of the vectors, and the interrupt
will still be screaming.
This is terribly broken, even on x86.
> > In any case, this sort of behaviour breaks so many thing in KVM's
> > implementation that I'd recommend you disable GICv4 until we have a
> > good solution for that.
> There seems to be no restriction in the GIC specification that multiple
> host irqs cannot be mapped to the same vlpi. Or maybe I didn't notice.
> Do you think there are any risks?
Please see 5.2.10 ("Restrictions for INTID mapping rules"), which
clearly forbids the case we have here: "Maps multiple EventID-DeviceID
combinations to the same virtual LPI INTID-vPEID.".
> GICv3 does not have this issue, but is this configuration legal?
With GICv3, the ITS doesn't see multiple mappings to the same
LPI. Each DeviceID/EventID pair has its own LPI, and KVM will just see
the injection callback from VFIO.
Honestly, the virtio driver is broken (irrespective of the
architecture), and incompatible with the GIC architecture.
M.
--
Without deviation from the norm, progress is not possible.
On Sat, Dec 2, 2023 at 8:20 PM Marc Zyngier <[email protected]> wrote:
>
> Hi Kunkun,
>
> On Wed, 08 Nov 2023 09:45:51 +0000,
> Kunkun Jiang <[email protected]> wrote:
> >
> > Hi Marc,
> >
> > On 2023/11/6 23:33, Marc Zyngier wrote:
> > > On Mon, 06 Nov 2023 14:59:01 +0000,
> > > Kunkun Jiang <[email protected]> wrote:
> > >> The virtio-pci driver write entry1-6
> > >> massage.data in the msix-table and trap to QEMU for processing. The
> > >> massage.data is as follow:
> > >>> entry-0 0
> > >>> entry-1 1
> > >>> entry-2 1
> > >>> entry-3 1
> > >>> entry-4 1
> > >>> entry-5 1
> > >>> entry-6 1
It looks like a bug from the driver. It should only use vector 0 and 1
in this case.
Could you please check the queue_msix_vector for each queue in this case?
> > > Urgh... is vp_modern_queue_vector() used in your configuration? This
> > > is ... terrible.
> > I encountered this problem using the 4.19 version kernel, but not the
> > 5.10 version. This vp_modern_queue_vector() function does not exist
> > in 4.19, but it uses 'vp_iowrite16(msix_vec, &cfg->queue_msix_vector)',
> > the same as vp_modern_queue_vector().
> >
> > In the past two days, I learned about the virtio driver and made some
> > new discoveries. When 'num_queues' is greater than maxcpus, it will
> > fall back into MSI-X with one shared for queues. The two patches[1],
> > submitted by Dongli, limits the number of hw queues used by
> > virtio-blk/virtio-scsi by 'nr_cpu_ids'. The two patches were merged
> > in 5.1-rc2. And the patch related virtio-blk was merged into the 4.19
> > stable branch.The patch related virtio-scsi was not merged.
> > [1]
> > https://lore.kernel.org/all/[email protected]/
> >
> > This is the earliest discussion.
> > https://lore.kernel.org/all/e4afe4c5-0262-4500-aeec-60f30734b4fc@default/
> >
> > I don't know if there are other circumstances that would cause it to
> > fall back into MSI-X with one shared for queues. At least the hack
> > method is possible.
> > > I wonder if PCIe actually allows this sort of thing.
> > Do you think the virtio driver should be modified?
>
> I think the virtio driver should stop messing with the MSI-X
> configuration behind the kernel's back. For example, what happens if
> the kernel needs to do a disable_irq() on the "shared" interrupt? It
> will mask the interrupt in *one* of the vectors, and the interrupt
> will still be screaming.
>
> This is terribly broken, even on x86.
Let's try to see the queue_msix_vector. A correct implemented device
should not try to use a vector more than 1.
>
> > > In any case, this sort of behaviour breaks so many thing in KVM's
> > > implementation that I'd recommend you disable GICv4 until we have a
> > > good solution for that.
> > There seems to be no restriction in the GIC specification that multiple
> > host irqs cannot be mapped to the same vlpi. Or maybe I didn't notice.
> > Do you think there are any risks?
>
> Please see 5.2.10 ("Restrictions for INTID mapping rules"), which
> clearly forbids the case we have here: "Maps multiple EventID-DeviceID
> combinations to the same virtual LPI INTID-vPEID.".
>
> > GICv3 does not have this issue, but is this configuration legal?
>
> With GICv3, the ITS doesn't see multiple mappings to the same
> LPI. Each DeviceID/EventID pair has its own LPI, and KVM will just see
> the injection callback from VFIO.
>
> Honestly, the virtio driver is broken (irrespective of the
> architecture), and incompatible with the GIC architecture.
No matter how the driver is written, the host/KVM should be able to
survive from that.
Thanks
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
>
On Mon, Dec 4, 2023 at 4:39 PM Jason Wang <[email protected]> wrote:
>
> On Sat, Dec 2, 2023 at 8:20 PM Marc Zyngier <[email protected]> wrote:
> >
> > Hi Kunkun,
> >
> > On Wed, 08 Nov 2023 09:45:51 +0000,
> > Kunkun Jiang <[email protected]> wrote:
> > >
> > > Hi Marc,
> > >
> > > On 2023/11/6 23:33, Marc Zyngier wrote:
> > > > On Mon, 06 Nov 2023 14:59:01 +0000,
> > > > Kunkun Jiang <[email protected]> wrote:
> > > >> The virtio-pci driver write entry1-6
> > > >> massage.data in the msix-table and trap to QEMU for processing. The
> > > >> massage.data is as follow:
> > > >>> entry-0 0
> > > >>> entry-1 1
> > > >>> entry-2 1
> > > >>> entry-3 1
> > > >>> entry-4 1
> > > >>> entry-5 1
> > > >>> entry-6 1
>
> It looks like a bug from the driver. It should only use vector 0 and 1
> in this case.
>
> Could you please check the queue_msix_vector for each queue in this case?
Or did you actually mean queue_msix_vector here? I'm not familiar with
ARM but 0 doesn't seem to be a good message.data anyhow.
Thanks
On Mon, 04 Dec 2023 08:39:00 +0000,
Jason Wang <[email protected]> wrote:
>
> > Honestly, the virtio driver is broken (irrespective of the
> > architecture), and incompatible with the GIC architecture.
>
> No matter how the driver is written, the host/KVM should be able to
> survive from that.
The host is perfectly fine, thanks for asking. The guest, however, is
in a bad shape.
M.
--
Without deviation from the norm, progress is not possible.
On Mon, 04 Dec 2023 08:47:49 +0000,
Jason Wang <[email protected]> wrote:
>
> On Mon, Dec 4, 2023 at 4:39 PM Jason Wang <[email protected]> wrote:
> >
> > On Sat, Dec 2, 2023 at 8:20 PM Marc Zyngier <[email protected]> wrote:
> > >
> > > Hi Kunkun,
> > >
> > > On Wed, 08 Nov 2023 09:45:51 +0000,
> > > Kunkun Jiang <[email protected]> wrote:
> > > >
> > > > Hi Marc,
> > > >
> > > > On 2023/11/6 23:33, Marc Zyngier wrote:
> > > > > On Mon, 06 Nov 2023 14:59:01 +0000,
> > > > > Kunkun Jiang <[email protected]> wrote:
> > > > >> The virtio-pci driver write entry1-6
> > > > >> massage.data in the msix-table and trap to QEMU for processing. The
> > > > >> massage.data is as follow:
> > > > >>> entry-0 0
> > > > >>> entry-1 1
> > > > >>> entry-2 1
> > > > >>> entry-3 1
> > > > >>> entry-4 1
> > > > >>> entry-5 1
> > > > >>> entry-6 1
> >
> > It looks like a bug from the driver. It should only use vector 0 and 1
> > in this case.
> >
> > Could you please check the queue_msix_vector for each queue in this case?
>
> Or did you actually mean queue_msix_vector here? I'm not familiar with
> ARM but 0 doesn't seem to be a good message.data anyhow.
Why? What's special about 0? 0 is a perfectly fine value.
M.
--
Without deviation from the norm, progress is not possible.
On Mon, Dec 4, 2023 at 5:00 PM Marc Zyngier <[email protected]> wrote:
>
> On Mon, 04 Dec 2023 08:47:49 +0000,
> Jason Wang <[email protected]> wrote:
> >
> > On Mon, Dec 4, 2023 at 4:39 PM Jason Wang <[email protected]> wrote:
> > >
> > > On Sat, Dec 2, 2023 at 8:20 PM Marc Zyngier <[email protected]> wrote:
> > > >
> > > > Hi Kunkun,
> > > >
> > > > On Wed, 08 Nov 2023 09:45:51 +0000,
> > > > Kunkun Jiang <[email protected]> wrote:
> > > > >
> > > > > Hi Marc,
> > > > >
> > > > > On 2023/11/6 23:33, Marc Zyngier wrote:
> > > > > > On Mon, 06 Nov 2023 14:59:01 +0000,
> > > > > > Kunkun Jiang <[email protected]> wrote:
> > > > > >> The virtio-pci driver write entry1-6
> > > > > >> massage.data in the msix-table and trap to QEMU for processing. The
> > > > > >> massage.data is as follow:
> > > > > >>> entry-0 0
> > > > > >>> entry-1 1
> > > > > >>> entry-2 1
> > > > > >>> entry-3 1
> > > > > >>> entry-4 1
> > > > > >>> entry-5 1
> > > > > >>> entry-6 1
> > >
> > > It looks like a bug from the driver. It should only use vector 0 and 1
> > > in this case.
> > >
> > > Could you please check the queue_msix_vector for each queue in this case?
> >
> > Or did you actually mean queue_msix_vector here? I'm not familiar with
> > ARM but 0 doesn't seem to be a good message.data anyhow.
>
> Why? What's special about 0? 0 is a perfectly fine value.
Ok, looks like I mis-read it as a message.addr. So it's fine.
Thanks
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
>