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

[Prashant Bhole]

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 <address@hidden>
> >
> > 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 <address@hidden>
> > Co-developed-by: Prashant Bhole <address@hidden>
> > Signed-off-by: Prashant Bhole <address@hidden>
>
> Any UAPI changes need to be copied to address@hidden
> (subscriber only) list please.

Sure.

Thanks.