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

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

[Amos Kong]

One VM only has 128 msix interrupt, virtio-config interrupt
has less workload. This patch shares one normal interrupt
for configuration between virtio devices.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 drivers/virtio/virtio_pci.c | 41 ++++++++++++++++-------------------------
 1 file changed, 16 insertions(+), 25 deletions(-)

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 3d1463c..b1263b3 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -52,6 +52,7 @@ struct virtio_pci_device
 	/* Name strings for interrupts. This size should be enough,
 	 * and I'm too lazy to allocate each name separately. */
 	char (*msix_names)[256];
+	char config_msix_name[256];
 	/* Number of available vectors */
 	unsigned msix_vectors;
 	/* Vectors allocated, excluding per-vq vectors if any */
@@ -282,12 +283,6 @@ static void vp_free_vectors(struct virtio_device *vdev)
 			free_cpumask_var(vp_dev->msix_affinity_masks[i]);
 
 	if (vp_dev->msix_enabled) {
-		/* Disable the vector used for configuration */
-		iowrite16(VIRTIO_MSI_NO_VECTOR,
-			  vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
-		/* Flush the write out to device */
-		ioread16(vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
-
 		pci_disable_msix(vp_dev->pci_dev);
 		vp_dev->msix_enabled = 0;
 	}
@@ -339,24 +334,18 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
 		goto error;
 	vp_dev->msix_enabled = 1;
 
-	/* Set the vector used for configuration */
-	v = vp_dev->msix_used_vectors;
-	snprintf(vp_dev->msix_names[v], sizeof *vp_dev->msix_names,
+	/* Set shared IRQ for configuration */
+	snprintf(vp_dev->config_msix_name, sizeof(*vp_dev->msix_names),
 		 "%s-config", name);
-	err = request_irq(vp_dev->msix_entries[v].vector,
-			  vp_config_changed, 0, vp_dev->msix_names[v],
+	err = request_irq(vp_dev->pci_dev->irq,
+			  vp_config_changed,
+			  IRQF_SHARED,
+			  vp_dev->config_msix_name,
 			  vp_dev);
-	if (err)
-		goto error;
-	++vp_dev->msix_used_vectors;
-
-	iowrite16(v, vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
-	/* Verify we had enough resources to assign the vector */
-	v = ioread16(vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
-	if (v == VIRTIO_MSI_NO_VECTOR) {
-		err = -EBUSY;
+	if (!err)
+		vp_dev->intx_enabled = 1;
+	else
 		goto error;
-	}
 
 	if (!per_vq_vectors) {
 		/* Shared vector for all VQs */
@@ -535,14 +524,16 @@ static int vp_try_to_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 			goto error_request;
 	} else {
 		if (per_vq_vectors) {
-			/* Best option: one for change interrupt, one per vq. */
-			nvectors = 1;
+			/* Best option: one normal interrupt for change,
+			   one msix per vq. */
+			nvectors = 0;
 			for (i = 0; i < nvqs; ++i)
 				if (callbacks[i])
 					++nvectors;
 		} else {
-			/* Second best: one for change, shared for all vqs. */
-			nvectors = 2;
+			/* Second best: one normal interrupt for
+			   change, share one msix for all vqs. */
+			nvectors = 1;
 		}
 
 		err = vp_request_msix_vectors(vdev, nvectors, per_vq_vectors);
-- 
1.9.3

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

[Michael S. Tsirkin]

On Mon, Sep 01, 2014 at 01:41:54PM +0800, Amos Kong wrote:
> One VM only has 128 msix interrupt, virtio-config interrupt
> has less workload. This patch shares one normal interrupt

normal == INT#x? Please don't call it normal. The proper name is
"legacy INT#x".

So you are trying to use legacy INT#x at the same time
with MSI-X?  This does not work: the PCI spec says:
	While enabled for MSI or MSI-X
	operation, a function is prohibited from using its INTx# pin (if
	implemented) to request
	service (MSI, MSI-X, and INTx# are mutually exclusive).

does the patch work for you? If it does it might be a (minor) spec
violation in kvm.

Besides, INT#x really leads to terrible performance because
sharing is forced even if there aren't many devices.

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.


> for configuration between virtio devices.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  drivers/virtio/virtio_pci.c | 41 ++++++++++++++++-------------------------
>  1 file changed, 16 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index 3d1463c..b1263b3 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -52,6 +52,7 @@ struct virtio_pci_device
>  	/* Name strings for interrupts. This size should be enough,
>  	 * and I'm too lazy to allocate each name separately. */
>  	char (*msix_names)[256];
> +	char config_msix_name[256];
>  	/* Number of available vectors */
>  	unsigned msix_vectors;
>  	/* Vectors allocated, excluding per-vq vectors if any */
> @@ -282,12 +283,6 @@ static void vp_free_vectors(struct virtio_device *vdev)
>  			free_cpumask_var(vp_dev->msix_affinity_masks[i]);
>  
>  	if (vp_dev->msix_enabled) {
> -		/* Disable the vector used for configuration */
> -		iowrite16(VIRTIO_MSI_NO_VECTOR,
> -			  vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
> -		/* Flush the write out to device */
> -		ioread16(vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
> -
>  		pci_disable_msix(vp_dev->pci_dev);
>  		vp_dev->msix_enabled = 0;
>  	}
> @@ -339,24 +334,18 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
>  		goto error;
>  	vp_dev->msix_enabled = 1;
>  
> -	/* Set the vector used for configuration */
> -	v = vp_dev->msix_used_vectors;
> -	snprintf(vp_dev->msix_names[v], sizeof *vp_dev->msix_names,
> +	/* Set shared IRQ for configuration */
> +	snprintf(vp_dev->config_msix_name, sizeof(*vp_dev->msix_names),
>  		 "%s-config", name);
> -	err = request_irq(vp_dev->msix_entries[v].vector,
> -			  vp_config_changed, 0, vp_dev->msix_names[v],
> +	err = request_irq(vp_dev->pci_dev->irq,
> +			  vp_config_changed,
> +			  IRQF_SHARED,
> +			  vp_dev->config_msix_name,
>  			  vp_dev);
> -	if (err)
> -		goto error;
> -	++vp_dev->msix_used_vectors;
> -
> -	iowrite16(v, vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
> -	/* Verify we had enough resources to assign the vector */
> -	v = ioread16(vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
> -	if (v == VIRTIO_MSI_NO_VECTOR) {
> -		err = -EBUSY;
> +	if (!err)
> +		vp_dev->intx_enabled = 1;
> +	else
>  		goto error;
> -	}
>  
>  	if (!per_vq_vectors) {
>  		/* Shared vector for all VQs */
> @@ -535,14 +524,16 @@ static int vp_try_to_find_vqs(struct virtio_device *vdev, unsigned nvqs,
>  			goto error_request;
>  	} else {
>  		if (per_vq_vectors) {
> -			/* Best option: one for change interrupt, one per vq. */
> -			nvectors = 1;
> +			/* Best option: one normal interrupt for change,
> +			   one msix per vq. */
> +			nvectors = 0;
>  			for (i = 0; i < nvqs; ++i)
>  				if (callbacks[i])
>  					++nvectors;
>  		} else {
> -			/* Second best: one for change, shared for all vqs. */
> -			nvectors = 2;
> +			/* Second best: one normal interrupt for
> +			   change, share one msix for all vqs. */
> +			nvectors = 1;
>  		}
>  
>  		err = vp_request_msix_vectors(vdev, nvectors, per_vq_vectors);
> -- 
> 1.9.3

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

[Amos Kong]

On Mon, Sep 01, 2014 at 09:37:30AM +0300, Michael S. Tsirkin wrote:
>

Hi Michael,

> On Mon, Sep 01, 2014 at 01:41:54PM +0800, Amos Kong wrote:
> > One VM only has 128 msix interrupt, virtio-config interrupt
> > has less workload. This patch shares one normal interrupt

Thanks for your quick reply.

> normal == INT#x? Please don't call it normal. The proper name is
> "legacy INT#x".
 
OK

> So you are trying to use legacy INT#x at the same time
> with MSI-X?  This does not work: the PCI spec says:
> 	While enabled for MSI or MSI-X
> 	operation, a function is prohibited from using its INTx# pin (if
> 	implemented) to request
> 	service (MSI, MSI-X, and INTx# are mutually exclusive).

It means we can't use interrupt and triggered-interrupt together for
one PCI device. I will study this problem.
 
> does the patch work for you? If it does it might be a (minor) spec
> violation in kvm.

I did some basic testing (multiple nics, scp, ping, etc), it works.
 
> Besides, INT#x really leads to terrible performance because
> sharing is forced even if there aren't many devices.
> 
> 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.
 
I have a draft patch to share one MSI-X for all virtio-config, it has
some problem in hotplugging devices. I will continue this way.
 
> > for configuration between virtio devices.
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> >  drivers/virtio/virtio_pci.c | 41 ++++++++++++++++-------------------------
> >  1 file changed, 16 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> > index 3d1463c..b1263b3 100644
> > --- a/drivers/virtio/virtio_pci.c
> > +++ b/drivers/virtio/virtio_pci.c
> > @@ -52,6 +52,7 @@ struct virtio_pci_device
> >  	/* Name strings for interrupts. This size should be enough,
> >  	 * and I'm too lazy to allocate each name separately. */
> >  	char (*msix_names)[256];
> > +	char config_msix_name[256];
> >  	/* Number of available vectors */
> >  	unsigned msix_vectors;
> >  	/* Vectors allocated, excluding per-vq vectors if any */
> > @@ -282,12 +283,6 @@ static void vp_free_vectors(struct virtio_device *vdev)
> >  			free_cpumask_var(vp_dev->msix_affinity_masks[i]);
> >  
> >  	if (vp_dev->msix_enabled) {
> > -		/* Disable the vector used for configuration */
> > -		iowrite16(VIRTIO_MSI_NO_VECTOR,
> > -			  vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
> > -		/* Flush the write out to device */
> > -		ioread16(vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
> > -
> >  		pci_disable_msix(vp_dev->pci_dev);
> >  		vp_dev->msix_enabled = 0;
> >  	}
> > @@ -339,24 +334,18 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
> >  		goto error;
> >  	vp_dev->msix_enabled = 1;
> >  
> > -	/* Set the vector used for configuration */
> > -	v = vp_dev->msix_used_vectors;
> > -	snprintf(vp_dev->msix_names[v], sizeof *vp_dev->msix_names,
> > +	/* Set shared IRQ for configuration */
> > +	snprintf(vp_dev->config_msix_name, sizeof(*vp_dev->msix_names),
> >  		 "%s-config", name);
> > -	err = request_irq(vp_dev->msix_entries[v].vector,
> > -			  vp_config_changed, 0, vp_dev->msix_names[v],
> > +	err = request_irq(vp_dev->pci_dev->irq,
> > +			  vp_config_changed,
> > +			  IRQF_SHARED,
> > +			  vp_dev->config_msix_name,
> >  			  vp_dev);
> > -	if (err)
> > -		goto error;
> > -	++vp_dev->msix_used_vectors;
> > -
> > -	iowrite16(v, vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
> > -	/* Verify we had enough resources to assign the vector */
> > -	v = ioread16(vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
> > -	if (v == VIRTIO_MSI_NO_VECTOR) {
> > -		err = -EBUSY;
> > +	if (!err)
> > +		vp_dev->intx_enabled = 1;
> > +	else
> >  		goto error;
> > -	}
> >  
> >  	if (!per_vq_vectors) {
> >  		/* Shared vector for all VQs */
> > @@ -535,14 +524,16 @@ static int vp_try_to_find_vqs(struct virtio_device *vdev, unsigned nvqs,
> >  			goto error_request;
> >  	} else {
> >  		if (per_vq_vectors) {
> > -			/* Best option: one for change interrupt, one per vq. */
> > -			nvectors = 1;
> > +			/* Best option: one normal interrupt for change,
> > +			   one msix per vq. */
> > +			nvectors = 0;
> >  			for (i = 0; i < nvqs; ++i)
> >  				if (callbacks[i])
> >  					++nvectors;
> >  		} else {
> > -			/* Second best: one for change, shared for all vqs. */
> > -			nvectors = 2;
> > +			/* Second best: one normal interrupt for
> > +			   change, share one msix for all vqs. */
> > +			nvectors = 1;
> >  		}
> >  
> >  		err = vp_request_msix_vectors(vdev, nvectors, per_vq_vectors);
> > -- 
> > 1.9.3
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
			Amos.

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

[Michael S. Tsirkin]

On Mon, Sep 01, 2014 at 03:58:02PM +0800, Amos Kong wrote:
> On Mon, Sep 01, 2014 at 09:37:30AM +0300, Michael S. Tsirkin wrote:
> >
> 
> Hi Michael,
> 
> > On Mon, Sep 01, 2014 at 01:41:54PM +0800, Amos Kong wrote:
> > > One VM only has 128 msix interrupt, virtio-config interrupt
> > > has less workload. This patch shares one normal interrupt
> 
> Thanks for your quick reply.
> 
> > normal == INT#x? Please don't call it normal. The proper name is
> > "legacy INT#x".
>  
> OK
> 
> > So you are trying to use legacy INT#x at the same time
> > with MSI-X?  This does not work: the PCI spec says:
> > 	While enabled for MSI or MSI-X
> > 	operation, a function is prohibited from using its INTx# pin (if
> > 	implemented) to request
> > 	service (MSI, MSI-X, and INTx# are mutually exclusive).
> 
> It means we can't use interrupt and triggered-interrupt together for
> one PCI device. I will study this problem.
>  
> > does the patch work for you? If it does it might be a (minor) spec
> > violation in kvm.
> 
> I did some basic testing (multiple nics, scp, ping, etc), it works.

It's quite likely there were no config interrupts in your
basic test. Trigger some event that requires a config interrupt
to function. For example, drop link and see if guest notices this.

> > Besides, INT#x really leads to terrible performance because
> > sharing is forced even if there aren't many devices.
> > 
> > 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.
>  
> I have a draft patch to share one MSI-X for all virtio-config, it has
> some problem in hotplugging devices. I will continue this way.
>  
> > > for configuration between virtio devices.
> > > 
> > > Signed-off-by: Amos Kong <akong@redhat.com>
> > > ---
> > >  drivers/virtio/virtio_pci.c | 41 ++++++++++++++++-------------------------
> > >  1 file changed, 16 insertions(+), 25 deletions(-)
> > > 
> > > diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> > > index 3d1463c..b1263b3 100644
> > > --- a/drivers/virtio/virtio_pci.c
> > > +++ b/drivers/virtio/virtio_pci.c
> > > @@ -52,6 +52,7 @@ struct virtio_pci_device
> > >  	/* Name strings for interrupts. This size should be enough,
> > >  	 * and I'm too lazy to allocate each name separately. */
> > >  	char (*msix_names)[256];
> > > +	char config_msix_name[256];
> > >  	/* Number of available vectors */
> > >  	unsigned msix_vectors;
> > >  	/* Vectors allocated, excluding per-vq vectors if any */
> > > @@ -282,12 +283,6 @@ static void vp_free_vectors(struct virtio_device *vdev)
> > >  			free_cpumask_var(vp_dev->msix_affinity_masks[i]);
> > >  
> > >  	if (vp_dev->msix_enabled) {
> > > -		/* Disable the vector used for configuration */
> > > -		iowrite16(VIRTIO_MSI_NO_VECTOR,
> > > -			  vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
> > > -		/* Flush the write out to device */
> > > -		ioread16(vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
> > > -
> > >  		pci_disable_msix(vp_dev->pci_dev);
> > >  		vp_dev->msix_enabled = 0;
> > >  	}
> > > @@ -339,24 +334,18 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
> > >  		goto error;
> > >  	vp_dev->msix_enabled = 1;
> > >  
> > > -	/* Set the vector used for configuration */
> > > -	v = vp_dev->msix_used_vectors;
> > > -	snprintf(vp_dev->msix_names[v], sizeof *vp_dev->msix_names,
> > > +	/* Set shared IRQ for configuration */
> > > +	snprintf(vp_dev->config_msix_name, sizeof(*vp_dev->msix_names),
> > >  		 "%s-config", name);
> > > -	err = request_irq(vp_dev->msix_entries[v].vector,
> > > -			  vp_config_changed, 0, vp_dev->msix_names[v],
> > > +	err = request_irq(vp_dev->pci_dev->irq,
> > > +			  vp_config_changed,
> > > +			  IRQF_SHARED,
> > > +			  vp_dev->config_msix_name,
> > >  			  vp_dev);
> > > -	if (err)
> > > -		goto error;
> > > -	++vp_dev->msix_used_vectors;
> > > -
> > > -	iowrite16(v, vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
> > > -	/* Verify we had enough resources to assign the vector */
> > > -	v = ioread16(vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
> > > -	if (v == VIRTIO_MSI_NO_VECTOR) {
> > > -		err = -EBUSY;
> > > +	if (!err)
> > > +		vp_dev->intx_enabled = 1;
> > > +	else
> > >  		goto error;
> > > -	}
> > >  
> > >  	if (!per_vq_vectors) {
> > >  		/* Shared vector for all VQs */
> > > @@ -535,14 +524,16 @@ static int vp_try_to_find_vqs(struct virtio_device *vdev, unsigned nvqs,
> > >  			goto error_request;
> > >  	} else {
> > >  		if (per_vq_vectors) {
> > > -			/* Best option: one for change interrupt, one per vq. */
> > > -			nvectors = 1;
> > > +			/* Best option: one normal interrupt for change,
> > > +			   one msix per vq. */
> > > +			nvectors = 0;
> > >  			for (i = 0; i < nvqs; ++i)
> > >  				if (callbacks[i])
> > >  					++nvectors;
> > >  		} else {
> > > -			/* Second best: one for change, shared for all vqs. */
> > > -			nvectors = 2;
> > > +			/* Second best: one normal interrupt for
> > > +			   change, share one msix for all vqs. */
> > > +			nvectors = 1;
> > >  		}
> > >  
> > >  		err = vp_request_msix_vectors(vdev, nvectors, per_vq_vectors);
> > > -- 
> > > 1.9.3
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> -- 
> 			Amos.

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

[Stefan Fritsch]

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.

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

[Michael S. Tsirkin]

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

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

[Stefan Fritsch]

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.

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

[Michael S. Tsirkin]

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

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

[Stefan Fritsch]

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.

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

[Sasha Levin]

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

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

[Michael S. Tsirkin]

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

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

[Sasha Levin]

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

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

[Michael S. Tsirkin]

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