Re: [PATCH virtio] virtio: virtio_console: fix DMA memory allocation for rproc serial
On 11/16/20 5:39 PM, Christoph Hellwig wrote: > Btw, I also still don't understand why remoteproc is using > dma_declare_coherent_memory to start with. The virtio code has exactly > one call to dma_alloc_coherent vring_alloc_queue, a function that > already switches between two different allocators. Why can't we just > add a third allocator specifically for these remoteproc memory carveouts > and bypass dma_declare_coherent_memory entirely? > The dma_declare_coherent_memory allows to associate vdev0buffer memory region to the remoteproc virtio device (vdev parent). This region is used to allocated the rpmsg buffers. The memory for the rpmsg buffer is allocated by the rpmsg_virtio device in rpmsg_virtio_bus[1]. The size depends on the total size needed for the rpmsg buffers. The vrings are allocated directly by the remoteproc device. [1] https://elixir.bootlin.com/linux/v5.10-rc3/source/drivers/rpmsg/virtio_rpmsg_bus.c#L925 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH virtio] virtio: virtio_console: fix DMA memory allocation for rproc serial
Hi all,
On 11/16/20 10:19 AM, Christoph Hellwig wrote:
> I just noticed this showing up in Linus' tree and I'm not happy.
>
> This whole model of the DMA subdevices in remoteproc is simply broken.
>
> We really need to change the virtio code pass an expicit DMA device (
> similar to what e.g. the USB and RDMA code does), instead of faking up
> devices with broken adhoc inheritance of DMA properties and magic poking
> into device parent relationships.
For your formation I started some stuff on my side to be able to declare the
virtio device in DT as a remoteproc child node.
https://lkml.org/lkml/2020/4/16/1817
Quite big refactoring, but could be a way to answer...
Regards,
Arnaud
>
> Bjorn, I thought you were going to look into this a while ago?
>
>
> On Wed, Nov 04, 2020 at 03:31:36PM +, Alexander Lobakin wrote:
>> Since commit 086d08725d34 ("remoteproc: create vdev subdevice with
>> specific dma memory pool"), every remoteproc has a DMA subdevice
>> ("remoteprocX#vdevYbuffer") for each virtio device, which inherits
>> DMA capabilities from the corresponding platform device. This allowed
>> to associate different DMA pools with each vdev, and required from
>> virtio drivers to perform DMA operations with the parent device
>> (vdev->dev.parent) instead of grandparent (vdev->dev.parent->parent).
>>
>> virtio_rpmsg_bus was already changed in the same merge cycle with
>> commit d999b622fcfb ("rpmsg: virtio: allocate buffer from parent"),
>> but virtio_console did not. In fact, operations using the grandparent
>> worked fine while the grandparent was the platform device, but since
>> commit c774ad010873 ("remoteproc: Fix and restore the parenting
>> hierarchy for vdev") this was changed, and now the grandparent device
>> is the remoteproc device without any DMA capabilities.
>> So, starting v5.8-rc1 the following warning is observed:
>>
>> [2.483925] [ cut here ]
>> [2.489148] WARNING: CPU: 3 PID: 101 at kernel/dma/mapping.c:427
>> 0x80e7eee8
>> [2.489152] Modules linked in: virtio_console(+)
>> [2.503737] virtio_rpmsg_bus rpmsg_core
>> [2.508903]
>> [2.528898]
>> [2.913043]
>> [2.914907] ---[ end trace 93ac8746beab612c ]---
>> [2.920102] virtio-ports vport1p0: Error allocating inbufs
>>
>> kernel/dma/mapping.c:427 is:
>>
>> WARN_ON_ONCE(!dev->coherent_dma_mask);
>>
>> obviously because the grandparent now is remoteproc dev without any
>> DMA caps:
>>
>> [3.104943] Parent: remoteproc0#vdev1buffer, grandparent: remoteproc0
>>
>> Fix this the same way as it was for virtio_rpmsg_bus, using just the
>> parent device (vdev->dev.parent, "remoteprocX#vdevYbuffer") for DMA
>> operations.
>> This also allows now to reserve DMA pools/buffers for rproc serial
>> via Device Tree.
>>
>> Fixes: c774ad010873 ("remoteproc: Fix and restore the parenting hierarchy
>> for vdev")
>> Cc: sta...@vger.kernel.org # 5.1+
>> Signed-off-by: Alexander Lobakin
>> ---
>> drivers/char/virtio_console.c | 8
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
>> index a2da8f768b94..1836cc56e357 100644
>> --- a/drivers/char/virtio_console.c
>> +++ b/drivers/char/virtio_console.c
>> @@ -435,12 +435,12 @@ static struct port_buffer *alloc_buf(struct
>> virtio_device *vdev, size_t buf_size
>> /*
>> * Allocate DMA memory from ancestor. When a virtio
>> * device is created by remoteproc, the DMA memory is
>> - * associated with the grandparent device:
>> - * vdev => rproc => platform-dev.
>> + * associated with the parent device:
>> + * virtioY => remoteprocX#vdevYbuffer.
>> */
>> -if (!vdev->dev.parent || !vdev->dev.parent->parent)
>> +buf->dev = vdev->dev.parent;
>> +if (!buf->dev)
>> goto free_buf;
>> -buf->dev = vdev->dev.parent->parent;
>>
>> /* Increase device refcnt to avoid freeing it */
>> get_device(buf->dev);
>> --
>> 2.29.2
>>
>>
> ---end quoted text---
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v6 0/4] Add a vhost RPMsg API
Hi Guennadi, On 9/18/20 11:47 AM, Guennadi Liakhovetski wrote: > 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 >>>>> Sent: jeudi 17 septembre 2020 07:47 >>>>> To: Arnaud POULIQUEN >>>>> Cc: k...@vger.kernel.org; linux-remotep...@vger.kernel.org; >>>>> virtualization@lists.linux-foundation.org; sound-open-firmware@alsa- >>>>> project.org; Pierre-Louis Bossart ; >>>>> Liam >>>>> Girdwood ; Michael S. Tsirkin >>>>> ; Jason Wang ; Ohad Ben-Cohen >>>>> ; Bjorn Andersson ; Mathieu >>>>> Poirier ; Vincent Whitchurch >>>>> >>>>> 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 >>&
Re: [PATCH v6 0/4] Add a vhost RPMsg API
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 >>> Sent: jeudi 17 septembre 2020 07:47 >>> To: Arnaud POULIQUEN >>> Cc: k...@vger.kernel.org; linux-remotep...@vger.kernel.org; >>> virtualization@lists.linux-foundation.org; sound-open-firmware@alsa- >>> project.org; Pierre-Louis Bossart ; >>> Liam >>> Girdwood ; Michael S. Tsirkin >>> ; Jason Wang ; Ohad Ben-Cohen >>> ; Bjorn Andersson ; Mathieu >>> Poirier ; Vincent Whitchurch >>> >>> 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 missin
RE: [PATCH v6 0/4] Add a vhost RPMsg API
Hi Guennadi, > -Original Message- > From: Guennadi Liakhovetski > Sent: jeudi 17 septembre 2020 07:47 > To: Arnaud POULIQUEN > Cc: k...@vger.kernel.org; linux-remotep...@vger.kernel.org; > virtualization@lists.linux-foundation.org; sound-open-firmware@alsa- > project.org; Pierre-Louis Bossart ; Liam > Girdwood ; Michael S. Tsirkin > ; Jason Wang ; Ohad Ben-Cohen > ; Bjorn Andersson ; Mathieu > Poirier ; Vincent Whitchurch > > 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... [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]
Re: [PATCH v8 15/18] ARM: STi: DT: STiH407: Add uniperif reader dt nodes
Hello ptere, Lee,
Thanks for your remarks,
Regards
Arnaud
On 08/31/2016 01:28 PM, Lee Jones wrote:
> On Tue, 30 Aug 2016, Peter Griffin wrote:
>> Thanks for reviewing and your very valuable feedback.
>> On Tue, 30 Aug 2016, Lee Jones wrote:
>>> On Fri, 26 Aug 2016, Peter Griffin wrote:
>>>
>>>> This patch adds the DT node for the uniperif reader
>>>> IP block found on STiH407 family silicon.
>>>>
>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliq...@st.com>
>>>> Signed-off-by: Peter Griffin <peter.grif...@linaro.org>
>>>> ---
>>>> arch/arm/boot/dts/stih407-family.dtsi | 26 ++
>>>> 1 file changed, 26 insertions(+)
>>>>
>>>> diff --git a/arch/arm/boot/dts/stih407-family.dtsi
>>>> b/arch/arm/boot/dts/stih407-family.dtsi
>>>> index d263c96..bdddf2c 100644
>>>> --- a/arch/arm/boot/dts/stih407-family.dtsi
>>>> +++ b/arch/arm/boot/dts/stih407-family.dtsi
>>>> @@ -956,5 +956,31 @@
>>>>st,version = <5>;
>>>>st,mode = "SPDIF";
>>>>};
>>>> +
>>>> + sti_uni_reader0: sti-uni-reader@0 {
>>>> + compatible = "st,sti-uni-reader";
>>>> + status = "disabled";
>>>
>>> I find it's normally nicer to place the status of the node at the
>>> bottom, separated by a '\n'.
>>
>> Ok I'll add a superflous '\n' in the next version.
>
> Everyone loves a smart arse!
>
> In this case I believe the '\n' to be a functional separator and not
> superfluous at all.
>
>>>> + dai-name = "Uni Reader #0 (PCM IN)";
>>>
>>> Oooo, not seen something like this before.
>>>
>>> If it does not already have one, it would require a DT Ack.
>>
>> No idea, the driver got merged 1 year ago.
This field could be suppressed and handled in source code, using
st,uniperiph-id to retreive it.
>>
>> Arnaud did you get a DT ack when you merged this driver & binding? i if i
>> remember well, i had sent to Alsa mailing list only, I missed
this obvious...
>>>
>>>> + st,version = <3>;
>>>
>>> This will likely need a DT Ack too. We usually encode this sort of
>>> information in the compatible string.
yes, better to use compatibility
>>
>> See 05c1b4480e86a871b18030d6f3d532dc0ecdf38c
>
> Well Rob's the boss. We certainly never used to take 'device ID' or
> 'version' attributes. I guess something must have changed.
I will try to provide patches for code and bindings rework this week.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
