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@xxxxxxxxxxxxxxx> > >>> Sent: jeudi 17 septembre 2020 07:47 > >>> To: Arnaud POULIQUEN <arnaud.pouliquen@xxxxxx> > >>> Cc: kvm@xxxxxxxxxxxxxxx; linux-remoteproc@xxxxxxxxxxxxxxx; > >>> virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx; sound-open-firmware@alsa- > >>> project.org; Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>; Liam > >>> Girdwood <liam.r.girdwood@xxxxxxxxxxxxxxx>; Michael S. Tsirkin > >>> <mst@xxxxxxxxxx>; Jason Wang <jasowang@xxxxxxxxxx>; Ohad Ben-Cohen > >>> <ohad@xxxxxxxxxx>; Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>; Mathieu > >>> Poirier <mathieu.poirier@xxxxxxxxxx>; Vincent Whitchurch > >>> <vincent.whitchurch@xxxxxxxx> > >>> 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@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization
