Virtualization - Sep 2014 - [PATCH RFC] virtio-pci: share config interrupt between virtio devices

On Monday 01 September 2014 09:37:30, Michael S. Tsirkin
wrote:
> Why do we need INT#x?
> How about setting IRQF_SHARED for the config interrupt
> while using MSI-X? You'd have to read ISR to check that the
> interrupt was intended for your device.
The virtio 0.9.5 spec says that ISR is "unused" when in MSI-X mode. I 
don't think that you can depend on the device to set the configuration 
changed bit.

The virtio 1.0 spec seems to have fixed that.

On Thu, Sep 18, 2014 at 09:18:37PM +0200, Stefan Fritsch
wrote:
> On Monday 01 September 2014 09:37:30, Michael S. Tsirkin wrote:
> > Why do we need INT#x?
> > How about setting IRQF_SHARED for the config interrupt
> > while using MSI-X? You'd have to read ISR to check that the
> > interrupt was intended for your device.
> 
> The virtio 0.9.5 spec says that ISR is "unused" when in MSI-X
mode. I
> don't think that you can depend on the device to set the configuration 
> changed bit.
> The virtio 1.0 spec seems to have fixed that.
Yes, virtio 0.9.5 has this bug. But in practice qemu always set this
bit, so for qemu we could do that unconditionally.  Pekka's lkvm tool
doesn't unfortunately.  It's easy to fix that, but it would be nicer to
additionally probe for old versions of the tool, and disable IRQF_SHARED
in that case.

To complicate things, lkvm does not use a distinct subsystem vendor ID,
in spite of the fact the virtio spec always required this explicitly.

After poking at things, we could probably try and distinguish old lkmv
based on bar sizes. I think lkvm has:
#define IOPORT_SIZE			0x400
this is the size of the IO bar (bar0) correct?
Qemu's BAR is smaller.

So if
1. new versions of lkvm are fixed to always set ISR on config change
  even when msi is enabled
2 lkvm folks can promise not to make bar0 size smaller *before*
  fixing (1)

then we could use the heuristic:
	bar size == 0x400
to clear IRQF_SHARED.

Cc some lkvm folks for all of the above: would you guys be
happier with some other heuristic?

I'd like to note that lkvm really should get some vendor to request and
then donate a subsystem vendor id (registered with pci sig) for their
use, instead of pretending they are qemu.

AFAIK a subsystem vendor id does not cost money to register, but
only pci sig members can do this, and membership costs $3000.
Maybe we should combine all this with checking subsystem vendor id,
and only implement the optimization if it matches qemu, for now.
This needs some thought.

-- 
MST

On Sunday 21 September 2014 11:09:14, Michael S. Tsirkin
wrote:
> On Thu, Sep 18, 2014 at 09:18:37PM +0200, Stefan Fritsch wrote:
> > On Monday 01 September 2014 09:37:30, Michael S. Tsirkin wrote:
> > > Why do we need INT#x?
> > > How about setting IRQF_SHARED for the config interrupt
> > > while using MSI-X? You'd have to read ISR to check that the
> > > interrupt was intended for your device.
> >
> > 
> >
> > The virtio 0.9.5 spec says that ISR is "unused" when in
MSI-X
> > mode. I  don't think that you can depend on the device to set the
> > configuration changed bit.
> > The virtio 1.0 spec seems to have fixed that.
> 
> Yes, virtio 0.9.5 has this bug. But in practice qemu always set this
> bit, so for qemu we could do that unconditionally.  Pekka's lkvm
> tool doesn't unfortunately.  It's easy to fix that, but it would be
> nicer to additionally probe for old versions of the tool, and
> disable IRQF_SHARED in that case.
What about other implementations? I think Linux should try to conform 
to the spec so that all device implementations which conform to the 
spec just work.

One implementation that comes to mind is virtualbox. But from a quick 
look at the source, it seems that it sets the ISR bit always, too. And 
it uses qemu's subsystem vendor id.

But there are other implementations. For example bhyve.

> AFAIK a subsystem vendor id does not cost money to register, but
> only pci sig members can do this, and membership costs $3000.
> Maybe we should combine all this with checking subsystem vendor id,
> and only implement the optimization if it matches qemu, for now.
> This needs some thought.
Maybe the virtio spec should include a way to query the vendor that 
does not involve the pci sig. Maybe use a string? Then no registry 
would be necessary.

On 09/21/2014 04:09 AM, Michael S. Tsirkin wrote:
>> The virtio 0.9.5 spec says that ISR is "unused" when in MSI-X
mode. I
>> > don't think that you can depend on the device to set the
configuration
>> > changed bit.
>> > The virtio 1.0 spec seems to have fixed that.
> Yes, virtio 0.9.5 has this bug. But in practice qemu always set this
> bit, so for qemu we could do that unconditionally.  Pekka's lkvm tool
> doesn't unfortunately.  It's easy to fix that, but it would be
nicer to
> additionally probe for old versions of the tool, and disable IRQF_SHARED
> in that case.
>
> To complicate things, lkvm does not use a distinct subsystem vendor ID,
> in spite of the fact the virtio spec always required this explicitly.
I think I may be a bit confused here, but AFAIK we do set subsystem vendor
ID properly for our virtio-pci devices?

        vpci->pci_hdr = (struct pci_device_header) {
                .vendor_id              =
cpu_to_le16(PCI_VENDOR_ID_REDHAT_QUMRANET),
                .device_id              = cpu_to_le16(device_id),
		[...]
                .subsys_vendor_id       =
cpu_to_le16(PCI_SUBSYSTEM_VENDOR_ID_REDHAT_QUMRANET),


Thanks,
Sasha