Re: [PATCH v3 0/5] Add a vhost RPMsg API
On Mon, Jun 08, 2020 at 01:16:38PM +0200, Guennadi Liakhovetski wrote: > On Mon, Jun 08, 2020 at 12:15:26PM +0200, Guennadi Liakhovetski wrote: > > On Mon, Jun 08, 2020 at 05:19:06AM -0400, Michael S. Tsirkin wrote: > > > On Mon, Jun 08, 2020 at 11:11:00AM +0200, Guennadi Liakhovetski wrote: > > > > Update: I looked through VirtIO 1.0 and 1.1 specs, data format their, > > > > including byte order, is defined on a per-device type basis. RPMsg is > > > > indeed included in the spec as device type 7, but that's the only > > > > mention of it in both versions. It seems RPMsg over VirtIO isn't > > > > standardised yet. > > > > > > Yes. And it would be very good to have some standartization before we > > > keep adding things. For example without any spec if host code breaks > > > with some guests, how do we know which side should be fixed? > > > > > > > Also it looks like newer interface definitions > > > > specify using "guest native endianness" for Virtual Queue data. > > > > > > They really don't or shouldn't. That's limited to legacy chapters. > > > Some definitions could have slipped through but it's not > > > the norm. I just quickly looked through the 1.1 spec and could > > > not find any instances that specify "guest native endianness" > > > but feel free to point them out to me. > > > > Oh, there you go. No, sorry, my fault, it's the other way round: "guest > > native" is for legacy and LE is for current / v1.0 and up. > > > > > > So > > > > I think the same should be done for RPMsg instead of enforcing LE? > > > > > > That makes hardware implementations as well as any cross-endian > > > hypervisors tricky. > > > > Yes, LE it is then. And we need to add some text to the spec. > > I found the protocol and the message format definition: > https://github.com/OpenAMP/open-amp/wiki/RPMsg-Messaging-Protocol#transport-layer---rpmsg > > Don't know what the best way for referencing it in the VirtIO standard > would be: just a link to the source or a quote. > > Thanks > Guennadi I wasn't aware of that one, thanks! OK so that's good. Ideally we'd have RPMsg Header Definition, RPMsg Channel and RPMsg Endppint in the spec proper. This link is informal so can't be copied into spec as is but can be used as a basis. We'd also need approval from authors for inclusion in the spec, sent to the TC mailing list. -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 0/5] Add a vhost RPMsg API
On Mon, Jun 08, 2020 at 12:15:27PM +0200, Guennadi Liakhovetski wrote: > On Mon, Jun 08, 2020 at 05:19:06AM -0400, Michael S. Tsirkin wrote: > > On Mon, Jun 08, 2020 at 11:11:00AM +0200, Guennadi Liakhovetski wrote: > > > Update: I looked through VirtIO 1.0 and 1.1 specs, data format their, > > > including byte order, is defined on a per-device type basis. RPMsg is > > > indeed included in the spec as device type 7, but that's the only > > > mention of it in both versions. It seems RPMsg over VirtIO isn't > > > standardised yet. > > > > Yes. And it would be very good to have some standartization before we > > keep adding things. For example without any spec if host code breaks > > with some guests, how do we know which side should be fixed? > > > > > Also it looks like newer interface definitions > > > specify using "guest native endianness" for Virtual Queue data. > > > > They really don't or shouldn't. That's limited to legacy chapters. > > Some definitions could have slipped through but it's not > > the norm. I just quickly looked through the 1.1 spec and could > > not find any instances that specify "guest native endianness" > > but feel free to point them out to me. > > Oh, there you go. No, sorry, my fault, it's the other way round: "guest > native" is for legacy and LE is for current / v1.0 and up. > > > > So > > > I think the same should be done for RPMsg instead of enforcing LE? > > > > That makes hardware implementations as well as any cross-endian > > hypervisors tricky. > > Yes, LE it is then. And we need to add some text to the spec. > > In theory there could be a backward compatibility issue - in case someone > was already using virtio_rpmsg_bus.c in BE mode, but I very much doubt > that... > > Thanks > Guennadi It's probably easiest to use virtio wrappers and then we don't need to worry about it. -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 0/5] Add a vhost RPMsg API
On Mon, Jun 08, 2020 at 12:15:26PM +0200, Guennadi Liakhovetski wrote: > On Mon, Jun 08, 2020 at 05:19:06AM -0400, Michael S. Tsirkin wrote: > > On Mon, Jun 08, 2020 at 11:11:00AM +0200, Guennadi Liakhovetski wrote: > > > Update: I looked through VirtIO 1.0 and 1.1 specs, data format their, > > > including byte order, is defined on a per-device type basis. RPMsg is > > > indeed included in the spec as device type 7, but that's the only > > > mention of it in both versions. It seems RPMsg over VirtIO isn't > > > standardised yet. > > > > Yes. And it would be very good to have some standartization before we > > keep adding things. For example without any spec if host code breaks > > with some guests, how do we know which side should be fixed? > > > > > Also it looks like newer interface definitions > > > specify using "guest native endianness" for Virtual Queue data. > > > > They really don't or shouldn't. That's limited to legacy chapters. > > Some definitions could have slipped through but it's not > > the norm. I just quickly looked through the 1.1 spec and could > > not find any instances that specify "guest native endianness" > > but feel free to point them out to me. > > Oh, there you go. No, sorry, my fault, it's the other way round: "guest > native" is for legacy and LE is for current / v1.0 and up. > > > > So > > > I think the same should be done for RPMsg instead of enforcing LE? > > > > That makes hardware implementations as well as any cross-endian > > hypervisors tricky. > > Yes, LE it is then. And we need to add some text to the spec. I found the protocol and the message format definition: https://github.com/OpenAMP/open-amp/wiki/RPMsg-Messaging-Protocol#transport-layer---rpmsg Don't know what the best way for referencing it in the VirtIO standard would be: just a link to the source or a quote. Thanks Guennadi ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 0/5] Add a vhost RPMsg API
On Mon, Jun 08, 2020 at 05:19:06AM -0400, Michael S. Tsirkin wrote: > On Mon, Jun 08, 2020 at 11:11:00AM +0200, Guennadi Liakhovetski wrote: > > Update: I looked through VirtIO 1.0 and 1.1 specs, data format their, > > including byte order, is defined on a per-device type basis. RPMsg is > > indeed included in the spec as device type 7, but that's the only > > mention of it in both versions. It seems RPMsg over VirtIO isn't > > standardised yet. > > Yes. And it would be very good to have some standartization before we > keep adding things. For example without any spec if host code breaks > with some guests, how do we know which side should be fixed? > > > Also it looks like newer interface definitions > > specify using "guest native endianness" for Virtual Queue data. > > They really don't or shouldn't. That's limited to legacy chapters. > Some definitions could have slipped through but it's not > the norm. I just quickly looked through the 1.1 spec and could > not find any instances that specify "guest native endianness" > but feel free to point them out to me. Oh, there you go. No, sorry, my fault, it's the other way round: "guest native" is for legacy and LE is for current / v1.0 and up. > > So > > I think the same should be done for RPMsg instead of enforcing LE? > > That makes hardware implementations as well as any cross-endian > hypervisors tricky. Yes, LE it is then. And we need to add some text to the spec. In theory there could be a backward compatibility issue - in case someone was already using virtio_rpmsg_bus.c in BE mode, but I very much doubt that... Thanks Guennadi ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 0/5] Add a vhost RPMsg API
On Mon, Jun 08, 2020 at 11:11:00AM +0200, Guennadi Liakhovetski wrote: > Update: I looked through VirtIO 1.0 and 1.1 specs, data format their, > including byte order, is defined on a per-device type basis. RPMsg is > indeed included in the spec as device type 7, but that's the only > mention of it in both versions. It seems RPMsg over VirtIO isn't > standardised yet. Yes. And it would be very good to have some standartization before we keep adding things. For example without any spec if host code breaks with some guests, how do we know which side should be fixed? > Also it looks like newer interface definitions > specify using "guest native endianness" for Virtual Queue data. They really don't or shouldn't. That's limited to legacy chapters. Some definitions could have slipped through but it's not the norm. I just quickly looked through the 1.1 spec and could not find any instances that specify "guest native endianness" but feel free to point them out to me. > So > I think the same should be done for RPMsg instead of enforcing LE? > > Thanks > Guennadi That makes hardware implementations as well as any cross-endian hypervisors tricky. -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 0/5] Add a vhost RPMsg API
Update: I looked through VirtIO 1.0 and 1.1 specs, data format their, including byte order, is defined on a per-device type basis. RPMsg is indeed included in the spec as device type 7, but that's the only mention of it in both versions. It seems RPMsg over VirtIO isn't standardised yet. Also it looks like newer interface definitions specify using "guest native endianness" for Virtual Queue data. So I think the same should be done for RPMsg instead of enforcing LE? Thanks Guennadi On Mon, Jun 08, 2020 at 09:37:15AM +0200, Guennadi Liakhovetski wrote: > Hi Michael, > > On Fri, Jun 05, 2020 at 08:34:35AM +0200, Guennadi Liakhovetski wrote: > > > > On Thu, Jun 04, 2020 at 03:23:37PM -0400, Michael S. Tsirkin wrote: > > [snip] > > > > Another it's out of line with 1.0 spec passing guest > > > endian data around. Won't work if host and guest > > > endian-ness do not match. Should pass eveything in LE and > > > convert. > > > > Yes, I have to fix this, thanks. > > Just to make sure my understanding is correct: this would involve also > modifying the current virtio_rpmsg_bus.c implementation to add > endianness conversions. That's what you meant, right? ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 0/5] Add a vhost RPMsg API
On Mon, Jun 08, 2020 at 09:37:15AM +0200, Guennadi Liakhovetski wrote: > Hi Michael, > > On Fri, Jun 05, 2020 at 08:34:35AM +0200, Guennadi Liakhovetski wrote: > > > > On Thu, Jun 04, 2020 at 03:23:37PM -0400, Michael S. Tsirkin wrote: > > [snip] > > > > Another it's out of line with 1.0 spec passing guest > > > endian data around. Won't work if host and guest > > > endian-ness do not match. Should pass eveything in LE and > > > convert. > > > > Yes, I have to fix this, thanks. > > Just to make sure my understanding is correct: this would involve also > modifying the current virtio_rpmsg_bus.c implementation to add > endianness conversions. That's what you meant, right? > > Thanks > Guennadi right and if there are legacy compat considerations, using _virtio16 and friends types, as well as virtio16_to_cpu and friends functions. -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 0/5] Add a vhost RPMsg API
Hi Michael, On Fri, Jun 05, 2020 at 08:34:35AM +0200, Guennadi Liakhovetski wrote: > > On Thu, Jun 04, 2020 at 03:23:37PM -0400, Michael S. Tsirkin wrote: [snip] > > Another it's out of line with 1.0 spec passing guest > > endian data around. Won't work if host and guest > > endian-ness do not match. Should pass eveything in LE and > > convert. > > Yes, I have to fix this, thanks. Just to make sure my understanding is correct: this would involve also modifying the current virtio_rpmsg_bus.c implementation to add endianness conversions. That's what you meant, right? Thanks Guennadi ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 0/5] Add a vhost RPMsg API
Hi Michael, Thanks for your review. On Thu, Jun 04, 2020 at 03:23:37PM -0400, Michael S. Tsirkin wrote: > On Wed, May 27, 2020 at 08:05:36PM +0200, Guennadi Liakhovetski wrote: > > 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 > > A further patch for the ADSP vhost RPMsg driver will be sent > > separately for review only since it cannot be merged without audio > > patches being upstreamed first. > > > RPMsg over virtio has several problems. One is that it's > not specced at all. Before we add more stuff, I'd like so > see at least an attempt at describing what it's supposed to do. Sure, I can work on this with the original authors of the virtio-rpmsg implementation. > Another it's out of line with 1.0 spec passing guest > endian data around. Won't work if host and guest > endian-ness do not match. Should pass eveything in LE and > convert. Yes, I have to fix this, thanks. > It's great to see it's seeing active development finally. > Do you think you will have time to address these? Sure, I'll try to take care of them. Thanks Guennadi > > Guennadi Liakhovetski (5): > > vhost: convert VHOST_VSOCK_SET_RUNNING to a generic ioctl > > vhost: (cosmetic) remove a superfluous variable initialisation > > 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| 382 > > +++ > > drivers/vhost/vhost.c| 2 +- > > drivers/vhost/vhost_rpmsg.h | 74 > > include/linux/virtio_rpmsg.h | 81 + > > include/uapi/linux/rpmsg.h | 3 + > > include/uapi/linux/vhost.h | 4 +- > > 10 files changed, 559 insertions(+), 81 deletions(-) > > create mode 100644 drivers/vhost/rpmsg.c > > create mode 100644 drivers/vhost/vhost_rpmsg.h > > create mode 100644 include/linux/virtio_rpmsg.h > > > > -- > > 1.9.3 > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 0/5] Add a vhost RPMsg API
On Wed, May 27, 2020 at 08:05:36PM +0200, Guennadi Liakhovetski wrote: > 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 > A further patch for the ADSP vhost RPMsg driver will be sent > separately for review only since it cannot be merged without audio > patches being upstreamed first. RPMsg over virtio has several problems. One is that it's not specced at all. Before we add more stuff, I'd like so see at least an attempt at describing what it's supposed to do. Another it's out of line with 1.0 spec passing guest endian data around. Won't work if host and guest endian-ness do not match. Should pass eveything in LE and convert. It's great to see it's seeing active development finally. Do you think you will have time to address these? > Thanks > Guennadi > > Guennadi Liakhovetski (5): > vhost: convert VHOST_VSOCK_SET_RUNNING to a generic ioctl > vhost: (cosmetic) remove a superfluous variable initialisation > 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| 382 > +++ > drivers/vhost/vhost.c| 2 +- > drivers/vhost/vhost_rpmsg.h | 74 > include/linux/virtio_rpmsg.h | 81 + > include/uapi/linux/rpmsg.h | 3 + > include/uapi/linux/vhost.h | 4 +- > 10 files changed, 559 insertions(+), 81 deletions(-) > create mode 100644 drivers/vhost/rpmsg.c > create mode 100644 drivers/vhost/vhost_rpmsg.h > create mode 100644 include/linux/virtio_rpmsg.h > > -- > 1.9.3 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 0/5] Add a vhost RPMsg API
On 2020/5/29 下午2:50, Guennadi Liakhovetski wrote: Hi Jason, On Fri, May 29, 2020 at 02:01:53PM +0800, Jason Wang wrote: On 2020/5/28 上午2:05, Guennadi Liakhovetski wrote: 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 A further patch for the ADSP vhost RPMsg driver will be sent separately for review only since it cannot be merged without audio patches being upstreamed first. Hi: It would be hard to evaluate this series without a real user. So if possible, I suggest to post the actual user for vhost rpmsg API. Sure, the whole series is available at https://github.com/thesofproject/linux/pull/1501/commits or would you prefer the missing patches posted to the lists too? Yes, since I see new virtio and vhost drivers there. Thanks Thanks Guennadi ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 0/5] Add a vhost RPMsg API
Hi Jason, On Fri, May 29, 2020 at 02:01:53PM +0800, Jason Wang wrote: > > On 2020/5/28 上午2:05, Guennadi Liakhovetski wrote: > > 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 > > A further patch for the ADSP vhost RPMsg driver will be sent > > separately for review only since it cannot be merged without audio > > patches being upstreamed first. > > > Hi: > > It would be hard to evaluate this series without a real user. So if > possible, I suggest to post the actual user for vhost rpmsg API. Sure, the whole series is available at https://github.com/thesofproject/linux/pull/1501/commits or would you prefer the missing patches posted to the lists too? Thanks Guennadi ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 0/5] Add a vhost RPMsg API
On 2020/5/28 上午2:05, Guennadi Liakhovetski wrote: 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 A further patch for the ADSP vhost RPMsg driver will be sent separately for review only since it cannot be merged without audio patches being upstreamed first. Hi: It would be hard to evaluate this series without a real user. So if possible, I suggest to post the actual user for vhost rpmsg API. Thanks Thanks Guennadi ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization