diff mbox series

[RFC,net-next,15/18] virtio_net: implement XDP prog offload functionality

Message ID 20191126100744.5083-16-prashantbhole.linux@gmail.com (mailing list archive)
State New, archived
Headers show
Series virtio_net XDP offload | expand

Commit Message

Prashant Bhole Nov. 26, 2019, 10:07 a.m. UTC 
From: Jason Wang <jasowang@redhat.com>

This patch implements bpf_prog_offload_ops callbacks and adds handling
for XDP_SETUP_PROG_HW. Handling of XDP_SETUP_PROG_HW involves setting
up struct virtio_net_ctrl_ebpf_prog and appending program instructions
to it. This control buffer is sent to Qemu with class
VIRTIO_NET_CTRL_EBPF and command VIRTIO_NET_BPF_CMD_SET_OFFLOAD.
The expected behavior from Qemu is that it should try to load the
program in host os and report the status.

It also adds restriction to have either driver or offloaded program
at a time. This restriction can be removed later after proper testing.

Signed-off-by: Jason Wang <jasowang@redhat.com>
Co-developed-by: Prashant Bhole <prashantbhole.linux@gmail.com>
Signed-off-by: Prashant Bhole <prashantbhole.linux@gmail.com>
---
 drivers/net/virtio_net.c        | 114 +++++++++++++++++++++++++++++++-
 include/uapi/linux/virtio_net.h |  27 ++++++++
 2 files changed, 139 insertions(+), 2 deletions(-)

Comments

Michael S. Tsirkin Nov. 27, 2019, 8:42 p.m. UTC | #1 
On Tue, Nov 26, 2019 at 07:07:41PM +0900, Prashant Bhole wrote:
> From: Jason Wang <jasowang@redhat.com>
> 
> This patch implements bpf_prog_offload_ops callbacks and adds handling
> for XDP_SETUP_PROG_HW. Handling of XDP_SETUP_PROG_HW involves setting
> up struct virtio_net_ctrl_ebpf_prog and appending program instructions
> to it. This control buffer is sent to Qemu with class
> VIRTIO_NET_CTRL_EBPF and command VIRTIO_NET_BPF_CMD_SET_OFFLOAD.
> The expected behavior from Qemu is that it should try to load the
> program in host os and report the status.

That's not great e.g. for migration: different hosts might have
a different idea about what's allowed.
Device capabilities should be preferably exported through
feature bits or config space such that it's easy to
intercept and limit these as needed.

Also, how are we going to handle e.g. endian-ness here?

> 
> It also adds restriction to have either driver or offloaded program
> at a time.

I'm not sure I understand what does the above say.
virtnet_xdp_offload_check?
Please add code comments so we remember what to do and when.

> This restriction can be removed later after proper testing.

What kind of testing is missing here?

> Signed-off-by: Jason Wang <jasowang@redhat.com>
> Co-developed-by: Prashant Bhole <prashantbhole.linux@gmail.com>
> Signed-off-by: Prashant Bhole <prashantbhole.linux@gmail.com>

Any UAPI changes need to be copied to virtio-dev@lists.oasis-open.org
(subscriber only) list please.

> ---
>  drivers/net/virtio_net.c        | 114 +++++++++++++++++++++++++++++++-
>  include/uapi/linux/virtio_net.h |  27 ++++++++
>  2 files changed, 139 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index a1088d0114f2..dddfbb2a2075 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -169,6 +169,7 @@ struct control_buf {
>  	u8 allmulti;
>  	__virtio16 vid;
>  	__virtio64 offloads;
> +	struct virtio_net_ctrl_ebpf_prog prog_ctrl;
>  };
>  
>  struct virtnet_info {
> @@ -272,6 +273,8 @@ struct virtnet_bpf_bound_prog {
>  	struct bpf_insn insnsi[0];
>  };
>  
> +#define VIRTNET_EA(extack, msg)	NL_SET_ERR_MSG_MOD((extack), msg)
> +
>  /* Converting between virtqueue no. and kernel tx/rx queue no.
>   * 0:rx0 1:tx0 2:rx1 3:tx1 ... 2N:rxN 2N+1:txN 2N+2:cvq
>   */
> @@ -2427,6 +2430,11 @@ static int virtnet_xdp_set(struct net_device *dev, struct netdev_bpf *bpf)
>  	if (!xdp_attachment_flags_ok(&vi->xdp, bpf))
>  		return -EBUSY;
>  
> +	if (rtnl_dereference(vi->offload_xdp_prog)) {
> +		VIRTNET_EA(bpf->extack, "program already attached in offload mode");
> +		return -EINVAL;
> +	}
> +
>  	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)
>  	    && (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO4) ||
>  	        virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO6) ||
> @@ -2528,17 +2536,114 @@ static int virtnet_bpf_verify_insn(struct bpf_verifier_env *env, int insn_idx,
>  
>  static void virtnet_bpf_destroy_prog(struct bpf_prog *prog)
>  {
> +	struct virtnet_bpf_bound_prog *state;
> +
> +	state = prog->aux->offload->dev_priv;
> +	list_del(&state->list);
> +	kfree(state);
> +}
> +
> +static int virtnet_xdp_offload_check(struct virtnet_info *vi,
> +				     struct netdev_bpf *bpf)
> +{
> +	if (!bpf->prog)
> +		return 0;
> +
> +	if (!bpf->prog->aux->offload) {
> +		VIRTNET_EA(bpf->extack, "xdpoffload of non-bound program");
> +		return -EINVAL;
> +	}
> +	if (bpf->prog->aux->offload->netdev != vi->dev) {
> +		VIRTNET_EA(bpf->extack, "program bound to different dev");
> +		return -EINVAL;
> +	}
> +
> +	if (rtnl_dereference(vi->xdp_prog)) {
> +		VIRTNET_EA(bpf->extack, "program already attached in driver mode");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
>  }
>  
>  static int virtnet_xdp_set_offload(struct virtnet_info *vi,
>  				   struct netdev_bpf *bpf)
>  {
> -	return -EBUSY;
> +	struct virtio_net_ctrl_ebpf_prog *ctrl;
> +	struct virtnet_bpf_bound_prog *bound_prog = NULL;
> +	struct virtio_device *vdev = vi->vdev;
> +	struct bpf_prog *prog = bpf->prog;
> +	void *ctrl_buf = NULL;
> +	struct scatterlist sg;
> +	int prog_len;
> +	int err = 0;
> +
> +	if (!xdp_attachment_flags_ok(&vi->xdp_hw, bpf))
> +		return -EBUSY;
> +
> +	if (prog) {
> +		if (prog->type != BPF_PROG_TYPE_XDP)
> +			return -EOPNOTSUPP;
> +		bound_prog = prog->aux->offload->dev_priv;
> +		prog_len = prog->len * sizeof(bound_prog->insnsi[0]);
> +
> +		ctrl_buf = kmalloc(GFP_KERNEL, sizeof(*ctrl) + prog_len);
> +		if (!ctrl_buf)
> +			return -ENOMEM;
> +		ctrl = ctrl_buf;
> +		ctrl->cmd = cpu_to_virtio32(vi->vdev,
> +					    VIRTIO_NET_BPF_CMD_SET_OFFLOAD);
> +		ctrl->len = cpu_to_virtio32(vi->vdev, prog_len);
> +		ctrl->gpl_compatible = cpu_to_virtio16(vi->vdev,
> +						       prog->gpl_compatible);
> +		memcpy(ctrl->insns, bound_prog->insnsi,
> +		       prog->len * sizeof(bound_prog->insnsi[0]));
> +		sg_init_one(&sg, ctrl_buf, sizeof(*ctrl) + prog_len);
> +	} else {
> +		ctrl = &vi->ctrl->prog_ctrl;
> +		ctrl->cmd  = cpu_to_virtio32(vi->vdev,
> +					     VIRTIO_NET_BPF_CMD_UNSET_OFFLOAD);
> +		sg_init_one(&sg, ctrl, sizeof(*ctrl));
> +	}
> +
> +	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_EBPF,
> +				  VIRTIO_NET_CTRL_EBPF_PROG,
> +				  &sg)) {
> +		dev_warn(&vdev->dev, "Failed to set bpf offload prog\n");
> +		err = -EFAULT;
> +		goto out;
> +	}
> +
> +	rcu_assign_pointer(vi->offload_xdp_prog, prog);
> +
> +	xdp_attachment_setup(&vi->xdp_hw, bpf);
> +
> +out:
> +	kfree(ctrl_buf);
> +	return err;
>  }
>  
>  static int virtnet_bpf_verifier_setup(struct bpf_prog *prog)
>  {
> -	return -ENOMEM;
> +	struct virtnet_info *vi = netdev_priv(prog->aux->offload->netdev);
> +	size_t insn_len = prog->len * sizeof(struct bpf_insn);
> +	struct virtnet_bpf_bound_prog *state;
> +
> +	state = kzalloc(sizeof(*state) + insn_len, GFP_KERNEL);
> +	if (!state)
> +		return -ENOMEM;
> +
> +	memcpy(&state->insnsi[0], prog->insnsi, insn_len);
> +
> +	state->vi = vi;
> +	state->prog = prog;
> +	state->len = prog->len;
> +
> +	list_add_tail(&state->list, &vi->bpf_bound_progs);
> +
> +	prog->aux->offload->dev_priv = state;
> +
> +	return 0;
>  }
>  
>  static int virtnet_bpf_verifier_prep(struct bpf_prog *prog)
> @@ -2568,12 +2673,17 @@ static const struct bpf_prog_offload_ops virtnet_bpf_dev_ops = {
>  static int virtnet_xdp(struct net_device *dev, struct netdev_bpf *xdp)
>  {
>  	struct virtnet_info *vi = netdev_priv(dev);
> +	int err;
> +
>  	switch (xdp->command) {
>  	case XDP_SETUP_PROG:
>  		return virtnet_xdp_set(dev, xdp);
>  	case XDP_QUERY_PROG:
>  		return xdp_attachment_query(&vi->xdp, xdp);
>  	case XDP_SETUP_PROG_HW:
> +		err = virtnet_xdp_offload_check(vi, xdp);
> +		if (err)
> +			return err;
>  		return virtnet_xdp_set_offload(vi, xdp);
>  	case XDP_QUERY_PROG_HW:
>  		return xdp_attachment_query(&vi->xdp_hw, xdp);
> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> index a3715a3224c1..0ea2f7910a5a 100644
> --- a/include/uapi/linux/virtio_net.h
> +++ b/include/uapi/linux/virtio_net.h
> @@ -261,4 +261,31 @@ struct virtio_net_ctrl_mq {
>  #define VIRTIO_NET_CTRL_GUEST_OFFLOADS   5
>  #define VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET        0
>  
> +/*
> + * Control XDP offloads offloads
> + *
> + * When guest wants to offload XDP program to the device, it calls
> + * VIRTIO_NET_CTRL_EBPF_PROG along with VIRTIO_NET_BPF_CMD_SET_OFFLOAD
> + * subcommands. When offloading is successful, the device runs offloaded
> + * XDP program for each packet before sending it to the guest.
> + *
> + * VIRTIO_NET_BPF_CMD_UNSET_OFFLOAD removes the the offloaded program from
> + * the device, if exists.
> + */
> +
> +struct virtio_net_ctrl_ebpf_prog {
> +	/* program length in bytes */
> +	__virtio32 len;
> +	__virtio16 cmd;
> +	__virtio16 gpl_compatible;
> +	__u8 insns[0];
> +};
> +
> +#define VIRTIO_NET_CTRL_EBPF 6
> + #define VIRTIO_NET_CTRL_EBPF_PROG 1
> +
> +/* Commands for VIRTIO_NET_CTRL_EBPF_PROG */
> +#define VIRTIO_NET_BPF_CMD_SET_OFFLOAD 1
> +#define VIRTIO_NET_BPF_CMD_UNSET_OFFLOAD 2
> +
>  #endif /* _UAPI_LINUX_VIRTIO_NET_H */
> -- 
> 2.20.1
Prashant Bhole Nov. 28, 2019, 2:53 a.m. UTC | #2 
On 11/28/19 5:42 AM, Michael S. Tsirkin wrote:
> On Tue, Nov 26, 2019 at 07:07:41PM +0900, Prashant Bhole wrote:
>> From: Jason Wang <jasowang@redhat.com>
>>
>> This patch implements bpf_prog_offload_ops callbacks and adds handling
>> for XDP_SETUP_PROG_HW. Handling of XDP_SETUP_PROG_HW involves setting
>> up struct virtio_net_ctrl_ebpf_prog and appending program instructions
>> to it. This control buffer is sent to Qemu with class
>> VIRTIO_NET_CTRL_EBPF and command VIRTIO_NET_BPF_CMD_SET_OFFLOAD.
>> The expected behavior from Qemu is that it should try to load the
>> program in host os and report the status.
> 
> That's not great e.g. for migration: different hosts might have
> a different idea about what's allowed.
> Device capabilities should be preferably exported through
> feature bits or config space such that it's easy to
> intercept and limit these as needed.

These things are mentioned in the TODO section of cover letter.
Having offload feature enabled should be a migration blocker.
A feature bit in virtio for offloading capability needs to be added.

> 
> Also, how are we going to handle e.g. endian-ness here?

For now I feel we should block offloading in case of cross endian
virtualization. Further to support cross endian-ness, the requests for 
offloading a map or program should include metadata such as BTF info.
Qemu needs to handle the conversion.

> 
>>
>> It also adds restriction to have either driver or offloaded program
>> at a time.
> 
> I'm not sure I understand what does the above say.
> virtnet_xdp_offload_check?
> Please add code comments so we remember what to do and when.
> 
>> This restriction can be removed later after proper testing.
> 
> What kind of testing is missing here?

This restriction mentioned above is about having multiple programs
attached to the same device. It can be possible to have a program
attached in driver mode and other in offload mode, but in current code
only one mode at a time is supported because I wasn't aware whether
bpf tooling supports the case. I will add a comment or remove the
restriction in the next revision.

> 
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> Co-developed-by: Prashant Bhole <prashantbhole.linux@gmail.com>
>> Signed-off-by: Prashant Bhole <prashantbhole.linux@gmail.com>
> 
> Any UAPI changes need to be copied to virtio-dev@lists.oasis-open.org
> (subscriber only) list please.

Sure.

Thanks.
diff mbox series

Patch

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index a1088d0114f2..dddfbb2a2075 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -169,6 +169,7 @@  struct control_buf {
 	u8 allmulti;
 	__virtio16 vid;
 	__virtio64 offloads;
+	struct virtio_net_ctrl_ebpf_prog prog_ctrl;
 };
 
 struct virtnet_info {
@@ -272,6 +273,8 @@  struct virtnet_bpf_bound_prog {
 	struct bpf_insn insnsi[0];
 };
 
+#define VIRTNET_EA(extack, msg)	NL_SET_ERR_MSG_MOD((extack), msg)
+
 /* Converting between virtqueue no. and kernel tx/rx queue no.
  * 0:rx0 1:tx0 2:rx1 3:tx1 ... 2N:rxN 2N+1:txN 2N+2:cvq
  */
@@ -2427,6 +2430,11 @@  static int virtnet_xdp_set(struct net_device *dev, struct netdev_bpf *bpf)
 	if (!xdp_attachment_flags_ok(&vi->xdp, bpf))
 		return -EBUSY;
 
+	if (rtnl_dereference(vi->offload_xdp_prog)) {
+		VIRTNET_EA(bpf->extack, "program already attached in offload mode");
+		return -EINVAL;
+	}
+
 	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)
 	    && (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO4) ||
 	        virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO6) ||
@@ -2528,17 +2536,114 @@  static int virtnet_bpf_verify_insn(struct bpf_verifier_env *env, int insn_idx,
 
 static void virtnet_bpf_destroy_prog(struct bpf_prog *prog)
 {
+	struct virtnet_bpf_bound_prog *state;
+
+	state = prog->aux->offload->dev_priv;
+	list_del(&state->list);
+	kfree(state);
+}
+
+static int virtnet_xdp_offload_check(struct virtnet_info *vi,
+				     struct netdev_bpf *bpf)
+{
+	if (!bpf->prog)
+		return 0;
+
+	if (!bpf->prog->aux->offload) {
+		VIRTNET_EA(bpf->extack, "xdpoffload of non-bound program");
+		return -EINVAL;
+	}
+	if (bpf->prog->aux->offload->netdev != vi->dev) {
+		VIRTNET_EA(bpf->extack, "program bound to different dev");
+		return -EINVAL;
+	}
+
+	if (rtnl_dereference(vi->xdp_prog)) {
+		VIRTNET_EA(bpf->extack, "program already attached in driver mode");
+		return -EINVAL;
+	}
+
+	return 0;
 }
 
 static int virtnet_xdp_set_offload(struct virtnet_info *vi,
 				   struct netdev_bpf *bpf)
 {
-	return -EBUSY;
+	struct virtio_net_ctrl_ebpf_prog *ctrl;
+	struct virtnet_bpf_bound_prog *bound_prog = NULL;
+	struct virtio_device *vdev = vi->vdev;
+	struct bpf_prog *prog = bpf->prog;
+	void *ctrl_buf = NULL;
+	struct scatterlist sg;
+	int prog_len;
+	int err = 0;
+
+	if (!xdp_attachment_flags_ok(&vi->xdp_hw, bpf))
+		return -EBUSY;
+
+	if (prog) {
+		if (prog->type != BPF_PROG_TYPE_XDP)
+			return -EOPNOTSUPP;
+		bound_prog = prog->aux->offload->dev_priv;
+		prog_len = prog->len * sizeof(bound_prog->insnsi[0]);
+
+		ctrl_buf = kmalloc(GFP_KERNEL, sizeof(*ctrl) + prog_len);
+		if (!ctrl_buf)
+			return -ENOMEM;
+		ctrl = ctrl_buf;
+		ctrl->cmd = cpu_to_virtio32(vi->vdev,
+					    VIRTIO_NET_BPF_CMD_SET_OFFLOAD);
+		ctrl->len = cpu_to_virtio32(vi->vdev, prog_len);
+		ctrl->gpl_compatible = cpu_to_virtio16(vi->vdev,
+						       prog->gpl_compatible);
+		memcpy(ctrl->insns, bound_prog->insnsi,
+		       prog->len * sizeof(bound_prog->insnsi[0]));
+		sg_init_one(&sg, ctrl_buf, sizeof(*ctrl) + prog_len);
+	} else {
+		ctrl = &vi->ctrl->prog_ctrl;
+		ctrl->cmd  = cpu_to_virtio32(vi->vdev,
+					     VIRTIO_NET_BPF_CMD_UNSET_OFFLOAD);
+		sg_init_one(&sg, ctrl, sizeof(*ctrl));
+	}
+
+	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_EBPF,
+				  VIRTIO_NET_CTRL_EBPF_PROG,
+				  &sg)) {
+		dev_warn(&vdev->dev, "Failed to set bpf offload prog\n");
+		err = -EFAULT;
+		goto out;
+	}
+
+	rcu_assign_pointer(vi->offload_xdp_prog, prog);
+
+	xdp_attachment_setup(&vi->xdp_hw, bpf);
+
+out:
+	kfree(ctrl_buf);
+	return err;
 }
 
 static int virtnet_bpf_verifier_setup(struct bpf_prog *prog)
 {
-	return -ENOMEM;
+	struct virtnet_info *vi = netdev_priv(prog->aux->offload->netdev);
+	size_t insn_len = prog->len * sizeof(struct bpf_insn);
+	struct virtnet_bpf_bound_prog *state;
+
+	state = kzalloc(sizeof(*state) + insn_len, GFP_KERNEL);
+	if (!state)
+		return -ENOMEM;
+
+	memcpy(&state->insnsi[0], prog->insnsi, insn_len);
+
+	state->vi = vi;
+	state->prog = prog;
+	state->len = prog->len;
+
+	list_add_tail(&state->list, &vi->bpf_bound_progs);
+
+	prog->aux->offload->dev_priv = state;
+
+	return 0;
 }
 
 static int virtnet_bpf_verifier_prep(struct bpf_prog *prog)
@@ -2568,12 +2673,17 @@  static const struct bpf_prog_offload_ops virtnet_bpf_dev_ops = {
 static int virtnet_xdp(struct net_device *dev, struct netdev_bpf *xdp)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
+	int err;
+
 	switch (xdp->command) {
 	case XDP_SETUP_PROG:
 		return virtnet_xdp_set(dev, xdp);
 	case XDP_QUERY_PROG:
 		return xdp_attachment_query(&vi->xdp, xdp);
 	case XDP_SETUP_PROG_HW:
+		err = virtnet_xdp_offload_check(vi, xdp);
+		if (err)
+			return err;
 		return virtnet_xdp_set_offload(vi, xdp);
 	case XDP_QUERY_PROG_HW:
 		return xdp_attachment_query(&vi->xdp_hw, xdp);
diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
index a3715a3224c1..0ea2f7910a5a 100644
--- a/include/uapi/linux/virtio_net.h
+++ b/include/uapi/linux/virtio_net.h
@@ -261,4 +261,31 @@  struct virtio_net_ctrl_mq {
 #define VIRTIO_NET_CTRL_GUEST_OFFLOADS   5
 #define VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET        0
 
+/*
+ * Control XDP offloads offloads
+ *
+ * When guest wants to offload XDP program to the device, it calls
+ * VIRTIO_NET_CTRL_EBPF_PROG along with VIRTIO_NET_BPF_CMD_SET_OFFLOAD
+ * subcommands. When offloading is successful, the device runs offloaded
+ * XDP program for each packet before sending it to the guest.
+ *
+ * VIRTIO_NET_BPF_CMD_UNSET_OFFLOAD removes the the offloaded program from
+ * the device, if exists.
+ */
+
+struct virtio_net_ctrl_ebpf_prog {
+	/* program length in bytes */
+	__virtio32 len;
+	__virtio16 cmd;
+	__virtio16 gpl_compatible;
+	__u8 insns[0];
+};
+
+#define VIRTIO_NET_CTRL_EBPF 6
+ #define VIRTIO_NET_CTRL_EBPF_PROG 1
+
+/* Commands for VIRTIO_NET_CTRL_EBPF_PROG */
+#define VIRTIO_NET_BPF_CMD_SET_OFFLOAD 1
+#define VIRTIO_NET_BPF_CMD_UNSET_OFFLOAD 2
+
 #endif /* _UAPI_LINUX_VIRTIO_NET_H */