Re: [PATCH virtio] virtio: virtio_console: fix DMA memory allocation for rproc serial

Re: [PATCH virtio] virtio: virtio_console: fix DMA memory allocation for rproc serial

[Jason Wang]

On 2020/11/4 下午11:31, 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] <Other modules, stack and call trace here>
> [    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: stable@vger.kernel.org # 5.1+
> Signed-off-by: Alexander Lobakin <alobakin@pm.me>
> ---
>   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;


I wonder it could be the right time to introduce dma_dev for virtio 
instead of depending on something magic via parent.

(Btw I don't even notice that there's transport specific code in virtio 
console, it's better to avoid it)

Thanks


>   
>   		/* Increase device refcnt to avoid freeing it */
>   		get_device(buf->dev);

_______________________________________________
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

[Jason Wang]

On 2020/11/5 下午8:22, Alexander Lobakin wrote:
> From: Jason Wang <jasowang@redhat.com>
> Date: Thu, 5 Nov 2020 11:10:24 +0800
>
> Hi Jason,
>
>> On 2020/11/4 下午11:31, 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] <Other modules, stack and call trace here>
>>> [    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: stable@vger.kernel.org # 5.1+
>>> Signed-off-by: Alexander Lobakin <alobakin@pm.me>
>>> ---
>>>    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;
>>
>> I wonder it could be the right time to introduce dma_dev for virtio
>> instead of depending on something magic via parent.
> This patch are meant to hit RC window and stable trees as a fix of
> the bug that is present since v5.8-rc1. So any new features are out
> of scope of this particular fix.


Right.


>
> The idea of DMAing through "dev->parent" is that "virtioX" itself is a
> logical dev, not the real one, but its parent *is*. This logic is used
> across the whole tree -- every subsystem creates its own logical device,
> but drivers should always use the backing PCI/platform/etc. devices for
> DMA operations, which represent the real hardware.


Yes, so what I meant is to use different variables for DMA and 
hierarchy. So it's the responsibility of the lower layer to pass a 
correct "dma_dev" to the upper layer instead of depending parent.

Anyway for this patch.

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks


>
>> (Btw I don't even notice that there's transport specific code in virtio
>> console, it's better to avoid it)
>>
>> Thanks
> Thanks,
> Al
>
>>>    		/* Increase device refcnt to avoid freeing it */
>>>    		get_device(buf->dev);

_______________________________________________
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

[Christoph Hellwig]

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.

Bjorn, I thought you were going to look into this a while ago?


On Wed, Nov 04, 2020 at 03:31:36PM +0000, 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] <Other modules, stack and call trace here>
> [    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: stable@vger.kernel.org # 5.1+
> Signed-off-by: Alexander Lobakin <alobakin@pm.me>
> ---
>  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 virtio] virtio: virtio_console: fix DMA memory allocation for rproc serial

[Michael S. Tsirkin]

On Mon, Nov 16, 2020 at 09:19:50AM +0000, 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),

Could you point me at an example or two please?

Thanks!

> instead of faking up
> devices with broken adhoc inheritance of DMA properties and magic poking
> into device parent relationships.
> 
> Bjorn, I thought you were going to look into this a while ago?
> 
> 
> On Wed, Nov 04, 2020 at 03:31:36PM +0000, 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] <Other modules, stack and call trace here>
> > [    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: stable@vger.kernel.org # 5.1+
> > Signed-off-by: Alexander Lobakin <alobakin@pm.me>
> > ---
> >  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 virtio] virtio: virtio_console: fix DMA memory allocation for rproc serial

[Christoph Hellwig]

On Mon, Nov 16, 2020 at 04:51:49AM -0500, Michael S. Tsirkin wrote:
> On Mon, Nov 16, 2020 at 09:19:50AM +0000, 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),
> 
> Could you point me at an example or two please?

Take a look at the ib_dma_* helper in include/rdma/ib_verbs.h and
dma_device member in struct ib_device for the best example.
_______________________________________________
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

[Arnaud POULIQUEN]

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 +0000, 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] <Other modules, stack and call trace here>
>> [    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: stable@vger.kernel.org # 5.1+
>> Signed-off-by: Alexander Lobakin <alobakin@pm.me>
>> ---
>>  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 virtio] virtio: virtio_console: fix DMA memory allocation for rproc serial

[Christoph Hellwig]

On Mon, Nov 16, 2020 at 11:46:59AM +0100, Arnaud POULIQUEN wrote:
> 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...

Yes, that series is exactly what we need to do conceptually (can't
comment on all the nitty grity details as I'm not too familiar with
the remoteproc code).
_______________________________________________
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

[Christoph Hellwig]

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?
_______________________________________________
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

[Arnaud POULIQUEN]

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

[Christoph Hellwig]

On Tue, Nov 17, 2020 at 03:00:32PM +0100, Arnaud POULIQUEN wrote:
> 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.

Weird.  I thought virtio was pretty strict in not allowing diret DMA
API usage in drivers to support the legacy no-mapping case.

Either way, the point stands:  if you want these magic buffers handed
out to specific rpmsg instances I think not having to detour through the
DMA API is going to make everyones life easier.
_______________________________________________
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

[Michael S. Tsirkin]

On Tue, Nov 17, 2020 at 02:02:30PM +0000, Christoph Hellwig wrote:
> On Tue, Nov 17, 2020 at 03:00:32PM +0100, Arnaud POULIQUEN wrote:
> > 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.
> 
> Weird.  I thought virtio was pretty strict in not allowing diret DMA
> API usage in drivers to support the legacy no-mapping case.

Well remoteproc is weird in that it's use of DMA API precedes
standartization efforts, and it was never standardized in the virtio
spec ..

> Either way, the point stands:  if you want these magic buffers handed
> out to specific rpmsg instances I think not having to detour through the
> DMA API is going to make everyones life easier.

_______________________________________________
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

[Michael S. Tsirkin]

On Mon, Nov 16, 2020 at 09:19:50AM +0000, Christoph Hellwig wrote:
> I just noticed this showing up in Linus' tree and I'm not happy.

Are you sure? I think it's in next.

> 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.

OK but we do have a regression since 5.7 and this looks like
a fix appropriate for e.g. stable, right?

> Bjorn, I thought you were going to look into this a while ago?
> 
> 
> On Wed, Nov 04, 2020 at 03:31:36PM +0000, 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] <Other modules, stack and call trace here>
> > [    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: stable@vger.kernel.org # 5.1+
> > Signed-off-by: Alexander Lobakin <alobakin@pm.me>
> > ---
> >  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 virtio] virtio: virtio_console: fix DMA memory allocation for rproc serial

[Michael S. Tsirkin]

On Mon, Nov 16, 2020 at 01:07:28PM +0000, Alexander Lobakin wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
> Date: Mon, 16 Nov 2020 07:25:31 -0500
> 
> > On Mon, Nov 16, 2020 at 09:19:50AM +0000, Christoph Hellwig wrote:
> >> I just noticed this showing up in Linus' tree and I'm not happy.
> >
> > Are you sure? I think it's in next.
> 
> Nope, it goes to fixes since it just fixes the regression introduced
> in 5.7.

Oh you are right, Greg merged it and I didn't notice because I didn't
rebase my tree.

> That's why it's not about any refactoring or rethinking the whole
> model.
> 
> >> 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.
> 
> But lots of subsystems like netdev for example uses dev->parent for
> DMA operations. I know that their pointers go directly to the
> platform/PCI/etc. device, but still.
> 
> The only reason behind "fake" DMA devices for rproc is to be able to
> reserve DMA memory through the Device Tree exclusively for only one
> virtio dev like virtio_console or virtio_rpmsg_bus. That's why
> they are present, are coercing DMA caps from physical dev
> representor, and why questinable dma_declare_coherent_memory()
> is still here and doesn't allow to build rproc core as a module.
> I agree that this is not the best model obviously, and we should take
> a look at it.
> 
> > OK but we do have a regression since 5.7 and this looks like
> > a fix appropriate for e.g. stable, right?
> >
> >> Bjorn, I thought you were going to look into this a while ago?
> >>
> >>
> >> On Wed, Nov 04, 2020 at 03:31:36PM +0000, 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] <Other modules, stack and call trace here>
> >>> [    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: stable@vger.kernel.org # 5.1+
> >>> Signed-off-by: Alexander Lobakin <alobakin@pm.me>
> >>> ---
> >>>  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---
> 
> Thanks,
> Al

_______________________________________________
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

[Christoph Hellwig]

On Mon, Nov 16, 2020 at 01:07:28PM +0000, Alexander Lobakin wrote:
> But lots of subsystems like netdev for example uses dev->parent for
> DMA operations. I know that their pointers go directly to the
> platform/PCI/etc. device, but still.

Oh, every drivers is perfectly fine to use ->parent as it suits.  The
problem is when we have layered architectures, where this pokes a
massive hole into the layering.

> The only reason behind "fake" DMA devices for rproc is to be able to
> reserve DMA memory through the Device Tree exclusively for only one
> virtio dev like virtio_console or virtio_rpmsg_bus. That's why
> they are present, are coercing DMA caps from physical dev
> representor, and why questinable dma_declare_coherent_memory()
> is still here and doesn't allow to build rproc core as a module.
> I agree that this is not the best model obviously, and we should take
> a look at it.

As far as I can tell the series from Arnaud does the right thing here.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization