* RE: [PATCH v2 4/4] hv_utils: Add the support of hibernation
From: Michael Kelley @ 2020-01-22 18:27 UTC (permalink / raw)
To: Dexuan Cui, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, Sasha Levin, linux-hyperv@vger.kernel.org,
vkuznets, linux-kernel@vger.kernel.org
In-Reply-To: <HK0P153MB0148B7D12E62DD559A5D2B9DBF350@HK0P153MB0148.APCP153.PROD.OUTLOOK.COM>
From: Dexuan Cui <decui@microsoft.com> Sent: Sunday, January 12, 2020 10:32 PM
>
> Add util_pre_suspend() and util_pre_resume() for some hv_utils devices
> (e.g. kvp/vss/fcopy), because they need special handling before
> util_suspend() calls vmbus_close().
>
> For kvp, all the possible pending work items should be cancelled.
>
> For vss and fcopy, extra clean-up needs to be done, i.e. fake a THAW
> message for hv_vss_daemon and fake a CANCEL_FCOPY message for
> hv_fcopy_daemonemon, otherwise when the VM resums back, the daemons
> can end up in an inconsistent state (i.e. the file systems are
> frozen but will never be thawed; the file transmitted via fcopy
> may not be complete). Note: there is an extra patch for the daemons:
> "Tools: hv: Reopen the devices if read() or write() returns errors",
> because the hv_utils driver can not guarantee the whole transaction
> finishes completely once util_suspend() starts to run (at this time,
> all the userspace processes are frozen).
>
> util_probe() disables channel->callback_event to avoid the race with
> the the channel callback.
>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
> drivers/hv/hv_fcopy.c | 58 ++++++++++++++++++++++++++++++++++++-
> drivers/hv/hv_kvp.c | 44 ++++++++++++++++++++++++++--
> drivers/hv/hv_snapshot.c | 60 +++++++++++++++++++++++++++++++++++++--
> drivers/hv/hv_util.c | 60 ++++++++++++++++++++++++++++++++++++++-
> drivers/hv/hyperv_vmbus.h | 6 ++++
> include/linux/hyperv.h | 2 ++
> 6 files changed, 224 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hv/hv_fcopy.c b/drivers/hv/hv_fcopy.c
> index 08fa4a5de644..d63853f16356 100644
> --- a/drivers/hv/hv_fcopy.c
> +++ b/drivers/hv/hv_fcopy.c
> @@ -346,9 +346,65 @@ int hv_fcopy_init(struct hv_util_service *srv)
> return 0;
> }
>
> +static void hv_fcopy_cancel_work(void)
> +{
> + cancel_delayed_work_sync(&fcopy_timeout_work);
> + cancel_work_sync(&fcopy_send_work);
> +}
> +
> +int hv_fcopy_pre_suspend(void)
> +{
> + struct vmbus_channel *channel = fcopy_transaction.recv_channel;
> + struct hv_fcopy_hdr *fcopy_msg;
> +
> + tasklet_disable(&channel->callback_event);
> +
> + /*
> + * Fake a CANCEL_FCOPY message to the user space daemon in case the
> + * daemon is in the middle of copying some file. It doesn't matter if
> + * there is already a message pending to be delivered to the user
> + * space: we force fcopy_transaction.state to be HVUTIL_READY, so the
> + * user space daemon's write() will fail with -EINVAL (see
> + * fcopy_on_msg()), and the daemon will reset the device by closing and
> + * re-opening it.
> + */
> + fcopy_msg = kzalloc(sizeof(*fcopy_msg), GFP_KERNEL);
> + if (!fcopy_msg)
> + goto err;
> +
> + fcopy_msg->operation = CANCEL_FCOPY;
> +
> + hv_fcopy_cancel_work();
> +
> + /* We don't care about the return value. */
> + hvutil_transport_send(hvt, fcopy_msg, sizeof(*fcopy_msg), NULL);
> +
> + kfree(fcopy_msg);
> +
> + fcopy_transaction.state = HVUTIL_READY;
Is the ordering correct here? It seems like the fcopy daemon could receive
the cancel message and do the write before the state is forced to
HVUTIL_READY.
> +
> + /* tasklet_enable() will be called in hv_fcopy_pre_resume(). */
> +
> + return 0;
> +err:
> + tasklet_enable(&channel->callback_event);
A nit, but if you did the memory allocation first, you could return -ENOMEM
immediately on error and avoid the err: label and re-enabling the tasklet.
> + return -ENOMEM;
> +}
> +
> +int hv_fcopy_pre_resume(void)
> +{
> + struct vmbus_channel *channel = fcopy_transaction.recv_channel;
> +
> + tasklet_enable(&channel->callback_event);
> +
> + return 0;
> +}
> +
> void hv_fcopy_deinit(void)
> {
> fcopy_transaction.state = HVUTIL_DEVICE_DYING;
> - cancel_delayed_work_sync(&fcopy_timeout_work);
> +
> + hv_fcopy_cancel_work();
> +
> hvutil_transport_destroy(hvt);
> }
> diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
> index ae7c028dc5a8..ca03f68df5d0 100644
> --- a/drivers/hv/hv_kvp.c
> +++ b/drivers/hv/hv_kvp.c
> @@ -758,11 +758,51 @@ hv_kvp_init(struct hv_util_service *srv)
> return 0;
> }
>
> -void hv_kvp_deinit(void)
> +static void hv_kvp_cancel_work(void)
> {
> - kvp_transaction.state = HVUTIL_DEVICE_DYING;
> cancel_delayed_work_sync(&kvp_host_handshake_work);
> cancel_delayed_work_sync(&kvp_timeout_work);
> cancel_work_sync(&kvp_sendkey_work);
> +}
> +
> +int hv_kvp_pre_suspend(void)
> +{
> + struct vmbus_channel *channel = kvp_transaction.recv_channel;
> +
> + tasklet_disable(&channel->callback_event);
> +
> + /*
> + * If there is a pending transtion, it's unnecessary to tell the host
> + * that the tranction will fail, becasue that is implied when
> + * util_suspend() calls vmbus_close() later.
> + */
> + hv_kvp_cancel_work();
> +
> + /*
> + * Forece the state to READY to handle the ICMSGTYPE_NEGOTIATE message
> + * later. The user space daemon may go out of order and its write()
> + * may get an EINVAL error: this doesn't matter since the daemon will
> + * reset the device by closing and re-opening the device.
> + */
> + kvp_transaction.state = HVUTIL_READY;
> +
> + return 0;
> +}
> +
> +int hv_kvp_pre_resume(void)
> +{
> + struct vmbus_channel *channel = kvp_transaction.recv_channel;
> +
> + tasklet_enable(&channel->callback_event);
> +
> + return 0;
> +}
> +
> +void hv_kvp_deinit(void)
> +{
> + kvp_transaction.state = HVUTIL_DEVICE_DYING;
> +
> + hv_kvp_cancel_work();
> +
> hvutil_transport_destroy(hvt);
> }
> diff --git a/drivers/hv/hv_snapshot.c b/drivers/hv/hv_snapshot.c
> index 03b6454268b3..eb766ff8841b 100644
> --- a/drivers/hv/hv_snapshot.c
> +++ b/drivers/hv/hv_snapshot.c
> @@ -229,6 +229,7 @@ static void vss_handle_request(struct work_struct *dummy)
> vss_transaction.state = HVUTIL_HOSTMSG_RECEIVED;
> vss_send_op();
> return;
> +
Gratuitous blank line added?
> case VSS_OP_GET_DM_INFO:
> vss_transaction.msg->dm_info.flags = 0;
> break;
> @@ -379,10 +380,65 @@ hv_vss_init(struct hv_util_service *srv)
> return 0;
> }
>
> -void hv_vss_deinit(void)
> +static void hv_vss_cancel_work(void)
> {
> - vss_transaction.state = HVUTIL_DEVICE_DYING;
> cancel_delayed_work_sync(&vss_timeout_work);
> cancel_work_sync(&vss_handle_request_work);
> +}
> +
> +int hv_vss_pre_suspend(void)
> +{
> + struct vmbus_channel *channel = vss_transaction.recv_channel;
> + struct hv_vss_msg *vss_msg;
> +
> + tasklet_disable(&channel->callback_event);
> +
> + /*
> + * Fake a THAW message for the user space daemon in case the daemon
> + * has frozen the file systems. It doesn't matter if there is already
> + * a message pending to be delivered to the user space: we force
> + * vss_transaction.state to be HVUTIL_READY, so the user space daemon's
> + * write() will fail with -EINVAL (see vss_on_msg()), and the daemon
> + * will reset the device by closing and re-opening it.
> + */
> + vss_msg = kzalloc(sizeof(*vss_msg), GFP_KERNEL);
> + if (!vss_msg)
> + goto err;
> +
> + vss_msg->vss_hdr.operation = VSS_OP_THAW;
> +
> + /* Cancel any possible pending work. */
> + hv_vss_cancel_work();
> +
> + /* We don't care about the return value. */
> + hvutil_transport_send(hvt, vss_msg, sizeof(*vss_msg), NULL);
> +
> + kfree(vss_msg);
> +
> + vss_transaction.state = HVUTIL_READY;
Same comment about the ordering.
> +
> + /* tasklet_enable() will be called in hv_vss_pre_resume(). */
> +
> + return 0;
> +err:
> + tasklet_enable(&channel->callback_event);
> + return -ENOMEM;
Same comment about simplifying the error handling applies here.
> +}
> +
> +int hv_vss_pre_resume(void)
> +{
> + struct vmbus_channel *channel = vss_transaction.recv_channel;
> +
> + tasklet_enable(&channel->callback_event);
> +
> + return 0;
> +}
> +
> +void hv_vss_deinit(void)
> +{
> + vss_transaction.state = HVUTIL_DEVICE_DYING;
> +
> + hv_vss_cancel_work();
> +
> hvutil_transport_destroy(hvt);
> }
> diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
> index d5216af62788..255faa3d657c 100644
> --- a/drivers/hv/hv_util.c
> +++ b/drivers/hv/hv_util.c
> @@ -123,12 +123,14 @@ static struct hv_util_service util_shutdown = {
> };
>
> static int hv_timesync_init(struct hv_util_service *srv);
> +static int hv_timesync_pre_suspend(void);
> static void hv_timesync_deinit(void);
>
> static void timesync_onchannelcallback(void *context);
> static struct hv_util_service util_timesynch = {
> .util_cb = timesync_onchannelcallback,
> .util_init = hv_timesync_init,
> + .util_pre_suspend = hv_timesync_pre_suspend,
> .util_deinit = hv_timesync_deinit,
> };
>
> @@ -140,18 +142,24 @@ static struct hv_util_service util_heartbeat = {
> static struct hv_util_service util_kvp = {
> .util_cb = hv_kvp_onchannelcallback,
> .util_init = hv_kvp_init,
> + .util_pre_suspend = hv_kvp_pre_suspend,
> + .util_pre_resume = hv_kvp_pre_resume,
> .util_deinit = hv_kvp_deinit,
> };
>
> static struct hv_util_service util_vss = {
> .util_cb = hv_vss_onchannelcallback,
> .util_init = hv_vss_init,
> + .util_pre_suspend = hv_vss_pre_suspend,
> + .util_pre_resume = hv_vss_pre_resume,
> .util_deinit = hv_vss_deinit,
> };
>
> static struct hv_util_service util_fcopy = {
> .util_cb = hv_fcopy_onchannelcallback,
> .util_init = hv_fcopy_init,
> + .util_pre_suspend = hv_fcopy_pre_suspend,
> + .util_pre_resume = hv_fcopy_pre_resume,
> .util_deinit = hv_fcopy_deinit,
> };
>
> @@ -512,6 +520,41 @@ static int util_remove(struct hv_device *dev)
> return 0;
> }
>
> +static int util_suspend(struct hv_device *dev)
> +{
> + struct hv_util_service *srv = hv_get_drvdata(dev);
> + int ret = 0;
> +
> + if (srv->util_pre_suspend) {
> + ret = srv->util_pre_suspend();
> +
Unneeded blank line?
> + if (ret)
> + return ret;
> + }
> +
> + vmbus_close(dev->channel);
> +
> + return 0;
> +}
> +
> +static int util_resume(struct hv_device *dev)
> +{
> + struct hv_util_service *srv = hv_get_drvdata(dev);
> + int ret = 0;
> +
> + if (srv->util_pre_resume) {
> + ret = srv->util_pre_resume();
> +
Unneeded blank line?
> + if (ret)
> + return ret;
> + }
> +
> + ret = vmbus_open(dev->channel, 4 * HV_HYP_PAGE_SIZE,
> + 4 * HV_HYP_PAGE_SIZE, NULL, 0, srv->util_cb,
> + dev->channel);
> + return ret;
> +}
> +
> static const struct hv_vmbus_device_id id_table[] = {
> /* Shutdown guid */
> { HV_SHUTDOWN_GUID,
> @@ -548,6 +591,8 @@ static struct hv_driver util_drv = {
> .id_table = id_table,
> .probe = util_probe,
> .remove = util_remove,
> + .suspend = util_suspend,
> + .resume = util_resume,
> .driver = {
> .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> },
> @@ -617,11 +662,24 @@ static int hv_timesync_init(struct hv_util_service *srv)
> return 0;
> }
>
> +static void hv_timesync_cancel_work(void)
> +{
> + cancel_work_sync(&adj_time_work);
> +}
> +
> +static int hv_timesync_pre_suspend(void)
> +{
> + hv_timesync_cancel_work();
> +
> + return 0;
> +}
> +
> static void hv_timesync_deinit(void)
> {
> if (hv_ptp_clock)
> ptp_clock_unregister(hv_ptp_clock);
> - cancel_work_sync(&adj_time_work);
> +
> + hv_timesync_cancel_work();
> }
>
> static int __init init_hyperv_utils(void)
> diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
> index 20edcfd3b96c..f5fa3b3c9baf 100644
> --- a/drivers/hv/hyperv_vmbus.h
> +++ b/drivers/hv/hyperv_vmbus.h
> @@ -352,14 +352,20 @@ void vmbus_on_msg_dpc(unsigned long data);
>
> int hv_kvp_init(struct hv_util_service *srv);
> void hv_kvp_deinit(void);
> +int hv_kvp_pre_suspend(void);
> +int hv_kvp_pre_resume(void);
> void hv_kvp_onchannelcallback(void *context);
>
> int hv_vss_init(struct hv_util_service *srv);
> void hv_vss_deinit(void);
> +int hv_vss_pre_suspend(void);
> +int hv_vss_pre_resume(void);
> void hv_vss_onchannelcallback(void *context);
>
> int hv_fcopy_init(struct hv_util_service *srv);
> void hv_fcopy_deinit(void);
> +int hv_fcopy_pre_suspend(void);
> +int hv_fcopy_pre_resume(void);
> void hv_fcopy_onchannelcallback(void *context);
> void vmbus_initiate_unload(bool crash);
>
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 41c58011431e..692c89ccf5df 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -1435,6 +1435,8 @@ struct hv_util_service {
> void (*util_cb)(void *);
> int (*util_init)(struct hv_util_service *);
> void (*util_deinit)(void);
> + int (*util_pre_suspend)(void);
> + int (*util_pre_resume)(void);
> };
>
> struct vmbuspipe_hdr {
> --
> 2.19.1
^ permalink raw reply
* RE: [PATCH v2 3/4] hv_utils: Support host-initiated hibernation request
From: Michael Kelley @ 2020-01-22 17:39 UTC (permalink / raw)
To: Dexuan Cui, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, Sasha Levin, linux-hyperv@vger.kernel.org,
vkuznets, linux-kernel@vger.kernel.org
In-Reply-To: <HK0P153MB0148FDF9A3AF2352544E6DADBF350@HK0P153MB0148.APCP153.PROD.OUTLOOK.COM>
From: Dexuan Cui <decui@microsoft.com> Sent: Sunday, January 12, 2020 10:32 PM
>
> Update the Shutdown IC version to 3.2, which is required for the host to
> send the hibernation request.
>
> The user is expected to create the below udev rule file, which is applied
> upon the host-initiated hibernation request:
>
> root@localhost:~# cat /usr/lib/udev/rules.d/40-vm-hibernation.rules
> SUBSYSTEM=="vmbus", ACTION=="change", DRIVER=="hv_utils",
> ENV{EVENT}=="hibernate", RUN+="/usr/bin/systemctl hibernate"
>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
> drivers/hv/hv_util.c | 52 +++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
> index fe3a316380c2..d5216af62788 100644
> --- a/drivers/hv/hv_util.c
> +++ b/drivers/hv/hv_util.c
> @@ -25,7 +25,9 @@
> #define SD_MAJOR 3
> #define SD_MINOR 0
> #define SD_MINOR_1 1
> +#define SD_MINOR_2 2
> #define SD_VERSION_3_1 (SD_MAJOR << 16 | SD_MINOR_1)
> +#define SD_VERSION_3_2 (SD_MAJOR << 16 | SD_MINOR_2)
> #define SD_VERSION (SD_MAJOR << 16 | SD_MINOR)
>
> #define SD_MAJOR_1 1
> @@ -52,9 +54,10 @@ static int sd_srv_version;
> static int ts_srv_version;
> static int hb_srv_version;
>
> -#define SD_VER_COUNT 3
> +#define SD_VER_COUNT 4
> static const int sd_versions[] = {
> SD_VERSION_3_1,
> + SD_VERSION_3_2,
I think these versions need to listed in descending order, so the new
SD_VERSION_3_2 should be listed first. Otherwise a Hyper-V host that
supports both 3.1 and 3.2 might match on 3.1 first without ever checking
for a match with 3.2.
> SD_VERSION,
> SD_VERSION_1
> };
> @@ -78,9 +81,45 @@ static const int fw_versions[] = {
> UTIL_WS2K8_FW_VERSION
> };
>
> +/*
> + * Send the "hibernate" udev event in a thread context.
> + */
> +struct hibernate_work_context {
> + struct work_struct work;
> + struct hv_device *dev;
> +};
> +
> +static struct hibernate_work_context hibernate_context;
> +static bool execute_hibernate;
> +
> +static void send_hibernate_uevent(struct work_struct *work)
> +{
> + char *uevent_env[2] = { "EVENT=hibernate", NULL };
> + struct hibernate_work_context *ctx;
> +
> + ctx = container_of(work, struct hibernate_work_context, work);
> +
> + kobject_uevent_env(&ctx->dev->device.kobj, KOBJ_CHANGE, uevent_env);
> +
> + pr_info("Sent hibernation uevent\n");
> +}
> +
> +static int hv_shutdown_init(struct hv_util_service *srv)
> +{
> + struct vmbus_channel *channel = srv->channel;
> +
> + INIT_WORK(&hibernate_context.work, send_hibernate_uevent);
> + hibernate_context.dev = channel->device_obj;
> +
> + execute_hibernate = hv_is_hibernation_supported();
> +
> + return 0;
> +}
> +
> static void shutdown_onchannelcallback(void *context);
> static struct hv_util_service util_shutdown = {
> .util_cb = shutdown_onchannelcallback,
> + .util_init = hv_shutdown_init,
> };
>
> static int hv_timesync_init(struct hv_util_service *srv);
> @@ -187,6 +226,17 @@ static void shutdown_onchannelcallback(void *context)
>
> schedule_work(&restart_work);
> break;
> + case 4:
> + case 5:
As before, I'm wondering about the interpretation of these numbers.
> + pr_info("Hibernation request received\n");
> +
> + if (execute_hibernate) {
> + icmsghdrp->status = HV_S_OK;
> + schedule_work(&hibernate_context.work);
Same comment here about the ordering of the schedule_work() call and the
sending of the response message. Seems like the code should be consistent
for all three cases -- shutdown, restart, and hibernate.
> + } else {
> + icmsghdrp->status = HV_E_FAIL;
> + }
> + break;
> default:
> icmsghdrp->status = HV_E_FAIL;
> execute_shutdown = false;
> --
> 2.19.1
^ permalink raw reply
* [PATCH V3,net-next, 1/2] hv_netvsc: Add XDP support
From: Haiyang Zhang @ 2020-01-22 17:23 UTC (permalink / raw)
To: sashal, linux-hyperv, netdev
Cc: haiyangz, kys, sthemmin, olaf, vkuznets, davem, linux-kernel
In-Reply-To: <1579713814-36061-1-git-send-email-haiyangz@microsoft.com>
This patch adds support of XDP in native mode for hv_netvsc driver, and
transparently sets the XDP program on the associated VF NIC as well.
Setting / unsetting XDP program on synthetic NIC (netvsc) propagates to
VF NIC automatically. Setting / unsetting XDP program on VF NIC directly
is not recommended, also not propagated to synthetic NIC, and may be
overwritten by setting of synthetic NIC.
The Azure/Hyper-V synthetic NIC receive buffer doesn't provide headroom
for XDP. We thought about re-use the RNDIS header space, but it's too
small. So we decided to copy the packets to a page buffer for XDP. And,
most of our VMs on Azure have Accelerated Network (SRIOV) enabled, so
most of the packets run on VF NIC. The synthetic NIC is considered as a
fallback data-path. So the data copy on netvsc won't impact performance
significantly.
XDP program cannot run with LRO (RSC) enabled, so you need to disable LRO
before running XDP:
ethtool -K eth0 lro off
XDP actions not yet supported:
XDP_REDIRECT
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
Changes:
v3: Minor code and comment updates.
v2: Added XDP_TX support. Addressed review comments.
---
drivers/net/hyperv/Makefile | 2 +-
drivers/net/hyperv/hyperv_net.h | 21 +++-
drivers/net/hyperv/netvsc.c | 31 +++++-
drivers/net/hyperv/netvsc_bpf.c | 209 ++++++++++++++++++++++++++++++++++++++
drivers/net/hyperv/netvsc_drv.c | 175 +++++++++++++++++++++++++------
drivers/net/hyperv/rndis_filter.c | 2 +-
6 files changed, 401 insertions(+), 39 deletions(-)
create mode 100644 drivers/net/hyperv/netvsc_bpf.c
diff --git a/drivers/net/hyperv/Makefile b/drivers/net/hyperv/Makefile
index 3a2aa07..0db7cca 100644
--- a/drivers/net/hyperv/Makefile
+++ b/drivers/net/hyperv/Makefile
@@ -1,4 +1,4 @@
# SPDX-License-Identifier: GPL-2.0-only
obj-$(CONFIG_HYPERV_NET) += hv_netvsc.o
-hv_netvsc-y := netvsc_drv.o netvsc.o rndis_filter.o netvsc_trace.o
+hv_netvsc-y := netvsc_drv.o netvsc.o rndis_filter.o netvsc_trace.o netvsc_bpf.o
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index dc44819..abda736 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -142,6 +142,8 @@ struct netvsc_device_info {
u32 send_section_size;
u32 recv_section_size;
+ struct bpf_prog *bprog;
+
u8 rss_key[NETVSC_HASH_KEYLEN];
};
@@ -189,7 +191,8 @@ int netvsc_send(struct net_device *net,
struct hv_netvsc_packet *packet,
struct rndis_message *rndis_msg,
struct hv_page_buffer *page_buffer,
- struct sk_buff *skb);
+ struct sk_buff *skb,
+ bool xdp_tx);
void netvsc_linkstatus_callback(struct net_device *net,
struct rndis_message *resp);
int netvsc_recv_callback(struct net_device *net,
@@ -198,6 +201,16 @@ int netvsc_recv_callback(struct net_device *net,
void netvsc_channel_cb(void *context);
int netvsc_poll(struct napi_struct *napi, int budget);
+u32 netvsc_run_xdp(struct net_device *ndev, struct netvsc_channel *nvchan,
+ struct xdp_buff *xdp);
+unsigned int netvsc_xdp_fraglen(unsigned int len);
+struct bpf_prog *netvsc_xdp_get(struct netvsc_device *nvdev);
+int netvsc_xdp_set(struct net_device *dev, struct bpf_prog *prog,
+ struct netlink_ext_ack *extack,
+ struct netvsc_device *nvdev);
+int netvsc_vf_setxdp(struct net_device *vf_netdev, struct bpf_prog *prog);
+int netvsc_bpf(struct net_device *dev, struct netdev_bpf *bpf);
+
int rndis_set_subchannel(struct net_device *ndev,
struct netvsc_device *nvdev,
struct netvsc_device_info *dev_info);
@@ -832,6 +845,8 @@ struct nvsp_message {
#define RNDIS_MAX_PKT_DEFAULT 8
#define RNDIS_PKT_ALIGN_DEFAULT 8
+#define NETVSC_XDP_HDRM 256
+
struct multi_send_data {
struct sk_buff *skb; /* skb containing the pkt */
struct hv_netvsc_packet *pkt; /* netvsc pkt pending */
@@ -867,6 +882,7 @@ struct netvsc_stats {
u64 bytes;
u64 broadcast;
u64 multicast;
+ u64 xdp_drop;
struct u64_stats_sync syncp;
};
@@ -972,6 +988,9 @@ struct netvsc_channel {
atomic_t queue_sends;
struct nvsc_rsc rsc;
+ struct bpf_prog __rcu *bpf_prog;
+ struct xdp_rxq_info xdp_rxq;
+
struct netvsc_stats tx_stats;
struct netvsc_stats rx_stats;
};
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index eab83e7..ae3f308 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -122,8 +122,10 @@ static void free_netvsc_device(struct rcu_head *head)
vfree(nvdev->send_buf);
kfree(nvdev->send_section_map);
- for (i = 0; i < VRSS_CHANNEL_MAX; i++)
+ for (i = 0; i < VRSS_CHANNEL_MAX; i++) {
+ xdp_rxq_info_unreg(&nvdev->chan_table[i].xdp_rxq);
vfree(nvdev->chan_table[i].mrc.slots);
+ }
kfree(nvdev);
}
@@ -900,7 +902,8 @@ int netvsc_send(struct net_device *ndev,
struct hv_netvsc_packet *packet,
struct rndis_message *rndis_msg,
struct hv_page_buffer *pb,
- struct sk_buff *skb)
+ struct sk_buff *skb,
+ bool xdp_tx)
{
struct net_device_context *ndev_ctx = netdev_priv(ndev);
struct netvsc_device *net_device
@@ -923,10 +926,11 @@ int netvsc_send(struct net_device *ndev,
packet->send_buf_index = NETVSC_INVALID_INDEX;
packet->cp_partial = false;
- /* Send control message directly without accessing msd (Multi-Send
- * Data) field which may be changed during data packet processing.
+ /* Send a control message or XDP packet directly without accessing
+ * msd (Multi-Send Data) field which may be changed during data packet
+ * processing.
*/
- if (!skb)
+ if (!skb || xdp_tx)
return netvsc_send_pkt(device, packet, net_device, pb, skb);
/* batch packets in send buffer if possible */
@@ -1392,6 +1396,21 @@ struct netvsc_device *netvsc_device_add(struct hv_device *device,
nvchan->net_device = net_device;
u64_stats_init(&nvchan->tx_stats.syncp);
u64_stats_init(&nvchan->rx_stats.syncp);
+
+ ret = xdp_rxq_info_reg(&nvchan->xdp_rxq, ndev, i);
+
+ if (ret) {
+ netdev_err(ndev, "xdp_rxq_info_reg fail: %d\n", ret);
+ goto cleanup2;
+ }
+
+ ret = xdp_rxq_info_reg_mem_model(&nvchan->xdp_rxq,
+ MEM_TYPE_PAGE_SHARED, NULL);
+
+ if (ret) {
+ netdev_err(ndev, "xdp reg_mem_model fail: %d\n", ret);
+ goto cleanup2;
+ }
}
/* Enable NAPI handler before init callbacks */
@@ -1437,6 +1456,8 @@ struct netvsc_device *netvsc_device_add(struct hv_device *device,
cleanup:
netif_napi_del(&net_device->chan_table[0].napi);
+
+cleanup2:
free_netvsc_device(&net_device->rcu);
return ERR_PTR(ret);
diff --git a/drivers/net/hyperv/netvsc_bpf.c b/drivers/net/hyperv/netvsc_bpf.c
new file mode 100644
index 0000000..20adfe5
--- /dev/null
+++ b/drivers/net/hyperv/netvsc_bpf.c
@@ -0,0 +1,209 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (c) 2019, Microsoft Corporation.
+ *
+ * Author:
+ * Haiyang Zhang <haiyangz@microsoft.com>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/netdevice.h>
+#include <linux/etherdevice.h>
+#include <linux/ethtool.h>
+#include <linux/bpf.h>
+#include <linux/bpf_trace.h>
+#include <linux/kernel.h>
+#include <net/xdp.h>
+
+#include <linux/mutex.h>
+#include <linux/rtnetlink.h>
+
+#include "hyperv_net.h"
+
+u32 netvsc_run_xdp(struct net_device *ndev, struct netvsc_channel *nvchan,
+ struct xdp_buff *xdp)
+{
+ void *data = nvchan->rsc.data[0];
+ u32 len = nvchan->rsc.len[0];
+ struct page *page = NULL;
+ struct bpf_prog *prog;
+ u32 act = XDP_PASS;
+
+ xdp->data_hard_start = NULL;
+
+ rcu_read_lock();
+ prog = rcu_dereference(nvchan->bpf_prog);
+
+ if (!prog)
+ goto out;
+
+ /* allocate page buffer for data */
+ page = alloc_page(GFP_ATOMIC);
+ if (!page) {
+ act = XDP_DROP;
+ goto out;
+ }
+
+ xdp->data_hard_start = page_address(page);
+ xdp->data = xdp->data_hard_start + NETVSC_XDP_HDRM;
+ xdp_set_data_meta_invalid(xdp);
+ xdp->data_end = xdp->data + len;
+ xdp->rxq = &nvchan->xdp_rxq;
+ xdp->handle = 0;
+
+ memcpy(xdp->data, data, len);
+
+ act = bpf_prog_run_xdp(prog, xdp);
+
+ switch (act) {
+ case XDP_PASS:
+ case XDP_TX:
+ case XDP_DROP:
+ break;
+
+ case XDP_ABORTED:
+ trace_xdp_exception(ndev, prog, act);
+ break;
+
+ default:
+ bpf_warn_invalid_xdp_action(act);
+ }
+
+out:
+ rcu_read_unlock();
+
+ if (page && act != XDP_PASS && act != XDP_TX) {
+ __free_page(page);
+ xdp->data_hard_start = NULL;
+ }
+
+ return act;
+}
+
+unsigned int netvsc_xdp_fraglen(unsigned int len)
+{
+ return SKB_DATA_ALIGN(len) +
+ SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
+}
+
+struct bpf_prog *netvsc_xdp_get(struct netvsc_device *nvdev)
+{
+ return rtnl_dereference(nvdev->chan_table[0].bpf_prog);
+}
+
+int netvsc_xdp_set(struct net_device *dev, struct bpf_prog *prog,
+ struct netlink_ext_ack *extack,
+ struct netvsc_device *nvdev)
+{
+ struct bpf_prog *old_prog;
+ int buf_max, i;
+
+ old_prog = netvsc_xdp_get(nvdev);
+
+ if (!old_prog && !prog)
+ return 0;
+
+ buf_max = NETVSC_XDP_HDRM + netvsc_xdp_fraglen(dev->mtu + ETH_HLEN);
+ if (prog && buf_max > PAGE_SIZE) {
+ netdev_err(dev, "XDP: mtu:%u too large, buf_max:%u\n",
+ dev->mtu, buf_max);
+ NL_SET_ERR_MSG_MOD(extack, "XDP: mtu too large");
+
+ return -EOPNOTSUPP;
+ }
+
+ if (prog && (dev->features & NETIF_F_LRO)) {
+ netdev_err(dev, "XDP: not support LRO\n");
+ NL_SET_ERR_MSG_MOD(extack, "XDP: not support LRO");
+
+ return -EOPNOTSUPP;
+ }
+
+ if (prog)
+ bpf_prog_add(prog, nvdev->num_chn);
+
+ for (i = 0; i < nvdev->num_chn; i++)
+ rcu_assign_pointer(nvdev->chan_table[i].bpf_prog, prog);
+
+ if (old_prog)
+ for (i = 0; i < nvdev->num_chn; i++)
+ bpf_prog_put(old_prog);
+
+ return 0;
+}
+
+int netvsc_vf_setxdp(struct net_device *vf_netdev, struct bpf_prog *prog)
+{
+ struct netdev_bpf xdp;
+ bpf_op_t ndo_bpf;
+
+ ASSERT_RTNL();
+
+ if (!vf_netdev)
+ return 0;
+
+ ndo_bpf = vf_netdev->netdev_ops->ndo_bpf;
+ if (!ndo_bpf)
+ return 0;
+
+ memset(&xdp, 0, sizeof(xdp));
+
+ xdp.command = XDP_SETUP_PROG;
+ xdp.prog = prog;
+
+ return ndo_bpf(vf_netdev, &xdp);
+}
+
+static u32 netvsc_xdp_query(struct netvsc_device *nvdev)
+{
+ struct bpf_prog *prog = netvsc_xdp_get(nvdev);
+
+ if (prog)
+ return prog->aux->id;
+
+ return 0;
+}
+
+int netvsc_bpf(struct net_device *dev, struct netdev_bpf *bpf)
+{
+ struct net_device_context *ndevctx = netdev_priv(dev);
+ struct netvsc_device *nvdev = rtnl_dereference(ndevctx->nvdev);
+ struct net_device *vf_netdev = rtnl_dereference(ndevctx->vf_netdev);
+ struct netlink_ext_ack *extack = bpf->extack;
+ int ret;
+
+ if (!nvdev || nvdev->destroy) {
+ if (bpf->command == XDP_QUERY_PROG) {
+ bpf->prog_id = 0;
+ return 0; /* Query must always succeed */
+ } else {
+ return -ENODEV;
+ }
+ }
+
+ switch (bpf->command) {
+ case XDP_SETUP_PROG:
+ ret = netvsc_xdp_set(dev, bpf->prog, extack, nvdev);
+
+ if (ret)
+ return ret;
+
+ ret = netvsc_vf_setxdp(vf_netdev, bpf->prog);
+
+ if (ret) {
+ netdev_err(dev, "vf_setxdp failed:%d\n", ret);
+ NL_SET_ERR_MSG_MOD(extack, "vf_setxdp failed");
+
+ netvsc_xdp_set(dev, NULL, extack, nvdev);
+ }
+
+ return ret;
+
+ case XDP_QUERY_PROG:
+ bpf->prog_id = netvsc_xdp_query(nvdev);
+ return 0;
+
+ default:
+ return -EINVAL;
+ }
+}
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index f3f9eb8..bb1ace4 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -25,6 +25,7 @@
#include <linux/slab.h>
#include <linux/rtnetlink.h>
#include <linux/netpoll.h>
+#include <linux/bpf.h>
#include <net/arp.h>
#include <net/route.h>
@@ -519,7 +520,7 @@ static int netvsc_vf_xmit(struct net_device *net, struct net_device *vf_netdev,
return rc;
}
-static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
+static int netvsc_xmit(struct sk_buff *skb, struct net_device *net, bool xdp_tx)
{
struct net_device_context *net_device_ctx = netdev_priv(net);
struct hv_netvsc_packet *packet = NULL;
@@ -686,7 +687,7 @@ static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
/* timestamp packet in software */
skb_tx_timestamp(skb);
- ret = netvsc_send(net, packet, rndis_msg, pb, skb);
+ ret = netvsc_send(net, packet, rndis_msg, pb, skb, xdp_tx);
if (likely(ret == 0))
return NETDEV_TX_OK;
@@ -709,6 +710,11 @@ static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
goto drop;
}
+static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *ndev)
+{
+ return netvsc_xmit(skb, ndev, false);
+}
+
/*
* netvsc_linkstatus_callback - Link up/down notification
*/
@@ -751,6 +757,14 @@ void netvsc_linkstatus_callback(struct net_device *net,
schedule_delayed_work(&ndev_ctx->dwork, 0);
}
+static int netvsc_xdp_xmit(struct sk_buff *skb, struct net_device *ndev)
+{
+ skb->queue_mapping = skb_get_rx_queue(skb);
+ __skb_push(skb, ETH_HLEN);
+
+ return netvsc_xmit(skb, ndev, true);
+}
+
static void netvsc_comp_ipcsum(struct sk_buff *skb)
{
struct iphdr *iph = (struct iphdr *)skb->data;
@@ -760,7 +774,8 @@ static void netvsc_comp_ipcsum(struct sk_buff *skb)
}
static struct sk_buff *netvsc_alloc_recv_skb(struct net_device *net,
- struct netvsc_channel *nvchan)
+ struct netvsc_channel *nvchan,
+ struct xdp_buff *xdp)
{
struct napi_struct *napi = &nvchan->napi;
const struct ndis_pkt_8021q_info *vlan = nvchan->rsc.vlan;
@@ -768,18 +783,37 @@ static struct sk_buff *netvsc_alloc_recv_skb(struct net_device *net,
nvchan->rsc.csum_info;
const u32 *hash_info = nvchan->rsc.hash_info;
struct sk_buff *skb;
+ void *xbuf = xdp->data_hard_start;
int i;
- skb = napi_alloc_skb(napi, nvchan->rsc.pktlen);
- if (!skb)
- return skb;
+ if (xbuf) {
+ unsigned int hdroom = xdp->data - xdp->data_hard_start;
+ unsigned int xlen = xdp->data_end - xdp->data;
+ unsigned int frag_size = netvsc_xdp_fraglen(hdroom + xlen);
- /*
- * Copy to skb. This copy is needed here since the memory pointed by
- * hv_netvsc_packet cannot be deallocated
- */
- for (i = 0; i < nvchan->rsc.cnt; i++)
- skb_put_data(skb, nvchan->rsc.data[i], nvchan->rsc.len[i]);
+ skb = build_skb(xbuf, frag_size);
+
+ if (!skb) {
+ __free_page(virt_to_page(xbuf));
+ return NULL;
+ }
+
+ skb_reserve(skb, hdroom);
+ skb_put(skb, xlen);
+ skb->dev = napi->dev;
+ } else {
+ skb = napi_alloc_skb(napi, nvchan->rsc.pktlen);
+
+ if (!skb)
+ return NULL;
+
+ /* Copy to skb. This copy is needed here since the memory
+ * pointed by hv_netvsc_packet cannot be deallocated.
+ */
+ for (i = 0; i < nvchan->rsc.cnt; i++)
+ skb_put_data(skb, nvchan->rsc.data[i],
+ nvchan->rsc.len[i]);
+ }
skb->protocol = eth_type_trans(skb, net);
@@ -829,13 +863,25 @@ int netvsc_recv_callback(struct net_device *net,
struct vmbus_channel *channel = nvchan->channel;
u16 q_idx = channel->offermsg.offer.sub_channel_index;
struct sk_buff *skb;
- struct netvsc_stats *rx_stats;
+ struct netvsc_stats *rx_stats = &nvchan->rx_stats;
+ struct xdp_buff xdp;
+ u32 act;
if (net->reg_state != NETREG_REGISTERED)
return NVSP_STAT_FAIL;
+ act = netvsc_run_xdp(net, nvchan, &xdp);
+
+ if (act != XDP_PASS && act != XDP_TX) {
+ u64_stats_update_begin(&rx_stats->syncp);
+ rx_stats->xdp_drop++;
+ u64_stats_update_end(&rx_stats->syncp);
+
+ return NVSP_STAT_SUCCESS; /* consumed by XDP */
+ }
+
/* Allocate a skb - TODO direct I/O to pages? */
- skb = netvsc_alloc_recv_skb(net, nvchan);
+ skb = netvsc_alloc_recv_skb(net, nvchan, &xdp);
if (unlikely(!skb)) {
++net_device_ctx->eth_stats.rx_no_memory;
@@ -849,7 +895,6 @@ int netvsc_recv_callback(struct net_device *net,
* on the synthetic device because modifying the VF device
* statistics will not work correctly.
*/
- rx_stats = &nvchan->rx_stats;
u64_stats_update_begin(&rx_stats->syncp);
rx_stats->packets++;
rx_stats->bytes += nvchan->rsc.pktlen;
@@ -860,6 +905,11 @@ int netvsc_recv_callback(struct net_device *net,
++rx_stats->multicast;
u64_stats_update_end(&rx_stats->syncp);
+ if (act == XDP_TX) {
+ netvsc_xdp_xmit(skb, net);
+ return NVSP_STAT_SUCCESS;
+ }
+
napi_gro_receive(&nvchan->napi, skb);
return NVSP_STAT_SUCCESS;
}
@@ -886,10 +936,11 @@ static void netvsc_get_channels(struct net_device *net,
/* Alloc struct netvsc_device_info, and initialize it from either existing
* struct netvsc_device, or from default values.
*/
-static struct netvsc_device_info *netvsc_devinfo_get
- (struct netvsc_device *nvdev)
+static
+struct netvsc_device_info *netvsc_devinfo_get(struct netvsc_device *nvdev)
{
struct netvsc_device_info *dev_info;
+ struct bpf_prog *prog;
dev_info = kzalloc(sizeof(*dev_info), GFP_ATOMIC);
@@ -897,6 +948,8 @@ static void netvsc_get_channels(struct net_device *net,
return NULL;
if (nvdev) {
+ ASSERT_RTNL();
+
dev_info->num_chn = nvdev->num_chn;
dev_info->send_sections = nvdev->send_section_cnt;
dev_info->send_section_size = nvdev->send_section_size;
@@ -905,6 +958,12 @@ static void netvsc_get_channels(struct net_device *net,
memcpy(dev_info->rss_key, nvdev->extension->rss_key,
NETVSC_HASH_KEYLEN);
+
+ prog = netvsc_xdp_get(nvdev);
+ if (prog) {
+ bpf_prog_inc(prog);
+ dev_info->bprog = prog;
+ }
} else {
dev_info->num_chn = VRSS_CHANNEL_DEFAULT;
dev_info->send_sections = NETVSC_DEFAULT_TX;
@@ -916,6 +975,17 @@ static void netvsc_get_channels(struct net_device *net,
return dev_info;
}
+/* Free struct netvsc_device_info */
+static void netvsc_devinfo_put(struct netvsc_device_info *dev_info)
+{
+ if (dev_info->bprog) {
+ ASSERT_RTNL();
+ bpf_prog_put(dev_info->bprog);
+ }
+
+ kfree(dev_info);
+}
+
static int netvsc_detach(struct net_device *ndev,
struct netvsc_device *nvdev)
{
@@ -927,6 +997,8 @@ static int netvsc_detach(struct net_device *ndev,
if (cancel_work_sync(&nvdev->subchan_work))
nvdev->num_chn = 1;
+ netvsc_xdp_set(ndev, NULL, NULL, nvdev);
+
/* If device was up (receiving) then shutdown */
if (netif_running(ndev)) {
netvsc_tx_disable(nvdev, ndev);
@@ -960,7 +1032,8 @@ static int netvsc_attach(struct net_device *ndev,
struct hv_device *hdev = ndev_ctx->device_ctx;
struct netvsc_device *nvdev;
struct rndis_device *rdev;
- int ret;
+ struct bpf_prog *prog;
+ int ret = 0;
nvdev = rndis_filter_device_add(hdev, dev_info);
if (IS_ERR(nvdev))
@@ -976,6 +1049,13 @@ static int netvsc_attach(struct net_device *ndev,
}
}
+ prog = dev_info->bprog;
+ if (prog) {
+ ret = netvsc_xdp_set(ndev, prog, NULL, nvdev);
+ if (ret)
+ goto err1;
+ }
+
/* In any case device is now ready */
netif_device_attach(ndev);
@@ -985,7 +1065,7 @@ static int netvsc_attach(struct net_device *ndev,
if (netif_running(ndev)) {
ret = rndis_filter_open(nvdev);
if (ret)
- goto err;
+ goto err2;
rdev = nvdev->extension;
if (!rdev->link_state)
@@ -994,9 +1074,10 @@ static int netvsc_attach(struct net_device *ndev,
return 0;
-err:
+err2:
netif_device_detach(ndev);
+err1:
rndis_filter_device_remove(hdev, nvdev);
return ret;
@@ -1046,7 +1127,7 @@ static int netvsc_set_channels(struct net_device *net,
}
out:
- kfree(device_info);
+ netvsc_devinfo_put(device_info);
return ret;
}
@@ -1153,7 +1234,7 @@ static int netvsc_change_mtu(struct net_device *ndev, int mtu)
dev_set_mtu(vf_netdev, orig_mtu);
out:
- kfree(device_info);
+ netvsc_devinfo_put(device_info);
return ret;
}
@@ -1378,8 +1459,8 @@ static int netvsc_set_mac_addr(struct net_device *ndev, void *p)
/* statistics per queue (rx/tx packets/bytes) */
#define NETVSC_PCPU_STATS_LEN (num_present_cpus() * ARRAY_SIZE(pcpu_stats))
-/* 4 statistics per queue (rx/tx packets/bytes) */
-#define NETVSC_QUEUE_STATS_LEN(dev) ((dev)->num_chn * 4)
+/* 5 statistics per queue (rx/tx packets/bytes, rx xdp_drop) */
+#define NETVSC_QUEUE_STATS_LEN(dev) ((dev)->num_chn * 5)
static int netvsc_get_sset_count(struct net_device *dev, int string_set)
{
@@ -1411,6 +1492,7 @@ static void netvsc_get_ethtool_stats(struct net_device *dev,
struct netvsc_ethtool_pcpu_stats *pcpu_sum;
unsigned int start;
u64 packets, bytes;
+ u64 xdp_drop;
int i, j, cpu;
if (!nvdev)
@@ -1439,9 +1521,11 @@ static void netvsc_get_ethtool_stats(struct net_device *dev,
start = u64_stats_fetch_begin_irq(&qstats->syncp);
packets = qstats->packets;
bytes = qstats->bytes;
+ xdp_drop = qstats->xdp_drop;
} while (u64_stats_fetch_retry_irq(&qstats->syncp, start));
data[i++] = packets;
data[i++] = bytes;
+ data[i++] = xdp_drop;
}
pcpu_sum = kvmalloc_array(num_possible_cpus(),
@@ -1489,6 +1573,8 @@ static void netvsc_get_strings(struct net_device *dev, u32 stringset, u8 *data)
p += ETH_GSTRING_LEN;
sprintf(p, "rx_queue_%u_bytes", i);
p += ETH_GSTRING_LEN;
+ sprintf(p, "rx_queue_%u_xdp_drop", i);
+ p += ETH_GSTRING_LEN;
}
for_each_present_cpu(cpu) {
@@ -1785,10 +1871,27 @@ static int netvsc_set_ringparam(struct net_device *ndev,
}
out:
- kfree(device_info);
+ netvsc_devinfo_put(device_info);
return ret;
}
+static netdev_features_t netvsc_fix_features(struct net_device *ndev,
+ netdev_features_t features)
+{
+ struct net_device_context *ndevctx = netdev_priv(ndev);
+ struct netvsc_device *nvdev = rtnl_dereference(ndevctx->nvdev);
+
+ if (!nvdev || nvdev->destroy)
+ return features;
+
+ if ((features & NETIF_F_LRO) && netvsc_xdp_get(nvdev)) {
+ features ^= NETIF_F_LRO;
+ netdev_info(ndev, "Skip LRO - unsupported with XDP\n");
+ }
+
+ return features;
+}
+
static int netvsc_set_features(struct net_device *ndev,
netdev_features_t features)
{
@@ -1875,12 +1978,14 @@ static void netvsc_set_msglevel(struct net_device *ndev, u32 val)
.ndo_start_xmit = netvsc_start_xmit,
.ndo_change_rx_flags = netvsc_change_rx_flags,
.ndo_set_rx_mode = netvsc_set_rx_mode,
+ .ndo_fix_features = netvsc_fix_features,
.ndo_set_features = netvsc_set_features,
.ndo_change_mtu = netvsc_change_mtu,
.ndo_validate_addr = eth_validate_addr,
.ndo_set_mac_address = netvsc_set_mac_addr,
.ndo_select_queue = netvsc_select_queue,
.ndo_get_stats64 = netvsc_get_stats64,
+ .ndo_bpf = netvsc_bpf,
};
/*
@@ -2167,6 +2272,7 @@ static int netvsc_register_vf(struct net_device *vf_netdev)
{
struct net_device_context *net_device_ctx;
struct netvsc_device *netvsc_dev;
+ struct bpf_prog *prog;
struct net_device *ndev;
int ret;
@@ -2211,6 +2317,9 @@ static int netvsc_register_vf(struct net_device *vf_netdev)
vf_netdev->wanted_features = ndev->features;
netdev_update_features(vf_netdev);
+ prog = netvsc_xdp_get(netvsc_dev);
+ netvsc_vf_setxdp(vf_netdev, prog);
+
return NOTIFY_OK;
}
@@ -2252,6 +2361,8 @@ static int netvsc_unregister_vf(struct net_device *vf_netdev)
netdev_info(ndev, "VF unregistering: %s\n", vf_netdev->name);
+ netvsc_vf_setxdp(vf_netdev, NULL);
+
netdev_rx_handler_unregister(vf_netdev);
netdev_upper_dev_unlink(vf_netdev, ndev);
RCU_INIT_POINTER(net_device_ctx->vf_netdev, NULL);
@@ -2363,14 +2474,14 @@ static int netvsc_probe(struct hv_device *dev,
list_add(&net_device_ctx->list, &netvsc_dev_list);
rtnl_unlock();
- kfree(device_info);
+ netvsc_devinfo_put(device_info);
return 0;
register_failed:
rtnl_unlock();
rndis_filter_device_remove(dev, nvdev);
rndis_failed:
- kfree(device_info);
+ netvsc_devinfo_put(device_info);
devinfo_failed:
free_percpu(net_device_ctx->vf_stats);
no_stats:
@@ -2398,8 +2509,10 @@ static int netvsc_remove(struct hv_device *dev)
rtnl_lock();
nvdev = rtnl_dereference(ndev_ctx->nvdev);
- if (nvdev)
+ if (nvdev) {
cancel_work_sync(&nvdev->subchan_work);
+ netvsc_xdp_set(net, NULL, NULL, nvdev);
+ }
/*
* Call to the vsc driver to let it know that the device is being
@@ -2472,11 +2585,11 @@ static int netvsc_resume(struct hv_device *dev)
ret = netvsc_attach(net, device_info);
- rtnl_unlock();
-
- kfree(device_info);
+ netvsc_devinfo_put(device_info);
net_device_ctx->saved_netvsc_dev_info = NULL;
+ rtnl_unlock();
+
return ret;
}
static const struct hv_vmbus_device_id id_table[] = {
diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
index e66d77d..b81ceba 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -235,7 +235,7 @@ static int rndis_filter_send_request(struct rndis_device *dev,
trace_rndis_send(dev->ndev, 0, &req->request_msg);
rcu_read_lock_bh();
- ret = netvsc_send(dev->ndev, packet, NULL, pb, NULL);
+ ret = netvsc_send(dev->ndev, packet, NULL, pb, NULL, false);
rcu_read_unlock_bh();
return ret;
--
1.8.3.1
^ permalink raw reply related
* [PATCH V3,net-next, 2/2] hv_netvsc: Update document for XDP support
From: Haiyang Zhang @ 2020-01-22 17:23 UTC (permalink / raw)
To: sashal, linux-hyperv, netdev
Cc: haiyangz, kys, sthemmin, olaf, vkuznets, davem, linux-kernel
In-Reply-To: <1579713814-36061-1-git-send-email-haiyangz@microsoft.com>
Added the new section in the document regarding XDP support
by hv_netvsc driver.
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
.../networking/device_drivers/microsoft/netvsc.txt | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/Documentation/networking/device_drivers/microsoft/netvsc.txt b/Documentation/networking/device_drivers/microsoft/netvsc.txt
index 3bfa635..cd63556 100644
--- a/Documentation/networking/device_drivers/microsoft/netvsc.txt
+++ b/Documentation/networking/device_drivers/microsoft/netvsc.txt
@@ -82,3 +82,24 @@ Features
contain one or more packets. The send buffer is an optimization, the driver
will use slower method to handle very large packets or if the send buffer
area is exhausted.
+
+ XDP support
+ -----------
+ XDP (eXpress Data Path) is a feature that runs eBPF bytecode at the early
+ stage when packets arrive at a NIC card. The goal is to increase performance
+ for packet processing, reducing the overhead of SKB allocation and other
+ upper network layers.
+
+ hv_netvsc supports XDP in native mode, and transparently sets the XDP
+ program on the associated VF NIC as well.
+
+ Setting / unsetting XDP program on synthetic NIC (netvsc) propagates to
+ VF NIC automatically. Setting / unsetting XDP program on VF NIC directly
+ is not recommended, also not propagated to synthetic NIC, and may be
+ overwritten by setting of synthetic NIC.
+
+ XDP program cannot run with LRO (RSC) enabled, so you need to disable LRO
+ before running XDP:
+ ethtool -K eth0 lro off
+
+ XDP_REDIRECT action is not yet supported.
--
1.8.3.1
^ permalink raw reply related
* [PATCH V3,net-next, 0/2] hv_netvsc: Add XDP support
From: Haiyang Zhang @ 2020-01-22 17:23 UTC (permalink / raw)
To: sashal, linux-hyperv, netdev
Cc: haiyangz, kys, sthemmin, olaf, vkuznets, davem, linux-kernel
Add XDP support and update related document.
Haiyang Zhang (2):
hv_netvsc: Add XDP support
hv_netvsc: Update document for XDP support
.../networking/device_drivers/microsoft/netvsc.txt | 21 +++
drivers/net/hyperv/Makefile | 2 +-
drivers/net/hyperv/hyperv_net.h | 21 ++-
drivers/net/hyperv/netvsc.c | 31 ++-
drivers/net/hyperv/netvsc_bpf.c | 209 +++++++++++++++++++++
drivers/net/hyperv/netvsc_drv.c | 175 ++++++++++++++---
drivers/net/hyperv/rndis_filter.c | 2 +-
7 files changed, 422 insertions(+), 39 deletions(-)
create mode 100644 drivers/net/hyperv/netvsc_bpf.c
--
1.8.3.1
^ permalink raw reply
* RE: [PATCH v2 2/4] hv_utils: Support host-initiated restart request
From: Michael Kelley @ 2020-01-22 17:16 UTC (permalink / raw)
To: Dexuan Cui, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, Sasha Levin, linux-hyperv@vger.kernel.org,
vkuznets, linux-kernel@vger.kernel.org
In-Reply-To: <HK0P153MB01487F54363A856730BB13FDBF350@HK0P153MB0148.APCP153.PROD.OUTLOOK.COM>
From: Dexuan Cui <decui@microsoft.com> Sent: Sunday, January 12, 2020 10:31 PM
>
>
> To test the code, run this command on the host:
>
> Restart-VM $vm -Type Reboot
>
Need a better commit message here. How about:
The hv_util driver currently supports a "shutdown" operation initiated from the
Hyper-V host. Newer versions of Hyper-V also support a "restart" operation. So
add support for the updated protocol version that has "restart" support, and
perform a clean reboot when such a message is received from Hyper-V.
To test the restart functionality, run this PowerShell command on the Hyper-V host:
Restart-VM <vmname> -Type Reboot
>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
> drivers/hv/hv_util.c | 23 ++++++++++++++++++++++-
> 1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
> index 766bd8457346..fe3a316380c2 100644
> --- a/drivers/hv/hv_util.c
> +++ b/drivers/hv/hv_util.c
> @@ -24,6 +24,8 @@
>
> #define SD_MAJOR 3
> #define SD_MINOR 0
> +#define SD_MINOR_1 1
> +#define SD_VERSION_3_1 (SD_MAJOR << 16 | SD_MINOR_1)
> #define SD_VERSION (SD_MAJOR << 16 | SD_MINOR)
>
> #define SD_MAJOR_1 1
> @@ -50,8 +52,9 @@ static int sd_srv_version;
> static int ts_srv_version;
> static int hb_srv_version;
>
> -#define SD_VER_COUNT 2
> +#define SD_VER_COUNT 3
> static const int sd_versions[] = {
> + SD_VERSION_3_1,
> SD_VERSION,
> SD_VERSION_1
> };
> @@ -118,11 +121,21 @@ static void perform_shutdown(struct work_struct *dummy)
> orderly_poweroff(true);
> }
>
> +static void perform_restart(struct work_struct *dummy)
> +{
> + orderly_reboot();
> +}
> +
> /*
> * Perform the shutdown operation in a thread context.
> */
> static DECLARE_WORK(shutdown_work, perform_shutdown);
>
> +/*
> + * Perform the restart operation in a thread context.
> + */
> +static DECLARE_WORK(restart_work, perform_restart);
> +
> static void shutdown_onchannelcallback(void *context)
> {
> struct vmbus_channel *channel = context;
> @@ -166,6 +179,14 @@ static void shutdown_onchannelcallback(void *context)
> pr_info("Shutdown request received -"
> " graceful shutdown initiated\n");
> break;
> + case 2:
> + case 3:
How are the flags values 0, 1, 2, and 3 interpreted? Perhaps a short comment
would be helpful.
> + pr_info("Restart request received -"
> + " graceful restart initiated\n");
> + icmsghdrp->status = HV_S_OK;
> +
> + schedule_work(&restart_work);
> + break;
For case 0 and 1 (shutdown), the schedule_work() call is performed only
after the response packet has been sent to the host. Is there a reason the
new code for case 2 and 3 (restart) is doing it in the opposite order?
> default:
> icmsghdrp->status = HV_E_FAIL;
> execute_shutdown = false;
> --
> 2.19.1
^ permalink raw reply
* RE: [PATCH v2 1/4] Tools: hv: Reopen the devices if read() or write() returns errors
From: Michael Kelley @ 2020-01-22 16:53 UTC (permalink / raw)
To: Dexuan Cui, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, Sasha Levin, linux-hyperv@vger.kernel.org,
vkuznets, linux-kernel@vger.kernel.org
In-Reply-To: <HK0P153MB01486C8C746F8936450C4CE1BF350@HK0P153MB0148.APCP153.PROD.OUTLOOK.COM>
From: Dexuan Cui <decui@microsoft.com> Sent: Sunday, January 12, 2020 10:30 PM
>
> The state machine in the hv_utils driver can run out of order in some
> corner cases, e.g. if the kvp daemon doesn't call write() fast enough
> due to some reason, kvp_timeout_func() can run first and move the state
> to HVUTIL_READY; next, when kvp_on_msg() is called it returns -EINVAL
> since kvp_transaction.state is smaller than HVUTIL_USERSPACE_REQ; later,
> the daemon's write() gets an error -EINVAL, and the daemon will exit().
>
> We can reproduce the issue by sending a SIGSTOP signal to the daemon, wait
> for 1 minute, and send a SIGCONT signal to the daemon: the daemon will
> exit() quickly.
>
> We can fix the issue by forcing a reset of the device (which means the
> daemon can close() and open() the device again) and doing extra necessary
> clean-up.
>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
> tools/hv/hv_fcopy_daemon.c | 19 +++++++++++++++----
> tools/hv/hv_kvp_daemon.c | 25 ++++++++++++++-----------
> tools/hv/hv_vss_daemon.c | 25 +++++++++++++++++++------
> 3 files changed, 48 insertions(+), 21 deletions(-)
>
> diff --git a/tools/hv/hv_fcopy_daemon.c b/tools/hv/hv_fcopy_daemon.c
> index aea2d91ab364..a78a5292589b 100644
> --- a/tools/hv/hv_fcopy_daemon.c
> +++ b/tools/hv/hv_fcopy_daemon.c
> @@ -21,7 +21,7 @@
> #include <fcntl.h>
> #include <getopt.h>
>
> -static int target_fd;
> +static int target_fd = -1;
> static char target_fname[PATH_MAX];
> static unsigned long long filesize;
>
> @@ -80,6 +80,8 @@ static int hv_start_fcopy(struct hv_start_fcopy *smsg)
>
> error = 0;
> done:
> + if (error)
> + memset(target_fname, 0, sizeof(target_fname));
> return error;
> }
>
> @@ -111,12 +113,16 @@ static int hv_copy_data(struct hv_do_fcopy *cpmsg)
> static int hv_copy_finished(void)
> {
> close(target_fd);
> + target_fd = -1;
> + memset(target_fname, 0, sizeof(target_fname));
I'm not completely clear on why target_fd and target_fname need to
be reset. Could you add a comment with an explanation? Also,
since target_fname is a null terminated string, it seems like
target_fname[0] = 0 would be sufficient vs. zero'ing all 4096 bytes
(PATH_MAX).
> return 0;
> }
> static int hv_copy_cancel(void)
> {
> close(target_fd);
> + target_fd = -1;
> unlink(target_fname);
> + memset(target_fname, 0, sizeof(target_fname));
> return 0;
>
> }
> @@ -141,7 +147,7 @@ int main(int argc, char *argv[])
> struct hv_do_fcopy copy;
> __u32 kernel_modver;
> } buffer = { };
> - int in_handshake = 1;
> + int in_handshake;
>
> static struct option long_options[] = {
> {"help", no_argument, 0, 'h' },
> @@ -170,6 +176,9 @@ int main(int argc, char *argv[])
> openlog("HV_FCOPY", 0, LOG_USER);
> syslog(LOG_INFO, "starting; pid is:%d", getpid());
>
> +reopen_fcopy_fd:
> + hv_copy_cancel();
> + in_handshake = 1;
> fcopy_fd = open("/dev/vmbus/hv_fcopy", O_RDWR);
>
> if (fcopy_fd < 0) {
> @@ -196,7 +205,8 @@ int main(int argc, char *argv[])
> len = pread(fcopy_fd, &buffer, sizeof(buffer), 0);
> if (len < 0) {
> syslog(LOG_ERR, "pread failed: %s", strerror(errno));
> - exit(EXIT_FAILURE);
> + close(fcopy_fd);
> + goto reopen_fcopy_fd;
In this case and all similar cases in this patch, there may be some risk
of getting stuck in a tight loop doing reopens if things are broken
in some strange and bizarre way. Having an absolute limit on the
number of reopens is potentially too restrictive as it could limit the
number of times a VM could be hibernated and resumed. Ideally
there could a simple rate limit on the reopens -- if it happens too frequently,
go ahead and exit like the current code does. Thoughts?
> }
>
> if (in_handshake) {
> @@ -233,7 +243,8 @@ int main(int argc, char *argv[])
>
> if (pwrite(fcopy_fd, &error, sizeof(int), 0) != sizeof(int)) {
> syslog(LOG_ERR, "pwrite failed: %s", strerror(errno));
> - exit(EXIT_FAILURE);
> + close(fcopy_fd);
> + goto reopen_fcopy_fd;
> }
> }
> }
> diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
> index e9ef4ca6a655..3282d48c4487 100644
> --- a/tools/hv/hv_kvp_daemon.c
> +++ b/tools/hv/hv_kvp_daemon.c
> @@ -76,7 +76,7 @@ enum {
> DNS
> };
>
> -static int in_hand_shake = 1;
> +static int in_hand_shake;
>
> static char *os_name = "";
> static char *os_major = "";
> @@ -1400,14 +1400,6 @@ int main(int argc, char *argv[])
> openlog("KVP", 0, LOG_USER);
> syslog(LOG_INFO, "KVP starting; pid is:%d", getpid());
>
> - kvp_fd = open("/dev/vmbus/hv_kvp", O_RDWR | O_CLOEXEC);
> -
> - if (kvp_fd < 0) {
> - syslog(LOG_ERR, "open /dev/vmbus/hv_kvp failed; error: %d %s",
> - errno, strerror(errno));
> - exit(EXIT_FAILURE);
> - }
> -
> /*
> * Retrieve OS release information.
> */
> @@ -1423,6 +1415,16 @@ int main(int argc, char *argv[])
> exit(EXIT_FAILURE);
> }
>
> +reopen_kvp_fd:
> + in_hand_shake = 1;
> + kvp_fd = open("/dev/vmbus/hv_kvp", O_RDWR | O_CLOEXEC);
> +
> + if (kvp_fd < 0) {
> + syslog(LOG_ERR, "open /dev/vmbus/hv_kvp failed; error: %d %s",
> + errno, strerror(errno));
> + exit(EXIT_FAILURE);
> + }
> +
> /*
> * Register ourselves with the kernel.
> */
> @@ -1458,7 +1460,7 @@ int main(int argc, char *argv[])
> errno, strerror(errno));
>
> close(kvp_fd);
> - return EXIT_FAILURE;
> + goto reopen_kvp_fd;
> }
>
> /*
> @@ -1623,7 +1625,8 @@ int main(int argc, char *argv[])
> if (len != sizeof(struct hv_kvp_msg)) {
> syslog(LOG_ERR, "write failed; error: %d %s", errno,
> strerror(errno));
> - exit(EXIT_FAILURE);
> + close(kvp_fd);
> + goto reopen_kvp_fd;
> }
> }
>
> diff --git a/tools/hv/hv_vss_daemon.c b/tools/hv/hv_vss_daemon.c
> index 92902a88f671..e70fed66a5ae 100644
> --- a/tools/hv/hv_vss_daemon.c
> +++ b/tools/hv/hv_vss_daemon.c
> @@ -28,6 +28,8 @@
> #include <stdbool.h>
> #include <dirent.h>
>
> +static bool fs_frozen;
> +
> /* Don't use syslog() in the function since that can cause write to disk */
> static int vss_do_freeze(char *dir, unsigned int cmd)
> {
> @@ -155,8 +157,11 @@ static int vss_operate(int operation)
> continue;
> }
> error |= vss_do_freeze(ent->mnt_dir, cmd);
> - if (error && operation == VSS_OP_FREEZE)
> - goto err;
> + if (operation == VSS_OP_FREEZE) {
> + if (error)
> + goto err;
> + fs_frozen = true;
> + }
> }
>
> endmntent(mounts);
Shortly after the above code, there's code specifically to
do the root filesystem last. It has the same error test as above,
and it seems like it should also be setting fs_frozen = true if
it is successful.
> @@ -167,6 +172,9 @@ static int vss_operate(int operation)
> goto err;
> }
>
> + if (operation == VSS_OP_THAW && !error)
> + fs_frozen = false;
> +
> goto out;
> err:
> save_errno = errno;
> @@ -175,6 +183,7 @@ static int vss_operate(int operation)
> endmntent(mounts);
> }
> vss_operate(VSS_OP_THAW);
> + fs_frozen = false;
> /* Call syslog after we thaw all filesystems */
> if (ent)
> syslog(LOG_ERR, "FREEZE of %s failed; error:%d %s",
> @@ -202,7 +211,7 @@ int main(int argc, char *argv[])
> int op;
> struct hv_vss_msg vss_msg[1];
> int daemonize = 1, long_index = 0, opt;
> - int in_handshake = 1;
> + int in_handshake;
> __u32 kernel_modver;
>
> static struct option long_options[] = {
> @@ -232,6 +241,10 @@ int main(int argc, char *argv[])
> openlog("Hyper-V VSS", 0, LOG_USER);
> syslog(LOG_INFO, "VSS starting; pid is:%d", getpid());
>
> +reopen_vss_fd:
> + if (fs_frozen)
> + vss_operate(VSS_OP_THAW);
Need to set fs_frozen = false after the above statement?
> + in_handshake = 1;
> vss_fd = open("/dev/vmbus/hv_vss", O_RDWR);
> if (vss_fd < 0) {
> syslog(LOG_ERR, "open /dev/vmbus/hv_vss failed; error: %d %s",
> @@ -285,7 +298,7 @@ int main(int argc, char *argv[])
> syslog(LOG_ERR, "read failed; error:%d %s",
> errno, strerror(errno));
> close(vss_fd);
> - return EXIT_FAILURE;
> + goto reopen_vss_fd;
> }
>
> op = vss_msg->vss_hdr.operation;
> @@ -318,8 +331,8 @@ int main(int argc, char *argv[])
> syslog(LOG_ERR, "write failed; error: %d %s", errno,
> strerror(errno));
>
> - if (op == VSS_OP_FREEZE)
> - vss_operate(VSS_OP_THAW);
> + close(vss_fd);
> + goto reopen_vss_fd;
> }
> }
>
> --
> 2.19.1
^ permalink raw reply
* Re: [PATCH net-next 0/3] vsock: support network namespace
From: Stefano Garzarella @ 2020-01-22 9:13 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: davem, netdev, linux-kernel, Jorgen Hansen, Jason Wang, kvm,
virtualization, linux-hyperv, Michael S. Tsirkin, Dexuan Cui
In-Reply-To: <20200121155053.GD641751@stefanha-x1.localdomain>
On Tue, Jan 21, 2020 at 03:50:53PM +0000, Stefan Hajnoczi wrote:
> What should vsock_dev_do_ioctl() IOCTL_VM_SOCKETS_GET_LOCAL_CID return?
> The answer is probably dependent on the caller's network namespace.
Right, and I'm not handling this case. I'll fix!
>
> Ultimately we may need per-namespace transports. Imagine assigning a
> G2H transport to a specific network namespace.
Agree.
>
> vsock_stream_connect() needs to be namespace-aware so that other
> namespaces cannot use the G2H transport to send a connection
> establishment packet.
Right, maybe I can change the vsock_assign_transport() to check if a
transport can be assigned to a socket, checking the namespace.
I'll send a v2 handling these cases and implementing the Michael's idea
about /dev/vhost-vsock-netns
Thanks,
Stefano
^ permalink raw reply
* RE: [PATCH V2,net-next, 1/2] hv_netvsc: Add XDP support
From: Haiyang Zhang @ 2020-01-22 2:12 UTC (permalink / raw)
To: David Miller
Cc: sashal@kernel.org, linux-hyperv@vger.kernel.org,
netdev@vger.kernel.org, KY Srinivasan, Stephen Hemminger,
olaf@aepfle.de, vkuznets, linux-kernel@vger.kernel.org
In-Reply-To: <20200121.222829.888926574980511328.davem@davemloft.net>
> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org <linux-kernel-
> owner@vger.kernel.org> On Behalf Of David Miller
> Sent: Tuesday, January 21, 2020 4:28 PM
> To: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: sashal@kernel.org; linux-hyperv@vger.kernel.org; netdev@vger.kernel.org;
> KY Srinivasan <kys@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; olaf@aepfle.de; vkuznets
> <vkuznets@redhat.com>; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH V2,net-next, 1/2] hv_netvsc: Add XDP support
>
> From: Haiyang Zhang <haiyangz@microsoft.com>
> Date: Tue, 21 Jan 2020 18:53:28 +0000
>
> > Sorry I was replying too quickly. See more detailed explanation below.
> >
> >> -----Original Message-----
> >> From: linux-hyperv-owner@vger.kernel.org <linux-hyperv-
> >> owner@vger.kernel.org> On Behalf Of David Miller
> >> Sent: Tuesday, January 21, 2020 5:05 AM
> >> To: Haiyang Zhang <haiyangz@microsoft.com>
> >> Cc: sashal@kernel.org; linux-hyperv@vger.kernel.org;
> >> netdev@vger.kernel.org; KY Srinivasan <kys@microsoft.com>; Stephen
> >> Hemminger <sthemmin@microsoft.com>; olaf@aepfle.de; vkuznets
> >> <vkuznets@redhat.com>; linux-kernel@vger.kernel.org
> >> Subject: Re: [PATCH V2,net-next, 1/2] hv_netvsc: Add XDP support
> >>
> >> From: Haiyang Zhang <haiyangz@microsoft.com>
> >> Date: Mon, 20 Jan 2020 14:22:36 -0800
> >>
> >> > +u32 netvsc_run_xdp(struct net_device *ndev, struct netvsc_channel
> *nvchan,
> >> > + struct xdp_buff *xdp)
> >> > +{
> >> > + struct page *page = NULL;
> >> > + void *data = nvchan->rsc.data[0];
> >> > + u32 len = nvchan->rsc.len[0];
> >> > + struct bpf_prog *prog;
> >> > + u32 act = XDP_PASS;
> >>
> >> Please use reverse christmas tree ordering of local variables.
> > Will do.
> >
> >>
> >> > + xdp->data_hard_start = page_address(page);
> >> > + xdp->data = xdp->data_hard_start + NETVSC_XDP_HDRM;
> >> > + xdp_set_data_meta_invalid(xdp);
> >> > + xdp->data_end = xdp->data + len;
> >> > + xdp->rxq = &nvchan->xdp_rxq;
> >> > + xdp->handle = 0;
> >> > +
> >> > + memcpy(xdp->data, data, len);
> >>
> >> Why can't the program run directly on nvchan->rsc.data[0]?
> >
> > The Azure/Hyper-V synthetic NIC receive buffer doesn't provide
> > headroom for XDP. We thought about re-use the RNDIS header space, but
> > it's too small. So we decided to copy the packets to a page buffer for
> > XDP. And, most of our VMs on Azure have Accelerated Network (SRIOV)
> > enabled, so most of the packets run on VF NIC. The synthetic NIC is
> > considered as a fallback data-path. So the data copy on netvsc won't
> > impact performance significantly.
>
> You need to explain this in your commit message otherwise every reviewer
> with XDP expertiece will ask the same question.
Will do.
Thanks,
- Haiyang
^ permalink raw reply
* Re: [PATCH V2,net-next, 1/2] hv_netvsc: Add XDP support
From: David Miller @ 2020-01-21 21:28 UTC (permalink / raw)
To: haiyangz
Cc: sashal, linux-hyperv, netdev, kys, sthemmin, olaf, vkuznets,
linux-kernel
In-Reply-To: <MN2PR21MB1375A6F208BC94CD4CFA016BCA0D0@MN2PR21MB1375.namprd21.prod.outlook.com>
From: Haiyang Zhang <haiyangz@microsoft.com>
Date: Tue, 21 Jan 2020 18:53:28 +0000
> Sorry I was replying too quickly. See more detailed explanation below.
>
>> -----Original Message-----
>> From: linux-hyperv-owner@vger.kernel.org <linux-hyperv-
>> owner@vger.kernel.org> On Behalf Of David Miller
>> Sent: Tuesday, January 21, 2020 5:05 AM
>> To: Haiyang Zhang <haiyangz@microsoft.com>
>> Cc: sashal@kernel.org; linux-hyperv@vger.kernel.org; netdev@vger.kernel.org;
>> KY Srinivasan <kys@microsoft.com>; Stephen Hemminger
>> <sthemmin@microsoft.com>; olaf@aepfle.de; vkuznets
>> <vkuznets@redhat.com>; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH V2,net-next, 1/2] hv_netvsc: Add XDP support
>>
>> From: Haiyang Zhang <haiyangz@microsoft.com>
>> Date: Mon, 20 Jan 2020 14:22:36 -0800
>>
>> > +u32 netvsc_run_xdp(struct net_device *ndev, struct netvsc_channel *nvchan,
>> > + struct xdp_buff *xdp)
>> > +{
>> > + struct page *page = NULL;
>> > + void *data = nvchan->rsc.data[0];
>> > + u32 len = nvchan->rsc.len[0];
>> > + struct bpf_prog *prog;
>> > + u32 act = XDP_PASS;
>>
>> Please use reverse christmas tree ordering of local variables.
> Will do.
>
>>
>> > + xdp->data_hard_start = page_address(page);
>> > + xdp->data = xdp->data_hard_start + NETVSC_XDP_HDRM;
>> > + xdp_set_data_meta_invalid(xdp);
>> > + xdp->data_end = xdp->data + len;
>> > + xdp->rxq = &nvchan->xdp_rxq;
>> > + xdp->handle = 0;
>> > +
>> > + memcpy(xdp->data, data, len);
>>
>> Why can't the program run directly on nvchan->rsc.data[0]?
>
> The Azure/Hyper-V synthetic NIC receive buffer doesn't provide headroom
> for XDP. We thought about re-use the RNDIS header space, but it's too
> small. So we decided to copy the packets to a page buffer for XDP. And,
> most of our VMs on Azure have Accelerated Network (SRIOV) enabled, so
> most of the packets run on VF NIC. The synthetic NIC is considered as a
> fallback data-path. So the data copy on netvsc won't impact performance
> significantly.
You need to explain this in your commit message otherwise every reviewer with
XDP expertiece will ask the same question.
^ permalink raw reply
* RE: [PATCH V2,net-next, 1/2] hv_netvsc: Add XDP support
From: Haiyang Zhang @ 2020-01-21 18:53 UTC (permalink / raw)
To: David Miller
Cc: sashal@kernel.org, linux-hyperv@vger.kernel.org,
netdev@vger.kernel.org, KY Srinivasan, Stephen Hemminger,
olaf@aepfle.de, vkuznets, linux-kernel@vger.kernel.org
In-Reply-To: <20200121.110454.2077433904156411260.davem@davemloft.net>
Sorry I was replying too quickly. See more detailed explanation below.
> -----Original Message-----
> From: linux-hyperv-owner@vger.kernel.org <linux-hyperv-
> owner@vger.kernel.org> On Behalf Of David Miller
> Sent: Tuesday, January 21, 2020 5:05 AM
> To: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: sashal@kernel.org; linux-hyperv@vger.kernel.org; netdev@vger.kernel.org;
> KY Srinivasan <kys@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; olaf@aepfle.de; vkuznets
> <vkuznets@redhat.com>; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH V2,net-next, 1/2] hv_netvsc: Add XDP support
>
> From: Haiyang Zhang <haiyangz@microsoft.com>
> Date: Mon, 20 Jan 2020 14:22:36 -0800
>
> > +u32 netvsc_run_xdp(struct net_device *ndev, struct netvsc_channel *nvchan,
> > + struct xdp_buff *xdp)
> > +{
> > + struct page *page = NULL;
> > + void *data = nvchan->rsc.data[0];
> > + u32 len = nvchan->rsc.len[0];
> > + struct bpf_prog *prog;
> > + u32 act = XDP_PASS;
>
> Please use reverse christmas tree ordering of local variables.
Will do.
>
> > + xdp->data_hard_start = page_address(page);
> > + xdp->data = xdp->data_hard_start + NETVSC_XDP_HDRM;
> > + xdp_set_data_meta_invalid(xdp);
> > + xdp->data_end = xdp->data + len;
> > + xdp->rxq = &nvchan->xdp_rxq;
> > + xdp->handle = 0;
> > +
> > + memcpy(xdp->data, data, len);
>
> Why can't the program run directly on nvchan->rsc.data[0]?
The Azure/Hyper-V synthetic NIC receive buffer doesn't provide headroom
for XDP. We thought about re-use the RNDIS header space, but it's too
small. So we decided to copy the packets to a page buffer for XDP. And,
most of our VMs on Azure have Accelerated Network (SRIOV) enabled, so
most of the packets run on VF NIC. The synthetic NIC is considered as a
fallback data-path. So the data copy on netvsc won't impact performance
significantly.
Thanks,
- Haiyang
^ permalink raw reply
* Re: [PATCH net-next 0/3] vsock: support network namespace
From: Stefan Hajnoczi @ 2020-01-21 15:50 UTC (permalink / raw)
To: Stefano Garzarella
Cc: davem, netdev, linux-kernel, Jorgen Hansen, Jason Wang, kvm,
virtualization, linux-hyperv, Michael S. Tsirkin, Dexuan Cui
In-Reply-To: <20200116172428.311437-1-sgarzare@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 407 bytes --]
What should vsock_dev_do_ioctl() IOCTL_VM_SOCKETS_GET_LOCAL_CID return?
The answer is probably dependent on the caller's network namespace.
Ultimately we may need per-namespace transports. Imagine assigning a
G2H transport to a specific network namespace.
vsock_stream_connect() needs to be namespace-aware so that other
namespaces cannot use the G2H transport to send a connection
establishment packet.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH net-next 1/3] vsock: add network namespace support
From: Stefan Hajnoczi @ 2020-01-21 15:44 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Stefano Garzarella, David Miller, netdev, linux-kernel,
Jorgen Hansen, Jason Wang, kvm, virtualization, linux-hyperv,
Dexuan Cui
In-Reply-To: <20200121093104-mutt-send-email-mst@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 6399 bytes --]
On Tue, Jan 21, 2020 at 09:31:42AM -0500, Michael S. Tsirkin wrote:
> On Tue, Jan 21, 2020 at 01:59:07PM +0000, Stefan Hajnoczi wrote:
> > On Tue, Jan 21, 2020 at 10:07:06AM +0100, Stefano Garzarella wrote:
> > > On Mon, Jan 20, 2020 at 11:02 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > On Mon, Jan 20, 2020 at 05:53:39PM +0100, Stefano Garzarella wrote:
> > > > > On Mon, Jan 20, 2020 at 5:04 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > On Mon, Jan 20, 2020 at 02:58:01PM +0100, Stefano Garzarella wrote:
> > > > > > > On Mon, Jan 20, 2020 at 1:03 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > > > On Mon, Jan 20, 2020 at 11:17:35AM +0100, Stefano Garzarella wrote:
> > > > > > > > > On Mon, Jan 20, 2020 at 10:06:10AM +0100, David Miller wrote:
> > > > > > > > > > From: Stefano Garzarella <sgarzare@redhat.com>
> > > > > > > > > > Date: Thu, 16 Jan 2020 18:24:26 +0100
> > > > > > > > > >
> > > > > > > > > > > This patch adds 'netns' module param to enable this new feature
> > > > > > > > > > > (disabled by default), because it changes vsock's behavior with
> > > > > > > > > > > network namespaces and could break existing applications.
> > > > > > > > > >
> > > > > > > > > > Sorry, no.
> > > > > > > > > >
> > > > > > > > > > I wonder if you can even design a legitimate, reasonable, use case
> > > > > > > > > > where these netns changes could break things.
> > > > > > > > >
> > > > > > > > > I forgot to mention the use case.
> > > > > > > > > I tried the RFC with Kata containers and we found that Kata shim-v1
> > > > > > > > > doesn't work (Kata shim-v2 works as is) because there are the following
> > > > > > > > > processes involved:
> > > > > > > > > - kata-runtime (runs in the init_netns) opens /dev/vhost-vsock and
> > > > > > > > > passes it to qemu
> > > > > > > > > - kata-shim (runs in a container) wants to talk with the guest but the
> > > > > > > > > vsock device is assigned to the init_netns and kata-shim runs in a
> > > > > > > > > different netns, so the communication is not allowed
> > > > > > > > > But, as you said, this could be a wrong design, indeed they already
> > > > > > > > > found a fix, but I was not sure if others could have the same issue.
> > > > > > > > >
> > > > > > > > > In this case, do you think it is acceptable to make this change in
> > > > > > > > > the vsock's behavior with netns and ask the user to change the design?
> > > > > > > >
> > > > > > > > David's question is what would be a usecase that's broken
> > > > > > > > (as opposed to fixed) by enabling this by default.
> > > > > > >
> > > > > > > Yes, I got that. Thanks for clarifying.
> > > > > > > I just reported a broken example that can be fixed with a different
> > > > > > > design (due to the fact that before this series, vsock devices were
> > > > > > > accessible to all netns).
> > > > > > >
> > > > > > > >
> > > > > > > > If it does exist, you need a way for userspace to opt-in,
> > > > > > > > module parameter isn't that.
> > > > > > >
> > > > > > > Okay, but I honestly can't find a case that can't be solved.
> > > > > > > So I don't know whether to add an option (ioctl, sysfs ?) or wait for
> > > > > > > a real case to come up.
> > > > > > >
> > > > > > > I'll try to see better if there's any particular case where we need
> > > > > > > to disable netns in vsock.
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Stefano
> > > > > >
> > > > > > Me neither. so what did you have in mind when you wrote:
> > > > > > "could break existing applications"?
> > > > >
> > > > > I had in mind:
> > > > > 1. the Kata case. It is fixable (the fix is not merged on kata), but
> > > > > older versions will not work with newer Linux.
> > > >
> > > > meaning they will keep not working, right?
> > >
> > > Right, I mean without this series they work, with this series they work
> > > only if the netns support is disabled or with a patch proposed but not
> > > merged in kata.
> > >
> > > >
> > > > > 2. a single process running on init_netns that wants to communicate with
> > > > > VMs handled by VMMs running in different netns, but this case can be
> > > > > solved opening the /dev/vhost-vsock in the same netns of the process
> > > > > that wants to communicate with the VMs (init_netns in this case), and
> > > > > passig it to the VMM.
> > > >
> > > > again right now they just don't work, right?
> > >
> > > Right, as above.
> > >
> > > What do you recommend I do?
> >
> > Existing userspace applications must continue to work.
> >
> > Guests are fine because G2H transports are always in the initial network
> > namespace.
> >
> > On the host side we have a real case where Kata Containers and other
> > vsock users break. Existing applications run in other network
> > namespaces and assume they can communicate over vsock (it's only
> > available in the initial network namespace by default).
> >
> > It seems we cannot isolate new network namespaces from the initial
> > network namespace by default because it will break existing
> > applications. That's a bummer.
> >
> > There is one solution that maintains compatibility:
> >
> > Introduce a per-namespace vsock isolation flag that can only transition
> > from false to true. Once it becomes true it cannot be reset to false
> > anymore (for security).
> >
> > When vsock isolation is false the initial network namespace is used for
> > <CID, port> addressing.
> >
> > When vsock isolation is true the current namespace is used for <CID,
> > port> addressing.
> >
> > I guess the vsock isolation flag would be set via a rtnetlink message,
> > but I haven't checked.
> >
> > The upshot is: existing software doesn't benefit from namespaces for
> > vsock isolation but it continues to work! New software makes 1 special
> > call after creating the namespace to opt in to vsock isolation.
> >
> > This approach is secure because whoever sets up namespaces can
> > transition the flag from false to true and know that it can never be
> > reset to false anymore.
> >
> > Does this make sense to everyone?
> >
> > Stefan
>
> Anything wrong with a separate device? whoever opens it decides
> whether netns will work ...
Your idea is better. I think a separate device is the way to go.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH net-next 1/3] vsock: add network namespace support
From: Stefan Hajnoczi @ 2020-01-21 15:43 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Stefano Garzarella, David Miller, netdev, linux-kernel,
Jorgen Hansen, Jason Wang, kvm, virtualization, linux-hyperv,
Dexuan Cui
In-Reply-To: <20200121055403-mutt-send-email-mst@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 4855 bytes --]
On Tue, Jan 21, 2020 at 06:14:48AM -0500, Michael S. Tsirkin wrote:
> On Tue, Jan 21, 2020 at 10:07:06AM +0100, Stefano Garzarella wrote:
> > On Mon, Jan 20, 2020 at 11:02 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > On Mon, Jan 20, 2020 at 05:53:39PM +0100, Stefano Garzarella wrote:
> > > > On Mon, Jan 20, 2020 at 5:04 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > On Mon, Jan 20, 2020 at 02:58:01PM +0100, Stefano Garzarella wrote:
> > > > > > On Mon, Jan 20, 2020 at 1:03 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > > On Mon, Jan 20, 2020 at 11:17:35AM +0100, Stefano Garzarella wrote:
> > > > > > > > On Mon, Jan 20, 2020 at 10:06:10AM +0100, David Miller wrote:
> > > > > > > > > From: Stefano Garzarella <sgarzare@redhat.com>
> > > > > > > > > Date: Thu, 16 Jan 2020 18:24:26 +0100
> > > > > > > > >
> > > > > > > > > > This patch adds 'netns' module param to enable this new feature
> > > > > > > > > > (disabled by default), because it changes vsock's behavior with
> > > > > > > > > > network namespaces and could break existing applications.
> > > > > > > > >
> > > > > > > > > Sorry, no.
> > > > > > > > >
> > > > > > > > > I wonder if you can even design a legitimate, reasonable, use case
> > > > > > > > > where these netns changes could break things.
> > > > > > > >
> > > > > > > > I forgot to mention the use case.
> > > > > > > > I tried the RFC with Kata containers and we found that Kata shim-v1
> > > > > > > > doesn't work (Kata shim-v2 works as is) because there are the following
> > > > > > > > processes involved:
> > > > > > > > - kata-runtime (runs in the init_netns) opens /dev/vhost-vsock and
> > > > > > > > passes it to qemu
> > > > > > > > - kata-shim (runs in a container) wants to talk with the guest but the
> > > > > > > > vsock device is assigned to the init_netns and kata-shim runs in a
> > > > > > > > different netns, so the communication is not allowed
> > > > > > > > But, as you said, this could be a wrong design, indeed they already
> > > > > > > > found a fix, but I was not sure if others could have the same issue.
> > > > > > > >
> > > > > > > > In this case, do you think it is acceptable to make this change in
> > > > > > > > the vsock's behavior with netns and ask the user to change the design?
> > > > > > >
> > > > > > > David's question is what would be a usecase that's broken
> > > > > > > (as opposed to fixed) by enabling this by default.
> > > > > >
> > > > > > Yes, I got that. Thanks for clarifying.
> > > > > > I just reported a broken example that can be fixed with a different
> > > > > > design (due to the fact that before this series, vsock devices were
> > > > > > accessible to all netns).
> > > > > >
> > > > > > >
> > > > > > > If it does exist, you need a way for userspace to opt-in,
> > > > > > > module parameter isn't that.
> > > > > >
> > > > > > Okay, but I honestly can't find a case that can't be solved.
> > > > > > So I don't know whether to add an option (ioctl, sysfs ?) or wait for
> > > > > > a real case to come up.
> > > > > >
> > > > > > I'll try to see better if there's any particular case where we need
> > > > > > to disable netns in vsock.
> > > > > >
> > > > > > Thanks,
> > > > > > Stefano
> > > > >
> > > > > Me neither. so what did you have in mind when you wrote:
> > > > > "could break existing applications"?
> > > >
> > > > I had in mind:
> > > > 1. the Kata case. It is fixable (the fix is not merged on kata), but
> > > > older versions will not work with newer Linux.
> > >
> > > meaning they will keep not working, right?
> >
> > Right, I mean without this series they work, with this series they work
> > only if the netns support is disabled or with a patch proposed but not
> > merged in kata.
> >
> > >
> > > > 2. a single process running on init_netns that wants to communicate with
> > > > VMs handled by VMMs running in different netns, but this case can be
> > > > solved opening the /dev/vhost-vsock in the same netns of the process
> > > > that wants to communicate with the VMs (init_netns in this case), and
> > > > passig it to the VMM.
> > >
> > > again right now they just don't work, right?
> >
> > Right, as above.
> >
> > What do you recommend I do?
> >
> > Thanks,
> > Stefano
>
> If this breaks userspace, then we need to maintain compatibility.
> For example, have two devices, /dev/vhost-vsock and /dev/vhost-vsock-netns?
/dev/vhost-vsock-netns is cleaner and simpler than my suggestion. I
like it!
This is nice for containers (say you want to run QEMU inside a container
on the host) because you can allow only /dev/vhost-vsock-netns inside
containers. This prevents them from opening /dev/vhost-vsock to get
access to the initial network namespace.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] pci: hyperv: Move retarget related struct definitions into tlfs
From: Bjorn Helgaas @ 2020-01-21 14:37 UTC (permalink / raw)
To: Boqun Feng
Cc: linux-hyperv, linux-kernel, Michael Kelley, K. Y. Srinivasan,
Haiyang Zhang, Stephen Hemminger, Sasha Levin, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H. Peter Anvin,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
Lorenzo Pieralisi, Andrew Murray,
open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS
In-Reply-To: <20200121015713.69691-2-boqun.feng@gmail.com>
On Tue, Jan 21, 2020 at 09:57:13AM +0800, Boqun Feng wrote:
> For future support of virtual PCI on non-x86 architecture.
1) Don't make up random subject line prefixes; look at previous
practice and follow it, e.g.,
$ git log --oneline drivers/pci/controller/pci-hyperv.c | head
877b911a5ba0 PCI: hv: Avoid a kmemleak false positive caused by the hbus buffer
14ef39fddd23 PCI: hv: Change pci_protocol_version to per-hbus
ac82fc832708 PCI: hv: Add hibernation support
a8e37506e79a PCI: hv: Reorganize the code in preparation of hibernation
f73f8a504e27 PCI: hv: Use bytes 4 and 5 from instance ID as the PCI domain numbers
348dd93e40c1 PCI: hv: Add a Hyper-V PCI interface driver for software backchannel interface
2) Make the commit log complete in itself. This one (and the previous
on) is not complete without reading the subject.
3) This patch claims to be a "move", but in fact it also *adds* union
hv_msi_entry, which didn't exist before. It's better if you do a pure
move that doesn't add or change things, plus a separate patch that
makes changes that need to be reviewed more closely.
> Signed-off-by: Boqun Feng (Microsoft) <boqun.feng@gmail.com>
> ---
> arch/x86/include/asm/hyperv-tlfs.h | 38 +++++++++++++++++++++++++++++
> arch/x86/include/asm/mshyperv.h | 8 ++++++
> drivers/pci/controller/pci-hyperv.c | 38 +++--------------------------
> 3 files changed, 50 insertions(+), 34 deletions(-)
>
> diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
> index b9ebc20b2385..debe017ae748 100644
> --- a/arch/x86/include/asm/hyperv-tlfs.h
> +++ b/arch/x86/include/asm/hyperv-tlfs.h
> @@ -912,4 +912,42 @@ struct hv_tlb_flush_ex {
> struct hv_partition_assist_pg {
> u32 tlb_lock_count;
> };
> +
> +union hv_msi_entry {
> + u64 as_uint64;
> + struct {
> + u32 address;
> + u32 data;
> + } __packed;
> +};
> +
> +struct hv_interrupt_entry {
> + u32 source; /* 1 for MSI(-X) */
> + u32 reserved1;
> + union hv_msi_entry msi_entry;
> +} __packed;
> +
> +/*
> + * flags for hv_device_interrupt_target.flags
> + */
> +#define HV_DEVICE_INTERRUPT_TARGET_MULTICAST 1
> +#define HV_DEVICE_INTERRUPT_TARGET_PROCESSOR_SET 2
> +
> +struct hv_device_interrupt_target {
> + u32 vector;
> + u32 flags;
> + union {
> + u64 vp_mask;
> + struct hv_vpset vp_set;
> + };
> +} __packed;
> +
> +/* HvRetargetDeviceInterrupt hypercall */
> +struct hv_retarget_device_interrupt {
> + u64 partition_id;
> + u64 device_id;
> + struct hv_interrupt_entry int_entry;
> + u64 reserved2;
> + struct hv_device_interrupt_target int_target;
> +} __packed __aligned(8);
> #endif
> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
> index 6b79515abb82..d13319d82f6b 100644
> --- a/arch/x86/include/asm/mshyperv.h
> +++ b/arch/x86/include/asm/mshyperv.h
> @@ -240,6 +240,14 @@ bool hv_vcpu_is_preempted(int vcpu);
> static inline void hv_apic_init(void) {}
> #endif
>
> +#if IS_ENABLED(CONFIG_PCI_HYPERV)
> +#define hv_set_msi_address_from_desc(msi_entry, msi_desc) \
> +do { \
> + (msi_entry)->address = (msi_desc)->msg.address_lo; \
> +} while (0)
> +
> +#endif /* CONFIG_PCI_HYPERV */
> +
> #else /* CONFIG_HYPERV */
> static inline void hyperv_init(void) {}
> static inline void hyperv_setup_mmu_ops(void) {}
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index aacfcc90d929..2240f2b3643e 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -406,36 +406,6 @@ struct pci_eject_response {
>
> static int pci_ring_size = (4 * PAGE_SIZE);
>
> -struct hv_interrupt_entry {
> - u32 source; /* 1 for MSI(-X) */
> - u32 reserved1;
> - u32 address;
> - u32 data;
> -};
> -
> -/*
> - * flags for hv_device_interrupt_target.flags
> - */
> -#define HV_DEVICE_INTERRUPT_TARGET_MULTICAST 1
> -#define HV_DEVICE_INTERRUPT_TARGET_PROCESSOR_SET 2
> -
> -struct hv_device_interrupt_target {
> - u32 vector;
> - u32 flags;
> - union {
> - u64 vp_mask;
> - struct hv_vpset vp_set;
> - };
> -};
> -
> -struct retarget_msi_interrupt {
> - u64 partition_id; /* use "self" */
> - u64 device_id;
> - struct hv_interrupt_entry int_entry;
> - u64 reserved2;
> - struct hv_device_interrupt_target int_target;
> -} __packed __aligned(8);
> -
> /*
> * Driver specific state.
> */
> @@ -482,7 +452,7 @@ struct hv_pcibus_device {
> struct workqueue_struct *wq;
>
> /* hypercall arg, must not cross page boundary */
> - struct retarget_msi_interrupt retarget_msi_interrupt_params;
> + struct hv_retarget_device_interrupt retarget_msi_interrupt_params;
>
> /*
> * Don't put anything here: retarget_msi_interrupt_params must be last
> @@ -1178,7 +1148,7 @@ static void hv_irq_unmask(struct irq_data *data)
> {
> struct msi_desc *msi_desc = irq_data_get_msi_desc(data);
> struct irq_cfg *cfg = irqd_cfg(data);
> - struct retarget_msi_interrupt *params;
> + struct hv_retarget_device_interrupt *params;
> struct hv_pcibus_device *hbus;
> struct cpumask *dest;
> cpumask_var_t tmp;
> @@ -1200,8 +1170,8 @@ static void hv_irq_unmask(struct irq_data *data)
> memset(params, 0, sizeof(*params));
> params->partition_id = HV_PARTITION_ID_SELF;
> params->int_entry.source = 1; /* MSI(-X) */
> - params->int_entry.address = msi_desc->msg.address_lo;
> - params->int_entry.data = msi_desc->msg.data;
> + hv_set_msi_address_from_desc(¶ms->int_entry.msi_entry, msi_desc);
> + params->int_entry.msi_entry.data = msi_desc->msg.data;
> params->device_id = (hbus->hdev->dev_instance.b[5] << 24) |
> (hbus->hdev->dev_instance.b[4] << 16) |
> (hbus->hdev->dev_instance.b[7] << 8) |
> --
> 2.24.1
>
^ permalink raw reply
* Re: [PATCH net-next 1/3] vsock: add network namespace support
From: Michael S. Tsirkin @ 2020-01-21 14:31 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Stefano Garzarella, David Miller, netdev, linux-kernel,
Jorgen Hansen, Jason Wang, kvm, virtualization, linux-hyperv,
Dexuan Cui
In-Reply-To: <20200121135907.GA641751@stefanha-x1.localdomain>
On Tue, Jan 21, 2020 at 01:59:07PM +0000, Stefan Hajnoczi wrote:
> On Tue, Jan 21, 2020 at 10:07:06AM +0100, Stefano Garzarella wrote:
> > On Mon, Jan 20, 2020 at 11:02 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > On Mon, Jan 20, 2020 at 05:53:39PM +0100, Stefano Garzarella wrote:
> > > > On Mon, Jan 20, 2020 at 5:04 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > On Mon, Jan 20, 2020 at 02:58:01PM +0100, Stefano Garzarella wrote:
> > > > > > On Mon, Jan 20, 2020 at 1:03 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > > On Mon, Jan 20, 2020 at 11:17:35AM +0100, Stefano Garzarella wrote:
> > > > > > > > On Mon, Jan 20, 2020 at 10:06:10AM +0100, David Miller wrote:
> > > > > > > > > From: Stefano Garzarella <sgarzare@redhat.com>
> > > > > > > > > Date: Thu, 16 Jan 2020 18:24:26 +0100
> > > > > > > > >
> > > > > > > > > > This patch adds 'netns' module param to enable this new feature
> > > > > > > > > > (disabled by default), because it changes vsock's behavior with
> > > > > > > > > > network namespaces and could break existing applications.
> > > > > > > > >
> > > > > > > > > Sorry, no.
> > > > > > > > >
> > > > > > > > > I wonder if you can even design a legitimate, reasonable, use case
> > > > > > > > > where these netns changes could break things.
> > > > > > > >
> > > > > > > > I forgot to mention the use case.
> > > > > > > > I tried the RFC with Kata containers and we found that Kata shim-v1
> > > > > > > > doesn't work (Kata shim-v2 works as is) because there are the following
> > > > > > > > processes involved:
> > > > > > > > - kata-runtime (runs in the init_netns) opens /dev/vhost-vsock and
> > > > > > > > passes it to qemu
> > > > > > > > - kata-shim (runs in a container) wants to talk with the guest but the
> > > > > > > > vsock device is assigned to the init_netns and kata-shim runs in a
> > > > > > > > different netns, so the communication is not allowed
> > > > > > > > But, as you said, this could be a wrong design, indeed they already
> > > > > > > > found a fix, but I was not sure if others could have the same issue.
> > > > > > > >
> > > > > > > > In this case, do you think it is acceptable to make this change in
> > > > > > > > the vsock's behavior with netns and ask the user to change the design?
> > > > > > >
> > > > > > > David's question is what would be a usecase that's broken
> > > > > > > (as opposed to fixed) by enabling this by default.
> > > > > >
> > > > > > Yes, I got that. Thanks for clarifying.
> > > > > > I just reported a broken example that can be fixed with a different
> > > > > > design (due to the fact that before this series, vsock devices were
> > > > > > accessible to all netns).
> > > > > >
> > > > > > >
> > > > > > > If it does exist, you need a way for userspace to opt-in,
> > > > > > > module parameter isn't that.
> > > > > >
> > > > > > Okay, but I honestly can't find a case that can't be solved.
> > > > > > So I don't know whether to add an option (ioctl, sysfs ?) or wait for
> > > > > > a real case to come up.
> > > > > >
> > > > > > I'll try to see better if there's any particular case where we need
> > > > > > to disable netns in vsock.
> > > > > >
> > > > > > Thanks,
> > > > > > Stefano
> > > > >
> > > > > Me neither. so what did you have in mind when you wrote:
> > > > > "could break existing applications"?
> > > >
> > > > I had in mind:
> > > > 1. the Kata case. It is fixable (the fix is not merged on kata), but
> > > > older versions will not work with newer Linux.
> > >
> > > meaning they will keep not working, right?
> >
> > Right, I mean without this series they work, with this series they work
> > only if the netns support is disabled or with a patch proposed but not
> > merged in kata.
> >
> > >
> > > > 2. a single process running on init_netns that wants to communicate with
> > > > VMs handled by VMMs running in different netns, but this case can be
> > > > solved opening the /dev/vhost-vsock in the same netns of the process
> > > > that wants to communicate with the VMs (init_netns in this case), and
> > > > passig it to the VMM.
> > >
> > > again right now they just don't work, right?
> >
> > Right, as above.
> >
> > What do you recommend I do?
>
> Existing userspace applications must continue to work.
>
> Guests are fine because G2H transports are always in the initial network
> namespace.
>
> On the host side we have a real case where Kata Containers and other
> vsock users break. Existing applications run in other network
> namespaces and assume they can communicate over vsock (it's only
> available in the initial network namespace by default).
>
> It seems we cannot isolate new network namespaces from the initial
> network namespace by default because it will break existing
> applications. That's a bummer.
>
> There is one solution that maintains compatibility:
>
> Introduce a per-namespace vsock isolation flag that can only transition
> from false to true. Once it becomes true it cannot be reset to false
> anymore (for security).
>
> When vsock isolation is false the initial network namespace is used for
> <CID, port> addressing.
>
> When vsock isolation is true the current namespace is used for <CID,
> port> addressing.
>
> I guess the vsock isolation flag would be set via a rtnetlink message,
> but I haven't checked.
>
> The upshot is: existing software doesn't benefit from namespaces for
> vsock isolation but it continues to work! New software makes 1 special
> call after creating the namespace to opt in to vsock isolation.
>
> This approach is secure because whoever sets up namespaces can
> transition the flag from false to true and know that it can never be
> reset to false anymore.
>
> Does this make sense to everyone?
>
> Stefan
Anything wrong with a separate device? whoever opens it decides
whether netns will work ...
--
MST
^ permalink raw reply
* Re: [PATCH net-next 1/3] vsock: add network namespace support
From: Stefan Hajnoczi @ 2020-01-21 13:59 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Michael S. Tsirkin, David Miller, netdev, linux-kernel,
Jorgen Hansen, Jason Wang, kvm, virtualization, linux-hyperv,
Dexuan Cui
In-Reply-To: <CAGxU2F4uW7FNe5xC0sb3Xxr_GABSXuu1Z9n5M=Ntq==T7MaaVw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 5589 bytes --]
On Tue, Jan 21, 2020 at 10:07:06AM +0100, Stefano Garzarella wrote:
> On Mon, Jan 20, 2020 at 11:02 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Mon, Jan 20, 2020 at 05:53:39PM +0100, Stefano Garzarella wrote:
> > > On Mon, Jan 20, 2020 at 5:04 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > On Mon, Jan 20, 2020 at 02:58:01PM +0100, Stefano Garzarella wrote:
> > > > > On Mon, Jan 20, 2020 at 1:03 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > On Mon, Jan 20, 2020 at 11:17:35AM +0100, Stefano Garzarella wrote:
> > > > > > > On Mon, Jan 20, 2020 at 10:06:10AM +0100, David Miller wrote:
> > > > > > > > From: Stefano Garzarella <sgarzare@redhat.com>
> > > > > > > > Date: Thu, 16 Jan 2020 18:24:26 +0100
> > > > > > > >
> > > > > > > > > This patch adds 'netns' module param to enable this new feature
> > > > > > > > > (disabled by default), because it changes vsock's behavior with
> > > > > > > > > network namespaces and could break existing applications.
> > > > > > > >
> > > > > > > > Sorry, no.
> > > > > > > >
> > > > > > > > I wonder if you can even design a legitimate, reasonable, use case
> > > > > > > > where these netns changes could break things.
> > > > > > >
> > > > > > > I forgot to mention the use case.
> > > > > > > I tried the RFC with Kata containers and we found that Kata shim-v1
> > > > > > > doesn't work (Kata shim-v2 works as is) because there are the following
> > > > > > > processes involved:
> > > > > > > - kata-runtime (runs in the init_netns) opens /dev/vhost-vsock and
> > > > > > > passes it to qemu
> > > > > > > - kata-shim (runs in a container) wants to talk with the guest but the
> > > > > > > vsock device is assigned to the init_netns and kata-shim runs in a
> > > > > > > different netns, so the communication is not allowed
> > > > > > > But, as you said, this could be a wrong design, indeed they already
> > > > > > > found a fix, but I was not sure if others could have the same issue.
> > > > > > >
> > > > > > > In this case, do you think it is acceptable to make this change in
> > > > > > > the vsock's behavior with netns and ask the user to change the design?
> > > > > >
> > > > > > David's question is what would be a usecase that's broken
> > > > > > (as opposed to fixed) by enabling this by default.
> > > > >
> > > > > Yes, I got that. Thanks for clarifying.
> > > > > I just reported a broken example that can be fixed with a different
> > > > > design (due to the fact that before this series, vsock devices were
> > > > > accessible to all netns).
> > > > >
> > > > > >
> > > > > > If it does exist, you need a way for userspace to opt-in,
> > > > > > module parameter isn't that.
> > > > >
> > > > > Okay, but I honestly can't find a case that can't be solved.
> > > > > So I don't know whether to add an option (ioctl, sysfs ?) or wait for
> > > > > a real case to come up.
> > > > >
> > > > > I'll try to see better if there's any particular case where we need
> > > > > to disable netns in vsock.
> > > > >
> > > > > Thanks,
> > > > > Stefano
> > > >
> > > > Me neither. so what did you have in mind when you wrote:
> > > > "could break existing applications"?
> > >
> > > I had in mind:
> > > 1. the Kata case. It is fixable (the fix is not merged on kata), but
> > > older versions will not work with newer Linux.
> >
> > meaning they will keep not working, right?
>
> Right, I mean without this series they work, with this series they work
> only if the netns support is disabled or with a patch proposed but not
> merged in kata.
>
> >
> > > 2. a single process running on init_netns that wants to communicate with
> > > VMs handled by VMMs running in different netns, but this case can be
> > > solved opening the /dev/vhost-vsock in the same netns of the process
> > > that wants to communicate with the VMs (init_netns in this case), and
> > > passig it to the VMM.
> >
> > again right now they just don't work, right?
>
> Right, as above.
>
> What do you recommend I do?
Existing userspace applications must continue to work.
Guests are fine because G2H transports are always in the initial network
namespace.
On the host side we have a real case where Kata Containers and other
vsock users break. Existing applications run in other network
namespaces and assume they can communicate over vsock (it's only
available in the initial network namespace by default).
It seems we cannot isolate new network namespaces from the initial
network namespace by default because it will break existing
applications. That's a bummer.
There is one solution that maintains compatibility:
Introduce a per-namespace vsock isolation flag that can only transition
from false to true. Once it becomes true it cannot be reset to false
anymore (for security).
When vsock isolation is false the initial network namespace is used for
<CID, port> addressing.
When vsock isolation is true the current namespace is used for <CID,
port> addressing.
I guess the vsock isolation flag would be set via a rtnetlink message,
but I haven't checked.
The upshot is: existing software doesn't benefit from namespaces for
vsock isolation but it continues to work! New software makes 1 special
call after creating the namespace to opt in to vsock isolation.
This approach is secure because whoever sets up namespaces can
transition the flag from false to true and know that it can never be
reset to false anymore.
Does this make sense to everyone?
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* RE: [PATCH V2,net-next, 1/2] hv_netvsc: Add XDP support
From: Haiyang Zhang @ 2020-01-21 13:56 UTC (permalink / raw)
To: David Miller
Cc: sashal@kernel.org, linux-hyperv@vger.kernel.org,
netdev@vger.kernel.org, KY Srinivasan, Stephen Hemminger,
olaf@aepfle.de, vkuznets, linux-kernel@vger.kernel.org
In-Reply-To: <20200121.110454.2077433904156411260.davem@davemloft.net>
> -----Original Message-----
> From: linux-hyperv-owner@vger.kernel.org <linux-hyperv-
> owner@vger.kernel.org> On Behalf Of David Miller
> Sent: Tuesday, January 21, 2020 5:05 AM
> To: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: sashal@kernel.org; linux-hyperv@vger.kernel.org; netdev@vger.kernel.org;
> KY Srinivasan <kys@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; olaf@aepfle.de; vkuznets
> <vkuznets@redhat.com>; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH V2,net-next, 1/2] hv_netvsc: Add XDP support
>
> From: Haiyang Zhang <haiyangz@microsoft.com>
> Date: Mon, 20 Jan 2020 14:22:36 -0800
>
> > +u32 netvsc_run_xdp(struct net_device *ndev, struct netvsc_channel *nvchan,
> > + struct xdp_buff *xdp)
> > +{
> > + struct page *page = NULL;
> > + void *data = nvchan->rsc.data[0];
> > + u32 len = nvchan->rsc.len[0];
> > + struct bpf_prog *prog;
> > + u32 act = XDP_PASS;
>
> Please use reverse christmas tree ordering of local variables.
>
> > + xdp->data_hard_start = page_address(page);
> > + xdp->data = xdp->data_hard_start + NETVSC_XDP_HDRM;
> > + xdp_set_data_meta_invalid(xdp);
> > + xdp->data_end = xdp->data + len;
> > + xdp->rxq = &nvchan->xdp_rxq;
> > + xdp->handle = 0;
> > +
> > + memcpy(xdp->data, data, len);
>
> Why can't the program run directly on nvchan->rsc.data[0]?
>
> This data copy defeats the whole performance gain of using XDP.
Sure I will update this, and the var order.
Thanks
- Haiyang
^ permalink raw reply
* Re: [PATCH net-next 1/3] vsock: add network namespace support
From: Stefano Garzarella @ 2020-01-21 13:13 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: David Miller, netdev, linux-kernel, Jorgen Hansen, Jason Wang,
kvm, Stefan Hajnoczi, virtualization, linux-hyperv, Dexuan Cui
In-Reply-To: <20200121055403-mutt-send-email-mst@kernel.org>
On Tue, Jan 21, 2020 at 06:14:48AM -0500, Michael S. Tsirkin wrote:
> On Tue, Jan 21, 2020 at 10:07:06AM +0100, Stefano Garzarella wrote:
> > On Mon, Jan 20, 2020 at 11:02 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > On Mon, Jan 20, 2020 at 05:53:39PM +0100, Stefano Garzarella wrote:
> > > > On Mon, Jan 20, 2020 at 5:04 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > On Mon, Jan 20, 2020 at 02:58:01PM +0100, Stefano Garzarella wrote:
> > > > > > On Mon, Jan 20, 2020 at 1:03 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > > On Mon, Jan 20, 2020 at 11:17:35AM +0100, Stefano Garzarella wrote:
> > > > > > > > On Mon, Jan 20, 2020 at 10:06:10AM +0100, David Miller wrote:
> > > > > > > > > From: Stefano Garzarella <sgarzare@redhat.com>
> > > > > > > > > Date: Thu, 16 Jan 2020 18:24:26 +0100
> > > > > > > > >
> > > > > > > > > > This patch adds 'netns' module param to enable this new feature
> > > > > > > > > > (disabled by default), because it changes vsock's behavior with
> > > > > > > > > > network namespaces and could break existing applications.
> > > > > > > > >
> > > > > > > > > Sorry, no.
> > > > > > > > >
> > > > > > > > > I wonder if you can even design a legitimate, reasonable, use case
> > > > > > > > > where these netns changes could break things.
> > > > > > > >
> > > > > > > > I forgot to mention the use case.
> > > > > > > > I tried the RFC with Kata containers and we found that Kata shim-v1
> > > > > > > > doesn't work (Kata shim-v2 works as is) because there are the following
> > > > > > > > processes involved:
> > > > > > > > - kata-runtime (runs in the init_netns) opens /dev/vhost-vsock and
> > > > > > > > passes it to qemu
> > > > > > > > - kata-shim (runs in a container) wants to talk with the guest but the
> > > > > > > > vsock device is assigned to the init_netns and kata-shim runs in a
> > > > > > > > different netns, so the communication is not allowed
> > > > > > > > But, as you said, this could be a wrong design, indeed they already
> > > > > > > > found a fix, but I was not sure if others could have the same issue.
> > > > > > > >
> > > > > > > > In this case, do you think it is acceptable to make this change in
> > > > > > > > the vsock's behavior with netns and ask the user to change the design?
> > > > > > >
> > > > > > > David's question is what would be a usecase that's broken
> > > > > > > (as opposed to fixed) by enabling this by default.
> > > > > >
> > > > > > Yes, I got that. Thanks for clarifying.
> > > > > > I just reported a broken example that can be fixed with a different
> > > > > > design (due to the fact that before this series, vsock devices were
> > > > > > accessible to all netns).
> > > > > >
> > > > > > >
> > > > > > > If it does exist, you need a way for userspace to opt-in,
> > > > > > > module parameter isn't that.
> > > > > >
> > > > > > Okay, but I honestly can't find a case that can't be solved.
> > > > > > So I don't know whether to add an option (ioctl, sysfs ?) or wait for
> > > > > > a real case to come up.
> > > > > >
> > > > > > I'll try to see better if there's any particular case where we need
> > > > > > to disable netns in vsock.
> > > > > >
> > > > > > Thanks,
> > > > > > Stefano
> > > > >
> > > > > Me neither. so what did you have in mind when you wrote:
> > > > > "could break existing applications"?
> > > >
> > > > I had in mind:
> > > > 1. the Kata case. It is fixable (the fix is not merged on kata), but
> > > > older versions will not work with newer Linux.
> > >
> > > meaning they will keep not working, right?
> >
> > Right, I mean without this series they work, with this series they work
> > only if the netns support is disabled or with a patch proposed but not
> > merged in kata.
> >
> > >
> > > > 2. a single process running on init_netns that wants to communicate with
> > > > VMs handled by VMMs running in different netns, but this case can be
> > > > solved opening the /dev/vhost-vsock in the same netns of the process
> > > > that wants to communicate with the VMs (init_netns in this case), and
> > > > passig it to the VMM.
> > >
> > > again right now they just don't work, right?
> >
> > Right, as above.
> >
> > What do you recommend I do?
> >
> > Thanks,
> > Stefano
>
> If this breaks userspace, then we need to maintain compatibility.
> For example, have two devices, /dev/vhost-vsock and /dev/vhost-vsock-netns?
Interesting!
So, VMs handled with /dev/vhost-vsock will be reachable from any netns (as
it happens now) and VMs handled with /dev/vhost-vsock-netns will be
reachable only from the same netns of the process that opens it.
It requires more changes, but we will preserve the previous behavior,
adding the new feature!
Thanks a lot for this idea! I'll try to implement it!
Stefano
^ permalink raw reply
* Re: [PATCH net-next 1/3] vsock: add network namespace support
From: Michael S. Tsirkin @ 2020-01-21 11:14 UTC (permalink / raw)
To: Stefano Garzarella
Cc: David Miller, netdev, linux-kernel, Jorgen Hansen, Jason Wang,
kvm, Stefan Hajnoczi, virtualization, linux-hyperv, Dexuan Cui
In-Reply-To: <CAGxU2F4uW7FNe5xC0sb3Xxr_GABSXuu1Z9n5M=Ntq==T7MaaVw@mail.gmail.com>
On Tue, Jan 21, 2020 at 10:07:06AM +0100, Stefano Garzarella wrote:
> On Mon, Jan 20, 2020 at 11:02 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Mon, Jan 20, 2020 at 05:53:39PM +0100, Stefano Garzarella wrote:
> > > On Mon, Jan 20, 2020 at 5:04 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > On Mon, Jan 20, 2020 at 02:58:01PM +0100, Stefano Garzarella wrote:
> > > > > On Mon, Jan 20, 2020 at 1:03 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > On Mon, Jan 20, 2020 at 11:17:35AM +0100, Stefano Garzarella wrote:
> > > > > > > On Mon, Jan 20, 2020 at 10:06:10AM +0100, David Miller wrote:
> > > > > > > > From: Stefano Garzarella <sgarzare@redhat.com>
> > > > > > > > Date: Thu, 16 Jan 2020 18:24:26 +0100
> > > > > > > >
> > > > > > > > > This patch adds 'netns' module param to enable this new feature
> > > > > > > > > (disabled by default), because it changes vsock's behavior with
> > > > > > > > > network namespaces and could break existing applications.
> > > > > > > >
> > > > > > > > Sorry, no.
> > > > > > > >
> > > > > > > > I wonder if you can even design a legitimate, reasonable, use case
> > > > > > > > where these netns changes could break things.
> > > > > > >
> > > > > > > I forgot to mention the use case.
> > > > > > > I tried the RFC with Kata containers and we found that Kata shim-v1
> > > > > > > doesn't work (Kata shim-v2 works as is) because there are the following
> > > > > > > processes involved:
> > > > > > > - kata-runtime (runs in the init_netns) opens /dev/vhost-vsock and
> > > > > > > passes it to qemu
> > > > > > > - kata-shim (runs in a container) wants to talk with the guest but the
> > > > > > > vsock device is assigned to the init_netns and kata-shim runs in a
> > > > > > > different netns, so the communication is not allowed
> > > > > > > But, as you said, this could be a wrong design, indeed they already
> > > > > > > found a fix, but I was not sure if others could have the same issue.
> > > > > > >
> > > > > > > In this case, do you think it is acceptable to make this change in
> > > > > > > the vsock's behavior with netns and ask the user to change the design?
> > > > > >
> > > > > > David's question is what would be a usecase that's broken
> > > > > > (as opposed to fixed) by enabling this by default.
> > > > >
> > > > > Yes, I got that. Thanks for clarifying.
> > > > > I just reported a broken example that can be fixed with a different
> > > > > design (due to the fact that before this series, vsock devices were
> > > > > accessible to all netns).
> > > > >
> > > > > >
> > > > > > If it does exist, you need a way for userspace to opt-in,
> > > > > > module parameter isn't that.
> > > > >
> > > > > Okay, but I honestly can't find a case that can't be solved.
> > > > > So I don't know whether to add an option (ioctl, sysfs ?) or wait for
> > > > > a real case to come up.
> > > > >
> > > > > I'll try to see better if there's any particular case where we need
> > > > > to disable netns in vsock.
> > > > >
> > > > > Thanks,
> > > > > Stefano
> > > >
> > > > Me neither. so what did you have in mind when you wrote:
> > > > "could break existing applications"?
> > >
> > > I had in mind:
> > > 1. the Kata case. It is fixable (the fix is not merged on kata), but
> > > older versions will not work with newer Linux.
> >
> > meaning they will keep not working, right?
>
> Right, I mean without this series they work, with this series they work
> only if the netns support is disabled or with a patch proposed but not
> merged in kata.
>
> >
> > > 2. a single process running on init_netns that wants to communicate with
> > > VMs handled by VMMs running in different netns, but this case can be
> > > solved opening the /dev/vhost-vsock in the same netns of the process
> > > that wants to communicate with the VMs (init_netns in this case), and
> > > passig it to the VMM.
> >
> > again right now they just don't work, right?
>
> Right, as above.
>
> What do you recommend I do?
>
> Thanks,
> Stefano
If this breaks userspace, then we need to maintain compatibility.
For example, have two devices, /dev/vhost-vsock and /dev/vhost-vsock-netns?
--
MST
^ permalink raw reply
* Re: [PATCH V2,net-next, 1/2] hv_netvsc: Add XDP support
From: David Miller @ 2020-01-21 10:04 UTC (permalink / raw)
To: haiyangz
Cc: sashal, linux-hyperv, netdev, kys, sthemmin, olaf, vkuznets,
linux-kernel
In-Reply-To: <1579558957-62496-2-git-send-email-haiyangz@microsoft.com>
From: Haiyang Zhang <haiyangz@microsoft.com>
Date: Mon, 20 Jan 2020 14:22:36 -0800
> +u32 netvsc_run_xdp(struct net_device *ndev, struct netvsc_channel *nvchan,
> + struct xdp_buff *xdp)
> +{
> + struct page *page = NULL;
> + void *data = nvchan->rsc.data[0];
> + u32 len = nvchan->rsc.len[0];
> + struct bpf_prog *prog;
> + u32 act = XDP_PASS;
Please use reverse christmas tree ordering of local variables.
> + xdp->data_hard_start = page_address(page);
> + xdp->data = xdp->data_hard_start + NETVSC_XDP_HDRM;
> + xdp_set_data_meta_invalid(xdp);
> + xdp->data_end = xdp->data + len;
> + xdp->rxq = &nvchan->xdp_rxq;
> + xdp->handle = 0;
> +
> + memcpy(xdp->data, data, len);
Why can't the program run directly on nvchan->rsc.data[0]?
This data copy defeats the whole performance gain of using XDP.
^ permalink raw reply
* Re: [PATCH 2/2] pci: hyperv: Move retarget related struct definitions into tlfs
From: Vitaly Kuznetsov @ 2020-01-21 9:25 UTC (permalink / raw)
To: Boqun Feng
Cc: Michael Kelley, Boqun Feng, K. Y. Srinivasan, Haiyang Zhang,
Stephen Hemminger, Sasha Levin, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
Lorenzo Pieralisi, Andrew Murray, Bjorn Helgaas,
open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS,
linux-hyperv, linux-kernel
In-Reply-To: <20200121015713.69691-2-boqun.feng@gmail.com>
Boqun Feng <boqun.feng@gmail.com> writes:
> For future support of virtual PCI on non-x86 architecture.
>
> Signed-off-by: Boqun Feng (Microsoft) <boqun.feng@gmail.com>
> ---
> arch/x86/include/asm/hyperv-tlfs.h | 38 +++++++++++++++++++++++++++++
> arch/x86/include/asm/mshyperv.h | 8 ++++++
> drivers/pci/controller/pci-hyperv.c | 38 +++--------------------------
> 3 files changed, 50 insertions(+), 34 deletions(-)
>
> diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
> index b9ebc20b2385..debe017ae748 100644
> --- a/arch/x86/include/asm/hyperv-tlfs.h
> +++ b/arch/x86/include/asm/hyperv-tlfs.h
> @@ -912,4 +912,42 @@ struct hv_tlb_flush_ex {
> struct hv_partition_assist_pg {
> u32 tlb_lock_count;
> };
> +
> +union hv_msi_entry {
> + u64 as_uint64;
> + struct {
> + u32 address;
> + u32 data;
> + } __packed;
> +};
While Hyper-V code is full of this, I was once told that 'Union aliasing
is UB. Avoid it for good.' Maybe we should start getting rid of it
instead of adding more?
> +
> +struct hv_interrupt_entry {
> + u32 source; /* 1 for MSI(-X) */
> + u32 reserved1;
> + union hv_msi_entry msi_entry;
> +} __packed;
> +
> +/*
> + * flags for hv_device_interrupt_target.flags
> + */
> +#define HV_DEVICE_INTERRUPT_TARGET_MULTICAST 1
> +#define HV_DEVICE_INTERRUPT_TARGET_PROCESSOR_SET 2
> +
> +struct hv_device_interrupt_target {
> + u32 vector;
> + u32 flags;
> + union {
> + u64 vp_mask;
> + struct hv_vpset vp_set;
> + };
> +} __packed;
> +
> +/* HvRetargetDeviceInterrupt hypercall */
> +struct hv_retarget_device_interrupt {
> + u64 partition_id;
> + u64 device_id;
> + struct hv_interrupt_entry int_entry;
> + u64 reserved2;
> + struct hv_device_interrupt_target int_target;
> +} __packed __aligned(8);
> #endif
> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
> index 6b79515abb82..d13319d82f6b 100644
> --- a/arch/x86/include/asm/mshyperv.h
> +++ b/arch/x86/include/asm/mshyperv.h
> @@ -240,6 +240,14 @@ bool hv_vcpu_is_preempted(int vcpu);
> static inline void hv_apic_init(void) {}
> #endif
>
> +#if IS_ENABLED(CONFIG_PCI_HYPERV)
> +#define hv_set_msi_address_from_desc(msi_entry, msi_desc) \
> +do { \
> + (msi_entry)->address = (msi_desc)->msg.address_lo; \
> +} while (0)
> +
> +#endif /* CONFIG_PCI_HYPERV */
It seems to be pointless to put defines under #if IS_ENABLED(): in case
it is not enabled and used you'll get a compilation error, in case it is
enabled and not used no code is going to be generated anyways.
> +
> #else /* CONFIG_HYPERV */
> static inline void hyperv_init(void) {}
> static inline void hyperv_setup_mmu_ops(void) {}
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index aacfcc90d929..2240f2b3643e 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -406,36 +406,6 @@ struct pci_eject_response {
>
> static int pci_ring_size = (4 * PAGE_SIZE);
>
> -struct hv_interrupt_entry {
> - u32 source; /* 1 for MSI(-X) */
> - u32 reserved1;
> - u32 address;
> - u32 data;
> -};
> -
> -/*
> - * flags for hv_device_interrupt_target.flags
> - */
> -#define HV_DEVICE_INTERRUPT_TARGET_MULTICAST 1
> -#define HV_DEVICE_INTERRUPT_TARGET_PROCESSOR_SET 2
> -
> -struct hv_device_interrupt_target {
> - u32 vector;
> - u32 flags;
> - union {
> - u64 vp_mask;
> - struct hv_vpset vp_set;
> - };
> -};
> -
> -struct retarget_msi_interrupt {
> - u64 partition_id; /* use "self" */
> - u64 device_id;
> - struct hv_interrupt_entry int_entry;
> - u64 reserved2;
> - struct hv_device_interrupt_target int_target;
> -} __packed __aligned(8);
> -
> /*
> * Driver specific state.
> */
> @@ -482,7 +452,7 @@ struct hv_pcibus_device {
> struct workqueue_struct *wq;
>
> /* hypercall arg, must not cross page boundary */
> - struct retarget_msi_interrupt retarget_msi_interrupt_params;
> + struct hv_retarget_device_interrupt retarget_msi_interrupt_params;
>
> /*
> * Don't put anything here: retarget_msi_interrupt_params must be last
> @@ -1178,7 +1148,7 @@ static void hv_irq_unmask(struct irq_data *data)
> {
> struct msi_desc *msi_desc = irq_data_get_msi_desc(data);
> struct irq_cfg *cfg = irqd_cfg(data);
> - struct retarget_msi_interrupt *params;
> + struct hv_retarget_device_interrupt *params;
> struct hv_pcibus_device *hbus;
> struct cpumask *dest;
> cpumask_var_t tmp;
> @@ -1200,8 +1170,8 @@ static void hv_irq_unmask(struct irq_data *data)
> memset(params, 0, sizeof(*params));
> params->partition_id = HV_PARTITION_ID_SELF;
> params->int_entry.source = 1; /* MSI(-X) */
> - params->int_entry.address = msi_desc->msg.address_lo;
> - params->int_entry.data = msi_desc->msg.data;
> + hv_set_msi_address_from_desc(¶ms->int_entry.msi_entry, msi_desc);
I don't quite see why this hv_set_msi_address_from_desc() is needed at
all.
> + params->int_entry.msi_entry.data = msi_desc->msg.data;
> params->device_id = (hbus->hdev->dev_instance.b[5] << 24) |
> (hbus->hdev->dev_instance.b[4] << 16) |
> (hbus->hdev->dev_instance.b[7] << 8) |
--
Vitaly
^ permalink raw reply
* Re: [PATCH net-next 1/3] vsock: add network namespace support
From: Stefano Garzarella @ 2020-01-21 9:07 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: David Miller, netdev, linux-kernel, Jorgen Hansen, Jason Wang,
kvm, Stefan Hajnoczi, virtualization, linux-hyperv, Dexuan Cui
In-Reply-To: <20200120170120-mutt-send-email-mst@kernel.org>
On Mon, Jan 20, 2020 at 11:02 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Jan 20, 2020 at 05:53:39PM +0100, Stefano Garzarella wrote:
> > On Mon, Jan 20, 2020 at 5:04 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > On Mon, Jan 20, 2020 at 02:58:01PM +0100, Stefano Garzarella wrote:
> > > > On Mon, Jan 20, 2020 at 1:03 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > On Mon, Jan 20, 2020 at 11:17:35AM +0100, Stefano Garzarella wrote:
> > > > > > On Mon, Jan 20, 2020 at 10:06:10AM +0100, David Miller wrote:
> > > > > > > From: Stefano Garzarella <sgarzare@redhat.com>
> > > > > > > Date: Thu, 16 Jan 2020 18:24:26 +0100
> > > > > > >
> > > > > > > > This patch adds 'netns' module param to enable this new feature
> > > > > > > > (disabled by default), because it changes vsock's behavior with
> > > > > > > > network namespaces and could break existing applications.
> > > > > > >
> > > > > > > Sorry, no.
> > > > > > >
> > > > > > > I wonder if you can even design a legitimate, reasonable, use case
> > > > > > > where these netns changes could break things.
> > > > > >
> > > > > > I forgot to mention the use case.
> > > > > > I tried the RFC with Kata containers and we found that Kata shim-v1
> > > > > > doesn't work (Kata shim-v2 works as is) because there are the following
> > > > > > processes involved:
> > > > > > - kata-runtime (runs in the init_netns) opens /dev/vhost-vsock and
> > > > > > passes it to qemu
> > > > > > - kata-shim (runs in a container) wants to talk with the guest but the
> > > > > > vsock device is assigned to the init_netns and kata-shim runs in a
> > > > > > different netns, so the communication is not allowed
> > > > > > But, as you said, this could be a wrong design, indeed they already
> > > > > > found a fix, but I was not sure if others could have the same issue.
> > > > > >
> > > > > > In this case, do you think it is acceptable to make this change in
> > > > > > the vsock's behavior with netns and ask the user to change the design?
> > > > >
> > > > > David's question is what would be a usecase that's broken
> > > > > (as opposed to fixed) by enabling this by default.
> > > >
> > > > Yes, I got that. Thanks for clarifying.
> > > > I just reported a broken example that can be fixed with a different
> > > > design (due to the fact that before this series, vsock devices were
> > > > accessible to all netns).
> > > >
> > > > >
> > > > > If it does exist, you need a way for userspace to opt-in,
> > > > > module parameter isn't that.
> > > >
> > > > Okay, but I honestly can't find a case that can't be solved.
> > > > So I don't know whether to add an option (ioctl, sysfs ?) or wait for
> > > > a real case to come up.
> > > >
> > > > I'll try to see better if there's any particular case where we need
> > > > to disable netns in vsock.
> > > >
> > > > Thanks,
> > > > Stefano
> > >
> > > Me neither. so what did you have in mind when you wrote:
> > > "could break existing applications"?
> >
> > I had in mind:
> > 1. the Kata case. It is fixable (the fix is not merged on kata), but
> > older versions will not work with newer Linux.
>
> meaning they will keep not working, right?
Right, I mean without this series they work, with this series they work
only if the netns support is disabled or with a patch proposed but not
merged in kata.
>
> > 2. a single process running on init_netns that wants to communicate with
> > VMs handled by VMMs running in different netns, but this case can be
> > solved opening the /dev/vhost-vsock in the same netns of the process
> > that wants to communicate with the VMs (init_netns in this case), and
> > passig it to the VMM.
>
> again right now they just don't work, right?
Right, as above.
What do you recommend I do?
Thanks,
Stefano
^ permalink raw reply
* RE: [PATCH V4] x86/Hyper-V: Balloon up according to request page number
From: Michael Kelley @ 2020-01-21 4:36 UTC (permalink / raw)
To: lantianyu1986@gmail.com, KY Srinivasan, Haiyang Zhang,
Stephen Hemminger, sashal@kernel.org
Cc: Tianyu Lan, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, vkuznets, stable@vger.kernel.org
In-Reply-To: <20200121034912.2725-1-Tianyu.Lan@microsoft.com>
From: Tianyu Lan <Tianyu.Lan@microsoft.com> Sent: Monday, January 20, 2020 7:49 PM
>
> Current code has assumption that balloon request memory size aligns
> with 2MB. But actually Hyper-V doesn't guarantee such alignment. When
> balloon driver receives non-aligned balloon request, it produces warning
> and balloon up more memory than requested in order to keep 2MB alignment.
> Remove the warning and balloon up memory according to actual requested
> memory size.
>
> Fixes: f6712238471a ("hv: hv_balloon: avoid memory leak on alloc_error of 2MB memory
> block")
> Cc: stable@vger.kernel.org
> Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
^ permalink raw reply
* [PATCH V4] x86/Hyper-V: Balloon up according to request page number
From: lantianyu1986 @ 2020-01-21 3:49 UTC (permalink / raw)
To: kys, haiyangz, sthemmin, sashal, michael.h.kelley
Cc: Tianyu Lan, linux-hyperv, linux-kernel, vkuznets, stable
From: Tianyu Lan <Tianyu.Lan@microsoft.com>
Current code has assumption that balloon request memory size aligns
with 2MB. But actually Hyper-V doesn't guarantee such alignment. When
balloon driver receives non-aligned balloon request, it produces warning
and balloon up more memory than requested in order to keep 2MB alignment.
Remove the warning and balloon up memory according to actual requested
memory size.
Fixes: f6712238471a ("hv: hv_balloon: avoid memory leak on alloc_error of 2MB memory block")
Cc: stable@vger.kernel.org
Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com>
---
Change since v3:
- Revert optimization of swtiching alloc_unit
Change since v2:
- Remove check between request page number and alloc_unit
in the alloc_balloon_pages() because it's redundant with
new change.
- Remove the "continue" just follwoing alloc_unit switch
from 2MB to 4K in order to avoid skipping allocated
memory.
Change since v1:
- Change logic of switching alloc_unit from 2MB to 4KB
in the balloon_up() to avoid redundant iteration when
handle non-aligned page request.
- Remove 2MB alignment operation and comment in balloon_up()
---
drivers/hv/hv_balloon.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
index 7f3e7ab22d5d..a03c5191101e 100644
--- a/drivers/hv/hv_balloon.c
+++ b/drivers/hv/hv_balloon.c
@@ -1681,10 +1681,7 @@ static unsigned int alloc_balloon_pages(struct hv_dynmem_device *dm,
unsigned int i, j;
struct page *pg;
- if (num_pages < alloc_unit)
- return 0;
-
- for (i = 0; (i * alloc_unit) < num_pages; i++) {
+ for (i = 0; i < num_pages / alloc_unit; i++) {
if (bl_resp->hdr.size + sizeof(union dm_mem_page_range) >
HV_HYP_PAGE_SIZE)
return i * alloc_unit;
@@ -1722,7 +1719,7 @@ static unsigned int alloc_balloon_pages(struct hv_dynmem_device *dm,
}
- return num_pages;
+ return i * alloc_unit;
}
static void balloon_up(union dm_msg_info *msg_info)
@@ -1737,9 +1734,6 @@ static void balloon_up(union dm_msg_info *msg_info)
long avail_pages;
unsigned long floor;
- /* The host balloons pages in 2M granularity. */
- WARN_ON_ONCE(num_pages % PAGES_IN_2M != 0);
-
/*
* We will attempt 2M allocations. However, if we fail to
* allocate 2M chunks, we will go back to PAGE_SIZE allocations.
@@ -1749,14 +1743,13 @@ static void balloon_up(union dm_msg_info *msg_info)
avail_pages = si_mem_available();
floor = compute_balloon_floor();
- /* Refuse to balloon below the floor, keep the 2M granularity. */
+ /* Refuse to balloon below the floor. */
if (avail_pages < num_pages || avail_pages - num_pages < floor) {
pr_warn("Balloon request will be partially fulfilled. %s\n",
avail_pages < num_pages ? "Not enough memory." :
"Balloon floor reached.");
num_pages = avail_pages > floor ? (avail_pages - floor) : 0;
- num_pages -= num_pages % PAGES_IN_2M;
}
while (!done) {
--
2.14.5
^ permalink raw reply related
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