* Re: [Qemu-devel] [RFC 0/3] VirtIO RDMA
From: Hannes Reinecke @ 2019-04-19 11:16 UTC (permalink / raw)
To: Yuval Shaia, Cornelia Huck
Cc: linux-rdma, jgg, virtualization, qemu-devel, mst
In-Reply-To: <20190415103546.GA6854@lap1>
On 4/15/19 12:35 PM, Yuval Shaia wrote:
> On Thu, Apr 11, 2019 at 07:02:15PM +0200, Cornelia Huck wrote:
>> On Thu, 11 Apr 2019 14:01:54 +0300
>> Yuval Shaia <yuval.shaia@oracle.com> wrote:
>>
>>> Data center backends use more and more RDMA or RoCE devices and more and
>>> more software runs in virtualized environment.
>>> There is a need for a standard to enable RDMA/RoCE on Virtual Machines.
>>>
>>> Virtio is the optimal solution since is the de-facto para-virtualizaton
>>> technology and also because the Virtio specification
>>> allows Hardware Vendors to support Virtio protocol natively in order to
>>> achieve bare metal performance.
>>>
>>> This RFC is an effort to addresses challenges in defining the RDMA/RoCE
>>> Virtio Specification and a look forward on possible implementation
>>> techniques.
>>>
>>> Open issues/Todo list:
>>> List is huge, this is only start point of the project.
>>> Anyway, here is one example of item in the list:
>>> - Multi VirtQ: Every QP has two rings and every CQ has one. This means that
>>> in order to support for example 32K QPs we will need 64K VirtQ. Not sure
>>> that this is reasonable so one option is to have one for all and
>>> multiplex the traffic on it. This is not good approach as by design it
>>> introducing an optional starvation. Another approach would be multi
>>> queues and round-robin (for example) between them.
>>>
Typically there will be a one-to-one mapping between QPs and CPUs (on
the guest). So while one would need to be prepared to support quite some
QPs, the expectation is that the actual number of QPs used will be
rather low.
In a similar vein, multiplexing QPs would be defeating the purpose, as
the overall idea was to have _independent_ QPs to enhance parallelism.
>>> Expectations from this posting:
>>> In general, any comment is welcome, starting from hey, drop this as it is a
>>> very bad idea, to yeah, go ahead, we really want it.
>>> Idea here is that since it is not a minor effort i first want to know if
>>> there is some sort interest in the community for such device.
>>
>> My first reaction is: Sounds sensible, but it would be good to have a
>> spec for this :)
>>
>> You'll need a spec if you want this to go forward anyway, so at least a
>> sketch would be good to answer questions such as how many virtqueues
>> you use for which purpose, what is actually put on the virtqueues,
>> whether there are negotiable features, and what the expectations for
>> the device and the driver are. It also makes it easier to understand
>> how this is supposed to work in practice.
>>
>> If folks agree that this sounds useful, the next step would be to
>> reserve an id for the device type.
>
> Thanks for the tips, will sure do that, it is that first i wanted to make
> sure there is a use case here.
>
> Waiting for any feedback from the community.
>
I really do like the ides; in fact, it saved me from coding a similar
thing myself :-)
However, I'm still curious about the overall intent of this driver.
Where would the I/O be routed _to_ ?
It's nice that we have a virtualized driver, but this driver is
intended to do I/O (even if it doesn't _do_ any I/O ATM :-)
And this I/O needs to be send to (and possibly received from)
something.
So what exactly is this something?
An existing piece of HW on the host?
If so, wouldn't it be more efficient to use vfio, either by using SR-IOV
or by using virtio-mdev?
Another guest?
If so, how would we route the I/O from one guest to the other?
Shared memory? Implementing a full-blown RDMA switch in qemu?
Oh, and I would _love_ to have a discussion about this at KVM Forum.
Maybe I'll manage to whip up guest-to-guest RDMA connection using
ivshmem ... let's see.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@suse.de +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v2 1/4] mm/balloon_compaction: list interfaces
From: Nadav Amit @ 2019-04-19 21:41 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Arnd Bergmann, Pv-drivers, Greg Kroah-Hartman,
Linux List Kernel Mailing,
virtualization@lists.linux-foundation.org, Linux-MM,
Julien Freche, Nadav Amit
In-Reply-To: <679D6F11-07D7-4227-9D02-41F9F8901E61@vmware.com>
> On Apr 8, 2019, at 10:35 AM, Nadav Amit <namit@vmware.com> wrote:
>
>> On Mar 27, 2019, at 6:07 PM, Nadav Amit <namit@vmware.com> wrote:
>>
>> Introduce interfaces for ballooning enqueueing and dequeueing of a list
>> of pages. These interfaces reduce the overhead of storing and restoring
>> IRQs by batching the operations. In addition they do not panic if the
>> list of pages is empty.
>>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>
> Michael, may I ping for your ack?
Ping again?
^ permalink raw reply
* Re: [PATCH v2 1/4] mm/balloon_compaction: list interfaces
From: Michael S. Tsirkin @ 2019-04-19 22:07 UTC (permalink / raw)
To: Nadav Amit
Cc: Arnd Bergmann, VMware, Inc., Greg Kroah-Hartman, linux-kernel,
virtualization, linux-mm, Julien Freche, Nadav Amit
In-Reply-To: <20190328010718.2248-2-namit@vmware.com>
On Thu, Mar 28, 2019 at 01:07:15AM +0000, Nadav Amit wrote:
> Introduce interfaces for ballooning enqueueing and dequeueing of a list
> of pages. These interfaces reduce the overhead of storing and restoring
> IRQs by batching the operations. In addition they do not panic if the
> list of pages is empty.
>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: linux-mm@kvack.org
> Cc: virtualization@lists.linux-foundation.org
> Reviewed-by: Xavier Deguillard <xdeguillard@vmware.com>
> Signed-off-by: Nadav Amit <namit@vmware.com>
> ---
> include/linux/balloon_compaction.h | 4 +
> mm/balloon_compaction.c | 145 +++++++++++++++++++++--------
> 2 files changed, 111 insertions(+), 38 deletions(-)
>
> diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h
> index f111c780ef1d..1da79edadb69 100644
> --- a/include/linux/balloon_compaction.h
> +++ b/include/linux/balloon_compaction.h
> @@ -64,6 +64,10 @@ extern struct page *balloon_page_alloc(void);
> extern void balloon_page_enqueue(struct balloon_dev_info *b_dev_info,
> struct page *page);
> extern struct page *balloon_page_dequeue(struct balloon_dev_info *b_dev_info);
> +extern size_t balloon_page_list_enqueue(struct balloon_dev_info *b_dev_info,
> + struct list_head *pages);
> +extern size_t balloon_page_list_dequeue(struct balloon_dev_info *b_dev_info,
> + struct list_head *pages, int n_req_pages);
>
Why size_t I wonder? It can never be > n_req_pages which is int.
Callers also seem to assume int.
> static inline void balloon_devinfo_init(struct balloon_dev_info *balloon)
> {
> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
> index ef858d547e2d..88d5d9a01072 100644
> --- a/mm/balloon_compaction.c
> +++ b/mm/balloon_compaction.c
> @@ -10,6 +10,106 @@
> #include <linux/export.h>
> #include <linux/balloon_compaction.h>
>
> +static int balloon_page_enqueue_one(struct balloon_dev_info *b_dev_info,
> + struct page *page)
> +{
> + /*
> + * Block others from accessing the 'page' when we get around to
> + * establishing additional references. We should be the only one
> + * holding a reference to the 'page' at this point.
> + */
> + if (!trylock_page(page)) {
> + WARN_ONCE(1, "balloon inflation failed to enqueue page\n");
> + return -EFAULT;
Looks like all callers bug on a failure. So let's just do it here,
and then make this void?
> + }
> + list_del(&page->lru);
> + balloon_page_insert(b_dev_info, page);
> + unlock_page(page);
> + __count_vm_event(BALLOON_INFLATE);
> + return 0;
> +}
> +
> +/**
> + * balloon_page_list_enqueue() - inserts a list of pages into the balloon page
> + * list.
> + * @b_dev_info: balloon device descriptor where we will insert a new page to
> + * @pages: pages to enqueue - allocated using balloon_page_alloc.
> + *
> + * Driver must call it to properly enqueue a balloon pages before definitively
> + * removing it from the guest system.
A bunch of grammar error here. Pls fix for clarify.
Also - document that nothing must lock the pages? More assumptions?
What is "it" in this context? All pages? And what does removing from
guest mean? Really adding to the balloon?
> + *
> + * Return: number of pages that were enqueued.
> + */
> +size_t balloon_page_list_enqueue(struct balloon_dev_info *b_dev_info,
> + struct list_head *pages)
> +{
> + struct page *page, *tmp;
> + unsigned long flags;
> + size_t n_pages = 0;
> +
> + spin_lock_irqsave(&b_dev_info->pages_lock, flags);
> + list_for_each_entry_safe(page, tmp, pages, lru) {
> + balloon_page_enqueue_one(b_dev_info, page);
Do we want to do something about an error here?
> + n_pages++;
> + }
> + spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
> + return n_pages;
> +}
> +EXPORT_SYMBOL_GPL(balloon_page_list_enqueue);
> +
> +/**
> + * balloon_page_list_dequeue() - removes pages from balloon's page list and
> + * returns a list of the pages.
> + * @b_dev_info: balloon device decriptor where we will grab a page from.
> + * @pages: pointer to the list of pages that would be returned to the caller.
> + * @n_req_pages: number of requested pages.
> + *
> + * Driver must call it to properly de-allocate a previous enlisted balloon pages
> + * before definetively releasing it back to the guest system. This function
> + * tries to remove @n_req_pages from the ballooned pages and return it to the
> + * caller in the @pages list.
> + *
> + * Note that this function may fail to dequeue some pages temporarily empty due
> + * to compaction isolated pages.
> + *
> + * Return: number of pages that were added to the @pages list.
> + */
> +size_t balloon_page_list_dequeue(struct balloon_dev_info *b_dev_info,
> + struct list_head *pages, int n_req_pages)
> +{
> + struct page *page, *tmp;
> + unsigned long flags;
> + size_t n_pages = 0;
> +
> + spin_lock_irqsave(&b_dev_info->pages_lock, flags);
> + list_for_each_entry_safe(page, tmp, &b_dev_info->pages, lru) {
> + /*
> + * Block others from accessing the 'page' while we get around
> + * establishing additional references and preparing the 'page'
> + * to be released by the balloon driver.
> + */
> + if (!trylock_page(page))
> + continue;
> +
> + if (IS_ENABLED(CONFIG_BALLOON_COMPACTION) &&
> + PageIsolated(page)) {
> + /* raced with isolation */
> + unlock_page(page);
> + continue;
> + }
> + balloon_page_delete(page);
> + __count_vm_event(BALLOON_DEFLATE);
> + unlock_page(page);
> + list_add(&page->lru, pages);
> + if (++n_pages >= n_req_pages)
> + break;
> + }
> + spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
> +
> + return n_pages;
> +}
> +EXPORT_SYMBOL_GPL(balloon_page_list_dequeue);
> +
> /*
> * balloon_page_alloc - allocates a new page for insertion into the balloon
> * page list.
> @@ -43,17 +143,9 @@ void balloon_page_enqueue(struct balloon_dev_info *b_dev_info,
> {
> unsigned long flags;
>
> - /*
> - * Block others from accessing the 'page' when we get around to
> - * establishing additional references. We should be the only one
> - * holding a reference to the 'page' at this point.
> - */
> - BUG_ON(!trylock_page(page));
> spin_lock_irqsave(&b_dev_info->pages_lock, flags);
> - balloon_page_insert(b_dev_info, page);
> - __count_vm_event(BALLOON_INFLATE);
> + balloon_page_enqueue_one(b_dev_info, page);
We used to bug on failure to lock page, now we
silently ignore this error. Why?
> spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
> - unlock_page(page);
> }
> EXPORT_SYMBOL_GPL(balloon_page_enqueue);
>
> @@ -70,36 +162,13 @@ EXPORT_SYMBOL_GPL(balloon_page_enqueue);
> */
> struct page *balloon_page_dequeue(struct balloon_dev_info *b_dev_info)
> {
> - struct page *page, *tmp;
> unsigned long flags;
> - bool dequeued_page;
> + LIST_HEAD(pages);
> + int n_pages;
>
> - dequeued_page = false;
> - spin_lock_irqsave(&b_dev_info->pages_lock, flags);
> - list_for_each_entry_safe(page, tmp, &b_dev_info->pages, lru) {
> - /*
> - * Block others from accessing the 'page' while we get around
> - * establishing additional references and preparing the 'page'
> - * to be released by the balloon driver.
> - */
> - if (trylock_page(page)) {
> -#ifdef CONFIG_BALLOON_COMPACTION
> - if (PageIsolated(page)) {
> - /* raced with isolation */
> - unlock_page(page);
> - continue;
> - }
> -#endif
> - balloon_page_delete(page);
> - __count_vm_event(BALLOON_DEFLATE);
> - unlock_page(page);
> - dequeued_page = true;
> - break;
> - }
> - }
> - spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
> + n_pages = balloon_page_list_dequeue(b_dev_info, &pages, 1);
>
> - if (!dequeued_page) {
> + if (n_pages != 1) {
> /*
> * If we are unable to dequeue a balloon page because the page
> * list is empty and there is no isolated pages, then something
> @@ -112,9 +181,9 @@ struct page *balloon_page_dequeue(struct balloon_dev_info *b_dev_info)
> !b_dev_info->isolated_pages))
> BUG();
> spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
> - page = NULL;
> + return NULL;
> }
> - return page;
> + return list_first_entry(&pages, struct page, lru);
> }
> EXPORT_SYMBOL_GPL(balloon_page_dequeue);
>
> --
> 2.19.1
^ permalink raw reply
* Re: [PATCH v2 1/4] mm/balloon_compaction: list interfaces
From: Michael S. Tsirkin @ 2019-04-19 22:47 UTC (permalink / raw)
To: Nadav Amit
Cc: Arnd Bergmann, Pv-drivers, Greg Kroah-Hartman,
linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org, linux-mm@kvack.org,
Julien Freche
In-Reply-To: <B2DD0CC3-DA8D-408C-986F-130B4B00A892@vmware.com>
On Fri, Apr 19, 2019 at 10:34:04PM +0000, Nadav Amit wrote:
> > On Apr 19, 2019, at 3:07 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Mar 28, 2019 at 01:07:15AM +0000, Nadav Amit wrote:
> >> Introduce interfaces for ballooning enqueueing and dequeueing of a list
> >> of pages. These interfaces reduce the overhead of storing and restoring
> >> IRQs by batching the operations. In addition they do not panic if the
> >> list of pages is empty.
> >>
> >> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> >> Cc: Jason Wang <jasowang@redhat.com>
> >> Cc: linux-mm@kvack.org
> >> Cc: virtualization@lists.linux-foundation.org
> >> Reviewed-by: Xavier Deguillard <xdeguillard@vmware.com>
> >> Signed-off-by: Nadav Amit <namit@vmware.com>
> >> ---
> >> include/linux/balloon_compaction.h | 4 +
> >> mm/balloon_compaction.c | 145 +++++++++++++++++++++--------
> >> 2 files changed, 111 insertions(+), 38 deletions(-)
> >>
> >> diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h
> >> index f111c780ef1d..1da79edadb69 100644
> >> --- a/include/linux/balloon_compaction.h
> >> +++ b/include/linux/balloon_compaction.h
> >> @@ -64,6 +64,10 @@ extern struct page *balloon_page_alloc(void);
> >> extern void balloon_page_enqueue(struct balloon_dev_info *b_dev_info,
> >> struct page *page);
> >> extern struct page *balloon_page_dequeue(struct balloon_dev_info *b_dev_info);
> >> +extern size_t balloon_page_list_enqueue(struct balloon_dev_info *b_dev_info,
> >> + struct list_head *pages);
> >> +extern size_t balloon_page_list_dequeue(struct balloon_dev_info *b_dev_info,
> >> + struct list_head *pages, int n_req_pages);
> >
> > Why size_t I wonder? It can never be > n_req_pages which is int.
> > Callers also seem to assume int.
>
> Only because on the previous iteration
> ( https://lkml.org/lkml/2019/2/6/912 ) you said:
>
> > Are we sure this int never overflows? Why not just use u64
> > or size_t straight away?
And the answer is because n_req_pages is an int too?
>
> I am ok either way, but please be consistent.
I guess n_req_pages should be size_t too then?
> >
> >> static inline void balloon_devinfo_init(struct balloon_dev_info *balloon)
> >> {
> >
> >
> >> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
> >> index ef858d547e2d..88d5d9a01072 100644
> >> --- a/mm/balloon_compaction.c
> >> +++ b/mm/balloon_compaction.c
> >> @@ -10,6 +10,106 @@
> >> #include <linux/export.h>
> >> #include <linux/balloon_compaction.h>
> >>
> >> +static int balloon_page_enqueue_one(struct balloon_dev_info *b_dev_info,
> >> + struct page *page)
> >> +{
> >> + /*
> >> + * Block others from accessing the 'page' when we get around to
> >> + * establishing additional references. We should be the only one
> >> + * holding a reference to the 'page' at this point.
> >> + */
> >> + if (!trylock_page(page)) {
> >> + WARN_ONCE(1, "balloon inflation failed to enqueue page\n");
> >> + return -EFAULT;
> >
> > Looks like all callers bug on a failure. So let's just do it here,
> > and then make this void?
>
> As you noted below, actually balloon_page_list_enqueue() does not do
> anything when an error occurs. I really prefer to avoid adding BUG_ON() -
> I always get pushed back on such things. Yes, this might lead to memory
> leak, but there is no reason to crash the system.
Need to audit callers to make sure they don't misbehave in worse ways.
I think in this case this indicates that someone is using the page so if
one keeps going and adds it into balloon this will lead to corruption down the road.
If you can change the caller code such that it's just a leak,
then a warning is more appropriate. Or even do not warn at all.
> >> + }
> >> + list_del(&page->lru);
> >> + balloon_page_insert(b_dev_info, page);
> >> + unlock_page(page);
> >> + __count_vm_event(BALLOON_INFLATE);
> >> + return 0;
> >> +}
> >> +
> >> +/**
> >> + * balloon_page_list_enqueue() - inserts a list of pages into the balloon page
> >> + * list.
> >> + * @b_dev_info: balloon device descriptor where we will insert a new page to
> >> + * @pages: pages to enqueue - allocated using balloon_page_alloc.
> >> + *
> >> + * Driver must call it to properly enqueue a balloon pages before definitively
> >> + * removing it from the guest system.
> >
> > A bunch of grammar error here. Pls fix for clarify.
> > Also - document that nothing must lock the pages? More assumptions?
> > What is "it" in this context? All pages? And what does removing from
> > guest mean? Really adding to the balloon?
>
> I pretty much copy-pasted this description from balloon_page_enqueue(). I
> see that you edited this message in the past at least couple of times (e.g.,
> c7cdff0e86471 “virtio_balloon: fix deadlock on OOM”) and left it as is.
>
> So maybe all of the comments in this file need a rework, but I don’t think
> this patch-set needs to do it.
I see.
That one dealt with one page so "it" was the page. This one deals with
many pages so you can't just copy it over without changes.
Makes it look like "it" refers to driver or guest.
> >> + *
> >> + * Return: number of pages that were enqueued.
> >> + */
> >> +size_t balloon_page_list_enqueue(struct balloon_dev_info *b_dev_info,
> >> + struct list_head *pages)
> >> +{
> >> + struct page *page, *tmp;
> >> + unsigned long flags;
> >> + size_t n_pages = 0;
> >> +
> >> + spin_lock_irqsave(&b_dev_info->pages_lock, flags);
> >> + list_for_each_entry_safe(page, tmp, pages, lru) {
> >> + balloon_page_enqueue_one(b_dev_info, page);
> >
> > Do we want to do something about an error here?
>
> Hmm… This is really something that should never happen, but I still prefer
> to avoid BUG_ON(), as I said before. I will just not count the page.
Callers can BUG then if they want. That is fine but you then
need to change the callers to do it.
> >
> >> + n_pages++;
> >> + }
> >> + spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
> >> + return n_pages;
> >> +}
> >> +EXPORT_SYMBOL_GPL(balloon_page_list_enqueue);
> >> +
> >> +/**
> >> + * balloon_page_list_dequeue() - removes pages from balloon's page list and
> >> + * returns a list of the pages.
> >> + * @b_dev_info: balloon device decriptor where we will grab a page from.
> >> + * @pages: pointer to the list of pages that would be returned to the caller.
> >> + * @n_req_pages: number of requested pages.
> >> + *
> >> + * Driver must call it to properly de-allocate a previous enlisted balloon pages
> >> + * before definetively releasing it back to the guest system. This function
> >> + * tries to remove @n_req_pages from the ballooned pages and return it to the
> >> + * caller in the @pages list.
> >> + *
> >> + * Note that this function may fail to dequeue some pages temporarily empty due
> >> + * to compaction isolated pages.
> >> + *
> >> + * Return: number of pages that were added to the @pages list.
> >> + */
> >> +size_t balloon_page_list_dequeue(struct balloon_dev_info *b_dev_info,
> >> + struct list_head *pages, int n_req_pages)
> >> +{
> >> + struct page *page, *tmp;
> >> + unsigned long flags;
> >> + size_t n_pages = 0;
> >> +
> >> + spin_lock_irqsave(&b_dev_info->pages_lock, flags);
> >> + list_for_each_entry_safe(page, tmp, &b_dev_info->pages, lru) {
> >> + /*
> >> + * Block others from accessing the 'page' while we get around
> >> + * establishing additional references and preparing the 'page'
> >> + * to be released by the balloon driver.
> >> + */
> >> + if (!trylock_page(page))
> >> + continue;
> >> +
> >> + if (IS_ENABLED(CONFIG_BALLOON_COMPACTION) &&
> >> + PageIsolated(page)) {
> >> + /* raced with isolation */
> >> + unlock_page(page);
> >> + continue;
> >> + }
> >> + balloon_page_delete(page);
> >> + __count_vm_event(BALLOON_DEFLATE);
> >> + unlock_page(page);
> >> + list_add(&page->lru, pages);
> >> + if (++n_pages >= n_req_pages)
> >> + break;
> >> + }
> >> + spin_unlock_irqrestore(&b_dev_info->pages_lock, flags);
> >> +
> >> + return n_pages;
> >> +}
> >> +EXPORT_SYMBOL_GPL(balloon_page_list_dequeue);
> >> +
> >> /*
> >> * balloon_page_alloc - allocates a new page for insertion into the balloon
> >> * page list.
> >> @@ -43,17 +143,9 @@ void balloon_page_enqueue(struct balloon_dev_info *b_dev_info,
> >> {
> >> unsigned long flags;
> >>
> >> - /*
> >> - * Block others from accessing the 'page' when we get around to
> >> - * establishing additional references. We should be the only one
> >> - * holding a reference to the 'page' at this point.
> >> - */
> >> - BUG_ON(!trylock_page(page));
> >> spin_lock_irqsave(&b_dev_info->pages_lock, flags);
> >> - balloon_page_insert(b_dev_info, page);
> >> - __count_vm_event(BALLOON_INFLATE);
> >> + balloon_page_enqueue_one(b_dev_info, page);
> >
> > We used to bug on failure to lock page, now we
> > silently ignore this error. Why?
>
> That’s a mistake. I’ll add a BUG_ON() if balloon_page_enqueue_one() fails.
>
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC PATCH] virtio_ring: Use DMA API if guest memory is encrypted
From: Michael S. Tsirkin @ 2019-04-19 23:09 UTC (permalink / raw)
To: Thiago Jung Bauermann
Cc: Mike Anderson, Jean-Philippe Brucker, Benjamin Herrenschmidt,
Alexey Kardashevskiy, Ram Pai, linux-kernel, virtualization,
Paul Mackerras, iommu, linuxppc-dev, Christoph Hellwig,
David Gibson
In-Reply-To: <87a7go71hz.fsf@morokweng.localdomain>
On Wed, Apr 17, 2019 at 06:42:00PM -0300, Thiago Jung Bauermann wrote:
>
> Michael S. Tsirkin <mst@redhat.com> writes:
>
> > On Thu, Mar 21, 2019 at 09:05:04PM -0300, Thiago Jung Bauermann wrote:
> >>
> >> Michael S. Tsirkin <mst@redhat.com> writes:
> >>
> >> > On Wed, Mar 20, 2019 at 01:13:41PM -0300, Thiago Jung Bauermann wrote:
> >> >> >From what I understand of the ACCESS_PLATFORM definition, the host will
> >> >> only ever try to access memory addresses that are supplied to it by the
> >> >> guest, so all of the secure guest memory that the host cares about is
> >> >> accessible:
> >> >>
> >> >> If this feature bit is set to 0, then the device has same access to
> >> >> memory addresses supplied to it as the driver has. In particular,
> >> >> the device will always use physical addresses matching addresses
> >> >> used by the driver (typically meaning physical addresses used by the
> >> >> CPU) and not translated further, and can access any address supplied
> >> >> to it by the driver. When clear, this overrides any
> >> >> platform-specific description of whether device access is limited or
> >> >> translated in any way, e.g. whether an IOMMU may be present.
> >> >>
> >> >> All of the above is true for POWER guests, whether they are secure
> >> >> guests or not.
> >> >>
> >> >> Or are you saying that a virtio device may want to access memory
> >> >> addresses that weren't supplied to it by the driver?
> >> >
> >> > Your logic would apply to IOMMUs as well. For your mode, there are
> >> > specific encrypted memory regions that driver has access to but device
> >> > does not. that seems to violate the constraint.
> >>
> >> Right, if there's a pre-configured 1:1 mapping in the IOMMU such that
> >> the device can ignore the IOMMU for all practical purposes I would
> >> indeed say that the logic would apply to IOMMUs as well. :-)
> >>
> >> I guess I'm still struggling with the purpose of signalling to the
> >> driver that the host may not have access to memory addresses that it
> >> will never try to access.
> >
> > For example, one of the benefits is to signal to host that driver does
> > not expect ability to access all memory. If it does, host can
> > fail initialization gracefully.
>
> But why would the ability to access all memory be necessary or even
> useful? When would the host access memory that the driver didn't tell it
> to access?
When I say all memory I mean even memory not allowed by the IOMMU.
> >> >> >> > But the name "sev_active" makes me scared because at least AMD guys who
> >> >> >> > were doing the sensible thing and setting ACCESS_PLATFORM
> >> >> >>
> >> >> >> My understanding is, AMD guest-platform knows in advance that their
> >> >> >> guest will run in secure mode and hence sets the flag at the time of VM
> >> >> >> instantiation. Unfortunately we dont have that luxury on our platforms.
> >> >> >
> >> >> > Well you do have that luxury. It looks like that there are existing
> >> >> > guests that already acknowledge ACCESS_PLATFORM and you are not happy
> >> >> > with how that path is slow. So you are trying to optimize for
> >> >> > them by clearing ACCESS_PLATFORM and then you have lost ability
> >> >> > to invoke DMA API.
> >> >> >
> >> >> > For example if there was another flag just like ACCESS_PLATFORM
> >> >> > just not yet used by anyone, you would be all fine using that right?
> >> >>
> >> >> Yes, a new flag sounds like a great idea. What about the definition
> >> >> below?
> >> >>
> >> >> VIRTIO_F_ACCESS_PLATFORM_NO_IOMMU This feature has the same meaning as
> >> >> VIRTIO_F_ACCESS_PLATFORM both when set and when not set, with the
> >> >> exception that the IOMMU is explicitly defined to be off or bypassed
> >> >> when accessing memory addresses supplied to the device by the
> >> >> driver. This flag should be set by the guest if offered, but to
> >> >> allow for backward-compatibility device implementations allow for it
> >> >> to be left unset by the guest. It is an error to set both this flag
> >> >> and VIRTIO_F_ACCESS_PLATFORM.
> >> >
> >> > It looks kind of narrow but it's an option.
> >>
> >> Great!
> >>
> >> > I wonder how we'll define what's an iommu though.
> >>
> >> Hm, it didn't occur to me it could be an issue. I'll try.
>
> I rephrased it in terms of address translation. What do you think of
> this version? The flag name is slightly different too:
>
>
> VIRTIO_F_ACCESS_PLATFORM_NO_TRANSLATION This feature has the same
> meaning as VIRTIO_F_ACCESS_PLATFORM both when set and when not set,
> with the exception that address translation is guaranteed to be
> unnecessary when accessing memory addresses supplied to the device
> by the driver. Which is to say, the device will always use physical
> addresses matching addresses used by the driver (typically meaning
> physical addresses used by the CPU) and not translated further. This
> flag should be set by the guest if offered, but to allow for
> backward-compatibility device implementations allow for it to be
> left unset by the guest. It is an error to set both this flag and
> VIRTIO_F_ACCESS_PLATFORM.
Thanks, I'll think about this approach. Will respond next week.
> >> > Another idea is maybe something like virtio-iommu?
> >>
> >> You mean, have legacy guests use virtio-iommu to request an IOMMU
> >> bypass? If so, it's an interesting idea for new guests but it doesn't
> >> help with guests that are out today in the field, which don't have A
> >> virtio-iommu driver.
> >
> > I presume legacy guests don't use encrypted memory so why do we
> > worry about them at all?
>
> They don't use encrypted memory, but a host machine will run a mix of
> secure and legacy guests. And since the hypervisor doesn't know whether
> a guest will be secure or not at the time it is launched, legacy guests
> will have to be launched with the same configuration as secure guests.
OK and so I think the issue is that hosts generally fail if they set
ACCESS_PLATFORM and guests do not negotiate it.
So you can not just set ACCESS_PLATFORM for everyone.
Is that the issue here?
> >> >> > Is there any justification to doing that beyond someone putting
> >> >> > out slow code in the past?
> >> >>
> >> >> The definition of the ACCESS_PLATFORM flag is generic and captures the
> >> >> notion of memory access restrictions for the device. Unfortunately, on
> >> >> powerpc pSeries guests it also implies that the IOMMU is turned on
> >> >
> >> > IIUC that's really because on pSeries IOMMU is *always* turned on.
> >> > Platform has no way to say what you want it to say
> >> > which is bypass the iommu for the specific device.
> >>
> >> Yes, that's correct. pSeries guests running on KVM are in a gray area
> >> where theoretically they use an IOMMU but in practice KVM ignores it.
> >> It's unfortunate but it's the reality on the ground today. :-/
> >
> > Well it's not just the reality, virt setups need something that
> > emulated IOMMUs don't provide. That is not uncommon, e.g.
> > intel's VTD has a "cache mode" field which AFAIK is only used for virt.
>
> That's good to know. Thanks for this example.
>
> --
> Thiago Jung Bauermann
> IBM Linux Technology Center
^ permalink raw reply
* Re: [PATCH v3 09/26] compat_ioctl: move drivers to compat_ptr_ioctl
From: Michael S. Tsirkin @ 2019-04-19 23:16 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-usb, kvm, Alexander Shishkin, Jarkko Sakkinen,
virtualization, Benjamin Tissoires, linux-mtd, Peter Huewe,
linux1394-devel, linux-stm32, devel, Marek Vasut, y2038,
Richard Weinberger, Jason Gunthorpe, Jason Gunthorpe,
linux-arm-kernel, linux-input, Tomas Winkler, Alexandre Torgue,
Jiri Kosina, Alexander Viro, Stefan Hajnoczi,
Willem de Bruijn <willemb>
In-Reply-To: <20190416202013.4034148-10-arnd@arndb.de>
On Tue, Apr 16, 2019 at 10:19:47PM +0200, Arnd Bergmann wrote:
> Each of these drivers has a copy of the same trivial helper function to
> convert the pointer argument and then call the native ioctl handler.
>
> We now have a generic implementation of that, so use it.
>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/char/ppdev.c | 12 +---------
> drivers/char/tpm/tpm_vtpm_proxy.c | 12 +---------
> drivers/firewire/core-cdev.c | 12 +---------
> drivers/hid/usbhid/hiddev.c | 11 +--------
> drivers/hwtracing/stm/core.c | 12 +---------
> drivers/misc/mei/main.c | 22 +----------------
> drivers/mtd/ubi/cdev.c | 36 +++-------------------------
> drivers/net/tap.c | 12 +---------
> drivers/staging/pi433/pi433_if.c | 12 +---------
> drivers/usb/core/devio.c | 16 +------------
> drivers/vfio/vfio.c | 39 +++----------------------------
> drivers/vhost/net.c | 12 +---------
> drivers/vhost/scsi.c | 12 +---------
> drivers/vhost/test.c | 12 +---------
> drivers/vhost/vsock.c | 12 +---------
> fs/fat/file.c | 13 +----------
> 16 files changed, 20 insertions(+), 237 deletions(-)
>
> diff --git a/drivers/char/ppdev.c b/drivers/char/ppdev.c
> index 1ae77b41050a..e96c8d9623e0 100644
> --- a/drivers/char/ppdev.c
> +++ b/drivers/char/ppdev.c
> @@ -674,14 +674,6 @@ static long pp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> return ret;
> }
>
> -#ifdef CONFIG_COMPAT
> -static long pp_compat_ioctl(struct file *file, unsigned int cmd,
> - unsigned long arg)
> -{
> - return pp_ioctl(file, cmd, (unsigned long)compat_ptr(arg));
> -}
> -#endif
> -
> static int pp_open(struct inode *inode, struct file *file)
> {
> unsigned int minor = iminor(inode);
> @@ -790,9 +782,7 @@ static const struct file_operations pp_fops = {
> .write = pp_write,
> .poll = pp_poll,
> .unlocked_ioctl = pp_ioctl,
> -#ifdef CONFIG_COMPAT
> - .compat_ioctl = pp_compat_ioctl,
> -#endif
> + .compat_ioctl = compat_ptr_ioctl,
> .open = pp_open,
> .release = pp_release,
> };
> diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
> index d74f3de74ae6..fb845f0a430b 100644
> --- a/drivers/char/tpm/tpm_vtpm_proxy.c
> +++ b/drivers/char/tpm/tpm_vtpm_proxy.c
> @@ -675,20 +675,10 @@ static long vtpmx_fops_ioctl(struct file *f, unsigned int ioctl,
> }
> }
>
> -#ifdef CONFIG_COMPAT
> -static long vtpmx_fops_compat_ioctl(struct file *f, unsigned int ioctl,
> - unsigned long arg)
> -{
> - return vtpmx_fops_ioctl(f, ioctl, (unsigned long)compat_ptr(arg));
> -}
> -#endif
> -
> static const struct file_operations vtpmx_fops = {
> .owner = THIS_MODULE,
> .unlocked_ioctl = vtpmx_fops_ioctl,
> -#ifdef CONFIG_COMPAT
> - .compat_ioctl = vtpmx_fops_compat_ioctl,
> -#endif
> + .compat_ioctl = compat_ptr_ioctl,
> .llseek = noop_llseek,
> };
>
> diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
> index 16a7045736a9..fb934680fdd3 100644
> --- a/drivers/firewire/core-cdev.c
> +++ b/drivers/firewire/core-cdev.c
> @@ -1659,14 +1659,6 @@ static long fw_device_op_ioctl(struct file *file,
> return dispatch_ioctl(file->private_data, cmd, (void __user *)arg);
> }
>
> -#ifdef CONFIG_COMPAT
> -static long fw_device_op_compat_ioctl(struct file *file,
> - unsigned int cmd, unsigned long arg)
> -{
> - return dispatch_ioctl(file->private_data, cmd, compat_ptr(arg));
> -}
> -#endif
> -
> static int fw_device_op_mmap(struct file *file, struct vm_area_struct *vma)
> {
> struct client *client = file->private_data;
> @@ -1808,7 +1800,5 @@ const struct file_operations fw_device_ops = {
> .mmap = fw_device_op_mmap,
> .release = fw_device_op_release,
> .poll = fw_device_op_poll,
> -#ifdef CONFIG_COMPAT
> - .compat_ioctl = fw_device_op_compat_ioctl,
> -#endif
> + .compat_ioctl = compat_ptr_ioctl,
> };
> diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
> index a746017fac17..ef4a1cd389d6 100644
> --- a/drivers/hid/usbhid/hiddev.c
> +++ b/drivers/hid/usbhid/hiddev.c
> @@ -855,13 +855,6 @@ static long hiddev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> return r;
> }
>
> -#ifdef CONFIG_COMPAT
> -static long hiddev_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> -{
> - return hiddev_ioctl(file, cmd, (unsigned long)compat_ptr(arg));
> -}
> -#endif
> -
> static const struct file_operations hiddev_fops = {
> .owner = THIS_MODULE,
> .read = hiddev_read,
> @@ -871,9 +864,7 @@ static const struct file_operations hiddev_fops = {
> .release = hiddev_release,
> .unlocked_ioctl = hiddev_ioctl,
> .fasync = hiddev_fasync,
> -#ifdef CONFIG_COMPAT
> - .compat_ioctl = hiddev_compat_ioctl,
> -#endif
> + .compat_ioctl = compat_ptr_ioctl,
> .llseek = noop_llseek,
> };
>
> diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
> index c7ba8acfd4d5..454da259f144 100644
> --- a/drivers/hwtracing/stm/core.c
> +++ b/drivers/hwtracing/stm/core.c
> @@ -840,23 +840,13 @@ stm_char_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> return err;
> }
>
> -#ifdef CONFIG_COMPAT
> -static long
> -stm_char_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> -{
> - return stm_char_ioctl(file, cmd, (unsigned long)compat_ptr(arg));
> -}
> -#else
> -#define stm_char_compat_ioctl NULL
> -#endif
> -
> static const struct file_operations stm_fops = {
> .open = stm_char_open,
> .release = stm_char_release,
> .write = stm_char_write,
> .mmap = stm_char_mmap,
> .unlocked_ioctl = stm_char_ioctl,
> - .compat_ioctl = stm_char_compat_ioctl,
> + .compat_ioctl = compat_ptr_ioctl,
> .llseek = no_llseek,
> };
>
> diff --git a/drivers/misc/mei/main.c b/drivers/misc/mei/main.c
> index 87281b3695e6..cc6af92cdef0 100644
> --- a/drivers/misc/mei/main.c
> +++ b/drivers/misc/mei/main.c
> @@ -535,24 +535,6 @@ static long mei_ioctl(struct file *file, unsigned int cmd, unsigned long data)
> return rets;
> }
>
> -/**
> - * mei_compat_ioctl - the compat IOCTL function
> - *
> - * @file: pointer to file structure
> - * @cmd: ioctl command
> - * @data: pointer to mei message structure
> - *
> - * Return: 0 on success , <0 on error
> - */
> -#ifdef CONFIG_COMPAT
> -static long mei_compat_ioctl(struct file *file,
> - unsigned int cmd, unsigned long data)
> -{
> - return mei_ioctl(file, cmd, (unsigned long)compat_ptr(data));
> -}
> -#endif
> -
> -
> /**
> * mei_poll - the poll function
> *
> @@ -855,9 +837,7 @@ static const struct file_operations mei_fops = {
> .owner = THIS_MODULE,
> .read = mei_read,
> .unlocked_ioctl = mei_ioctl,
> -#ifdef CONFIG_COMPAT
> - .compat_ioctl = mei_compat_ioctl,
> -#endif
> + .compat_ioctl = compat_ptr_ioctl,
> .open = mei_open,
> .release = mei_release,
> .write = mei_write,
> diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c
> index 947a8adbc799..265d34fa3efa 100644
> --- a/drivers/mtd/ubi/cdev.c
> +++ b/drivers/mtd/ubi/cdev.c
> @@ -1091,36 +1091,6 @@ static long ctrl_cdev_ioctl(struct file *file, unsigned int cmd,
> return err;
> }
>
> -#ifdef CONFIG_COMPAT
> -static long vol_cdev_compat_ioctl(struct file *file, unsigned int cmd,
> - unsigned long arg)
> -{
> - unsigned long translated_arg = (unsigned long)compat_ptr(arg);
> -
> - return vol_cdev_ioctl(file, cmd, translated_arg);
> -}
> -
> -static long ubi_cdev_compat_ioctl(struct file *file, unsigned int cmd,
> - unsigned long arg)
> -{
> - unsigned long translated_arg = (unsigned long)compat_ptr(arg);
> -
> - return ubi_cdev_ioctl(file, cmd, translated_arg);
> -}
> -
> -static long ctrl_cdev_compat_ioctl(struct file *file, unsigned int cmd,
> - unsigned long arg)
> -{
> - unsigned long translated_arg = (unsigned long)compat_ptr(arg);
> -
> - return ctrl_cdev_ioctl(file, cmd, translated_arg);
> -}
> -#else
> -#define vol_cdev_compat_ioctl NULL
> -#define ubi_cdev_compat_ioctl NULL
> -#define ctrl_cdev_compat_ioctl NULL
> -#endif
> -
> /* UBI volume character device operations */
> const struct file_operations ubi_vol_cdev_operations = {
> .owner = THIS_MODULE,
> @@ -1131,7 +1101,7 @@ const struct file_operations ubi_vol_cdev_operations = {
> .write = vol_cdev_write,
> .fsync = vol_cdev_fsync,
> .unlocked_ioctl = vol_cdev_ioctl,
> - .compat_ioctl = vol_cdev_compat_ioctl,
> + .compat_ioctl = compat_ptr_ioctl,
> };
>
> /* UBI character device operations */
> @@ -1139,13 +1109,13 @@ const struct file_operations ubi_cdev_operations = {
> .owner = THIS_MODULE,
> .llseek = no_llseek,
> .unlocked_ioctl = ubi_cdev_ioctl,
> - .compat_ioctl = ubi_cdev_compat_ioctl,
> + .compat_ioctl = compat_ptr_ioctl,
> };
>
> /* UBI control character device operations */
> const struct file_operations ubi_ctrl_cdev_operations = {
> .owner = THIS_MODULE,
> .unlocked_ioctl = ctrl_cdev_ioctl,
> - .compat_ioctl = ctrl_cdev_compat_ioctl,
> + .compat_ioctl = compat_ptr_ioctl,
> .llseek = no_llseek,
> };
> diff --git a/drivers/net/tap.c b/drivers/net/tap.c
> index 2ea9b4976f4a..ebe425e65992 100644
> --- a/drivers/net/tap.c
> +++ b/drivers/net/tap.c
> @@ -1123,14 +1123,6 @@ static long tap_ioctl(struct file *file, unsigned int cmd,
> }
> }
>
> -#ifdef CONFIG_COMPAT
> -static long tap_compat_ioctl(struct file *file, unsigned int cmd,
> - unsigned long arg)
> -{
> - return tap_ioctl(file, cmd, (unsigned long)compat_ptr(arg));
> -}
> -#endif
> -
> static const struct file_operations tap_fops = {
> .owner = THIS_MODULE,
> .open = tap_open,
> @@ -1140,9 +1132,7 @@ static const struct file_operations tap_fops = {
> .poll = tap_poll,
> .llseek = no_llseek,
> .unlocked_ioctl = tap_ioctl,
> -#ifdef CONFIG_COMPAT
> - .compat_ioctl = tap_compat_ioctl,
> -#endif
> + .compat_ioctl = compat_ptr_ioctl,
> };
>
> static int tap_get_user_xdp(struct tap_queue *q, struct xdp_buff *xdp)
> diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> index b2314636dc89..ab7dfc7c2917 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -935,16 +935,6 @@ pi433_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> return retval;
> }
>
> -#ifdef CONFIG_COMPAT
> -static long
> -pi433_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> -{
> - return pi433_ioctl(filp, cmd, (unsigned long)compat_ptr(arg));
> -}
> -#else
> -#define pi433_compat_ioctl NULL
> -#endif /* CONFIG_COMPAT */
> -
> /*-------------------------------------------------------------------------*/
>
> static int pi433_open(struct inode *inode, struct file *filp)
> @@ -1101,7 +1091,7 @@ static const struct file_operations pi433_fops = {
> .write = pi433_write,
> .read = pi433_read,
> .unlocked_ioctl = pi433_ioctl,
> - .compat_ioctl = pi433_compat_ioctl,
> + .compat_ioctl = compat_ptr_ioctl,
> .open = pi433_open,
> .release = pi433_release,
> .llseek = no_llseek,
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index fa783531ee88..d75052b36584 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -2568,18 +2568,6 @@ static long usbdev_ioctl(struct file *file, unsigned int cmd,
> return ret;
> }
>
> -#ifdef CONFIG_COMPAT
> -static long usbdev_compat_ioctl(struct file *file, unsigned int cmd,
> - unsigned long arg)
> -{
> - int ret;
> -
> - ret = usbdev_do_ioctl(file, cmd, compat_ptr(arg));
> -
> - return ret;
> -}
> -#endif
> -
> /* No kernel lock - fine */
> static __poll_t usbdev_poll(struct file *file,
> struct poll_table_struct *wait)
> @@ -2603,9 +2591,7 @@ const struct file_operations usbdev_file_operations = {
> .read = usbdev_read,
> .poll = usbdev_poll,
> .unlocked_ioctl = usbdev_ioctl,
> -#ifdef CONFIG_COMPAT
> - .compat_ioctl = usbdev_compat_ioctl,
> -#endif
> + .compat_ioctl = compat_ptr_ioctl,
> .mmap = usbdev_mmap,
> .open = usbdev_open,
> .release = usbdev_release,
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index a3030cdf3c18..a5efe82584a5 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -1200,15 +1200,6 @@ static long vfio_fops_unl_ioctl(struct file *filep,
> return ret;
> }
>
> -#ifdef CONFIG_COMPAT
> -static long vfio_fops_compat_ioctl(struct file *filep,
> - unsigned int cmd, unsigned long arg)
> -{
> - arg = (unsigned long)compat_ptr(arg);
> - return vfio_fops_unl_ioctl(filep, cmd, arg);
> -}
> -#endif /* CONFIG_COMPAT */
> -
> static int vfio_fops_open(struct inode *inode, struct file *filep)
> {
> struct vfio_container *container;
> @@ -1291,9 +1282,7 @@ static const struct file_operations vfio_fops = {
> .read = vfio_fops_read,
> .write = vfio_fops_write,
> .unlocked_ioctl = vfio_fops_unl_ioctl,
> -#ifdef CONFIG_COMPAT
> - .compat_ioctl = vfio_fops_compat_ioctl,
> -#endif
> + .compat_ioctl = compat_ptr_ioctl,
> .mmap = vfio_fops_mmap,
> };
>
> @@ -1572,15 +1561,6 @@ static long vfio_group_fops_unl_ioctl(struct file *filep,
> return ret;
> }
>
> -#ifdef CONFIG_COMPAT
> -static long vfio_group_fops_compat_ioctl(struct file *filep,
> - unsigned int cmd, unsigned long arg)
> -{
> - arg = (unsigned long)compat_ptr(arg);
> - return vfio_group_fops_unl_ioctl(filep, cmd, arg);
> -}
> -#endif /* CONFIG_COMPAT */
> -
> static int vfio_group_fops_open(struct inode *inode, struct file *filep)
> {
> struct vfio_group *group;
> @@ -1636,9 +1616,7 @@ static int vfio_group_fops_release(struct inode *inode, struct file *filep)
> static const struct file_operations vfio_group_fops = {
> .owner = THIS_MODULE,
> .unlocked_ioctl = vfio_group_fops_unl_ioctl,
> -#ifdef CONFIG_COMPAT
> - .compat_ioctl = vfio_group_fops_compat_ioctl,
> -#endif
> + .compat_ioctl = compat_ptr_ioctl,
> .open = vfio_group_fops_open,
> .release = vfio_group_fops_release,
> };
> @@ -1703,24 +1681,13 @@ static int vfio_device_fops_mmap(struct file *filep, struct vm_area_struct *vma)
> return device->ops->mmap(device->device_data, vma);
> }
>
> -#ifdef CONFIG_COMPAT
> -static long vfio_device_fops_compat_ioctl(struct file *filep,
> - unsigned int cmd, unsigned long arg)
> -{
> - arg = (unsigned long)compat_ptr(arg);
> - return vfio_device_fops_unl_ioctl(filep, cmd, arg);
> -}
> -#endif /* CONFIG_COMPAT */
> -
> static const struct file_operations vfio_device_fops = {
> .owner = THIS_MODULE,
> .release = vfio_device_fops_release,
> .read = vfio_device_fops_read,
> .write = vfio_device_fops_write,
> .unlocked_ioctl = vfio_device_fops_unl_ioctl,
> -#ifdef CONFIG_COMPAT
> - .compat_ioctl = vfio_device_fops_compat_ioctl,
> -#endif
> + .compat_ioctl = compat_ptr_ioctl,
> .mmap = vfio_device_fops_mmap,
> };
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index df51a35cf537..1642b3573230 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -1765,14 +1765,6 @@ static long vhost_net_ioctl(struct file *f, unsigned int ioctl,
> }
> }
>
> -#ifdef CONFIG_COMPAT
> -static long vhost_net_compat_ioctl(struct file *f, unsigned int ioctl,
> - unsigned long arg)
> -{
> - return vhost_net_ioctl(f, ioctl, (unsigned long)compat_ptr(arg));
> -}
> -#endif
> -
> static ssize_t vhost_net_chr_read_iter(struct kiocb *iocb, struct iov_iter *to)
> {
> struct file *file = iocb->ki_filp;
> @@ -1808,9 +1800,7 @@ static const struct file_operations vhost_net_fops = {
> .write_iter = vhost_net_chr_write_iter,
> .poll = vhost_net_chr_poll,
> .unlocked_ioctl = vhost_net_ioctl,
> -#ifdef CONFIG_COMPAT
> - .compat_ioctl = vhost_net_compat_ioctl,
> -#endif
> + .compat_ioctl = compat_ptr_ioctl,
> .open = vhost_net_open,
> .llseek = noop_llseek,
> };
> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index 618fb6461017..f9b14c39d89b 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c
> @@ -1721,21 +1721,11 @@ vhost_scsi_ioctl(struct file *f,
> }
> }
>
> -#ifdef CONFIG_COMPAT
> -static long vhost_scsi_compat_ioctl(struct file *f, unsigned int ioctl,
> - unsigned long arg)
> -{
> - return vhost_scsi_ioctl(f, ioctl, (unsigned long)compat_ptr(arg));
> -}
> -#endif
> -
> static const struct file_operations vhost_scsi_fops = {
> .owner = THIS_MODULE,
> .release = vhost_scsi_release,
> .unlocked_ioctl = vhost_scsi_ioctl,
> -#ifdef CONFIG_COMPAT
> - .compat_ioctl = vhost_scsi_compat_ioctl,
> -#endif
> + .compat_ioctl = compat_ptr_ioctl,
> .open = vhost_scsi_open,
> .llseek = noop_llseek,
> };
> diff --git a/drivers/vhost/test.c b/drivers/vhost/test.c
> index 40589850eb33..61d4d98c8f70 100644
> --- a/drivers/vhost/test.c
> +++ b/drivers/vhost/test.c
> @@ -298,21 +298,11 @@ static long vhost_test_ioctl(struct file *f, unsigned int ioctl,
> }
> }
>
> -#ifdef CONFIG_COMPAT
> -static long vhost_test_compat_ioctl(struct file *f, unsigned int ioctl,
> - unsigned long arg)
> -{
> - return vhost_test_ioctl(f, ioctl, (unsigned long)compat_ptr(arg));
> -}
> -#endif
> -
> static const struct file_operations vhost_test_fops = {
> .owner = THIS_MODULE,
> .release = vhost_test_release,
> .unlocked_ioctl = vhost_test_ioctl,
> -#ifdef CONFIG_COMPAT
> - .compat_ioctl = vhost_test_compat_ioctl,
> -#endif
> + .compat_ioctl = compat_ptr_ioctl,
> .open = vhost_test_open,
> .llseek = noop_llseek,
> };
> diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> index bb5fc0e9fbc2..9a86202678b6 100644
> --- a/drivers/vhost/vsock.c
> +++ b/drivers/vhost/vsock.c
> @@ -716,23 +716,13 @@ static long vhost_vsock_dev_ioctl(struct file *f, unsigned int ioctl,
> }
> }
>
> -#ifdef CONFIG_COMPAT
> -static long vhost_vsock_dev_compat_ioctl(struct file *f, unsigned int ioctl,
> - unsigned long arg)
> -{
> - return vhost_vsock_dev_ioctl(f, ioctl, (unsigned long)compat_ptr(arg));
> -}
> -#endif
> -
> static const struct file_operations vhost_vsock_fops = {
> .owner = THIS_MODULE,
> .open = vhost_vsock_dev_open,
> .release = vhost_vsock_dev_release,
> .llseek = noop_llseek,
> .unlocked_ioctl = vhost_vsock_dev_ioctl,
> -#ifdef CONFIG_COMPAT
> - .compat_ioctl = vhost_vsock_dev_compat_ioctl,
> -#endif
> + .compat_ioctl = compat_ptr_ioctl,
> };
>
> static struct miscdevice vhost_vsock_misc = {
> diff --git a/fs/fat/file.c b/fs/fat/file.c
> index b3bed32946b1..f173d9261115 100644
> --- a/fs/fat/file.c
> +++ b/fs/fat/file.c
> @@ -171,15 +171,6 @@ long fat_generic_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> }
> }
>
> -#ifdef CONFIG_COMPAT
> -static long fat_generic_compat_ioctl(struct file *filp, unsigned int cmd,
> - unsigned long arg)
> -
> -{
> - return fat_generic_ioctl(filp, cmd, (unsigned long)compat_ptr(arg));
> -}
> -#endif
> -
> static int fat_file_release(struct inode *inode, struct file *filp)
> {
> if ((filp->f_mode & FMODE_WRITE) &&
> @@ -209,9 +200,7 @@ const struct file_operations fat_file_operations = {
> .mmap = generic_file_mmap,
> .release = fat_file_release,
> .unlocked_ioctl = fat_generic_ioctl,
> -#ifdef CONFIG_COMPAT
> - .compat_ioctl = fat_generic_compat_ioctl,
> -#endif
> + .compat_ioctl = compat_ptr_ioctl,
> .fsync = fat_file_fsync,
> .splice_read = generic_file_splice_read,
> .splice_write = iter_file_splice_write,
> --
> 2.20.0
^ permalink raw reply
* Re: [RFC 2/3] hw/virtio-rdma: VirtIO rdma device
From: Michael S. Tsirkin @ 2019-04-19 23:20 UTC (permalink / raw)
To: Yuval Shaia; +Cc: linux-rdma, cohuck, qemu-devel, virtualization, jgg
In-Reply-To: <20190411110157.14252-3-yuval.shaia@oracle.com>
On Thu, Apr 11, 2019 at 02:01:56PM +0300, Yuval Shaia wrote:
> diff --git a/include/standard-headers/linux/virtio_ids.h b/include/standard-headers/linux/virtio_ids.h
> index 6d5c3b2d4f..bd2c699450 100644
> --- a/include/standard-headers/linux/virtio_ids.h
> +++ b/include/standard-headers/linux/virtio_ids.h
> @@ -43,5 +43,6 @@
> #define VIRTIO_ID_INPUT 18 /* virtio input */
> #define VIRTIO_ID_VSOCK 19 /* virtio vsock transport */
> #define VIRTIO_ID_CRYPTO 20 /* virtio crypto */
> +#define VIRTIO_ID_RDMA 26 /* virtio crypto */
>
> #endif /* _LINUX_VIRTIO_IDS_H */
Please reserve an ID with the virtio TC.
Process:
- create a github issue
- post patch with Fixes: tag
- wait a bit
- if no comments on either ask for a vote
> --
> 2.20.1
^ permalink raw reply
* RE: [PATCH v3 09/26] compat_ioctl: move drivers to compat_ptr_ioctl
From: Winkler, Tomas @ 2019-04-20 8:03 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-usb@vger.kernel.org, kvm@vger.kernel.org,
Michael S. Tsirkin, Alexander Shishkin, Jarkko Sakkinen,
virtualization@lists.linux-foundation.org, Benjamin Tissoires,
linux-mtd@lists.infradead.org, Peter Huewe,
linux1394-devel@lists.sourceforge.net,
linux-stm32@st-md-mailman.stormreply.com,
devel@driverdev.osuosl.org, Marek Vasut, y2038@lists.linaro.org,
Richard Weinberger, Jason Gunthorpe
In-Reply-To: <20190419191633-mutt-send-email-mst@kernel.org>
>
> On Tue, Apr 16, 2019 at 10:19:47PM +0200, Arnd Bergmann wrote:
> > Each of these drivers has a copy of the same trivial helper function
> > to convert the pointer argument and then call the native ioctl handler.
> >
> > We now have a generic implementation of that, so use it.
> >
> > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Tomas Winkler <tomas.winkler@intel.com>
>
> > ---
> > drivers/char/ppdev.c | 12 +---------
> > drivers/char/tpm/tpm_vtpm_proxy.c | 12 +---------
> > drivers/firewire/core-cdev.c | 12 +---------
> > drivers/hid/usbhid/hiddev.c | 11 +--------
> > drivers/hwtracing/stm/core.c | 12 +---------
> > drivers/misc/mei/main.c | 22 +----------------
> > drivers/mtd/ubi/cdev.c | 36 +++-------------------------
> > drivers/net/tap.c | 12 +---------
> > drivers/staging/pi433/pi433_if.c | 12 +---------
> > drivers/usb/core/devio.c | 16 +------------
> > drivers/vfio/vfio.c | 39 +++----------------------------
> > drivers/vhost/net.c | 12 +---------
> > drivers/vhost/scsi.c | 12 +---------
> > drivers/vhost/test.c | 12 +---------
> > drivers/vhost/vsock.c | 12 +---------
> > fs/fat/file.c | 13 +----------
> > 16 files changed, 20 insertions(+), 237 deletions(-)
> >
> > diff --git a/drivers/char/ppdev.c b/drivers/char/ppdev.c index
> > 1ae77b41050a..e96c8d9623e0 100644
> > --- a/drivers/char/ppdev.c
> > +++ b/drivers/char/ppdev.c
> > @@ -674,14 +674,6 @@ static long pp_ioctl(struct file *file, unsigned int
> cmd, unsigned long arg)
> > return ret;
> > }
> >
> > -#ifdef CONFIG_COMPAT
> > -static long pp_compat_ioctl(struct file *file, unsigned int cmd,
> > - unsigned long arg)
> > -{
> > - return pp_ioctl(file, cmd, (unsigned long)compat_ptr(arg));
> > -}
> > -#endif
> > -
> > static int pp_open(struct inode *inode, struct file *file) {
> > unsigned int minor = iminor(inode);
> > @@ -790,9 +782,7 @@ static const struct file_operations pp_fops = {
> > .write = pp_write,
> > .poll = pp_poll,
> > .unlocked_ioctl = pp_ioctl,
> > -#ifdef CONFIG_COMPAT
> > - .compat_ioctl = pp_compat_ioctl,
> > -#endif
> > + .compat_ioctl = compat_ptr_ioctl,
> > .open = pp_open,
> > .release = pp_release,
> > };
> > diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c
> > b/drivers/char/tpm/tpm_vtpm_proxy.c
> > index d74f3de74ae6..fb845f0a430b 100644
> > --- a/drivers/char/tpm/tpm_vtpm_proxy.c
> > +++ b/drivers/char/tpm/tpm_vtpm_proxy.c
> > @@ -675,20 +675,10 @@ static long vtpmx_fops_ioctl(struct file *f, unsigned
> int ioctl,
> > }
> > }
> >
> > -#ifdef CONFIG_COMPAT
> > -static long vtpmx_fops_compat_ioctl(struct file *f, unsigned int ioctl,
> > - unsigned long arg)
> > -{
> > - return vtpmx_fops_ioctl(f, ioctl, (unsigned long)compat_ptr(arg));
> > -}
> > -#endif
> > -
> > static const struct file_operations vtpmx_fops = {
> > .owner = THIS_MODULE,
> > .unlocked_ioctl = vtpmx_fops_ioctl,
> > -#ifdef CONFIG_COMPAT
> > - .compat_ioctl = vtpmx_fops_compat_ioctl,
> > -#endif
> > + .compat_ioctl = compat_ptr_ioctl,
> > .llseek = noop_llseek,
> > };
> >
> > diff --git a/drivers/firewire/core-cdev.c
> > b/drivers/firewire/core-cdev.c index 16a7045736a9..fb934680fdd3 100644
> > --- a/drivers/firewire/core-cdev.c
> > +++ b/drivers/firewire/core-cdev.c
> > @@ -1659,14 +1659,6 @@ static long fw_device_op_ioctl(struct file *file,
> > return dispatch_ioctl(file->private_data, cmd, (void __user *)arg);
> > }
> >
> > -#ifdef CONFIG_COMPAT
> > -static long fw_device_op_compat_ioctl(struct file *file,
> > - unsigned int cmd, unsigned long arg)
> > -{
> > - return dispatch_ioctl(file->private_data, cmd, compat_ptr(arg));
> > -}
> > -#endif
> > -
> > static int fw_device_op_mmap(struct file *file, struct vm_area_struct
> > *vma) {
> > struct client *client = file->private_data; @@ -1808,7 +1800,5 @@
> > const struct file_operations fw_device_ops = {
> > .mmap = fw_device_op_mmap,
> > .release = fw_device_op_release,
> > .poll = fw_device_op_poll,
> > -#ifdef CONFIG_COMPAT
> > - .compat_ioctl = fw_device_op_compat_ioctl,
> > -#endif
> > + .compat_ioctl = compat_ptr_ioctl,
> > };
> > diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
> > index a746017fac17..ef4a1cd389d6 100644
> > --- a/drivers/hid/usbhid/hiddev.c
> > +++ b/drivers/hid/usbhid/hiddev.c
> > @@ -855,13 +855,6 @@ static long hiddev_ioctl(struct file *file, unsigned int
> cmd, unsigned long arg)
> > return r;
> > }
> >
> > -#ifdef CONFIG_COMPAT
> > -static long hiddev_compat_ioctl(struct file *file, unsigned int cmd,
> > unsigned long arg) -{
> > - return hiddev_ioctl(file, cmd, (unsigned long)compat_ptr(arg));
> > -}
> > -#endif
> > -
> > static const struct file_operations hiddev_fops = {
> > .owner = THIS_MODULE,
> > .read = hiddev_read,
> > @@ -871,9 +864,7 @@ static const struct file_operations hiddev_fops = {
> > .release = hiddev_release,
> > .unlocked_ioctl = hiddev_ioctl,
> > .fasync = hiddev_fasync,
> > -#ifdef CONFIG_COMPAT
> > - .compat_ioctl = hiddev_compat_ioctl,
> > -#endif
> > + .compat_ioctl = compat_ptr_ioctl,
> > .llseek = noop_llseek,
> > };
> >
> > diff --git a/drivers/hwtracing/stm/core.c
> > b/drivers/hwtracing/stm/core.c index c7ba8acfd4d5..454da259f144 100644
> > --- a/drivers/hwtracing/stm/core.c
> > +++ b/drivers/hwtracing/stm/core.c
> > @@ -840,23 +840,13 @@ stm_char_ioctl(struct file *file, unsigned int cmd,
> unsigned long arg)
> > return err;
> > }
> >
> > -#ifdef CONFIG_COMPAT
> > -static long
> > -stm_char_compat_ioctl(struct file *file, unsigned int cmd, unsigned
> > long arg) -{
> > - return stm_char_ioctl(file, cmd, (unsigned long)compat_ptr(arg));
> > -}
> > -#else
> > -#define stm_char_compat_ioctl NULL
> > -#endif
> > -
> > static const struct file_operations stm_fops = {
> > .open = stm_char_open,
> > .release = stm_char_release,
> > .write = stm_char_write,
> > .mmap = stm_char_mmap,
> > .unlocked_ioctl = stm_char_ioctl,
> > - .compat_ioctl = stm_char_compat_ioctl,
> > + .compat_ioctl = compat_ptr_ioctl,
> > .llseek = no_llseek,
> > };
> >
> > diff --git a/drivers/misc/mei/main.c b/drivers/misc/mei/main.c index
> > 87281b3695e6..cc6af92cdef0 100644
> > --- a/drivers/misc/mei/main.c
> > +++ b/drivers/misc/mei/main.c
> > @@ -535,24 +535,6 @@ static long mei_ioctl(struct file *file, unsigned int
> cmd, unsigned long data)
> > return rets;
> > }
> >
> > -/**
> > - * mei_compat_ioctl - the compat IOCTL function
> > - *
> > - * @file: pointer to file structure
> > - * @cmd: ioctl command
> > - * @data: pointer to mei message structure
> > - *
> > - * Return: 0 on success , <0 on error
> > - */
> > -#ifdef CONFIG_COMPAT
> > -static long mei_compat_ioctl(struct file *file,
> > - unsigned int cmd, unsigned long data)
> > -{
> > - return mei_ioctl(file, cmd, (unsigned long)compat_ptr(data));
> > -}
> > -#endif
> > -
> > -
> > /**
> > * mei_poll - the poll function
> > *
> > @@ -855,9 +837,7 @@ static const struct file_operations mei_fops = {
> > .owner = THIS_MODULE,
> > .read = mei_read,
> > .unlocked_ioctl = mei_ioctl,
> > -#ifdef CONFIG_COMPAT
> > - .compat_ioctl = mei_compat_ioctl,
> > -#endif
> > + .compat_ioctl = compat_ptr_ioctl,
> > .open = mei_open,
> > .release = mei_release,
> > .write = mei_write,
> > diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c index
> > 947a8adbc799..265d34fa3efa 100644
> > --- a/drivers/mtd/ubi/cdev.c
> > +++ b/drivers/mtd/ubi/cdev.c
> > @@ -1091,36 +1091,6 @@ static long ctrl_cdev_ioctl(struct file *file,
> unsigned int cmd,
> > return err;
> > }
> >
> > -#ifdef CONFIG_COMPAT
> > -static long vol_cdev_compat_ioctl(struct file *file, unsigned int cmd,
> > - unsigned long arg)
> > -{
> > - unsigned long translated_arg = (unsigned long)compat_ptr(arg);
> > -
> > - return vol_cdev_ioctl(file, cmd, translated_arg);
> > -}
> > -
> > -static long ubi_cdev_compat_ioctl(struct file *file, unsigned int cmd,
> > - unsigned long arg)
> > -{
> > - unsigned long translated_arg = (unsigned long)compat_ptr(arg);
> > -
> > - return ubi_cdev_ioctl(file, cmd, translated_arg);
> > -}
> > -
> > -static long ctrl_cdev_compat_ioctl(struct file *file, unsigned int cmd,
> > - unsigned long arg)
> > -{
> > - unsigned long translated_arg = (unsigned long)compat_ptr(arg);
> > -
> > - return ctrl_cdev_ioctl(file, cmd, translated_arg);
> > -}
> > -#else
> > -#define vol_cdev_compat_ioctl NULL
> > -#define ubi_cdev_compat_ioctl NULL
> > -#define ctrl_cdev_compat_ioctl NULL
> > -#endif
> > -
> > /* UBI volume character device operations */ const struct
> > file_operations ubi_vol_cdev_operations = {
> > .owner = THIS_MODULE,
> > @@ -1131,7 +1101,7 @@ const struct file_operations
> ubi_vol_cdev_operations = {
> > .write = vol_cdev_write,
> > .fsync = vol_cdev_fsync,
> > .unlocked_ioctl = vol_cdev_ioctl,
> > - .compat_ioctl = vol_cdev_compat_ioctl,
> > + .compat_ioctl = compat_ptr_ioctl,
> > };
> >
> > /* UBI character device operations */ @@ -1139,13 +1109,13 @@ const
> > struct file_operations ubi_cdev_operations = {
> > .owner = THIS_MODULE,
> > .llseek = no_llseek,
> > .unlocked_ioctl = ubi_cdev_ioctl,
> > - .compat_ioctl = ubi_cdev_compat_ioctl,
> > + .compat_ioctl = compat_ptr_ioctl,
> > };
> >
> > /* UBI control character device operations */ const struct
> > file_operations ubi_ctrl_cdev_operations = {
> > .owner = THIS_MODULE,
> > .unlocked_ioctl = ctrl_cdev_ioctl,
> > - .compat_ioctl = ctrl_cdev_compat_ioctl,
> > + .compat_ioctl = compat_ptr_ioctl,
> > .llseek = no_llseek,
> > };
> > diff --git a/drivers/net/tap.c b/drivers/net/tap.c index
> > 2ea9b4976f4a..ebe425e65992 100644
> > --- a/drivers/net/tap.c
> > +++ b/drivers/net/tap.c
> > @@ -1123,14 +1123,6 @@ static long tap_ioctl(struct file *file, unsigned int
> cmd,
> > }
> > }
> >
> > -#ifdef CONFIG_COMPAT
> > -static long tap_compat_ioctl(struct file *file, unsigned int cmd,
> > - unsigned long arg)
> > -{
> > - return tap_ioctl(file, cmd, (unsigned long)compat_ptr(arg));
> > -}
> > -#endif
> > -
> > static const struct file_operations tap_fops = {
> > .owner = THIS_MODULE,
> > .open = tap_open,
> > @@ -1140,9 +1132,7 @@ static const struct file_operations tap_fops = {
> > .poll = tap_poll,
> > .llseek = no_llseek,
> > .unlocked_ioctl = tap_ioctl,
> > -#ifdef CONFIG_COMPAT
> > - .compat_ioctl = tap_compat_ioctl,
> > -#endif
> > + .compat_ioctl = compat_ptr_ioctl,
> > };
> >
> > static int tap_get_user_xdp(struct tap_queue *q, struct xdp_buff
> > *xdp) diff --git a/drivers/staging/pi433/pi433_if.c
> > b/drivers/staging/pi433/pi433_if.c
> > index b2314636dc89..ab7dfc7c2917 100644
> > --- a/drivers/staging/pi433/pi433_if.c
> > +++ b/drivers/staging/pi433/pi433_if.c
> > @@ -935,16 +935,6 @@ pi433_ioctl(struct file *filp, unsigned int cmd,
> unsigned long arg)
> > return retval;
> > }
> >
> > -#ifdef CONFIG_COMPAT
> > -static long
> > -pi433_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long
> > arg) -{
> > - return pi433_ioctl(filp, cmd, (unsigned long)compat_ptr(arg));
> > -}
> > -#else
> > -#define pi433_compat_ioctl NULL
> > -#endif /* CONFIG_COMPAT */
> > -
> >
> > /*--------------------------------------------------------------------
> > -----*/
> >
> > static int pi433_open(struct inode *inode, struct file *filp) @@
> > -1101,7 +1091,7 @@ static const struct file_operations pi433_fops = {
> > .write = pi433_write,
> > .read = pi433_read,
> > .unlocked_ioctl = pi433_ioctl,
> > - .compat_ioctl = pi433_compat_ioctl,
> > + .compat_ioctl = compat_ptr_ioctl,
> > .open = pi433_open,
> > .release = pi433_release,
> > .llseek = no_llseek,
> > diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c index
> > fa783531ee88..d75052b36584 100644
> > --- a/drivers/usb/core/devio.c
> > +++ b/drivers/usb/core/devio.c
> > @@ -2568,18 +2568,6 @@ static long usbdev_ioctl(struct file *file, unsigned
> int cmd,
> > return ret;
> > }
> >
> > -#ifdef CONFIG_COMPAT
> > -static long usbdev_compat_ioctl(struct file *file, unsigned int cmd,
> > - unsigned long arg)
> > -{
> > - int ret;
> > -
> > - ret = usbdev_do_ioctl(file, cmd, compat_ptr(arg));
> > -
> > - return ret;
> > -}
> > -#endif
> > -
> > /* No kernel lock - fine */
> > static __poll_t usbdev_poll(struct file *file,
> > struct poll_table_struct *wait)
> > @@ -2603,9 +2591,7 @@ const struct file_operations usbdev_file_operations
> = {
> > .read = usbdev_read,
> > .poll = usbdev_poll,
> > .unlocked_ioctl = usbdev_ioctl,
> > -#ifdef CONFIG_COMPAT
> > - .compat_ioctl = usbdev_compat_ioctl,
> > -#endif
> > + .compat_ioctl = compat_ptr_ioctl,
> > .mmap = usbdev_mmap,
> > .open = usbdev_open,
> > .release = usbdev_release,
> > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c index
> > a3030cdf3c18..a5efe82584a5 100644
> > --- a/drivers/vfio/vfio.c
> > +++ b/drivers/vfio/vfio.c
> > @@ -1200,15 +1200,6 @@ static long vfio_fops_unl_ioctl(struct file *filep,
> > return ret;
> > }
> >
> > -#ifdef CONFIG_COMPAT
> > -static long vfio_fops_compat_ioctl(struct file *filep,
> > - unsigned int cmd, unsigned long arg)
> > -{
> > - arg = (unsigned long)compat_ptr(arg);
> > - return vfio_fops_unl_ioctl(filep, cmd, arg);
> > -}
> > -#endif /* CONFIG_COMPAT */
> > -
> > static int vfio_fops_open(struct inode *inode, struct file *filep) {
> > struct vfio_container *container;
> > @@ -1291,9 +1282,7 @@ static const struct file_operations vfio_fops = {
> > .read = vfio_fops_read,
> > .write = vfio_fops_write,
> > .unlocked_ioctl = vfio_fops_unl_ioctl,
> > -#ifdef CONFIG_COMPAT
> > - .compat_ioctl = vfio_fops_compat_ioctl,
> > -#endif
> > + .compat_ioctl = compat_ptr_ioctl,
> > .mmap = vfio_fops_mmap,
> > };
> >
> > @@ -1572,15 +1561,6 @@ static long vfio_group_fops_unl_ioctl(struct file
> *filep,
> > return ret;
> > }
> >
> > -#ifdef CONFIG_COMPAT
> > -static long vfio_group_fops_compat_ioctl(struct file *filep,
> > - unsigned int cmd, unsigned long arg)
> > -{
> > - arg = (unsigned long)compat_ptr(arg);
> > - return vfio_group_fops_unl_ioctl(filep, cmd, arg);
> > -}
> > -#endif /* CONFIG_COMPAT */
> > -
> > static int vfio_group_fops_open(struct inode *inode, struct file
> > *filep) {
> > struct vfio_group *group;
> > @@ -1636,9 +1616,7 @@ static int vfio_group_fops_release(struct inode
> > *inode, struct file *filep) static const struct file_operations vfio_group_fops =
> {
> > .owner = THIS_MODULE,
> > .unlocked_ioctl = vfio_group_fops_unl_ioctl,
> > -#ifdef CONFIG_COMPAT
> > - .compat_ioctl = vfio_group_fops_compat_ioctl,
> > -#endif
> > + .compat_ioctl = compat_ptr_ioctl,
> > .open = vfio_group_fops_open,
> > .release = vfio_group_fops_release,
> > };
> > @@ -1703,24 +1681,13 @@ static int vfio_device_fops_mmap(struct file
> *filep, struct vm_area_struct *vma)
> > return device->ops->mmap(device->device_data, vma); }
> >
> > -#ifdef CONFIG_COMPAT
> > -static long vfio_device_fops_compat_ioctl(struct file *filep,
> > - unsigned int cmd, unsigned long arg)
> > -{
> > - arg = (unsigned long)compat_ptr(arg);
> > - return vfio_device_fops_unl_ioctl(filep, cmd, arg);
> > -}
> > -#endif /* CONFIG_COMPAT */
> > -
> > static const struct file_operations vfio_device_fops = {
> > .owner = THIS_MODULE,
> > .release = vfio_device_fops_release,
> > .read = vfio_device_fops_read,
> > .write = vfio_device_fops_write,
> > .unlocked_ioctl = vfio_device_fops_unl_ioctl,
> > -#ifdef CONFIG_COMPAT
> > - .compat_ioctl = vfio_device_fops_compat_ioctl,
> > -#endif
> > + .compat_ioctl = compat_ptr_ioctl,
> > .mmap = vfio_device_fops_mmap,
> > };
> >
> > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c index
> > df51a35cf537..1642b3573230 100644
> > --- a/drivers/vhost/net.c
> > +++ b/drivers/vhost/net.c
> > @@ -1765,14 +1765,6 @@ static long vhost_net_ioctl(struct file *f, unsigned
> int ioctl,
> > }
> > }
> >
> > -#ifdef CONFIG_COMPAT
> > -static long vhost_net_compat_ioctl(struct file *f, unsigned int ioctl,
> > - unsigned long arg)
> > -{
> > - return vhost_net_ioctl(f, ioctl, (unsigned long)compat_ptr(arg));
> > -}
> > -#endif
> > -
> > static ssize_t vhost_net_chr_read_iter(struct kiocb *iocb, struct
> > iov_iter *to) {
> > struct file *file = iocb->ki_filp;
> > @@ -1808,9 +1800,7 @@ static const struct file_operations vhost_net_fops =
> {
> > .write_iter = vhost_net_chr_write_iter,
> > .poll = vhost_net_chr_poll,
> > .unlocked_ioctl = vhost_net_ioctl,
> > -#ifdef CONFIG_COMPAT
> > - .compat_ioctl = vhost_net_compat_ioctl,
> > -#endif
> > + .compat_ioctl = compat_ptr_ioctl,
> > .open = vhost_net_open,
> > .llseek = noop_llseek,
> > };
> > diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c index
> > 618fb6461017..f9b14c39d89b 100644
> > --- a/drivers/vhost/scsi.c
> > +++ b/drivers/vhost/scsi.c
> > @@ -1721,21 +1721,11 @@ vhost_scsi_ioctl(struct file *f,
> > }
> > }
> >
> > -#ifdef CONFIG_COMPAT
> > -static long vhost_scsi_compat_ioctl(struct file *f, unsigned int ioctl,
> > - unsigned long arg)
> > -{
> > - return vhost_scsi_ioctl(f, ioctl, (unsigned long)compat_ptr(arg));
> > -}
> > -#endif
> > -
> > static const struct file_operations vhost_scsi_fops = {
> > .owner = THIS_MODULE,
> > .release = vhost_scsi_release,
> > .unlocked_ioctl = vhost_scsi_ioctl,
> > -#ifdef CONFIG_COMPAT
> > - .compat_ioctl = vhost_scsi_compat_ioctl,
> > -#endif
> > + .compat_ioctl = compat_ptr_ioctl,
> > .open = vhost_scsi_open,
> > .llseek = noop_llseek,
> > };
> > diff --git a/drivers/vhost/test.c b/drivers/vhost/test.c index
> > 40589850eb33..61d4d98c8f70 100644
> > --- a/drivers/vhost/test.c
> > +++ b/drivers/vhost/test.c
> > @@ -298,21 +298,11 @@ static long vhost_test_ioctl(struct file *f, unsigned
> int ioctl,
> > }
> > }
> >
> > -#ifdef CONFIG_COMPAT
> > -static long vhost_test_compat_ioctl(struct file *f, unsigned int ioctl,
> > - unsigned long arg)
> > -{
> > - return vhost_test_ioctl(f, ioctl, (unsigned long)compat_ptr(arg));
> > -}
> > -#endif
> > -
> > static const struct file_operations vhost_test_fops = {
> > .owner = THIS_MODULE,
> > .release = vhost_test_release,
> > .unlocked_ioctl = vhost_test_ioctl,
> > -#ifdef CONFIG_COMPAT
> > - .compat_ioctl = vhost_test_compat_ioctl,
> > -#endif
> > + .compat_ioctl = compat_ptr_ioctl,
> > .open = vhost_test_open,
> > .llseek = noop_llseek,
> > };
> > diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c index
> > bb5fc0e9fbc2..9a86202678b6 100644
> > --- a/drivers/vhost/vsock.c
> > +++ b/drivers/vhost/vsock.c
> > @@ -716,23 +716,13 @@ static long vhost_vsock_dev_ioctl(struct file *f,
> unsigned int ioctl,
> > }
> > }
> >
> > -#ifdef CONFIG_COMPAT
> > -static long vhost_vsock_dev_compat_ioctl(struct file *f, unsigned int ioctl,
> > - unsigned long arg)
> > -{
> > - return vhost_vsock_dev_ioctl(f, ioctl, (unsigned long)compat_ptr(arg));
> > -}
> > -#endif
> > -
> > static const struct file_operations vhost_vsock_fops = {
> > .owner = THIS_MODULE,
> > .open = vhost_vsock_dev_open,
> > .release = vhost_vsock_dev_release,
> > .llseek = noop_llseek,
> > .unlocked_ioctl = vhost_vsock_dev_ioctl, -#ifdef CONFIG_COMPAT
> > - .compat_ioctl = vhost_vsock_dev_compat_ioctl,
> > -#endif
> > + .compat_ioctl = compat_ptr_ioctl,
> > };
> >
> > static struct miscdevice vhost_vsock_misc = { diff --git
> > a/fs/fat/file.c b/fs/fat/file.c index b3bed32946b1..f173d9261115
> > 100644
> > --- a/fs/fat/file.c
> > +++ b/fs/fat/file.c
> > @@ -171,15 +171,6 @@ long fat_generic_ioctl(struct file *filp, unsigned int
> cmd, unsigned long arg)
> > }
> > }
> >
> > -#ifdef CONFIG_COMPAT
> > -static long fat_generic_compat_ioctl(struct file *filp, unsigned int cmd,
> > - unsigned long arg)
> > -
> > -{
> > - return fat_generic_ioctl(filp, cmd, (unsigned long)compat_ptr(arg));
> > -}
> > -#endif
> > -
> > static int fat_file_release(struct inode *inode, struct file *filp)
> > {
> > if ((filp->f_mode & FMODE_WRITE) &&
> > @@ -209,9 +200,7 @@ const struct file_operations fat_file_operations = {
> > .mmap = generic_file_mmap,
> > .release = fat_file_release,
> > .unlocked_ioctl = fat_generic_ioctl,
> > -#ifdef CONFIG_COMPAT
> > - .compat_ioctl = fat_generic_compat_ioctl,
> > -#endif
> > + .compat_ioctl = compat_ptr_ioctl,
> > .fsync = fat_file_fsync,
> > .splice_read = generic_file_splice_read,
> > .splice_write = iter_file_splice_write,
> > --
> > 2.20.0
^ permalink raw reply
* Re: [PATCH] vhost: remove unneeded linux/miscdevice.h include
From: Corentin Labbe @ 2019-04-20 18:05 UTC (permalink / raw)
To: mst, jasowang; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20170918165854.23408-1-clabbe.montjoie@gmail.com>
On Mon, Sep 18, 2017 at 06:58:54PM +0200, Corentin Labbe wrote:
> drivers/vhost/vhost.c does not use any miscdevice, so this patch
> remove this unnecessary inclusion.
>
> Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
> ---
> drivers/vhost/vhost.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 33ac2b186b85..33ab839696f9 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -16,7 +16,6 @@
> #include <linux/uio.h>
> #include <linux/mm.h>
> #include <linux/mmu_context.h>
> -#include <linux/miscdevice.h>
> #include <linux/mutex.h>
> #include <linux/poll.h>
> #include <linux/file.h>
> --
> 2.13.5
>
Hello
gentle ping
Regards
^ permalink raw reply
* Re: Stats for XDP actions
From: David Ahern @ 2019-04-21 0:16 UTC (permalink / raw)
To: Toke Høiland-Jørgensen, Jesper Dangaard Brouer
Cc: Jakub Kicinski, hawk, mst, netdev, John Fastabend, virtualization,
Saeed Mahameed, Daniel Borkmann, David Miller, Tariq Toukan
In-Reply-To: <87tvevpf0y.fsf@toke.dk>
On 4/18/19 8:24 AM, Toke Høiland-Jørgensen wrote:
>>>
>>
>> Understood. Hopefully in March I will get some time to come back to this
>> and propose an idea on what I would like to see - namely, the admin has
>> a config option at load time to enable driver counters versus custom map
>> counters. (meaning the operator of the node chooses standard stats over
>> strict performance.) But of course that means the drivers have the code
>> to collect those stats.
>
> Hi David
>
> I don't recall seeing any follow-up on this. Did you have a chance to
> formulate your ideas? :)
>
Not yet. Almost done with the nexthop changes. Once that is out of the
way I can come back to this.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [Qemu-devel] [RFC 0/3] VirtIO RDMA
From: Leon Romanovsky @ 2019-04-22 6:00 UTC (permalink / raw)
To: Hannes Reinecke
Cc: mst, linux-rdma, Cornelia Huck, qemu-devel, Yuval Shaia,
virtualization, jgg
In-Reply-To: <e73e03c2-ea2b-6ffc-cd23-e8e44d42ce80@suse.de>
On Fri, Apr 19, 2019 at 01:16:06PM +0200, Hannes Reinecke wrote:
> On 4/15/19 12:35 PM, Yuval Shaia wrote:
> > On Thu, Apr 11, 2019 at 07:02:15PM +0200, Cornelia Huck wrote:
> > > On Thu, 11 Apr 2019 14:01:54 +0300
> > > Yuval Shaia <yuval.shaia@oracle.com> wrote:
> > >
> > > > Data center backends use more and more RDMA or RoCE devices and more and
> > > > more software runs in virtualized environment.
> > > > There is a need for a standard to enable RDMA/RoCE on Virtual Machines.
> > > >
> > > > Virtio is the optimal solution since is the de-facto para-virtualizaton
> > > > technology and also because the Virtio specification
> > > > allows Hardware Vendors to support Virtio protocol natively in order to
> > > > achieve bare metal performance.
> > > >
> > > > This RFC is an effort to addresses challenges in defining the RDMA/RoCE
> > > > Virtio Specification and a look forward on possible implementation
> > > > techniques.
> > > >
> > > > Open issues/Todo list:
> > > > List is huge, this is only start point of the project.
> > > > Anyway, here is one example of item in the list:
> > > > - Multi VirtQ: Every QP has two rings and every CQ has one. This means that
> > > > in order to support for example 32K QPs we will need 64K VirtQ. Not sure
> > > > that this is reasonable so one option is to have one for all and
> > > > multiplex the traffic on it. This is not good approach as by design it
> > > > introducing an optional starvation. Another approach would be multi
> > > > queues and round-robin (for example) between them.
> > > >
> Typically there will be a one-to-one mapping between QPs and CPUs (on the
> guest). So while one would need to be prepared to support quite some QPs,
> the expectation is that the actual number of QPs used will be rather low.
> In a similar vein, multiplexing QPs would be defeating the purpose, as the
> overall idea was to have _independent_ QPs to enhance parallelism.
>
> > > > Expectations from this posting:
> > > > In general, any comment is welcome, starting from hey, drop this as it is a
> > > > very bad idea, to yeah, go ahead, we really want it.
> > > > Idea here is that since it is not a minor effort i first want to know if
> > > > there is some sort interest in the community for such device.
> > >
> > > My first reaction is: Sounds sensible, but it would be good to have a
> > > spec for this :)
> > >
> > > You'll need a spec if you want this to go forward anyway, so at least a
> > > sketch would be good to answer questions such as how many virtqueues
> > > you use for which purpose, what is actually put on the virtqueues,
> > > whether there are negotiable features, and what the expectations for
> > > the device and the driver are. It also makes it easier to understand
> > > how this is supposed to work in practice.
> > >
> > > If folks agree that this sounds useful, the next step would be to
> > > reserve an id for the device type.
> >
> > Thanks for the tips, will sure do that, it is that first i wanted to make
> > sure there is a use case here.
> >
> > Waiting for any feedback from the community.
> >
> I really do like the ides; in fact, it saved me from coding a similar thing
> myself :-)
>
> However, I'm still curious about the overall intent of this driver. Where
> would the I/O be routed _to_ ?
> It's nice that we have a virtualized driver, but this driver is
> intended to do I/O (even if it doesn't _do_ any I/O ATM :-)
> And this I/O needs to be send to (and possibly received from)
> something.
>
> So what exactly is this something?
> An existing piece of HW on the host?
> If so, wouldn't it be more efficient to use vfio, either by using SR-IOV or
> by using virtio-mdev?
>
> Another guest?
> If so, how would we route the I/O from one guest to the other?
> Shared memory? Implementing a full-blown RDMA switch in qemu?
>
> Oh, and I would _love_ to have a discussion about this at KVM Forum.
> Maybe I'll manage to whip up guest-to-guest RDMA connection using ivshmem
> ... let's see.
Following success in previous years to transfer ideas into code,
we started to prepare RDMA miniconference in LPC 2019, which will
be co-located with Kernel Summit and networking track.
I'm confident that such broad audience of kernel developers
will be good fit for such discussion.
Previous years:
2016: https://www.spinics.net/lists/linux-rdma/msg43074.html
2017: https://lwn.net/Articles/734163/
2018: It was so full in audience and intensive that I failed to
summarize it :(
Thanks
>
> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke Teamlead Storage & Networking
> hare@suse.de +49 911 74053 688
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N??rnberg
> GF: Felix Imend??rffer, Mary Higgins, Sri Rasiah
> HRB 21284 (AG N??rnberg)
^ permalink raw reply
* Re: [PATCH v5 1/6] libnvdimm: nd_region flush callback support
From: Jeff Moyer @ 2019-04-22 15:51 UTC (permalink / raw)
To: Dan Williams
Cc: Pankaj Gupta, cohuck, Jan Kara, KVM list, Michael S. Tsirkin,
david, Qemu Developers, virtualization, Andreas Dilger,
Ross Zwisler, Andrea Arcangeli, Dave Jiang, linux-nvdimm,
Vishal L Verma, Matthew Wilcox, Christoph Hellwig, Linux ACPI,
linux-ext4, Len Brown, kilobyte, Rik van Riel, yuval shaia,
Stefan Hajnoczi, Paolo Bonzini, lcapitulino
In-Reply-To: <CAPcyv4jPOF_+R6U17J_+88+X8LC-LSPCugA6gbKxuphuuFB2gg@mail.gmail.com>
Dan Williams <dan.j.williams@intel.com> writes:
> On Thu, Apr 18, 2019 at 9:18 AM Christoph Hellwig <hch@infradead.org> wrote:
>>
>> On Thu, Apr 18, 2019 at 09:05:05AM -0700, Dan Williams wrote:
>> > > > I'd either add a comment about avoiding retpoline overhead here or just
>> > > > make ->flush == NULL mean generic_nvdimm_flush(). Just so that people don't
>> > > > get confused by the code.
>> > >
>> > > Isn't this premature optimization? I really don't like adding things
>> > > like this without some numbers to show it's worth it.
>> >
>> > I don't think it's premature given this optimization technique is
>> > already being deployed elsewhere, see:
>> >
>> > https://lwn.net/Articles/774347/
>>
>> For one this one was backed by numbers, and second after feedback
>> from Linux we switched to the NULL pointer check instead.
>
> Ok I should have noticed the switch to NULL pointer check. However,
> the question still stands do we want everyone to run numbers to
> justify this optimization, or make it a new common kernel coding
> practice to do:
>
> if (!object->op)
> generic_op(object);
> else
> object->op(object);
>
> ...in hot paths?
I don't think nvdimm_flush is a hot path. Numbers of some
representative workload would prove one of us right.
-Jeff
^ permalink raw reply
* Re: [Qemu-devel] [RFC 0/3] VirtIO RDMA
From: Jason Gunthorpe @ 2019-04-22 16:45 UTC (permalink / raw)
To: Hannes Reinecke
Cc: mst, linux-rdma, Cornelia Huck, qemu-devel, Yuval Shaia,
virtualization
In-Reply-To: <e73e03c2-ea2b-6ffc-cd23-e8e44d42ce80@suse.de>
On Fri, Apr 19, 2019 at 01:16:06PM +0200, Hannes Reinecke wrote:
> On 4/15/19 12:35 PM, Yuval Shaia wrote:
> > On Thu, Apr 11, 2019 at 07:02:15PM +0200, Cornelia Huck wrote:
> > > On Thu, 11 Apr 2019 14:01:54 +0300
> > > Yuval Shaia <yuval.shaia@oracle.com> wrote:
> > >
> > > > Data center backends use more and more RDMA or RoCE devices and more and
> > > > more software runs in virtualized environment.
> > > > There is a need for a standard to enable RDMA/RoCE on Virtual Machines.
> > > >
> > > > Virtio is the optimal solution since is the de-facto para-virtualizaton
> > > > technology and also because the Virtio specification
> > > > allows Hardware Vendors to support Virtio protocol natively in order to
> > > > achieve bare metal performance.
> > > >
> > > > This RFC is an effort to addresses challenges in defining the RDMA/RoCE
> > > > Virtio Specification and a look forward on possible implementation
> > > > techniques.
> > > >
> > > > Open issues/Todo list:
> > > > List is huge, this is only start point of the project.
> > > > Anyway, here is one example of item in the list:
> > > > - Multi VirtQ: Every QP has two rings and every CQ has one. This means that
> > > > in order to support for example 32K QPs we will need 64K VirtQ. Not sure
> > > > that this is reasonable so one option is to have one for all and
> > > > multiplex the traffic on it. This is not good approach as by design it
> > > > introducing an optional starvation. Another approach would be multi
> > > > queues and round-robin (for example) between them.
> > > >
> Typically there will be a one-to-one mapping between QPs and CPUs (on the
> guest).
Er we are really overloading words here.. The typical expectation is
that a 'RDMA QP' will have thousands and thousands of instances on a
system.
Most likely I think mapping 1:1 a virtio queue to a 'RDMA QP, CQ, SRQ,
etc' is a bad idea...
> However, I'm still curious about the overall intent of this driver. Where
> would the I/O be routed _to_ ?
> It's nice that we have a virtualized driver, but this driver is
> intended to do I/O (even if it doesn't _do_ any I/O ATM :-)
> And this I/O needs to be send to (and possibly received from)
> something.
As yet I have never heard of public RDMA HW that could be coupled to a
virtio scheme. All HW defines their own queue ring buffer formats
without standardization.
> If so, wouldn't it be more efficient to use vfio, either by using SR-IOV or
> by using virtio-mdev?
Using PCI pass through means the guest has to have drivers for the
device. A generic, perhaps slower, virtio path has some appeal in some
cases.
> If so, how would we route the I/O from one guest to the other?
> Shared memory? Implementing a full-blown RDMA switch in qemu?
RoCE rides over the existing ethernet switching layer quemu plugs
into
So if you built a shared memory, local host only, virtio-rdma then
you'd probably run through the ethernet switch upon connection
establishment to match the participating VMs.
Jason
^ permalink raw reply
* Re: [PATCH v5 1/6] libnvdimm: nd_region flush callback support
From: Dan Williams @ 2019-04-22 19:44 UTC (permalink / raw)
To: Jeff Moyer
Cc: Pankaj Gupta, cohuck, Jan Kara, KVM list, Michael S. Tsirkin,
david, Qemu Developers, virtualization, Andreas Dilger,
Ross Zwisler, Andrea Arcangeli, Dave Jiang, linux-nvdimm,
Vishal L Verma, Matthew Wilcox, Christoph Hellwig, Linux ACPI,
linux-ext4, Len Brown, kilobyte, Rik van Riel, yuval shaia,
Stefan Hajnoczi, Paolo Bonzini, lcapitulino
In-Reply-To: <x497ebm82d4.fsf@segfault.boston.devel.redhat.com>
On Mon, Apr 22, 2019 at 8:59 AM Jeff Moyer <jmoyer@redhat.com> wrote:
>
> Dan Williams <dan.j.williams@intel.com> writes:
>
> > On Thu, Apr 18, 2019 at 9:18 AM Christoph Hellwig <hch@infradead.org> wrote:
> >>
> >> On Thu, Apr 18, 2019 at 09:05:05AM -0700, Dan Williams wrote:
> >> > > > I'd either add a comment about avoiding retpoline overhead here or just
> >> > > > make ->flush == NULL mean generic_nvdimm_flush(). Just so that people don't
> >> > > > get confused by the code.
> >> > >
> >> > > Isn't this premature optimization? I really don't like adding things
> >> > > like this without some numbers to show it's worth it.
> >> >
> >> > I don't think it's premature given this optimization technique is
> >> > already being deployed elsewhere, see:
> >> >
> >> > https://lwn.net/Articles/774347/
> >>
> >> For one this one was backed by numbers, and second after feedback
> >> from Linux we switched to the NULL pointer check instead.
> >
> > Ok I should have noticed the switch to NULL pointer check. However,
> > the question still stands do we want everyone to run numbers to
> > justify this optimization, or make it a new common kernel coding
> > practice to do:
> >
> > if (!object->op)
> > generic_op(object);
> > else
> > object->op(object);
> >
> > ...in hot paths?
>
> I don't think nvdimm_flush is a hot path. Numbers of some
> representative workload would prove one of us right.
I'd rather say that the if "if (!op) do_generic()" pattern is more
readable in the general case, saves grepping for who set the op in the
common case. The fact that it has the potential to be faster is gravy
at that point.
^ permalink raw reply
* Re: [PATCH v5 1/6] libnvdimm: nd_region flush callback support
From: Jeff Moyer @ 2019-04-22 21:03 UTC (permalink / raw)
To: Dan Williams
Cc: Pankaj Gupta, cohuck, Jan Kara, KVM list, Michael S. Tsirkin,
david, Qemu Developers, virtualization, Andreas Dilger,
Ross Zwisler, Andrea Arcangeli, Dave Jiang, linux-nvdimm,
Vishal L Verma, Matthew Wilcox, Christoph Hellwig, Linux ACPI,
linux-ext4, Len Brown, kilobyte, Rik van Riel, yuval shaia,
Stefan Hajnoczi, Paolo Bonzini, lcapitulino
In-Reply-To: <CAPcyv4gGmN2LLMPy6BzWpYhRyV7fFNVgh=xAWSAdnnAWu5APtg@mail.gmail.com>
Dan Williams <dan.j.williams@intel.com> writes:
> On Mon, Apr 22, 2019 at 8:59 AM Jeff Moyer <jmoyer@redhat.com> wrote:
>>
>> Dan Williams <dan.j.williams@intel.com> writes:
>>
>> > On Thu, Apr 18, 2019 at 9:18 AM Christoph Hellwig <hch@infradead.org> wrote:
>> >>
>> >> On Thu, Apr 18, 2019 at 09:05:05AM -0700, Dan Williams wrote:
>> >> > > > I'd either add a comment about avoiding retpoline overhead here or just
>> >> > > > make ->flush == NULL mean generic_nvdimm_flush(). Just so that people don't
>> >> > > > get confused by the code.
>> >> > >
>> >> > > Isn't this premature optimization? I really don't like adding things
>> >> > > like this without some numbers to show it's worth it.
>> >> >
>> >> > I don't think it's premature given this optimization technique is
>> >> > already being deployed elsewhere, see:
>> >> >
>> >> > https://lwn.net/Articles/774347/
>> >>
>> >> For one this one was backed by numbers, and second after feedback
>> >> from Linux we switched to the NULL pointer check instead.
>> >
>> > Ok I should have noticed the switch to NULL pointer check. However,
>> > the question still stands do we want everyone to run numbers to
>> > justify this optimization, or make it a new common kernel coding
>> > practice to do:
>> >
>> > if (!object->op)
>> > generic_op(object);
>> > else
>> > object->op(object);
>> >
>> > ...in hot paths?
>>
>> I don't think nvdimm_flush is a hot path. Numbers of some
>> representative workload would prove one of us right.
>
> I'd rather say that the if "if (!op) do_generic()" pattern is more
> readable in the general case, saves grepping for who set the op in the
> common case. The fact that it has the potential to be faster is gravy
> at that point.
If the primary motivation is performance, then I'd expect performance
numbers to back it up. If that isn't the primary motivation, then
choose whichever way you feel is appropriate.
Cheers,
Jeff
^ permalink raw reply
* Re: [PATCH 3/3] virtio-gpu api: VIRTIO_GPU_F_RESSOURCE_V2
From: Gurchetan Singh @ 2019-04-23 0:12 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Tomeu Vizoso, Michael S. Tsirkin, David Airlie, open list,
ML dri-devel, open list:VIRTIO CORE, NET AND BLOCK DRIVERS,
David Airlie, virtio, Chia-I Wu
In-Reply-To: <20190417095750.lre3xrg4dlgskfjg@sirius.home.kraxel.org>
On Wed, Apr 17, 2019 at 2:57 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Fri, Apr 12, 2019 at 04:34:20PM -0700, Chia-I Wu wrote:
> > Hi,
> >
> > I am still new to virgl, and missed the last round of discussion about
> > resource_create_v2.
> >
> > From the discussion below, semantically resource_create_v2 creates a host
> > resource object _without_ any storage; memory_create creates a host memory
> > object which provides the storage. Is that correct?
>
> Right now all resource_create_* variants create a resource object with
> host storage. memory_create creates guest storage, and
> resource_attach_memory binds things together. Then you have to transfer
> the data.
>
> Hmm, maybe we need a flag indicating that host storage is not needed,
> for resources where we want establish some kind of shared mapping later
> on.
>
> > Do we expect these new commands to be supported by OpenGL, which does not
> > separate resources and memories?
>
> Well, for opengl you need a 1:1 relationship between memory region and
> resource.
>
> > > Yes, even though it is not clear yet how we are going to handle
> > > host-allocated buffers in the vhost-user case ...
> >
> > This might be another dumb question, but is this only an issue for
> > vhost-user(-gpu) case? What mechanisms are used to map host dma-buf into
> > the guest address space?
>
> qemu can change the address space, that includes mmap()ing stuff there.
> An external vhost-user process can't do this, it can only read the
> address space layout, and read/write from/to guest memory.
>
> > But one needs to create the resource first to know which memory types can
> > be attached to it. I think the metadata needs to be returned with
> > resource_create_v2.
>
> There is a resource_info reply for that.
The memory type should probably be in resource_create_v2, not in the
reply. The metadata will be different based on the memory heap it's
allocated from.
Also, not all heaps need to be exposed to the guest kernel. For
example, device local memory in Vulkan could be un-mappable. In fact,
for resources that are not host visible we might be better off
sidestepping the kernel altogether and tracking allocation in guest
userspace.
Here is an example of memory types the guest kernel may be interested
in (based on i965):
Type 0 --> DEVICE_LOCAL_BIT | HOST_VISIBLE_BIT | HOST_COHERENT_BIT |
HOST_CACHED_BIT | RENDERER_ALLOCATED (Vulkan)
Type 1 --> HOST_VISIBLE_BIT | HOST_COHERENT_BIT | EXTERNAL_ALLOCATED
(gbm write combine)
Type 2 --> HOST_VISIBLE_BIT | HOST_COHERENT_BIT | GUEST_ALLOCATED
(guest allocated memory, which I assume is also write combine)
Type 3 --> HOST_VISIBLE_BIT | HOST_CACHED | EXTERNAL_ALLOCATED (gbm
cached memory)
>
> > That should be good enough. But by returning alignments, we can minimize
> > the gaps when attaching multiple resources, especially when the resources
> > are only used by GPU.
>
> We can add alignments to the resource_info reply.
>
> cheers,
> Gerd
>
^ permalink raw reply
* Re: [PATCH v5 1/6] libnvdimm: nd_region flush callback support
From: Pankaj Gupta @ 2019-04-23 4:07 UTC (permalink / raw)
To: Jeff Moyer
Cc: cohuck, Jan Kara, KVM list, Michael S. Tsirkin, david,
Qemu Developers, virtualization, Andreas Dilger, Ross Zwisler,
Andrea Arcangeli, Dave Jiang, linux-nvdimm, Vishal L Verma,
Matthew Wilcox, Christoph Hellwig, Linux ACPI, linux-ext4,
Len Brown, kilobyte, Rik van Riel, yuval shaia, Stefan Hajnoczi,
Paolo Bonzini, Dan Williams, lcapitulino
In-Reply-To: <x49ef5t7nwp.fsf@segfault.boston.devel.redhat.com>
>
> Dan Williams <dan.j.williams@intel.com> writes:
>
> > On Mon, Apr 22, 2019 at 8:59 AM Jeff Moyer <jmoyer@redhat.com> wrote:
> >>
> >> Dan Williams <dan.j.williams@intel.com> writes:
> >>
> >> > On Thu, Apr 18, 2019 at 9:18 AM Christoph Hellwig <hch@infradead.org>
> >> > wrote:
> >> >>
> >> >> On Thu, Apr 18, 2019 at 09:05:05AM -0700, Dan Williams wrote:
> >> >> > > > I'd either add a comment about avoiding retpoline overhead here
> >> >> > > > or just
> >> >> > > > make ->flush == NULL mean generic_nvdimm_flush(). Just so that
> >> >> > > > people don't
> >> >> > > > get confused by the code.
> >> >> > >
> >> >> > > Isn't this premature optimization? I really don't like adding
> >> >> > > things
> >> >> > > like this without some numbers to show it's worth it.
> >> >> >
> >> >> > I don't think it's premature given this optimization technique is
> >> >> > already being deployed elsewhere, see:
> >> >> >
> >> >> > https://lwn.net/Articles/774347/
> >> >>
> >> >> For one this one was backed by numbers, and second after feedback
> >> >> from Linux we switched to the NULL pointer check instead.
> >> >
> >> > Ok I should have noticed the switch to NULL pointer check. However,
> >> > the question still stands do we want everyone to run numbers to
> >> > justify this optimization, or make it a new common kernel coding
> >> > practice to do:
> >> >
> >> > if (!object->op)
> >> > generic_op(object);
> >> > else
> >> > object->op(object);
> >> >
> >> > ...in hot paths?
> >>
> >> I don't think nvdimm_flush is a hot path. Numbers of some
> >> representative workload would prove one of us right.
> >
> > I'd rather say that the if "if (!op) do_generic()" pattern is more
> > readable in the general case, saves grepping for who set the op in the
> > common case. The fact that it has the potential to be faster is gravy
> > at that point.
>
> If the primary motivation is performance, then I'd expect performance
> numbers to back it up. If that isn't the primary motivation, then
> choose whichever way you feel is appropriate.
Agree. This change enhances the code readability. Will add this change in
v6 with other changes.
Thank you!
Pankaj
>
> Cheers,
> Jeff
>
^ permalink raw reply
* [RFC PATCH V3 0/6] vhost: accelerate metadata access
From: Jason Wang @ 2019-04-23 5:54 UTC (permalink / raw)
To: mst, jasowang, kvm, virtualization, netdev, linux-kernel
Cc: aarcange, hch, christophe.de.dinechin, linux-parisc,
James.Bottomley, linux-mm, jglisse, jrdr.linux, davem,
linux-arm-kernel
This series tries to access virtqueue metadata through kernel virtual
address instead of copy_user() friends since they had too much
overheads like checks, spec barriers or even hardware feature
toggling. This is done through setup kernel address through direct
mapping and co-opreate VM management with MMU notifiers.
Test shows about 23% improvement on TX PPS. TCP_STREAM doesn't see
obvious improvement.
Thanks
Changes from RFC V2:
- switch to use direct mapping instead of vmap()
- switch to use spinlock + RCU to synchronize MMU notifier and vhost
data/control path
- set dirty pages in the invalidation callbacks
- always use copy_to/from_users() friends for the archs that may need
flush_dcache_pages()
- various minor fixes
Changes from V4:
- use invalidate_range() instead of invalidate_range_start()
- track dirty pages
Changes from V3:
- don't try to use vmap for file backed pages
- rebase to master
Changes from V2:
- fix buggy range overlapping check
- tear down MMU notifier during vhost ioctl to make sure invalidation
request can read metadata userspace address and vq size without
holding vq mutex.
Changes from V1:
- instead of pinning pages, use MMU notifier to invalidate vmaps and
remap duing metadata prefetch
- fix build warning on MIPS
Jason Wang (6):
vhost: generalize adding used elem
vhost: fine grain userspace memory accessors
vhost: rename vq_iotlb_prefetch() to vq_meta_prefetch()
vhost: introduce helpers to get the size of metadata area
vhost: factor out setting vring addr and num
vhost: access vq metadata through kernel virtual address
drivers/vhost/net.c | 4 +-
drivers/vhost/vhost.c | 852 ++++++++++++++++++++++++++++++++++++------
drivers/vhost/vhost.h | 34 +-
3 files changed, 764 insertions(+), 126 deletions(-)
--
2.18.1
^ permalink raw reply
* [RFC PATCH V3 1/6] vhost: generalize adding used elem
From: Jason Wang @ 2019-04-23 5:54 UTC (permalink / raw)
To: mst, jasowang, kvm, virtualization, netdev, linux-kernel
Cc: aarcange, hch, christophe.de.dinechin, linux-parisc,
James.Bottomley, linux-mm, jglisse, jrdr.linux, davem,
linux-arm-kernel
In-Reply-To: <20190423055420.26408-1-jasowang@redhat.com>
Use one generic vhost_copy_to_user() instead of two dedicated
accessor. This will simplify the conversion to fine grain
accessors. About 2% improvement of PPS were seen during vitio-user
txonly test.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/vhost.c | 11 +----------
1 file changed, 1 insertion(+), 10 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 351af88231ad..6df76ac8200a 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -2255,16 +2255,7 @@ static int __vhost_add_used_n(struct vhost_virtqueue *vq,
start = vq->last_used_idx & (vq->num - 1);
used = vq->used->ring + start;
- if (count == 1) {
- if (vhost_put_user(vq, heads[0].id, &used->id)) {
- vq_err(vq, "Failed to write used id");
- return -EFAULT;
- }
- if (vhost_put_user(vq, heads[0].len, &used->len)) {
- vq_err(vq, "Failed to write used len");
- return -EFAULT;
- }
- } else if (vhost_copy_to_user(vq, used, heads, count * sizeof *used)) {
+ if (vhost_copy_to_user(vq, used, heads, count * sizeof *used)) {
vq_err(vq, "Failed to write used");
return -EFAULT;
}
--
2.18.1
^ permalink raw reply related
* [RFC PATCH V3 2/6] vhost: fine grain userspace memory accessors
From: Jason Wang @ 2019-04-23 5:54 UTC (permalink / raw)
To: mst, jasowang, kvm, virtualization, netdev, linux-kernel
Cc: aarcange, hch, christophe.de.dinechin, linux-parisc,
James.Bottomley, linux-mm, jglisse, jrdr.linux, davem,
linux-arm-kernel
In-Reply-To: <20190423055420.26408-1-jasowang@redhat.com>
This is used to hide the metadata address from virtqueue helpers. This
will allow to implement a vmap based fast accessing to metadata.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/vhost.c | 94 +++++++++++++++++++++++++++++++++++--------
1 file changed, 77 insertions(+), 17 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 6df76ac8200a..7335f2275ed3 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -869,6 +869,34 @@ static inline void __user *__vhost_get_user(struct vhost_virtqueue *vq,
ret; \
})
+static inline int vhost_put_avail_event(struct vhost_virtqueue *vq)
+{
+ return vhost_put_user(vq, cpu_to_vhost16(vq, vq->avail_idx),
+ vhost_avail_event(vq));
+}
+
+static inline int vhost_put_used(struct vhost_virtqueue *vq,
+ struct vring_used_elem *head, int idx,
+ int count)
+{
+ return vhost_copy_to_user(vq, vq->used->ring + idx, head,
+ count * sizeof(*head));
+}
+
+static inline int vhost_put_used_flags(struct vhost_virtqueue *vq)
+
+{
+ return vhost_put_user(vq, cpu_to_vhost16(vq, vq->used_flags),
+ &vq->used->flags);
+}
+
+static inline int vhost_put_used_idx(struct vhost_virtqueue *vq)
+
+{
+ return vhost_put_user(vq, cpu_to_vhost16(vq, vq->last_used_idx),
+ &vq->used->idx);
+}
+
#define vhost_get_user(vq, x, ptr, type) \
({ \
int ret; \
@@ -907,6 +935,43 @@ static void vhost_dev_unlock_vqs(struct vhost_dev *d)
mutex_unlock(&d->vqs[i]->mutex);
}
+static inline int vhost_get_avail_idx(struct vhost_virtqueue *vq,
+ __virtio16 *idx)
+{
+ return vhost_get_avail(vq, *idx, &vq->avail->idx);
+}
+
+static inline int vhost_get_avail_head(struct vhost_virtqueue *vq,
+ __virtio16 *head, int idx)
+{
+ return vhost_get_avail(vq, *head,
+ &vq->avail->ring[idx & (vq->num - 1)]);
+}
+
+static inline int vhost_get_avail_flags(struct vhost_virtqueue *vq,
+ __virtio16 *flags)
+{
+ return vhost_get_avail(vq, *flags, &vq->avail->flags);
+}
+
+static inline int vhost_get_used_event(struct vhost_virtqueue *vq,
+ __virtio16 *event)
+{
+ return vhost_get_avail(vq, *event, vhost_used_event(vq));
+}
+
+static inline int vhost_get_used_idx(struct vhost_virtqueue *vq,
+ __virtio16 *idx)
+{
+ return vhost_get_used(vq, *idx, &vq->used->idx);
+}
+
+static inline int vhost_get_desc(struct vhost_virtqueue *vq,
+ struct vring_desc *desc, int idx)
+{
+ return vhost_copy_from_user(vq, desc, vq->desc + idx, sizeof(*desc));
+}
+
static int vhost_new_umem_range(struct vhost_umem *umem,
u64 start, u64 size, u64 end,
u64 userspace_addr, int perm)
@@ -1844,8 +1909,7 @@ EXPORT_SYMBOL_GPL(vhost_log_write);
static int vhost_update_used_flags(struct vhost_virtqueue *vq)
{
void __user *used;
- if (vhost_put_user(vq, cpu_to_vhost16(vq, vq->used_flags),
- &vq->used->flags) < 0)
+ if (vhost_put_used_flags(vq))
return -EFAULT;
if (unlikely(vq->log_used)) {
/* Make sure the flag is seen before log. */
@@ -1862,8 +1926,7 @@ static int vhost_update_used_flags(struct vhost_virtqueue *vq)
static int vhost_update_avail_event(struct vhost_virtqueue *vq, u16 avail_event)
{
- if (vhost_put_user(vq, cpu_to_vhost16(vq, vq->avail_idx),
- vhost_avail_event(vq)))
+ if (vhost_put_avail_event(vq))
return -EFAULT;
if (unlikely(vq->log_used)) {
void __user *used;
@@ -1899,7 +1962,7 @@ int vhost_vq_init_access(struct vhost_virtqueue *vq)
r = -EFAULT;
goto err;
}
- r = vhost_get_used(vq, last_used_idx, &vq->used->idx);
+ r = vhost_get_used_idx(vq, &last_used_idx);
if (r) {
vq_err(vq, "Can't access used idx at %p\n",
&vq->used->idx);
@@ -2098,7 +2161,7 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
last_avail_idx = vq->last_avail_idx;
if (vq->avail_idx == vq->last_avail_idx) {
- if (unlikely(vhost_get_avail(vq, avail_idx, &vq->avail->idx))) {
+ if (unlikely(vhost_get_avail_idx(vq, &avail_idx))) {
vq_err(vq, "Failed to access avail idx at %p\n",
&vq->avail->idx);
return -EFAULT;
@@ -2125,8 +2188,7 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
/* Grab the next descriptor number they're advertising, and increment
* the index we've seen. */
- if (unlikely(vhost_get_avail(vq, ring_head,
- &vq->avail->ring[last_avail_idx & (vq->num - 1)]))) {
+ if (unlikely(vhost_get_avail_head(vq, &ring_head, last_avail_idx))) {
vq_err(vq, "Failed to read head: idx %d address %p\n",
last_avail_idx,
&vq->avail->ring[last_avail_idx % vq->num]);
@@ -2161,8 +2223,7 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
i, vq->num, head);
return -EINVAL;
}
- ret = vhost_copy_from_user(vq, &desc, vq->desc + i,
- sizeof desc);
+ ret = vhost_get_desc(vq, &desc, i);
if (unlikely(ret)) {
vq_err(vq, "Failed to get descriptor: idx %d addr %p\n",
i, vq->desc + i);
@@ -2255,7 +2316,7 @@ static int __vhost_add_used_n(struct vhost_virtqueue *vq,
start = vq->last_used_idx & (vq->num - 1);
used = vq->used->ring + start;
- if (vhost_copy_to_user(vq, used, heads, count * sizeof *used)) {
+ if (vhost_put_used(vq, heads, start, count)) {
vq_err(vq, "Failed to write used");
return -EFAULT;
}
@@ -2297,8 +2358,7 @@ int vhost_add_used_n(struct vhost_virtqueue *vq, struct vring_used_elem *heads,
/* Make sure buffer is written before we update index. */
smp_wmb();
- if (vhost_put_user(vq, cpu_to_vhost16(vq, vq->last_used_idx),
- &vq->used->idx)) {
+ if (vhost_put_used_idx(vq)) {
vq_err(vq, "Failed to increment used idx");
return -EFAULT;
}
@@ -2331,7 +2391,7 @@ static bool vhost_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
if (!vhost_has_feature(vq, VIRTIO_RING_F_EVENT_IDX)) {
__virtio16 flags;
- if (vhost_get_avail(vq, flags, &vq->avail->flags)) {
+ if (vhost_get_avail_flags(vq, &flags)) {
vq_err(vq, "Failed to get flags");
return true;
}
@@ -2345,7 +2405,7 @@ static bool vhost_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
if (unlikely(!v))
return true;
- if (vhost_get_avail(vq, event, vhost_used_event(vq))) {
+ if (vhost_get_used_event(vq, &event)) {
vq_err(vq, "Failed to get used event idx");
return true;
}
@@ -2390,7 +2450,7 @@ bool vhost_vq_avail_empty(struct vhost_dev *dev, struct vhost_virtqueue *vq)
if (vq->avail_idx != vq->last_avail_idx)
return false;
- r = vhost_get_avail(vq, avail_idx, &vq->avail->idx);
+ r = vhost_get_avail_idx(vq, &avail_idx);
if (unlikely(r))
return false;
vq->avail_idx = vhost16_to_cpu(vq, avail_idx);
@@ -2426,7 +2486,7 @@ bool vhost_enable_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
/* They could have slipped one in as we were doing that: make
* sure it's written, then check again. */
smp_mb();
- r = vhost_get_avail(vq, avail_idx, &vq->avail->idx);
+ r = vhost_get_avail_idx(vq, &avail_idx);
if (r) {
vq_err(vq, "Failed to check avail idx at %p: %d\n",
&vq->avail->idx, r);
--
2.18.1
^ permalink raw reply related
* [RFC PATCH V3 3/6] vhost: rename vq_iotlb_prefetch() to vq_meta_prefetch()
From: Jason Wang @ 2019-04-23 5:54 UTC (permalink / raw)
To: mst, jasowang, kvm, virtualization, netdev, linux-kernel
Cc: aarcange, hch, christophe.de.dinechin, linux-parisc,
James.Bottomley, linux-mm, jglisse, jrdr.linux, davem,
linux-arm-kernel
In-Reply-To: <20190423055420.26408-1-jasowang@redhat.com>
Rename the function to be more accurate since it actually tries to
prefetch vq metadata address in IOTLB. And this will be used by
following patch to prefetch metadata virtual addresses.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/net.c | 4 ++--
drivers/vhost/vhost.c | 4 ++--
drivers/vhost/vhost.h | 2 +-
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index df51a35cf537..bf55f995ebae 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -971,7 +971,7 @@ static void handle_tx(struct vhost_net *net)
if (!sock)
goto out;
- if (!vq_iotlb_prefetch(vq))
+ if (!vq_meta_prefetch(vq))
goto out;
vhost_disable_notify(&net->dev, vq);
@@ -1140,7 +1140,7 @@ static void handle_rx(struct vhost_net *net)
if (!sock)
goto out;
- if (!vq_iotlb_prefetch(vq))
+ if (!vq_meta_prefetch(vq))
goto out;
vhost_disable_notify(&net->dev, vq);
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 7335f2275ed3..bff4d586871d 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1313,7 +1313,7 @@ static bool iotlb_access_ok(struct vhost_virtqueue *vq,
return true;
}
-int vq_iotlb_prefetch(struct vhost_virtqueue *vq)
+int vq_meta_prefetch(struct vhost_virtqueue *vq)
{
size_t s = vhost_has_feature(vq, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
unsigned int num = vq->num;
@@ -1332,7 +1332,7 @@ int vq_iotlb_prefetch(struct vhost_virtqueue *vq)
num * sizeof(*vq->used->ring) + s,
VHOST_ADDR_USED);
}
-EXPORT_SYMBOL_GPL(vq_iotlb_prefetch);
+EXPORT_SYMBOL_GPL(vq_meta_prefetch);
/* Can we log writes? */
/* Caller should have device mutex but not vq mutex */
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 9490e7ddb340..7a7fc001265f 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -209,7 +209,7 @@ bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *);
int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
unsigned int log_num, u64 len,
struct iovec *iov, int count);
-int vq_iotlb_prefetch(struct vhost_virtqueue *vq);
+int vq_meta_prefetch(struct vhost_virtqueue *vq);
struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type);
void vhost_enqueue_msg(struct vhost_dev *dev,
--
2.18.1
^ permalink raw reply related
* [RFC PATCH V3 4/6] vhost: introduce helpers to get the size of metadata area
From: Jason Wang @ 2019-04-23 5:54 UTC (permalink / raw)
To: mst, jasowang, kvm, virtualization, netdev, linux-kernel
Cc: aarcange, hch, christophe.de.dinechin, linux-parisc,
James.Bottomley, linux-mm, jglisse, jrdr.linux, davem,
linux-arm-kernel
In-Reply-To: <20190423055420.26408-1-jasowang@redhat.com>
To avoid code duplication since it will be used by kernel VA prefetching.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/vhost.c | 51 ++++++++++++++++++++++++++++---------------
1 file changed, 33 insertions(+), 18 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index bff4d586871d..f3f86c3ed659 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -413,6 +413,32 @@ static void vhost_dev_free_iovecs(struct vhost_dev *dev)
vhost_vq_free_iovecs(dev->vqs[i]);
}
+static size_t vhost_get_avail_size(struct vhost_virtqueue *vq,
+ unsigned int num)
+{
+ size_t event __maybe_unused =
+ vhost_has_feature(vq, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
+
+ return sizeof(*vq->avail) +
+ sizeof(*vq->avail->ring) * num + event;
+}
+
+static size_t vhost_get_used_size(struct vhost_virtqueue *vq,
+ unsigned int num)
+{
+ size_t event __maybe_unused =
+ vhost_has_feature(vq, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
+
+ return sizeof(*vq->used) +
+ sizeof(*vq->used->ring) * num + event;
+}
+
+static size_t vhost_get_desc_size(struct vhost_virtqueue *vq,
+ unsigned int num)
+{
+ return sizeof(*vq->desc) * num;
+}
+
void vhost_dev_init(struct vhost_dev *dev,
struct vhost_virtqueue **vqs, int nvqs, int iov_limit)
{
@@ -1257,13 +1283,9 @@ static bool vq_access_ok(struct vhost_virtqueue *vq, unsigned int num,
struct vring_used __user *used)
{
- size_t s __maybe_unused = vhost_has_feature(vq, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
-
- return access_ok(desc, num * sizeof *desc) &&
- access_ok(avail,
- sizeof *avail + num * sizeof *avail->ring + s) &&
- access_ok(used,
- sizeof *used + num * sizeof *used->ring + s);
+ return access_ok(desc, vhost_get_desc_size(vq, num)) &&
+ access_ok(avail, vhost_get_avail_size(vq, num)) &&
+ access_ok(used, vhost_get_used_size(vq, num));
}
static void vhost_vq_meta_update(struct vhost_virtqueue *vq,
@@ -1315,22 +1337,18 @@ static bool iotlb_access_ok(struct vhost_virtqueue *vq,
int vq_meta_prefetch(struct vhost_virtqueue *vq)
{
- size_t s = vhost_has_feature(vq, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
unsigned int num = vq->num;
if (!vq->iotlb)
return 1;
return iotlb_access_ok(vq, VHOST_ACCESS_RO, (u64)(uintptr_t)vq->desc,
- num * sizeof(*vq->desc), VHOST_ADDR_DESC) &&
+ vhost_get_desc_size(vq, num), VHOST_ADDR_DESC) &&
iotlb_access_ok(vq, VHOST_ACCESS_RO, (u64)(uintptr_t)vq->avail,
- sizeof *vq->avail +
- num * sizeof(*vq->avail->ring) + s,
+ vhost_get_avail_size(vq, num),
VHOST_ADDR_AVAIL) &&
iotlb_access_ok(vq, VHOST_ACCESS_WO, (u64)(uintptr_t)vq->used,
- sizeof *vq->used +
- num * sizeof(*vq->used->ring) + s,
- VHOST_ADDR_USED);
+ vhost_get_used_size(vq, num), VHOST_ADDR_USED);
}
EXPORT_SYMBOL_GPL(vq_meta_prefetch);
@@ -1347,13 +1365,10 @@ EXPORT_SYMBOL_GPL(vhost_log_access_ok);
static bool vq_log_access_ok(struct vhost_virtqueue *vq,
void __user *log_base)
{
- size_t s = vhost_has_feature(vq, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
-
return vq_memory_access_ok(log_base, vq->umem,
vhost_has_feature(vq, VHOST_F_LOG_ALL)) &&
(!vq->log_used || log_access_ok(log_base, vq->log_addr,
- sizeof *vq->used +
- vq->num * sizeof *vq->used->ring + s));
+ vhost_get_used_size(vq, vq->num)));
}
/* Can we start vq? */
--
2.18.1
^ permalink raw reply related
* [RFC PATCH V3 5/6] vhost: factor out setting vring addr and num
From: Jason Wang @ 2019-04-23 5:54 UTC (permalink / raw)
To: mst, jasowang, kvm, virtualization, netdev, linux-kernel
Cc: aarcange, hch, christophe.de.dinechin, linux-parisc,
James.Bottomley, linux-mm, jglisse, jrdr.linux, davem,
linux-arm-kernel
In-Reply-To: <20190423055420.26408-1-jasowang@redhat.com>
Factoring vring address and num setting which needs special care for
accelerating vq metadata accessing.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/vhost.c | 177 ++++++++++++++++++++++++------------------
1 file changed, 103 insertions(+), 74 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index f3f86c3ed659..c2362ed5839e 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1468,6 +1468,104 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m)
return -EFAULT;
}
+static long vhost_vring_set_num(struct vhost_dev *d,
+ struct vhost_virtqueue *vq,
+ void __user *argp)
+{
+ struct vhost_vring_state s;
+
+ /* Resizing ring with an active backend?
+ * You don't want to do that. */
+ if (vq->private_data)
+ return -EBUSY;
+
+ if (copy_from_user(&s, argp, sizeof s))
+ return -EFAULT;
+
+ if (!s.num || s.num > 0xffff || (s.num & (s.num - 1)))
+ return -EINVAL;
+ vq->num = s.num;
+
+ return 0;
+}
+
+static long vhost_vring_set_addr(struct vhost_dev *d,
+ struct vhost_virtqueue *vq,
+ void __user *argp)
+{
+ struct vhost_vring_addr a;
+
+ if (copy_from_user(&a, argp, sizeof a))
+ return -EFAULT;
+ if (a.flags & ~(0x1 << VHOST_VRING_F_LOG))
+ return -EOPNOTSUPP;
+
+ /* For 32bit, verify that the top 32bits of the user
+ data are set to zero. */
+ if ((u64)(unsigned long)a.desc_user_addr != a.desc_user_addr ||
+ (u64)(unsigned long)a.used_user_addr != a.used_user_addr ||
+ (u64)(unsigned long)a.avail_user_addr != a.avail_user_addr)
+ return -EFAULT;
+
+ /* Make sure it's safe to cast pointers to vring types. */
+ BUILD_BUG_ON(__alignof__ *vq->avail > VRING_AVAIL_ALIGN_SIZE);
+ BUILD_BUG_ON(__alignof__ *vq->used > VRING_USED_ALIGN_SIZE);
+ if ((a.avail_user_addr & (VRING_AVAIL_ALIGN_SIZE - 1)) ||
+ (a.used_user_addr & (VRING_USED_ALIGN_SIZE - 1)) ||
+ (a.log_guest_addr & (VRING_USED_ALIGN_SIZE - 1)))
+ return -EINVAL;
+
+ /* We only verify access here if backend is configured.
+ * If it is not, we don't as size might not have been setup.
+ * We will verify when backend is configured. */
+ if (vq->private_data) {
+ if (!vq_access_ok(vq, vq->num,
+ (void __user *)(unsigned long)a.desc_user_addr,
+ (void __user *)(unsigned long)a.avail_user_addr,
+ (void __user *)(unsigned long)a.used_user_addr))
+ return -EINVAL;
+
+ /* Also validate log access for used ring if enabled. */
+ if ((a.flags & (0x1 << VHOST_VRING_F_LOG)) &&
+ !log_access_ok(vq->log_base, a.log_guest_addr,
+ sizeof *vq->used +
+ vq->num * sizeof *vq->used->ring))
+ return -EINVAL;
+ }
+
+ vq->log_used = !!(a.flags & (0x1 << VHOST_VRING_F_LOG));
+ vq->desc = (void __user *)(unsigned long)a.desc_user_addr;
+ vq->avail = (void __user *)(unsigned long)a.avail_user_addr;
+ vq->log_addr = a.log_guest_addr;
+ vq->used = (void __user *)(unsigned long)a.used_user_addr;
+
+ return 0;
+}
+
+static long vhost_vring_set_num_addr(struct vhost_dev *d,
+ struct vhost_virtqueue *vq,
+ unsigned int ioctl,
+ void __user *argp)
+{
+ long r;
+
+ mutex_lock(&vq->mutex);
+
+ switch (ioctl) {
+ case VHOST_SET_VRING_NUM:
+ r = vhost_vring_set_num(d, vq, argp);
+ break;
+ case VHOST_SET_VRING_ADDR:
+ r = vhost_vring_set_addr(d, vq, argp);
+ break;
+ default:
+ BUG();
+ }
+
+ mutex_unlock(&vq->mutex);
+
+ return r;
+}
long vhost_vring_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
{
struct file *eventfp, *filep = NULL;
@@ -1477,7 +1575,6 @@ long vhost_vring_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *arg
struct vhost_virtqueue *vq;
struct vhost_vring_state s;
struct vhost_vring_file f;
- struct vhost_vring_addr a;
u32 idx;
long r;
@@ -1490,26 +1587,14 @@ long vhost_vring_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *arg
idx = array_index_nospec(idx, d->nvqs);
vq = d->vqs[idx];
+ if (ioctl == VHOST_SET_VRING_NUM ||
+ ioctl == VHOST_SET_VRING_ADDR) {
+ return vhost_vring_set_num_addr(d, vq, ioctl, argp);
+ }
+
mutex_lock(&vq->mutex);
switch (ioctl) {
- case VHOST_SET_VRING_NUM:
- /* Resizing ring with an active backend?
- * You don't want to do that. */
- if (vq->private_data) {
- r = -EBUSY;
- break;
- }
- if (copy_from_user(&s, argp, sizeof s)) {
- r = -EFAULT;
- break;
- }
- if (!s.num || s.num > 0xffff || (s.num & (s.num - 1))) {
- r = -EINVAL;
- break;
- }
- vq->num = s.num;
- break;
case VHOST_SET_VRING_BASE:
/* Moving base with an active backend?
* You don't want to do that. */
@@ -1535,62 +1620,6 @@ long vhost_vring_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *arg
if (copy_to_user(argp, &s, sizeof s))
r = -EFAULT;
break;
- case VHOST_SET_VRING_ADDR:
- if (copy_from_user(&a, argp, sizeof a)) {
- r = -EFAULT;
- break;
- }
- if (a.flags & ~(0x1 << VHOST_VRING_F_LOG)) {
- r = -EOPNOTSUPP;
- break;
- }
- /* For 32bit, verify that the top 32bits of the user
- data are set to zero. */
- if ((u64)(unsigned long)a.desc_user_addr != a.desc_user_addr ||
- (u64)(unsigned long)a.used_user_addr != a.used_user_addr ||
- (u64)(unsigned long)a.avail_user_addr != a.avail_user_addr) {
- r = -EFAULT;
- break;
- }
-
- /* Make sure it's safe to cast pointers to vring types. */
- BUILD_BUG_ON(__alignof__ *vq->avail > VRING_AVAIL_ALIGN_SIZE);
- BUILD_BUG_ON(__alignof__ *vq->used > VRING_USED_ALIGN_SIZE);
- if ((a.avail_user_addr & (VRING_AVAIL_ALIGN_SIZE - 1)) ||
- (a.used_user_addr & (VRING_USED_ALIGN_SIZE - 1)) ||
- (a.log_guest_addr & (VRING_USED_ALIGN_SIZE - 1))) {
- r = -EINVAL;
- break;
- }
-
- /* We only verify access here if backend is configured.
- * If it is not, we don't as size might not have been setup.
- * We will verify when backend is configured. */
- if (vq->private_data) {
- if (!vq_access_ok(vq, vq->num,
- (void __user *)(unsigned long)a.desc_user_addr,
- (void __user *)(unsigned long)a.avail_user_addr,
- (void __user *)(unsigned long)a.used_user_addr)) {
- r = -EINVAL;
- break;
- }
-
- /* Also validate log access for used ring if enabled. */
- if ((a.flags & (0x1 << VHOST_VRING_F_LOG)) &&
- !log_access_ok(vq->log_base, a.log_guest_addr,
- sizeof *vq->used +
- vq->num * sizeof *vq->used->ring)) {
- r = -EINVAL;
- break;
- }
- }
-
- vq->log_used = !!(a.flags & (0x1 << VHOST_VRING_F_LOG));
- vq->desc = (void __user *)(unsigned long)a.desc_user_addr;
- vq->avail = (void __user *)(unsigned long)a.avail_user_addr;
- vq->log_addr = a.log_guest_addr;
- vq->used = (void __user *)(unsigned long)a.used_user_addr;
- break;
case VHOST_SET_VRING_KICK:
if (copy_from_user(&f, argp, sizeof f)) {
r = -EFAULT;
--
2.18.1
^ permalink raw reply related
* [RFC PATCH V3 6/6] vhost: access vq metadata through kernel virtual address
From: Jason Wang @ 2019-04-23 5:54 UTC (permalink / raw)
To: mst, jasowang, kvm, virtualization, netdev, linux-kernel
Cc: aarcange, hch, christophe.de.dinechin, linux-parisc,
James.Bottomley, linux-mm, jglisse, jrdr.linux, davem,
linux-arm-kernel
In-Reply-To: <20190423055420.26408-1-jasowang@redhat.com>
It was noticed that the copy_to/from_user() friends that was used to
access virtqueue metdata tends to be very expensive for dataplane
implementation like vhost since it involves lots of software checks,
speculation barriers, hardware feature toggling (e.g SMAP). The
extra cost will be more obvious when transferring small packets since
the time spent on metadata accessing become more significant.
This patch tries to eliminate those overheads by accessing them
through direct mapping of those pages. Invalidation callbacks is
implemented for co-operation with general VM management (swap, KSM,
THP or NUMA balancing). We will try to get the direct mapping of vq
metadata before each round of packet processing if it doesn't
exist. If we fail, we will simplely fallback to copy_to/from_user()
friends.
This invalidation and direct mapping access are synchronized through
spinlock and RCU. All matedata accessing through direct map is
protected by RCU, and the setup or invalidation are done under
spinlock.
This method might does not work for high mem page which requires
temporary mapping so we just fallback to normal
copy_to/from_user() and may not for arch that has virtual tagged cache
since extra cache flushing is needed to eliminate the alias. This will
result complex logic and bad performance. For those archs, this patch
simply go for copy_to/from_user() friends. This is done by ruling out
kernel mapping codes through ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE.
Note that this is only done when device IOTLB is not enabled. We
could use similar method to optimize IOTLB in the future.
Tests shows at most about 23% improvement on TX PPS when using
virtio-user + vhost_net + xdp1 + TAP on 2.6GHz Broadwell:
SMAP on | SMAP off
Before: 5.2Mpps | 7.1Mpps
After: 6.4Mpps | 8.2Mpps
Cc: Andrea Arcangeli <aarcange@redhat.com>
Cc: James Bottomley <James.Bottomley@hansenpartnership.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: David Miller <davem@davemloft.net>
Cc: Jerome Glisse <jglisse@redhat.com>
Cc: linux-mm@kvack.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-parisc@vger.kernel.org
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/vhost.c | 517 +++++++++++++++++++++++++++++++++++++++++-
drivers/vhost/vhost.h | 32 +++
2 files changed, 546 insertions(+), 3 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index c2362ed5839e..a9fe04f0016a 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -299,6 +299,49 @@ static void vhost_vq_meta_reset(struct vhost_dev *d)
__vhost_vq_meta_reset(d->vqs[i]);
}
+#if ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE == 0
+static void vhost_map_unprefetch(struct vhost_map *map)
+{
+ kfree(map->pages);
+ map->pages = NULL;
+ map->npages = 0;
+ map->addr = NULL;
+}
+
+static void vhost_uninit_vq_maps(struct vhost_virtqueue *vq)
+{
+ struct vhost_map *map[VHOST_NUM_ADDRS];
+ int i;
+
+ spin_lock(&vq->mmu_lock);
+ for (i = 0; i < VHOST_NUM_ADDRS; i++) {
+ map[i] = rcu_dereference_protected(vq->maps[i],
+ lockdep_is_held(&vq->mmu_lock));
+ if (map[i])
+ rcu_assign_pointer(vq->maps[i], NULL);
+ }
+ spin_unlock(&vq->mmu_lock);
+
+ synchronize_rcu();
+
+ for (i = 0; i < VHOST_NUM_ADDRS; i++)
+ if (map[i])
+ vhost_map_unprefetch(map[i]);
+
+}
+#endif
+
+static void vhost_reset_vq_maps(struct vhost_virtqueue *vq)
+{
+#if ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE == 0
+ int i;
+
+ vhost_uninit_vq_maps(vq);
+ for (i = 0; i < VHOST_NUM_ADDRS; i++)
+ vq->uaddrs[i].size = 0;
+#endif
+}
+
static void vhost_vq_reset(struct vhost_dev *dev,
struct vhost_virtqueue *vq)
{
@@ -327,7 +370,9 @@ static void vhost_vq_reset(struct vhost_dev *dev,
vq->busyloop_timeout = 0;
vq->umem = NULL;
vq->iotlb = NULL;
+ vq->invalidate_count = 0;
__vhost_vq_meta_reset(vq);
+ vhost_reset_vq_maps(vq);
}
static int vhost_worker(void *data)
@@ -439,6 +484,123 @@ static size_t vhost_get_desc_size(struct vhost_virtqueue *vq,
return sizeof(*vq->desc) * num;
}
+#if ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE == 0
+static bool vhost_map_range_overlap(struct vhost_uaddr *uaddr,
+ unsigned long start,
+ unsigned long end)
+{
+ if (unlikely(!uaddr->size))
+ return false;
+
+ return !(end < uaddr->uaddr || start > uaddr->uaddr - 1 + uaddr->size);
+}
+
+static void vhost_invalidate_vq_start(struct vhost_virtqueue *vq,
+ int index,
+ unsigned long start,
+ unsigned long end)
+{
+ struct vhost_uaddr *uaddr = &vq->uaddrs[index];
+ struct vhost_map *map;
+ int i;
+
+ if (!vhost_map_range_overlap(uaddr, start, end))
+ return;
+
+ spin_lock(&vq->mmu_lock);
+ ++vq->invalidate_count;
+
+ map = rcu_dereference_protected(vq->maps[index],
+ lockdep_is_held(&vq->mmu_lock));
+ if (map) {
+ if (uaddr->write) {
+ for (i = 0; i < map->npages; i++)
+ set_page_dirty(map->pages[i]);
+ }
+ rcu_assign_pointer(vq->maps[index], NULL);
+ }
+ spin_unlock(&vq->mmu_lock);
+
+ if (map) {
+ synchronize_rcu();
+ vhost_map_unprefetch(map);
+ }
+}
+
+static void vhost_invalidate_vq_end(struct vhost_virtqueue *vq,
+ int index,
+ unsigned long start,
+ unsigned long end)
+{
+ if (!vhost_map_range_overlap(&vq->uaddrs[index], start, end))
+ return;
+
+ spin_lock(&vq->mmu_lock);
+ --vq->invalidate_count;
+ spin_unlock(&vq->mmu_lock);
+}
+
+static int vhost_invalidate_range_start(struct mmu_notifier *mn,
+ const struct mmu_notifier_range *range)
+{
+ struct vhost_dev *dev = container_of(mn, struct vhost_dev,
+ mmu_notifier);
+ int i, j;
+
+ if (!range->blockable)
+ return -EAGAIN;
+
+ for (i = 0; i < dev->nvqs; i++) {
+ struct vhost_virtqueue *vq = dev->vqs[i];
+
+ for (j = 0; j < VHOST_NUM_ADDRS; j++)
+ vhost_invalidate_vq_start(vq, j,
+ range->start,
+ range->end);
+ }
+
+ return 0;
+}
+
+static void vhost_invalidate_range_end(struct mmu_notifier *mn,
+ const struct mmu_notifier_range *range)
+{
+ struct vhost_dev *dev = container_of(mn, struct vhost_dev,
+ mmu_notifier);
+ int i, j;
+
+ for (i = 0; i < dev->nvqs; i++) {
+ struct vhost_virtqueue *vq = dev->vqs[i];
+
+ for (j = 0; j < VHOST_NUM_ADDRS; j++)
+ vhost_invalidate_vq_end(vq, j,
+ range->start,
+ range->end);
+ }
+}
+
+static const struct mmu_notifier_ops vhost_mmu_notifier_ops = {
+ .invalidate_range_start = vhost_invalidate_range_start,
+ .invalidate_range_end = vhost_invalidate_range_end,
+};
+#endif
+
+static void vhost_init_maps(struct vhost_dev *dev)
+{
+#if ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE == 0
+ struct vhost_virtqueue *vq;
+ int i, j;
+
+ dev->mmu_notifier.ops = &vhost_mmu_notifier_ops;
+
+ for (i = 0; i < dev->nvqs; ++i) {
+ vq = dev->vqs[i];
+ for (j = 0; j < VHOST_NUM_ADDRS; j++)
+ RCU_INIT_POINTER(vq->maps[j], NULL);
+ }
+#endif
+}
+
void vhost_dev_init(struct vhost_dev *dev,
struct vhost_virtqueue **vqs, int nvqs, int iov_limit)
{
@@ -459,7 +621,7 @@ void vhost_dev_init(struct vhost_dev *dev,
INIT_LIST_HEAD(&dev->read_list);
INIT_LIST_HEAD(&dev->pending_list);
spin_lock_init(&dev->iotlb_lock);
-
+ vhost_init_maps(dev);
for (i = 0; i < dev->nvqs; ++i) {
vq = dev->vqs[i];
@@ -468,6 +630,7 @@ void vhost_dev_init(struct vhost_dev *dev,
vq->heads = NULL;
vq->dev = dev;
mutex_init(&vq->mutex);
+ spin_lock_init(&vq->mmu_lock);
vhost_vq_reset(dev, vq);
if (vq->handle_kick)
vhost_poll_init(&vq->poll, vq->handle_kick,
@@ -547,7 +710,18 @@ long vhost_dev_set_owner(struct vhost_dev *dev)
if (err)
goto err_cgroup;
+#if ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE == 0
+ err = mmu_notifier_register(&dev->mmu_notifier, dev->mm);
+ if (err)
+ goto err_mmu_notifier;
+#endif
+
return 0;
+
+#if ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE == 0
+err_mmu_notifier:
+ vhost_dev_free_iovecs(dev);
+#endif
err_cgroup:
kthread_stop(worker);
dev->worker = NULL;
@@ -638,6 +812,107 @@ static void vhost_clear_msg(struct vhost_dev *dev)
spin_unlock(&dev->iotlb_lock);
}
+#if ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE == 0
+static void vhost_setup_uaddr(struct vhost_virtqueue *vq,
+ int index, unsigned long uaddr,
+ size_t size, bool write)
+{
+ struct vhost_uaddr *addr = &vq->uaddrs[index];
+
+ addr->uaddr = uaddr;
+ addr->size = size;
+ addr->write = write;
+}
+
+static void vhost_setup_vq_uaddr(struct vhost_virtqueue *vq)
+{
+ vhost_setup_uaddr(vq, VHOST_ADDR_DESC,
+ (unsigned long)vq->desc,
+ vhost_get_desc_size(vq, vq->num),
+ false);
+ vhost_setup_uaddr(vq, VHOST_ADDR_AVAIL,
+ (unsigned long)vq->avail,
+ vhost_get_avail_size(vq, vq->num),
+ false);
+ vhost_setup_uaddr(vq, VHOST_ADDR_USED,
+ (unsigned long)vq->used,
+ vhost_get_used_size(vq, vq->num),
+ true);
+}
+
+static int vhost_map_prefetch(struct vhost_virtqueue *vq,
+ int index)
+{
+ struct vhost_map *map;
+ struct vhost_uaddr *uaddr = &vq->uaddrs[index];
+ struct page **pages;
+ int npages = DIV_ROUND_UP(uaddr->size, PAGE_SIZE);
+ int npinned;
+ void *vaddr, *v;
+ int err;
+ int i;
+
+ spin_lock(&vq->mmu_lock);
+
+ err = -EFAULT;
+ if (vq->invalidate_count)
+ goto err;
+
+ err = -ENOMEM;
+ map = kmalloc(sizeof(*map), GFP_ATOMIC);
+ if (!map)
+ goto err;
+
+ pages = kmalloc_array(npages, sizeof(struct page *), GFP_ATOMIC);
+ if (!pages)
+ goto err_pages;
+
+ err = EFAULT;
+ npinned = __get_user_pages_fast(uaddr->uaddr, npages,
+ uaddr->write, pages);
+ if (npinned > 0)
+ release_pages(pages, npinned);
+ if (npinned != npages)
+ goto err_gup;
+
+ for (i = 0; i < npinned; i++)
+ if (PageHighMem(pages[i]))
+ goto err_gup;
+
+ vaddr = v = page_address(pages[0]);
+
+ /* For simplicity, fallback to userspace address if VA is not
+ * contigious.
+ */
+ for (i = 1; i < npinned; i++) {
+ v += PAGE_SIZE;
+ if (v != page_address(pages[i]))
+ goto err_gup;
+ }
+
+ map->addr = vaddr + (uaddr->uaddr & (PAGE_SIZE - 1));
+ map->npages = npages;
+ map->pages = pages;
+
+ rcu_assign_pointer(vq->maps[index], map);
+ /* No need for a synchronize_rcu(). This function should be
+ * called by dev->worker so we are serialized with all
+ * readers.
+ */
+ spin_unlock(&vq->mmu_lock);
+
+ return 0;
+
+err_gup:
+ kfree(pages);
+err_pages:
+ kfree(map);
+err:
+ spin_unlock(&vq->mmu_lock);
+ return err;
+}
+#endif
+
void vhost_dev_cleanup(struct vhost_dev *dev)
{
int i;
@@ -667,8 +942,16 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
kthread_stop(dev->worker);
dev->worker = NULL;
}
- if (dev->mm)
+ if (dev->mm) {
+#if ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE == 0
+ mmu_notifier_unregister(&dev->mmu_notifier, dev->mm);
+#endif
mmput(dev->mm);
+ }
+#if ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE == 0
+ for (i = 0; i < dev->nvqs; i++)
+ vhost_uninit_vq_maps(dev->vqs[i]);
+#endif
dev->mm = NULL;
}
EXPORT_SYMBOL_GPL(vhost_dev_cleanup);
@@ -897,6 +1180,26 @@ static inline void __user *__vhost_get_user(struct vhost_virtqueue *vq,
static inline int vhost_put_avail_event(struct vhost_virtqueue *vq)
{
+#if ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE == 0
+ struct vhost_map *map;
+ struct vring_used *used;
+
+ if (!vq->iotlb) {
+ rcu_read_lock();
+
+ map = rcu_dereference(vq->maps[VHOST_ADDR_USED]);
+ if (likely(map)) {
+ used = map->addr;
+ *((__virtio16 *)&used->ring[vq->num]) =
+ cpu_to_vhost16(vq, vq->avail_idx);
+ rcu_read_unlock();
+ return 0;
+ }
+
+ rcu_read_unlock();
+ }
+#endif
+
return vhost_put_user(vq, cpu_to_vhost16(vq, vq->avail_idx),
vhost_avail_event(vq));
}
@@ -905,6 +1208,27 @@ static inline int vhost_put_used(struct vhost_virtqueue *vq,
struct vring_used_elem *head, int idx,
int count)
{
+#if ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE == 0
+ struct vhost_map *map;
+ struct vring_used *used;
+ size_t size;
+
+ if (!vq->iotlb) {
+ rcu_read_lock();
+
+ map = rcu_dereference(vq->maps[VHOST_ADDR_USED]);
+ if (likely(map)) {
+ used = map->addr;
+ size = count * sizeof(*head);
+ memcpy(used->ring + idx, head, size);
+ rcu_read_unlock();
+ return 0;
+ }
+
+ rcu_read_unlock();
+ }
+#endif
+
return vhost_copy_to_user(vq, vq->used->ring + idx, head,
count * sizeof(*head));
}
@@ -912,6 +1236,25 @@ static inline int vhost_put_used(struct vhost_virtqueue *vq,
static inline int vhost_put_used_flags(struct vhost_virtqueue *vq)
{
+#if ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE == 0
+ struct vhost_map *map;
+ struct vring_used *used;
+
+ if (!vq->iotlb) {
+ rcu_read_lock();
+
+ map = rcu_dereference(vq->maps[VHOST_ADDR_USED]);
+ if (likely(map)) {
+ used = map->addr;
+ used->flags = cpu_to_vhost16(vq, vq->used_flags);
+ rcu_read_unlock();
+ return 0;
+ }
+
+ rcu_read_unlock();
+ }
+#endif
+
return vhost_put_user(vq, cpu_to_vhost16(vq, vq->used_flags),
&vq->used->flags);
}
@@ -919,6 +1262,25 @@ static inline int vhost_put_used_flags(struct vhost_virtqueue *vq)
static inline int vhost_put_used_idx(struct vhost_virtqueue *vq)
{
+#if ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE == 0
+ struct vhost_map *map;
+ struct vring_used *used;
+
+ if (!vq->iotlb) {
+ rcu_read_lock();
+
+ map = rcu_dereference(vq->maps[VHOST_ADDR_USED]);
+ if (likely(map)) {
+ used = map->addr;
+ used->idx = cpu_to_vhost16(vq, vq->last_used_idx);
+ rcu_read_unlock();
+ return 0;
+ }
+
+ rcu_read_unlock();
+ }
+#endif
+
return vhost_put_user(vq, cpu_to_vhost16(vq, vq->last_used_idx),
&vq->used->idx);
}
@@ -964,12 +1326,50 @@ static void vhost_dev_unlock_vqs(struct vhost_dev *d)
static inline int vhost_get_avail_idx(struct vhost_virtqueue *vq,
__virtio16 *idx)
{
+#if ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE == 0
+ struct vhost_map *map;
+ struct vring_avail *avail;
+
+ if (!vq->iotlb) {
+ rcu_read_lock();
+
+ map = rcu_dereference(vq->maps[VHOST_ADDR_AVAIL]);
+ if (likely(map)) {
+ avail = map->addr;
+ *idx = avail->idx;
+ rcu_read_unlock();
+ return 0;
+ }
+
+ rcu_read_unlock();
+ }
+#endif
+
return vhost_get_avail(vq, *idx, &vq->avail->idx);
}
static inline int vhost_get_avail_head(struct vhost_virtqueue *vq,
__virtio16 *head, int idx)
{
+#if ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE == 0
+ struct vhost_map *map;
+ struct vring_avail *avail;
+
+ if (!vq->iotlb) {
+ rcu_read_lock();
+
+ map = rcu_dereference(vq->maps[VHOST_ADDR_AVAIL]);
+ if (likely(map)) {
+ avail = map->addr;
+ *head = avail->ring[idx & (vq->num - 1)];
+ rcu_read_unlock();
+ return 0;
+ }
+
+ rcu_read_unlock();
+ }
+#endif
+
return vhost_get_avail(vq, *head,
&vq->avail->ring[idx & (vq->num - 1)]);
}
@@ -977,24 +1377,98 @@ static inline int vhost_get_avail_head(struct vhost_virtqueue *vq,
static inline int vhost_get_avail_flags(struct vhost_virtqueue *vq,
__virtio16 *flags)
{
+#if ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE == 0
+ struct vhost_map *map;
+ struct vring_avail *avail;
+
+ if (!vq->iotlb) {
+ rcu_read_lock();
+
+ map = rcu_dereference(vq->maps[VHOST_ADDR_AVAIL]);
+ if (likely(map)) {
+ avail = map->addr;
+ *flags = avail->flags;
+ rcu_read_unlock();
+ return 0;
+ }
+
+ rcu_read_unlock();
+ }
+#endif
+
return vhost_get_avail(vq, *flags, &vq->avail->flags);
}
static inline int vhost_get_used_event(struct vhost_virtqueue *vq,
__virtio16 *event)
{
+#if ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE == 0
+ struct vhost_map *map;
+ struct vring_avail *avail;
+
+ if (!vq->iotlb) {
+ rcu_read_lock();
+ map = rcu_dereference(vq->maps[VHOST_ADDR_AVAIL]);
+ if (likely(map)) {
+ avail = map->addr;
+ *event = (__virtio16)avail->ring[vq->num];
+ rcu_read_unlock();
+ return 0;
+ }
+ rcu_read_unlock();
+ }
+#endif
+
return vhost_get_avail(vq, *event, vhost_used_event(vq));
}
static inline int vhost_get_used_idx(struct vhost_virtqueue *vq,
__virtio16 *idx)
{
+#if ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE == 0
+ struct vhost_map *map;
+ struct vring_used *used;
+
+ if (!vq->iotlb) {
+ rcu_read_lock();
+
+ map = rcu_dereference(vq->maps[VHOST_ADDR_USED]);
+ if (likely(map)) {
+ used = map->addr;
+ *idx = used->idx;
+ rcu_read_unlock();
+ return 0;
+ }
+
+ rcu_read_unlock();
+ }
+#endif
+
return vhost_get_used(vq, *idx, &vq->used->idx);
}
static inline int vhost_get_desc(struct vhost_virtqueue *vq,
struct vring_desc *desc, int idx)
{
+#if ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE == 0
+ struct vhost_map *map;
+ struct vring_desc *d;
+
+ if (!vq->iotlb) {
+ rcu_read_lock();
+
+ map = rcu_dereference(vq->maps[VHOST_ADDR_DESC]);
+ if (likely(map)) {
+ d = map->addr;
+ *desc = *(d + idx);
+ rcu_read_unlock();
+ return 0;
+ }
+
+ rcu_read_unlock();
+ }
+#endif
+
return vhost_copy_from_user(vq, desc, vq->desc + idx, sizeof(*desc));
}
@@ -1335,12 +1809,32 @@ static bool iotlb_access_ok(struct vhost_virtqueue *vq,
return true;
}
+#if ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE == 0
+static void vhost_vq_map_prefetch(struct vhost_virtqueue *vq)
+{
+ struct vhost_map __rcu *map;
+ int i;
+
+ for (i = 0; i < VHOST_NUM_ADDRS; i++) {
+ rcu_read_lock();
+ map = rcu_dereference(vq->maps[i]);
+ rcu_read_unlock();
+ if (unlikely(!map))
+ vhost_map_prefetch(vq, i);
+ }
+}
+#endif
+
int vq_meta_prefetch(struct vhost_virtqueue *vq)
{
unsigned int num = vq->num;
- if (!vq->iotlb)
+ if (!vq->iotlb) {
+#if ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE == 0
+ vhost_vq_map_prefetch(vq);
+#endif
return 1;
+ }
return iotlb_access_ok(vq, VHOST_ACCESS_RO, (u64)(uintptr_t)vq->desc,
vhost_get_desc_size(vq, num), VHOST_ADDR_DESC) &&
@@ -1551,6 +2045,16 @@ static long vhost_vring_set_num_addr(struct vhost_dev *d,
mutex_lock(&vq->mutex);
+#if ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE == 0
+ /* Unregister MMU notifer to allow invalidation callback
+ * can access vq->uaddrs[] without holding a lock.
+ */
+ if (d->mm)
+ mmu_notifier_unregister(&d->mmu_notifier, d->mm);
+
+ vhost_uninit_vq_maps(vq);
+#endif
+
switch (ioctl) {
case VHOST_SET_VRING_NUM:
r = vhost_vring_set_num(d, vq, argp);
@@ -1562,6 +2066,13 @@ static long vhost_vring_set_num_addr(struct vhost_dev *d,
BUG();
}
+#if ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE == 0
+ vhost_setup_vq_uaddr(vq);
+
+ if (d->mm)
+ mmu_notifier_register(&d->mmu_notifier, d->mm);
+#endif
+
mutex_unlock(&vq->mutex);
return r;
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 7a7fc001265f..f33208b55875 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -12,6 +12,9 @@
#include <linux/virtio_config.h>
#include <linux/virtio_ring.h>
#include <linux/atomic.h>
+#include <linux/pagemap.h>
+#include <linux/mmu_notifier.h>
+#include <asm/cacheflush.h>
struct vhost_work;
typedef void (*vhost_work_fn_t)(struct vhost_work *work);
@@ -80,6 +83,18 @@ enum vhost_uaddr_type {
VHOST_NUM_ADDRS = 3,
};
+struct vhost_map {
+ int npages;
+ void *addr;
+ struct page **pages;
+};
+
+struct vhost_uaddr {
+ unsigned long uaddr;
+ size_t size;
+ bool write;
+};
+
/* The virtqueue structure describes a queue attached to a device. */
struct vhost_virtqueue {
struct vhost_dev *dev;
@@ -90,7 +105,21 @@ struct vhost_virtqueue {
struct vring_desc __user *desc;
struct vring_avail __user *avail;
struct vring_used __user *used;
+
+#if ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE == 0
+ /* read by memory accessors, modified by meta data
+ * prefetching, MMU notifier and vring ioctl().
+ * Synchonrized through mmu_lock (writers) and RCU (writers
+ * and readers).
+ */
+ struct vhost_map __rcu *maps[VHOST_NUM_ADDRS];
+ /* read by MMU notifier, write by vring ioctl(), synchronized
+ * through register/unregister MMU notifier.
+ */
+ struct vhost_uaddr uaddrs[VHOST_NUM_ADDRS];
+#endif
const struct vhost_umem_node *meta_iotlb[VHOST_NUM_ADDRS];
+
struct file *kick;
struct eventfd_ctx *call_ctx;
struct eventfd_ctx *error_ctx;
@@ -145,6 +174,8 @@ struct vhost_virtqueue {
bool user_be;
#endif
u32 busyloop_timeout;
+ spinlock_t mmu_lock;
+ int invalidate_count;
};
struct vhost_msg_node {
@@ -158,6 +189,7 @@ struct vhost_msg_node {
struct vhost_dev {
struct mm_struct *mm;
+ struct mmu_notifier mmu_notifier;
struct mutex mutex;
struct vhost_virtqueue **vqs;
int nvqs;
--
2.18.1
^ permalink raw reply related
* Re: [RFC 2/3] hw/virtio-rdma: VirtIO rdma device
From: Cornelia Huck @ 2019-04-23 7:59 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-rdma, qemu-devel, Yuval Shaia, virtualization, jgg
In-Reply-To: <20190419191910-mutt-send-email-mst@kernel.org>
On Fri, 19 Apr 2019 19:20:22 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Apr 11, 2019 at 02:01:56PM +0300, Yuval Shaia wrote:
> > diff --git a/include/standard-headers/linux/virtio_ids.h b/include/standard-headers/linux/virtio_ids.h
> > index 6d5c3b2d4f..bd2c699450 100644
> > --- a/include/standard-headers/linux/virtio_ids.h
> > +++ b/include/standard-headers/linux/virtio_ids.h
> > @@ -43,5 +43,6 @@
> > #define VIRTIO_ID_INPUT 18 /* virtio input */
> > #define VIRTIO_ID_VSOCK 19 /* virtio vsock transport */
> > #define VIRTIO_ID_CRYPTO 20 /* virtio crypto */
> > +#define VIRTIO_ID_RDMA 26 /* virtio crypto */
I think 26 is already de facto used by virtio-audio(?) and 27 has just
been reserved for virtio-pmem. The next free id seems to be 28.
(Also, the comment is a straight copy-paste :)
> >
> > #endif /* _LINUX_VIRTIO_IDS_H */
>
>
> Please reserve an ID with the virtio TC.
> Process:
> - create a github issue
> - post patch with Fixes: tag
> - wait a bit
> - if no comments on either ask for a vote
Agreed, let's reserve an id. There seems to be enough interest in this
device type.
^ 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