Re: [PATCH 1/2] virito: introduce methods of fixing device features
On Thu, 13 Nov 2014 13:52:53 +0800
Jason Wang jasow...@redhat.com wrote:
typo in subject-prefix: s/virito/virtio/
Buggy host may advertised buggy host features (a usual case is that host
advertise a feature whose dependencies were missed). In this case, driver
should detect and disable the buggy features by itself.
This patch introduces driver specific fix_features() method which is called
just before features finalizing to detect and disable buggy features
advertised by host.
So the basic problem this patch fixes is that an individual driver may
only specify a static set of features but cannot specify any
dependencies, right? Adding a sanitizer step makes sense, I guess.
Virtio-net will be the first user.
Cc: Rusty Russell ru...@rustcorp.com.au
Cc: Michael S. Tsirkin m...@redhat.com
Signed-off-by: Jason Wang jasow...@redhat.com
---
drivers/virtio/virtio.c | 4
include/linux/virtio.h| 1 +
include/linux/virtio_config.h | 12
3 files changed, 17 insertions(+)
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index df598dd..7001d6e 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -181,6 +181,10 @@ static int virtio_dev_probe(struct device *_d)
if (device_features (1 i))
set_bit(i, dev-features);
+ /* Fix buggy features advertised by host */
+ if (drv-fix_features)
+ drv-fix_features(dev);
I'd probably call this sanitize_features instead.
+
dev-config-finalize_features(dev);
err = drv-probe(dev);
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index 7f4ef66..7bd89ea 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -96,6 +96,18 @@ static inline bool virtio_has_feature(const struct
virtio_device *vdev,
return test_bit(fbit, vdev-features);
}
+static inline void virtio_disable_feature(struct virtio_device *vdev,
+ unsigned int fbit)
+{
+ BUG_ON(fbit = VIRTIO_TRANSPORT_F_START);
+ BUG_ON(vdev-config-get_status(vdev)
+~(VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER));
When we add virtio-1 support, we can add a check for FEATURES_OK here,
so we're really on the safe side.
+
+ virtio_check_driver_offered_feature(vdev, fbit);
+
+ clear_bit(fbit, vdev-features);
+}
+
static inline
struct virtqueue *virtio_find_single_vq(struct virtio_device *vdev,
vq_callback_t *c, const char *n)
The approach looks good to me.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] virtio-net: fix buggy features advertised by host
On Thu, 13 Nov 2014 13:52:54 +0800
Jason Wang jasow...@redhat.com wrote:
This patch tries to detect the possible buggy features advertised by host
and fix them. One example is booting virtio-net with only ctrl_vq disabled,
qemu may still advertise many features which depends on it. This will
trigger several BUG()s in virtnet_send_command().
This patch utilizes the fix_features() method, and disables all features that
depends on ctrl_vq if it was not advertised.
This fixes the crash when booting with ctrl_vq=off.
That's a qemu device property, right? Might want to mention that, as
this line sounds like it is a kernel parameter.
Cc: Rusty Russell ru...@rustcorp.com.au
Cc: Michael S. Tsirkin m...@redhat.com
Signed-off-by: Jason Wang jasow...@redhat.com
---
Changes from V1:
- fix the cut-and-paste error
---
drivers/net/virtio_net.c | 35 +++
1 file changed, 35 insertions(+)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index ec2a8b4..6ce125e 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1948,6 +1948,40 @@ static int virtnet_restore(struct virtio_device *vdev)
}
#endif
+static void virtnet_fix_features(struct virtio_device *dev)
+{
+ if (!virtio_has_feature(dev, VIRTIO_NET_F_CTRL_VQ)) {
+ if (virtio_has_feature(dev, VIRTIO_NET_F_CTRL_RX)) {
+ pr_warning(Disable VIRTIO_NET_F_CTRL_RX since host
+does not advertise VIRTIO_NET_F_CTRL_VQ);
+ virtio_disable_feature(dev, VIRTIO_NET_F_CTRL_RX);
+ }
You should probably use dev_warn() or so, so that the user can figure
out which device the message is for. And perhaps add buggy hypervisor
to the message to make clear that it's not a guest problem.
I also like the suggestion to use a dependency array.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] virito: introduce methods of fixing device features
On 11/13/2014 04:46 PM, Cornelia Huck wrote:
On Thu, 13 Nov 2014 13:52:53 +0800
Jason Wang jasow...@redhat.com wrote:
typo in subject-prefix: s/virito/virtio/
Will correct this.
Buggy host may advertised buggy host features (a usual case is that host
advertise a feature whose dependencies were missed). In this case, driver
should detect and disable the buggy features by itself.
This patch introduces driver specific fix_features() method which is called
just before features finalizing to detect and disable buggy features
advertised by host.
So the basic problem this patch fixes is that an individual driver may
only specify a static set of features but cannot specify any
dependencies, right?
Right, and what even worse is qemu could not handle dependencies as
well. So we need fix both sides.
Adding a sanitizer step makes sense, I guess.
Virtio-net will be the first user.
Cc: Rusty Russell ru...@rustcorp.com.au
Cc: Michael S. Tsirkin m...@redhat.com
Signed-off-by: Jason Wang jasow...@redhat.com
---
drivers/virtio/virtio.c | 4
include/linux/virtio.h| 1 +
include/linux/virtio_config.h | 12
3 files changed, 17 insertions(+)
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index df598dd..7001d6e 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -181,6 +181,10 @@ static int virtio_dev_probe(struct device *_d)
if (device_features (1 i))
set_bit(i, dev-features);
+/* Fix buggy features advertised by host */
+if (drv-fix_features)
+drv-fix_features(dev);
I'd probably call this sanitize_features instead.
Ok.
+
dev-config-finalize_features(dev);
err = drv-probe(dev);
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index 7f4ef66..7bd89ea 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -96,6 +96,18 @@ static inline bool virtio_has_feature(const struct
virtio_device *vdev,
return test_bit(fbit, vdev-features);
}
+static inline void virtio_disable_feature(struct virtio_device *vdev,
+ unsigned int fbit)
+{
+BUG_ON(fbit = VIRTIO_TRANSPORT_F_START);
+BUG_ON(vdev-config-get_status(vdev)
+ ~(VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER));
When we add virtio-1 support, we can add a check for FEATURES_OK here,
so we're really on the safe side.
If I read the spec correctly, FEATURES_OK was set only after writing the
features bits to device. But we want to sanitize the them before.
+
+virtio_check_driver_offered_feature(vdev, fbit);
+
+clear_bit(fbit, vdev-features);
+}
+
static inline
struct virtqueue *virtio_find_single_vq(struct virtio_device *vdev,
vq_callback_t *c, const char *n)
The approach looks good to me.
Thanks for the review.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] virtio-net: fix buggy features advertised by host
On 11/13/2014 04:53 PM, Cornelia Huck wrote:
On Thu, 13 Nov 2014 13:52:54 +0800
Jason Wang jasow...@redhat.com wrote:
This patch tries to detect the possible buggy features advertised by host
and fix them. One example is booting virtio-net with only ctrl_vq disabled,
qemu may still advertise many features which depends on it. This will
trigger several BUG()s in virtnet_send_command().
This patch utilizes the fix_features() method, and disables all features that
depends on ctrl_vq if it was not advertised.
This fixes the crash when booting with ctrl_vq=off.
That's a qemu device property, right? Might want to mention that, as
this line sounds like it is a kernel parameter.
Right, ok.
Cc: Rusty Russell ru...@rustcorp.com.au
Cc: Michael S. Tsirkin m...@redhat.com
Signed-off-by: Jason Wang jasow...@redhat.com
---
Changes from V1:
- fix the cut-and-paste error
---
drivers/net/virtio_net.c | 35 +++
1 file changed, 35 insertions(+)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index ec2a8b4..6ce125e 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1948,6 +1948,40 @@ static int virtnet_restore(struct virtio_device *vdev)
}
#endif
+static void virtnet_fix_features(struct virtio_device *dev)
+{
+if (!virtio_has_feature(dev, VIRTIO_NET_F_CTRL_VQ)) {
+if (virtio_has_feature(dev, VIRTIO_NET_F_CTRL_RX)) {
+pr_warning(Disable VIRTIO_NET_F_CTRL_RX since host
+ does not advertise VIRTIO_NET_F_CTRL_VQ);
+virtio_disable_feature(dev, VIRTIO_NET_F_CTRL_RX);
+}
You should probably use dev_warn() or so, so that the user can figure
out which device the message is for. And perhaps add buggy hypervisor
to the message to make clear that it's not a guest problem.
Ok.
I also like the suggestion to use a dependency array.
Yes, will do it in next version.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] virito: introduce methods of fixing device features
On Thu, 13 Nov 2014 17:11:30 +0800
Jason Wang jasow...@redhat.com wrote:
On 11/13/2014 04:46 PM, Cornelia Huck wrote:
On Thu, 13 Nov 2014 13:52:53 +0800
Jason Wang jasow...@redhat.com wrote:
+static inline void virtio_disable_feature(struct virtio_device *vdev,
+ unsigned int fbit)
+{
+ BUG_ON(fbit = VIRTIO_TRANSPORT_F_START);
+ BUG_ON(vdev-config-get_status(vdev)
+ ~(VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER));
When we add virtio-1 support, we can add a check for FEATURES_OK here,
so we're really on the safe side.
If I read the spec correctly, FEATURES_OK was set only after writing the
features bits to device. But we want to sanitize the them before.
I meant doing a BUG when FEATURES_OK is set - sorry for not being clear.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH] virtio-mmio: support for multiple irqs
On 2014/11/13 2:33, Pawel Moll wrote: On Wed, 2014-11-12 at 08:32 +, Shannon Zhao wrote: On 2014/11/11 23:11, Pawel Moll wrote: On Tue, 2014-11-04 at 09:35 +, Shannon Zhao wrote: As the current virtio-mmio only support single irq, so some advanced features such as vhost-net with irqfd are not supported. And the net performance is not the best without vhost-net and irqfd supporting. Could you, please, help understanding me where does the main issue is? Is it about: 1. The fact that the existing implementation blindly kicks all queues, instead only of the updated ones? or: 2. Literally having a dedicated interrupt line (remember, we're talking real interrupts here, not message signalled ones) per queue, so they can be handled by different processors at the same time? The main issue is that current virtio-mmio only support one interrupt which is shared by config and queues. Yes. Additional question: is your device changing the configuration space often? Other devices (for example block devices) change configuration very rarely (eg. change of the media size, usually meaning hot-un-plugging), so unless you tell me that the config space is frequently changes, I'll consider the config event irrelevant from the performance point of view. No, change the configuration space not often. Therefore the virtio-mmio driver should read the VIRTIO_MMIO_INTERRUPT_STATUS to get the interrupt reason and check whom this interrupt is to. Correct. If we use vhost-net which uses irqfd to inject interrupt, the vhost-net doesn't update VIRTIO_MMIO_INTERRUPT_STATUS, then the guest driver can't read the interrupt reason and doesn't call a handler to process. Ok, so this is the bit I don't understand. Explain, please how does this whole thing work. And keep in mind that I've just looked up irqfd for the first time in my life, so know nothing about its implementation. The same applies to vhost-net. I'm simply coming from a completely different background, so bear with me on this. Ok, sorry. I ignored this. When we use only virtio-mmio or vhost-net without irqfd, the device uses qemu_set_irq(within qemu) to inject interrupt and at the same time qemu update VIRTIO_MMIO_INTERRUPT_STATUS to tell guest driver whom this interrupt to. All these things are done by qemu. Though qemu_set_irq will call kvm_set_irq to do the real interrupt inject, but the trigger is qemu and it can update the VIRTIO_MMIO_INTERRUPT_STATUS register before injecting the interrupt. But if we use vhost-net with irqfd, the device uses ioeventfd mechanism to inject interrupt. When an interrupt happened, it doesn't transfer to qemu, while the irqfd finally call kvm_set_irq to inject the interrupt directly. All these things are done in kvm and it can't update the VIRTIO_MMIO_INTERRUPT_STATUS register. So if the guest driver still uses the old interrupt handler, it has to read the VIRTIO_MMIO_INTERRUPT_STATUS register to get the interrupt reason and run different handlers for different reasons. But the register has nothing and none of handlers will be called. I make myself clear? :-) Now, the virtio-mmio device *must* behave in a certain way, as specified in both the old and the new version of the spec. If a Used Ring of *any* of the queues has been updated the device *must* set bit 0 of the interrupt status register, and - in effect - generate the interrupt. But you are telling me that in some circumstances the interrupt is *not* generated when a queue requires handling. Sounds like a bug to me, as it is not following the requirements of the device specification. It is quite likely that I simply don't see some facts which are obvious for you, so please - help me understand. So we can assign a dedicated interrupt line per queue for virtio-mmio and it can work with irqfd. If my reasoning above is correct, I'd say that you are just working around a functional bug, not improving performance. And this is a wrong way of solving the problem. Theoretically the number of interrupts has no limit, but as the limit of ARM interrupt line, the number should be less than ARM interrupt lines. Let me just emphasize the fact that virtio-mmio is not related to ARM in any way, it's just a memory mapped device with a generic interrupt output. Any limitations of a particular hardware platform (I guess you're referring to the maximum number of peripheral interrupts a GIC can take) are irrelevant - if we go with multiple interrupts, it must cater for as many interrupt lines as one wishes... :-) In the real situation, I think, the number is generally less than 17 (8 pairs of vring interrupts and one config interrupt). ... but of course we could optimize for the real scenarios. And I'm glad to hear that the number you quoted is less then 30 :-) we use GSI for multiple irq. I'm not sure what GSI stands for, but looking at the code I assume it's just a normal
Re: [PATCH 1/2] virito: introduce methods of fixing device features
On 11/13/2014 05:14 PM, Cornelia Huck wrote:
On Thu, 13 Nov 2014 17:11:30 +0800
Jason Wang jasow...@redhat.com wrote:
On 11/13/2014 04:46 PM, Cornelia Huck wrote:
On Thu, 13 Nov 2014 13:52:53 +0800
Jason Wang jasow...@redhat.com wrote:
+static inline void virtio_disable_feature(struct virtio_device *vdev,
+ unsigned int fbit)
+{
+ BUG_ON(fbit = VIRTIO_TRANSPORT_F_START);
+ BUG_ON(vdev-config-get_status(vdev)
+ ~(VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER));
When we add virtio-1 support, we can add a check for FEATURES_OK here,
so we're really on the safe side.
If I read the spec correctly, FEATURES_OK was set only after writing the
features bits to device. But we want to sanitize the them before.
I meant doing a BUG when FEATURES_OK is set - sorry for not being clear.
I get it, thanks for the clarification.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: kernel BUG at drivers/block/virtio_blk.c:172!
On 12.11.2014 11:18, Jens Axboe wrote: On 11/11/2014 09:42 AM, Ming Lei wrote: The attached patch should fix the problem, and hope it is the last one, :-) Dongsu and Jeff, any of you test this variant? I think this is the last one, at least I hope so as well... Yes, I've just tested it again with the Ming's patch. It passed a full cycle of xfstests: No crash, no particular regression. The code in blk_recount_segments() seems to make sense too. Thanks! ;-) Dongsu ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net] Revert drivers/net: Disable UFO through virtio in macvtap and tun
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 12/11/14 20:02, Stefan Hajnoczi wrote: On Tue, Nov 11, 2014 at 05:12:58PM +, Ben Hutchings wrote: This reverts commit 88e0e0e5aa722b193c8758c8b45d041de5316924 for the tap drivers, but leaves UFO disabled in virtio_net. libvirt at least assumes that tap features will never be dropped in new kernel versions, and doing so prevents migration of VMs to the never kernel version while they are running with virtio net devices. Fixes: 88e0e0e5aa7a (drivers/net: Disable UFO through virtio) Signed-off-by: Ben Hutchings b...@decadent.org.uk --- Compile-tested only. Jelle, care to test this and reply with Tested-by: Jelle de Jong jelledej...@powercraft.nl if it solves the live migration problem you reported? It requires applying the patch to the host kernel on your virt01 host. If someone can provide an x86_64 kernel package for debian wheezy (or put it in wheezy-backports I would be willing to test it and report (I cant set-up a cross-build building system right now). I'm currently planning on taking four virtualisation platform down this weekend to reboot all systems and guests to work around the issue that already hit production. Many thanks for picking up the problem and working on a solution. Kind regards, Jelle de Jong -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.12 (GNU/Linux) iJwEAQECAAYFAlRknWIACgkQ1WclBW9j5HlBLwP/aMoJy9Jgs6M6nuQXtWOPZZ12 N30le4kj+s6AP7Bt5j9vDjJf1/B0bdKbykEvUFlW0xbrD7YGFZm0HflM0lqwZ6lZ lql9mqrcooumYZuDt/CxBHsUlRIbiR/Bzd4qdkCkpxHo7aXLRk0HyPsK4XG6OulN 4MVHRR8W8Yk87FBDLCw= =DwU5 -END PGP SIGNATURE- ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PULL] vhost: cleanups and fixes
The following changes since commit 206c5f60a3d902bc4b56dab2de3e88de5eb06108: Linux 3.18-rc4 (2014-11-09 14:55:29 -0800) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git tags/for_linus for you to fetch changes up to 65eca3a20264a8999570c269406196bd1ae23be7: virtio_console: move early VQ enablement (2014-11-13 09:53:26 +0200) It seems like a good idea to merge this bugfix now, as it's clearly a regression and several people complained. virtio: bugfix for 3.18 This fixes a crash in virtio console multi-channel mode that got introduced in -rc1. Signed-off-by: Michael S. Tsirkin m...@redhat.com Cornelia Huck (1): virtio_console: move early VQ enablement drivers/char/virtio_console.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 52/56] drivers/char/virtio: support compiling out splice
On Thu, Nov 13, 2014 at 10:23:29PM +0100, Pieter Smith wrote:
Compile out splice support from virtio character driver when the splice-family
of syscalls is not supported by the system (i.e. CONFIG_SYSCALL_SPLICE is
undefined).
Signed-off-by: Pieter Smith pie...@boesman.nl
---
drivers/char/virtio_console.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index b585b47..de5e2cb 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -870,6 +870,7 @@ struct sg_list {
struct scatterlist *sg;
};
+#ifdef CONFIG_SYSCALL_SPLICE
static int pipe_to_sg(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
struct splice_desc *sd)
{
@@ -976,6 +977,7 @@ error_out:
pipe_unlock(pipe);
return ret;
}
+#endif /* #ifdef CONFIG_SYSCALL_SPLICE */
Not worth the #ifdef mess.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
