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

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

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.


-- 
MST

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