Re: [PATCH] pci: endpoint: functions: Add a virtnet EP function

[Haotian Wang <haotian.wang@sifive.com>]

Hi Jason,

On Sun, Sep 1, 2019 at 11:50 PM Jason Wang <jasowang@redhat.com> wrote:
> >> - You refer virtio specification in the above, does it mean your device
> >> is fully compatible with virtio (or only datapath is compatible?)
> > I discussed this issue with Kishon in the previous emails a lot.
> > Theoretically this should be compatible with all virtio devices, but
> > right now the code is closely coupled with virtio_net only.
> 
> 
> We probably want a generic solution like virtio transport instead of a 
> device specific one.

There is the question of motivation. Virtual ethernet over PCI has some
very immediate use cases, especially ssh. Virtual block/cosole devices
over PCI do not make whole lot of sense to me.

In supporting virtual ethernet, I created two virtio_devices that talk
to each other using skb. However, when supporting block/console devices,
it is not obvious how many devices there will be, what the relationship
between the devices is, and why they are created in the first place.

> >> - What's the reason for introducing kthreads for some kinds of
> >> translation or copying of descriptor?
> > So there is a virtio_device A on the endpoint, there is another
> > virtio_device B on the endpoint that acts as a virtio_net device for the
> > PCI host. Then I copied data from the tx virtqueue of B to rx virtqueue
> > of A, and vice versa, directly.
> 
> 
> If my understanding is correct. You only want device B to be visible as 
> a virtio device for Linux?

Device A is on endpoint Linux. Device B is on host Linux.
Code that controls how A behaves is entrely in this epf. This epf has
another part of code that polls and manipulates data on the host side so
that B on host side indeed behaves like a virtio_device.

> Another note, it looks to me that CAIF virtio is something similar but 
> the only differences are:
> 
> 1) rx virtqueue are flipped, which means it use virtio queue for TX and 
> vringh queue for RX
> 2) accessors
> 
> As you said, if the copying is done by software, can use manage to use 
> method 1 as CAIF virtio then we can try to use vringh code by simply 
> introducing new accessor (epf based)?

I'm not sure what you mean here. Are you saying we let device A's rx queue
BE the tx queue of device B and vice versa?

Also that design uses the conventional virtio/vhost framework. In this
epf, are you implying instead of creating a Device A, create some sort
of vhost instead?

> >> - Is it possible to reuse e.g vringh (by introducing new accesor) and
> >> virtio core codes?
> > Two structures are used that are not in source files. One is struct
> > vring_virtqueue and the other is struct virtnet_info.
> 
> 
> Note that, vringh allows different type of accessor. If the only 
> difference is the way to access the vring, it should work.

The objective is not accessing vrings. struct vring_virtqueue is used for
the part of code that handles Device A.

virtio_ring.h exposes a function that creates virtqueues and I used that
function. Under the hood of that function, a bigger struct,
vring_virtqueue containing struct virtqueue, is used internally. It
would be great if I can access some fields in vring_virtqueue just by
passing in a pointer of virtqueue. It could be something as simple as

bool is_vq_broken(struct virtqueue *_vq)
{
	struct vring_virtqueue *vq = to_vvq(_vq);
	return vq->broken;
}
EXPORT_SYMBOL(is_vq_broken);

If these accessors are added to virtio_ring.h or virtio_ring.c, I do not
need to copy the whole vring_virtqueue struct into my pci-epf-virtio.h.

All I need is accessors to "broken" and "last_used_idx" of
vring_virtqueue.

> > The descriptors are not copied. The data indicated by the physical
> > addresses in those descriptors are copied using pci endpoint framework
> > API.
> >
> > The problem is that this only works for virtio_net with the split
> > virtio_ring configuration.
> 
> 
> I think do need to think of a way of using vringh, then we can try to 
> implement packed ring layout there.

Sure, though making packed rings work will happen much later. I do not
have the VCU118 board right now.

> > virtnet_info can be solved more easily. For a virtio_net device.
> > ((struct virtnet_info *)virtio_device->priv)->dev is the struct
> > net_device created together with the virtio_device. I just need a
> > pointer to that struct net_device after all.
> 
> 
> I'm still not clear why we need to expose virtnet_info. Usually, we just 
> need to set vendor id and device id and call register_virtio_device().

I must delay the start of kthreads until the virtual network interface on
endpoint is brought up by `ifconfig eth0` up. If the kthreads started
copying data from host into the endpoint rx queue while the net_device's
flags did not contain IFF_UP, a crash would occur. I can do a more
thorough investigation of the cause of this, must either way, I need to
have access to the net_device in the epf.

Thank you for the feedback!

Best,
Haotian