* Re: [PATCH 1/5] virtio: move queue_index and num_free fields into core struct virtqueue.
From: Rusty Russell @ 2012-10-16 14:19 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtualization
In-Reply-To: <20121016135322.GB12972@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Wed, Oct 17, 2012 at 12:00:48AM +1030, Rusty Russell wrote:
>> * @priv: a pointer for the virtqueue implementation to use.
>> + * @index: the zero-based ordinal number for this queue.
>> + * @num_free: number of buffers we expect to be able to fit.
>
> Only it's not exactly buffers: a single buffer can use
> multiple s/g entries, right?
> Maybe clarify as 'linear buffers'?
Yes, it needs clarification, but I don't think I can do so tersely.
Here's what I ended up with:
* @num_free: number of elements we expect to be able to fit.
*
* A note on @num_free: with indirect buffers, each buffer needs one
* element in the queue, otherwise a buffer will need one element per
* sg element.
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCH 1/5] virtio: move queue_index and num_free fields into core struct virtqueue.
From: Michael S. Tsirkin @ 2012-10-16 14:34 UTC (permalink / raw)
To: Rusty Russell; +Cc: virtualization
In-Reply-To: <1350394252-23934-1-git-send-email-rusty@rustcorp.com.au>
On Wed, Oct 17, 2012 at 12:00:48AM +1030, Rusty Russell wrote:
> They're generic concepts, so hoist them. This also avoids accessor
> functions.
>
> This goes even further than Jason Wang's 17bb6d4088 patch
> ("virtio-ring: move queue_index to vring_virtqueue") which moved the
> queue_index from the specific transport.
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
ACK series (with the corrected num_free description)
> ---
> drivers/virtio/virtio_mmio.c | 4 ++--
> drivers/virtio/virtio_pci.c | 6 ++----
> drivers/virtio/virtio_ring.c | 34 +++++++++++-----------------------
> include/linux/virtio.h | 6 ++++--
> 4 files changed, 19 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index 6b1b7e1..5a0e1d3 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -225,7 +225,7 @@ static void vm_notify(struct virtqueue *vq)
>
> /* We write the queue's selector into the notification register to
> * signal the other end */
> - writel(virtqueue_get_queue_index(vq), vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY);
> + writel(vq->index, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY);
> }
>
> /* Notify all virtqueues on an interrupt. */
> @@ -266,7 +266,7 @@ static void vm_del_vq(struct virtqueue *vq)
> struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev);
> struct virtio_mmio_vq_info *info = vq->priv;
> unsigned long flags, size;
> - unsigned int index = virtqueue_get_queue_index(vq);
> + unsigned int index = vq->index;
>
> spin_lock_irqsave(&vm_dev->lock, flags);
> list_del(&info->node);
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index b59237c..e3ecc94 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -203,8 +203,7 @@ static void vp_notify(struct virtqueue *vq)
>
> /* we write the queue's selector into the notification register to
> * signal the other end */
> - iowrite16(virtqueue_get_queue_index(vq),
> - vp_dev->ioaddr + VIRTIO_PCI_QUEUE_NOTIFY);
> + iowrite16(vq->index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_NOTIFY);
> }
>
> /* Handle a configuration change: Tell driver if it wants to know. */
> @@ -479,8 +478,7 @@ static void vp_del_vq(struct virtqueue *vq)
> list_del(&info->node);
> spin_unlock_irqrestore(&vp_dev->lock, flags);
>
> - iowrite16(virtqueue_get_queue_index(vq),
> - vp_dev->ioaddr + VIRTIO_PCI_QUEUE_SEL);
> + iowrite16(vq->index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_SEL);
>
> if (vp_dev->msix_enabled) {
> iowrite16(VIRTIO_MSI_NO_VECTOR,
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index e639584..335dcec 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -93,8 +93,6 @@ struct vring_virtqueue
> /* Host publishes avail event idx */
> bool event;
>
> - /* Number of free buffers */
> - unsigned int num_free;
> /* Head of free buffer list. */
> unsigned int free_head;
> /* Number we've added since last sync. */
> @@ -106,9 +104,6 @@ struct vring_virtqueue
> /* How to notify other side. FIXME: commonalize hcalls! */
> void (*notify)(struct virtqueue *vq);
>
> - /* Index of the queue */
> - int queue_index;
> -
> #ifdef DEBUG
> /* They're supposed to lock for us. */
> unsigned int in_use;
> @@ -160,7 +155,7 @@ static int vring_add_indirect(struct vring_virtqueue *vq,
> desc[i-1].next = 0;
>
> /* We're about to use a buffer */
> - vq->num_free--;
> + vq->vq.num_free--;
>
> /* Use a single buffer which doesn't continue */
> head = vq->free_head;
> @@ -174,13 +169,6 @@ static int vring_add_indirect(struct vring_virtqueue *vq,
> return head;
> }
>
> -int virtqueue_get_queue_index(struct virtqueue *_vq)
> -{
> - struct vring_virtqueue *vq = to_vvq(_vq);
> - return vq->queue_index;
> -}
> -EXPORT_SYMBOL_GPL(virtqueue_get_queue_index);
> -
> /**
> * virtqueue_add_buf - expose buffer to other end
> * @vq: the struct virtqueue we're talking about.
> @@ -228,7 +216,7 @@ int virtqueue_add_buf(struct virtqueue *_vq,
>
> /* If the host supports indirect descriptor tables, and we have multiple
> * buffers, then go indirect. FIXME: tune this threshold */
> - if (vq->indirect && (out + in) > 1 && vq->num_free) {
> + if (vq->indirect && (out + in) > 1 && vq->vq.num_free) {
> head = vring_add_indirect(vq, sg, out, in, gfp);
> if (likely(head >= 0))
> goto add_head;
> @@ -237,9 +225,9 @@ int virtqueue_add_buf(struct virtqueue *_vq,
> BUG_ON(out + in > vq->vring.num);
> BUG_ON(out + in == 0);
>
> - if (vq->num_free < out + in) {
> + if (vq->vq.num_free < out + in) {
> pr_debug("Can't add buf len %i - avail = %i\n",
> - out + in, vq->num_free);
> + out + in, vq->vq.num_free);
> /* FIXME: for historical reasons, we force a notify here if
> * there are outgoing parts to the buffer. Presumably the
> * host should service the ring ASAP. */
> @@ -250,7 +238,7 @@ int virtqueue_add_buf(struct virtqueue *_vq,
> }
>
> /* We're about to use some buffers from the free list. */
> - vq->num_free -= out + in;
> + vq->vq.num_free -= out + in;
>
> head = vq->free_head;
> for (i = vq->free_head; out; i = vq->vring.desc[i].next, out--) {
> @@ -296,7 +284,7 @@ add_head:
> pr_debug("Added buffer head %i to %p\n", head, vq);
> END_USE(vq);
>
> - return vq->num_free;
> + return vq->vq.num_free;
> }
> EXPORT_SYMBOL_GPL(virtqueue_add_buf);
>
> @@ -393,13 +381,13 @@ static void detach_buf(struct vring_virtqueue *vq, unsigned int head)
>
> while (vq->vring.desc[i].flags & VRING_DESC_F_NEXT) {
> i = vq->vring.desc[i].next;
> - vq->num_free++;
> + vq->vq.num_free++;
> }
>
> vq->vring.desc[i].next = vq->free_head;
> vq->free_head = head;
> /* Plus final descriptor */
> - vq->num_free++;
> + vq->vq.num_free++;
> }
>
> static inline bool more_used(const struct vring_virtqueue *vq)
> @@ -599,7 +587,7 @@ void *virtqueue_detach_unused_buf(struct virtqueue *_vq)
> return buf;
> }
> /* That should have freed everything. */
> - BUG_ON(vq->num_free != vq->vring.num);
> + BUG_ON(vq->vq.num_free != vq->vring.num);
>
> END_USE(vq);
> return NULL;
> @@ -653,12 +641,13 @@ struct virtqueue *vring_new_virtqueue(unsigned int index,
> vq->vq.callback = callback;
> vq->vq.vdev = vdev;
> vq->vq.name = name;
> + vq->vq.num_free = num;
> + vq->vq.index = index;
> vq->notify = notify;
> vq->weak_barriers = weak_barriers;
> vq->broken = false;
> vq->last_used_idx = 0;
> vq->num_added = 0;
> - vq->queue_index = index;
> list_add_tail(&vq->vq.list, &vdev->vqs);
> #ifdef DEBUG
> vq->in_use = false;
> @@ -673,7 +662,6 @@ struct virtqueue *vring_new_virtqueue(unsigned int index,
> vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
>
> /* Put everything in free lists. */
> - vq->num_free = num;
> vq->free_head = 0;
> for (i = 0; i < num-1; i++) {
> vq->vring.desc[i].next = i+1;
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index 533b115..ed4c437 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -16,12 +16,16 @@
> * @name: the name of this virtqueue (mainly for debugging)
> * @vdev: the virtio device this queue was created for.
> * @priv: a pointer for the virtqueue implementation to use.
> + * @index: the zero-based ordinal number for this queue.
> + * @num_free: number of buffers we expect to be able to fit.
> */
> struct virtqueue {
> struct list_head list;
> void (*callback)(struct virtqueue *vq);
> const char *name;
> struct virtio_device *vdev;
> + unsigned int index;
> + unsigned int num_free;
> void *priv;
> };
>
> @@ -50,8 +54,6 @@ void *virtqueue_detach_unused_buf(struct virtqueue *vq);
>
> unsigned int virtqueue_get_vring_size(struct virtqueue *vq);
>
> -int virtqueue_get_queue_index(struct virtqueue *vq);
> -
> /**
> * virtio_device - representation of a device using virtio
> * @index: unique position on the virtio bus
> --
> 1.7.9.5
^ permalink raw reply
* Re: [PATCH] virtio: 9p: correctly pass physical address to userspace for high pages
From: Rusty Russell @ 2012-10-18 2:19 UTC (permalink / raw)
To: linux-kernel
Cc: Marc Zyngier, Will Deacon, lf-virt, Eric Van Hensbergen,
Sasha Levin, Andrew Morton
In-Reply-To: <1350468877-7677-1-git-send-email-will.deacon@arm.com>
Will Deacon <will.deacon@arm.com> writes:
> When using a virtio transport, the 9p net device allocates pages to back
> the descriptors inserted into the virtqueue. These allocations may be
> performed from atomic context (under the channel lock) and can therefore
> return high mappings which aren't suitable for virt_to_phys.
I had not appreciated that subtlety about GFP_ATOMIC :(
This isn't just 9p, the console, block, scsi and net devices also use
GFP_ATOMIC.
> @@ -165,7 +166,8 @@ static int vring_add_indirect(struct vring_virtqueue *vq,
> /* Use a single buffer which doesn't continue */
> head = vq->free_head;
> vq->vring.desc[head].flags = VRING_DESC_F_INDIRECT;
> - vq->vring.desc[head].addr = virt_to_phys(desc);
> + vq->vring.desc[head].addr = page_to_phys(kmap_to_page(desc)) +
> + ((unsigned long)desc & ~PAGE_MASK);
> vq->vring.desc[head].len = i * sizeof(struct vring_desc);
Gah, virt_to_phys_harder()?
What's the performance effect? If it's negligible, why doesn't
virt_to_phys() just do this for us?
We do have an alternate solution: masking out __GFP_HIGHMEM from the
kmalloc of desc. If it fails, we will fall back to laying out the
virtio request directly inside the ring; if it doesn't fit, we'll wait
for the device to consume more buffers.
> @@ -325,7 +326,7 @@ static int p9_get_mapped_pages(struct virtio_chan *chan,
> int count = nr_pages;
> while (nr_pages) {
> s = rest_of_page(data);
> - pages[index++] = virt_to_page(data);
> + pages[index++] = kmap_to_page(data);
> data += s;
> nr_pages--;
> }
This seems like a separate bug fix.
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCH] vhost-blk: Add vhost-blk support v2
From: Rusty Russell @ 2012-10-18 4:20 UTC (permalink / raw)
To: Asias He, Michael S. Tsirkin; +Cc: kvm, virtualization
In-Reply-To: <507818BE.7060404@redhat.com>
Asias He <asias@redhat.com> writes:
>>> +#define BLK_HDR 0
>>
>> What's this for, exactly? Please add a comment.
>
> The block headr is in the first and separate buffer.
Please don't assume this! We're trying to fix all the assumptions in
qemu at the moment.
vhost_net handles this correctly, taking bytes off the descriptor chain
as required.
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCH] vhost-blk: Add vhost-blk support v2
From: Michael S. Tsirkin @ 2012-10-18 6:30 UTC (permalink / raw)
To: Rusty Russell; +Cc: kvm, virtualization
In-Reply-To: <87d30go1g7.fsf@rustcorp.com.au>
On Thu, Oct 18, 2012 at 02:50:56PM +1030, Rusty Russell wrote:
> Asias He <asias@redhat.com> writes:
> >>> +#define BLK_HDR 0
> >>
> >> What's this for, exactly? Please add a comment.
> >
> > The block headr is in the first and separate buffer.
>
> Please don't assume this! We're trying to fix all the assumptions in
> qemu at the moment.
>
> vhost_net handles this correctly, taking bytes off the descriptor chain
> as required.
>
> Thanks,
> Rusty.
BTW are we agreed on the spec update that makes cmd 32 bytes?
^ permalink raw reply
* PCI device not properly reset after VFIO
From: Hannes Reinecke @ 2012-10-18 6:47 UTC (permalink / raw)
To: Alex Williamson; +Cc: kvm-devel, Linux Virtualization
Hi Alex,
I've been playing around with VFIO and megasas (of course).
What I did now was switching between VFIO and 'normal' operation, ie
emulated access.
megasas is happily running under VFIO, but when I do an emergency
stop like killing the Qemu session the PCI device is not properly reset.
IE when I load 'megaraid_sas' after unbinding the vfio_pci module
the driver cannot initialize the card and waits forever for the
firmware state to change.
I need to do a proper pci reset via
echo 1 > /sys/bus/pci/device/XXXX/reset
to get it into a working state again.
Looking at vfio_pci_disable() pci reset is called before the config
state and BARs are restored.
Seeing that vfio_pci_enable() calls pci reset right at the start,
too, before modifying anything I do wonder whether the pci reset is
at the correct location for disable.
I would have expected to call pci reset in vfio_pci_disable()
_after_ we have restored the configuration, to ensure a sane state
after reset.
And, as experience show, we do need to call it there.
So what is the rationale for the pci reset?
Can we move it to the end of vfio_pci_disable() or do we need to
call pci reset twice?
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
^ permalink raw reply
* Re: [PATCH] virtio: 9p: correctly pass physical address to userspace for high pages
From: Will Deacon @ 2012-10-18 9:42 UTC (permalink / raw)
To: Rusty Russell
Cc: Marc Zyngier, linux-kernel@vger.kernel.org, lf-virt,
Eric Van Hensbergen, Sasha Levin, Andrew Morton
In-Reply-To: <87lif4o739.fsf@rustcorp.com.au>
Hi Rusty,
On Thu, Oct 18, 2012 at 03:19:06AM +0100, Rusty Russell wrote:
> Will Deacon <will.deacon@arm.com> writes:
> > When using a virtio transport, the 9p net device allocates pages to back
> > the descriptors inserted into the virtqueue. These allocations may be
> > performed from atomic context (under the channel lock) and can therefore
> > return high mappings which aren't suitable for virt_to_phys.
>
> I had not appreciated that subtlety about GFP_ATOMIC :(
Yeah, it's unfortunate for poor old userspace.
> This isn't just 9p, the console, block, scsi and net devices also use
> GFP_ATOMIC.
Ok, I'll split this patch in two since I think that only 9p has the
zero-copy stuff, which is why an extra fix is needed there for creating the
scatterlist correctly.
> > @@ -165,7 +166,8 @@ static int vring_add_indirect(struct vring_virtqueue *vq,
> > /* Use a single buffer which doesn't continue */
> > head = vq->free_head;
> > vq->vring.desc[head].flags = VRING_DESC_F_INDIRECT;
> > - vq->vring.desc[head].addr = virt_to_phys(desc);
> > + vq->vring.desc[head].addr = page_to_phys(kmap_to_page(desc)) +
> > + ((unsigned long)desc & ~PAGE_MASK);
> > vq->vring.desc[head].len = i * sizeof(struct vring_desc);
>
> Gah, virt_to_phys_harder()?
Tell me about it...
> What's the performance effect? If it's negligible, why doesn't
> virt_to_phys() just do this for us?
I've not measured it, but even when you don't have CONFIG_HIGHMEM, there's
going to be an overhead here because we go around the houses to get the page
and then add the offset on afterwards. I doubt it's something we want to
plumb directly into virt_to_phys (also, kmap_to_page may call virt_to_phys via
the __pa macro so we'd get stuck).
> We do have an alternate solution: masking out __GFP_HIGHMEM from the
> kmalloc of desc. If it fails, we will fall back to laying out the
> virtio request directly inside the ring; if it doesn't fit, we'll wait
> for the device to consume more buffers.
Hmm, that will probably work for the vring but the zero-copy code for 9p may
just give us an address from userspace if I'm understanding it correctly. In
that case, we really have to do the translation as below (which is actually
much cleaner because everything is page-aligned).
> > @@ -325,7 +326,7 @@ static int p9_get_mapped_pages(struct virtio_chan *chan,
> > int count = nr_pages;
> > while (nr_pages) {
> > s = rest_of_page(data);
> > - pages[index++] = virt_to_page(data);
> > + pages[index++] = kmap_to_page(data);
> > data += s;
> > nr_pages--;
> > }
So what do you reckon? How about I leave this hunk as a separate patch and
have a play masking out __GFP_HIGHMEM for the vring descriptor?
Cheers,
Will
^ permalink raw reply
* Re: PCI device not properly reset after VFIO
From: Alex Williamson @ 2012-10-18 14:40 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: kvm-devel, Linux Virtualization
In-Reply-To: <507FA602.9050801@suse.de>
Hi Hannes,
Thanks for testing vfio
On Thu, 2012-10-18 at 08:47 +0200, Hannes Reinecke wrote:
> Hi Alex,
>
> I've been playing around with VFIO and megasas (of course).
> What I did now was switching between VFIO and 'normal' operation, ie
> emulated access.
>
> megasas is happily running under VFIO, but when I do an emergency
> stop like killing the Qemu session the PCI device is not properly reset.
> IE when I load 'megaraid_sas' after unbinding the vfio_pci module
> the driver cannot initialize the card and waits forever for the
> firmware state to change.
>
> I need to do a proper pci reset via
> echo 1 > /sys/bus/pci/device/XXXX/reset
> to get it into a working state again.
>
> Looking at vfio_pci_disable() pci reset is called before the config
> state and BARs are restored.
> Seeing that vfio_pci_enable() calls pci reset right at the start,
> too, before modifying anything I do wonder whether the pci reset is
> at the correct location for disable.
>
> I would have expected to call pci reset in vfio_pci_disable()
> _after_ we have restored the configuration, to ensure a sane state
> after reset.
> And, as experience show, we do need to call it there.
>
> So what is the rationale for the pci reset?
> Can we move it to the end of vfio_pci_disable() or do we need to
> call pci reset twice?
I believe the rationale was that by resetting the device before we
restore the state we stop anything that the device was doing. Restoring
the saved state on a running device seems like it could cause problems,
so you may be right and we actually need to do reset, load, restore,
reset. Does adding another call to pci_reset_function in the
pci_restore_state (as below) solve the problem? Traditional KVM device
assignment has a nearly identical path, does it have this same bug?
Thanks,
Alex
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 6c11994..d07a45c 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -107,9 +107,10 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev)
pci_reset_function(vdev->pdev);
if (pci_load_and_free_saved_state(vdev->pdev,
- &vdev->pci_saved_state) == 0)
+ &vdev->pci_saved_state) == 0) {
pci_restore_state(vdev->pdev);
- else
+ pci_reset_function(vdev->pdev);
+ } else
pr_info("%s: Couldn't reload %s saved state\n",
__func__, dev_name(&vdev->pdev->dev));
^ permalink raw reply related
* Re: PCI device not properly reset after VFIO
From: Hannes Reinecke @ 2012-10-18 15:06 UTC (permalink / raw)
To: Alex Williamson; +Cc: kvm-devel, Linux Virtualization
In-Reply-To: <1350571230.2112.344.camel@bling.home>
On 10/18/2012 04:40 PM, Alex Williamson wrote:
> Hi Hannes,
>
> Thanks for testing vfio
>
> On Thu, 2012-10-18 at 08:47 +0200, Hannes Reinecke wrote:
>> Hi Alex,
>>
>> I've been playing around with VFIO and megasas (of course).
>> What I did now was switching between VFIO and 'normal' operation, ie
>> emulated access.
>>
>> megasas is happily running under VFIO, but when I do an emergency
>> stop like killing the Qemu session the PCI device is not properly reset.
>> IE when I load 'megaraid_sas' after unbinding the vfio_pci module
>> the driver cannot initialize the card and waits forever for the
>> firmware state to change.
>>
>> I need to do a proper pci reset via
>> echo 1 > /sys/bus/pci/device/XXXX/reset
>> to get it into a working state again.
>>
>> Looking at vfio_pci_disable() pci reset is called before the config
>> state and BARs are restored.
>> Seeing that vfio_pci_enable() calls pci reset right at the start,
>> too, before modifying anything I do wonder whether the pci reset is
>> at the correct location for disable.
>>
>> I would have expected to call pci reset in vfio_pci_disable()
>> _after_ we have restored the configuration, to ensure a sane state
>> after reset.
>> And, as experience show, we do need to call it there.
>>
>> So what is the rationale for the pci reset?
>> Can we move it to the end of vfio_pci_disable() or do we need to
>> call pci reset twice?
>
> I believe the rationale was that by resetting the device before we
> restore the state we stop anything that the device was doing. Restoring
> the saved state on a running device seems like it could cause problems,
> so you may be right and we actually need to do reset, load, restore,
> reset. Does adding another call to pci_reset_function in the
> pci_restore_state (as below) solve the problem? Traditional KVM device
> assignment has a nearly identical path, does it have this same bug?
It's actually the first time I've been able to test this (the
hardware is a bit tricky to setup ...), so I cannot tell (yet)
if KVM exhibited the same thing.
> Thanks,
>
> Alex
>
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index 6c11994..d07a45c 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -107,9 +107,10 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev)
> pci_reset_function(vdev->pdev);
>
> if (pci_load_and_free_saved_state(vdev->pdev,
> - &vdev->pci_saved_state) == 0)
> + &vdev->pci_saved_state) == 0) {
> pci_restore_state(vdev->pdev);
> - else
> + pci_reset_function(vdev->pdev);
> + } else
> pr_info("%s: Couldn't reload %s saved state\n",
> __func__, dev_name(&vdev->pdev->dev));
>
>
>
I would have called reset after unmapping the BARs; the HBA I'm
working with does need to access the BARs, so the content of them
might be relevant, too.
But then I'm not really a PCI expert.
Maybe we should ask Tony Luck or Bjorn Helgaas.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: PCI device not properly reset after VFIO
From: Alex Williamson @ 2012-10-18 20:40 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: linux-pci, kvm-devel, Linux Virtualization
In-Reply-To: <50801AF8.7040305@suse.de>
On Thu, 2012-10-18 at 17:06 +0200, Hannes Reinecke wrote:
> On 10/18/2012 04:40 PM, Alex Williamson wrote:
> > Hi Hannes,
> >
> > Thanks for testing vfio
> >
> > On Thu, 2012-10-18 at 08:47 +0200, Hannes Reinecke wrote:
> >> Hi Alex,
> >>
> >> I've been playing around with VFIO and megasas (of course).
> >> What I did now was switching between VFIO and 'normal' operation, ie
> >> emulated access.
> >>
> >> megasas is happily running under VFIO, but when I do an emergency
> >> stop like killing the Qemu session the PCI device is not properly reset.
> >> IE when I load 'megaraid_sas' after unbinding the vfio_pci module
> >> the driver cannot initialize the card and waits forever for the
> >> firmware state to change.
> >>
> >> I need to do a proper pci reset via
> >> echo 1 > /sys/bus/pci/device/XXXX/reset
> >> to get it into a working state again.
> >>
> >> Looking at vfio_pci_disable() pci reset is called before the config
> >> state and BARs are restored.
> >> Seeing that vfio_pci_enable() calls pci reset right at the start,
> >> too, before modifying anything I do wonder whether the pci reset is
> >> at the correct location for disable.
> >>
> >> I would have expected to call pci reset in vfio_pci_disable()
> >> _after_ we have restored the configuration, to ensure a sane state
> >> after reset.
> >> And, as experience show, we do need to call it there.
> >>
> >> So what is the rationale for the pci reset?
> >> Can we move it to the end of vfio_pci_disable() or do we need to
> >> call pci reset twice?
> >
> > I believe the rationale was that by resetting the device before we
> > restore the state we stop anything that the device was doing. Restoring
> > the saved state on a running device seems like it could cause problems,
> > so you may be right and we actually need to do reset, load, restore,
> > reset. Does adding another call to pci_reset_function in the
> > pci_restore_state (as below) solve the problem? Traditional KVM device
> > assignment has a nearly identical path, does it have this same bug?
>
> It's actually the first time I've been able to test this (the
> hardware is a bit tricky to setup ...), so I cannot tell (yet)
> if KVM exhibited the same thing.
>
> > Thanks,
> >
> > Alex
> >
> > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > index 6c11994..d07a45c 100644
> > --- a/drivers/vfio/pci/vfio_pci.c
> > +++ b/drivers/vfio/pci/vfio_pci.c
> > @@ -107,9 +107,10 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev)
> > pci_reset_function(vdev->pdev);
> >
> > if (pci_load_and_free_saved_state(vdev->pdev,
> > - &vdev->pci_saved_state) == 0)
> > + &vdev->pci_saved_state) == 0) {
> > pci_restore_state(vdev->pdev);
> > - else
> > + pci_reset_function(vdev->pdev);
> > + } else
> > pr_info("%s: Couldn't reload %s saved state\n",
> > __func__, dev_name(&vdev->pdev->dev));
> >
> >
> >
> I would have called reset after unmapping the BARs; the HBA I'm
> working with does need to access the BARs, so the content of them
> might be relevant, too.
I think I copied the ordering from kvm since it seems to work there,
maybe it doesn't for your device if the kvm path is still an unknown.
Logically it does seem like we'd want to unmap and release before the
final reset, but I can't find that those paths actually cause
interactions where it would matter.
Since we first disable the device and free the interrupt config it seems
like we should be relatively at ease restoring the saved config prior to
reset and, as you suggest, re-ordering the unmap and region release.
That leaves us with something like below. Does that work better for
your device? Anyone on linux-pci have advice on ordering of
pci_reset_function? Thanks,
Alex
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 6c11994..af0b165 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -104,7 +104,13 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev)
vfio_config_free(vdev);
- pci_reset_function(vdev->pdev);
+ for (bar = PCI_STD_RESOURCES; bar <= PCI_STD_RESOURCE_END; bar++) {
+ if (!vdev->barmap[bar])
+ continue;
+ pci_iounmap(vdev->pdev, vdev->barmap[bar]);
+ pci_release_selected_regions(vdev->pdev, 1 << bar);
+ vdev->barmap[bar] = NULL;
+ }
if (pci_load_and_free_saved_state(vdev->pdev,
&vdev->pci_saved_state) == 0)
@@ -113,13 +119,7 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev)
pr_info("%s: Couldn't reload %s saved state\n",
__func__, dev_name(&vdev->pdev->dev));
- for (bar = PCI_STD_RESOURCES; bar <= PCI_STD_RESOURCE_END; bar++) {
- if (!vdev->barmap[bar])
- continue;
- pci_iounmap(vdev->pdev, vdev->barmap[bar]);
- pci_release_selected_regions(vdev->pdev, 1 << bar);
- vdev->barmap[bar] = NULL;
- }
+ pci_reset_function(vdev->pdev);
}
static void vfio_pci_release(void *device_data)
^ permalink raw reply related
* Re: [PATCH] virtio: 9p: correctly pass physical address to userspace for high pages
From: Rusty Russell @ 2012-10-18 23:39 UTC (permalink / raw)
To: Will Deacon
Cc: Marc Zyngier, linux-kernel@vger.kernel.org, lf-virt,
Eric Van Hensbergen, Sasha Levin, Andrew Morton
In-Reply-To: <20121018094247.GA17296@mudshark.cambridge.arm.com>
Will Deacon <will.deacon@arm.com> writes:
> On Thu, Oct 18, 2012 at 03:19:06AM +0100, Rusty Russell wrote:
>> We do have an alternate solution: masking out __GFP_HIGHMEM from the
>> kmalloc of desc. If it fails, we will fall back to laying out the
>> virtio request directly inside the ring; if it doesn't fit, we'll wait
>> for the device to consume more buffers.
>
> Hmm, that will probably work for the vring but the zero-copy code for 9p may
> just give us an address from userspace if I'm understanding it correctly. In
> that case, we really have to do the translation as below (which is actually
> much cleaner because everything is page-aligned).
>
>> > @@ -325,7 +326,7 @@ static int p9_get_mapped_pages(struct virtio_chan *chan,
>> > int count = nr_pages;
>> > while (nr_pages) {
>> > s = rest_of_page(data);
>> > - pages[index++] = virt_to_page(data);
>> > + pages[index++] = kmap_to_page(data);
>> > data += s;
>> > nr_pages--;
>> > }
>
> So what do you reckon? How about I leave this hunk as a separate patch and
> have a play masking out __GFP_HIGHMEM for the vring descriptor?
Yes, I think so. A scathing comment would be nice, too...
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCH] vhost-blk: Add vhost-blk support v2
From: Asias He @ 2012-10-19 6:44 UTC (permalink / raw)
To: Rusty Russell; +Cc: virtualization, kvm, Michael S. Tsirkin
In-Reply-To: <87d30go1g7.fsf@rustcorp.com.au>
On 10/18/2012 12:20 PM, Rusty Russell wrote:
> Asias He <asias@redhat.com> writes:
>>>> +#define BLK_HDR 0
>>>
>>> What's this for, exactly? Please add a comment.
>>
>> The block headr is in the first and separate buffer.
>
> Please don't assume this! We're trying to fix all the assumptions in
> qemu at the moment.
>
> vhost_net handles this correctly, taking bytes off the descriptor chain
> as required.
Well, I will switch to "no assumption about the buffer layout" in next
version.
--
Asias
^ permalink raw reply
* [PATCH v2 1/3] mm: highmem: export kmap_to_page for modules
From: Will Deacon @ 2012-10-19 13:03 UTC (permalink / raw)
To: linux-kernel
Cc: Marc.Zyngier, Will Deacon, virtualization, ericvh, levinsasha928,
akpm
Some virtio device drivers (9p) need to translate high virtual addresses
to physical addresses, which are inserted into the virtqueue for
processing by userspace.
This patch exports the kmap_to_page symbol, so that the affected drivers
can be compiled as modules.
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
mm/highmem.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/mm/highmem.c b/mm/highmem.c
index d517cd1..2a07f97 100644
--- a/mm/highmem.c
+++ b/mm/highmem.c
@@ -105,6 +105,7 @@ struct page *kmap_to_page(void *vaddr)
return virt_to_page(addr);
}
+EXPORT_SYMBOL(kmap_to_page);
static void flush_all_zero_pkmaps(void)
{
--
1.7.4.1
^ permalink raw reply related
* [PATCH v2 2/3] virtio: 9p: correctly pass physical address to userspace for high pages
From: Will Deacon @ 2012-10-19 13:03 UTC (permalink / raw)
To: linux-kernel
Cc: Marc.Zyngier, Will Deacon, virtualization, ericvh, levinsasha928,
akpm
In-Reply-To: <1350651813-8694-1-git-send-email-will.deacon@arm.com>
When using a virtio transport, the 9p net device may pass the physical
address of a kernel buffer to userspace via a scatterlist inside a
virtqueue. If the kernel buffer is mapped outside of the linear mapping
(e.g. highmem), then virt_to_page will return a bogus value and we will
populate the scatterlist with junk.
This patch uses kmap_to_page when populating the page array for a kernel
buffer.
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Sasha Levin <levinsasha928@gmail.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
net/9p/trans_virtio.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
index 35b8911..fd05c81 100644
--- a/net/9p/trans_virtio.c
+++ b/net/9p/trans_virtio.c
@@ -39,6 +39,7 @@
#include <linux/inet.h>
#include <linux/idr.h>
#include <linux/file.h>
+#include <linux/highmem.h>
#include <linux/slab.h>
#include <net/9p/9p.h>
#include <linux/parser.h>
@@ -325,7 +326,7 @@ static int p9_get_mapped_pages(struct virtio_chan *chan,
int count = nr_pages;
while (nr_pages) {
s = rest_of_page(data);
- pages[index++] = virt_to_page(data);
+ pages[index++] = kmap_to_page(data);
data += s;
nr_pages--;
}
--
1.7.4.1
^ permalink raw reply related
* [PATCH v2 3/3] virtio: force vring descriptors to be allocated from lowmem
From: Will Deacon @ 2012-10-19 13:03 UTC (permalink / raw)
To: linux-kernel
Cc: Marc.Zyngier, Will Deacon, virtualization, ericvh, levinsasha928,
akpm
In-Reply-To: <1350651813-8694-1-git-send-email-will.deacon@arm.com>
Virtio devices may attempt to add descriptors to a virtqueue from atomic
context using GFP_ATOMIC allocation. This is problematic because such
allocations can fall outside of the lowmem mapping, causing virt_to_phys
to report bogus physical addresses which are subsequently passed to
userspace via the buffers for the virtual device.
This patch masks out __GFP_HIGH and __GFP_HIGHMEM from the requested
flags when allocating descriptors for a virtqueue. If an atomic
allocation is requested and later fails, we will return -ENOSPC which
will be handled by the driver.
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Sasha Levin <levinsasha928@gmail.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
drivers/virtio/virtio_ring.c | 7 +++++++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index e639584..286c30c 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -135,6 +135,13 @@ static int vring_add_indirect(struct vring_virtqueue *vq,
unsigned head;
int i;
+ /*
+ * We require lowmem mappings for the descriptors because
+ * otherwise virt_to_phys will give us bogus addresses in the
+ * virtqueue.
+ */
+ gfp &= ~(__GFP_HIGHMEM | __GFP_HIGH);
+
desc = kmalloc((out + in) * sizeof(struct vring_desc), gfp);
if (!desc)
return -ENOMEM;
--
1.7.4.1
^ permalink raw reply related
* Re: [PATCH for-3.7] vhost: fix mergeable bufs on BE hosts
From: Michael S. Tsirkin @ 2012-10-21 12:49 UTC (permalink / raw)
To: Alexander Graf; +Cc: netdev, stable, kvm, virtualization
In-Reply-To: <20121015175534.GA19489@redhat.com>
On Mon, Oct 15, 2012 at 07:55:34PM +0200, Michael S. Tsirkin wrote:
> We copy head count to a 16 bit field,
> this works by chance on LE but on BE
> guest gets 0. Fix it up.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Tested-by: Alexander Graf <agraf@suse.de>
> Cc: stable@kernel.org
Ping. Dave, could you apply this to -net please?
> ---
> drivers/vhost/net.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 9ab6d47..2bb463c 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -448,7 +448,8 @@ static void handle_rx(struct vhost_net *net)
> .hdr.gso_type = VIRTIO_NET_HDR_GSO_NONE
> };
> size_t total_len = 0;
> - int err, headcount, mergeable;
> + int err, mergeable;
> + s16 headcount;
> size_t vhost_hlen, sock_hlen;
> size_t vhost_len, sock_len;
> /* TODO: check that we are running from vhost_worker? */
> --
> MST
^ permalink raw reply
* Re: [PATCH for-3.7] vhost: fix mergeable bufs on BE hosts
From: David Miller @ 2012-10-21 23:24 UTC (permalink / raw)
To: mst; +Cc: netdev, stable, kvm, virtualization
In-Reply-To: <20121021124901.GA5315@redhat.com>
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Sun, 21 Oct 2012 14:49:01 +0200
> On Mon, Oct 15, 2012 at 07:55:34PM +0200, Michael S. Tsirkin wrote:
>> We copy head count to a 16 bit field,
>> this works by chance on LE but on BE
>> guest gets 0. Fix it up.
>>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> Tested-by: Alexander Graf <agraf@suse.de>
>> Cc: stable@kernel.org
>
> Ping. Dave, could you apply this to -net please?
Pinging me but not cc:'ing me? That's really strange.
What if I operate by just mass deleting things that I'm
not explicitly on the To: or CC: when I'm very backlogged?
^ permalink raw reply
* Re: [PATCHv7 0/4] virtio_console: Add rproc_serial driver
From: Amit Shah @ 2012-10-22 13:00 UTC (permalink / raw)
To: sjur.brandeland
Cc: Michael S. Tsirkin, Linus Walleij, linux-kernel, virtualization,
Masami Hiramatsu
In-Reply-To: <1350287856-5284-1-git-send-email-sjur.brandeland@stericsson.com>
Hello Sjur,
On (Mon) 15 Oct 2012 [09:57:32], sjur.brandeland@stericsson.com wrote:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> This patch-set introduces a new virtio type "rproc_serial" for communicating
> with remote processors over shared memory. The driver depends on the
> the remoteproc framework. As preparation for introducing "rproc_serial"
> I've done a refactoring of the transmit buffer handling.
>
> This patch-set is a rework of the patch-set from Sept 25th, hopefully all
> review comments has been addressed.
>
> The fist patch is a bugfix and migth be applicable for 3.7.
Looks good,
Acked-by: Amit Shah <amit.shah@redhat.com>
include/linux/virtio_ids.h has moved to include/uapi/linux/. Last
patch has to be re-spun for that.
Otherwise, I just checked if comments from the previous submission
were addressed. The virtio-console test suite passes fine, so I
didn't really go through everything again.
Thanks,
Amit
^ permalink raw reply
* Re: [PATCH v2 2/3] virtio: 9p: correctly pass physical address to userspace for high pages
From: Andrew Morton @ 2012-10-22 22:05 UTC (permalink / raw)
To: Will Deacon
Cc: Marc.Zyngier, linux-kernel, virtualization, ericvh, levinsasha928
In-Reply-To: <1350651813-8694-2-git-send-email-will.deacon@arm.com>
On Fri, 19 Oct 2012 14:03:32 +0100
Will Deacon <will.deacon@arm.com> wrote:
> When using a virtio transport, the 9p net device may pass the physical
> address of a kernel buffer to userspace via a scatterlist inside a
> virtqueue. If the kernel buffer is mapped outside of the linear mapping
> (e.g. highmem), then virt_to_page will return a bogus value and we will
> populate the scatterlist with junk.
>
> This patch uses kmap_to_page when populating the page array for a kernel
> buffer.
>
> ...
>
> --- a/net/9p/trans_virtio.c
> +++ b/net/9p/trans_virtio.c
> @@ -39,6 +39,7 @@
> #include <linux/inet.h>
> #include <linux/idr.h>
> #include <linux/file.h>
> +#include <linux/highmem.h>
> #include <linux/slab.h>
> #include <net/9p/9p.h>
> #include <linux/parser.h>
> @@ -325,7 +326,7 @@ static int p9_get_mapped_pages(struct virtio_chan *chan,
> int count = nr_pages;
> while (nr_pages) {
> s = rest_of_page(data);
> - pages[index++] = virt_to_page(data);
> + pages[index++] = kmap_to_page(data);
> data += s;
> nr_pages--;
I am suspecting that this code has been busted for a while on x86
highmem, but nobody noticed. True or false? If "true" then I expect
that a -stable backport is appropriate?
^ permalink raw reply
* Re: [PATCH v2 1/3] mm: highmem: export kmap_to_page for modules
From: Andrew Morton @ 2012-10-22 22:08 UTC (permalink / raw)
To: Will Deacon
Cc: Marc.Zyngier, linux-kernel, virtualization, ericvh, levinsasha928
In-Reply-To: <1350651813-8694-1-git-send-email-will.deacon@arm.com>
On Fri, 19 Oct 2012 14:03:31 +0100
Will Deacon <will.deacon@arm.com> wrote:
> Some virtio device drivers (9p) need to translate high virtual addresses
> to physical addresses, which are inserted into the virtqueue for
> processing by userspace.
>
> This patch exports the kmap_to_page symbol, so that the affected drivers
> can be compiled as modules.
>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
> mm/highmem.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/mm/highmem.c b/mm/highmem.c
> index d517cd1..2a07f97 100644
> --- a/mm/highmem.c
> +++ b/mm/highmem.c
> @@ -105,6 +105,7 @@ struct page *kmap_to_page(void *vaddr)
>
> return virt_to_page(addr);
> }
> +EXPORT_SYMBOL(kmap_to_page);
>
Looks OK to me. Would generally prefer that an exported-to-modules
symbol have some documentation, but I guess this one is obvious enough.
Rusty, please include this if you grab the rest of the series.
^ permalink raw reply
* Re: [PATCH v2 1/3] mm: highmem: export kmap_to_page for modules
From: Rusty Russell @ 2012-10-22 23:55 UTC (permalink / raw)
To: linux-kernel
Cc: Marc.Zyngier, Will Deacon, virtualization, ericvh, levinsasha928,
akpm
In-Reply-To: <1350651813-8694-1-git-send-email-will.deacon@arm.com>
Will Deacon <will.deacon@arm.com> writes:
> Some virtio device drivers (9p) need to translate high virtual addresses
> to physical addresses, which are inserted into the virtqueue for
> processing by userspace.
>
> This patch exports the kmap_to_page symbol, so that the affected drivers
> can be compiled as modules.
Thanks Will!
I've applied these, and add cc:stable. I'll push them to Linus after a
little time in linux-next.
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCH] vhost-blk: Add vhost-blk support v2
From: Rusty Russell @ 2012-10-23 0:07 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Paolo Bonzini, kvm, virtualization
In-Reply-To: <20121018063040.GA15676@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Thu, Oct 18, 2012 at 02:50:56PM +1030, Rusty Russell wrote:
>> Asias He <asias@redhat.com> writes:
>> >>> +#define BLK_HDR 0
>> >>
>> >> What's this for, exactly? Please add a comment.
>> >
>> > The block headr is in the first and separate buffer.
>>
>> Please don't assume this! We're trying to fix all the assumptions in
>> qemu at the moment.
>>
>> vhost_net handles this correctly, taking bytes off the descriptor chain
>> as required.
>>
>> Thanks,
>> Rusty.
>
> BTW are we agreed on the spec update that makes cmd 32 bytes?
vhost-blk doesn't handle scsi requests, does it?
But since we're forced to use a feature bit, we could just put the cmd
size in explicitly. Though Paulo seems convinced that 32 is always
sufficient. Whoever implements it gets to decide...
Here's my TODO list:
1) Create qemu helpers to efficiently handle iovecs.
2) Switch all the qemu devices to use them.
3) ... except a special hack for virtio-blk in old-layout mode.
4) Implement pci capability layout RFC for qemu.
- Detect whether guest uses capabilities.
- Device config in new mode is le.
- Add strict checking mode for extra compliance checks?
5) Add explicit size-based accessors to virtio_config in kernel.
6) Update pci capability RFC patches for linux to match.
- Use explicit accessors to allow for endian conversion.
6) Push virtio torture patch to test variable boundaries.
7) Update spec.
That should keep me amused for a while...
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCHv7 1/4] virtio_console: Free buffer if splice fails
From: Rusty Russell @ 2012-10-23 0:12 UTC (permalink / raw)
To: Amit Shah
Cc: Michael S. Tsirkin, Linus Walleij, linux-kernel, virtualization,
Masami Hiramatsu, Sjur Brændeland
In-Reply-To: <1350287856-5284-2-git-send-email-sjur.brandeland@stericsson.com>
sjur.brandeland@stericsson.com writes:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> Free the allocated scatter list if send_pages fails in function
> port_splice_write.
>
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
Didn't see Amit ack this, but applied anyway.
Thanks,
Rusty.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCHv7 3/4] virtio_console: Merge struct buffer_token into struct port_buffer
From: Rusty Russell @ 2012-10-23 0:19 UTC (permalink / raw)
To: Amit Shah
Cc: Michael S. Tsirkin, Linus Walleij, linux-kernel, virtualization,
Masami Hiramatsu, Sjur Brændeland
In-Reply-To: <1350287856-5284-4-git-send-email-sjur.brandeland@stericsson.com>
sjur.brandeland@stericsson.com writes:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> Refactoring the splice functionality by unifying the approach for
> sending scatter-lists and regular buffers. This simplifies
> buffer handling and reduces code size. Splice will now allocate
> a port_buffer and send_buf() and free_buf() can always be used
> for any buffer.
>
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
This looks sensible; a couple of extra blank lines inserted though.
Amit?
> @@ -1033,6 +1008,8 @@ static const struct file_operations port_fops = {
> static int put_chars(u32 vtermno, const char *buf, int count)
> {
> struct port *port;
> + struct scatterlist sg[1];
> +
>
> if (unlikely(early_put_chars))
> return early_put_chars(vtermno, buf, count);
> @@ -1041,7 +1018,9 @@ static int put_chars(u32 vtermno, const char *buf, int count)
> if (!port)
> return -EPIPE;
>
> - return send_buf(port, (void *)buf, count, false);
> + sg_init_one(sg, buf, count);
> + return __send_to_port(port, sg, 1, count, (void *)buf, false);
> +
> }
>
> /*
Cheers,
Rusty.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCHv7 2/4] virtio_console: Use kmalloc instead of kzalloc
From: Rusty Russell @ 2012-10-23 1:36 UTC (permalink / raw)
To: Amit Shah
Cc: Michael S. Tsirkin, Linus Walleij, linux-kernel, virtualization,
Masami Hiramatsu, Sjur Brændeland
In-Reply-To: <1350287856-5284-3-git-send-email-sjur.brandeland@stericsson.com>
sjur.brandeland@stericsson.com writes:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> Avoid the more cpu expensive kzalloc when allocating buffers.
> Originally kzalloc was intended for isolating the guest from
> the host by not sending random guest data to the host. But device
> isolation is not yet in place so kzalloc is not really needed.
>
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
This looks fine to me. This is *why* the device gives us the length
which was written; we can trust that, even if we can't trust the
writer of data.
(In theory: noone has implemented such a system, yet).
Applied.
Rusty.
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index c36b2f6..301d17e 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -349,7 +349,7 @@ static struct port_buffer *alloc_buf(size_t buf_size)
> buf = kmalloc(sizeof(*buf), GFP_KERNEL);
> if (!buf)
> goto fail;
> - buf->buf = kzalloc(buf_size, GFP_KERNEL);
> + buf->buf = kmalloc(buf_size, GFP_KERNEL);
> if (!buf->buf)
> goto free_buf;
> buf->len = 0;
> --
> 1.7.5.4
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox