* Re: [RFC PATCH net-next v6 4/4] netvsc: refactor notifier/event handling code to use the bypass framework
From: Stephen Hemminger @ 2018-04-10 23:59 UTC (permalink / raw)
To: Siwei Liu
Cc: Alexander Duyck, virtio-dev, Jiri Pirko, Michael S. Tsirkin,
Jakub Kicinski, Sridhar Samudrala, virtualization, Netdev,
David Miller
In-Reply-To: <CADGSJ22rVsC0TDTd6OKVnwbx0ExoQ8xWXBMumKB-OFH4sX=yaQ@mail.gmail.com>
On Tue, 10 Apr 2018 16:44:47 -0700
Siwei Liu <loseweigh@gmail.com> wrote:
> On Tue, Apr 10, 2018 at 4:28 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Tue, Apr 10, 2018 at 02:26:08PM -0700, Stephen Hemminger wrote:
> >> On Tue, 10 Apr 2018 11:59:50 -0700
> >> Sridhar Samudrala <sridhar.samudrala@intel.com> wrote:
> >>
> >> > Use the registration/notification framework supported by the generic
> >> > bypass infrastructure.
> >> >
> >> > Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> >> > ---
> >>
> >> Thanks for doing this. Your current version has couple show stopper
> >> issues.
> >>
> >> First, the slave device is instantly taking over the slave.
> >> This doesn't allow udev/systemd to do its device rename of the slave
> >> device. Netvsc uses a delayed work to workaround this.
> >
> > Interesting. Does this mean udev must act within a specific time window
> > then?
>
> Sighs, lots of hacks. Why propgating this from driver to a common
> module. We really need a clean solution.
>
I had a patch to wait for udev to do the rename and go from there
but davem rejected it.
^ permalink raw reply
* Re: [RFC PATCH net-next v6 4/4] netvsc: refactor notifier/event handling code to use the bypass framework
From: Michael S. Tsirkin @ 2018-04-11 1:21 UTC (permalink / raw)
To: Stephen Hemminger
Cc: alexander.h.duyck, virtio-dev, jiri, kubakici, Sridhar Samudrala,
virtualization, loseweigh, netdev, davem
In-Reply-To: <20180410142608.50f15b45@xeon-e3>
On Tue, Apr 10, 2018 at 02:26:08PM -0700, Stephen Hemminger wrote:
> On Tue, 10 Apr 2018 11:59:50 -0700
> Sridhar Samudrala <sridhar.samudrala@intel.com> wrote:
>
> > Use the registration/notification framework supported by the generic
> > bypass infrastructure.
> >
> > Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> > ---
>
> Thanks for doing this. Your current version has couple show stopper
> issues.
>
> First, the slave device is instantly taking over the slave.
> This doesn't allow udev/systemd to do its device rename of the slave
> device. Netvsc uses a delayed work to workaround this.
>
> Secondly, the select queue needs to call queue selection in VF.
> The bonding/teaming logic doesn't work well for UDP flows.
> Commit b3bf5666a510 ("hv_netvsc: defer queue selection to VF")
> fixed this performance problem.
>
> Lastly, more indirection is bad in current climate.
Well right now netvsc does an indirect call to the PT device,
does it not? If you really want max performance when PT
is in use you need to do the reverse and have PT forward to netvsc.
> I am not completely adverse to this but it needs to be fast, simple
> and completely transparent.
^ permalink raw reply
* Re: [PATCH v2 0/2] vhost: fix vhost_vq_access_ok() log check
From: Stefan Hajnoczi @ 2018-04-11 1:21 UTC (permalink / raw)
To: David Miller
Cc: kvm, mst, netdev, syzkaller-bugs, linux-kernel, virtualization,
torvalds
In-Reply-To: <20180410.105043.1798364865868187298.davem@davemloft.net>
[-- Attachment #1.1: Type: text/plain, Size: 1182 bytes --]
On Tue, Apr 10, 2018 at 10:50:43AM -0400, David Miller wrote:
> From: Jason Wang <jasowang@redhat.com>
> Date: Tue, 10 Apr 2018 14:40:10 +0800
>
> > On 2018年04月10日 13:26, Stefan Hajnoczi wrote:
> >> v2:
> >> * Rewrote the conditional to make the vq access check clearer [Linus]
> >> * Added Patch 2 to make the return type consistent and harder to misuse
> >> * [Linus]
> >>
> >> The first patch fixes the vhost virtqueue access check which was
> >> recently
> >> broken. The second patch replaces the int return type with bool to
> >> prevent
> >> future bugs.
> >>
> >> Stefan Hajnoczi (2):
> >> vhost: fix vhost_vq_access_ok() log check
> >> vhost: return bool from *_access_ok() functions
> >>
> >> drivers/vhost/vhost.h | 4 +--
> >> drivers/vhost/vhost.c | 70
> >> ++++++++++++++++++++++++++-------------------------
> >> 2 files changed, 38 insertions(+), 36 deletions(-)
> >>
> >
> > Acked-by: Jason Wang <jasowang@redhat.com>
>
> This sereis doesn't apply cleanly to the net tree, please respin
> and add the appropriate Acks and Fixes: tags that Michael and Jason
> have provided.
Sorry, will fix.
Stefan
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v29 1/4] mm: support reporting free page blocks
From: Wei Wang @ 2018-04-11 1:22 UTC (permalink / raw)
To: Michael S. Tsirkin, Andrew Morton
Cc: yang.zhang.wz, virtio-dev, riel, quan.xu0, kvm, nilal,
liliang.opensource, linux-kernel, mhocko, linux-mm, huangzhichao,
pbonzini, virtualization
In-Reply-To: <20180411022440-mutt-send-email-mst@kernel.org>
On 04/11/2018 07:25 AM, Michael S. Tsirkin wrote:
> On Tue, Apr 10, 2018 at 01:54:29PM -0700, Andrew Morton wrote:
>> On Tue, 10 Apr 2018 21:19:31 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>
>>> Andrew, were your questions answered? If yes could I bother you for an ack on this?
>>>
>> Still not very happy that readers are told that "this function may
>> sleep" when it clearly doesn't do so. If we wish to be able to change
>> it to sleep in the future then that should be mentioned. And even put a
>> might_sleep() in there, to catch people who didn't read the comments...
>>
>> Otherwise it looks OK.
> Oh, might_sleep with a comment explaining it's for the future sounds
> good to me. I queued this - Wei, could you post a patch on top pls?
>
I'm just thinking if it would be necessary to add another might_sleep,
because we've had a cond_resched there which has wrapped a __might_sleep.
Best,
Wei
^ permalink raw reply
* RE: [virtio-dev] Re: [RFC] vhost: introduce mdev based hardware vhost backend
From: Tian, Kevin @ 2018-04-11 1:38 UTC (permalink / raw)
To: Liang, Cunming, Michael S. Tsirkin
Cc: Duyck, Alexander H, virtio-dev@lists.oasis-open.org,
kvm@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org, Wang, Xiao W,
ddutile@redhat.com, Tan, Jianfeng, Wang, Zhihong, Paolo Bonzini
In-Reply-To: <D0158A423229094DA7ABF71CF2FA0DA34E9226AF@SHSMSX104.ccr.corp.intel.com>
> From: Liang, Cunming
> Sent: Tuesday, April 10, 2018 10:24 PM
>
[...]
> >
> > As others said, we do not need to go overeboard. A couple of small
> vendor-
> > specific quirks in qemu isn't a big deal.
>
> It's quite challenge to identify it's small or not, there's no uniform metric.
>
> It's only dependent on QEMU itself, that's the obvious benefit. Tradeoff is
> to build the entire device driver. We don't object to do that in QEMU, but
> wanna make sure to understand the boundary size clearly.
>
It might be helpful if you can post some sample code using proposed
framework - then people have a clear feeling about what size is talked
about here and whether it falls into the concept of 'small quirks'.
Thanks
Kevin
^ permalink raw reply
* Re: [virtio-dev] Re: [RFC] vhost: introduce mdev based hardware vhost backend
From: Stefan Hajnoczi @ 2018-04-11 2:01 UTC (permalink / raw)
To: Liang, Cunming
Cc: Duyck, Alexander H, virtio-dev@lists.oasis-open.org,
kvm@vger.kernel.org, mst@redhat.com, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org, Wang, Xiao W,
ddutile@redhat.com, Tan, Jianfeng, Wang, Zhihong, Paolo Bonzini
In-Reply-To: <D0158A423229094DA7ABF71CF2FA0DA34E9222F4@SHSMSX104.ccr.corp.intel.com>
[-- Attachment #1.1: Type: text/plain, Size: 401 bytes --]
On Tue, Apr 10, 2018 at 09:23:53AM +0000, Liang, Cunming wrote:
> If QEMU is going to build a user space driver framework there, we're open mind on that, even leveraging DPDK as the underlay library. Looking forward to more others' comments from community.
There is already an NVMe VFIO driver in QEMU (see block/nvme.c). So in
principle there's no reason against userspace drivers in QEMU.
Stefan
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [virtio-dev] Re: [RFC] vhost: introduce mdev based hardware vhost backend
From: Jason Wang @ 2018-04-11 2:08 UTC (permalink / raw)
To: Liang, Cunming, Paolo Bonzini, Bie, Tiwei
Cc: Duyck, Alexander H, virtio-dev@lists.oasis-open.org,
kvm@vger.kernel.org, mst@redhat.com, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org, Wang, Xiao W,
ddutile@redhat.com, Tan, Jianfeng, Wang, Zhihong
In-Reply-To: <D0158A423229094DA7ABF71CF2FA0DA34E9222F4@SHSMSX104.ccr.corp.intel.com>
On 2018年04月10日 17:23, Liang, Cunming wrote:
>
>> -----Original Message-----
>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>> Sent: Tuesday, April 10, 2018 3:52 PM
>> To: Bie, Tiwei <tiwei.bie@intel.com>; Jason Wang <jasowang@redhat.com>
>> Cc: mst@redhat.com; alex.williamson@redhat.com; ddutile@redhat.com;
>> Duyck, Alexander H <alexander.h.duyck@intel.com>; virtio-dev@lists.oasis-
>> open.org; linux-kernel@vger.kernel.org; kvm@vger.kernel.org;
>> virtualization@lists.linux-foundation.org; netdev@vger.kernel.org; Daly, Dan
>> <dan.daly@intel.com>; Liang, Cunming <cunming.liang@intel.com>; Wang,
>> Zhihong <zhihong.wang@intel.com>; Tan, Jianfeng <jianfeng.tan@intel.com>;
>> Wang, Xiao W <xiao.w.wang@intel.com>
>> Subject: Re: [virtio-dev] Re: [RFC] vhost: introduce mdev based hardware
>> vhost backend
>>
>> On 10/04/2018 06:57, Tiwei Bie wrote:
>>>> So you just move the abstraction layer from qemu to kernel, and you
>>>> still need different drivers in kernel for different device
>>>> interfaces of accelerators. This looks even more complex than leaving
>>>> it in qemu. As you said, another idea is to implement userspace vhost
>>>> backend for accelerators which seems easier and could co-work with
>>>> other parts of qemu without inventing new type of messages.
>>> I'm not quite sure. Do you think it's acceptable to add various vendor
>>> specific hardware drivers in QEMU?
>> I think so. We have vendor-specific quirks, and at some point there was an
>> idea of using quirks to implement (vendor-specific) live migration support for
>> assigned devices.
> Vendor-specific quirks of accessing VGA is a small portion. Other major portions are still handled by guest driver.
>
> While in this case, when saying various vendor specific drivers in QEMU, it says QEMU takes over and provides the entire user space device drivers. Some parts are even not relevant to vhost, they're basic device function enabling. Moreover, it could be different kinds of devices(network/block/...) under vhost. No matter # of vendors or # of types, total LOC is not small.
>
> The idea is to avoid introducing these extra complexity out of QEMU, keeping vhost adapter simple. As vhost protocol is de factor standard, it leverages kernel device driver to provide the diversity. Changing once in QEMU, then it supports multi-vendor devices whose drivers naturally providing kernel driver there.
Let me clarify my question, it's not qemu vs kenrel but userspace vs
kernel. It could be a library which could be linked to qemu. Doing it in
userspace have the following obvious advantages:
- attack surface was limited to userspace
- easier to be maintained (compared to kernel driver)
- easier to be extended without introducing new userspace/kernel interfaces
- not tied to a specific operating system
If we want to do it in the kernel, need to consider to unify code
between mdev device driver and generic driver. For net driver, maybe we
can even consider to do it on top of exist drivers.
>
> If QEMU is going to build a user space driver framework there, we're open mind on that, even leveraging DPDK as the underlay library. Looking forward to more others' comments from community.
I'm doing this now by implementing vhost inside qemu IOThreads. Hope I
can post RFC in few months.
Thanks
> Steve
>
>> Paolo
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [virtio-dev] Re: [RFC] vhost: introduce mdev based hardware vhost backend
From: Jason Wang @ 2018-04-11 2:18 UTC (permalink / raw)
To: Liang, Cunming, Michael S. Tsirkin
Cc: Duyck, Alexander H, virtio-dev@lists.oasis-open.org,
kvm@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org, Wang, Xiao W,
ddutile@redhat.com, Tan, Jianfeng, Wang, Zhihong, Paolo Bonzini
In-Reply-To: <D0158A423229094DA7ABF71CF2FA0DA34E9226AF@SHSMSX104.ccr.corp.intel.com>
On 2018年04月10日 22:23, Liang, Cunming wrote:
>> -----Original Message-----
>> From: Michael S. Tsirkin [mailto:mst@redhat.com]
>> Sent: Tuesday, April 10, 2018 9:36 PM
>> To: Liang, Cunming<cunming.liang@intel.com>
>> Cc: Paolo Bonzini<pbonzini@redhat.com>; Bie, Tiwei<tiwei.bie@intel.com>;
>> Jason Wang<jasowang@redhat.com>;alex.williamson@redhat.com;
>> ddutile@redhat.com; Duyck, Alexander H<alexander.h.duyck@intel.com>;
>> virtio-dev@lists.oasis-open.org;linux-kernel@vger.kernel.org;
>> kvm@vger.kernel.org;virtualization@lists.linux-foundation.org;
>> netdev@vger.kernel.org; Daly, Dan<dan.daly@intel.com>; Wang, Zhihong
>> <zhihong.wang@intel.com>; Tan, Jianfeng<jianfeng.tan@intel.com>; Wang, Xiao
>> W<xiao.w.wang@intel.com>
>> Subject: Re: [virtio-dev] Re: [RFC] vhost: introduce mdev based hardware vhost
>> backend
>>
>> On Tue, Apr 10, 2018 at 09:23:53AM +0000, Liang, Cunming wrote:
>>>> -----Original Message-----
>>>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>>>> Sent: Tuesday, April 10, 2018 3:52 PM
>>>> To: Bie, Tiwei<tiwei.bie@intel.com>; Jason Wang
>>>> <jasowang@redhat.com>
>>>> Cc:mst@redhat.com;alex.williamson@redhat.com;ddutile@redhat.com;
>>>> Duyck, Alexander H<alexander.h.duyck@intel.com>;
>>>> virtio-dev@lists.oasis- open.org;linux-kernel@vger.kernel.org;
>>>> kvm@vger.kernel.org;virtualization@lists.linux-foundation.org;
>>>> netdev@vger.kernel.org; Daly, Dan<dan.daly@intel.com>; Liang,
>>>> Cunming<cunming.liang@intel.com>; Wang, Zhihong
>>>> <zhihong.wang@intel.com>; Tan, Jianfeng<jianfeng.tan@intel.com>;
>>>> Wang, Xiao W<xiao.w.wang@intel.com>
>>>> Subject: Re: [virtio-dev] Re: [RFC] vhost: introduce mdev based
>>>> hardware vhost backend
>>>>
>>>> On 10/04/2018 06:57, Tiwei Bie wrote:
>>>>>> So you just move the abstraction layer from qemu to kernel, and
>>>>>> you still need different drivers in kernel for different device
>>>>>> interfaces of accelerators. This looks even more complex than
>>>>>> leaving it in qemu. As you said, another idea is to implement
>>>>>> userspace vhost backend for accelerators which seems easier and
>>>>>> could co-work with other parts of qemu without inventing new type of
>> messages.
>>>>> I'm not quite sure. Do you think it's acceptable to add various
>>>>> vendor specific hardware drivers in QEMU?
>>>> I think so. We have vendor-specific quirks, and at some point there
>>>> was an idea of using quirks to implement (vendor-specific) live
>>>> migration support for assigned devices.
>>> Vendor-specific quirks of accessing VGA is a small portion. Other major portions
>> are still handled by guest driver.
>>> While in this case, when saying various vendor specific drivers in QEMU, it says
>> QEMU takes over and provides the entire user space device drivers. Some parts
>> are even not relevant to vhost, they're basic device function enabling. Moreover,
>> it could be different kinds of devices(network/block/...) under vhost. No matter
>> # of vendors or # of types, total LOC is not small.
>>> The idea is to avoid introducing these extra complexity out of QEMU, keeping
>> vhost adapter simple. As vhost protocol is de factor standard, it leverages kernel
>> device driver to provide the diversity. Changing once in QEMU, then it supports
>> multi-vendor devices whose drivers naturally providing kernel driver there.
>>> If QEMU is going to build a user space driver framework there, we're open mind
>> on that, even leveraging DPDK as the underlay library. Looking forward to more
>> others' comments from community.
>>> Steve
>> Dependency on a kernel driver is fine IMHO. It's the dependency on a DPDK
>> backend that makes people unhappy, since the functionality in question is setup-
>> time only.
> Agreed, we don't see dependency on kernel driver is a problem.
At engineering level, kernel driver is harder than userspace driver.
>
> mdev based vhost backend (this patch set) is independent with vhost-user extension patch set. In fact, there're a few vhost-user providers, DPDK librte_vhost is one of them. FD.IO/VPP and snabbswitch have their own vhost-user providers. So I can't agree on vhost-user extension patch depends on DPDK backend. But anyway, that's the topic of another mail thread.
>
Well we can treat mdev as another kind of transport of vhost-user. And
technically we can even implement a relay mdev than forward vhost-user
messages to dpdk.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH v3 0/2] vhost: fix vhost_vq_access_ok() log check
From: Stefan Hajnoczi @ 2018-04-11 2:35 UTC (permalink / raw)
To: virtualization
Cc: kvm, mst, netdev, syzkaller-bugs, linux-kernel, Stefan Hajnoczi,
Linus Torvalds
v3:
* Rebased onto net/master and resolved conflict [DaveM]
v2:
* Rewrote the conditional to make the vq access check clearer [Linus]
* Added Patch 2 to make the return type consistent and harder to misuse [Linus]
The first patch fixes the vhost virtqueue access check which was recently
broken. The second patch replaces the int return type with bool to prevent
future bugs.
Stefan Hajnoczi (2):
vhost: fix vhost_vq_access_ok() log check
vhost: return bool from *_access_ok() functions
drivers/vhost/vhost.h | 4 +--
drivers/vhost/vhost.c | 70 ++++++++++++++++++++++++++-------------------------
2 files changed, 38 insertions(+), 36 deletions(-)
--
2.14.3
^ permalink raw reply
* [PATCH v3 2/2] vhost: return bool from *_access_ok() functions
From: Stefan Hajnoczi @ 2018-04-11 2:35 UTC (permalink / raw)
To: virtualization
Cc: kvm, mst, netdev, syzkaller-bugs, linux-kernel, Stefan Hajnoczi,
Linus Torvalds
In-Reply-To: <20180411023541.15776-1-stefanha@redhat.com>
Currently vhost *_access_ok() functions return int. This is error-prone
because there are two popular conventions:
1. 0 means failure, 1 means success
2. -errno means failure, 0 means success
Although vhost mostly uses #1, it does not do so consistently.
umem_access_ok() uses #2.
This patch changes the return type from int to bool so that false means
failure and true means success. This eliminates a potential source of
errors.
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
drivers/vhost/vhost.h | 4 ++--
drivers/vhost/vhost.c | 66 +++++++++++++++++++++++++--------------------------
2 files changed, 35 insertions(+), 35 deletions(-)
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index d8ee85ae8fdc..6c844b90a168 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -178,8 +178,8 @@ void vhost_dev_cleanup(struct vhost_dev *);
void vhost_dev_stop(struct vhost_dev *);
long vhost_dev_ioctl(struct vhost_dev *, unsigned int ioctl, void __user *argp);
long vhost_vring_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp);
-int vhost_vq_access_ok(struct vhost_virtqueue *vq);
-int vhost_log_access_ok(struct vhost_dev *);
+bool vhost_vq_access_ok(struct vhost_virtqueue *vq);
+bool vhost_log_access_ok(struct vhost_dev *);
int vhost_get_vq_desc(struct vhost_virtqueue *,
struct iovec iov[], unsigned int iov_count,
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index fc805b7fad9d..0fcb51a9940c 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -641,14 +641,14 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
}
EXPORT_SYMBOL_GPL(vhost_dev_cleanup);
-static int log_access_ok(void __user *log_base, u64 addr, unsigned long sz)
+static bool log_access_ok(void __user *log_base, u64 addr, unsigned long sz)
{
u64 a = addr / VHOST_PAGE_SIZE / 8;
/* Make sure 64 bit math will not overflow. */
if (a > ULONG_MAX - (unsigned long)log_base ||
a + (unsigned long)log_base > ULONG_MAX)
- return 0;
+ return false;
return access_ok(VERIFY_WRITE, log_base + a,
(sz + VHOST_PAGE_SIZE * 8 - 1) / VHOST_PAGE_SIZE / 8);
@@ -661,30 +661,30 @@ static bool vhost_overflow(u64 uaddr, u64 size)
}
/* Caller should have vq mutex and device mutex. */
-static int vq_memory_access_ok(void __user *log_base, struct vhost_umem *umem,
- int log_all)
+static bool vq_memory_access_ok(void __user *log_base, struct vhost_umem *umem,
+ int log_all)
{
struct vhost_umem_node *node;
if (!umem)
- return 0;
+ return false;
list_for_each_entry(node, &umem->umem_list, link) {
unsigned long a = node->userspace_addr;
if (vhost_overflow(node->userspace_addr, node->size))
- return 0;
+ return false;
if (!access_ok(VERIFY_WRITE, (void __user *)a,
node->size))
- return 0;
+ return false;
else if (log_all && !log_access_ok(log_base,
node->start,
node->size))
- return 0;
+ return false;
}
- return 1;
+ return true;
}
static inline void __user *vhost_vq_meta_fetch(struct vhost_virtqueue *vq,
@@ -701,13 +701,13 @@ static inline void __user *vhost_vq_meta_fetch(struct vhost_virtqueue *vq,
/* Can we switch to this memory table? */
/* Caller should have device mutex but not vq mutex */
-static int memory_access_ok(struct vhost_dev *d, struct vhost_umem *umem,
- int log_all)
+static bool memory_access_ok(struct vhost_dev *d, struct vhost_umem *umem,
+ int log_all)
{
int i;
for (i = 0; i < d->nvqs; ++i) {
- int ok;
+ bool ok;
bool log;
mutex_lock(&d->vqs[i]->mutex);
@@ -717,12 +717,12 @@ static int memory_access_ok(struct vhost_dev *d, struct vhost_umem *umem,
ok = vq_memory_access_ok(d->vqs[i]->log_base,
umem, log);
else
- ok = 1;
+ ok = true;
mutex_unlock(&d->vqs[i]->mutex);
if (!ok)
- return 0;
+ return false;
}
- return 1;
+ return true;
}
static int translate_desc(struct vhost_virtqueue *vq, u64 addr, u32 len,
@@ -959,21 +959,21 @@ static void vhost_iotlb_notify_vq(struct vhost_dev *d,
spin_unlock(&d->iotlb_lock);
}
-static int umem_access_ok(u64 uaddr, u64 size, int access)
+static bool umem_access_ok(u64 uaddr, u64 size, int access)
{
unsigned long a = uaddr;
/* Make sure 64 bit math will not overflow. */
if (vhost_overflow(uaddr, size))
- return -EFAULT;
+ return false;
if ((access & VHOST_ACCESS_RO) &&
!access_ok(VERIFY_READ, (void __user *)a, size))
- return -EFAULT;
+ return false;
if ((access & VHOST_ACCESS_WO) &&
!access_ok(VERIFY_WRITE, (void __user *)a, size))
- return -EFAULT;
- return 0;
+ return false;
+ return true;
}
static int vhost_process_iotlb_msg(struct vhost_dev *dev,
@@ -988,7 +988,7 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev,
ret = -EFAULT;
break;
}
- if (umem_access_ok(msg->uaddr, msg->size, msg->perm)) {
+ if (!umem_access_ok(msg->uaddr, msg->size, msg->perm)) {
ret = -EFAULT;
break;
}
@@ -1135,10 +1135,10 @@ static int vhost_iotlb_miss(struct vhost_virtqueue *vq, u64 iova, int access)
return 0;
}
-static int vq_access_ok(struct vhost_virtqueue *vq, unsigned int num,
- struct vring_desc __user *desc,
- struct vring_avail __user *avail,
- struct vring_used __user *used)
+static bool vq_access_ok(struct vhost_virtqueue *vq, unsigned int num,
+ struct vring_desc __user *desc,
+ struct vring_avail __user *avail,
+ struct vring_used __user *used)
{
size_t s = vhost_has_feature(vq, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
@@ -1161,8 +1161,8 @@ static void vhost_vq_meta_update(struct vhost_virtqueue *vq,
vq->meta_iotlb[type] = node;
}
-static int iotlb_access_ok(struct vhost_virtqueue *vq,
- int access, u64 addr, u64 len, int type)
+static bool iotlb_access_ok(struct vhost_virtqueue *vq,
+ int access, u64 addr, u64 len, int type)
{
const struct vhost_umem_node *node;
struct vhost_umem *umem = vq->iotlb;
@@ -1220,7 +1220,7 @@ EXPORT_SYMBOL_GPL(vq_iotlb_prefetch);
/* Can we log writes? */
/* Caller should have device mutex but not vq mutex */
-int vhost_log_access_ok(struct vhost_dev *dev)
+bool vhost_log_access_ok(struct vhost_dev *dev)
{
return memory_access_ok(dev, dev->umem, 1);
}
@@ -1228,8 +1228,8 @@ EXPORT_SYMBOL_GPL(vhost_log_access_ok);
/* Verify access for write logging. */
/* Caller should have vq mutex and device mutex */
-static int vq_log_access_ok(struct vhost_virtqueue *vq,
- void __user *log_base)
+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;
@@ -1242,14 +1242,14 @@ static int vq_log_access_ok(struct vhost_virtqueue *vq,
/* Can we start vq? */
/* Caller should have vq mutex and device mutex */
-int vhost_vq_access_ok(struct vhost_virtqueue *vq)
+bool vhost_vq_access_ok(struct vhost_virtqueue *vq)
{
if (!vq_log_access_ok(vq, vq->log_base))
- return 0;
+ return false;
/* Access validation occurs at prefetch time with IOTLB */
if (vq->iotlb)
- return 1;
+ return true;
return vq_access_ok(vq, vq->num, vq->desc, vq->avail, vq->used);
}
--
2.14.3
^ permalink raw reply related
* Re: [virtio-dev] Re: [PATCH v2] virtio_balloon: export hugetlb page allocation counts
From: Jason Wang @ 2018-04-11 3:12 UTC (permalink / raw)
To: Jonathan Helman, Michael S. Tsirkin
Cc: Mihai Carabas, virtio-dev, linux-kernel, virtualization
In-Reply-To: <b1414be4-84bc-5d02-afff-9e42c1fd7210@oracle.com>
On 2018年04月10日 05:11, Jonathan Helman wrote:
>
>
> On 03/22/2018 07:38 PM, Jason Wang wrote:
>>
>>
>> On 2018年03月22日 11:10, Michael S. Tsirkin wrote:
>>> On Thu, Mar 22, 2018 at 09:52:18AM +0800, Jason Wang wrote:
>>>> On 2018年03月20日 12:26, Jonathan Helman wrote:
>>>>>> On Mar 19, 2018, at 7:31 PM, Jason Wang<jasowang@redhat.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 2018年03月20日 06:14, Jonathan Helman wrote:
>>>>>>> Export the number of successful and failed hugetlb page
>>>>>>> allocations via the virtio balloon driver. These 2 counts
>>>>>>> come directly from the vm_events HTLB_BUDDY_PGALLOC and
>>>>>>> HTLB_BUDDY_PGALLOC_FAIL.
>>>>>>>
>>>>>>> Signed-off-by: Jonathan Helman<jonathan.helman@oracle.com>
>>>>>> Reviewed-by: Jason Wang<jasowang@redhat.com>
>>>>> Thanks.
>>>>>
>>>>>>> ---
>>>>>>> drivers/virtio/virtio_balloon.c | 6 ++++++
>>>>>>> include/uapi/linux/virtio_balloon.h | 4 +++-
>>>>>>> 2 files changed, 9 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/virtio/virtio_balloon.c
>>>>>>> b/drivers/virtio/virtio_balloon.c
>>>>>>> index dfe5684..6b237e3 100644
>>>>>>> --- a/drivers/virtio/virtio_balloon.c
>>>>>>> +++ b/drivers/virtio/virtio_balloon.c
>>>>>>> @@ -272,6 +272,12 @@ static unsigned int
>>>>>>> update_balloon_stats(struct virtio_balloon *vb)
>>>>>>> pages_to_bytes(events[PSWPOUT]));
>>>>>>> update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT,
>>>>>>> events[PGMAJFAULT]);
>>>>>>> update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT,
>>>>>>> events[PGFAULT]);
>>>>>>> +#ifdef CONFIG_HUGETLB_PAGE
>>>>>>> + update_stat(vb, idx++, VIRTIO_BALLOON_S_HTLB_PGALLOC,
>>>>>>> + events[HTLB_BUDDY_PGALLOC]);
>>>>>>> + update_stat(vb, idx++, VIRTIO_BALLOON_S_HTLB_PGFAIL,
>>>>>>> + events[HTLB_BUDDY_PGALLOC_FAIL]);
>>>>>>> +#endif
>>>>>>> #endif
>>>>>>> update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE,
>>>>>>> pages_to_bytes(i.freeram));
>>>>>>> diff --git a/include/uapi/linux/virtio_balloon.h
>>>>>>> b/include/uapi/linux/virtio_balloon.h
>>>>>>> index 4e8b830..40297a3 100644
>>>>>>> --- a/include/uapi/linux/virtio_balloon.h
>>>>>>> +++ b/include/uapi/linux/virtio_balloon.h
>>>>>>> @@ -53,7 +53,9 @@ struct virtio_balloon_config {
>>>>>>> #define VIRTIO_BALLOON_S_MEMTOT 5 /* Total amount of
>>>>>>> memory */
>>>>>>> #define VIRTIO_BALLOON_S_AVAIL 6 /* Available memory as
>>>>>>> in /proc */
>>>>>>> #define VIRTIO_BALLOON_S_CACHES 7 /* Disk caches */
>>>>>>> -#define VIRTIO_BALLOON_S_NR 8
>>>>>>> +#define VIRTIO_BALLOON_S_HTLB_PGALLOC 8 /* Hugetlb page
>>>>>>> allocations */
>>>>>>> +#define VIRTIO_BALLOON_S_HTLB_PGFAIL 9 /* Hugetlb page
>>>>>>> allocation failures */
>>>>>>> +#define VIRTIO_BALLOON_S_NR 10
>>>>>>> /*
>>>>>>> * Memory statistics structure.
>>>>>> Not for this patch, but it looks to me that exporting such nr
>>>>>> through uapi is fragile.
>>>>> Sorry, can you explain what you mean here?
>>>>>
>>>>> Jon
>>>> Spec said "Within an output buffer submitted to the statsq, the
>>>> device MUST
>>>> ignore entries with tag values that it does not recognize". So
>>>> exporting
>>>> VIRTIO_BALLOON_S_NR seems useless and device implementation can not
>>>> depend
>>>> on such number in uapi.
>>>>
>>>> Thanks
>>> Suggestions? I don't like to break build for people ...
>>>
>>
>> Didn't have a good idea. But maybe we should keep VIRTIO_BALLOON_S_NR
>> unchanged, and add a comment here.
>>
>> Thanks
>
> I think Jason's comment is for a future patch. Didn't see this patch
> get applied, so wondering if it could be.
>
> Thanks,
> Jon
Hi Jon:
Have you tested new driver with old qemu?
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC PATCH net-next v5 3/4] virtio_net: Extend virtio to use VF datapath when available
From: Samudrala, Sridhar @ 2018-04-11 6:24 UTC (permalink / raw)
To: Jiri Pirko
Cc: alexander.h.duyck, virtio-dev, mst, kubakici, netdev,
virtualization, loseweigh, davem
In-Reply-To: <20180411060327.GH2028@nanopsycho>
On 4/10/2018 11:03 PM, Jiri Pirko wrote:
> Tue, Apr 10, 2018 at 05:59:02PM CEST, sridhar.samudrala@intel.com wrote:
>> On 4/10/2018 8:43 AM, Jiri Pirko wrote:
>>> Tue, Apr 10, 2018 at 05:27:48PM CEST, sridhar.samudrala@intel.com wrote:
>>>> On 4/10/2018 8:22 AM, Jiri Pirko wrote:
>>>>> Tue, Apr 10, 2018 at 05:13:40PM CEST, sridhar.samudrala@intel.com wrote:
>>>>>> On 4/10/2018 3:55 AM, Jiri Pirko wrote:
>>>>>>> Mon, Apr 09, 2018 at 08:47:06PM CEST, sridhar.samudrala@intel.com wrote:
>>>>>>>> On 4/9/2018 1:07 AM, Jiri Pirko wrote:
>>>>>>>>> Sat, Apr 07, 2018 at 12:59:14AM CEST, sridhar.samudrala@intel.com wrote:
>>>>>>>>>> On 4/6/2018 5:48 AM, Jiri Pirko wrote:
>>>>>>>>>>> Thu, Apr 05, 2018 at 11:08:22PM CEST, sridhar.samudrala@intel.com wrote:
>>>>>>>>> [...]
>>>>>>>>>
>>>>>>>>>>>> +static int virtnet_bypass_join_child(struct net_device *bypass_netdev,
>>>>>>>>>>>> + struct net_device *child_netdev)
>>>>>>>>>>>> +{
>>>>>>>>>>>> + struct virtnet_bypass_info *vbi;
>>>>>>>>>>>> + bool backup;
>>>>>>>>>>>> +
>>>>>>>>>>>> + vbi = netdev_priv(bypass_netdev);
>>>>>>>>>>>> + backup = (child_netdev->dev.parent == bypass_netdev->dev.parent);
>>>>>>>>>>>> + if (backup ? rtnl_dereference(vbi->backup_netdev) :
>>>>>>>>>>>> + rtnl_dereference(vbi->active_netdev)) {
>>>>>>>>>>>> + netdev_info(bypass_netdev,
>>>>>>>>>>>> + "%s attempting to join bypass dev when %s already present\n",
>>>>>>>>>>>> + child_netdev->name, backup ? "backup" : "active");
>>>>>>>>>>> Bypass module should check if there is already some other netdev
>>>>>>>>>>> enslaved and refuse right there.
>>>>>>>>>> This will work for virtio-net with 3 netdev model, but this check has to be done by netvsc
>>>>>>>>>> as its bypass_netdev is same as the backup_netdev.
>>>>>>>>>> Will add a flag while registering with the bypass module to indicate if the driver is doing
>>>>>>>>>> a 2 netdev or 3 netdev model and based on that flag this check can be done in bypass module
>>>>>>>>>> for 3 netdev scenario.
>>>>>>>>> Just let me undestand it clearly. What I expect the difference would be
>>>>>>>>> between 2netdev and3 netdev model is this:
>>>>>>>>> 2netdev:
>>>>>>>>> bypass_master
>>>>>>>>> /
>>>>>>>>> /
>>>>>>>>> VF_slave
>>>>>>>>>
>>>>>>>>> 3netdev:
>>>>>>>>> bypass_master
>>>>>>>>> / \
>>>>>>>>> / \
>>>>>>>>> VF_slave backup_slave
>>>>>>>>>
>>>>>>>>> Is that correct? If not, how does it look like?
>>>>>>>>>
>>>>>>>>>
>>>>>>>> Looks correct.
>>>>>>>> VF_slave and backup_slave are the original netdevs and are present in both the models.
>>>>>>>> In the 3 netdev model, bypass_master netdev is created and VF_slave and backup_slave are
>>>>>>>> marked as the 2 slaves of this new netdev.
>>>>>>> You say it looks correct and in another sentence you provide completely
>>>>>>> different description. Could you please look again?
>>>>>>>
>>>>>> To be exact, 2 netdev model with netvsc looks like this.
>>>>>>
>>>>>> netvsc_netdev
>>>>>> /
>>>>>> /
>>>>>> VF_slave
>>>>>>
>>>>>> With virtio_net, 3 netdev model
>>>>>>
>>>>>> bypass_netdev
>>>>>> / \
>>>>>> / \
>>>>>> VF_slave virtio_net netdev
>>>>> Could you also mark the original netdev which is there now? is it
>>>>> bypass_netdev or virtio_net_netdev ?
>>>> bypass_netdev
>>>> / \
>>>> / \
>>>> VF_slave virtio_net netdev (original)
>>> That does not make sense.
>>> 1) You diverge from the behaviour of the netvsc, where the original
>>> netdev is a master of the VF
>>> 2) If the original netdev is a slave, you cannot have any IP address
>>> configured on it (well you could, but the rx_handler would eat every
>>> incoming packet). So you will break the user bacause he would have to
>>> move the configuration to the new master device.
>>> This only makes sense that the original netdev becomes the master for both
>>> netvsc and virtio_net.
>> Forgot to mention that bypass_netdev takes over the name of the original
>> netdev and
>> virtio_net netdev will get the backup name.
> What do you mean by "name"?
bypass_netdev also is associated with the same pci device as the original virtio_net
netdev via SET_NETDEV_DEV(). Also, we added ndo_get_phys_port_name() to virtio_net
that will return _bkup when BACKUP feature is enabled.
So for ex: if virtio_net inteface was getting 'ens12' as the name assigned by udev
without BACKUP feature, when BACKUP feature is enabled, the bypass_netdev will be
named 'ens12' and the original virtio_net will get named as ens12n_bkup.
>
>> So the userspace network configuration doesn't need to change.
>>
>>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC PATCH net-next v6 4/4] netvsc: refactor notifier/event handling code to use the bypass framework
From: Jiri Pirko @ 2018-04-11 7:50 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: alexander.h.duyck, virtio-dev, kubakici, Sridhar Samudrala,
virtualization, loseweigh, netdev, davem
In-Reply-To: <20180411022807-mutt-send-email-mst@kernel.org>
Wed, Apr 11, 2018 at 01:28:51AM CEST, mst@redhat.com wrote:
>On Tue, Apr 10, 2018 at 02:26:08PM -0700, Stephen Hemminger wrote:
>> On Tue, 10 Apr 2018 11:59:50 -0700
>> Sridhar Samudrala <sridhar.samudrala@intel.com> wrote:
>>
>> > Use the registration/notification framework supported by the generic
>> > bypass infrastructure.
>> >
>> > Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
>> > ---
>>
>> Thanks for doing this. Your current version has couple show stopper
>> issues.
>>
>> First, the slave device is instantly taking over the slave.
>> This doesn't allow udev/systemd to do its device rename of the slave
>> device. Netvsc uses a delayed work to workaround this.
>
>Interesting. Does this mean udev must act within a specific time window
>then?
Yeah. That is scarry. Also, wrong.
>
>> Secondly, the select queue needs to call queue selection in VF.
>> The bonding/teaming logic doesn't work well for UDP flows.
>> Commit b3bf5666a510 ("hv_netvsc: defer queue selection to VF")
>> fixed this performance problem.
>>
>> Lastly, more indirection is bad in current climate.
>>
>> I am not completely adverse to this but it needs to be fast, simple
>> and completely transparent.
^ permalink raw reply
* Re: [RFC PATCH net-next v6 4/4] netvsc: refactor notifier/event handling code to use the bypass framework
From: Jiri Pirko @ 2018-04-11 7:53 UTC (permalink / raw)
To: Stephen Hemminger
Cc: alexander.h.duyck, virtio-dev, mst, kubakici, Sridhar Samudrala,
virtualization, loseweigh, netdev, davem
In-Reply-To: <20180410142608.50f15b45@xeon-e3>
Tue, Apr 10, 2018 at 11:26:08PM CEST, stephen@networkplumber.org wrote:
>On Tue, 10 Apr 2018 11:59:50 -0700
>Sridhar Samudrala <sridhar.samudrala@intel.com> wrote:
>
>> Use the registration/notification framework supported by the generic
>> bypass infrastructure.
>>
>> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
>> ---
>
>Thanks for doing this. Your current version has couple show stopper
>issues.
>
>First, the slave device is instantly taking over the slave.
>This doesn't allow udev/systemd to do its device rename of the slave
>device. Netvsc uses a delayed work to workaround this.
Wait. Why the fact a device is enslaved has to affect the udev in any
way? If it does, smells like a bug in udev.
>
>Secondly, the select queue needs to call queue selection in VF.
>The bonding/teaming logic doesn't work well for UDP flows.
>Commit b3bf5666a510 ("hv_netvsc: defer queue selection to VF")
>fixed this performance problem.
>
>Lastly, more indirection is bad in current climate.
>
>I am not completely adverse to this but it needs to be fast, simple
>and completely transparent.
^ permalink raw reply
* Re: [RFC PATCH net-next v5 3/4] virtio_net: Extend virtio to use VF datapath when available
From: Jiri Pirko @ 2018-04-11 8:03 UTC (permalink / raw)
To: Samudrala, Sridhar
Cc: alexander.h.duyck, virtio-dev, mst, kubakici, netdev,
virtualization, loseweigh, davem
In-Reply-To: <5d56e572-24f1-745d-49ae-c2dada5db03c@intel.com>
Wed, Apr 11, 2018 at 08:24:43AM CEST, sridhar.samudrala@intel.com wrote:
>On 4/10/2018 11:03 PM, Jiri Pirko wrote:
>> Tue, Apr 10, 2018 at 05:59:02PM CEST, sridhar.samudrala@intel.com wrote:
>> > On 4/10/2018 8:43 AM, Jiri Pirko wrote:
>> > > Tue, Apr 10, 2018 at 05:27:48PM CEST, sridhar.samudrala@intel.com wrote:
>> > > > On 4/10/2018 8:22 AM, Jiri Pirko wrote:
>> > > > > Tue, Apr 10, 2018 at 05:13:40PM CEST, sridhar.samudrala@intel.com wrote:
>> > > > > > On 4/10/2018 3:55 AM, Jiri Pirko wrote:
>> > > > > > > Mon, Apr 09, 2018 at 08:47:06PM CEST, sridhar.samudrala@intel.com wrote:
>> > > > > > > > On 4/9/2018 1:07 AM, Jiri Pirko wrote:
>> > > > > > > > > Sat, Apr 07, 2018 at 12:59:14AM CEST, sridhar.samudrala@intel.com wrote:
>> > > > > > > > > > On 4/6/2018 5:48 AM, Jiri Pirko wrote:
>> > > > > > > > > > > Thu, Apr 05, 2018 at 11:08:22PM CEST, sridhar.samudrala@intel.com wrote:
>> > > > > > > > > [...]
>> > > > > > > > >
>> > > > > > > > > > > > +static int virtnet_bypass_join_child(struct net_device *bypass_netdev,
>> > > > > > > > > > > > + struct net_device *child_netdev)
>> > > > > > > > > > > > +{
>> > > > > > > > > > > > + struct virtnet_bypass_info *vbi;
>> > > > > > > > > > > > + bool backup;
>> > > > > > > > > > > > +
>> > > > > > > > > > > > + vbi = netdev_priv(bypass_netdev);
>> > > > > > > > > > > > + backup = (child_netdev->dev.parent == bypass_netdev->dev.parent);
>> > > > > > > > > > > > + if (backup ? rtnl_dereference(vbi->backup_netdev) :
>> > > > > > > > > > > > + rtnl_dereference(vbi->active_netdev)) {
>> > > > > > > > > > > > + netdev_info(bypass_netdev,
>> > > > > > > > > > > > + "%s attempting to join bypass dev when %s already present\n",
>> > > > > > > > > > > > + child_netdev->name, backup ? "backup" : "active");
>> > > > > > > > > > > Bypass module should check if there is already some other netdev
>> > > > > > > > > > > enslaved and refuse right there.
>> > > > > > > > > > This will work for virtio-net with 3 netdev model, but this check has to be done by netvsc
>> > > > > > > > > > as its bypass_netdev is same as the backup_netdev.
>> > > > > > > > > > Will add a flag while registering with the bypass module to indicate if the driver is doing
>> > > > > > > > > > a 2 netdev or 3 netdev model and based on that flag this check can be done in bypass module
>> > > > > > > > > > for 3 netdev scenario.
>> > > > > > > > > Just let me undestand it clearly. What I expect the difference would be
>> > > > > > > > > between 2netdev and3 netdev model is this:
>> > > > > > > > > 2netdev:
>> > > > > > > > > bypass_master
>> > > > > > > > > /
>> > > > > > > > > /
>> > > > > > > > > VF_slave
>> > > > > > > > >
>> > > > > > > > > 3netdev:
>> > > > > > > > > bypass_master
>> > > > > > > > > / \
>> > > > > > > > > / \
>> > > > > > > > > VF_slave backup_slave
>> > > > > > > > >
>> > > > > > > > > Is that correct? If not, how does it look like?
>> > > > > > > > >
>> > > > > > > > >
>> > > > > > > > Looks correct.
>> > > > > > > > VF_slave and backup_slave are the original netdevs and are present in both the models.
>> > > > > > > > In the 3 netdev model, bypass_master netdev is created and VF_slave and backup_slave are
>> > > > > > > > marked as the 2 slaves of this new netdev.
>> > > > > > > You say it looks correct and in another sentence you provide completely
>> > > > > > > different description. Could you please look again?
>> > > > > > >
>> > > > > > To be exact, 2 netdev model with netvsc looks like this.
>> > > > > >
>> > > > > > netvsc_netdev
>> > > > > > /
>> > > > > > /
>> > > > > > VF_slave
>> > > > > >
>> > > > > > With virtio_net, 3 netdev model
>> > > > > >
>> > > > > > bypass_netdev
>> > > > > > / \
>> > > > > > / \
>> > > > > > VF_slave virtio_net netdev
>> > > > > Could you also mark the original netdev which is there now? is it
>> > > > > bypass_netdev or virtio_net_netdev ?
>> > > > bypass_netdev
>> > > > / \
>> > > > / \
>> > > > VF_slave virtio_net netdev (original)
>> > > That does not make sense.
>> > > 1) You diverge from the behaviour of the netvsc, where the original
>> > > netdev is a master of the VF
>> > > 2) If the original netdev is a slave, you cannot have any IP address
>> > > configured on it (well you could, but the rx_handler would eat every
>> > > incoming packet). So you will break the user bacause he would have to
>> > > move the configuration to the new master device.
>> > > This only makes sense that the original netdev becomes the master for both
>> > > netvsc and virtio_net.
>> > Forgot to mention that bypass_netdev takes over the name of the original
>> > netdev and
>> > virtio_net netdev will get the backup name.
>> What do you mean by "name"?
>
>bypass_netdev also is associated with the same pci device as the original virtio_net
>netdev via SET_NETDEV_DEV(). Also, we added ndo_get_phys_port_name() to virtio_net
>that will return _bkup when BACKUP feature is enabled.
Okay.
>
>So for ex: if virtio_net inteface was getting 'ens12' as the name assigned by udev
>without BACKUP feature, when BACKUP feature is enabled, the bypass_netdev will be
>named 'ens12' and the original virtio_net will get named as ens12n_bkup.
Got it.
I don't like the bypass_master to look differently in netvsc and
virtio_net :/ The best would be to convert netvsc to 3 netdev model and
treat them the same. The more I think about it, the more the 2 netdev
model feels wrong.
>
>
>>
>> > So the userspace network configuration doesn't need to change.
>> >
>> >
>
^ permalink raw reply
* Re: [PATCH v3 1/2] vhost: fix vhost_vq_access_ok() log check
From: Michael S. Tsirkin @ 2018-04-11 13:23 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: kvm, netdev, syzkaller-bugs, linux-kernel, virtualization,
Linus Torvalds
In-Reply-To: <20180411023541.15776-2-stefanha@redhat.com>
On Wed, Apr 11, 2018 at 10:35:40AM +0800, Stefan Hajnoczi wrote:
> Commit d65026c6c62e7d9616c8ceb5a53b68bcdc050525 ("vhost: validate log
> when IOTLB is enabled") introduced a regression. The logic was
> originally:
>
> if (vq->iotlb)
> return 1;
> return A && B;
>
> After the patch the short-circuit logic for A was inverted:
>
> if (A || vq->iotlb)
> return A;
> return B;
>
> This patch fixes the regression by rewriting the checks in the obvious
> way, no longer returning A when vq->iotlb is non-NULL (which is hard to
> understand).
>
> Reported-by: syzbot+65a84dde0214b0387ccd@syzkaller.appspotmail.com
> Cc: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/vhost/vhost.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index bec722e41f58..fc805b7fad9d 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -1244,10 +1244,12 @@ static int vq_log_access_ok(struct vhost_virtqueue *vq,
> /* Caller should have vq mutex and device mutex */
> int vhost_vq_access_ok(struct vhost_virtqueue *vq)
> {
> - int ret = vq_log_access_ok(vq, vq->log_base);
> + if (!vq_log_access_ok(vq, vq->log_base))
> + return 0;
>
> - if (ret || vq->iotlb)
> - return ret;
> + /* Access validation occurs at prefetch time with IOTLB */
> + if (vq->iotlb)
> + return 1;
>
> return vq_access_ok(vq, vq->num, vq->desc, vq->avail, vq->used);
> }
> --
> 2.14.3
^ permalink raw reply
* Re: [PATCH v3 2/2] vhost: return bool from *_access_ok() functions
From: Michael S. Tsirkin @ 2018-04-11 13:23 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: kvm, netdev, syzkaller-bugs, linux-kernel, virtualization,
Linus Torvalds
In-Reply-To: <20180411023541.15776-3-stefanha@redhat.com>
On Wed, Apr 11, 2018 at 10:35:41AM +0800, Stefan Hajnoczi wrote:
> Currently vhost *_access_ok() functions return int. This is error-prone
> because there are two popular conventions:
>
> 1. 0 means failure, 1 means success
> 2. -errno means failure, 0 means success
>
> Although vhost mostly uses #1, it does not do so consistently.
> umem_access_ok() uses #2.
>
> This patch changes the return type from int to bool so that false means
> failure and true means success. This eliminates a potential source of
> errors.
>
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/vhost/vhost.h | 4 ++--
> drivers/vhost/vhost.c | 66 +++++++++++++++++++++++++--------------------------
> 2 files changed, 35 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index d8ee85ae8fdc..6c844b90a168 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -178,8 +178,8 @@ void vhost_dev_cleanup(struct vhost_dev *);
> void vhost_dev_stop(struct vhost_dev *);
> long vhost_dev_ioctl(struct vhost_dev *, unsigned int ioctl, void __user *argp);
> long vhost_vring_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp);
> -int vhost_vq_access_ok(struct vhost_virtqueue *vq);
> -int vhost_log_access_ok(struct vhost_dev *);
> +bool vhost_vq_access_ok(struct vhost_virtqueue *vq);
> +bool vhost_log_access_ok(struct vhost_dev *);
>
> int vhost_get_vq_desc(struct vhost_virtqueue *,
> struct iovec iov[], unsigned int iov_count,
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index fc805b7fad9d..0fcb51a9940c 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -641,14 +641,14 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
> }
> EXPORT_SYMBOL_GPL(vhost_dev_cleanup);
>
> -static int log_access_ok(void __user *log_base, u64 addr, unsigned long sz)
> +static bool log_access_ok(void __user *log_base, u64 addr, unsigned long sz)
> {
> u64 a = addr / VHOST_PAGE_SIZE / 8;
>
> /* Make sure 64 bit math will not overflow. */
> if (a > ULONG_MAX - (unsigned long)log_base ||
> a + (unsigned long)log_base > ULONG_MAX)
> - return 0;
> + return false;
>
> return access_ok(VERIFY_WRITE, log_base + a,
> (sz + VHOST_PAGE_SIZE * 8 - 1) / VHOST_PAGE_SIZE / 8);
> @@ -661,30 +661,30 @@ static bool vhost_overflow(u64 uaddr, u64 size)
> }
>
> /* Caller should have vq mutex and device mutex. */
> -static int vq_memory_access_ok(void __user *log_base, struct vhost_umem *umem,
> - int log_all)
> +static bool vq_memory_access_ok(void __user *log_base, struct vhost_umem *umem,
> + int log_all)
> {
> struct vhost_umem_node *node;
>
> if (!umem)
> - return 0;
> + return false;
>
> list_for_each_entry(node, &umem->umem_list, link) {
> unsigned long a = node->userspace_addr;
>
> if (vhost_overflow(node->userspace_addr, node->size))
> - return 0;
> + return false;
>
>
> if (!access_ok(VERIFY_WRITE, (void __user *)a,
> node->size))
> - return 0;
> + return false;
> else if (log_all && !log_access_ok(log_base,
> node->start,
> node->size))
> - return 0;
> + return false;
> }
> - return 1;
> + return true;
> }
>
> static inline void __user *vhost_vq_meta_fetch(struct vhost_virtqueue *vq,
> @@ -701,13 +701,13 @@ static inline void __user *vhost_vq_meta_fetch(struct vhost_virtqueue *vq,
>
> /* Can we switch to this memory table? */
> /* Caller should have device mutex but not vq mutex */
> -static int memory_access_ok(struct vhost_dev *d, struct vhost_umem *umem,
> - int log_all)
> +static bool memory_access_ok(struct vhost_dev *d, struct vhost_umem *umem,
> + int log_all)
> {
> int i;
>
> for (i = 0; i < d->nvqs; ++i) {
> - int ok;
> + bool ok;
> bool log;
>
> mutex_lock(&d->vqs[i]->mutex);
> @@ -717,12 +717,12 @@ static int memory_access_ok(struct vhost_dev *d, struct vhost_umem *umem,
> ok = vq_memory_access_ok(d->vqs[i]->log_base,
> umem, log);
> else
> - ok = 1;
> + ok = true;
> mutex_unlock(&d->vqs[i]->mutex);
> if (!ok)
> - return 0;
> + return false;
> }
> - return 1;
> + return true;
> }
>
> static int translate_desc(struct vhost_virtqueue *vq, u64 addr, u32 len,
> @@ -959,21 +959,21 @@ static void vhost_iotlb_notify_vq(struct vhost_dev *d,
> spin_unlock(&d->iotlb_lock);
> }
>
> -static int umem_access_ok(u64 uaddr, u64 size, int access)
> +static bool umem_access_ok(u64 uaddr, u64 size, int access)
> {
> unsigned long a = uaddr;
>
> /* Make sure 64 bit math will not overflow. */
> if (vhost_overflow(uaddr, size))
> - return -EFAULT;
> + return false;
>
> if ((access & VHOST_ACCESS_RO) &&
> !access_ok(VERIFY_READ, (void __user *)a, size))
> - return -EFAULT;
> + return false;
> if ((access & VHOST_ACCESS_WO) &&
> !access_ok(VERIFY_WRITE, (void __user *)a, size))
> - return -EFAULT;
> - return 0;
> + return false;
> + return true;
> }
>
> static int vhost_process_iotlb_msg(struct vhost_dev *dev,
> @@ -988,7 +988,7 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev,
> ret = -EFAULT;
> break;
> }
> - if (umem_access_ok(msg->uaddr, msg->size, msg->perm)) {
> + if (!umem_access_ok(msg->uaddr, msg->size, msg->perm)) {
> ret = -EFAULT;
> break;
> }
> @@ -1135,10 +1135,10 @@ static int vhost_iotlb_miss(struct vhost_virtqueue *vq, u64 iova, int access)
> return 0;
> }
>
> -static int vq_access_ok(struct vhost_virtqueue *vq, unsigned int num,
> - struct vring_desc __user *desc,
> - struct vring_avail __user *avail,
> - struct vring_used __user *used)
> +static bool vq_access_ok(struct vhost_virtqueue *vq, unsigned int num,
> + struct vring_desc __user *desc,
> + struct vring_avail __user *avail,
> + struct vring_used __user *used)
>
> {
> size_t s = vhost_has_feature(vq, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
> @@ -1161,8 +1161,8 @@ static void vhost_vq_meta_update(struct vhost_virtqueue *vq,
> vq->meta_iotlb[type] = node;
> }
>
> -static int iotlb_access_ok(struct vhost_virtqueue *vq,
> - int access, u64 addr, u64 len, int type)
> +static bool iotlb_access_ok(struct vhost_virtqueue *vq,
> + int access, u64 addr, u64 len, int type)
> {
> const struct vhost_umem_node *node;
> struct vhost_umem *umem = vq->iotlb;
> @@ -1220,7 +1220,7 @@ EXPORT_SYMBOL_GPL(vq_iotlb_prefetch);
>
> /* Can we log writes? */
> /* Caller should have device mutex but not vq mutex */
> -int vhost_log_access_ok(struct vhost_dev *dev)
> +bool vhost_log_access_ok(struct vhost_dev *dev)
> {
> return memory_access_ok(dev, dev->umem, 1);
> }
> @@ -1228,8 +1228,8 @@ EXPORT_SYMBOL_GPL(vhost_log_access_ok);
>
> /* Verify access for write logging. */
> /* Caller should have vq mutex and device mutex */
> -static int vq_log_access_ok(struct vhost_virtqueue *vq,
> - void __user *log_base)
> +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;
>
> @@ -1242,14 +1242,14 @@ static int vq_log_access_ok(struct vhost_virtqueue *vq,
>
> /* Can we start vq? */
> /* Caller should have vq mutex and device mutex */
> -int vhost_vq_access_ok(struct vhost_virtqueue *vq)
> +bool vhost_vq_access_ok(struct vhost_virtqueue *vq)
> {
> if (!vq_log_access_ok(vq, vq->log_base))
> - return 0;
> + return false;
>
> /* Access validation occurs at prefetch time with IOTLB */
> if (vq->iotlb)
> - return 1;
> + return true;
>
> return vq_access_ok(vq, vq->num, vq->desc, vq->avail, vq->used);
> }
> --
> 2.14.3
^ permalink raw reply
* Re: [PATCH v3 0/2] vhost: fix vhost_vq_access_ok() log check
From: Michael S. Tsirkin @ 2018-04-11 13:24 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: kvm, netdev, syzkaller-bugs, linux-kernel, virtualization,
Linus Torvalds
In-Reply-To: <20180411023541.15776-1-stefanha@redhat.com>
On Wed, Apr 11, 2018 at 10:35:39AM +0800, Stefan Hajnoczi wrote:
> v3:
> * Rebased onto net/master and resolved conflict [DaveM]
>
> v2:
> * Rewrote the conditional to make the vq access check clearer [Linus]
> * Added Patch 2 to make the return type consistent and harder to misuse [Linus]
>
> The first patch fixes the vhost virtqueue access check which was recently
> broken. The second patch replaces the int return type with bool to prevent
> future bugs.
Acked-by: Michael S. Tsirkin <mst@redhat.com>
We need the 1st one on stable I think.
> Stefan Hajnoczi (2):
> vhost: fix vhost_vq_access_ok() log check
> vhost: return bool from *_access_ok() functions
>
> drivers/vhost/vhost.h | 4 +--
> drivers/vhost/vhost.c | 70 ++++++++++++++++++++++++++-------------------------
> 2 files changed, 38 insertions(+), 36 deletions(-)
>
> --
> 2.14.3
^ permalink raw reply
* [PATCH] vhost: Fix vhost_copy_to_user()
From: Eric Auger @ 2018-04-11 13:30 UTC (permalink / raw)
To: eric.auger.pro, eric.auger, linux-kernel, virtualization, netdev,
kvm, jasowang, mst, kvmarm
Cc: stefanha
vhost_copy_to_user is used to copy vring used elements to userspace.
We should use VHOST_ADDR_USED instead of VHOST_ADDR_DESC.
Fixes: f88949138058 ("vhost: introduce O(1) vq metadata cache")
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
This fixes a stall observed when running an aarch64 guest with
virtual smmu
---
drivers/vhost/vhost.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index bec722e..f44aead 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -744,7 +744,7 @@ static int vhost_copy_to_user(struct vhost_virtqueue *vq, void __user *to,
struct iov_iter t;
void __user *uaddr = vhost_vq_meta_fetch(vq,
(u64)(uintptr_t)to, size,
- VHOST_ADDR_DESC);
+ VHOST_ADDR_USED);
if (uaddr)
return __copy_to_user(uaddr, from, size);
--
2.5.5
^ permalink raw reply related
* Re: [PATCH] vhost: Fix vhost_copy_to_user()
From: Jason Wang @ 2018-04-11 13:44 UTC (permalink / raw)
To: Eric Auger, eric.auger.pro, linux-kernel, virtualization, netdev,
kvm, mst, kvmarm
Cc: stefanha
In-Reply-To: <1523453438-4266-1-git-send-email-eric.auger@redhat.com>
On 2018年04月11日 21:30, Eric Auger wrote:
> vhost_copy_to_user is used to copy vring used elements to userspace.
> We should use VHOST_ADDR_USED instead of VHOST_ADDR_DESC.
>
> Fixes: f88949138058 ("vhost: introduce O(1) vq metadata cache")
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>
> ---
>
> This fixes a stall observed when running an aarch64 guest with
> virtual smmu
> ---
> drivers/vhost/vhost.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index bec722e..f44aead 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -744,7 +744,7 @@ static int vhost_copy_to_user(struct vhost_virtqueue *vq, void __user *to,
> struct iov_iter t;
> void __user *uaddr = vhost_vq_meta_fetch(vq,
> (u64)(uintptr_t)to, size,
> - VHOST_ADDR_DESC);
> + VHOST_ADDR_USED);
>
> if (uaddr)
> return __copy_to_user(uaddr, from, size);
Acked-by: Jason Wang <jasowang@redhat.com>
Thanks!
Stable material I think.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH] vhost: Fix vhost_copy_to_user()
From: Auger Eric @ 2018-04-11 13:45 UTC (permalink / raw)
To: Jason Wang, eric.auger.pro, linux-kernel, virtualization, netdev,
kvm, mst, kvmarm
Cc: stefanha
In-Reply-To: <85c033b9-b230-7ef9-744c-4e2799684609@redhat.com>
Hi Jason,
On 11/04/18 15:44, Jason Wang wrote:
>
>
> On 2018年04月11日 21:30, Eric Auger wrote:
>> vhost_copy_to_user is used to copy vring used elements to userspace.
>> We should use VHOST_ADDR_USED instead of VHOST_ADDR_DESC.
>>
>> Fixes: f88949138058 ("vhost: introduce O(1) vq metadata cache")
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> This fixes a stall observed when running an aarch64 guest with
>> virtual smmu
>> ---
>> drivers/vhost/vhost.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>> index bec722e..f44aead 100644
>> --- a/drivers/vhost/vhost.c
>> +++ b/drivers/vhost/vhost.c
>> @@ -744,7 +744,7 @@ static int vhost_copy_to_user(struct
>> vhost_virtqueue *vq, void __user *to,
>> struct iov_iter t;
>> void __user *uaddr = vhost_vq_meta_fetch(vq,
>> (u64)(uintptr_t)to, size,
>> - VHOST_ADDR_DESC);
>> + VHOST_ADDR_USED);
>> if (uaddr)
>> return __copy_to_user(uaddr, from, size);
>
> Acked-by: Jason Wang <jasowang@redhat.com>
>
> Thanks!
>
> Stable material I think.
yes I think so.
Thanks
Eric
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH] vhost: Fix vhost_copy_to_user()
From: Michael S. Tsirkin @ 2018-04-11 13:51 UTC (permalink / raw)
To: Eric Auger
Cc: kvm, netdev, linux-kernel, virtualization, stefanha, kvmarm,
eric.auger.pro
In-Reply-To: <1523453438-4266-1-git-send-email-eric.auger@redhat.com>
On Wed, Apr 11, 2018 at 03:30:38PM +0200, Eric Auger wrote:
> vhost_copy_to_user is used to copy vring used elements to userspace.
> We should use VHOST_ADDR_USED instead of VHOST_ADDR_DESC.
>
> Fixes: f88949138058 ("vhost: introduce O(1) vq metadata cache")
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> This fixes a stall observed when running an aarch64 guest with
> virtual smmu
> ---
> drivers/vhost/vhost.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index bec722e..f44aead 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -744,7 +744,7 @@ static int vhost_copy_to_user(struct vhost_virtqueue *vq, void __user *to,
> struct iov_iter t;
> void __user *uaddr = vhost_vq_meta_fetch(vq,
> (u64)(uintptr_t)to, size,
> - VHOST_ADDR_DESC);
> + VHOST_ADDR_USED);
>
> if (uaddr)
> return __copy_to_user(uaddr, from, size);
> --
> 2.5.5
^ permalink raw reply
* Re: [RFC PATCH net-next v5 3/4] virtio_net: Extend virtio to use VF datapath when available
From: Michael S. Tsirkin @ 2018-04-11 14:45 UTC (permalink / raw)
To: Jiri Pirko
Cc: alexander.h.duyck, virtio-dev, kubakici, Samudrala, Sridhar,
virtualization, loseweigh, netdev, davem
In-Reply-To: <20180411080332.GL2028@nanopsycho>
On Wed, Apr 11, 2018 at 10:03:32AM +0200, Jiri Pirko wrote:
> Wed, Apr 11, 2018 at 08:24:43AM CEST, sridhar.samudrala@intel.com wrote:
> >On 4/10/2018 11:03 PM, Jiri Pirko wrote:
> >> Tue, Apr 10, 2018 at 05:59:02PM CEST, sridhar.samudrala@intel.com wrote:
> >> > On 4/10/2018 8:43 AM, Jiri Pirko wrote:
> >> > > Tue, Apr 10, 2018 at 05:27:48PM CEST, sridhar.samudrala@intel.com wrote:
> >> > > > On 4/10/2018 8:22 AM, Jiri Pirko wrote:
> >> > > > > Tue, Apr 10, 2018 at 05:13:40PM CEST, sridhar.samudrala@intel.com wrote:
> >> > > > > > On 4/10/2018 3:55 AM, Jiri Pirko wrote:
> >> > > > > > > Mon, Apr 09, 2018 at 08:47:06PM CEST, sridhar.samudrala@intel.com wrote:
> >> > > > > > > > On 4/9/2018 1:07 AM, Jiri Pirko wrote:
> >> > > > > > > > > Sat, Apr 07, 2018 at 12:59:14AM CEST, sridhar.samudrala@intel.com wrote:
> >> > > > > > > > > > On 4/6/2018 5:48 AM, Jiri Pirko wrote:
> >> > > > > > > > > > > Thu, Apr 05, 2018 at 11:08:22PM CEST, sridhar.samudrala@intel.com wrote:
> >> > > > > > > > > [...]
> >> > > > > > > > >
> >> > > > > > > > > > > > +static int virtnet_bypass_join_child(struct net_device *bypass_netdev,
> >> > > > > > > > > > > > + struct net_device *child_netdev)
> >> > > > > > > > > > > > +{
> >> > > > > > > > > > > > + struct virtnet_bypass_info *vbi;
> >> > > > > > > > > > > > + bool backup;
> >> > > > > > > > > > > > +
> >> > > > > > > > > > > > + vbi = netdev_priv(bypass_netdev);
> >> > > > > > > > > > > > + backup = (child_netdev->dev.parent == bypass_netdev->dev.parent);
> >> > > > > > > > > > > > + if (backup ? rtnl_dereference(vbi->backup_netdev) :
> >> > > > > > > > > > > > + rtnl_dereference(vbi->active_netdev)) {
> >> > > > > > > > > > > > + netdev_info(bypass_netdev,
> >> > > > > > > > > > > > + "%s attempting to join bypass dev when %s already present\n",
> >> > > > > > > > > > > > + child_netdev->name, backup ? "backup" : "active");
> >> > > > > > > > > > > Bypass module should check if there is already some other netdev
> >> > > > > > > > > > > enslaved and refuse right there.
> >> > > > > > > > > > This will work for virtio-net with 3 netdev model, but this check has to be done by netvsc
> >> > > > > > > > > > as its bypass_netdev is same as the backup_netdev.
> >> > > > > > > > > > Will add a flag while registering with the bypass module to indicate if the driver is doing
> >> > > > > > > > > > a 2 netdev or 3 netdev model and based on that flag this check can be done in bypass module
> >> > > > > > > > > > for 3 netdev scenario.
> >> > > > > > > > > Just let me undestand it clearly. What I expect the difference would be
> >> > > > > > > > > between 2netdev and3 netdev model is this:
> >> > > > > > > > > 2netdev:
> >> > > > > > > > > bypass_master
> >> > > > > > > > > /
> >> > > > > > > > > /
> >> > > > > > > > > VF_slave
> >> > > > > > > > >
> >> > > > > > > > > 3netdev:
> >> > > > > > > > > bypass_master
> >> > > > > > > > > / \
> >> > > > > > > > > / \
> >> > > > > > > > > VF_slave backup_slave
> >> > > > > > > > >
> >> > > > > > > > > Is that correct? If not, how does it look like?
> >> > > > > > > > >
> >> > > > > > > > >
> >> > > > > > > > Looks correct.
> >> > > > > > > > VF_slave and backup_slave are the original netdevs and are present in both the models.
> >> > > > > > > > In the 3 netdev model, bypass_master netdev is created and VF_slave and backup_slave are
> >> > > > > > > > marked as the 2 slaves of this new netdev.
> >> > > > > > > You say it looks correct and in another sentence you provide completely
> >> > > > > > > different description. Could you please look again?
> >> > > > > > >
> >> > > > > > To be exact, 2 netdev model with netvsc looks like this.
> >> > > > > >
> >> > > > > > netvsc_netdev
> >> > > > > > /
> >> > > > > > /
> >> > > > > > VF_slave
> >> > > > > >
> >> > > > > > With virtio_net, 3 netdev model
> >> > > > > >
> >> > > > > > bypass_netdev
> >> > > > > > / \
> >> > > > > > / \
> >> > > > > > VF_slave virtio_net netdev
> >> > > > > Could you also mark the original netdev which is there now? is it
> >> > > > > bypass_netdev or virtio_net_netdev ?
> >> > > > bypass_netdev
> >> > > > / \
> >> > > > / \
> >> > > > VF_slave virtio_net netdev (original)
> >> > > That does not make sense.
> >> > > 1) You diverge from the behaviour of the netvsc, where the original
> >> > > netdev is a master of the VF
> >> > > 2) If the original netdev is a slave, you cannot have any IP address
> >> > > configured on it (well you could, but the rx_handler would eat every
> >> > > incoming packet). So you will break the user bacause he would have to
> >> > > move the configuration to the new master device.
> >> > > This only makes sense that the original netdev becomes the master for both
> >> > > netvsc and virtio_net.
> >> > Forgot to mention that bypass_netdev takes over the name of the original
> >> > netdev and
> >> > virtio_net netdev will get the backup name.
> >> What do you mean by "name"?
> >
> >bypass_netdev also is associated with the same pci device as the original virtio_net
> >netdev via SET_NETDEV_DEV(). Also, we added ndo_get_phys_port_name() to virtio_net
> >that will return _bkup when BACKUP feature is enabled.
>
> Okay.
>
> >
> >So for ex: if virtio_net inteface was getting 'ens12' as the name assigned by udev
> >without BACKUP feature, when BACKUP feature is enabled, the bypass_netdev will be
> >named 'ens12' and the original virtio_net will get named as ens12n_bkup.
>
> Got it.
>
> I don't like the bypass_master to look differently in netvsc and
> virtio_net :/ The best would be to convert netvsc to 3 netdev model and
> treat them the same. The more I think about it, the more the 2 netdev
> model feels wrong.
If you believe that, then this patchset is a step in the right
direction.
With something like this patchset applied, converting netvsc to a 3
device model will presumably be just a flag flip away. Afterwards
we'll be able to drop dead code handling the bypass_master flag.
>
> >
> >
> >>
> >> > So the userspace network configuration doesn't need to change.
> >> >
> >> >
> >
^ permalink raw reply
* Re: [RFC PATCH net-next v5 3/4] virtio_net: Extend virtio to use VF datapath when available
From: Jiri Pirko @ 2018-04-11 14:51 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: alexander.h.duyck, virtio-dev, kubakici, Samudrala, Sridhar,
virtualization, loseweigh, netdev, davem
In-Reply-To: <20180411174157-mutt-send-email-mst@kernel.org>
Wed, Apr 11, 2018 at 04:45:07PM CEST, mst@redhat.com wrote:
>On Wed, Apr 11, 2018 at 10:03:32AM +0200, Jiri Pirko wrote:
>> Wed, Apr 11, 2018 at 08:24:43AM CEST, sridhar.samudrala@intel.com wrote:
>> >On 4/10/2018 11:03 PM, Jiri Pirko wrote:
>> >> Tue, Apr 10, 2018 at 05:59:02PM CEST, sridhar.samudrala@intel.com wrote:
>> >> > On 4/10/2018 8:43 AM, Jiri Pirko wrote:
>> >> > > Tue, Apr 10, 2018 at 05:27:48PM CEST, sridhar.samudrala@intel.com wrote:
>> >> > > > On 4/10/2018 8:22 AM, Jiri Pirko wrote:
>> >> > > > > Tue, Apr 10, 2018 at 05:13:40PM CEST, sridhar.samudrala@intel.com wrote:
>> >> > > > > > On 4/10/2018 3:55 AM, Jiri Pirko wrote:
>> >> > > > > > > Mon, Apr 09, 2018 at 08:47:06PM CEST, sridhar.samudrala@intel.com wrote:
>> >> > > > > > > > On 4/9/2018 1:07 AM, Jiri Pirko wrote:
>> >> > > > > > > > > Sat, Apr 07, 2018 at 12:59:14AM CEST, sridhar.samudrala@intel.com wrote:
>> >> > > > > > > > > > On 4/6/2018 5:48 AM, Jiri Pirko wrote:
>> >> > > > > > > > > > > Thu, Apr 05, 2018 at 11:08:22PM CEST, sridhar.samudrala@intel.com wrote:
>> >> > > > > > > > > [...]
>> >> > > > > > > > >
>> >> > > > > > > > > > > > +static int virtnet_bypass_join_child(struct net_device *bypass_netdev,
>> >> > > > > > > > > > > > + struct net_device *child_netdev)
>> >> > > > > > > > > > > > +{
>> >> > > > > > > > > > > > + struct virtnet_bypass_info *vbi;
>> >> > > > > > > > > > > > + bool backup;
>> >> > > > > > > > > > > > +
>> >> > > > > > > > > > > > + vbi = netdev_priv(bypass_netdev);
>> >> > > > > > > > > > > > + backup = (child_netdev->dev.parent == bypass_netdev->dev.parent);
>> >> > > > > > > > > > > > + if (backup ? rtnl_dereference(vbi->backup_netdev) :
>> >> > > > > > > > > > > > + rtnl_dereference(vbi->active_netdev)) {
>> >> > > > > > > > > > > > + netdev_info(bypass_netdev,
>> >> > > > > > > > > > > > + "%s attempting to join bypass dev when %s already present\n",
>> >> > > > > > > > > > > > + child_netdev->name, backup ? "backup" : "active");
>> >> > > > > > > > > > > Bypass module should check if there is already some other netdev
>> >> > > > > > > > > > > enslaved and refuse right there.
>> >> > > > > > > > > > This will work for virtio-net with 3 netdev model, but this check has to be done by netvsc
>> >> > > > > > > > > > as its bypass_netdev is same as the backup_netdev.
>> >> > > > > > > > > > Will add a flag while registering with the bypass module to indicate if the driver is doing
>> >> > > > > > > > > > a 2 netdev or 3 netdev model and based on that flag this check can be done in bypass module
>> >> > > > > > > > > > for 3 netdev scenario.
>> >> > > > > > > > > Just let me undestand it clearly. What I expect the difference would be
>> >> > > > > > > > > between 2netdev and3 netdev model is this:
>> >> > > > > > > > > 2netdev:
>> >> > > > > > > > > bypass_master
>> >> > > > > > > > > /
>> >> > > > > > > > > /
>> >> > > > > > > > > VF_slave
>> >> > > > > > > > >
>> >> > > > > > > > > 3netdev:
>> >> > > > > > > > > bypass_master
>> >> > > > > > > > > / \
>> >> > > > > > > > > / \
>> >> > > > > > > > > VF_slave backup_slave
>> >> > > > > > > > >
>> >> > > > > > > > > Is that correct? If not, how does it look like?
>> >> > > > > > > > >
>> >> > > > > > > > >
>> >> > > > > > > > Looks correct.
>> >> > > > > > > > VF_slave and backup_slave are the original netdevs and are present in both the models.
>> >> > > > > > > > In the 3 netdev model, bypass_master netdev is created and VF_slave and backup_slave are
>> >> > > > > > > > marked as the 2 slaves of this new netdev.
>> >> > > > > > > You say it looks correct and in another sentence you provide completely
>> >> > > > > > > different description. Could you please look again?
>> >> > > > > > >
>> >> > > > > > To be exact, 2 netdev model with netvsc looks like this.
>> >> > > > > >
>> >> > > > > > netvsc_netdev
>> >> > > > > > /
>> >> > > > > > /
>> >> > > > > > VF_slave
>> >> > > > > >
>> >> > > > > > With virtio_net, 3 netdev model
>> >> > > > > >
>> >> > > > > > bypass_netdev
>> >> > > > > > / \
>> >> > > > > > / \
>> >> > > > > > VF_slave virtio_net netdev
>> >> > > > > Could you also mark the original netdev which is there now? is it
>> >> > > > > bypass_netdev or virtio_net_netdev ?
>> >> > > > bypass_netdev
>> >> > > > / \
>> >> > > > / \
>> >> > > > VF_slave virtio_net netdev (original)
>> >> > > That does not make sense.
>> >> > > 1) You diverge from the behaviour of the netvsc, where the original
>> >> > > netdev is a master of the VF
>> >> > > 2) If the original netdev is a slave, you cannot have any IP address
>> >> > > configured on it (well you could, but the rx_handler would eat every
>> >> > > incoming packet). So you will break the user bacause he would have to
>> >> > > move the configuration to the new master device.
>> >> > > This only makes sense that the original netdev becomes the master for both
>> >> > > netvsc and virtio_net.
>> >> > Forgot to mention that bypass_netdev takes over the name of the original
>> >> > netdev and
>> >> > virtio_net netdev will get the backup name.
>> >> What do you mean by "name"?
>> >
>> >bypass_netdev also is associated with the same pci device as the original virtio_net
>> >netdev via SET_NETDEV_DEV(). Also, we added ndo_get_phys_port_name() to virtio_net
>> >that will return _bkup when BACKUP feature is enabled.
>>
>> Okay.
>>
>> >
>> >So for ex: if virtio_net inteface was getting 'ens12' as the name assigned by udev
>> >without BACKUP feature, when BACKUP feature is enabled, the bypass_netdev will be
>> >named 'ens12' and the original virtio_net will get named as ens12n_bkup.
>>
>> Got it.
>>
>> I don't like the bypass_master to look differently in netvsc and
>> virtio_net :/ The best would be to convert netvsc to 3 netdev model and
>> treat them the same. The more I think about it, the more the 2 netdev
>> model feels wrong.
>
>If you believe that, then this patchset is a step in the right
>direction.
>
>With something like this patchset applied, converting netvsc to a 3
>device model will presumably be just a flag flip away. Afterwards
If done properly. Yes.
>we'll be able to drop dead code handling the bypass_master flag.
>
>>
>> >
>> >
>> >>
>> >> > So the userspace network configuration doesn't need to change.
>> >> >
>> >> >
>> >
^ permalink raw reply
* Re: [PATCH v3 0/2] vhost: fix vhost_vq_access_ok() log check
From: David Miller @ 2018-04-11 14:55 UTC (permalink / raw)
To: mst
Cc: kvm, netdev, syzkaller-bugs, linux-kernel, virtualization,
stefanha, torvalds
In-Reply-To: <20180411162345-mutt-send-email-mst@kernel.org>
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Wed, 11 Apr 2018 16:24:02 +0300
> On Wed, Apr 11, 2018 at 10:35:39AM +0800, Stefan Hajnoczi wrote:
>> v3:
>> * Rebased onto net/master and resolved conflict [DaveM]
>>
>> v2:
>> * Rewrote the conditional to make the vq access check clearer [Linus]
>> * Added Patch 2 to make the return type consistent and harder to misuse [Linus]
>>
>> The first patch fixes the vhost virtqueue access check which was recently
>> broken. The second patch replaces the int return type with bool to prevent
>> future bugs.
>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>
> We need the 1st one on stable I think.
Series applied and patch #1 queued up for -stable.
^ 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