Re: [PATCH RFC] virtio-pci: share config interrupt between virtio devices

["Michael S. Tsirkin" <mst@redhat.com>]

On Sun, Sep 21, 2014 at 11:19:36AM -0400, Sasha Levin wrote:
> On 09/21/2014 11:02 AM, Michael S. Tsirkin wrote:
> > On Sun, Sep 21, 2014 at 09:47:51AM -0400, Sasha Levin wrote:
> >> > 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
> > 
> > Yes but the spec says:
> > 	The Subsystem Vendor ID should reflect the PCI Vendor ID of the environment.
> > 
> > IOW lkvm shouldn't reuse the ID from qemu, it should have its own
> > (qemu and lkvm hypervisors being a different environment).
> > 
> > virtio 1.0 have weakened this requirement:
> > 	The PCI Subsystem Vendor ID and the PCI Subsystem Device ID MAY
> > 	reflect the PCI Vendor and Device
> > 	ID of the environment (for informational purposes by the driver).
> > 
> > I reasoned that since it's for informational purposes only, there's no
> > reason to make it a SHOULD.
> > 
> > It might or might not be a good idea to change it back, worth
> > considering.
> 
> Ow. The 0.9.5 spec also says:
> 
> 	"(it's currently only used for informational purposes by the guest)."
> 
> That and the combination of "should" rather then "must" (recommended rather than
> required) prompted us to just put something that works in there and leave it be.
> 
> 
> Thanks,
> Sasha

Note "currently" as well as "should" which means "before you don't, make
sure you understand the implications".


-- 
MST