Re: [PATCH v6 0/4] Add a vhost RPMsg API

[Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>]

Hi Arnaud,

On Fri, Sep 18, 2020 at 09:47:45AM +0200, Arnaud POULIQUEN wrote:
> Hi Guennadi,
> 
> On 9/18/20 7:44 AM, Guennadi Liakhovetski wrote:
> > Hi Arnaud,
> > 
> > On Thu, Sep 17, 2020 at 05:21:02PM +0200, Arnaud POULIQUEN wrote:
> >> Hi Guennadi,
> >>
> >>> -----Original Message-----
> >>> From: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> >>> Sent: jeudi 17 septembre 2020 07:47
> >>> To: Arnaud POULIQUEN <arnaud.pouliquen@st.com>
> >>> Cc: kvm@vger.kernel.org; linux-remoteproc@vger.kernel.org;
> >>> virtualization@lists.linux-foundation.org; sound-open-firmware@alsa-
> >>> project.org; Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>; Liam
> >>> Girdwood <liam.r.girdwood@linux.intel.com>; Michael S. Tsirkin
> >>> <mst@redhat.com>; Jason Wang <jasowang@redhat.com>; Ohad Ben-Cohen
> >>> <ohad@wizery.com>; Bjorn Andersson <bjorn.andersson@linaro.org>; Mathieu
> >>> Poirier <mathieu.poirier@linaro.org>; Vincent Whitchurch
> >>> <vincent.whitchurch@axis.com>
> >>> Subject: Re: [PATCH v6 0/4] Add a vhost RPMsg API
> >>>
> >>> Hi Arnaud,
> >>>
> >>> On Tue, Sep 15, 2020 at 02:13:23PM +0200, Arnaud POULIQUEN wrote:
> >>>> Hi  Guennadi,
> >>>>
> >>>> On 9/1/20 5:11 PM, Guennadi Liakhovetski wrote:
> >>>>> Hi,
> >>>>>
> >>>>> Next update:
> >>>>>
> >>>>> v6:
> >>>>> - rename include/linux/virtio_rpmsg.h ->
> >>>>> include/linux/rpmsg/virtio.h
> >>>>>
> >>>>> v5:
> >>>>> - don't hard-code message layout
> >>>>>
> >>>>> v4:
> >>>>> - add endianness conversions to comply with the VirtIO standard
> >>>>>
> >>>>> v3:
> >>>>> - address several checkpatch warnings
> >>>>> - address comments from Mathieu Poirier
> >>>>>
> >>>>> v2:
> >>>>> - update patch #5 with a correct vhost_dev_init() prototype
> >>>>> - drop patch #6 - it depends on a different patch, that is currently
> >>>>>   an RFC
> >>>>> - address comments from Pierre-Louis Bossart:
> >>>>>   * remove "default n" from Kconfig
> >>>>>
> >>>>> Linux supports RPMsg over VirtIO for "remote processor" / AMP use
> >>>>> cases. It can however also be used for virtualisation scenarios,
> >>>>> e.g. when using KVM to run Linux on both the host and the guests.
> >>>>> This patch set adds a wrapper API to facilitate writing vhost
> >>>>> drivers for such RPMsg-based solutions. The first use case is an
> >>>>> audio DSP virtualisation project, currently under development, ready
> >>>>> for review and submission, available at
> >>>>> https://github.com/thesofproject/linux/pull/1501/commits
> >>>>
> >>>> Mathieu pointed me your series. On my side i proposed the rpmsg_ns_msg
> >>>> service[1] that does not match with your implementation.
> >>>> As i come late, i hope that i did not miss something in the history...
> >>>> Don't hesitate to point me the discussions, if it is the case.
> >>>
> >>> Well, as you see, this is a v6 only of this patch set, and apart from it there have
> >>> been several side discussions and patch sets.
> >>>
> >>>> Regarding your patchset, it is quite confusing for me. It seems that
> >>>> you implement your own protocol on top of vhost forked from the RPMsg
> >>> one.
> >>>> But look to me that it is not the RPMsg protocol.
> >>>
> >>> I'm implementing a counterpart to the rpmsg protocol over VirtIO as initially
> >>> implemented by drivers/rpmsg/virtio_rpmsg_bus.c for the "main CPU" (in case
> >>> of remoteproc over VirtIO) or the guest side in case of Linux virtualisation.
> >>> Since my implementation can talk to that driver, I don't think, that I'm inventing
> >>> a new protocol. I'm adding support for the same protocol for the opposite side
> >>> of the VirtIO divide.
> >>
> >> The main point I would like to highlight here is related to the use of the name "RPMsg"
> >> more than how you implement your IPC protocol.
> >> If It is a counterpart, it probably does not respect interface for RPMsg clients.
> >> A good way to answer this, might be to respond to this question:
> >> Is the rpmsg sample client[4] can be used on top of your vhost RPMsg implementation?
> >> If the response is no, describe it as a RPMsg implementation could lead to confusion...
> > 
> > Sorry, I don't quite understand your logic. RPMsg is a communication protocol, not an 
> > API. An RPMsg implementation has to be able to communicate with other compliant RPMsg 
> > implementations, it doesn't have to provide any specific API. Am I missing anything?
> 
> You are right nothing is written in stone that compliance with the user RPMsg API defined
> in the Linux Documentation [5] is mandatory.

A quote from [5]:

<quote>
Rpmsg is a virtio-based messaging bus that allows kernel drivers to communicate
with remote processors available on the system.
</quote>

So, that document describes the API used by Linux drivers to talk to remote processors. 
It says nothing about VMs. What my patches do, they add a capability to the Linux RPMsg 
implementation to also be used with VMs. Moreover, this is a particularly good fit, 
because both cases can use VirtIO, so, the "VirtIO side" of the communication doesn't 
have to change, and indeed it remains unchanged and uses the API in [5]. But what I do, 
is I also add RPMsg support to the host side.

> IMO, as this API is defined in the Linux documentation [5] we should respect it, to ensure
> one generic implementation. The RPMsg sample client[4] uses this user API, so seems to me
> a good candidate to verify this. 
> 
> That's said, shall we multiple the RPMsg implementations in Linux with several APIs,
> With the risk to make the RPMsg clients devices dependent on these implementations?
> That could lead to complex code or duplications...

So, no, in my understanding there aren't two competing alternative APIs, you'd never have 
to choose between them. If you're writing a driver for Linux to communicate with remote 
processors or to run on VMs, you use the existing API. If you're writing a driver for 
Linux to communicate with those VMs, you use the vhost API and whatever help is available 
for RPMsg processing.

However, I can in principle imagine a single driver, written to work on both sides. 
Something like the rpmsg_char.c or maybe some networking driver. Is that what you're 
referring to? I can see that as a fun exercise, but are there any real uses for that? 
You could do the same with VirtIO, however, it has been decided to go with two 
distinct APIs: virtio for guests and vhost for the host, noone bothered to create a 
single API for both and nobody seems to miss one. Why would we want one with RPMsg?

Thanks
Guennadi

> I'm not the right person to answer, Bjorn and Mathieu are.
> 
> [5] https://elixir.bootlin.com/linux/v5.8.10/source/Documentation/rpmsg.txt#L66
> 
> Thanks,
> Arnaud
> 
>   
> > 
> > Thanks
> > Guennadi
> > 
> >> [4] https://elixir.bootlin.com/linux/v5.9-rc5/source/samples/rpmsg/rpmsg_client_sample.c
> >>
> >> Regards,
> >> Arnaud
> >>
> >>>
> >>>> So i would be agree with Vincent[2] which proposed to switch on a
> >>>> RPMsg API and creating a vhost rpmsg device. This is also proposed in
> >>>> the "Enhance VHOST to enable SoC-to-SoC communication" RFC[3].
> >>>> Do you think that this alternative could match with your need?
> >>>
> >>> As I replied to Vincent, I understand his proposal and the approach taken in the
> >>> series [3], but I'm not sure I agree, that adding yet another virtual device /
> >>> driver layer on the vhost side is a good idea. As far as I understand adding new
> >>> completely virtual devices isn't considered to be a good practice in the kernel.
> >>> Currently vhost is just a passive "library"
> >>> and my vhost-rpmsg support keeps it that way. Not sure I'm in favour of
> >>> converting vhost to a virtual device infrastructure.
> >>>
> >>> Thanks for pointing me out at [3], I should have a better look at it.
> >>>
> >>> Thanks
> >>> Guennadi
> >>>
> >>>> [1].
> >>>> https://patchwork.kernel.org/project/linux-remoteproc/list/?series=338
> >>>> 335 [2].
> >>>> https://www.spinics.net/lists/linux-virtualization/msg44195.html
> >>>> [3]. https://www.spinics.net/lists/linux-remoteproc/msg06634.html
> >>>>
> >>>> Thanks,
> >>>> Arnaud
> >>>>
> >>>>>
> >>>>> Thanks
> >>>>> Guennadi
> >>>>>
> >>>>> Guennadi Liakhovetski (4):
> >>>>>   vhost: convert VHOST_VSOCK_SET_RUNNING to a generic ioctl
> >>>>>   rpmsg: move common structures and defines to headers
> >>>>>   rpmsg: update documentation
> >>>>>   vhost: add an RPMsg API
> >>>>>
> >>>>>  Documentation/rpmsg.txt          |   6 +-
> >>>>>  drivers/rpmsg/virtio_rpmsg_bus.c |  78 +------
> >>>>>  drivers/vhost/Kconfig            |   7 +
> >>>>>  drivers/vhost/Makefile           |   3 +
> >>>>>  drivers/vhost/rpmsg.c            | 373 +++++++++++++++++++++++++++++++
> >>>>>  drivers/vhost/vhost_rpmsg.h      |  74 ++++++
> >>>>>  include/linux/rpmsg/virtio.h     |  83 +++++++
> >>>>>  include/uapi/linux/rpmsg.h       |   3 +
> >>>>>  include/uapi/linux/vhost.h       |   4 +-
> >>>>>  9 files changed, 551 insertions(+), 80 deletions(-)  create mode
> >>>>> 100644 drivers/vhost/rpmsg.c  create mode 100644
> >>>>> drivers/vhost/vhost_rpmsg.h  create mode 100644
> >>>>> include/linux/rpmsg/virtio.h
> >>>>>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization