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

From: Prashant Bhole <prashantbhole.linux@gmail.com>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: "David S . Miller" <davem@davemloft.net>,
	Jason Wang <jasowang@redhat.com>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Jakub Kicinski <jakub.kicinski@netronome.com>,
	Jesper Dangaard Brouer <hawk@kernel.org>,
	John Fastabend <john.fastabend@gmail.com>,
	Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
	Yonghong Song <yhs@fb.com>, Andrii Nakryiko <andriin@fb.com>,
	netdev@vger.kernel.org, qemu-devel@nongnu.org,
	kvm@vger.kernel.org
Subject: Re: [RFC net-next 15/18] virtio_net: implement XDP prog offload functionality
Date: Thu, 28 Nov 2019 11:53:41 +0900	[thread overview]
Message-ID: <73323055-3f8a-802d-87da-e8f61ef5cfb7@gmail.com> (raw)
In-Reply-To: <20191127153253-mutt-send-email-mst@kernel.org>

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.