Re: [PATCH][kvmtool] virtio/pci: Correctly handle MSI-X masking while MSI-X is disabled

[Marc Zyngier <maz@kernel.org>]

Hi Andre,

On Mon, 23 Aug 2021 17:48:33 +0100,
Andre Przywara <andre.przywara@arm.com> wrote:
> 
> On Sat, 21 Aug 2021 13:07:42 +0100
> Marc Zyngier <maz@kernel.org> wrote:
> 
> Hi Marc,
> 
> > Since Linux commit 7d5ec3d36123 ("PCI/MSI: Mask all unused MSI-X
> > entries"), kvmtool segfaults when the guest boots and tries to
> > disable all the MSI-X entries of a virtio device while MSI-X itself
> > is disabled.
> > 
> > What Linux does is seems perfectly correct. However, kvmtool uses
> > a different decoding depending on whether MSI-X is enabled for
> > this device or not. Which seems pretty wrong.
> 
> While I really wish this would be wrong, I think this is
> indeed how this is supposed to work: The Virtio legacy spec makes the
> existence of those two virtio config fields dependent on the
> (dynamic!) enablement status of MSI-X. This is reflected in:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/virtio_pci.h#n72
> and explicitly mentioned as a footnote in the virtio 0.9.5 spec[1]:
> "3) ie. once you enable MSI-X on the device, the other fields move. If
> you turn it off again, they move back!"

Madness! What was Rusty on at the time? I really hope the bitcoin
thing is buying him better stuff...

> I agree that this looks like a bad idea, but I am afraid we are stuck
> with this. It looks like the Linux driver is at fault here, it should
> not issue the config access when MSIs are disabled. Something like this
> (untested):
> 
> --- a/drivers/virtio/virtio_pci_legacy.c
> +++ b/drivers/virtio/virtio_pci_legacy.c
> @@ -103,6 +103,9 @@ static void vp_reset(struct virtio_device *vdev)
>  
>  static u16 vp_config_vector(struct virtio_pci_device *vp_dev, u16 vector)
>  {
> +       if (!vp_dev->msix_enabled)
> +               return VIRTIO_MSI_NO_VECTOR;
> +
>         /* Setup the vector used for configuration events */
>         iowrite16(vector, vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
>         /* Verify we had enough resources to assign the vector */
> 
> This is just my first idea after looking at this, happy to stand
> corrected or hear about a better solution.

I don't think this works. It instead completely disables MSI-X, which
is a total bore. I think the only way to deal with it is to quirk it
to prevent the bulk masking to take effect before MSI-X is enabled.

Gawd, more crap. Just what we need...

	M.

-- 
Without deviation from the norm, progress is not possible.