* Re: vhost-blk and qemu
From: Stefan Hajnoczi @ 2015-11-23 8:39 UTC (permalink / raw)
To: Mohan G; +Cc: virtualization@lists.linux-foundation.org
In-Reply-To: <1872664481.6220477.1447766581884.JavaMail.yahoo@mail.yahoo.com>
On Tue, Nov 17, 2015 at 9:23 PM, Mohan G via Virtualization
<virtualization@lists.linux-foundation.org> wrote:
> I am looking to experiment the vhost-blk stack. Can some one point me to the
> latest code version and the corresponding qemu version location.
> I am on centos 7 (3.10 ) kernel. I am hoping using the vhost-blk.ko and
> corresponding qemu version will get me started to measure some numbers.
vhost-blk is not under active development. New revisions of the
patches have not been posted for some time.
You may find the recent vhost-nvme patches from Ming Lin interesting instead:
http://permalink.gmane.org/gmane.linux.kernel.virtualization/26254
Stefan
^ permalink raw reply
* Re: [RFC PATCH 0/9] vhost-nvme: new qemu nvme backend using nvme target
From: Ming Lin @ 2015-11-23 8:17 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: virtualization, Christoph Hellwig, linux-nvme, qemu-devel
In-Reply-To: <56506D95.70101@redhat.com>
On Sat, 2015-11-21 at 14:11 +0100, Paolo Bonzini wrote:
>
> On 20/11/2015 01:20, Ming Lin wrote:
> > One improvment could be to use google's NVMe vendor extension that
> > I send in another thread, aslo here:
> > https://git.kernel.org/cgit/linux/kernel/git/mlin/linux.git/log/?h=nvme-google-ext
> >
> > Qemu side:
> > http://www.minggr.net/cgit/cgit.cgi/qemu/log/?h=vhost-nvme.0
> > Kernel side also here:
> > https://git.kernel.org/cgit/linux/kernel/git/mlin/linux.git/log/?h=vhost-nvme.0
>
> How much do you get with vhost-nvme plus vendor extension, compared to
> 190 MB/s for QEMU?
There is still some bug. I'll update.
>
> Note that in all likelihood, QEMU can actually do better than 190 MB/s,
> and gain more parallelism too, by moving the processing of the
> ioeventfds to a separate thread. This is similar to
> hw/block/dataplane/virtio-blk.c.
>
> It's actually pretty easy to do. Even though
> hw/block/dataplane/virtio-blk.c is still using some old APIs, all memory
> access in QEMU is now thread-safe. I have pending patches for 2.6 that
> cut that file down to a mere 200 lines of code, NVMe would probably be
> about the same.
Is there a git tree for your patches?
Did you mean some pseduo code as below?
1. need a iothread for each cq/sq?
2. need a AioContext for each cq/sq?
hw/block/nvme.c | 32 ++++++++++++++++++++++++++++++--
hw/block/nvme.h | 8 ++++++++
2 files changed, 38 insertions(+), 2 deletions(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index f27fd35..fed4827 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -28,6 +28,8 @@
#include "sysemu/sysemu.h"
#include "qapi/visitor.h"
#include "sysemu/block-backend.h"
+#include "sysemu/iothread.h"
+#include "qom/object_interfaces.h"
#include "nvme.h"
@@ -558,9 +560,22 @@ static void nvme_init_cq_eventfd(NvmeCQueue *cq)
uint16_t offset = (cq->cqid*2+1) * (4 << NVME_CAP_DSTRD(n->bar.cap));
event_notifier_init(&cq->notifier, 0);
- event_notifier_set_handler(&cq->notifier, nvme_cq_notifier);
memory_region_add_eventfd(&n->iomem,
0x1000 + offset, 4, false, 0, &cq->notifier);
+
+ object_initialize(&cq->internal_iothread_obj,
+ sizeof(cq->internal_iothread_obj),
+ TYPE_IOTHREAD);
+ user_creatable_complete(OBJECT(&cq->internal_iothread_obj), &error_abort);
+ cq->iothread = &cq->internal_iothread_obj;
+ cq->ctx = iothread_get_aio_context(cq->iothread);
+ //Question: Need a conf.blk for each cq/sq???
+ //blk_set_aio_context(cq->conf->conf.blk, cq->ctx);
+
+ aio_context_acquire(cq->ctx);
+ aio_set_event_notifier(cq->ctx, &cq->notifier, true,
+ nvme_cq_notifier);
+ aio_context_release(cq->ctx);
}
static void nvme_sq_notifier(EventNotifier *e)
@@ -578,9 +593,22 @@ static void nvme_init_sq_eventfd(NvmeSQueue *sq)
uint16_t offset = sq->sqid * 2 * (4 << NVME_CAP_DSTRD(n->bar.cap));
event_notifier_init(&sq->notifier, 0);
- event_notifier_set_handler(&sq->notifier, nvme_sq_notifier);
memory_region_add_eventfd(&n->iomem,
0x1000 + offset, 4, false, 0, &sq->notifier);
+
+ object_initialize(&sq->internal_iothread_obj,
+ sizeof(sq->internal_iothread_obj),
+ TYPE_IOTHREAD);
+ user_creatable_complete(OBJECT(&sq->internal_iothread_obj), &error_abort);
+ sq->iothread = &sq->internal_iothread_obj;
+ sq->ctx = iothread_get_aio_context(sq->iothread);
+ //Question: Need a conf.blk for each cq/sq???
+ //blk_set_aio_context(sq->conf->conf.blk, sq->ctx);
+
+ aio_context_acquire(sq->ctx);
+ aio_set_event_notifier(sq->ctx, &sq->notifier, true,
+ nvme_sq_notifier);
+ aio_context_release(sq->ctx);
}
static uint16_t nvme_set_db_memory(NvmeCtrl *n, const NvmeCmd *cmd)
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 608f202..171ee0b 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -667,6 +667,10 @@ typedef struct NvmeSQueue {
* do not go over this value will not result in MMIO writes (but will
* still write the tail pointer to the "db_addr" location above). */
uint64_t eventidx_addr;
+
+ IOThread *iothread;
+ IOThread internal_iothread_obj;
+ AioContext *ctx;
EventNotifier notifier;
} NvmeSQueue;
@@ -690,6 +694,10 @@ typedef struct NvmeCQueue {
* do not go over this value will not result in MMIO writes (but will
* still write the head pointer to the "db_addr" location above). */
uint64_t eventidx_addr;
+
+ IOThread *iothread;
+ IOThread internal_iothread_obj;
+ AioContext *ctx;
EventNotifier notifier;
} NvmeCQueue;
>
> Paolo
^ permalink raw reply related
* Re: [PATCH v3 0/3] virtio DMA API core stuff
From: Michael S. Tsirkin @ 2015-11-23 7:56 UTC (permalink / raw)
To: David Woodhouse
Cc: linux-s390, KVM, Marcel Apfelbaum, Benjamin Herrenschmidt,
Sebastian Ott, linux-kernel@vger.kernel.org, Andy Lutomirski,
Christian Borntraeger, Joerg Roedel, Martin Schwidefsky,
Paolo Bonzini, Linux Virtualization, Christoph Hellwig
In-Reply-To: <adab330424e9faea591560c4097fc644.squirrel@twosheds.infradead.org>
On Sun, Nov 22, 2015 at 10:21:34PM -0000, David Woodhouse wrote:
>
>
> > There's that, and there's an "I care about security, but
> > do not want to burn up cycles on fake protections that
> > do not work" case.
>
> It would seem to make most sense for this use case simply *not* to expose
> virtio devices to guests as being behind an IOMMU at all. Sure, there are
> esoteric use cases where the guest actually nests and runs further guests
> inside itself and wants to pass through the virtio devices from the real
> hardware host. But presumably those configurations will have multiple
> virtio devices assigned by the host anyway, and further tweaking the
> configuration to put them behind an IOMMU shouldn't be hard.
Unfortunately it's a no-go: this breaks the much less esoteric usecase
of DPDK: using virtio devices with userspace drivers.
Well - not breaks as such as this doesn't currently work,
but this approach would prevent us from making it work.
>
> --
> dwmw2
^ permalink raw reply
* Re: [PATCH v3 0/3] virtio DMA API core stuff
From: David Woodhouse @ 2015-11-22 22:21 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-s390, KVM, Marcel Apfelbaum, Benjamin Herrenschmidt,
Sebastian Ott, linux-kernel@vger.kernel.org, Andy Lutomirski,
Christian Borntraeger, Joerg Roedel, Martin Schwidefsky,
Paolo Bonzini, Linux Virtualization, David Woodhouse,
Christoph Hellwig
In-Reply-To: <20151122231622-mutt-send-email-mst@redhat.com>
> There's that, and there's an "I care about security, but
> do not want to burn up cycles on fake protections that
> do not work" case.
It would seem to make most sense for this use case simply *not* to expose
virtio devices to guests as being behind an IOMMU at all. Sure, there are
esoteric use cases where the guest actually nests and runs further guests
inside itself and wants to pass through the virtio devices from the real
hardware host. But presumably those configurations will have multiple
virtio devices assigned by the host anyway, and further tweaking the
configuration to put them behind an IOMMU shouldn't be hard.
--
dwmw2
^ permalink raw reply
* Re: [PATCH v3 0/3] virtio DMA API core stuff
From: David Woodhouse @ 2015-11-22 22:21 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-s390, KVM, Marcel Apfelbaum, Benjamin Herrenschmidt,
Sebastian Ott, linux-kernel@vger.kernel.org, Andy Lutomirski,
Christian Borntraeger, Joerg Roedel, Martin Schwidefsky,
Paolo Bonzini, Linux Virtualization, David Woodhouse,
Christoph Hellwig
In-Reply-To: <20151122231622-mutt-send-email-mst@redhat.com>
> There's that, and there's an "I care about security, but
> do not want to burn up cycles on fake protections that
> do not work" case.
It would seem to make most sense for this use case simply *not* to expose
virtio devices to guests as being behind an IOMMU at all. Sure, there are
esoteric use cases where the guest actually nests and runs further guests
inside itself and wants to pass through the virtio devices from the real
hardware host. But presumably those configurations will have multiple
virtio devices assigned by the host anyway, and further tweaking the
configuration to put them behind an IOMMU shouldn't be hard.
--
dwmw2
^ permalink raw reply
* Re: [PATCH v3 0/3] virtio DMA API core stuff
From: Michael S. Tsirkin @ 2015-11-22 22:11 UTC (permalink / raw)
To: David Woodhouse
Cc: linux-s390, Joerg Roedel, KVM, Benjamin Herrenschmidt,
Sebastian Ott, linux-kernel@vger.kernel.org, Andy Lutomirski,
Christian Borntraeger, Andy Lutomirski, marcel, Paolo Bonzini,
Linux Virtualization, Christoph Hellwig, Martin Schwidefsky
In-Reply-To: <1448207661.89124.50.camel@infradead.org>
On Sun, Nov 22, 2015 at 03:54:21PM +0000, David Woodhouse wrote:
> On Sun, 2015-11-22 at 15:06 +0200, Marcel Apfelbaum wrote:
> >
> >
> > I tried to generate a DMAR table that excludes some devices from
> > IOMMU translation, however it does not help.
> >
> > The reason is, as far as I understand, that Linux kernel does
> > not allow any device being outside an IOMMU scope if the
> > iommu kernel option is activated.
> >
> > Does anybody know if it is "by design" or is simply an uncommon
> > configuration?
> > (some devices in an IOMMU scope, while others outside *any* IOMMU
> > scope)
>
> That's a kernel bug in the way it handles per-device DMA operations. Or
> more to the point, in the way it doesn't — the non-translated devices
> end up being pointed to the intel_dma_ops despite the fact they
> shouldn't be. I'm working on that...
>
> --
> dwmw2
>
Interesting. This seems to imply such configurations aren't
common, so I wonder whether other guest OS-es treat them
correctly.
If many of them are, we probably shouldn't use this in QEMU:
we care about guests actually working :)
--
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v3 0/3] virtio DMA API core stuff
From: Michael S. Tsirkin @ 2015-11-22 21:52 UTC (permalink / raw)
To: David Woodhouse
Cc: linux-s390, KVM, Marcel Apfelbaum, Benjamin Herrenschmidt,
Sebastian Ott, linux-kernel@vger.kernel.org, Andy Lutomirski,
Christian Borntraeger, Joerg Roedel, Martin Schwidefsky,
Paolo Bonzini, Linux Virtualization, Christoph Hellwig
In-Reply-To: <1448207908.89124.54.camel@infradead.org>
On Sun, Nov 22, 2015 at 03:58:28PM +0000, David Woodhouse wrote:
> On Fri, 2015-11-20 at 10:21 +0200, Michael S. Tsirkin wrote:
> >
> > David, there are two things a hypervisor needs to tell the guest.
> > 1. The actual device is behind an IOMMU. This is what you
> > are suggesting we use DMAR for.
> > 2. Using IOMMU from kernel (as opposed to from userspace with VFIO)
> > actually adds security. For exising virtio devices on KVM,
> > the answer is no. And DMAR has no way to reflect that.
>
> Using the IOMMU from the kernel *always* adds security. It protects
> against device driver (and device) bugs which can be made exploitable
> by allowing DMA to anywhere in the system.
No - speaking about QEMU/KVM here - you are not "allowing" DMA - by
programming the virtual IOMMU you are asking the hypervisor nicely to do
that. If it's buggy, it can ignore you and there's nothing you can do.
As with any random change in the system, some bugs might get masked and
become non-exploitable, but then some other bugs might surface and
become exploitable.
I gather that e.g. Xen is different.
> Sure, there are classes of that which are far more interesting, for
> example where you give the whole device to a guest and let it load the
> firmware. But "we trust the hypervisor" and "we trust the hardware" are
> not *so* far apart conceptually.
Depends on the hypervisor I guess. At least for QEMU/KVM, one conceptual
difference is that we actually could have the hypervisor tell us whether
a specific device has to be trusted, or can be protected against, and
user can actually read the code and verify that QEMU is doing the right
thing.
Hardware is closed source so harder to trust.
> Hell, with ATS you *still* have to trust the hardware to a large
> extent.
>
> I really think that something like the proposed DMA_ATTR_IOMMU_BYPASS
> should suffice
I'm not sure how that is supposed to be used - does
the driver request DMA_ATTR_IOMMU_BYPASS at setup time?
If yes then I think that will work for virtio -
we can just set that in the driver.
> for the "who cares about security; we want performance"
> case.
>
> --
> dwmw2
>
There's that, and there's an "I care about security, but
do not want to burn up cycles on fake protections that
do not work" case.
--
MST
^ permalink raw reply
* Re: [PATCH v3 0/3] virtio DMA API core stuff
From: Marcel Apfelbaum @ 2015-11-22 17:04 UTC (permalink / raw)
To: David Woodhouse, Joerg Roedel, Michael S. Tsirkin
Cc: linux-s390, KVM, Benjamin Herrenschmidt, Sebastian Ott,
linux-kernel@vger.kernel.org, Andy Lutomirski,
Christian Borntraeger, Andy Lutomirski, Martin Schwidefsky,
Paolo Bonzini, Linux Virtualization, Christoph Hellwig
In-Reply-To: <1448207661.89124.50.camel@infradead.org>
On 11/22/2015 05:54 PM, David Woodhouse wrote:
> On Sun, 2015-11-22 at 15:06 +0200, Marcel Apfelbaum wrote:
>>
>>
>> I tried to generate a DMAR table that excludes some devices from
>> IOMMU translation, however it does not help.
>>
>> The reason is, as far as I understand, that Linux kernel does
>> not allow any device being outside an IOMMU scope if the
>> iommu kernel option is activated.
>>
>> Does anybody know if it is "by design" or is simply an uncommon
>> configuration?
>> (some devices in an IOMMU scope, while others outside *any* IOMMU
>> scope)
>
> That's a kernel bug in the way it handles per-device DMA operations. Or
> more to the point, in the way it doesn't — the non-translated devices
> end up being pointed to the intel_dma_ops despite the fact they
> shouldn't be. I'm working on that...
>
Hi David,
Thank you for the fast response.
Sadly I am not familiar with the DMA/IOMMU code to contribute
with a sane idea, but I'll gladly test it.
If you lack the time and have an idea to share, I can give it a try though.
Thanks,
Marcel
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v3 0/3] virtio DMA API core stuff
From: David Woodhouse @ 2015-11-22 15:58 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-s390, KVM, Marcel Apfelbaum, Benjamin Herrenschmidt,
Sebastian Ott, linux-kernel@vger.kernel.org, Andy Lutomirski,
Christian Borntraeger, Joerg Roedel, Martin Schwidefsky,
Paolo Bonzini, Linux Virtualization, Christoph Hellwig
In-Reply-To: <20151120085658-mutt-send-email-mst@redhat.com>
[-- Attachment #1.1: Type: text/plain, Size: 1119 bytes --]
On Fri, 2015-11-20 at 10:21 +0200, Michael S. Tsirkin wrote:
>
> David, there are two things a hypervisor needs to tell the guest.
> 1. The actual device is behind an IOMMU. This is what you
> are suggesting we use DMAR for.
> 2. Using IOMMU from kernel (as opposed to from userspace with VFIO)
> actually adds security. For exising virtio devices on KVM,
> the answer is no. And DMAR has no way to reflect that.
Using the IOMMU from the kernel *always* adds security. It protects
against device driver (and device) bugs which can be made exploitable
by allowing DMA to anywhere in the system.
Sure, there are classes of that which are far more interesting, for
example where you give the whole device to a guest and let it load the
firmware. But "we trust the hypervisor" and "we trust the hardware" are
not *so* far apart conceptually.
Hell, with ATS you *still* have to trust the hardware to a large
extent.
I really think that something like the proposed DMA_ATTR_IOMMU_BYPASS
should suffice for the "who cares about security; we want performance"
case.
--
dwmw2
[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v3 0/3] virtio DMA API core stuff
From: David Woodhouse @ 2015-11-22 15:54 UTC (permalink / raw)
To: marcel, Joerg Roedel, Michael S. Tsirkin
Cc: linux-s390, KVM, Benjamin Herrenschmidt, Sebastian Ott,
linux-kernel@vger.kernel.org, Andy Lutomirski,
Christian Borntraeger, Andy Lutomirski, Martin Schwidefsky,
Paolo Bonzini, Linux Virtualization, Christoph Hellwig
In-Reply-To: <5651BDC0.2040206@gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 785 bytes --]
On Sun, 2015-11-22 at 15:06 +0200, Marcel Apfelbaum wrote:
>
>
> I tried to generate a DMAR table that excludes some devices from
> IOMMU translation, however it does not help.
>
> The reason is, as far as I understand, that Linux kernel does
> not allow any device being outside an IOMMU scope if the
> iommu kernel option is activated.
>
> Does anybody know if it is "by design" or is simply an uncommon
> configuration?
> (some devices in an IOMMU scope, while others outside *any* IOMMU
> scope)
That's a kernel bug in the way it handles per-device DMA operations. Or
more to the point, in the way it doesn't — the non-translated devices
end up being pointed to the intel_dma_ops despite the fact they
shouldn't be. I'm working on that...
--
dwmw2
[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v3 0/3] virtio DMA API core stuff
From: Marcel Apfelbaum @ 2015-11-22 13:06 UTC (permalink / raw)
To: Joerg Roedel, Michael S. Tsirkin
Cc: linux-s390, KVM, Benjamin Herrenschmidt, Sebastian Ott,
linux-kernel@vger.kernel.org, Andy Lutomirski,
Christian Borntraeger, Andy Lutomirski, Martin Schwidefsky,
Paolo Bonzini, Linux Virtualization, David Woodhouse,
Christoph Hellwig
In-Reply-To: <20151108114946.GG2255@suse.de>
On 11/08/2015 01:49 PM, Joerg Roedel wrote:
> On Sun, Nov 08, 2015 at 12:37:47PM +0200, Michael S. Tsirkin wrote:
>> I have no problem with that. For example, can we teach
>> the DMA API on intel x86 to use PT for virtio by default?
>> That would allow merging Andy's patches with
>> full compatibility with old guests and hosts.
>
> Well, the only incompatibility comes from an experimental qemu feature,
> more explicitly from a bug in that features implementation. So why
> should we work around that in the kernel? I think it is not too hard to
> fix qemu to generate a correct DMAR table which excludes the virtio
> devices from iommu translation.
Hi,
I tried to generate a DMAR table that excludes some devices from
IOMMU translation, however it does not help.
The reason is, as far as I understand, that Linux kernel does
not allow any device being outside an IOMMU scope if the
iommu kernel option is activated.
Does anybody know if it is "by design" or is simply an uncommon configuration?
(some devices in an IOMMU scope, while others outside *any* IOMMU scope)
Thanks,
Marcel
>
>
> Joerg
>
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
>
^ permalink raw reply
* Re: [PATCH -qemu] nvme: support Google vendor extension
From: Ming Lin @ 2015-11-22 7:45 UTC (permalink / raw)
To: Paolo Bonzini
Cc: fes, axboe, tytso, qemu-devel, linux-nvme, virtualization,
keith.busch, Rob Nelson, Christoph Hellwig, Mihai Rusu
In-Reply-To: <565069F0.5000805@redhat.com>
On Sat, 2015-11-21 at 13:56 +0100, Paolo Bonzini wrote:
>
> On 21/11/2015 00:05, Ming Lin wrote:
> > [ 1.752129] Freeing unused kernel memory: 420K (ffff880001b97000 - ffff880001c00000)
> > [ 1.986573] clocksource: tsc: mask: 0xffffffffffffffff max_cycles: 0x30e5c9bbf83, max_idle_ns: 440795378954 ns
> > [ 1.988187] clocksource: Switched to clocksource tsc
> > [ 3.235423] clocksource: timekeeping watchdog: Marking clocksource 'tsc' as unstable because the skew is too large:
> > [ 3.358713] clocksource: 'refined-jiffies' wd_now: fffeddf3 wd_last: fffedd76 mask: ffffffff
> > [ 3.410013] clocksource: 'tsc' cs_now: 3c121d4ec cs_last: 340888eb7 mask: ffffffffffffffff
> > [ 3.450026] clocksource: Switched to clocksource refined-jiffies
> > [ 7.696769] Adding 392188k swap on /dev/vda5. Priority:-1 extents:1 across:392188k
> > [ 7.902174] EXT4-fs (vda1): re-mounted. Opts: (null)
> > [ 8.734178] EXT4-fs (vda1): re-mounted. Opts: errors=remount-ro
> >
> > Then it doesn't response input for almost 1 minute.
> > Without this patch, kernel loads quickly.
>
> Interesting. I guess there's time to debug it, since QEMU 2.6 is still
> a few months away. In the meanwhile we can apply your patch as is,
> apart from disabling the "if (new_head >= cq->size)" and the similar
> one for "if (new_ tail >= sq->size".
>
> But, I have a possible culprit. In your nvme_cq_notifier you are not doing the
> equivalent of:
>
> start_sqs = nvme_cq_full(cq) ? 1 : 0;
> cq->head = new_head;
> if (start_sqs) {
> NvmeSQueue *sq;
> QTAILQ_FOREACH(sq, &cq->sq_list, entry) {
> timer_mod(sq->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 500);
> }
> timer_mod(cq->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 500);
> }
>
> Instead, you are just calling nvme_post_cqes, which is the equivalent of
>
> timer_mod(cq->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 500);
>
> Adding a loop to nvme_cq_notifier, and having it call nvme_process_sq, might
> fix the weird 1-minute delay.
I found it.
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 31572f2..f27fd35 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -548,6 +548,7 @@ static void nvme_cq_notifier(EventNotifier *e)
NvmeCQueue *cq =
container_of(e, NvmeCQueue, notifier);
+ event_notifier_test_and_clear(&cq->notifier);
nvme_post_cqes(cq);
}
@@ -567,6 +568,7 @@ static void nvme_sq_notifier(EventNotifier *e)
NvmeSQueue *sq =
container_of(e, NvmeSQueue, notifier);
+ event_notifier_test_and_clear(&sq->notifier);
nvme_process_sq(sq);
}
Here is new performance number:
qemu-nvme + google-ext + eventfd: 294MB/s
virtio-blk: 344MB/s
virtio-scsi: 296MB/s
It's almost same as virtio-scsi. Nice.
^ permalink raw reply related
* Re: [RFC PATCH 0/9] vhost-nvme: new qemu nvme backend using nvme target
From: Paolo Bonzini @ 2015-11-21 13:11 UTC (permalink / raw)
To: Ming Lin, linux-nvme; +Cc: virtualization, Christoph Hellwig, qemu-devel
In-Reply-To: <1447978868-17138-1-git-send-email-mlin@kernel.org>
On 20/11/2015 01:20, Ming Lin wrote:
> One improvment could be to use google's NVMe vendor extension that
> I send in another thread, aslo here:
> https://git.kernel.org/cgit/linux/kernel/git/mlin/linux.git/log/?h=nvme-google-ext
>
> Qemu side:
> http://www.minggr.net/cgit/cgit.cgi/qemu/log/?h=vhost-nvme.0
> Kernel side also here:
> https://git.kernel.org/cgit/linux/kernel/git/mlin/linux.git/log/?h=vhost-nvme.0
How much do you get with vhost-nvme plus vendor extension, compared to
190 MB/s for QEMU?
Note that in all likelihood, QEMU can actually do better than 190 MB/s,
and gain more parallelism too, by moving the processing of the
ioeventfds to a separate thread. This is similar to
hw/block/dataplane/virtio-blk.c.
It's actually pretty easy to do. Even though
hw/block/dataplane/virtio-blk.c is still using some old APIs, all memory
access in QEMU is now thread-safe. I have pending patches for 2.6 that
cut that file down to a mere 200 lines of code, NVMe would probably be
about the same.
Paolo
^ permalink raw reply
* Re: [PATCH -qemu] nvme: support Google vendor extension
From: Paolo Bonzini @ 2015-11-21 12:56 UTC (permalink / raw)
To: Ming Lin
Cc: fes, axboe, tytso, qemu-devel, linux-nvme, virtualization,
keith.busch, Rob Nelson, Christoph Hellwig, Mihai Rusu
In-Reply-To: <1448060745.6565.1.camel@ssi>
On 21/11/2015 00:05, Ming Lin wrote:
> [ 1.752129] Freeing unused kernel memory: 420K (ffff880001b97000 - ffff880001c00000)
> [ 1.986573] clocksource: tsc: mask: 0xffffffffffffffff max_cycles: 0x30e5c9bbf83, max_idle_ns: 440795378954 ns
> [ 1.988187] clocksource: Switched to clocksource tsc
> [ 3.235423] clocksource: timekeeping watchdog: Marking clocksource 'tsc' as unstable because the skew is too large:
> [ 3.358713] clocksource: 'refined-jiffies' wd_now: fffeddf3 wd_last: fffedd76 mask: ffffffff
> [ 3.410013] clocksource: 'tsc' cs_now: 3c121d4ec cs_last: 340888eb7 mask: ffffffffffffffff
> [ 3.450026] clocksource: Switched to clocksource refined-jiffies
> [ 7.696769] Adding 392188k swap on /dev/vda5. Priority:-1 extents:1 across:392188k
> [ 7.902174] EXT4-fs (vda1): re-mounted. Opts: (null)
> [ 8.734178] EXT4-fs (vda1): re-mounted. Opts: errors=remount-ro
>
> Then it doesn't response input for almost 1 minute.
> Without this patch, kernel loads quickly.
Interesting. I guess there's time to debug it, since QEMU 2.6 is still
a few months away. In the meanwhile we can apply your patch as is,
apart from disabling the "if (new_head >= cq->size)" and the similar
one for "if (new_ tail >= sq->size".
But, I have a possible culprit. In your nvme_cq_notifier you are not doing the
equivalent of:
start_sqs = nvme_cq_full(cq) ? 1 : 0;
cq->head = new_head;
if (start_sqs) {
NvmeSQueue *sq;
QTAILQ_FOREACH(sq, &cq->sq_list, entry) {
timer_mod(sq->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 500);
}
timer_mod(cq->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 500);
}
Instead, you are just calling nvme_post_cqes, which is the equivalent of
timer_mod(cq->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 500);
Adding a loop to nvme_cq_notifier, and having it call nvme_process_sq, might
fix the weird 1-minute delay.
Paolo
> void memory_region_add_eventfd(MemoryRegion *mr,
> hwaddr addr,
> unsigned size,
> bool match_data,
> uint64_t data,
> EventNotifier *e)
>
> Could you help to explain what "match_data" and "data" mean?
If match_data is true, the eventfd is only signalled if "data" is being written to memory.
Paolo
^ permalink raw reply
* Re: [PATCH -qemu] nvme: support Google vendor extension
From: Ming Lin @ 2015-11-20 23:05 UTC (permalink / raw)
To: Paolo Bonzini
Cc: fes, keith.busch, tytso, qemu-devel, linux-nvme, virtualization,
axboe, Rob Nelson, Christoph Hellwig, Mihai Rusu
In-Reply-To: <564EE0A0.1020800@redhat.com>
On Fri, 2015-11-20 at 09:58 +0100, Paolo Bonzini wrote:
>
> On 20/11/2015 09:11, Ming Lin wrote:
> > On Thu, 2015-11-19 at 11:37 +0100, Paolo Bonzini wrote:
> >>
> >> On 18/11/2015 06:47, Ming Lin wrote:
> >>> @@ -726,7 +798,11 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
> >>> }
> >>>
> >>> start_sqs = nvme_cq_full(cq) ? 1 : 0;
> >>> - cq->head = new_head;
> >>> + /* When the mapped pointer memory area is setup, we don't rely on
> >>> + * the MMIO written values to update the head pointer. */
> >>> + if (!cq->db_addr) {
> >>> + cq->head = new_head;
> >>> + }
> >>
> >> You are still checking
> >>
> >> if (new_head >= cq->size) {
> >> return;
> >> }
> >>
> >> above. I think this is incorrect when the extension is present, and
> >> furthermore it's the only case where val is being used.
> >>
> >> If you're not using val, you could use ioeventfd for the MMIO. An
> >> ioeventfd cuts the MMIO cost by at least 55% and up to 70%. Here are
> >> quick and dirty measurements from kvm-unit-tests's vmexit.flat
> >> benchmark, on two very different machines:
> >>
> >> Haswell-EP Ivy Bridge i7
> >> MMIO memory write 5100 -> 2250 (55%) 7000 -> 3000 (58%)
> >> I/O port write 3800 -> 1150 (70%) 4100 -> 1800 (57%)
> >>
> >> You would need to allocate two eventfds for each qid, one for the sq and
> >> one for the cq. Also, processing the queues is now bounced to the QEMU
> >> iothread, so you can probably get rid of sq->timer and cq->timer.
> >
> > Here is a quick try.
> > Too late now, I'll test it morning.
> >
> > Do you see obvious problem?
> >
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 3e1c38d..d28690d 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -543,6 +543,44 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> > return NVME_SUCCESS;
> > }
> >
> > +static void nvme_cq_notifier(EventNotifier *e)
> > +{
> > + NvmeCQueue *cq =
> > + container_of(e, NvmeCQueue, notifier);
> > +
> > + nvme_post_cqes(cq);
> > +}
> > +
> > +static void nvme_init_cq_eventfd(NvmeCQueue *cq)
> > +{
> > + NvmeCtrl *n = cq->ctrl;
> > + uint16_t offset = (cq->cqid*2+1) * NVME_CAP_DSTRD(n->bar.cap);
> > +
> > + event_notifier_init(&cq->notifier, 0);
> > + event_notifier_set_handler(&cq->notifier, nvme_cq_notifier);
> > + memory_region_add_eventfd(&n->iomem,
> > + 0x1000 + offset, 4, true, cq->cqid*2+1, &cq->notifier);
>
> should be 0x1000 + offset, 4, false, 0, &cq->notifier
>
> > +}
> > +
> > +static void nvme_sq_notifier(EventNotifier *e)
> > +{
> > + NvmeSQueue *sq =
> > + container_of(e, NvmeSQueue, notifier);
> > +
> > + nvme_process_sq(sq);
> > +}
> > +
> > +static void nvme_init_sq_eventfd(NvmeSQueue *sq)
> > +{
> > + NvmeCtrl *n = sq->ctrl;
> > + uint16_t offset = sq->sqid * 2 * NVME_CAP_DSTRD(n->bar.cap);
> > +
> > + event_notifier_init(&sq->notifier, 0);
> > + event_notifier_set_handler(&sq->notifier, nvme_sq_notifier);
> > + memory_region_add_eventfd(&n->iomem,
> > + 0x1000 + offset, 4, true, sq->sqid * 2, &sq->notifier);
>
> likewise should be 0x1000 + offset, 4, false, 0, &sq->notifier
It works. But for some unknown reason, when boots guest kernel, it stuck
for almost 1 minute.
[ 1.752129] Freeing unused kernel memory: 420K (ffff880001b97000 - ffff880001c00000)
[ 1.986573] clocksource: tsc: mask: 0xffffffffffffffff max_cycles: 0x30e5c9bbf83, max_idle_ns: 440795378954 ns
[ 1.988187] clocksource: Switched to clocksource tsc
[ 3.235423] clocksource: timekeeping watchdog: Marking clocksource 'tsc' as unstable because the skew is too large:
[ 3.358713] clocksource: 'refined-jiffies' wd_now: fffeddf3 wd_last: fffedd76 mask: ffffffff
[ 3.410013] clocksource: 'tsc' cs_now: 3c121d4ec cs_last: 340888eb7 mask: ffffffffffffffff
[ 3.450026] clocksource: Switched to clocksource refined-jiffies
[ 7.696769] Adding 392188k swap on /dev/vda5. Priority:-1 extents:1 across:392188k
[ 7.902174] EXT4-fs (vda1): re-mounted. Opts: (null)
[ 8.734178] EXT4-fs (vda1): re-mounted. Opts: errors=remount-ro
Then it doesn't response input for almost 1 minute.
Without this patch, kernel loads quickly.
[ 1.351095] Freeing unused kernel memory: 2968K (ffffffff81d49000 - ffffffff8202f000)
[ 1.352039] Write protecting the kernel read-only data: 12288k
[ 1.353340] Freeing unused kernel memory: 216K (ffff8800017ca000 - ffff880001800000)
[ 1.354670] Freeing unused kernel memory: 420K (ffff880001b97000 - ffff880001c00000)
[ 1.796272] Adding 392188k swap on /dev/vda5. Priority:-1 extents:1 across:392188k
[ 1.809579] EXT4-fs (vda1): re-mounted. Opts: (null)
[ 1.864834] EXT4-fs (vda1): re-mounted. Opts: errors=remount-ro
[ 1.964181] clocksource: tsc: mask: 0xffffffffffffffff max_cycles: 0x30e58d9c595, max_idle_ns: 440795220169 ns
[ 1.965610] clocksource: Switched to clocksource tsc
[ 2.377965] random: dd urandom read with 19 bits of entropy available
void memory_region_add_eventfd(MemoryRegion *mr,
hwaddr addr,
unsigned size,
bool match_data,
uint64_t data,
EventNotifier *e)
Could you help to explain what "match_data" and "data" mean?
I use a real NVMe device as backend.
-drive file=/dev/nvme0n1,format=raw,if=none,id=D22 \
-device nvme,drive=D22,serial=1234
Here is the test results:
local NVMe: 860MB/s
qemu-nvme: 108MB/s
qemu-nvme+google-ext: 140MB/s
qemu-nvme-google-ext+eventfd: 190MB/s
root@wheezy:~# cat test.job
[global]
bs=4k
ioengine=libaio
iodepth=64
direct=1
runtime=60
time_based
norandommap
group_reporting
gtod_reduce=1
numjobs=8
[job1]
filename=/dev/nvme0n1
rw=read
^ permalink raw reply
* Re: [PATCH] virtio_ring: Shadow available ring flags & index
From: Venkatesh Srinivas via Virtualization @ 2015-11-20 18:30 UTC (permalink / raw)
To: Xie, Huawei
Cc: KVM list, Michael S. Tsirkin,
virtualization@lists.linux-foundation.org, Venkatesh Srinivas,
luto@kernel.org, David Matlack, Paolo Bonzini
In-Reply-To: <C37D651A908B024F974696C65296B57B4B19D2A4@SHSMSX101.ccr.corp.intel.com>
On Thu, Nov 19, 2015 at 04:15:48PM +0000, Xie, Huawei wrote:
> On 11/18/2015 12:28 PM, Venkatesh Srinivas wrote:
> > On Tue, Nov 17, 2015 at 08:08:18PM -0800, Venkatesh Srinivas wrote:
> >> On Mon, Nov 16, 2015 at 7:46 PM, Xie, Huawei <huawei.xie@intel.com> wrote:
> >>
> >>> On 11/14/2015 7:41 AM, Venkatesh Srinivas wrote:
> >>>> On Wed, Nov 11, 2015 at 02:34:33PM +0200, Michael S. Tsirkin wrote:
> >>>>> On Tue, Nov 10, 2015 at 04:21:07PM -0800, Venkatesh Srinivas wrote:
> >>>>>> Improves cacheline transfer flow of available ring header.
> >>>>>>
> >>>>>> Virtqueues are implemented as a pair of rings, one producer->consumer
> >>>>>> avail ring and one consumer->producer used ring; preceding the
> >>>>>> avail ring in memory are two contiguous u16 fields -- avail->flags
> >>>>>> and avail->idx. A producer posts work by writing to avail->idx and
> >>>>>> a consumer reads avail->idx.
> >>>>>>
> >>>>>> The flags and idx fields only need to be written by a producer CPU
> >>>>>> and only read by a consumer CPU; when the producer and consumer are
> >>>>>> running on different CPUs and the virtio_ring code is structured to
> >>>>>> only have source writes/sink reads, we can continuously transfer the
> >>>>>> avail header cacheline between 'M' states between cores. This flow
> >>>>>> optimizes core -> core bandwidth on certain CPUs.
> >>>>>>
> >>>>>> (see: "Software Optimization Guide for AMD Family 15h Processors",
> >>>>>> Section 11.6; similar language appears in the 10h guide and should
> >>>>>> apply to CPUs w/ exclusive caches, using LLC as a transfer cache)
> >>>>>>
> >>>>>> Unfortunately the existing virtio_ring code issued reads to the
> >>>>>> avail->idx and read-modify-writes to avail->flags on the producer.
> >>>>>>
> >>>>>> This change shadows the flags and index fields in producer memory;
> >>>>>> the vring code now reads from the shadows and only ever writes to
> >>>>>> avail->flags and avail->idx, allowing the cacheline to transfer
> >>>>>> core -> core optimally.
> >>>>> Sounds logical, I'll apply this after a bit of testing
> >>>>> of my own, thanks!
> >>>> Thanks!
> >>> Venkatesh:
> >>> Is it that your patch only applies to CPUs w/ exclusive caches?
> >> No --- it applies when the inter-cache coherence flow is optimized by
> >> 'M' -> 'M' transfers and when producer reads might interfere w/
> >> consumer prefetchw/reads. The AMD Optimization guides have specific
> >> language on this subject, but other platforms may benefit.
> >> (see Intel #'s below)
> For core2core case(not HT paire), after consumer reads that M cache line
> for avail_idx, is that line still in the producer core's L1 data cache
> with state changing from M->O state?
Textbook MOESI would not allow that state combination -- when the consumer
gets the line in 'M' state, the producer cannot hold it in 'O' state.
On the AMD Piledriver, per the Optimization guide, I use PREFETCHW/Load to
get the line in 'M' state on the consumer (invalidating it in the Producer's
cache):
"* Use PREFETCHW on the consumer side, even if the consumer will not modify
the data"
That, plus the "Optimizing Inter-Core Data Transfer" section imply that
PREFETCHW + MOV will cause the consumer to load the line into 'M' state.
PREFETCHW was not available on Intel CPUs pre-Broadwell; from the public
documentation alone, I don't think we can tell what transition the producer's
cacheline undergoes on these cores. For that matter, the latest documentation
I can find (for Nehalem), indicated there was no 'O' state -- Nehalem
implemented MESIF, not MOESI.
HTH,
-- vs;
^ permalink raw reply
* Re: [PATCH -qemu] nvme: support Google vendor extension
From: Paolo Bonzini @ 2015-11-20 8:58 UTC (permalink / raw)
To: Ming Lin
Cc: fes, keith.busch, tytso, qemu-devel, linux-nvme, virtualization,
axboe, Rob Nelson, Christoph Hellwig, Mihai Rusu
In-Reply-To: <1448007096.3473.10.camel@hasee>
On 20/11/2015 09:11, Ming Lin wrote:
> On Thu, 2015-11-19 at 11:37 +0100, Paolo Bonzini wrote:
>>
>> On 18/11/2015 06:47, Ming Lin wrote:
>>> @@ -726,7 +798,11 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
>>> }
>>>
>>> start_sqs = nvme_cq_full(cq) ? 1 : 0;
>>> - cq->head = new_head;
>>> + /* When the mapped pointer memory area is setup, we don't rely on
>>> + * the MMIO written values to update the head pointer. */
>>> + if (!cq->db_addr) {
>>> + cq->head = new_head;
>>> + }
>>
>> You are still checking
>>
>> if (new_head >= cq->size) {
>> return;
>> }
>>
>> above. I think this is incorrect when the extension is present, and
>> furthermore it's the only case where val is being used.
>>
>> If you're not using val, you could use ioeventfd for the MMIO. An
>> ioeventfd cuts the MMIO cost by at least 55% and up to 70%. Here are
>> quick and dirty measurements from kvm-unit-tests's vmexit.flat
>> benchmark, on two very different machines:
>>
>> Haswell-EP Ivy Bridge i7
>> MMIO memory write 5100 -> 2250 (55%) 7000 -> 3000 (58%)
>> I/O port write 3800 -> 1150 (70%) 4100 -> 1800 (57%)
>>
>> You would need to allocate two eventfds for each qid, one for the sq and
>> one for the cq. Also, processing the queues is now bounced to the QEMU
>> iothread, so you can probably get rid of sq->timer and cq->timer.
>
> Here is a quick try.
> Too late now, I'll test it morning.
>
> Do you see obvious problem?
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 3e1c38d..d28690d 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -543,6 +543,44 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> return NVME_SUCCESS;
> }
>
> +static void nvme_cq_notifier(EventNotifier *e)
> +{
> + NvmeCQueue *cq =
> + container_of(e, NvmeCQueue, notifier);
> +
> + nvme_post_cqes(cq);
> +}
> +
> +static void nvme_init_cq_eventfd(NvmeCQueue *cq)
> +{
> + NvmeCtrl *n = cq->ctrl;
> + uint16_t offset = (cq->cqid*2+1) * NVME_CAP_DSTRD(n->bar.cap);
> +
> + event_notifier_init(&cq->notifier, 0);
> + event_notifier_set_handler(&cq->notifier, nvme_cq_notifier);
> + memory_region_add_eventfd(&n->iomem,
> + 0x1000 + offset, 4, true, cq->cqid*2+1, &cq->notifier);
should be 0x1000 + offset, 4, false, 0, &cq->notifier
> +}
> +
> +static void nvme_sq_notifier(EventNotifier *e)
> +{
> + NvmeSQueue *sq =
> + container_of(e, NvmeSQueue, notifier);
> +
> + nvme_process_sq(sq);
> +}
> +
> +static void nvme_init_sq_eventfd(NvmeSQueue *sq)
> +{
> + NvmeCtrl *n = sq->ctrl;
> + uint16_t offset = sq->sqid * 2 * NVME_CAP_DSTRD(n->bar.cap);
> +
> + event_notifier_init(&sq->notifier, 0);
> + event_notifier_set_handler(&sq->notifier, nvme_sq_notifier);
> + memory_region_add_eventfd(&n->iomem,
> + 0x1000 + offset, 4, true, sq->sqid * 2, &sq->notifier);
likewise should be 0x1000 + offset, 4, false, 0, &sq->notifier
Otherwise looks good!
Paolo
> +}
> +
> static uint16_t nvme_set_db_memory(NvmeCtrl *n, const NvmeCmd *cmd)
> {
> uint64_t db_addr = le64_to_cpu(cmd->prp1);
> @@ -565,6 +603,7 @@ static uint16_t nvme_set_db_memory(NvmeCtrl *n, const NvmeCmd *cmd)
> /* Submission queue tail pointer location, 2 * QID * stride. */
> sq->db_addr = db_addr + 2 * i * 4;
> sq->eventidx_addr = eventidx_addr + 2 * i * 4;
> + nvme_init_sq_eventfd(sq);
> }
>
> if (cq != NULL) {
> @@ -572,6 +611,7 @@ static uint16_t nvme_set_db_memory(NvmeCtrl *n, const NvmeCmd *cmd)
> */
> cq->db_addr = db_addr + (2 * i + 1) * 4;
> cq->eventidx_addr = eventidx_addr + (2 * i + 1) * 4;
> + nvme_init_cq_eventfd(cq);
> }
> }
> return NVME_SUCCESS;
> @@ -793,7 +833,7 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
> }
>
> cq = n->cq[qid];
> - if (new_head >= cq->size) {
> + if (!cq->db_addr && new_head >= cq->size) {
> return;
> }
>
> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> index 82aeab4..608f202 100644
> --- a/hw/block/nvme.h
> +++ b/hw/block/nvme.h
> @@ -667,6 +667,7 @@ typedef struct NvmeSQueue {
> * do not go over this value will not result in MMIO writes (but will
> * still write the tail pointer to the "db_addr" location above). */
> uint64_t eventidx_addr;
> + EventNotifier notifier;
> } NvmeSQueue;
>
> typedef struct NvmeCQueue {
> @@ -689,6 +690,7 @@ typedef struct NvmeCQueue {
> * do not go over this value will not result in MMIO writes (but will
> * still write the head pointer to the "db_addr" location above). */
> uint64_t eventidx_addr;
> + EventNotifier notifier;
> } NvmeCQueue;
>
> typedef struct NvmeNamespace {
>
>>
>> Paolo
^ permalink raw reply
* Re: [PATCH v3 0/3] virtio DMA API core stuff
From: Michael S. Tsirkin @ 2015-11-20 8:34 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: linux-s390, KVM, Sebastian Ott, linux-kernel@vger.kernel.org,
Andy Lutomirski, Christian Borntraeger, Joerg Roedel,
Martin Schwidefsky, Paolo Bonzini, Linux Virtualization,
David Woodhouse, Christoph Hellwig
In-Reply-To: <1447988199.3016.2.camel@kernel.crashing.org>
On Fri, Nov 20, 2015 at 01:56:39PM +1100, Benjamin Herrenschmidt wrote:
> On Thu, 2015-11-19 at 23:38 +0000, David Woodhouse wrote:
> >
> > I understand that POWER and other platforms don't currently have a
> > clean way to indicate that certain device don't have translation. And I
> > understand that we may end up with a *quirk* which ensures that the DMA
> > API does the right thing (i.e. nothing) in certain cases.
> >
> > But we should *NOT* be involving the virtio device drivers in that
> > quirk, in any way. And putting a feature bit in the virtio device
> > itself doesn't seem at all sane either.
> >
> > Bear in mind that qemu-system-x86_64 currently has the *same* problem
> > with assigned physical devices. It's claiming they're translated, and
> > they're not.
>
> It's not that clear but yeah ... as I mentioned, I can't find a
> way to do that quirk that won't break when we want to actually use
> the iommu...
>
> Ben.
Yes, I am not at all sure we need a quirk for assigned devices.
Better teach QEMU to make iommu work for them.
--
MST
^ permalink raw reply
* Re: [PATCH v3 0/3] virtio DMA API core stuff
From: Michael S. Tsirkin @ 2015-11-20 8:21 UTC (permalink / raw)
To: David Woodhouse
Cc: linux-s390, KVM, Marcel Apfelbaum, Benjamin Herrenschmidt,
Sebastian Ott, linux-kernel@vger.kernel.org, Andy Lutomirski,
Christian Borntraeger, Joerg Roedel, Martin Schwidefsky,
Paolo Bonzini, Linux Virtualization, Christoph Hellwig
In-Reply-To: <1447976286.145626.122.camel@infradead.org>
On Thu, Nov 19, 2015 at 11:38:06PM +0000, David Woodhouse wrote:
> On Thu, 2015-11-19 at 13:59 -0800, Andy Lutomirski wrote:
> >
> > >
> > > So thinking hard about it, I don't see any real drawbacks to making this
> > > conditional on a new feature bit, that Xen can then set..
> >
> > Can you elaborate? If I run QEMU, hosting Xen, hosting Linux, and the
> > virtio device is provided by QEMU, then how does Xen set the bit?
> > Similarly, how would Xen set the bit for a real physical device?
>
> Right. This is *not* a fundamental characteristic of the device. This
> is all about how your *particular* hypervisor (in the set of turtles-
> all-the-way-down) happened to expose the thing to you.
>
> This is why it lives in the DMAR table, in the Intel world, which
> *tells* you which devices are behind which IOMMU (and which are not).
David, there are two things a hypervisor needs to tell the guest.
1. The actual device is behind an IOMMU. This is what you
are suggesting we use DMAR for.
2. Using IOMMU from kernel (as opposed to from userspace with VFIO)
actually adds security. For exising virtio devices on KVM,
the answer is no. And DMAR has no way to reflect that.
Question 2 only makes sense if you answer yes to question 1 and if user
wants protection from malicious devices with iommu=on, and
if you care about getting good performance from *other*
devices. And what guest would do is use 1:1 for the
devices where answer 2 is "no".
Maybe for now I should just give up and say "don't use iommu=on within
VMs if you want any performance". But the point is, if we just fix QEMU
to actually obey IOMMU mappings for assigned devices, then there's
already a kind of answer with virtio being trusted since it's part of
hypervisor, all this without guest changes. Seems kind of sad to let
performance regress.
So a (yet another) feature bit would be a possible solution there, but
we don't seem to be able to even agree on using a feature bit for a
quirk.
> And why I keep repeating myself that it has nothing to do with the
> actual device or the virtio drivers.
>
> I understand that POWER and other platforms don't currently have a
> clean way to indicate that certain device don't have translation. And I
> understand that we may end up with a *quirk* which ensures that the DMA
> API does the right thing (i.e. nothing) in certain cases.
So assuming we forget about 2 above for now, then yes, all we need
is a quirk, using some logic to detect these systems.
> But we should *NOT* be involving the virtio device drivers in that
> quirk, in any way. And putting a feature bit in the virtio device
> itself doesn't seem at all sane either.
Only if there's some other device that benefits from all this work. If
virtio is the only one that benefits, then why do we want to
spread the quirk rules around so much? A feature bit gives us
a single, portable rule that the quirk can use on all platforms.
> Bear in mind that qemu-system-x86_64 currently has the *same* problem
> with assigned physical devices. It's claiming they're translated, and
> they're not.
>
> --
> dwmw2
>
Presumably people either don't assign
devices or don't have an iommu otherwise things won't work for them,
but if they do have an iommu and don't assign devices, then Andy's
patch will break them.
This is not QEMU specific unfortunately, we don't know who
might have implemented virtio.
--
MST
^ permalink raw reply
* Re: [PATCH -qemu] nvme: support Google vendor extension
From: Ming Lin @ 2015-11-20 8:11 UTC (permalink / raw)
To: Paolo Bonzini
Cc: fes, axboe, tytso, qemu-devel, linux-nvme, virtualization,
keith.busch, Rob Nelson, Christoph Hellwig, Mihai Rusu
In-Reply-To: <564DA682.8050706@redhat.com>
On Thu, 2015-11-19 at 11:37 +0100, Paolo Bonzini wrote:
>
> On 18/11/2015 06:47, Ming Lin wrote:
> > @@ -726,7 +798,11 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
> > }
> >
> > start_sqs = nvme_cq_full(cq) ? 1 : 0;
> > - cq->head = new_head;
> > + /* When the mapped pointer memory area is setup, we don't rely on
> > + * the MMIO written values to update the head pointer. */
> > + if (!cq->db_addr) {
> > + cq->head = new_head;
> > + }
>
> You are still checking
>
> if (new_head >= cq->size) {
> return;
> }
>
> above. I think this is incorrect when the extension is present, and
> furthermore it's the only case where val is being used.
>
> If you're not using val, you could use ioeventfd for the MMIO. An
> ioeventfd cuts the MMIO cost by at least 55% and up to 70%. Here are
> quick and dirty measurements from kvm-unit-tests's vmexit.flat
> benchmark, on two very different machines:
>
> Haswell-EP Ivy Bridge i7
> MMIO memory write 5100 -> 2250 (55%) 7000 -> 3000 (58%)
> I/O port write 3800 -> 1150 (70%) 4100 -> 1800 (57%)
>
> You would need to allocate two eventfds for each qid, one for the sq and
> one for the cq. Also, processing the queues is now bounced to the QEMU
> iothread, so you can probably get rid of sq->timer and cq->timer.
Here is a quick try.
Too late now, I'll test it morning.
Do you see obvious problem?
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 3e1c38d..d28690d 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -543,6 +543,44 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
return NVME_SUCCESS;
}
+static void nvme_cq_notifier(EventNotifier *e)
+{
+ NvmeCQueue *cq =
+ container_of(e, NvmeCQueue, notifier);
+
+ nvme_post_cqes(cq);
+}
+
+static void nvme_init_cq_eventfd(NvmeCQueue *cq)
+{
+ NvmeCtrl *n = cq->ctrl;
+ uint16_t offset = (cq->cqid*2+1) * NVME_CAP_DSTRD(n->bar.cap);
+
+ event_notifier_init(&cq->notifier, 0);
+ event_notifier_set_handler(&cq->notifier, nvme_cq_notifier);
+ memory_region_add_eventfd(&n->iomem,
+ 0x1000 + offset, 4, true, cq->cqid*2+1, &cq->notifier);
+}
+
+static void nvme_sq_notifier(EventNotifier *e)
+{
+ NvmeSQueue *sq =
+ container_of(e, NvmeSQueue, notifier);
+
+ nvme_process_sq(sq);
+}
+
+static void nvme_init_sq_eventfd(NvmeSQueue *sq)
+{
+ NvmeCtrl *n = sq->ctrl;
+ uint16_t offset = sq->sqid * 2 * NVME_CAP_DSTRD(n->bar.cap);
+
+ event_notifier_init(&sq->notifier, 0);
+ event_notifier_set_handler(&sq->notifier, nvme_sq_notifier);
+ memory_region_add_eventfd(&n->iomem,
+ 0x1000 + offset, 4, true, sq->sqid * 2, &sq->notifier);
+}
+
static uint16_t nvme_set_db_memory(NvmeCtrl *n, const NvmeCmd *cmd)
{
uint64_t db_addr = le64_to_cpu(cmd->prp1);
@@ -565,6 +603,7 @@ static uint16_t nvme_set_db_memory(NvmeCtrl *n, const NvmeCmd *cmd)
/* Submission queue tail pointer location, 2 * QID * stride. */
sq->db_addr = db_addr + 2 * i * 4;
sq->eventidx_addr = eventidx_addr + 2 * i * 4;
+ nvme_init_sq_eventfd(sq);
}
if (cq != NULL) {
@@ -572,6 +611,7 @@ static uint16_t nvme_set_db_memory(NvmeCtrl *n, const NvmeCmd *cmd)
*/
cq->db_addr = db_addr + (2 * i + 1) * 4;
cq->eventidx_addr = eventidx_addr + (2 * i + 1) * 4;
+ nvme_init_cq_eventfd(cq);
}
}
return NVME_SUCCESS;
@@ -793,7 +833,7 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
}
cq = n->cq[qid];
- if (new_head >= cq->size) {
+ if (!cq->db_addr && new_head >= cq->size) {
return;
}
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 82aeab4..608f202 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -667,6 +667,7 @@ typedef struct NvmeSQueue {
* do not go over this value will not result in MMIO writes (but will
* still write the tail pointer to the "db_addr" location above). */
uint64_t eventidx_addr;
+ EventNotifier notifier;
} NvmeSQueue;
typedef struct NvmeCQueue {
@@ -689,6 +690,7 @@ typedef struct NvmeCQueue {
* do not go over this value will not result in MMIO writes (but will
* still write the head pointer to the "db_addr" location above). */
uint64_t eventidx_addr;
+ EventNotifier notifier;
} NvmeCQueue;
typedef struct NvmeNamespace {
>
> Paolo
^ permalink raw reply related
* Re: [PATCH v3 0/3] virtio DMA API core stuff
From: Michael S. Tsirkin @ 2015-11-20 7:47 UTC (permalink / raw)
To: Andy Lutomirski
Cc: linux-s390, KVM, Benjamin Herrenschmidt, Sebastian Ott,
linux-kernel@vger.kernel.org, Linux Virtualization,
Christian Borntraeger, Joerg Roedel, Martin Schwidefsky,
Paolo Bonzini, David Woodhouse, Christoph Hellwig
In-Reply-To: <20151120083857-mutt-send-email-mst@redhat.com>
On Fri, Nov 20, 2015 at 08:56:46AM +0200, Michael S. Tsirkin wrote:
> On Thu, Nov 19, 2015 at 01:59:05PM -0800, Andy Lutomirski wrote:
> > On Nov 19, 2015 5:45 AM, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >
> > > On Tue, Oct 27, 2015 at 11:38:57PM -0700, Andy Lutomirski wrote:
> > > > This switches virtio to use the DMA API unconditionally. I'm sure
> > > > it breaks things, but it seems to work on x86 using virtio-pci, with
> > > > and without Xen, and using both the modern 1.0 variant and the
> > > > legacy variant.
> > >
> > > So thinking hard about it, I don't see any real drawbacks to making this
> > > conditional on a new feature bit, that Xen can then set..
> >
> > Can you elaborate? If I run QEMU, hosting Xen, hosting Linux, and the
> > virtio device is provided by QEMU, then how does Xen set the bit?
>
> You would run QEMU with the appropriate flag. E.g.
> -global virtio-pci,use_platform_dma=on
Or Xen code within QEMU can tweak this global internally
so users don't need to care.
> > Similarly, how would Xen set the bit for a real physical device?
> >
> >
> > --Andy
>
> There's no need to set bits for physical devices I think: from security
> point of view, using them from a VM isn't very different from using them
> from host.
>
>
>
> --
> MST
^ permalink raw reply
* Re: [PATCH v3 0/3] virtio DMA API core stuff
From: Michael S. Tsirkin @ 2015-11-20 6:56 UTC (permalink / raw)
To: Andy Lutomirski
Cc: linux-s390, KVM, Benjamin Herrenschmidt, Sebastian Ott,
linux-kernel@vger.kernel.org, Linux Virtualization,
Christian Borntraeger, Joerg Roedel, Martin Schwidefsky,
Paolo Bonzini, David Woodhouse, Christoph Hellwig
In-Reply-To: <CALCETrXXsThXpbRMQ_CWTsq7_HiRFaK+dTeDYaz5cSm0QtoW_w@mail.gmail.com>
On Thu, Nov 19, 2015 at 01:59:05PM -0800, Andy Lutomirski wrote:
> On Nov 19, 2015 5:45 AM, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > On Tue, Oct 27, 2015 at 11:38:57PM -0700, Andy Lutomirski wrote:
> > > This switches virtio to use the DMA API unconditionally. I'm sure
> > > it breaks things, but it seems to work on x86 using virtio-pci, with
> > > and without Xen, and using both the modern 1.0 variant and the
> > > legacy variant.
> >
> > So thinking hard about it, I don't see any real drawbacks to making this
> > conditional on a new feature bit, that Xen can then set..
>
> Can you elaborate? If I run QEMU, hosting Xen, hosting Linux, and the
> virtio device is provided by QEMU, then how does Xen set the bit?
You would run QEMU with the appropriate flag. E.g.
-global virtio-pci,use_platform_dma=on
> Similarly, how would Xen set the bit for a real physical device?
>
>
> --Andy
There's no need to set bits for physical devices I think: from security
point of view, using them from a VM isn't very different from using them
from host.
--
MST
^ permalink raw reply
* Re: [RFC PATCH 0/9] vhost-nvme: new qemu nvme backend using nvme target
From: Ming Lin @ 2015-11-20 5:33 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: qemu-devel, linux-nvme, virtualization
In-Reply-To: <20151120051645.GB19405@lst.de>
On Fri, 2015-11-20 at 06:16 +0100, Christoph Hellwig wrote:
> Thanks Ming,
>
> from a first quick view this looks great. I'll look over it in a bit
> more detail once I get a bit more time.
Thanks to CC Nic :-)
But funny, I double-checked bash history. I actually CCed Nic.
Don't know why it's lost.
mlin@ssi:~$ history |grep "nab"
1861 git send-email --from "Ming Lin <mlin@kernel.org>" --to
"linux-nvme@lists.infradead.org" --cc "qemu-devel@nongnu.org" --cc
"virtualization@lists.linux-foundation.org" --cc "Christoph Hellwig
<hch@lst.de>" --cc "Nicholas A. Bellinger <nab@linux-iscsi.org>"
--compose ~/patches/*.patch
^ permalink raw reply
* Re: [RFC PATCH 4/9] nvmet: add a controller "start" hook
From: Ming Lin @ 2015-11-20 5:31 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Ming Lin, qemu-devel, linux-nvme, virtualization
In-Reply-To: <20151120051355.GA19405@lst.de>
On Fri, 2015-11-20 at 06:13 +0100, Christoph Hellwig wrote:
> On Thu, Nov 19, 2015 at 04:21:03PM -0800, Ming Lin wrote:
> > #define NVMET_SUBSYS_NAME_LEN 256
> > char subsys_name[NVMET_SUBSYS_NAME_LEN];
> > +
> > + void *opaque;
> > + void (*start)(void *);
> > };
>
> Why can't vhost use container_of to get at the containing structure
> similar to what the loop driver does?
struct nvme_loop_ctrl {
struct nvme_ctrl ctrl;
struct nvmet_ctrl *target_ctrl;
};
static inline struct nvme_loop_ctrl *to_loop_ctrl(struct nvme_ctrl *ctrl)
{
return container_of(ctrl, struct nvme_loop_ctrl, ctrl);
}
loop driver's container_of is used for "nvme_ctrl", not "nvmet_ctrl"
Possibly change:
struct nvmet_ctrl *nvmet_alloc_ctrl(struct nvmet_subsys *subsys,
const char *subsys_name)
to
int nvmet_init_ctrl(struct nvmet_ctrl *, ....)
?
So we can embed "struct nvmet_ctrl" to upper layer structure.
>
> In addition I think we'll eventually need an ops structure here,
> but I can take care of that later.
^ permalink raw reply
* Re: [RFC PATCH 0/9] vhost-nvme: new qemu nvme backend using nvme target
From: Christoph Hellwig @ 2015-11-20 5:16 UTC (permalink / raw)
To: Ming Lin; +Cc: Christoph Hellwig, qemu-devel, linux-nvme, virtualization
In-Reply-To: <1447978868-17138-1-git-send-email-mlin@kernel.org>
Thanks Ming,
from a first quick view this looks great. I'll look over it in a bit
more detail once I get a bit more time.
^ 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