Re: virtio balloon: do not call blocking ops when !TASK_RUNNING
Cornelia Huck writes: > On Wed, 4 Mar 2015 11:25:56 +0100 > "Michael S. Tsirkin" wrote: > >> On Wed, Mar 04, 2015 at 04:44:54PM +1030, Rusty Russell wrote: >> > "Michael S. Tsirkin" writes: >> > > On Mon, Mar 02, 2015 at 10:37:26AM +1030, Rusty Russell wrote: >> > >> Thomas Huth writes: >> > >> > On Thu, 26 Feb 2015 11:50:42 +1030 >> > >> > Rusty Russell wrote: >> > >> > >> > >> >> Thomas Huth writes: >> > >> >> > Hi all, >> > >> >> > >> > >> >> > with the recent kernel 3.19, I get a kernel warning when I start my >> > >> >> > KVM guest on s390 with virtio balloon enabled: >> > >> >> >> > >> >> The deeper problem is that virtio_ccw_get_config just silently fails >> > >> >> on >> > >> >> OOM. >> > >> >> >> > >> >> Neither get_config nor set_config are expected to fail. >> > >> > >> > >> > AFAIK this is currently not a problem. According to >> > >> > http://lwn.net/Articles/627419/ these kmalloc calls never >> > >> > fail because they allocate less than a page. >> > >> >> > >> I strongly suggest you unlearn that fact. >> > >> The fix for this is in two parts: >> > >> >> > >> 1) Annotate using sched_annotate_sleep() and add a comment: we may spin >> > >>a few times in low memory situations, but this isn't a high >> > >>performance path. >> > >> >> > >> 2) Handle get_config (and other) failure in some more elegant way. >> > >> >> > >> Cheers, >> > >> Rusty. >> > > >> > > I agree, but I'd like to point out that even without kmalloc, >> > > on s390 get_config is blocking - it's waiting >> > > for a hardware interrupt. >> > > >> > > And it makes sense: config is not data path, I don't think >> > > we should spin there. >> > > >> > > So I think besides these two parts, we still need my two patches: >> > > virtio-balloon: do not call blocking ops when !TASK_RUNNING >> > >> > I prefer to annotate, over trying to fix this. >> > >> > Because it's not important. We might spin a few times, but it's very >> > unlikely, and it's certainly not performance critical. >> > >> > Thanks, >> > Rusty. >> > >> > Subject: virtio_balloon: annotate possible sleep waiting for event. >> > >> > CCW (s390) does this. >> > >> > Reported-by: Thomas Huth >> > Signed-off-by: Rusty Russell >> > >> > diff --git a/drivers/virtio/virtio_balloon.c >> > b/drivers/virtio/virtio_balloon.c >> > index 0413157f3b49..3f4d5acdbde0 100644 >> > --- a/drivers/virtio/virtio_balloon.c >> > +++ b/drivers/virtio/virtio_balloon.c >> > @@ -340,6 +340,15 @@ static int balloon(void *_vballoon) >> >s64 diff; >> > >> >try_to_freeze(); >> > + >> > + /* >> > + * Reading the config on the ccw backend involves an >> > + * allocation, so we may actually sleep and have an >> > + * extra iteration. It's extremely unlikely, >> >> Hmm, this part of the comment seems wrong to me. >> Reading the config on the ccw backend always sleeps >> because it's interrupt driven. > > (...) > >> So I suspect >> http://mid.gmane.org/1424874878-17155-1-git-send-email-...@redhat.com >> is better. >> >> What do you think? > > I'd prefer to fix this as well. While the I/O request completes > instantly on current qemu (the ssch backend handles the start function > immediately, not asynchronously as on real hardware), this (a) is an > implementation detail that may change and (b) doesn't account for the > need to deliver the interrupt to the guest - which might take non-zero > time. Ah, I see. My mistake. I've thrown out my patch, applied that one. Thanks, Rusty. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2] virtio-balloon: do not call blocking ops when !TASK_RUNNING
Thomas Huth writes: > On Wed, 25 Feb 2015 16:11:27 +0100 > Cornelia Huck wrote: > >> On Wed, 25 Feb 2015 15:36:02 +0100 >> "Michael S. Tsirkin" wrote: >> >> > virtio balloon has this code: >> > wait_event_interruptible(vb->config_change, >> > (diff = towards_target(vb)) != 0 >> > || vb->need_stats_update >> > || kthread_should_stop() >> > || freezing(current)); >> > >> > Which is a problem because towards_target() call might block after >> > wait_event_interruptible sets task state to TAST_INTERRUPTIBLE, causing >> > the task_struct::state collision typical of nesting of sleeping >> > primitives >> > >> > See also http://lwn.net/Articles/628628/ or Thomas's >> > bug report >> > http://article.gmane.org/gmane.linux.kernel.virtualization/24846 >> > for a fuller explanation. >> > >> > To fix, rewrite using wait_woken. >> > >> > Cc: sta...@vger.kernel.org >> > Reported-by: Thomas Huth >> > Signed-off-by: Michael S. Tsirkin >> > --- >> > >> > changes from v1: >> >remove wait_event_interruptible >> >noticed by Cornelia Huck >> > >> > drivers/virtio/virtio_balloon.c | 19 ++- >> > 1 file changed, 14 insertions(+), 5 deletions(-) >> > >> >> I was able to reproduce Thomas' original problem and can confirm that >> it is gone with this patch. >> >> Reviewed-by: Cornelia Huck > > Right, I just applied the patch on my system, too, and the problem is > indeed gone! Thanks for the quick fix! > > Tested-by: Thomas Huth Applied. Thanks, Rusty. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] 9p/trans_virtio: fix hot-unplug
On device hot-unplug, 9p/virtio currently will kfree channel while
it might still be in use.
Of course, it might stay used forever, so it's an extremely ugly hack,
but it seems better than use-after-free that we have now.
Signed-off-by: Michael S. Tsirkin
---
net/9p/trans_virtio.c | 25 +
1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
index d8e376a..d1b2f306 100644
--- a/net/9p/trans_virtio.c
+++ b/net/9p/trans_virtio.c
@@ -658,14 +658,31 @@ p9_virtio_create(struct p9_client *client, const char
*devname, char *args)
static void p9_virtio_remove(struct virtio_device *vdev)
{
struct virtio_chan *chan = vdev->priv;
-
- if (chan->inuse)
- p9_virtio_close(chan->client);
- vdev->config->del_vqs(vdev);
+ unsigned long warning_time;
+ bool inuse;
mutex_lock(&virtio_9p_lock);
+
+ /* Remove self from list so we don't get new users. */
list_del(&chan->chan_list);
+ warning_time = jiffies;
+
+ /* Wait for existing users to close. */
+ while (chan->inuse) {
+ mutex_unlock(&virtio_9p_lock);
+ msleep(250);
+ if (time_after(jiffies, warning_time + 10 * HZ)) {
+ dev_emerg(&vdev->dev, "p9_virtio_remove: "
+ "waiting for device in use.\n");
+ warning_time = jiffies;
+ }
+ mutex_lock(&virtio_9p_lock);
+ }
+
mutex_unlock(&virtio_9p_lock);
+
+ vdev->config->del_vqs(vdev);
+
sysfs_remove_file(&(vdev->dev.kobj), &dev_attr_mount_tag.attr);
kobject_uevent(&(vdev->dev.kobj), KOBJ_CHANGE);
kfree(chan->tag);
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: virtio fixes pull for 4.0?
On Mon, 09 Mar 2015 17:43:19 +1030 Rusty Russell wrote: > "Michael S. Tsirkin" writes: > > virtio-balloon: do not call blocking ops when !TASK_RUNNING > > > > Rusty, it seems you prefer a different fix for this issue, > > while Cornelia prefers mine. Maybe both me and Cornelia misunderstand the > > issue? I know you dealt with a ton of similar issues recently > > so you are more likely to be right, but I'd like to understand > > what's going on better all the same. Could you help please? > > In the longer run, we should handle failures from these callbacks. But > we don't need to do that right now. So we want the minimal fix. > > And an annotation is the minimal fix. The bug has been there for ages; > it's just the warning that is new (if we *always* slept, we would > spin, but in practice we'll rarely sleep). I don't think doing_io() in virtio-ccw will sleep often, although it might happen under loads that delay posting the interrupt. I understand why you'd like to do the minimal fix, and it's not likely that new problems start popping up now. So I'm OK with doing the annotation for 4.0 and more involved changes for 4.1. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio_rpmsg: set DRIVER_OK before using device
On Sat, Mar 07, 2015 at 08:06:56PM +0100, Michael S. Tsirkin wrote: > virtio spec requires that all drivers set DRIVER_OK > before using devices. While rpmsg isn't yet > included in the virtio 1 spec, previous spec versions > also required this. > > virtio rpmsg violates this rule: is calls kick > before setting DRIVER_OK. > > The fix isn't trivial since simply calling virtio_device_ready earlier > would mean we might get an interrupt in parallel with adding buffers. > > Instead, split kick out to prepare+notify calls. prepare before > virtio_device_ready - when we know we won't get interrupts. notify right > afterwards. > > Signed-off-by: Michael S. Tsirkin > --- Ohad, can you review and ack pls? > Note: compile-tested only. > > drivers/rpmsg/virtio_rpmsg_bus.c | 17 - > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c > b/drivers/rpmsg/virtio_rpmsg_bus.c > index 92f6af6..73354ee 100644 > --- a/drivers/rpmsg/virtio_rpmsg_bus.c > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c > @@ -951,6 +951,7 @@ static int rpmsg_probe(struct virtio_device *vdev) > void *bufs_va; > int err = 0, i; > size_t total_buf_space; > + bool notify; > > vrp = kzalloc(sizeof(*vrp), GFP_KERNEL); > if (!vrp) > @@ -1030,8 +1031,22 @@ static int rpmsg_probe(struct virtio_device *vdev) > } > } > > + /* > + * Prepare to kick but don't notify yet - we can't do this before > + * device is ready. > + */ > + notify = virtqueue_kick_prepare(vrp->rvq); > + > + /* From this point on, we can notify and get callbacks. */ > + virtio_device_ready(vdev); > + > /* tell the remote processor it can start sending messages */ > - virtqueue_kick(vrp->rvq); > + /* > + * this might be concurrent with callbacks, but we are only > + * doing notify, not a full kick here, so that's ok. > + */ > + if (notify) > + virtqueue_notify(vrp->rvq); > > dev_info(&vdev->dev, "rpmsg host is online\n"); > > -- > MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio_mmio: generation support
On Thu, Mar 05, 2015 at 11:09:42PM +0100, Michael S. Tsirkin wrote:
> virtio_mmio currently lacks generation support which
> makes multi-byte field access racy.
> Fix by getting the value at offset 0xfc for version 2
> devices. Nothing we can do for version 1, so return
> generation id 0.
>
> Signed-off-by: Michael S. Tsirkin
> ---
>
> Pawel, you mentioned you have a working virtio 1.0
> hypervisor, can you pls confirm this works correctly?
Same here - can you ack pls?
> drivers/virtio/virtio_mmio.c | 12
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index 0375456..69b2e4d 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -237,6 +237,16 @@ static void vm_set(struct virtio_device *vdev, unsigned
> offset,
> }
> }
>
> +static u32 vm_generation(struct virtio_device *vdev)
> +{
> + struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
> +
> + if (vm_dev->version == 1)
> + return 0;
> + else
> + return readl(vm_dev->base + VIRTIO_MMIO_CONFIG_GENERATION);
> +}
> +
> static u8 vm_get_status(struct virtio_device *vdev)
> {
> struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
> @@ -503,6 +513,8 @@ static const char *vm_bus_name(struct virtio_device *vdev)
> static const struct virtio_config_ops virtio_mmio_config_ops = {
> .get= vm_get,
> .set= vm_set,
> + .generation = vm_generation,
> + .get_status = vm_get_status,
> .get_status = vm_get_status,
> .set_status = vm_set_status,
> .reset = vm_reset,
> --
> MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio_mmio: fix endian-ness for mmio
On Thu, Mar 05, 2015 at 10:54:31PM +0100, Michael S. Tsirkin wrote:
> Going over the virtio mmio code, I noticed that it doesn't correctly
> return device config values in LE format when using virtio 1.0.
> Borrow code from virtio_pci_modern to do this correctly.
>
> Signed-off-by: Michael S. Tsirkin
Pawel, could you review and ack please?
It'd be unfortunate if we released a version
with incorrect endian-ness.
> ---
>
> Note: untested: QEMU doesn't support virtio 1.0 for virtio-mmio.
> Pawel, could you please confirm this patch makes sense?
>
> drivers/virtio/virtio_mmio.c | 79
> +++-
> 1 file changed, 71 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index cad5698..0375456 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -156,22 +156,85 @@ static void vm_get(struct virtio_device *vdev, unsigned
> offset,
> void *buf, unsigned len)
> {
> struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
> - u8 *ptr = buf;
> - int i;
> + void __iomem *base = vm_dev->base + VIRTIO_MMIO_CONFIG;
> + u8 b;
> + __le16 w;
> + __le32 l;
>
> - for (i = 0; i < len; i++)
> - ptr[i] = readb(vm_dev->base + VIRTIO_MMIO_CONFIG + offset + i);
> + if (vm_dev->version == 1) {
> + u8 *ptr = buf;
> + int i;
> +
> + for (i = 0; i < len; i++)
> + ptr[i] = readb(base + offset + i);
> + return;
> + }
> +
> + switch (len) {
> + case 1:
> + b = readb(base + offset);
> + memcpy(buf, &b, sizeof b);
> + break;
> + case 2:
> + w = cpu_to_le16(readw(base + offset));
> + memcpy(buf, &w, sizeof w);
> + break;
> + case 4:
> + l = cpu_to_le32(readl(base + offset));
> + memcpy(buf, &l, sizeof l);
> + break;
> + case 8:
> + l = cpu_to_le32(readl(base + offset));
> + memcpy(buf, &l, sizeof l);
> + l = cpu_to_le32(ioread32(base + offset + sizeof l));
> + memcpy(buf + sizeof l, &l, sizeof l);
> + break;
> + default:
> + BUG();
> + }
> }
>
> static void vm_set(struct virtio_device *vdev, unsigned offset,
> const void *buf, unsigned len)
> {
> struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
> - const u8 *ptr = buf;
> - int i;
> + void __iomem *base = vm_dev->base + VIRTIO_MMIO_CONFIG;
> + u8 b;
> + __le16 w;
> + __le32 l;
>
> - for (i = 0; i < len; i++)
> - writeb(ptr[i], vm_dev->base + VIRTIO_MMIO_CONFIG + offset + i);
> + if (vm_dev->version == 1) {
> + const u8 *ptr = buf;
> + int i;
> +
> + for (i = 0; i < len; i++)
> + writeb(ptr[i], base + offset + i);
> +
> + return;
> + }
> +
> + switch (len) {
> + case 1:
> + memcpy(&b, buf, sizeof b);
> + writeb(b, base + offset);
> + break;
> + case 2:
> + memcpy(&w, buf, sizeof w);
> + writew(le16_to_cpu(w), base + offset);
> + break;
> + case 4:
> + memcpy(&l, buf, sizeof l);
> + writel(le32_to_cpu(l), base + offset);
> + break;
> + case 8:
> + memcpy(&l, buf, sizeof l);
> + writel(le32_to_cpu(l), base + offset);
> + memcpy(&l, buf + sizeof l, sizeof l);
> + writel(le32_to_cpu(l), base + offset + sizeof l);
> + break;
> + default:
> + BUG();
> + }
> }
>
> static u8 vm_get_status(struct virtio_device *vdev)
> --
> MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio_rpmsg: set DRIVER_OK before using device
On Mon, Mar 09, 2015 at 05:39:20PM +1030, Rusty Russell wrote: > "Michael S. Tsirkin" writes: > > virtio spec requires that all drivers set DRIVER_OK > > before using devices. While rpmsg isn't yet > > included in the virtio 1 spec, previous spec versions > > also required this. > > > > virtio rpmsg violates this rule: is calls kick > > before setting DRIVER_OK. > > > > The fix isn't trivial since simply calling virtio_device_ready earlier > > would mean we might get an interrupt in parallel with adding buffers. > > > > Instead, split kick out to prepare+notify calls. prepare before > > virtio_device_ready - when we know we won't get interrupts. notify right > > afterwards. > > > > Signed-off-by: Michael S. Tsirkin > > Applied. I'll wait for Ohad to ack before sending to Linus. > > BTW I assume you have a version of qemu which warns on these kind of > failures? That'd be nice to have! > > Thanks, > Rusty. Yes but it's hacky ATM - it's just a one-liner that checks the status on kick and does fprintf(stderr). I'm trying to rework the code so it'll actually set the status automatically, but there are some difficulties there when ioeventfd is used, in particular we need a way to re-inject the kick after status update. > > --- > > > > Note: compile-tested only. > > > > drivers/rpmsg/virtio_rpmsg_bus.c | 17 - > > 1 file changed, 16 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c > > b/drivers/rpmsg/virtio_rpmsg_bus.c > > index 92f6af6..73354ee 100644 > > --- a/drivers/rpmsg/virtio_rpmsg_bus.c > > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c > > @@ -951,6 +951,7 @@ static int rpmsg_probe(struct virtio_device *vdev) > > void *bufs_va; > > int err = 0, i; > > size_t total_buf_space; > > + bool notify; > > > > vrp = kzalloc(sizeof(*vrp), GFP_KERNEL); > > if (!vrp) > > @@ -1030,8 +1031,22 @@ static int rpmsg_probe(struct virtio_device *vdev) > > } > > } > > > > + /* > > +* Prepare to kick but don't notify yet - we can't do this before > > +* device is ready. > > +*/ > > + notify = virtqueue_kick_prepare(vrp->rvq); > > + > > + /* From this point on, we can notify and get callbacks. */ > > + virtio_device_ready(vdev); > > + > > /* tell the remote processor it can start sending messages */ > > - virtqueue_kick(vrp->rvq); > > + /* > > +* this might be concurrent with callbacks, but we are only > > +* doing notify, not a full kick here, so that's ok. > > +*/ > > + if (notify) > > + virtqueue_notify(vrp->rvq); > > > > dev_info(&vdev->dev, "rpmsg host is online\n"); > > > > -- > > MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio_rpmsg: set DRIVER_OK before using device
"Michael S. Tsirkin" writes: > virtio spec requires that all drivers set DRIVER_OK > before using devices. While rpmsg isn't yet > included in the virtio 1 spec, previous spec versions > also required this. > > virtio rpmsg violates this rule: is calls kick > before setting DRIVER_OK. > > The fix isn't trivial since simply calling virtio_device_ready earlier > would mean we might get an interrupt in parallel with adding buffers. > > Instead, split kick out to prepare+notify calls. prepare before > virtio_device_ready - when we know we won't get interrupts. notify right > afterwards. > > Signed-off-by: Michael S. Tsirkin Applied. I'll wait for Ohad to ack before sending to Linus. BTW I assume you have a version of qemu which warns on these kind of failures? That'd be nice to have! Thanks, Rusty. > --- > > Note: compile-tested only. > > drivers/rpmsg/virtio_rpmsg_bus.c | 17 - > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c > b/drivers/rpmsg/virtio_rpmsg_bus.c > index 92f6af6..73354ee 100644 > --- a/drivers/rpmsg/virtio_rpmsg_bus.c > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c > @@ -951,6 +951,7 @@ static int rpmsg_probe(struct virtio_device *vdev) > void *bufs_va; > int err = 0, i; > size_t total_buf_space; > + bool notify; > > vrp = kzalloc(sizeof(*vrp), GFP_KERNEL); > if (!vrp) > @@ -1030,8 +1031,22 @@ static int rpmsg_probe(struct virtio_device *vdev) > } > } > > + /* > + * Prepare to kick but don't notify yet - we can't do this before > + * device is ready. > + */ > + notify = virtqueue_kick_prepare(vrp->rvq); > + > + /* From this point on, we can notify and get callbacks. */ > + virtio_device_ready(vdev); > + > /* tell the remote processor it can start sending messages */ > - virtqueue_kick(vrp->rvq); > + /* > + * this might be concurrent with callbacks, but we are only > + * doing notify, not a full kick here, so that's ok. > + */ > + if (notify) > + virtqueue_notify(vrp->rvq); > > dev_info(&vdev->dev, "rpmsg host is online\n"); > > -- > MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: virtio fixes pull for 4.0?
"Michael S. Tsirkin" writes:
> Hi Rusty!
> There are a bunch of (mostly virtio 1.0 related) fixes for virtio
> that need to go into 4.0 I think.
> virtio_blk: typo fix
> virtio_blk: fix comment for virtio 1.0
OK, I've added these two. I tend to be overcautious after the merge
window.
> virtio_console: init work unconditionally
> virtio_console: avoid config access from irq
> virtio_balloon: set DRIVER_OK before using device
>
> seem ready?
These are in my virtio-next tree already.
> virtio_mmio: generation support
> virtio_mmio: fix endian-ness for mmio these two are waiting for ack by
> Pawel
>
> These two fix bugs in virtio 1.0 code for mmio.
> Host code for that was AFAIK not posted, so I can't test properly.
> Pawel?
I'm waiting on Acks for these two.
> virtio-balloon: do not call blocking ops when !TASK_RUNNING
>
> Rusty, it seems you prefer a different fix for this issue,
> while Cornelia prefers mine. Maybe both me and Cornelia misunderstand the
> issue? I know you dealt with a ton of similar issues recently
> so you are more likely to be right, but I'd like to understand
> what's going on better all the same. Could you help please?
In the longer run, we should handle failures from these callbacks. But
we don't need to do that right now. So we want the minimal fix.
And an annotation is the minimal fix. The bug has been there for ages;
it's just the warning that is new (if we *always* slept, we would
spin, but in practice we'll rarely sleep).
> virtio_rpmsg: set DRIVER_OK before using device
>
> Just posted this, but seems pretty obvious.
Yep, I've applied this too. Thanks!
> I think it's a good idea to merge these patches (maybe except the
> !TASK_RUNNING thing) sooner rather than later, to make sure people have
> the time to test the fixes properly. Would you like me to pack up (some
> of them) them up and do a pull request?
I'm waiting a bit longer, since they seem to still be tricking in.
I'm still chasing a QEMU bug, where it seems to fill in a number too
large in the 'len' field for a block device. It should be 1 byte for a
block device write, for example. See patch which causes assert() in
qemu, but I had to stop at that point (should get back tomorrow I hope).
Thanks,
Rusty.
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 882a31b..98e99b8 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -243,16 +243,21 @@ int virtio_queue_empty(VirtQueue *vq)
}
void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
-unsigned int len, unsigned int idx)
+unsigned int len_written, unsigned int idx)
{
-unsigned int offset;
+unsigned int offset, tot_wlen;
int i;
-trace_virtqueue_fill(vq, elem, len, idx);
+trace_virtqueue_fill(vq, elem, len_written, idx);
+
+for (tot_wlen = i = 0; i < elem->out_num; i++) {
+tot_wlen += elem->out_sg[i].iov_len;
+}
+assert(len_written <= tot_wlen);
offset = 0;
for (i = 0; i < elem->in_num; i++) {
-size_t size = MIN(len - offset, elem->in_sg[i].iov_len);
+size_t size = MIN(len_written - offset, elem->in_sg[i].iov_len);
cpu_physical_memory_unmap(elem->in_sg[i].iov_base,
elem->in_sg[i].iov_len,
@@ -270,7 +275,7 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement
*elem,
/* Get a pointer to the next entry in the used ring. */
vring_used_ring_id(vq, idx, elem->index);
-vring_used_ring_len(vq, idx, len);
+vring_used_ring_len(vq, idx, len_written);
}
void virtqueue_flush(VirtQueue *vq, unsigned int count)
@@ -288,9 +293,9 @@ void virtqueue_flush(VirtQueue *vq, unsigned int count)
}
void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
-unsigned int len)
+unsigned int len_written)
{
-virtqueue_fill(vq, elem, len, 0);
+virtqueue_fill(vq, elem, len_written, 0);
virtqueue_flush(vq, 1);
}
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index df09993..153374f 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -191,10 +191,10 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int
queue_size,
void virtio_del_queue(VirtIODevice *vdev, int n);
void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
-unsigned int len);
+unsigned int len_written);
void virtqueue_flush(VirtQueue *vq, unsigned int count);
void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
-unsigned int len, unsigned int idx);
+unsigned int len_written, unsigned int idx);
void virtqueue_map_sg(struct iovec *sg, hwaddr *addr,
size_t num_sg, int is_write);
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/m
Re: [PATCH] virtio_balloon: set DRIVER_OK before using device
"Michael S. Tsirkin" writes: > On Thu, Mar 05, 2015 at 01:24:47PM +1030, Rusty Russell wrote: >> "Michael S. Tsirkin" writes: >> > virtio spec requires that all drivers set DRIVER_OK >> > before using devices. While balloon isn't yet >> > included in the virtio 1 spec, previous spec versions >> > also required this. >> > >> > virtio balloon might violate this rule: probe calls >> > kthread_run before setting DRIVER_OK, which might run >> > immediately and cause balloon to inflate/deflate. >> > >> > To fix, call virtio_device_ready before running the kthread. >> > >> > Signed-off-by: Michael S. Tsirkin >> >> Replied, CC'd stable. > > Did you mean applied? I stand erected. Franks, Rusty. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
