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

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 Sun, Sep 21, 2014 at 11:36:44AM +0200, Stefan Fritsch
wrote:
> 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.
I couldn't find any code in bhyve that sets VTCFG_ISR_CONF_CHANGED.
Maybe it doesn't generate config changed interrupts?

bhyve sets subsystem vendor to 0 apparently?
We could use that to detect it.

But maybe we should just make it a 1.0 only feature.
> 
> > 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.
We can make the requirement for the vendor specific ID stronger in 1.0,
SHOULD instead of MAY.

But it seems that people will still copy-paste working code
across hypervisors, I'm not sure this can be helped.

-- 
MST

On Sunday 21 September 2014 13:21:06, Michael S. Tsirkin
wrote:
> On Sun, Sep 21, 2014 at 11:36:44AM +0200, Stefan Fritsch wrote:
> > 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.
> 
> I couldn't find any code in bhyve that sets VTCFG_ISR_CONF_CHANGED.
> Maybe it doesn't generate config changed interrupts?
> 
> bhyve sets subsystem vendor to 0 apparently?
> We could use that to detect it.
My point was that there are many virtio implementations by now and you 
can't assume you know all of them.
> But maybe we should just make it a 1.0 only feature.
FWIW, I think that would be the better option.