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

Sasha Levin
2014-09-21T15:19:36Z

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