RE: [PATCH v6 kernel 3/5] virtio-balloon: speed up inflate/deflate process
>
>
> > +static void free_extended_page_bitmap(struct virtio_balloon *vb) {
> > + int i, bmap_count = vb->nr_page_bmap;
> > +
> > + for (i = 1; i < bmap_count; i++) {
> > + kfree(vb->page_bitmap[i]);
> > + vb->page_bitmap[i] = NULL;
> > + vb->nr_page_bmap--;
> > + }
> > +}
> > +
> > +static void kfree_page_bitmap(struct virtio_balloon *vb) {
> > + int i;
> > +
> > + for (i = 0; i < vb->nr_page_bmap; i++)
> > + kfree(vb->page_bitmap[i]);
> > +}
>
> It might be worth commenting that pair of functions to make it clear why
> they are so different; I guess the kfree_page_bitmap is used just before you
> free the structure above it so you don't need to keep the count/pointers
> updated?
>
Yes. I will add some comments for that. Thanks!
Liang
> Dave
> --
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vhost: fix initialization for vq->is_le
On 2017年01月30日 18:09, Halil Pasic wrote:
Currently, under certain circumstances vhost_init_is_le does just a part
of the initialization job, and depends on vhost_reset_is_le being called
too. For this reason vhost_vq_init_access used to call vhost_reset_is_le
when vq->private_data is NULL. This is not only counter intuitive, but
also real a problem because it breaks vhost_net. The bug was introduced to
vhost_net with commit 2751c9882b94 ("vhost: cross-endian support for
legacy devices"). The symptom is corruption of the vq's used.idx field
(virtio) after VHOST_NET_SET_BACKEND was issued as a part of the vhost
shutdown on a vq with pending descriptors.
Let us make sure the outcome of vhost_init_is_le never depend on the state
it is actually supposed to initialize, and fix virtio_net by removing the
reset from vhost_vq_init_access.
With the above, there is no reason for vhost_reset_is_le to do just half
of the job. Let us make vhost_reset_is_le reinitialize is_le.
Signed-off-by: Halil Pasic
Reported-by: Michael A. Tebolt
Reported-by: Dr. David Alan Gilbert
Fixes: commit 2751c9882b94 ("vhost: cross-endian support for legacy devices")
---
The bug was already discussed here:
http://www.spinics.net/lists/kvm/msg144365.html
This is a follow up patch.
---
drivers/vhost/vhost.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index d643260..8f99fe0 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -130,14 +130,14 @@ static long vhost_get_vring_endian(struct vhost_virtqueue
*vq, u32 idx,
static void vhost_init_is_le(struct vhost_virtqueue *vq)
{
- if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
- vq->is_le = true;
+ vq->is_le = vhost_has_feature(vq, VIRTIO_F_VERSION_1)
+ || virtio_legacy_is_little_endian();
}
#endif /* CONFIG_VHOST_CROSS_ENDIAN_LEGACY */
static void vhost_reset_is_le(struct vhost_virtqueue *vq)
{
- vq->is_le = virtio_legacy_is_little_endian();
+ vhost_init_is_le(vq);
}
struct vhost_flush_struct {
@@ -1714,10 +1714,8 @@ int vhost_vq_init_access(struct vhost_virtqueue *vq)
int r;
bool is_le = vq->is_le;
- if (!vq->private_data) {
- vhost_reset_is_le(vq);
+ if (!vq->private_data)
return 0;
- }
vhost_init_is_le(vq);
Acked-by: Jason Wang
We can probably just drop vhost_reset_is_le() and just use
vhost_init_is_le() instead.
Thanks
___
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 566cf877a1fcb6d6dc0126b076aad062054c2637: Linux 4.10-rc6 (2017-01-29 14:25:17 -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 79134d11d030b886106bf45a5638c1ccb1f0856c: MAINTAINERS: update email address for Amit Shah (2017-02-03 23:40:36 +0200) virtio, vhost: last minute fixes ARM DMA fix revert vhost endian-ness fix MAINTAINERS: email address change for Amit Signed-off-by: Michael S. Tsirkin Amit Shah (1): MAINTAINERS: update email address for Amit Shah Halil Pasic (1): vhost: fix initialization for vq->is_le Michael S. Tsirkin (1): Revert "vring: Force use of DMA API for ARM-based systems with legacy devices" MAINTAINERS | 2 +- drivers/vhost/vhost.c| 10 -- drivers/virtio/virtio_ring.c | 7 --- 3 files changed, 5 insertions(+), 14 deletions(-) ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] MAINTAINERS: update my email address
On (Fri) 03 Feb 2017 [17:11:49], Michael S. Tsirkin wrote: > On Fri, Feb 03, 2017 at 04:48:14PM +0530, Amit Shah wrote: > > I'm leaving my job at Red Hat, this email address will stop working next > > week. > > Update it to one that I will have access to later. > > > > Signed-off-by: Amit Shah > > It's great that we'll still have you around! Do you want to send a pull > request with this patch? Or want me to merge it? Yes, I expect to be around :) Please merge it via your tree, thanks. Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] MAINTAINERS: update my email address
On Fri, Feb 03, 2017 at 04:48:14PM +0530, Amit Shah wrote: > I'm leaving my job at Red Hat, this email address will stop working next week. > Update it to one that I will have access to later. > > Signed-off-by: Amit Shah It's great that we'll still have you around! Do you want to send a pull request with this patch? Or want me to merge it? Acked-by: Michael S. Tsirkin > --- > MAINTAINERS | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 3960e7f..187b961 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -13065,7 +13065,7 @@ F:drivers/input/serio/userio.c > F: include/uapi/linux/userio.h > > VIRTIO CONSOLE DRIVER > -M: Amit Shah > +M: Amit Shah > L: virtualization@lists.linux-foundation.org > S: Maintained > F: drivers/char/virtio_console.c > -- > 1.8.3.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] MAINTAINERS: update my email address
I'm leaving my job at Red Hat, this email address will stop working next week. Update it to one that I will have access to later. Signed-off-by: Amit Shah --- MAINTAINERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index 3960e7f..187b961 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -13065,7 +13065,7 @@ F: drivers/input/serio/userio.c F: include/uapi/linux/userio.h VIRTIO CONSOLE DRIVER -M: Amit Shah +M: Amit Shah L: virtualization@lists.linux-foundation.org S: Maintained F: drivers/char/virtio_console.c -- 1.8.3.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] Revert "vring: Force use of DMA API for ARM-based systems with legacy devices"
On Fri, Feb 03, 2017 at 05:49:11AM +0200, Michael S. Tsirkin wrote:
> This reverts commit c7070619f3408d9a0dffbed9149e6f00479cf43b.
>
> This has been shown to regress on some ARM systems:
>
> by forcing on DMA API usage for ARM systems, we have inadvertently
> kicked open a hornets' nest in terms of cache-coherency. Namely that
> unless the virtio device is explicitly described as capable of coherent
> DMA by firmware, the DMA APIs on ARM and other DT-based platforms will
> assume it is non-coherent. This turns out to cause a big problem for the
> likes of QEMU and kvmtool, which generate virtio-mmio devices in their
> guest DTs but neglect to add the often-overlooked "dma-coherent"
> property; as a result, we end up with the guest making non-cacheable
> accesses to the vring, the host doing so cacheably, both talking past
> each other and things going horribly wrong.
>
> We are working on a safer work-around.
>
> Fixes: c7070619f340 ("vring: Force use of DMA API for ARM-based systems with
> legacy devices")
> Reported-by: Robin Murphy
> Cc:
> Signed-off-by: Will Deacon
> Signed-off-by: Michael S. Tsirkin
> Acked-by: Marc Zyngier
> ---
>
> I'll merge this for 4.10. Let's fix it properly for 4.11.
Acked-by: Will Deacon
Will
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/9] virtio_pci: use shared interrupts for virtqueues
On 2017年02月03日 17:52, Christoph Hellwig wrote: On Fri, Feb 03, 2017 at 05:47:41PM +0800, Jason Wang wrote: No, we need to allocate the array larger in that case as want proper names for the interrupts. Consider the case of !per_vq_vectors, the size of msix_names is 2, but snprintf can do out of bound accessing here. (We name the msix shared by virtqueues with something like "%s-virtqueues" before the patch). Yes, that's what I meant above - we need to allocate a large array starting with this patch. I'll fix it up for the next version. I see. Thanks ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 2/2] virtio: Document DMA coherency
On Thu, Feb 02, 2017 at 04:36:21PM +, Robin Murphy wrote: > Since making use of the DMA API will require the architecture code to > have the correct notion of device cache-coherency on architectures like > ARM, explicitly call this out in the virtio-mmio DT binding. The ship > has sailed for legacy virtio, but let's hope that we can head off any > future firmware mishaps. > > Signed-off-by: Robin Murphy > --- > Documentation/devicetree/bindings/virtio/mmio.txt | 11 +++ > 1 file changed, 11 insertions(+) > > diff --git a/Documentation/devicetree/bindings/virtio/mmio.txt > b/Documentation/devicetree/bindings/virtio/mmio.txt > index 5069c1b8e193..999a93faa67c 100644 > --- a/Documentation/devicetree/bindings/virtio/mmio.txt > +++ b/Documentation/devicetree/bindings/virtio/mmio.txt > @@ -7,6 +7,16 @@ Required properties: > - compatible:"virtio,mmio" compatibility string > - reg: control registers base address and size including > configuration space > - interrupts:interrupt generated by the device > +- dma-coherent: required if the device (or host emulation) accesses > memory > + cache-coherently, absent otherwise > + > +Linux implementation note: > + > +virtio devices not advertising the VIRTIO_F_IOMMU_PLATFORM flag have been > +implicitly assumed to be cache-coherent by Linux, and for legacy reasons this > +behaviour is likely to remain. If VIRTIO_F_IOMMU_PLATFORM is advertised, > then > +such assumptions cannot be relied upon and the "dma-coherent" property must > +accurately reflect the coherency of the device. > > Example: > > @@ -14,4 +24,5 @@ Example: > compatible = "virtio,mmio"; > reg = <0x3000 0x100>; > interrupts = <41>; > + dma-coherent; I think this is a sensible update to the binding and is independent of whatever we decide to do for IOMMUs and DMA on legacy devices. Acked-by: Will Deacon Will ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/9] virtio_pci: use shared interrupts for virtqueues
On Fri, Feb 03, 2017 at 05:47:41PM +0800, Jason Wang wrote: >> No, we need to allocate the array larger in that case as want proper >> names for the interrupts. > > Consider the case of !per_vq_vectors, the size of msix_names is 2, but > snprintf can do out of bound accessing here. (We name the msix shared by > virtqueues with something like "%s-virtqueues" before the patch). Yes, that's what I meant above - we need to allocate a large array starting with this patch. I'll fix it up for the next version. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/9] virtio_pci: use shared interrupts for virtqueues
On 2017年02月03日 16:26, Christoph Hellwig wrote: On Fri, Feb 03, 2017 at 03:54:54PM +0800, Jason Wang wrote: On 2017年01月27日 16:16, Christoph Hellwig wrote: + snprintf(vp_dev->msix_names[i + 1], +sizeof(*vp_dev->msix_names), "%s-%s", dev_name(&vp_dev->vdev.dev), names[i]); err = request_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec), - vring_interrupt, 0, - vp_dev->msix_names[msix_vec], - vqs[i]); + vring_interrupt, IRQF_SHARED, + vp_dev->msix_names[i + 1], vqs[i]); Do we need to check per_vq_vectors before dereferencing msix_names[i + 1] ? No, we need to allocate the array larger in that case as want proper names for the interrupts. Consider the case of !per_vq_vectors, the size of msix_names is 2, but snprintf can do out of bound accessing here. (We name the msix shared by virtqueues with something like "%s-virtqueues" before the patch). Thanks ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/9] virtio_pci: use shared interrupts for virtqueues
On Fri, Feb 03, 2017 at 03:54:54PM +0800, Jason Wang wrote: > On 2017年01月27日 16:16, Christoph Hellwig wrote: >> +snprintf(vp_dev->msix_names[i + 1], >> + sizeof(*vp_dev->msix_names), "%s-%s", >> dev_name(&vp_dev->vdev.dev), names[i]); >> err = request_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec), >> - vring_interrupt, 0, >> - vp_dev->msix_names[msix_vec], >> - vqs[i]); >> + vring_interrupt, IRQF_SHARED, >> + vp_dev->msix_names[i + 1], vqs[i]); > > Do we need to check per_vq_vectors before dereferencing msix_names[i + 1] ? No, we need to allocate the array larger in that case as want proper names for the interrupts. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/9] virtio_pci: remove struct virtio_pci_vq_info
On Fri, Feb 03, 2017 at 03:54:36PM +0800, Jason Wang wrote:
>> +list_for_each_entry(vq, &vp_dev->vdev.vqs, list) {
>> +if (vq->callback && vring_interrupt(irq, vq) == IRQ_HANDLED)
>
> The check of vq->callback seems redundant, we will check it soon in
> vring_interrupt().
Good point - I wanted to keep things exactly as-is and dіdn't notice
we were already protected.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 6/9] virtio: provide a method to get the IRQ affinity mask for a virtqueue
On 2017年01月27日 16:16, Christoph Hellwig wrote:
This basically passed up the pci_irq_get_affinity information through
virtio through an optional get_vq_affinity method. It is only implemented
by the PCI backend for now, and only when we use per-virtqueue IRQs.
Signed-off-by: Christoph Hellwig
---
Reviewed-by: Jason Wang
drivers/virtio/virtio_pci_common.c | 11 +++
drivers/virtio/virtio_pci_common.h | 2 ++
drivers/virtio/virtio_pci_legacy.c | 1 +
drivers/virtio/virtio_pci_modern.c | 2 ++
include/linux/virtio_config.h | 3 +++
5 files changed, 19 insertions(+)
diff --git a/drivers/virtio/virtio_pci_common.c
b/drivers/virtio/virtio_pci_common.c
index c244212..31ba259 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -361,6 +361,17 @@ int vp_set_vq_affinity(struct virtqueue *vq, int cpu)
return 0;
}
+const struct cpumask *vp_get_vq_affinity(struct virtio_device *vdev, int index)
+{
+ struct virtio_pci_device *vp_dev = to_vp_device(vdev);
+ unsigned int *map = vp_dev->msix_vector_map;
+
+ if (!map || map[index] == VIRTIO_MSI_NO_VECTOR)
+ return NULL;
+
+ return pci_irq_get_affinity(vp_dev->pci_dev, map[index]);
+}
+
#ifdef CONFIG_PM_SLEEP
static int virtio_pci_freeze(struct device *dev)
{
diff --git a/drivers/virtio/virtio_pci_common.h
b/drivers/virtio/virtio_pci_common.h
index a6ad9ec..ac8c9d7 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -108,6 +108,8 @@ const char *vp_bus_name(struct virtio_device *vdev);
*/
int vp_set_vq_affinity(struct virtqueue *vq, int cpu);
+const struct cpumask *vp_get_vq_affinity(struct virtio_device *vdev, int index);
+
#if IS_ENABLED(CONFIG_VIRTIO_PCI_LEGACY)
int virtio_pci_legacy_probe(struct virtio_pci_device *);
void virtio_pci_legacy_remove(struct virtio_pci_device *);
diff --git a/drivers/virtio/virtio_pci_legacy.c
b/drivers/virtio/virtio_pci_legacy.c
index 2ab6aee..f7362c5 100644
--- a/drivers/virtio/virtio_pci_legacy.c
+++ b/drivers/virtio/virtio_pci_legacy.c
@@ -190,6 +190,7 @@ static const struct virtio_config_ops virtio_pci_config_ops
= {
.finalize_features = vp_finalize_features,
.bus_name = vp_bus_name,
.set_vq_affinity = vp_set_vq_affinity,
+ .get_vq_affinity = vp_get_vq_affinity,
};
/* the PCI probing function */
diff --git a/drivers/virtio/virtio_pci_modern.c
b/drivers/virtio/virtio_pci_modern.c
index a7a0981..7bc3004 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -437,6 +437,7 @@ static const struct virtio_config_ops
virtio_pci_config_nodev_ops = {
.finalize_features = vp_finalize_features,
.bus_name = vp_bus_name,
.set_vq_affinity = vp_set_vq_affinity,
+ .get_vq_affinity = vp_get_vq_affinity,
};
static const struct virtio_config_ops virtio_pci_config_ops = {
@@ -452,6 +453,7 @@ static const struct virtio_config_ops virtio_pci_config_ops
= {
.finalize_features = vp_finalize_features,
.bus_name = vp_bus_name,
.set_vq_affinity = vp_set_vq_affinity,
+ .get_vq_affinity = vp_get_vq_affinity,
};
/**
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index 2ebe506..8355bab 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -58,6 +58,7 @@ struct irq_affinity;
* This returns a pointer to the bus name a la pci_name from which
* the caller can then copy.
* @set_vq_affinity: set the affinity for a virtqueue.
+ * @get_vq_affinity: get the affinity for a virtqueue (optional).
*/
typedef void vq_callback_t(struct virtqueue *);
struct virtio_config_ops {
@@ -77,6 +78,8 @@ struct virtio_config_ops {
int (*finalize_features)(struct virtio_device *vdev);
const char *(*bus_name)(struct virtio_device *vdev);
int (*set_vq_affinity)(struct virtqueue *vq, int cpu);
+ const struct cpumask *(*get_vq_affinity)(struct virtio_device *vdev,
+ int index);
};
/* If driver didn't advertise the feature, it will never appear. */
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 5/9] virtio: allow drivers to request IRQ affinity when creating VQs
On 2017年01月27日 16:16, Christoph Hellwig wrote: Add a struct irq_affinity pointer to the find_vqs methods, which if set is used to tell the PCI layer to create the MSI-X vectors for our I/O virtqueues with the proper affinity from the start. Compared to after the fact affinity hints this gives us an instantly working setup and allows to allocate the irq descritors node-local and avoid interconnect traffic. Last but not least this will allow blk-mq queues are created based on the interrupt affinity for storage drivers. Signed-off-by: Christoph Hellwig --- Reviewed-by: Jason Wang ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
