* Re: [PATCH] VFIO/VMBUS: Add VFIO VMBUS driver support
From: Greg KH @ 2019-11-11 9:49 UTC (permalink / raw)
To: lantianyu1986
Cc: alex.williamson, cohuck, kys, haiyangz, sthemmin, sashal,
mchehab+samsung, davem, robh, Jonathan.Cameron, paulmck,
michael.h.kelley, Tianyu Lan, linux-kernel, kvm, linux-hyperv,
vkuznets
In-Reply-To: <20191111084507.9286-1-Tianyu.Lan@microsoft.com>
On Mon, Nov 11, 2019 at 04:45:07PM +0800, lantianyu1986@gmail.com wrote:
> +#define DRIVER_VERSION "0.0.1"
Never a need for DRIVER_VERSION as your driver just becomes part of the
main kernel tree, so please drop this. We keep trying to delete these
types of numbers and they keep coming back...
> +static void
> +vfio_vmbus_new_channel(struct vmbus_channel *new_sc)
> +{
> + struct hv_device *hv_dev = new_sc->primary_channel->device_obj;
> + struct device *device = &hv_dev->device;
> + int ret;
> +
> + /* Create host communication ring */
> + ret = vmbus_open(new_sc, HV_RING_SIZE, HV_RING_SIZE, NULL, 0,
> + vfio_vmbus_channel_cb, new_sc);
> + if (ret) {
> + dev_err(device, "vmbus_open subchannel failed: %d\n", ret);
> + return;
> + }
> +
> + /* Disable interrupts on sub channel */
> + new_sc->inbound.ring_buffer->interrupt_mask = 1;
> + set_channel_read_mode(new_sc, HV_CALL_ISR);
> +
> + ret = sysfs_create_bin_file(&new_sc->kobj, &ring_buffer_bin_attr);
No documentation on this new sysfs file?
And by creating it here, userspace is not notified of it, so tools will
not see it :(
> + if (ret)
> + dev_notice(&hv_dev->device,
> + "sysfs create ring bin file failed; %d\n", ret);
Doesn't the call spit out an error if something happens?
> + ret = sysfs_create_bin_file(&channel->kobj, &ring_buffer_bin_attr);
> + if (ret)
> + dev_notice(&dev->device,
> + "sysfs create ring bin file failed; %d\n", ret);
> +
Again, don't create sysfs files on your own, the bus code should be
doing this for you automatically and in a way that is race-free.
thanks,
greg k-h
^ permalink raw reply
* RE: [PATCH net-next 11/14] vsock: add multi-transports support
From: Jorgen Hansen @ 2019-11-11 13:53 UTC (permalink / raw)
To: 'Stefano Garzarella', netdev@vger.kernel.org
Cc: Michael S. Tsirkin, kvm@vger.kernel.org, Greg Kroah-Hartman,
Jason Wang, David S. Miller, Dexuan Cui, Haiyang Zhang,
Sasha Levin, linux-kernel@vger.kernel.org, Arnd Bergmann,
Stefan Hajnoczi, linux-hyperv@vger.kernel.org, K. Y. Srinivasan,
Stephen Hemminger, virtualization@lists.linux-foundation.org
In-Reply-To: <20191023095554.11340-12-sgarzare@redhat.com>
> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> Sent: Wednesday, October 23, 2019 11:56 AM
Thanks a lot for working on this!
> With the multi-transports support, we can use vsock with nested VMs (using
> also different hypervisors) loading both guest->host and
> host->guest transports at the same time.
>
> Major changes:
> - vsock core module can be loaded regardless of the transports
> - vsock_core_init() and vsock_core_exit() are renamed to
> vsock_core_register() and vsock_core_unregister()
> - vsock_core_register() has a feature parameter (H2G, G2H, DGRAM)
> to identify which directions the transport can handle and if it's
> support DGRAM (only vmci)
> - each stream socket is assigned to a transport when the remote CID
> is set (during the connect() or when we receive a connection request
> on a listener socket).
How about allowing the transport to be set during bind as well? That
would allow an application to ensure that it is using a specific transport,
i.e., if it binds to the host CID, it will use H2G, and if it binds to something
else it will use G2H? You can still use VMADDR_CID_ANY if you want to
initially listen to both transports.
> The remote CID is used to decide which transport to use:
> - remote CID > VMADDR_CID_HOST will use host->guest transport
> - remote CID <= VMADDR_CID_HOST will use guest->host transport
> - listener sockets are not bound to any transports since no transport
> operations are done on it. In this way we can create a listener
> socket, also if the transports are not loaded or with VMADDR_CID_ANY
> to listen on all transports.
> - DGRAM sockets are handled as before, since only the vmci_transport
> provides this feature.
>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> RFC -> v1:
> - documented VSOCK_TRANSPORT_F_* flags
> - fixed vsock_assign_transport() when the socket is already assigned
> (e.g connection failed)
> - moved features outside of struct vsock_transport, and used as
> parameter of vsock_core_register()
> ---
> drivers/vhost/vsock.c | 5 +-
> include/net/af_vsock.h | 17 +-
> net/vmw_vsock/af_vsock.c | 237 ++++++++++++++++++------
> net/vmw_vsock/hyperv_transport.c | 26 ++-
> net/vmw_vsock/virtio_transport.c | 7 +-
> net/vmw_vsock/virtio_transport_common.c | 28 ++-
> net/vmw_vsock/vmci_transport.c | 31 +++-
> 7 files changed, 270 insertions(+), 81 deletions(-)
>
> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c index
> d89381166028..dddd85d9a147 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -130,7 +130,12 @@ static struct proto vsock_proto = { #define
> VSOCK_DEFAULT_BUFFER_MAX_SIZE (1024 * 256) #define
> VSOCK_DEFAULT_BUFFER_MIN_SIZE 128
>
> -static const struct vsock_transport *transport_single;
> +/* Transport used for host->guest communication */ static const struct
> +vsock_transport *transport_h2g;
> +/* Transport used for guest->host communication */ static const struct
> +vsock_transport *transport_g2h;
> +/* Transport used for DGRAM communication */ static const struct
> +vsock_transport *transport_dgram;
> static DEFINE_MUTEX(vsock_register_mutex);
>
> /**** UTILS ****/
> @@ -182,7 +187,7 @@ static int vsock_auto_bind(struct vsock_sock *vsk)
> return __vsock_bind(sk, &local_addr);
> }
>
> -static int __init vsock_init_tables(void)
> +static void vsock_init_tables(void)
> {
> int i;
>
> @@ -191,7 +196,6 @@ static int __init vsock_init_tables(void)
>
> for (i = 0; i < ARRAY_SIZE(vsock_connected_table); i++)
> INIT_LIST_HEAD(&vsock_connected_table[i]);
> - return 0;
> }
>
> static void __vsock_insert_bound(struct list_head *list, @@ -376,6 +380,62
> @@ void vsock_enqueue_accept(struct sock *listener, struct sock
> *connected) } EXPORT_SYMBOL_GPL(vsock_enqueue_accept);
>
> +/* Assign a transport to a socket and call the .init transport callback.
> + *
> + * Note: for stream socket this must be called when vsk->remote_addr is
> +set
> + * (e.g. during the connect() or when a connection request on a
> +listener
> + * socket is received).
> + * The vsk->remote_addr is used to decide which transport to use:
> + * - remote CID > VMADDR_CID_HOST will use host->guest transport
> + * - remote CID <= VMADDR_CID_HOST will use guest->host transport */
> +int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock
> +*psk) {
> + const struct vsock_transport *new_transport;
> + struct sock *sk = sk_vsock(vsk);
> +
> + switch (sk->sk_type) {
> + case SOCK_DGRAM:
> + new_transport = transport_dgram;
> + break;
> + case SOCK_STREAM:
> + if (vsk->remote_addr.svm_cid > VMADDR_CID_HOST)
> + new_transport = transport_h2g;
> + else
> + new_transport = transport_g2h;
> + break;
You already mentioned that you are working on a fix for loopback
here for the guest, but presumably a host could also do loopback.
If we select transport during bind to a specific CID, this comment
Isn't relevant, but otherwise, we should look at the local addr as
well, since a socket with local addr of host CID shouldn't use
the guest to host transport, and a socket with local addr > host CID
shouldn't use host to guest.
> + default:
> + return -ESOCKTNOSUPPORT;
> + }
> +
> + if (vsk->transport) {
> + if (vsk->transport == new_transport)
> + return 0;
> +
> + vsk->transport->release(vsk);
> + vsk->transport->destruct(vsk);
> + }
> +
> + if (!new_transport)
> + return -ENODEV;
> +
> + vsk->transport = new_transport;
> +
> + return vsk->transport->init(vsk, psk); }
> +EXPORT_SYMBOL_GPL(vsock_assign_transport);
> +
> +static bool vsock_find_cid(unsigned int cid) {
> + if (transport_g2h && cid == transport_g2h->get_local_cid())
> + return true;
> +
> + if (transport_h2g && cid == VMADDR_CID_HOST)
> + return true;
> +
> + return false;
> +}
> +
> static struct sock *vsock_dequeue_accept(struct sock *listener) {
> struct vsock_sock *vlistener;
> diff --git a/net/vmw_vsock/vmci_transport.c
> b/net/vmw_vsock/vmci_transport.c index 5955238ffc13..2eb3f16d53e7
> 100644
> --- a/net/vmw_vsock/vmci_transport.c
> +++ b/net/vmw_vsock/vmci_transport.c
> @@ -1017,6 +1018,15 @@ static int vmci_transport_recv_listen(struct sock
> *sk,
> vsock_addr_init(&vpending->remote_addr, pkt->dg.src.context,
> pkt->src_port);
>
> + err = vsock_assign_transport(vpending, vsock_sk(sk));
> + /* Transport assigned (looking at remote_addr) must be the same
> + * where we received the request.
> + */
> + if (err || !vmci_check_transport(vpending)) {
We need to send a reset on error, i.e.,
vmci_transport_send_reset(sk, pkt);
> + sock_put(pending);
> + return err;
> + }
> +
> /* If the proposed size fits within our min/max, accept it. Otherwise
> * propose our own size.
> */
Thanks,
Jorgen
^ permalink raw reply
* Re: [PATCH v1] tools/hv: async name resolution in kvp_daemon
From: Olaf Hering @ 2019-11-11 15:55 UTC (permalink / raw)
To: Sasha Levin
Cc: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
open list:Hyper-V CORE AND DRIVERS, open list
In-Reply-To: <20191102041856.GY1554@sasha-vm>
[-- Attachment #1: Type: text/plain, Size: 689 bytes --]
Am Sat, 2 Nov 2019 00:18:56 -0400
schrieb Sasha Levin <sashal@kernel.org>:
> make[1]: Leaving directory '/home/sasha/linux/tools/hv'
> gcc -O2 -Wall -g -D_GNU_SOURCE -Iinclude -lpthread hv_kvp_daemon-in.o -o hv_kvp_daemon
> /usr/bin/ld: hv_kvp_daemon-in.o: in function `kvp_obtain_domain_name':
> /home/sasha/linux/tools/hv/hv_kvp_daemon.c:1372: undefined reference to `pthread_create'
> /usr/bin/ld: /home/sasha/linux/tools/hv/hv_kvp_daemon.c:1377: undefined reference to `pthread_detach'
Is perhaps '-pthread' instead of -lpthread' required, as indicated by pthread_create(3)?
Not sure why it happens to work for me. But I will make this change for the upcoming v2.
Olaf
[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* RE: [PATCH net-next 12/14] vsock/vmci: register vmci_transport only when VMCI guest/host are active
From: Jorgen Hansen @ 2019-11-11 16:27 UTC (permalink / raw)
To: 'Stefano Garzarella', netdev@vger.kernel.org
Cc: Michael S. Tsirkin, kvm@vger.kernel.org, Greg Kroah-Hartman,
Jason Wang, David S. Miller, Dexuan Cui, Haiyang Zhang,
Sasha Levin, linux-kernel@vger.kernel.org, Arnd Bergmann,
Stefan Hajnoczi, linux-hyperv@vger.kernel.org, K. Y. Srinivasan,
Stephen Hemminger, virtualization@lists.linux-foundation.org
In-Reply-To: <20191023095554.11340-13-sgarzare@redhat.com>
> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> Sent: Wednesday, October 23, 2019 11:56 AM
>
> To allow other transports to be loaded with vmci_transport,
> we register the vmci_transport as G2H or H2G only when a VMCI guest
> or host is active.
>
> To do that, this patch adds a callback registered in the vmci driver
> that will be called when a new host or guest become active.
> This callback will register the vmci_transport in the VSOCK core.
> If the transport is already registered, we ignore the error coming
> from vsock_core_register().
So today this is mainly an issue for the VMCI vsock transport, because
VMCI autoloads with vsock (and with this solution it can continue to
do that, so none of our old products break due to changed behavior,
which is great). Shouldn't vhost behave similar, so that any module
that registers a h2g transport only does so if it is in active use?
> --- a/drivers/misc/vmw_vmci/vmci_host.c
> +++ b/drivers/misc/vmw_vmci/vmci_host.c
> @@ -108,6 +108,11 @@ bool vmci_host_code_active(void)
> atomic_read(&vmci_host_active_users) > 0);
> }
>
> +int vmci_host_users(void)
> +{
> + return atomic_read(&vmci_host_active_users);
> +}
> +
> /*
> * Called on open of /dev/vmci.
> */
> @@ -338,6 +343,8 @@ static int vmci_host_do_init_context(struct
> vmci_host_dev *vmci_host_dev,
> vmci_host_dev->ct_type = VMCIOBJ_CONTEXT;
> atomic_inc(&vmci_host_active_users);
>
> + vmci_call_vsock_callback(true);
> +
Since we don't unregister the transport if user count drops back to 0, we could
just call this the first time, a VM is powered on after the module is loaded.
> retval = 0;
>
> out:
^ permalink raw reply
* RE: [PATCH net-next 13/14] vsock: prevent transport modules unloading
From: Jorgen Hansen @ 2019-11-11 16:36 UTC (permalink / raw)
To: 'Stefano Garzarella', netdev@vger.kernel.org
Cc: Michael S. Tsirkin, kvm@vger.kernel.org, Greg Kroah-Hartman,
Jason Wang, David S. Miller, Dexuan Cui, Haiyang Zhang,
Sasha Levin, linux-kernel@vger.kernel.org, Arnd Bergmann,
Stefan Hajnoczi, linux-hyperv@vger.kernel.org, K. Y. Srinivasan,
Stephen Hemminger, virtualization@lists.linux-foundation.org
In-Reply-To: <20191023095554.11340-14-sgarzare@redhat.com>
> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> Sent: Wednesday, October 23, 2019 11:56 AM
> This patch adds 'module' member in the 'struct vsock_transport'
> in order to get/put the transport module. This prevents the
> module unloading while sockets are assigned to it.
>
> We increase the module refcnt when a socket is assigned to a
> transport, and we decrease the module refcnt when the socket
> is destructed.
>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> RFC -> v1:
> - fixed typo 's/tranport/transport/' in a comment (Stefan)
> ---
> drivers/vhost/vsock.c | 2 ++
> include/net/af_vsock.h | 2 ++
> net/vmw_vsock/af_vsock.c | 20 ++++++++++++++++----
> net/vmw_vsock/hyperv_transport.c | 2 ++
> net/vmw_vsock/virtio_transport.c | 2 ++
> net/vmw_vsock/vmci_transport.c | 1 +
> 6 files changed, 25 insertions(+), 4 deletions(-)
Reviewed-by: Jorgen Hansen <jhansen@vmware.com>
^ permalink raw reply
* Re: [PATCH] VFIO/VMBUS: Add VFIO VMBUS driver support
From: Stephen Hemminger @ 2019-11-11 16:47 UTC (permalink / raw)
To: Greg KH
Cc: lantianyu1986, alex.williamson, cohuck, KY Srinivasan,
Haiyang Zhang, Stephen Hemminger, sashal, mchehab+samsung, davem,
robh, Jonathan.Cameron, paulmck, Michael Kelley, Tianyu Lan,
linux-kernel, kvm, linux-hyperv, vkuznets
In-Reply-To: <20191111094920.GA135867@kroah.com>
On Mon, 11 Nov 2019 01:49:20 -0800
"Greg KH" <gregkh@linuxfoundation.org> wrote:
> > + ret = sysfs_create_bin_file(&channel->kobj,
> &ring_buffer_bin_attr);
> > + if (ret)
> > + dev_notice(&dev->device,
> > + "sysfs create ring bin file failed; %d\n",
> ret);
> > +
>
> Again, don't create sysfs files on your own, the bus code should be
> doing this for you automatically and in a way that is race-free.
>
> thanks,
>
> greg k-h
The sysfs file is only created if the VFIO/UIO driveris used.
^ permalink raw reply
* RE: [PATCH net-next 14/14] vsock: fix bind() behaviour taking care of CID
From: Jorgen Hansen @ 2019-11-11 16:53 UTC (permalink / raw)
To: 'Stefano Garzarella', netdev@vger.kernel.org
Cc: Michael S. Tsirkin, kvm@vger.kernel.org, Greg Kroah-Hartman,
Jason Wang, David S. Miller, Dexuan Cui, Haiyang Zhang,
Sasha Levin, linux-kernel@vger.kernel.org, Arnd Bergmann,
Stefan Hajnoczi, linux-hyperv@vger.kernel.org, K. Y. Srinivasan,
Stephen Hemminger, virtualization@lists.linux-foundation.org
In-Reply-To: <20191023095554.11340-15-sgarzare@redhat.com>
> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> Sent: Wednesday, October 23, 2019 11:56 AM
> When we are looking for a socket bound to a specific address,
> we also have to take into account the CID.
>
> This patch is useful with multi-transports support because it
> allows the binding of the same port with different CID, and
> it prevents a connection to a wrong socket bound to the same
> port, but with different CID.
>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> net/vmw_vsock/af_vsock.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
Reviewed-by: Jorgen Hansen <jhansen@vmware.com>
^ permalink raw reply
* Re: [PATCH net-next 11/14] vsock: add multi-transports support
From: Stefano Garzarella @ 2019-11-11 17:17 UTC (permalink / raw)
To: Jorgen Hansen
Cc: netdev@vger.kernel.org, Michael S. Tsirkin, kvm@vger.kernel.org,
Greg Kroah-Hartman, Jason Wang, David S. Miller, Dexuan Cui,
Haiyang Zhang, Sasha Levin, linux-kernel@vger.kernel.org,
Arnd Bergmann, Stefan Hajnoczi, linux-hyperv@vger.kernel.org,
K. Y. Srinivasan, Stephen Hemminger,
virtualization@lists.linux-foundation.org
In-Reply-To: <MWHPR05MB33761FE4DA27130C72FC5048DA740@MWHPR05MB3376.namprd05.prod.outlook.com>
On Mon, Nov 11, 2019 at 01:53:39PM +0000, Jorgen Hansen wrote:
> > From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> > Sent: Wednesday, October 23, 2019 11:56 AM
>
> Thanks a lot for working on this!
>
Thanks to you for the reviews!
> > With the multi-transports support, we can use vsock with nested VMs (using
> > also different hypervisors) loading both guest->host and
> > host->guest transports at the same time.
> >
> > Major changes:
> > - vsock core module can be loaded regardless of the transports
> > - vsock_core_init() and vsock_core_exit() are renamed to
> > vsock_core_register() and vsock_core_unregister()
> > - vsock_core_register() has a feature parameter (H2G, G2H, DGRAM)
> > to identify which directions the transport can handle and if it's
> > support DGRAM (only vmci)
> > - each stream socket is assigned to a transport when the remote CID
> > is set (during the connect() or when we receive a connection request
> > on a listener socket).
>
> How about allowing the transport to be set during bind as well? That
> would allow an application to ensure that it is using a specific transport,
> i.e., if it binds to the host CID, it will use H2G, and if it binds to something
> else it will use G2H? You can still use VMADDR_CID_ANY if you want to
> initially listen to both transports.
Do you mean for socket that will call the connect()?
For listener socket the "[PATCH net-next 14/14] vsock: fix bind() behaviour
taking care of CID" provides this behaviour.
Since the listener sockets don't use any transport specific callback
(they don't send any data to the remote peer), but they are used as placeholder,
we don't need to assign them to a transport.
>
>
> > The remote CID is used to decide which transport to use:
> > - remote CID > VMADDR_CID_HOST will use host->guest transport
> > - remote CID <= VMADDR_CID_HOST will use guest->host transport
> > - listener sockets are not bound to any transports since no transport
> > operations are done on it. In this way we can create a listener
> > socket, also if the transports are not loaded or with VMADDR_CID_ANY
> > to listen on all transports.
> > - DGRAM sockets are handled as before, since only the vmci_transport
> > provides this feature.
> >
> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > ---
> > RFC -> v1:
> > - documented VSOCK_TRANSPORT_F_* flags
> > - fixed vsock_assign_transport() when the socket is already assigned
> > (e.g connection failed)
> > - moved features outside of struct vsock_transport, and used as
> > parameter of vsock_core_register()
> > ---
> > drivers/vhost/vsock.c | 5 +-
> > include/net/af_vsock.h | 17 +-
> > net/vmw_vsock/af_vsock.c | 237 ++++++++++++++++++------
> > net/vmw_vsock/hyperv_transport.c | 26 ++-
> > net/vmw_vsock/virtio_transport.c | 7 +-
> > net/vmw_vsock/virtio_transport_common.c | 28 ++-
> > net/vmw_vsock/vmci_transport.c | 31 +++-
> > 7 files changed, 270 insertions(+), 81 deletions(-)
> >
>
>
> > diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c index
> > d89381166028..dddd85d9a147 100644
> > --- a/net/vmw_vsock/af_vsock.c
> > +++ b/net/vmw_vsock/af_vsock.c
> > @@ -130,7 +130,12 @@ static struct proto vsock_proto = { #define
> > VSOCK_DEFAULT_BUFFER_MAX_SIZE (1024 * 256) #define
> > VSOCK_DEFAULT_BUFFER_MIN_SIZE 128
> >
> > -static const struct vsock_transport *transport_single;
> > +/* Transport used for host->guest communication */ static const struct
> > +vsock_transport *transport_h2g;
> > +/* Transport used for guest->host communication */ static const struct
> > +vsock_transport *transport_g2h;
> > +/* Transport used for DGRAM communication */ static const struct
> > +vsock_transport *transport_dgram;
> > static DEFINE_MUTEX(vsock_register_mutex);
> >
> > /**** UTILS ****/
> > @@ -182,7 +187,7 @@ static int vsock_auto_bind(struct vsock_sock *vsk)
> > return __vsock_bind(sk, &local_addr);
> > }
> >
> > -static int __init vsock_init_tables(void)
> > +static void vsock_init_tables(void)
> > {
> > int i;
> >
> > @@ -191,7 +196,6 @@ static int __init vsock_init_tables(void)
> >
> > for (i = 0; i < ARRAY_SIZE(vsock_connected_table); i++)
> > INIT_LIST_HEAD(&vsock_connected_table[i]);
> > - return 0;
> > }
> >
> > static void __vsock_insert_bound(struct list_head *list, @@ -376,6 +380,62
> > @@ void vsock_enqueue_accept(struct sock *listener, struct sock
> > *connected) } EXPORT_SYMBOL_GPL(vsock_enqueue_accept);
> >
> > +/* Assign a transport to a socket and call the .init transport callback.
> > + *
> > + * Note: for stream socket this must be called when vsk->remote_addr is
> > +set
> > + * (e.g. during the connect() or when a connection request on a
> > +listener
> > + * socket is received).
> > + * The vsk->remote_addr is used to decide which transport to use:
> > + * - remote CID > VMADDR_CID_HOST will use host->guest transport
> > + * - remote CID <= VMADDR_CID_HOST will use guest->host transport */
> > +int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock
> > +*psk) {
> > + const struct vsock_transport *new_transport;
> > + struct sock *sk = sk_vsock(vsk);
> > +
> > + switch (sk->sk_type) {
> > + case SOCK_DGRAM:
> > + new_transport = transport_dgram;
> > + break;
> > + case SOCK_STREAM:
> > + if (vsk->remote_addr.svm_cid > VMADDR_CID_HOST)
> > + new_transport = transport_h2g;
> > + else
> > + new_transport = transport_g2h;
> > + break;
>
> You already mentioned that you are working on a fix for loopback
> here for the guest, but presumably a host could also do loopback.
IIUC we don't support loopback in the host, because in this case the
application will use the CID_HOST as address, but if we are in a nested
VM environment we are in trouble.
Since several people asked about this feature at the KVM Forum, I would like
to add a new VMADDR_CID_LOCAL (i.e. using the reserved 1) and implement
loopback in the core.
What do you think?
> If we select transport during bind to a specific CID, this comment
Also in this case, are you talking about the peer that will call
connect()?
> Isn't relevant, but otherwise, we should look at the local addr as
> well, since a socket with local addr of host CID shouldn't use
> the guest to host transport, and a socket with local addr > host CID
> shouldn't use host to guest.
Yes, I agree, in my fix I'm looking at the local addr, and in L1 I'll
not allow to assign a CID to a nested L2 equal to the CID of L1 (in
vhost-vsock)
Maybe we can allow the host loopback (using CID_HOST), only if there isn't
G2H loaded, but also in this case I'd like to move the loopback in the vsock
core, since we can do that, also if there are no transports loaded.
>
>
> > + default:
> > + return -ESOCKTNOSUPPORT;
> > + }
> > +
> > + if (vsk->transport) {
> > + if (vsk->transport == new_transport)
> > + return 0;
> > +
> > + vsk->transport->release(vsk);
> > + vsk->transport->destruct(vsk);
> > + }
> > +
> > + if (!new_transport)
> > + return -ENODEV;
> > +
> > + vsk->transport = new_transport;
> > +
> > + return vsk->transport->init(vsk, psk); }
> > +EXPORT_SYMBOL_GPL(vsock_assign_transport);
> > +
> > +static bool vsock_find_cid(unsigned int cid) {
> > + if (transport_g2h && cid == transport_g2h->get_local_cid())
> > + return true;
> > +
> > + if (transport_h2g && cid == VMADDR_CID_HOST)
> > + return true;
> > +
> > + return false;
> > +}
> > +
> > static struct sock *vsock_dequeue_accept(struct sock *listener) {
> > struct vsock_sock *vlistener;
>
>
> > diff --git a/net/vmw_vsock/vmci_transport.c
> > b/net/vmw_vsock/vmci_transport.c index 5955238ffc13..2eb3f16d53e7
> > 100644
> > --- a/net/vmw_vsock/vmci_transport.c
> > +++ b/net/vmw_vsock/vmci_transport.c
>
> > @@ -1017,6 +1018,15 @@ static int vmci_transport_recv_listen(struct sock
> > *sk,
> > vsock_addr_init(&vpending->remote_addr, pkt->dg.src.context,
> > pkt->src_port);
> >
> > + err = vsock_assign_transport(vpending, vsock_sk(sk));
> > + /* Transport assigned (looking at remote_addr) must be the same
> > + * where we received the request.
> > + */
> > + if (err || !vmci_check_transport(vpending)) {
>
> We need to send a reset on error, i.e.,
> vmci_transport_send_reset(sk, pkt);
Good catch, I'll fix in the v2.
Thanks,
Stefano
^ permalink raw reply
* Re: [PATCH] VFIO/VMBUS: Add VFIO VMBUS driver support
From: Greg KH @ 2019-11-11 17:23 UTC (permalink / raw)
To: Stephen Hemminger
Cc: lantianyu1986, alex.williamson, cohuck, KY Srinivasan,
Haiyang Zhang, Stephen Hemminger, sashal, mchehab+samsung, davem,
robh, Jonathan.Cameron, paulmck, Michael Kelley, Tianyu Lan,
linux-kernel, kvm, linux-hyperv, vkuznets
In-Reply-To: <20191111084712.37ba7d5a@hermes.lan>
On Mon, Nov 11, 2019 at 08:47:12AM -0800, Stephen Hemminger wrote:
> On Mon, 11 Nov 2019 01:49:20 -0800
> "Greg KH" <gregkh@linuxfoundation.org> wrote:
>
> > > + ret = sysfs_create_bin_file(&channel->kobj,
> > &ring_buffer_bin_attr);
> > > + if (ret)
> > > + dev_notice(&dev->device,
> > > + "sysfs create ring bin file failed; %d\n",
> > ret);
> > > +
> >
> > Again, don't create sysfs files on your own, the bus code should be
> > doing this for you automatically and in a way that is race-free.
> >
> > thanks,
> >
> > greg k-h
>
> The sysfs file is only created if the VFIO/UIO driveris used.
That's even worse. Again, sysfs files should be automatically created
by the driver core when the device is created. To randomly add/remove
random files after that happens means userspace is never notified of
that and that's not good.
We've been working for a while to fix up these types of races, don't
purposfully add new ones for no good reason please :)
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH] VFIO/VMBUS: Add VFIO VMBUS driver support
From: Stephen Hemminger @ 2019-11-11 17:29 UTC (permalink / raw)
To: Greg KH
Cc: lantianyu1986, alex.williamson, cohuck, KY Srinivasan,
Haiyang Zhang, Stephen Hemminger, sashal, mchehab+samsung, davem,
robh, Jonathan.Cameron, paulmck, Michael Kelley, Tianyu Lan,
linux-kernel, kvm, linux-hyperv, vkuznets
In-Reply-To: <20191111172322.GB1077444@kroah.com>
On Mon, 11 Nov 2019 18:23:22 +0100
Greg KH <gregkh@linuxfoundation.org> wrote:
> On Mon, Nov 11, 2019 at 08:47:12AM -0800, Stephen Hemminger wrote:
> > On Mon, 11 Nov 2019 01:49:20 -0800
> > "Greg KH" <gregkh@linuxfoundation.org> wrote:
> >
> > > > + ret = sysfs_create_bin_file(&channel->kobj,
> > > &ring_buffer_bin_attr);
> > > > + if (ret)
> > > > + dev_notice(&dev->device,
> > > > + "sysfs create ring bin file failed; %d\n",
> > > ret);
> > > > +
> > >
> > > Again, don't create sysfs files on your own, the bus code should be
> > > doing this for you automatically and in a way that is race-free.
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> > The sysfs file is only created if the VFIO/UIO driveris used.
>
> That's even worse. Again, sysfs files should be automatically created
> by the driver core when the device is created. To randomly add/remove
> random files after that happens means userspace is never notified of
> that and that's not good.
>
> We've been working for a while to fix up these types of races, don't
> purposfully add new ones for no good reason please :)
>
> thanks,
>
> greg k-h
The handler for this sysfs file is in the vfio (and uio) driver.
How would this work if bus handled it?
^ permalink raw reply
* Re: [PATCH net-next 12/14] vsock/vmci: register vmci_transport only when VMCI guest/host are active
From: Stefano Garzarella @ 2019-11-11 17:30 UTC (permalink / raw)
To: Jorgen Hansen
Cc: netdev@vger.kernel.org, Michael S. Tsirkin, kvm@vger.kernel.org,
Greg Kroah-Hartman, Jason Wang, David S. Miller, Dexuan Cui,
Haiyang Zhang, Sasha Levin, linux-kernel@vger.kernel.org,
Arnd Bergmann, Stefan Hajnoczi, linux-hyperv@vger.kernel.org,
K. Y. Srinivasan, Stephen Hemminger,
virtualization@lists.linux-foundation.org
In-Reply-To: <MWHPR05MB3376266BC6AE9E6E0B75F1A1DA740@MWHPR05MB3376.namprd05.prod.outlook.com>
On Mon, Nov 11, 2019 at 04:27:28PM +0000, Jorgen Hansen wrote:
> > From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> > Sent: Wednesday, October 23, 2019 11:56 AM
> >
> > To allow other transports to be loaded with vmci_transport,
> > we register the vmci_transport as G2H or H2G only when a VMCI guest
> > or host is active.
> >
> > To do that, this patch adds a callback registered in the vmci driver
> > that will be called when a new host or guest become active.
> > This callback will register the vmci_transport in the VSOCK core.
> > If the transport is already registered, we ignore the error coming
> > from vsock_core_register().
>
> So today this is mainly an issue for the VMCI vsock transport, because
> VMCI autoloads with vsock (and with this solution it can continue to
> do that, so none of our old products break due to changed behavior,
> which is great).
I tried to not break anything :-)
> Shouldn't vhost behave similar, so that any module
> that registers a h2g transport only does so if it is in active use?
>
The vhost-vsock module will load when the first hypervisor open
/dev/vhost-vsock, so in theory, when there's at least one active user.
>
> > --- a/drivers/misc/vmw_vmci/vmci_host.c
> > +++ b/drivers/misc/vmw_vmci/vmci_host.c
> > @@ -108,6 +108,11 @@ bool vmci_host_code_active(void)
> > atomic_read(&vmci_host_active_users) > 0);
> > }
> >
> > +int vmci_host_users(void)
> > +{
> > + return atomic_read(&vmci_host_active_users);
> > +}
> > +
> > /*
> > * Called on open of /dev/vmci.
> > */
> > @@ -338,6 +343,8 @@ static int vmci_host_do_init_context(struct
> > vmci_host_dev *vmci_host_dev,
> > vmci_host_dev->ct_type = VMCIOBJ_CONTEXT;
> > atomic_inc(&vmci_host_active_users);
> >
> > + vmci_call_vsock_callback(true);
> > +
>
> Since we don't unregister the transport if user count drops back to 0, we could
> just call this the first time, a VM is powered on after the module is loaded.
Yes, make sense. can I use the 'vmci_host_active_users' or is better to
add a new 'vmci_host_vsock_loaded'?
My doubt is that vmci_host_active_users can return to 0, so when it returns
to 1, we call vmci_call_vsock_callback() again.
Thanks,
Stefano
^ permalink raw reply
* RE: [PATCH net-next 11/14] vsock: add multi-transports support
From: Jorgen Hansen @ 2019-11-12 9:59 UTC (permalink / raw)
To: 'Stefano Garzarella'
Cc: netdev@vger.kernel.org, Michael S. Tsirkin, kvm@vger.kernel.org,
Greg Kroah-Hartman, Jason Wang, David S. Miller, Dexuan Cui,
Haiyang Zhang, Sasha Levin, linux-kernel@vger.kernel.org,
Arnd Bergmann, Stefan Hajnoczi, linux-hyperv@vger.kernel.org,
K. Y. Srinivasan, Stephen Hemminger,
virtualization@lists.linux-foundation.org
In-Reply-To: <20191111171740.xwo7isdmtt7ywibo@steredhat>
> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> Sent: Monday, November 11, 2019 6:18 PM
> To: Jorgen Hansen <jhansen@vmware.com>
> Subject: Re: [PATCH net-next 11/14] vsock: add multi-transports support
>
> On Mon, Nov 11, 2019 at 01:53:39PM +0000, Jorgen Hansen wrote:
> > > From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> > > Sent: Wednesday, October 23, 2019 11:56 AM
> >
> > Thanks a lot for working on this!
> >
>
> Thanks to you for the reviews!
>
> > > With the multi-transports support, we can use vsock with nested VMs
> (using
> > > also different hypervisors) loading both guest->host and
> > > host->guest transports at the same time.
> > >
> > > Major changes:
> > > - vsock core module can be loaded regardless of the transports
> > > - vsock_core_init() and vsock_core_exit() are renamed to
> > > vsock_core_register() and vsock_core_unregister()
> > > - vsock_core_register() has a feature parameter (H2G, G2H, DGRAM)
> > > to identify which directions the transport can handle and if it's
> > > support DGRAM (only vmci)
> > > - each stream socket is assigned to a transport when the remote CID
> > > is set (during the connect() or when we receive a connection request
> > > on a listener socket).
> >
> > How about allowing the transport to be set during bind as well? That
> > would allow an application to ensure that it is using a specific transport,
> > i.e., if it binds to the host CID, it will use H2G, and if it binds to something
> > else it will use G2H? You can still use VMADDR_CID_ANY if you want to
> > initially listen to both transports.
>
> Do you mean for socket that will call the connect()?
I was just thinking that in general we know the transport at that point, so we
could ensure that the socket would only see traffic from the relevant transport,
but as you mention below - the updated bind lookup, and the added checks
when selecting transport should also take care of this, so that is fine.
> For listener socket the "[PATCH net-next 14/14] vsock: fix bind() behaviour
> taking care of CID" provides this behaviour.
> Since the listener sockets don't use any transport specific callback
> (they don't send any data to the remote peer), but they are used as
> placeholder,
> we don't need to assign them to a transport.
>
> >
> >
> > > The remote CID is used to decide which transport to use:
> > > - remote CID > VMADDR_CID_HOST will use host->guest transport
> > > - remote CID <= VMADDR_CID_HOST will use guest->host transport
> > > - listener sockets are not bound to any transports since no transport
> > > operations are done on it. In this way we can create a listener
> > > socket, also if the transports are not loaded or with VMADDR_CID_ANY
> > > to listen on all transports.
> > > - DGRAM sockets are handled as before, since only the vmci_transport
> > > provides this feature.
> > >
> > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > ---
> > > RFC -> v1:
> > > - documented VSOCK_TRANSPORT_F_* flags
> > > - fixed vsock_assign_transport() when the socket is already assigned
> > > (e.g connection failed)
> > > - moved features outside of struct vsock_transport, and used as
> > > parameter of vsock_core_register()
> > > ---
> > > drivers/vhost/vsock.c | 5 +-
> > > include/net/af_vsock.h | 17 +-
> > > net/vmw_vsock/af_vsock.c | 237 ++++++++++++++++++------
> > > net/vmw_vsock/hyperv_transport.c | 26 ++-
> > > net/vmw_vsock/virtio_transport.c | 7 +-
> > > net/vmw_vsock/virtio_transport_common.c | 28 ++-
> > > net/vmw_vsock/vmci_transport.c | 31 +++-
> > > 7 files changed, 270 insertions(+), 81 deletions(-)
> > >
> >
> >
> > > diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index
> > > d89381166028..dddd85d9a147 100644
> > > --- a/net/vmw_vsock/af_vsock.c
> > > +++ b/net/vmw_vsock/af_vsock.c
> > > @@ -130,7 +130,12 @@ static struct proto vsock_proto = { #define
> > > VSOCK_DEFAULT_BUFFER_MAX_SIZE (1024 * 256) #define
> > > VSOCK_DEFAULT_BUFFER_MIN_SIZE 128
> > >
> > > -static const struct vsock_transport *transport_single;
> > > +/* Transport used for host->guest communication */ static const struct
> > > +vsock_transport *transport_h2g;
> > > +/* Transport used for guest->host communication */ static const struct
> > > +vsock_transport *transport_g2h;
> > > +/* Transport used for DGRAM communication */ static const struct
> > > +vsock_transport *transport_dgram;
> > > static DEFINE_MUTEX(vsock_register_mutex);
> > >
> > > /**** UTILS ****/
> > > @@ -182,7 +187,7 @@ static int vsock_auto_bind(struct vsock_sock *vsk)
> > > return __vsock_bind(sk, &local_addr);
> > > }
> > >
> > > -static int __init vsock_init_tables(void)
> > > +static void vsock_init_tables(void)
> > > {
> > > int i;
> > >
> > > @@ -191,7 +196,6 @@ static int __init vsock_init_tables(void)
> > >
> > > for (i = 0; i < ARRAY_SIZE(vsock_connected_table); i++)
> > > INIT_LIST_HEAD(&vsock_connected_table[i]);
> > > - return 0;
> > > }
> > >
> > > static void __vsock_insert_bound(struct list_head *list, @@ -376,6
> +380,62
> > > @@ void vsock_enqueue_accept(struct sock *listener, struct sock
> > > *connected) } EXPORT_SYMBOL_GPL(vsock_enqueue_accept);
> > >
> > > +/* Assign a transport to a socket and call the .init transport callback.
> > > + *
> > > + * Note: for stream socket this must be called when vsk->remote_addr
> is
> > > +set
> > > + * (e.g. during the connect() or when a connection request on a
> > > +listener
> > > + * socket is received).
> > > + * The vsk->remote_addr is used to decide which transport to use:
> > > + * - remote CID > VMADDR_CID_HOST will use host->guest transport
> > > + * - remote CID <= VMADDR_CID_HOST will use guest->host transport
> */
> > > +int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock
> > > +*psk) {
> > > + const struct vsock_transport *new_transport;
> > > + struct sock *sk = sk_vsock(vsk);
> > > +
> > > + switch (sk->sk_type) {
> > > + case SOCK_DGRAM:
> > > + new_transport = transport_dgram;
> > > + break;
> > > + case SOCK_STREAM:
> > > + if (vsk->remote_addr.svm_cid > VMADDR_CID_HOST)
> > > + new_transport = transport_h2g;
> > > + else
> > > + new_transport = transport_g2h;
> > > + break;
> >
> > You already mentioned that you are working on a fix for loopback
> > here for the guest, but presumably a host could also do loopback.
>
> IIUC we don't support loopback in the host, because in this case the
> application will use the CID_HOST as address, but if we are in a nested
> VM environment we are in trouble.
If both src and dst CID are CID_HOST, we should be fairly sure that this
Is host loopback, no? If src is anything else, we would do G2H.
>
> Since several people asked about this feature at the KVM Forum, I would like
> to add a new VMADDR_CID_LOCAL (i.e. using the reserved 1) and implement
> loopback in the core.
>
> What do you think?
What kind of use cases are mentioned in the KVM forum for loopback? One concern
is that we have to maintain yet another interprocess communication mechanism,
even though other choices exist already (and those are likely to be more efficient
given the development time and specific focus that went into those). To me, the
local connections are mainly useful as a way to sanity test the protocol and transports.
However, if loopback is compelling, it would make sense have it in the core, since it
shouldn't need a specific transport.
>
> > If we select transport during bind to a specific CID, this comment
>
> Also in this case, are you talking about the peer that will call
> connect()?
The same thought as mentioned in the beginning - but as mentioned
above, I agree that your updated bind and transport selection should
handle this as well.
> > Isn't relevant, but otherwise, we should look at the local addr as
> > well, since a socket with local addr of host CID shouldn't use
> > the guest to host transport, and a socket with local addr > host CID
> > shouldn't use host to guest.
>
> Yes, I agree, in my fix I'm looking at the local addr, and in L1 I'll
> not allow to assign a CID to a nested L2 equal to the CID of L1 (in
> vhost-vsock)
>
> Maybe we can allow the host loopback (using CID_HOST), only if there isn't
> G2H loaded, but also in this case I'd like to move the loopback in the vsock
> core, since we can do that, also if there are no transports loaded.
> >
> >
> > > + default:
> > > + return -ESOCKTNOSUPPORT;
> > > + }
> > > +
> > > + if (vsk->transport) {
> > > + if (vsk->transport == new_transport)
> > > + return 0;
> > > +
> > > + vsk->transport->release(vsk);
> > > + vsk->transport->destruct(vsk);
> > > + }
> > > +
> > > + if (!new_transport)
> > > + return -ENODEV;
> > > +
> > > + vsk->transport = new_transport;
> > > +
> > > + return vsk->transport->init(vsk, psk); }
> > > +EXPORT_SYMBOL_GPL(vsock_assign_transport);
> > > +
> > > +static bool vsock_find_cid(unsigned int cid) {
> > > + if (transport_g2h && cid == transport_g2h->get_local_cid())
> > > + return true;
> > > +
> > > + if (transport_h2g && cid == VMADDR_CID_HOST)
> > > + return true;
> > > +
> > > + return false;
> > > +}
> > > +
> > > static struct sock *vsock_dequeue_accept(struct sock *listener) {
> > > struct vsock_sock *vlistener;
> >
> >
> > > diff --git a/net/vmw_vsock/vmci_transport.c
> > > b/net/vmw_vsock/vmci_transport.c index 5955238ffc13..2eb3f16d53e7
> > > 100644
> > > --- a/net/vmw_vsock/vmci_transport.c
> > > +++ b/net/vmw_vsock/vmci_transport.c
> >
> > > @@ -1017,6 +1018,15 @@ static int vmci_transport_recv_listen(struct
> sock
> > > *sk,
> > > vsock_addr_init(&vpending->remote_addr, pkt->dg.src.context,
> > > pkt->src_port);
> > >
> > > + err = vsock_assign_transport(vpending, vsock_sk(sk));
> > > + /* Transport assigned (looking at remote_addr) must be the same
> > > + * where we received the request.
> > > + */
> > > + if (err || !vmci_check_transport(vpending)) {
> >
> > We need to send a reset on error, i.e.,
> > vmci_transport_send_reset(sk, pkt);
>
> Good catch, I'll fix in the v2.
>
> Thanks,
> Stefano
Thanks,
Jorgen
^ permalink raw reply
* RE: [PATCH net-next 12/14] vsock/vmci: register vmci_transport only when VMCI guest/host are active
From: Jorgen Hansen @ 2019-11-12 10:03 UTC (permalink / raw)
To: 'Stefano Garzarella'
Cc: netdev@vger.kernel.org, Michael S. Tsirkin, kvm@vger.kernel.org,
Greg Kroah-Hartman, Jason Wang, David S. Miller, Dexuan Cui,
Haiyang Zhang, Sasha Levin, linux-kernel@vger.kernel.org,
Arnd Bergmann, Stefan Hajnoczi, linux-hyperv@vger.kernel.org,
K. Y. Srinivasan, Stephen Hemminger,
virtualization@lists.linux-foundation.org
In-Reply-To: <20191111173053.erwfzawioxje635o@steredhat>
> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> Sent: Monday, November 11, 2019 6:31 PM
> On Mon, Nov 11, 2019 at 04:27:28PM +0000, Jorgen Hansen wrote:
> > > From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> > > Sent: Wednesday, October 23, 2019 11:56 AM
> > >
> > > To allow other transports to be loaded with vmci_transport,
> > > we register the vmci_transport as G2H or H2G only when a VMCI guest
> > > or host is active.
> > >
> > > To do that, this patch adds a callback registered in the vmci driver
> > > that will be called when a new host or guest become active.
> > > This callback will register the vmci_transport in the VSOCK core.
> > > If the transport is already registered, we ignore the error coming
> > > from vsock_core_register().
> >
> > So today this is mainly an issue for the VMCI vsock transport, because
> > VMCI autoloads with vsock (and with this solution it can continue to
> > do that, so none of our old products break due to changed behavior,
> > which is great).
>
> I tried to not break anything :-)
>
> > Shouldn't vhost behave similar, so that any module
> > that registers a h2g transport only does so if it is in active use?
> >
>
> The vhost-vsock module will load when the first hypervisor open
> /dev/vhost-vsock, so in theory, when there's at least one active user.
Ok, sounds good then.
>
> >
> > > --- a/drivers/misc/vmw_vmci/vmci_host.c
> > > +++ b/drivers/misc/vmw_vmci/vmci_host.c
> > > @@ -108,6 +108,11 @@ bool vmci_host_code_active(void)
> > > atomic_read(&vmci_host_active_users) > 0);
> > > }
> > >
> > > +int vmci_host_users(void)
> > > +{
> > > + return atomic_read(&vmci_host_active_users);
> > > +}
> > > +
> > > /*
> > > * Called on open of /dev/vmci.
> > > */
> > > @@ -338,6 +343,8 @@ static int vmci_host_do_init_context(struct
> > > vmci_host_dev *vmci_host_dev,
> > > vmci_host_dev->ct_type = VMCIOBJ_CONTEXT;
> > > atomic_inc(&vmci_host_active_users);
> > >
> > > + vmci_call_vsock_callback(true);
> > > +
> >
> > Since we don't unregister the transport if user count drops back to 0, we
> could
> > just call this the first time, a VM is powered on after the module is loaded.
>
> Yes, make sense. can I use the 'vmci_host_active_users' or is better to
> add a new 'vmci_host_vsock_loaded'?
>
> My doubt is that vmci_host_active_users can return to 0, so when it returns
> to 1, we call vmci_call_vsock_callback() again.
vmci_host_active_users can drop to 0 and then increase again, so having a flag
indicating whether the callback has been invoked would ensure that it is only
called once.
Thanks,
Jorgen
^ permalink raw reply
* Re: [PATCH net-next 11/14] vsock: add multi-transports support
From: Stefano Garzarella @ 2019-11-12 10:36 UTC (permalink / raw)
To: Jorgen Hansen
Cc: netdev@vger.kernel.org, Michael S. Tsirkin, kvm@vger.kernel.org,
Greg Kroah-Hartman, Jason Wang, David S. Miller, Dexuan Cui,
Haiyang Zhang, Sasha Levin, linux-kernel@vger.kernel.org,
Arnd Bergmann, Stefan Hajnoczi, linux-hyperv@vger.kernel.org,
K. Y. Srinivasan, Stephen Hemminger,
virtualization@lists.linux-foundation.org
In-Reply-To: <MWHPR05MB33764F82AFA882B921A11E56DA770@MWHPR05MB3376.namprd05.prod.outlook.com>
On Tue, Nov 12, 2019 at 09:59:12AM +0000, Jorgen Hansen wrote:
> > From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> > Sent: Monday, November 11, 2019 6:18 PM
> > To: Jorgen Hansen <jhansen@vmware.com>
> > Subject: Re: [PATCH net-next 11/14] vsock: add multi-transports support
> >
> > On Mon, Nov 11, 2019 at 01:53:39PM +0000, Jorgen Hansen wrote:
> > > > From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> > > > Sent: Wednesday, October 23, 2019 11:56 AM
> > >
> > > Thanks a lot for working on this!
> > >
> >
> > Thanks to you for the reviews!
> >
> > > > With the multi-transports support, we can use vsock with nested VMs
> > (using
> > > > also different hypervisors) loading both guest->host and
> > > > host->guest transports at the same time.
> > > >
> > > > Major changes:
> > > > - vsock core module can be loaded regardless of the transports
> > > > - vsock_core_init() and vsock_core_exit() are renamed to
> > > > vsock_core_register() and vsock_core_unregister()
> > > > - vsock_core_register() has a feature parameter (H2G, G2H, DGRAM)
> > > > to identify which directions the transport can handle and if it's
> > > > support DGRAM (only vmci)
> > > > - each stream socket is assigned to a transport when the remote CID
> > > > is set (during the connect() or when we receive a connection request
> > > > on a listener socket).
> > >
> > > How about allowing the transport to be set during bind as well? That
> > > would allow an application to ensure that it is using a specific transport,
> > > i.e., if it binds to the host CID, it will use H2G, and if it binds to something
> > > else it will use G2H? You can still use VMADDR_CID_ANY if you want to
> > > initially listen to both transports.
> >
> > Do you mean for socket that will call the connect()?
>
> I was just thinking that in general we know the transport at that point, so we
> could ensure that the socket would only see traffic from the relevant transport,
> but as you mention below - the updated bind lookup, and the added checks
> when selecting transport should also take care of this, so that is fine.
>
> > For listener socket the "[PATCH net-next 14/14] vsock: fix bind() behaviour
> > taking care of CID" provides this behaviour.
> > Since the listener sockets don't use any transport specific callback
> > (they don't send any data to the remote peer), but they are used as
> > placeholder,
> > we don't need to assign them to a transport.
> >
> > >
> > >
> > > > The remote CID is used to decide which transport to use:
> > > > - remote CID > VMADDR_CID_HOST will use host->guest transport
> > > > - remote CID <= VMADDR_CID_HOST will use guest->host transport
> > > > - listener sockets are not bound to any transports since no transport
> > > > operations are done on it. In this way we can create a listener
> > > > socket, also if the transports are not loaded or with VMADDR_CID_ANY
> > > > to listen on all transports.
> > > > - DGRAM sockets are handled as before, since only the vmci_transport
> > > > provides this feature.
> > > >
> > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > > ---
> > > > RFC -> v1:
> > > > - documented VSOCK_TRANSPORT_F_* flags
> > > > - fixed vsock_assign_transport() when the socket is already assigned
> > > > (e.g connection failed)
> > > > - moved features outside of struct vsock_transport, and used as
> > > > parameter of vsock_core_register()
> > > > ---
> > > > drivers/vhost/vsock.c | 5 +-
> > > > include/net/af_vsock.h | 17 +-
> > > > net/vmw_vsock/af_vsock.c | 237 ++++++++++++++++++------
> > > > net/vmw_vsock/hyperv_transport.c | 26 ++-
> > > > net/vmw_vsock/virtio_transport.c | 7 +-
> > > > net/vmw_vsock/virtio_transport_common.c | 28 ++-
> > > > net/vmw_vsock/vmci_transport.c | 31 +++-
> > > > 7 files changed, 270 insertions(+), 81 deletions(-)
> > > >
> > >
> > >
> > > > diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> > index
> > > > d89381166028..dddd85d9a147 100644
> > > > --- a/net/vmw_vsock/af_vsock.c
> > > > +++ b/net/vmw_vsock/af_vsock.c
> > > > @@ -130,7 +130,12 @@ static struct proto vsock_proto = { #define
> > > > VSOCK_DEFAULT_BUFFER_MAX_SIZE (1024 * 256) #define
> > > > VSOCK_DEFAULT_BUFFER_MIN_SIZE 128
> > > >
> > > > -static const struct vsock_transport *transport_single;
> > > > +/* Transport used for host->guest communication */ static const struct
> > > > +vsock_transport *transport_h2g;
> > > > +/* Transport used for guest->host communication */ static const struct
> > > > +vsock_transport *transport_g2h;
> > > > +/* Transport used for DGRAM communication */ static const struct
> > > > +vsock_transport *transport_dgram;
> > > > static DEFINE_MUTEX(vsock_register_mutex);
> > > >
> > > > /**** UTILS ****/
> > > > @@ -182,7 +187,7 @@ static int vsock_auto_bind(struct vsock_sock *vsk)
> > > > return __vsock_bind(sk, &local_addr);
> > > > }
> > > >
> > > > -static int __init vsock_init_tables(void)
> > > > +static void vsock_init_tables(void)
> > > > {
> > > > int i;
> > > >
> > > > @@ -191,7 +196,6 @@ static int __init vsock_init_tables(void)
> > > >
> > > > for (i = 0; i < ARRAY_SIZE(vsock_connected_table); i++)
> > > > INIT_LIST_HEAD(&vsock_connected_table[i]);
> > > > - return 0;
> > > > }
> > > >
> > > > static void __vsock_insert_bound(struct list_head *list, @@ -376,6
> > +380,62
> > > > @@ void vsock_enqueue_accept(struct sock *listener, struct sock
> > > > *connected) } EXPORT_SYMBOL_GPL(vsock_enqueue_accept);
> > > >
> > > > +/* Assign a transport to a socket and call the .init transport callback.
> > > > + *
> > > > + * Note: for stream socket this must be called when vsk->remote_addr
> > is
> > > > +set
> > > > + * (e.g. during the connect() or when a connection request on a
> > > > +listener
> > > > + * socket is received).
> > > > + * The vsk->remote_addr is used to decide which transport to use:
> > > > + * - remote CID > VMADDR_CID_HOST will use host->guest transport
> > > > + * - remote CID <= VMADDR_CID_HOST will use guest->host transport
> > */
> > > > +int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock
> > > > +*psk) {
> > > > + const struct vsock_transport *new_transport;
> > > > + struct sock *sk = sk_vsock(vsk);
> > > > +
> > > > + switch (sk->sk_type) {
> > > > + case SOCK_DGRAM:
> > > > + new_transport = transport_dgram;
> > > > + break;
> > > > + case SOCK_STREAM:
> > > > + if (vsk->remote_addr.svm_cid > VMADDR_CID_HOST)
> > > > + new_transport = transport_h2g;
> > > > + else
> > > > + new_transport = transport_g2h;
> > > > + break;
> > >
> > > You already mentioned that you are working on a fix for loopback
> > > here for the guest, but presumably a host could also do loopback.
> >
> > IIUC we don't support loopback in the host, because in this case the
> > application will use the CID_HOST as address, but if we are in a nested
> > VM environment we are in trouble.
>
> If both src and dst CID are CID_HOST, we should be fairly sure that this
> Is host loopback, no? If src is anything else, we would do G2H.
>
The problem is that we don't know the src until we assign a transport
looking at the dst. (or if the user bound the socket to CID_HOST before
the connect(), but it is not very common)
So if we are in a L1 and the user uses the local guest CID, it works, but if
it uses the HOST_CID, the packet will go to the L0.
If we are in L0, it could be simple, because we can simply check if G2H
is not loaded, so any packet to CID_HOST, is host loopback.
I think that if the user uses the IOCTL_VM_SOCKETS_GET_LOCAL_CID, to set
the dest CID for the loopback, it works in both cases because we return the
HOST_CID in L0, and always the guest CID in L1, also if a H2G is loaded to
handle the L2.
Maybe we should document this in the man page.
But I have a question: Does vmci support the host loopback?
I've tried, and it seems not.
Also vhost-vsock doesn't support it, but virtio-vsock does.
> >
> > Since several people asked about this feature at the KVM Forum, I would like
> > to add a new VMADDR_CID_LOCAL (i.e. using the reserved 1) and implement
> > loopback in the core.
> >
> > What do you think?
>
> What kind of use cases are mentioned in the KVM forum for loopback? One concern
> is that we have to maintain yet another interprocess communication mechanism,
> even though other choices exist already (and those are likely to be more efficient
> given the development time and specific focus that went into those). To me, the
> local connections are mainly useful as a way to sanity test the protocol and transports.
> However, if loopback is compelling, it would make sense have it in the core, since it
> shouldn't need a specific transport.
The common use cases is for developer point of view, and to test the
protocol and transports as you said.
People that are introducing VSOCK support in their projects, would like to
test them on their own PC without starting a VM.
The idea is to move the code to handle loopback from the virtio-vsock,
in the core, but in another series :-)
>
> >
> > > If we select transport during bind to a specific CID, this comment
> >
> > Also in this case, are you talking about the peer that will call
> > connect()?
>
> The same thought as mentioned in the beginning - but as mentioned
> above, I agree that your updated bind and transport selection should
> handle this as well.
Got it.
Thanks,
Stefano
^ permalink raw reply
* Re: [PATCH net-next 12/14] vsock/vmci: register vmci_transport only when VMCI guest/host are active
From: Stefano Garzarella @ 2019-11-12 10:42 UTC (permalink / raw)
To: Jorgen Hansen
Cc: netdev@vger.kernel.org, Michael S. Tsirkin, kvm@vger.kernel.org,
Greg Kroah-Hartman, Jason Wang, David S. Miller, Dexuan Cui,
Haiyang Zhang, Sasha Levin, linux-kernel@vger.kernel.org,
Arnd Bergmann, Stefan Hajnoczi, linux-hyperv@vger.kernel.org,
K. Y. Srinivasan, Stephen Hemminger,
virtualization@lists.linux-foundation.org
In-Reply-To: <MWHPR05MB33769FD52B833FC1C82F0A80DA770@MWHPR05MB3376.namprd05.prod.outlook.com>
On Tue, Nov 12, 2019 at 10:03:54AM +0000, Jorgen Hansen wrote:
> > From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> > Sent: Monday, November 11, 2019 6:31 PM
> > On Mon, Nov 11, 2019 at 04:27:28PM +0000, Jorgen Hansen wrote:
> > > > From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> > > > Sent: Wednesday, October 23, 2019 11:56 AM
> > > >
> > > > To allow other transports to be loaded with vmci_transport,
> > > > we register the vmci_transport as G2H or H2G only when a VMCI guest
> > > > or host is active.
> > > >
> > > > To do that, this patch adds a callback registered in the vmci driver
> > > > that will be called when a new host or guest become active.
> > > > This callback will register the vmci_transport in the VSOCK core.
> > > > If the transport is already registered, we ignore the error coming
> > > > from vsock_core_register().
> > >
> > > So today this is mainly an issue for the VMCI vsock transport, because
> > > VMCI autoloads with vsock (and with this solution it can continue to
> > > do that, so none of our old products break due to changed behavior,
> > > which is great).
> >
> > I tried to not break anything :-)
> >
> > > Shouldn't vhost behave similar, so that any module
> > > that registers a h2g transport only does so if it is in active use?
> > >
> >
> > The vhost-vsock module will load when the first hypervisor open
> > /dev/vhost-vsock, so in theory, when there's at least one active user.
>
> Ok, sounds good then.
>
> >
> > >
> > > > --- a/drivers/misc/vmw_vmci/vmci_host.c
> > > > +++ b/drivers/misc/vmw_vmci/vmci_host.c
> > > > @@ -108,6 +108,11 @@ bool vmci_host_code_active(void)
> > > > atomic_read(&vmci_host_active_users) > 0);
> > > > }
> > > >
> > > > +int vmci_host_users(void)
> > > > +{
> > > > + return atomic_read(&vmci_host_active_users);
> > > > +}
> > > > +
> > > > /*
> > > > * Called on open of /dev/vmci.
> > > > */
> > > > @@ -338,6 +343,8 @@ static int vmci_host_do_init_context(struct
> > > > vmci_host_dev *vmci_host_dev,
> > > > vmci_host_dev->ct_type = VMCIOBJ_CONTEXT;
> > > > atomic_inc(&vmci_host_active_users);
> > > >
> > > > + vmci_call_vsock_callback(true);
> > > > +
> > >
> > > Since we don't unregister the transport if user count drops back to 0, we
> > could
> > > just call this the first time, a VM is powered on after the module is loaded.
> >
> > Yes, make sense. can I use the 'vmci_host_active_users' or is better to
> > add a new 'vmci_host_vsock_loaded'?
> >
> > My doubt is that vmci_host_active_users can return to 0, so when it returns
> > to 1, we call vmci_call_vsock_callback() again.
>
> vmci_host_active_users can drop to 0 and then increase again, so having a flag
> indicating whether the callback has been invoked would ensure that it is only
> called once.
I agree, I will use a dedicated flag, maybe in the
vmci_call_vsock_callback(), since it can be called or during the
vmci_host_do_init_context() or when the callback is registered.
Thanks,
Stefano
^ permalink raw reply
* Investment opportunity
From: Peter Wong @ 2019-11-12 17:57 UTC (permalink / raw)
To: linux-hyperv
Greetings,
Find attached email very confidential. reply for more details
Thanks.
Peter Wong
----------------------------------------------------
This email was sent by the shareware version of Postman Professional.
^ permalink raw reply
* [PATCH v2 1/1] x86/hyperv: Initialize clockevents earlier in CPU onlining
From: Michael Kelley @ 2019-11-13 1:11 UTC (permalink / raw)
To: linux-kernel@vger.kernel.org, tglx@linutronix.de,
daniel.lezcano@linaro.org, vkuznets, Dexuan Cui, KY Srinivasan,
Stephen Hemminger, sashal@kernel.org, mingo@redhat.com,
bp@alien8.de, hpa@zytor.com, x86@kernel.org, will@kernel.org,
Ganapatrao.Kulkarni@cavium.com, james.morse@arm.com,
steven.price@arm.com, josephl@nvidia.com,
m.szyprowski@samsung.com, linux-hyperv@vger.kernel.org
Cc: Michael Kelley
Hyper-V has historically initialized stimer-based clockevents late
in the process of onlining a CPU because clockevents depend on
stimer interrupts. In the original Hyper-V design, stimer
interrupts generate a VMbus message, so the VMbus machinery must
be running first, and VMbus can't be initialized until relatively
late. On x86/64, LAPIC timer based clockevents are used during early
initialization before VMbus and stimer-based clockevents are ready,
and again during CPU offlining after the stimer clockevents have been
shut down.
Unfortunately, this design creates problems when offling CPUs for
hibernation or other purposes. stimer-based clockevents are shut
down relatively early in the offlining process, so
clockevents_unbind_device() must be used to fallback to the
LAPIC-based clockevents for the remainder of the offlining process.
Furthermore, the late initialization and early shutdown of
stimer-based clockevents doesn't work well on ARM64 since there
is no other timer like the LAPIC to fallback to. So CPU onlining and
offlining doesn't work properly.
Fix this by recognizing that stimer Direct Mode is the normal path
for newer versions of Hyper-V on x86/64, and the only path on other
architectures. With stimer Direct Mode, stimer interrupts don't
require any VMbus machinery. stimer clockevents can be initialized
and shut down consistent with how it is done for other clockevent
devices. While the old VMbus-based stimer interrupts must still
be supported for backward compatibility on x86, that mode of
operation can be treated as legacy.
So add a new Hyper-V stimer entry in the CPU hotplug state
list, and use that new state when in Direct Mode. Update the
Hyper-V clocksource driver to allocate and initialize stimer
clockevents earlier during boot. Update Hyper-V initialization
and the VMbus driver to use this new design. As a result,
the LAPIC timer is no longer used during boot or CPU
onlining/offlining and clockevents_unbind_device() is not
called. But retain the old design as a legacy implementation
for older versions of Hyper-V that don't support Direct Mode.
Signed-off-by: Michael Kelley <mikelley@microsoft.com>
---
Changes in v2 [all from Dexuan Cui]:
* Tweaked hv_stimer_alloc() so errors can be ignored on x86,
since a fallback to LAPIC timer clockevents will happen
* Fixed handling of return value from cpuhp_setup_state()
* Fixed hv_stimer_cleanup() to skip redundant call to
hv_ce_shutdown() when clockevents_unbind_device() is called
* Fixed hv_stimer_global_cleanup() to allow hv_stimer_free() and
the cpuhp state mechanism stop the stimer on all CPUs instead
of looping through the CPUs
* Re-added the call to hv_stimer_cleanup() in hv_crash_handler().
A separate patch will fix the problem that hv_synic_cleanup()
doesn't actually do anything in many cases because a channel
interrupt is assigned to the CPU.
---
arch/x86/hyperv/hv_init.c | 6 ++
drivers/clocksource/hyperv_timer.c | 154 +++++++++++++++++++++++++++++--------
drivers/hv/hv.c | 4 +-
drivers/hv/vmbus_drv.c | 30 ++++----
include/clocksource/hyperv_timer.h | 7 +-
include/linux/cpuhotplug.h | 1 +
6 files changed, 151 insertions(+), 51 deletions(-)
diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index b2daf0e..426dc8b 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -319,6 +319,12 @@ void __init hyperv_init(void)
hypercall_msr.guest_physical_address = vmalloc_to_pfn(hv_hypercall_pg);
wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
+ /*
+ * Ignore any errors in setting up stimer clockevents
+ * as we can run with the LAPIC timer as a fallback.
+ */
+ (void)hv_stimer_alloc();
+
hv_apic_init();
x86_init.pci.arch_init = hv_pci_init;
diff --git a/drivers/clocksource/hyperv_timer.c b/drivers/clocksource/hyperv_timer.c
index 2317d4e..9b99a79 100644
--- a/drivers/clocksource/hyperv_timer.c
+++ b/drivers/clocksource/hyperv_timer.c
@@ -17,6 +17,7 @@
#include <linux/clocksource.h>
#include <linux/sched_clock.h>
#include <linux/mm.h>
+#include <linux/cpuhotplug.h>
#include <clocksource/hyperv_timer.h>
#include <asm/hyperv-tlfs.h>
#include <asm/mshyperv.h>
@@ -30,6 +31,15 @@
* mechanism is used when running on older versions of Hyper-V
* that don't support Direct Mode. While Hyper-V provides
* four stimer's per CPU, Linux uses only stimer0.
+ *
+ * Because Direct Mode does not require processing a VMbus
+ * message, stimer interrupts can be enabled earlier in the
+ * process of booting a CPU, and consistent with when timer
+ * interrupts are enabled for other clocksource drivers.
+ * However, for legacy versions of Hyper-V when Direct Mode
+ * is not enabled, setting up stimer interrupts must be
+ * delayed until VMbus is initialized and can process the
+ * interrupt message.
*/
static bool direct_mode_enabled;
@@ -102,17 +112,12 @@ static int hv_ce_set_oneshot(struct clock_event_device *evt)
/*
* hv_stimer_init - Per-cpu initialization of the clockevent
*/
-void hv_stimer_init(unsigned int cpu)
+static int hv_stimer_init(unsigned int cpu)
{
struct clock_event_device *ce;
- /*
- * Synthetic timers are always available except on old versions of
- * Hyper-V on x86. In that case, just return as Linux will use a
- * clocksource based on emulated PIT or LAPIC timer hardware.
- */
- if (!(ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE))
- return;
+ if (!hv_clock_event)
+ return 0;
ce = per_cpu_ptr(hv_clock_event, cpu);
ce->name = "Hyper-V clockevent";
@@ -127,28 +132,55 @@ void hv_stimer_init(unsigned int cpu)
HV_CLOCK_HZ,
HV_MIN_DELTA_TICKS,
HV_MAX_MAX_DELTA_TICKS);
+ return 0;
}
-EXPORT_SYMBOL_GPL(hv_stimer_init);
/*
* hv_stimer_cleanup - Per-cpu cleanup of the clockevent
*/
-void hv_stimer_cleanup(unsigned int cpu)
+int hv_stimer_cleanup(unsigned int cpu)
{
struct clock_event_device *ce;
- /* Turn off clockevent device */
- if (ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE) {
- ce = per_cpu_ptr(hv_clock_event, cpu);
+ if (!hv_clock_event)
+ return 0;
+
+ /*
+ * In the legacy case where Direct Mode is not enabled
+ * (which can only be on x86/64), stimer cleanup happens
+ * relatively early in the CPU offlining process. We
+ * must unbind the stimer-based clockevent device so
+ * that the LAPIC timer can take over until clockevents
+ * are no longer needed in the offlining process. Note
+ * that clockevents_unbind_device() eventually calls
+ * hv_ce_shutdown().
+ *
+ * The unbind should not be done when Direct Mode is
+ * enabled because we may be on an architecture where
+ * there are no other clockevent devices to fallback to.
+ */
+ ce = per_cpu_ptr(hv_clock_event, cpu);
+ if (direct_mode_enabled)
hv_ce_shutdown(ce);
- }
+ else
+ clockevents_unbind_device(ce, cpu);
+
+ return 0;
}
EXPORT_SYMBOL_GPL(hv_stimer_cleanup);
/* hv_stimer_alloc - Global initialization of the clockevent and stimer0 */
-int hv_stimer_alloc(int sint)
+int hv_stimer_alloc(void)
{
- int ret;
+ int ret = 0;
+
+ /*
+ * Synthetic timers are always available except on old versions of
+ * Hyper-V on x86. In that case, return as error as Linux will use a
+ * clockevent based on emulated LAPIC timer hardware.
+ */
+ if (!(ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE))
+ return -EINVAL;
hv_clock_event = alloc_percpu(struct clock_event_device);
if (!hv_clock_event)
@@ -159,22 +191,78 @@ int hv_stimer_alloc(int sint)
if (direct_mode_enabled) {
ret = hv_setup_stimer0_irq(&stimer0_irq, &stimer0_vector,
hv_stimer0_isr);
- if (ret) {
- free_percpu(hv_clock_event);
- hv_clock_event = NULL;
- return ret;
- }
+ if (ret)
+ goto free_percpu;
+
+ /*
+ * Since we are in Direct Mode, stimer initialization
+ * can be done now with a CPUHP value in the same range
+ * as other clockevent devices.
+ */
+ ret = cpuhp_setup_state(CPUHP_AP_HYPERV_TIMER_STARTING,
+ "clockevents/hyperv/stimer:starting",
+ hv_stimer_init, hv_stimer_cleanup);
+ if (ret < 0)
+ goto free_stimer0_irq;
}
+ return ret;
- stimer0_message_sint = sint;
- return 0;
+free_stimer0_irq:
+ hv_remove_stimer0_irq(stimer0_irq);
+ stimer0_irq = 0;
+free_percpu:
+ free_percpu(hv_clock_event);
+ hv_clock_event = NULL;
+ return ret;
}
EXPORT_SYMBOL_GPL(hv_stimer_alloc);
+/*
+ * hv_stimer_legacy_init -- Called from the VMbus driver to handle
+ * the case when Direct Mode is not enabled, and the stimer
+ * must be initialized late in the CPU onlining process.
+ *
+ */
+void hv_stimer_legacy_init(unsigned int cpu, int sint)
+{
+ if (direct_mode_enabled)
+ return;
+
+ /*
+ * This function gets called by each vCPU, so setting the
+ * global stimer_message_sint value each time is conceptually
+ * not ideal, but the value passed in is always the same and
+ * it avoids introducing yet another interface into this
+ * clocksource driver just to set the sint in the legacy case.
+ */
+ stimer0_message_sint = sint;
+ (void)hv_stimer_init(cpu);
+}
+EXPORT_SYMBOL_GPL(hv_stimer_legacy_init);
+
+/*
+ * hv_stimer_legacy_cleanup -- Called from the VMbus driver to
+ * handle the case when Direct Mode is not enabled, and the
+ * stimer must be cleaned up early in the CPU offlining
+ * process.
+ */
+void hv_stimer_legacy_cleanup(unsigned int cpu)
+{
+ if (direct_mode_enabled)
+ return;
+ (void)hv_stimer_cleanup(cpu);
+}
+EXPORT_SYMBOL_GPL(hv_stimer_legacy_cleanup);
+
+
/* hv_stimer_free - Free global resources allocated by hv_stimer_alloc() */
void hv_stimer_free(void)
{
- if (direct_mode_enabled && (stimer0_irq != 0)) {
+ if (!hv_clock_event)
+ return;
+
+ if (direct_mode_enabled) {
+ cpuhp_remove_state(CPUHP_AP_HYPERV_TIMER_STARTING);
hv_remove_stimer0_irq(stimer0_irq);
stimer0_irq = 0;
}
@@ -190,14 +278,20 @@ void hv_stimer_free(void)
void hv_stimer_global_cleanup(void)
{
int cpu;
- struct clock_event_device *ce;
- if (ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE) {
- for_each_present_cpu(cpu) {
- ce = per_cpu_ptr(hv_clock_event, cpu);
- clockevents_unbind_device(ce, cpu);
- }
+ /*
+ * hv_stime_legacy_cleanup() will stop the stimer if Direct
+ * Mode is not enabled, and fallback to the LAPIC timer.
+ */
+ for_each_present_cpu(cpu) {
+ hv_stimer_legacy_cleanup(cpu);
}
+
+ /*
+ * If Direct Mode is enabled, the cpuhp teardown callback
+ * (hv_stimer_cleanup) will be run on all CPUs to stop the
+ * stimers.
+ */
hv_stimer_free();
}
EXPORT_SYMBOL_GPL(hv_stimer_global_cleanup);
diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
index fcc5279..6098e0c 100644
--- a/drivers/hv/hv.c
+++ b/drivers/hv/hv.c
@@ -202,7 +202,7 @@ int hv_synic_init(unsigned int cpu)
{
hv_synic_enable_regs(cpu);
- hv_stimer_init(cpu);
+ hv_stimer_legacy_init(cpu, VMBUS_MESSAGE_SINT);
return 0;
}
@@ -277,7 +277,7 @@ int hv_synic_cleanup(unsigned int cpu)
if (channel_found && vmbus_connection.conn_state == CONNECTED)
return -EBUSY;
- hv_stimer_cleanup(cpu);
+ hv_stimer_legacy_cleanup(cpu);
hv_synic_disable_regs(cpu);
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 5d2304b..664a415 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1342,10 +1342,6 @@ static int vmbus_bus_init(void)
if (ret)
goto err_alloc;
- ret = hv_stimer_alloc(VMBUS_MESSAGE_SINT);
- if (ret < 0)
- goto err_alloc;
-
/*
* Initialize the per-cpu interrupt state and stimer state.
* Then connect to the host.
@@ -1402,9 +1398,8 @@ static int vmbus_bus_init(void)
err_connect:
cpuhp_remove_state(hyperv_cpuhp_online);
err_cpuhp:
- hv_stimer_free();
-err_alloc:
hv_synic_free();
+err_alloc:
hv_remove_vmbus_irq();
bus_unregister(&hv_bus);
@@ -2317,20 +2312,23 @@ static void hv_crash_handler(struct pt_regs *regs)
static int hv_synic_suspend(void)
{
/*
- * When we reach here, all the non-boot CPUs have been offlined, and
- * the stimers on them have been unbound in hv_synic_cleanup() ->
+ * When we reach here, all the non-boot CPUs have been offlined.
+ * If we're in a legacy configuration where stimer Direct Mode is
+ * not enabled, the stimers on the non-boot CPUs have been unbound
+ * in hv_synic_cleanup() -> hv_stimer_legacy_cleanup() ->
* hv_stimer_cleanup() -> clockevents_unbind_device().
*
- * hv_synic_suspend() only runs on CPU0 with interrupts disabled. Here
- * we do not unbind the stimer on CPU0 because: 1) it's unnecessary
- * because the interrupts remain disabled between syscore_suspend()
- * and syscore_resume(): see create_image() and resume_target_kernel();
+ * hv_synic_suspend() only runs on CPU0 with interrupts disabled.
+ * Here we do not call hv_stimer_legacy_cleanup() on CPU0 because:
+ * 1) it's unnecessary as interrupts remain disabled between
+ * syscore_suspend() and syscore_resume(): see create_image() and
+ * resume_target_kernel()
* 2) the stimer on CPU0 is automatically disabled later by
* syscore_suspend() -> timekeeping_suspend() -> tick_suspend() -> ...
- * -> clockevents_shutdown() -> ... -> hv_ce_shutdown(); 3) a warning
- * would be triggered if we call clockevents_unbind_device(), which
- * may sleep, in an interrupts-disabled context. So, we intentionally
- * don't call hv_stimer_cleanup(0) here.
+ * -> clockevents_shutdown() -> ... -> hv_ce_shutdown()
+ * 3) a warning would be triggered if we call
+ * clockevents_unbind_device(), which may sleep, in an
+ * interrupts-disabled context.
*/
hv_synic_disable_regs(0);
diff --git a/include/clocksource/hyperv_timer.h b/include/clocksource/hyperv_timer.h
index 422f5e5..553e539 100644
--- a/include/clocksource/hyperv_timer.h
+++ b/include/clocksource/hyperv_timer.h
@@ -21,10 +21,11 @@
#define HV_MIN_DELTA_TICKS 1
/* Routines called by the VMbus driver */
-extern int hv_stimer_alloc(int sint);
+extern int hv_stimer_alloc(void);
extern void hv_stimer_free(void);
-extern void hv_stimer_init(unsigned int cpu);
-extern void hv_stimer_cleanup(unsigned int cpu);
+extern int hv_stimer_cleanup(unsigned int cpu);
+extern void hv_stimer_legacy_init(unsigned int cpu, int sint);
+extern void hv_stimer_legacy_cleanup(unsigned int cpu);
extern void hv_stimer_global_cleanup(void);
extern void hv_stimer0_isr(void);
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 89d75ed..e51ee77 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -129,6 +129,7 @@ enum cpuhp_state {
CPUHP_AP_ARC_TIMER_STARTING,
CPUHP_AP_RISCV_TIMER_STARTING,
CPUHP_AP_CSKY_TIMER_STARTING,
+ CPUHP_AP_HYPERV_TIMER_STARTING,
CPUHP_AP_KVM_STARTING,
CPUHP_AP_KVM_ARM_VGIC_INIT_STARTING,
CPUHP_AP_KVM_ARM_VGIC_STARTING,
--
1.8.3.1
^ permalink raw reply related
* RE: [PATCH net-next 11/14] vsock: add multi-transports support
From: Jorgen Hansen @ 2019-11-13 14:30 UTC (permalink / raw)
To: 'Stefano Garzarella'
Cc: netdev@vger.kernel.org, Michael S. Tsirkin, kvm@vger.kernel.org,
Greg Kroah-Hartman, Jason Wang, David S. Miller, Dexuan Cui,
Haiyang Zhang, Sasha Levin, linux-kernel@vger.kernel.org,
Arnd Bergmann, Stefan Hajnoczi, linux-hyperv@vger.kernel.org,
K. Y. Srinivasan, Stephen Hemminger,
virtualization@lists.linux-foundation.org
In-Reply-To: <20191112103630.vd3kbk7xnplv6rey@steredhat>
> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> Sent: Tuesday, November 12, 2019 11:37 AM
> > > > You already mentioned that you are working on a fix for loopback
> > > > here for the guest, but presumably a host could also do loopback.
> > >
> > > IIUC we don't support loopback in the host, because in this case the
> > > application will use the CID_HOST as address, but if we are in a nested
> > > VM environment we are in trouble.
> >
> > If both src and dst CID are CID_HOST, we should be fairly sure that this
> > Is host loopback, no? If src is anything else, we would do G2H.
> >
>
> The problem is that we don't know the src until we assign a transport
> looking at the dst. (or if the user bound the socket to CID_HOST before
> the connect(), but it is not very common)
>
> So if we are in a L1 and the user uses the local guest CID, it works, but if
> it uses the HOST_CID, the packet will go to the L0.
>
> If we are in L0, it could be simple, because we can simply check if G2H
> is not loaded, so any packet to CID_HOST, is host loopback.
>
> I think that if the user uses the IOCTL_VM_SOCKETS_GET_LOCAL_CID, to set
> the dest CID for the loopback, it works in both cases because we return the
> HOST_CID in L0, and always the guest CID in L1, also if a H2G is loaded to
> handle the L2.
>
> Maybe we should document this in the man page.
Yeah, it seems like a good idea to flesh out the routing behavior for nested
VMs in the man page.
>
> But I have a question: Does vmci support the host loopback?
> I've tried, and it seems not.
Only for datagrams - not for stream sockets.
> Also vhost-vsock doesn't support it, but virtio-vsock does.
>
> > >
> > > Since several people asked about this feature at the KVM Forum, I would
> like
> > > to add a new VMADDR_CID_LOCAL (i.e. using the reserved 1) and
> implement
> > > loopback in the core.
> > >
> > > What do you think?
> >
> > What kind of use cases are mentioned in the KVM forum for loopback?
> One concern
> > is that we have to maintain yet another interprocess communication
> mechanism,
> > even though other choices exist already (and those are likely to be more
> efficient
> > given the development time and specific focus that went into those). To
> me, the
> > local connections are mainly useful as a way to sanity test the protocol and
> transports.
> > However, if loopback is compelling, it would make sense have it in the core,
> since it
> > shouldn't need a specific transport.
>
> The common use cases is for developer point of view, and to test the
> protocol and transports as you said.
>
> People that are introducing VSOCK support in their projects, would like to
> test them on their own PC without starting a VM.
>
> The idea is to move the code to handle loopback from the virtio-vsock,
> in the core, but in another series :-)
OK, that makes sense.
Thanks,
Jorgen
^ permalink raw reply
* Re: [PATCH v1] tools/hv: async name resolution in kvp_daemon
From: Olaf Hering @ 2019-11-13 15:39 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin,
open list:Hyper-V CORE AND DRIVERS, open list
In-Reply-To: <87wocbagqc.fsf@vitty.brq.redhat.com>
[-- Attachment #1: Type: text/plain, Size: 535 bytes --]
Am Thu, 07 Nov 2019 15:44:27 +0100
schrieb Vitaly Kuznetsov <vkuznets@redhat.com>:
> I *think* what you're aiming at is EAI_AGAIN and EAI_FAIL, the rest
> should probably terminate the resolver thread (e.g. AF_INET is
> unsupported or something like that).
The thread aims for success. Resolving of the hostname can take some time.
Maybe the network is not fully functional when the thread starts.
Maybe the VM admin does further tweaks while the thread is running.
IMO there is no downside to wait for success.
Olaf
[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2] tools/hv: async name resolution in kvp_daemon
From: Olaf Hering @ 2019-11-13 16:00 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger
Cc: Sasha Levin, open list:Hyper-V CORE AND DRIVERS, open list
In-Reply-To: <20191113155400.25456-1-olaf@aepfle.de>
[-- Attachment #1: Type: text/plain, Size: 558 bytes --]
Am Wed, 13 Nov 2019 16:54:00 +0100
schrieb Olaf Hering <olaf@aepfle.de>:
> During this time all "FullyQualifiedDomainName" requests will be
> answered with an empty string.
> + strcpy(key_value, full_domain_name ? : "");
> strcpy(key_name, "FullyQualifiedDomainName");
KY, Haiyang, Stephen,
please check if the host side can actually handle such temporary, empty string.
If the tools cache the first result, the implemented approach can not work.
If that is indeed the case, is there some sort of -ERETRY in the protocol?
Olaf
[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v1] tools/hv: add .gitignore
From: Olaf Hering @ 2019-11-13 16:02 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin,
open list, open list:Hyper-V CORE AND DRIVERS
Cc: Olaf Hering
Hide build artefacts in "git status" output.
Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
tools/hv/.gitignore | 3 +++
1 file changed, 3 insertions(+)
create mode 100644 tools/hv/.gitignore
diff --git a/tools/hv/.gitignore b/tools/hv/.gitignore
new file mode 100644
index 000000000000..88b05e35a6e0
--- /dev/null
+++ b/tools/hv/.gitignore
@@ -0,0 +1,3 @@
+hv_fcopy_daemon
+hv_kvp_daemon
+hv_vss_daemon
^ permalink raw reply related
* [PATCH v2] tools/hv: async name resolution in kvp_daemon
From: Olaf Hering @ 2019-11-13 15:54 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin,
open list:Hyper-V CORE AND DRIVERS, open list
Cc: Olaf Hering
The hostname is resolved just once since commit 58125210ab3b ("Tools:
hv: cache FQDN in kvp_daemon to avoid timeouts") to make sure the VM
responds within the timeout limits to requests from the host.
If for some reason getaddrinfo fails, the string returned by the
"FullyQualifiedDomainName" request contains some error string, which is
then used by tools on the host side.
Adjust the code to resolve the current hostname in a separate thread.
This thread loops until getaddrinfo returns success. During this time
all "FullyQualifiedDomainName" requests will be answered with an empty
string.
Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
v2:
- link with -pthread instead of -lpthread
- consider some errors from getaddrinfo fatal, log them via syslog
- update also hostname in the loop
tools/hv/Makefile | 2 ++
tools/hv/hv_kvp_daemon.c | 83 ++++++++++++++++++++++++++++++++++--------------
2 files changed, 62 insertions(+), 23 deletions(-)
diff --git a/tools/hv/Makefile b/tools/hv/Makefile
index b57143d9459c..9bbab96ac06d 100644
--- a/tools/hv/Makefile
+++ b/tools/hv/Makefile
@@ -22,6 +22,8 @@ ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS))
ALL_SCRIPTS := hv_get_dhcp_info.sh hv_get_dns_info.sh hv_set_ifconfig.sh
+$(OUTPUT)hv_kvp_daemon: LDFLAGS += -pthread
+
all: $(ALL_PROGRAMS)
export srctree OUTPUT CC LD CFLAGS
diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index e9ef4ca6a655..b930506a9632 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -41,6 +41,7 @@
#include <net/if.h>
#include <limits.h>
#include <getopt.h>
+#include <pthread.h>
/*
* KVP protocol: The user mode component first registers with the
@@ -85,7 +86,7 @@ static char *processor_arch;
static char *os_build;
static char *os_version;
static char *lic_version = "Unknown version";
-static char full_domain_name[HV_KVP_EXCHANGE_MAX_VALUE_SIZE];
+static char *full_domain_name;
static struct utsname uts_buf;
/*
@@ -1327,27 +1328,67 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
return error;
}
-
-static void
-kvp_get_domain_name(char *buffer, int length)
+/*
+ * Async retrival of Fully Qualified Domain Name because getaddrinfo takes an
+ * unpredictable amount of time to finish.
+ */
+static void *kvp_getaddrinfo(void *p)
{
- struct addrinfo hints, *info ;
- int error = 0;
+ char *tmp, **str_ptr = (char **)p;
+ char hostname[HOST_NAME_MAX + 1];
+ struct addrinfo *info, hints = {
+ .ai_family = AF_INET, /* Get only ipv4 addrinfo. */
+ .ai_socktype = SOCK_STREAM,
+ .ai_flags = AI_CANONNAME,
+ };
+ int ret;
- gethostname(buffer, length);
- memset(&hints, 0, sizeof(hints));
- hints.ai_family = AF_INET; /*Get only ipv4 addrinfo. */
- hints.ai_socktype = SOCK_STREAM;
- hints.ai_flags = AI_CANONNAME;
+ do {
+ if (gethostname(hostname, sizeof(hostname) - 1) < 0)
+ goto out;
- error = getaddrinfo(buffer, NULL, &hints, &info);
- if (error != 0) {
- snprintf(buffer, length, "getaddrinfo failed: 0x%x %s",
- error, gai_strerror(error));
+ ret = getaddrinfo(hostname, NULL, &hints, &info);
+ switch (ret) {
+ case 0:
+ break;
+ case EAI_BADFLAGS:
+ case EAI_MEMORY:
+ case EAI_OVERFLOW:
+ case EAI_SOCKTYPE:
+ case EAI_SYSTEM:
+ /* Permanent failure */
+ syslog(LOG_ERR, "getaddrinfo failed: %d %s",
+ ret, gai_strerror(ret));
+ goto out;
+ default:
+ /* Temporary failure, aim for success. */
+ sleep(1);
+ }
+ } while (ret);
+
+ ret = asprintf(&tmp, "%s", info->ai_canonname);
+ freeaddrinfo(info);
+ if (ret <= 0)
+ goto out;
+
+ if (ret > HV_KVP_EXCHANGE_MAX_VALUE_SIZE)
+ tmp[HV_KVP_EXCHANGE_MAX_VALUE_SIZE - 1] = '\0';
+ *str_ptr = tmp;
+
+out:
+ pthread_exit(NULL);
+}
+
+static void kvp_obtain_domain_name(char **str_ptr)
+{
+ pthread_t t;
+
+ if (pthread_create(&t, NULL, kvp_getaddrinfo, str_ptr)) {
+ syslog(LOG_ERR, "pthread_create failed; error: %d %s",
+ errno, strerror(errno));
return;
}
- snprintf(buffer, length, "%s", info->ai_canonname);
- freeaddrinfo(info);
+ pthread_detach(t);
}
void print_usage(char *argv[])
@@ -1412,11 +1453,7 @@ int main(int argc, char *argv[])
* Retrieve OS release information.
*/
kvp_get_os_info();
- /*
- * Cache Fully Qualified Domain Name because getaddrinfo takes an
- * unpredictable amount of time to finish.
- */
- kvp_get_domain_name(full_domain_name, sizeof(full_domain_name));
+ kvp_obtain_domain_name(&full_domain_name);
if (kvp_file_init()) {
syslog(LOG_ERR, "Failed to initialize the pools");
@@ -1571,7 +1608,7 @@ int main(int argc, char *argv[])
switch (hv_msg->body.kvp_enum_data.index) {
case FullyQualifiedDomainName:
- strcpy(key_value, full_domain_name);
+ strcpy(key_value, full_domain_name ? : "");
strcpy(key_name, "FullyQualifiedDomainName");
break;
case IntegrationServicesVersion:
^ permalink raw reply related
* Re: [PATCH net-next 11/14] vsock: add multi-transports support
From: Stefano Garzarella @ 2019-11-13 16:38 UTC (permalink / raw)
To: Jorgen Hansen
Cc: netdev@vger.kernel.org, Michael S. Tsirkin, kvm@vger.kernel.org,
Greg Kroah-Hartman, Jason Wang, David S. Miller, Dexuan Cui,
Haiyang Zhang, Sasha Levin, linux-kernel@vger.kernel.org,
Arnd Bergmann, Stefan Hajnoczi, linux-hyperv@vger.kernel.org,
K. Y. Srinivasan, Stephen Hemminger,
virtualization@lists.linux-foundation.org
In-Reply-To: <MWHPR05MB3376560CFD2A710723843828DA760@MWHPR05MB3376.namprd05.prod.outlook.com>
On Wed, Nov 13, 2019 at 02:30:24PM +0000, Jorgen Hansen wrote:
> > From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> > Sent: Tuesday, November 12, 2019 11:37 AM
>
> > > > > You already mentioned that you are working on a fix for loopback
> > > > > here for the guest, but presumably a host could also do loopback.
> > > >
> > > > IIUC we don't support loopback in the host, because in this case the
> > > > application will use the CID_HOST as address, but if we are in a nested
> > > > VM environment we are in trouble.
> > >
> > > If both src and dst CID are CID_HOST, we should be fairly sure that this
> > > Is host loopback, no? If src is anything else, we would do G2H.
> > >
> >
> > The problem is that we don't know the src until we assign a transport
> > looking at the dst. (or if the user bound the socket to CID_HOST before
> > the connect(), but it is not very common)
> >
> > So if we are in a L1 and the user uses the local guest CID, it works, but if
> > it uses the HOST_CID, the packet will go to the L0.
> >
> > If we are in L0, it could be simple, because we can simply check if G2H
> > is not loaded, so any packet to CID_HOST, is host loopback.
> >
> > I think that if the user uses the IOCTL_VM_SOCKETS_GET_LOCAL_CID, to set
> > the dest CID for the loopback, it works in both cases because we return the
> > HOST_CID in L0, and always the guest CID in L1, also if a H2G is loaded to
> > handle the L2.
> >
> > Maybe we should document this in the man page.
>
> Yeah, it seems like a good idea to flesh out the routing behavior for nested
> VMs in the man page.
I'll do it.
>
> >
> > But I have a question: Does vmci support the host loopback?
> > I've tried, and it seems not.
>
> Only for datagrams - not for stream sockets.
>
Ok, I'll leave the datagram loopback as before.
> > Also vhost-vsock doesn't support it, but virtio-vsock does.
> >
> > > >
> > > > Since several people asked about this feature at the KVM Forum, I would
> > like
> > > > to add a new VMADDR_CID_LOCAL (i.e. using the reserved 1) and
> > implement
> > > > loopback in the core.
> > > >
> > > > What do you think?
> > >
> > > What kind of use cases are mentioned in the KVM forum for loopback?
> > One concern
> > > is that we have to maintain yet another interprocess communication
> > mechanism,
> > > even though other choices exist already (and those are likely to be more
> > efficient
> > > given the development time and specific focus that went into those). To
> > me, the
> > > local connections are mainly useful as a way to sanity test the protocol and
> > transports.
> > > However, if loopback is compelling, it would make sense have it in the core,
> > since it
> > > shouldn't need a specific transport.
> >
> > The common use cases is for developer point of view, and to test the
> > protocol and transports as you said.
> >
> > People that are introducing VSOCK support in their projects, would like to
> > test them on their own PC without starting a VM.
> >
> > The idea is to move the code to handle loopback from the virtio-vsock,
> > in the core, but in another series :-)
>
> OK, that makes sense.
Thanks,
Stefano
^ permalink raw reply
* [PATCH 1/1] Drivers: hv: vmbus: Fix crash handler reset of Hyper-V synic
From: Michael Kelley @ 2019-11-14 6:32 UTC (permalink / raw)
To: linux-kernel@vger.kernel.org, vkuznets, KY Srinivasan,
Stephen Hemminger, sashal@kernel.org, Dexuan Cui,
linux-hyperv@vger.kernel.org
Cc: Michael Kelley
The crash handler calls hv_synic_cleanup() to shutdown the
Hyper-V synthetic interrupt controller. But if the CPU
that calls hv_synic_cleanup() has a VMbus channel interrupt
assigned to it (which is likely the case in smaller VM sizes),
hv_synic_cleanup() returns an error and the synthetic
interrupt controller isn't shutdown. While the lack of
being shutdown hasn't caused a known problem, it still
should be fixed for highest reliability.
So directly call hv_synic_disable_regs() instead of
hv_synic_cleanup(), which ensures that the synic is always
shutdown.
Signed-off-by: Michael Kelley <mikelley@microsoft.com>
---
drivers/hv/vmbus_drv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 664a415..665920d 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -2305,7 +2305,7 @@ static void hv_crash_handler(struct pt_regs *regs)
vmbus_connection.conn_state = DISCONNECTED;
cpu = smp_processor_id();
hv_stimer_cleanup(cpu);
- hv_synic_cleanup(cpu);
+ hv_synic_disable_regs(cpu);
hyperv_cleanup();
};
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next v2 00/15] vsock: add multi-transports support
From: Stefano Garzarella @ 2019-11-14 9:57 UTC (permalink / raw)
To: netdev
Cc: Stephen Hemminger, Arnd Bergmann, Jorgen Hansen, Jason Wang,
Greg Kroah-Hartman, linux-kernel, Michael S. Tsirkin,
Haiyang Zhang, Stefan Hajnoczi, David S. Miller, virtualization,
kvm, Sasha Levin, K. Y. Srinivasan, Dexuan Cui, linux-hyperv
Most of the patches are reviewed by Dexuan, Stefan, and Jorgen.
The following patches need reviews:
- [11/15] vsock: add multi-transports support
- [12/15] vsock/vmci: register vmci_transport only when VMCI guest/host
are active
- [15/15] vhost/vsock: refuse CID assigned to the guest->host transport
RFC: https://patchwork.ozlabs.org/cover/1168442/
v1: https://patchwork.ozlabs.org/cover/1181986/
v1 -> v2:
- Patch 11:
+ vmci_transport: sent reset when vsock_assign_transport() fails
[Jorgen]
+ fixed loopback in the guests, checking if the remote_addr is the
same of transport_g2h->get_local_cid()
+ virtio_transport_common: updated space available while creating
the new child socket during a connection request
- Patch 12:
+ removed 'features' variable in vmci_transport_init() [Stefan]
+ added a flag to register only once the host [Jorgen]
- Added patch 15 to refuse CID assigned to the guest->host transport in
the vhost_transport
This series adds the multi-transports support to vsock, following
this proposal: https://www.spinics.net/lists/netdev/msg575792.html
With the multi-transports support, we can use VSOCK with nested VMs
(using also different hypervisors) loading both guest->host and
host->guest transports at the same time.
Before this series, vmci_transport supported this behavior but only
using VMware hypervisor on L0, L1, etc.
The first 9 patches are cleanups and preparations, maybe some of
these can go regardless of this series.
Patch 10 changes the hvs_remote_addr_init(). setting the
VMADDR_CID_HOST as remote CID instead of VMADDR_CID_ANY to make
the choice of transport to be used work properly.
Patch 11 adds multi-transports support.
Patch 12 changes a little bit the vmci_transport and the vmci driver
to register the vmci_transport only when there are active host/guest.
Patch 13 prevents the transport modules unloading while sockets are
assigned to them.
Patch 14 fixes an issue in the bind() logic discoverable only with
the new multi-transport support.
Patch 15 refuses CID assigned to the guest->host transport in the
vhost_transport.
I've tested this series with nested KVM (vsock-transport [L0,L1],
virtio-transport[L1,L2]) and with VMware (L0) + KVM (L1)
(vmci-transport [L0,L1], vhost-transport [L1], virtio-transport[L2]).
Dexuan successfully tested the RFC series on HyperV with a Linux guest.
Stefano Garzarella (15):
vsock/vmci: remove unused VSOCK_DEFAULT_CONNECT_TIMEOUT
vsock: remove vm_sockets_get_local_cid()
vsock: remove include/linux/vm_sockets.h file
vsock: add 'transport' member in the struct vsock_sock
vsock/virtio: add transport parameter to the
virtio_transport_reset_no_sock()
vsock: add 'struct vsock_sock *' param to vsock_core_get_transport()
vsock: handle buffer_size sockopts in the core
vsock: add vsock_create_connected() called by transports
vsock: move vsock_insert_unbound() in the vsock_create()
hv_sock: set VMADDR_CID_HOST in the hvs_remote_addr_init()
vsock: add multi-transports support
vsock/vmci: register vmci_transport only when VMCI guest/host are
active
vsock: prevent transport modules unloading
vsock: fix bind() behaviour taking care of CID
vhost/vsock: refuse CID assigned to the guest->host transport
drivers/misc/vmw_vmci/vmci_driver.c | 67 +++++
drivers/misc/vmw_vmci/vmci_driver.h | 2 +
drivers/misc/vmw_vmci/vmci_guest.c | 2 +
drivers/misc/vmw_vmci/vmci_host.c | 7 +
drivers/vhost/vsock.c | 102 ++++---
include/linux/virtio_vsock.h | 18 +-
include/linux/vm_sockets.h | 15 -
include/linux/vmw_vmci_api.h | 2 +
include/net/af_vsock.h | 45 +--
include/net/vsock_addr.h | 2 +-
net/vmw_vsock/af_vsock.c | 382 ++++++++++++++++++------
net/vmw_vsock/hyperv_transport.c | 70 ++---
net/vmw_vsock/virtio_transport.c | 177 ++++++-----
net/vmw_vsock/virtio_transport_common.c | 166 +++++-----
net/vmw_vsock/vmci_transport.c | 140 ++++-----
net/vmw_vsock/vmci_transport.h | 3 -
net/vmw_vsock/vmci_transport_notify.h | 1 -
17 files changed, 679 insertions(+), 522 deletions(-)
delete mode 100644 include/linux/vm_sockets.h
--
2.21.0
^ 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