RE: virtio remoteproc device

[Loic PALLARDY <loic.pallardy@st.com>]

> -----Original Message-----
> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> Sent: Monday, April 23, 2018 9:41 PM
> To: Loic PALLARDY <loic.pallardy@st.com>
> Cc: Anup Patel <anup@brainfault.org>; linux-remoteproc@vger.kernel.org;
> Ohad Ben-Cohen <ohad@wizery.com>; Bjorn Andersson
> <bjorn.andersson@linaro.org>; virtualization@lists.linux-foundation.org
> Subject: Re: virtio remoteproc device
> 
> On Mon, Apr 23, 2018 at 08:55:57AM +0000, Loic PALLARDY wrote:
> >
> >
> > > -----Original Message-----
> > > From: linux-remoteproc-owner@vger.kernel.org [mailto:linux-
> remoteproc-
> > > owner@vger.kernel.org] On Behalf Of Anup Patel
> > > Sent: Sunday, April 22, 2018 6:08 AM
> > > To: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: linux-remoteproc@vger.kernel.org; Ohad Ben-Cohen
> > > <ohad@wizery.com>; Bjorn Andersson <bjorn.andersson@linaro.org>;
> > > virtualization@lists.linux-foundation.org
> > > Subject: Re: virtio remoteproc device
> > >
> > > On Fri, Apr 20, 2018 at 10:23 PM, Michael S. Tsirkin <mst@redhat.com>
> > > wrote:
> > > > Hello!
> > > > I note the following in the serial console:
> > > >
> > > >       if (is_rproc_serial(vdev)) {
> > > >                 /*
> > > >                  * 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.
> > > >                  */
> > > >                 if (!vdev->dev.parent || !vdev->dev.parent->parent)
> > > >                         goto free_buf;
> > > >                 buf->dev = vdev->dev.parent->parent;
> > > >
> > > >                 /* Increase device refcnt to avoid freeing it */
> > > >                 get_device(buf->dev);
> > > >                 buf->buf = dma_alloc_coherent(buf->dev, buf_size, &buf-
> >dma,
> > > >                                               GFP_KERNEL);
> > > >         }
> > > >
> > > > Added here:
> > > >         commit 1b6370463e88b0c1c317de16d7b962acc1dab4f2
> > > >         Author: Sjur Brændeland <sjur.brandeland@stericsson.com>
> > > >         Date:   Fri Dec 14 14:40:51 2012 +1030
> > > >
> > > >     virtio_console: Add support for remoteproc serial
> > > >
> > > >
> > > > I am not familiar with rproc so I have a question:
> > > > why is it required to use coherent memory here,
> > > > and why through a grandparent device?
> > >
> > > I faced similar issue when I tried VirtIO RPMSG bus over
> > > VirtIO MMIO transport.
> > >
> > > Here's my fix for VirtIO RPMSG bus driver:
> > > https://patchwork.kernel.org/patch/10155145/
> >
> > Hi Anup and Michael,
> >
> > I pushed a series to modify virtio device allocation in remoteproc. Please
> see [1].
> > It allows to remove allocation based on "grand-parent" device in the case
> of virtio device allocated by remoteproc.
> > Virto_console patch missing, only virtio_rpmsg modification proposed. I can
> add it in next version.
> >
> > Regards,
> > Loic
> >
> > [1] https://lkml.org/lkml/2018/3/1/644
> >
> > >
> > > Regards,
> > > Anup
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-
> remoteproc" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> So on top of your patches, can we just force use of DMA API
> and drop special casting?
> E.g. maybe something like the below - completely untested - patch?
> Does it work for you?
> 

Just have a look to VIRTIO_F_IOMMU_PLATFORM usage.
If we activate it, this will change virtio_ring behavior, allowing to rely on dma api, but need to have deeper look to virtio_console impacts.

For sure, if you remove/disable rproc specific code in virtio_console, alloc_buf will rely on kmalloc instead of dma_alloc_coherent. As some coprocessors doesn't have access to complete memory map and in some case no access to DDR memory, buffer copy will have to be done in dedicated vdev memory before adding it to vring.
Specific rproc case was added to avoid copy and directly using shared memory for buffer allocation.

Regards,
Loic
> 
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 2108551..6c6767c 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -40,7 +40,8 @@
>  #include <linux/dma-mapping.h>
>  #include "../tty/hvc/hvc_console.h"
> 
> -#define is_rproc_enabled IS_ENABLED(CONFIG_REMOTEPROC)
> +//#define is_rproc_enabled IS_ENABLED(CONFIG_REMOTEPROC)
> +#define is_rproc_enabled 0
> 
>  /*
>   * This is a global struct for storing common data for all the devices
> diff --git a/drivers/remoteproc/remoteproc_virtio.c
> b/drivers/remoteproc/remoteproc_virtio.c
> index b0633fd..4a13ca2 100644
> --- a/drivers/remoteproc/remoteproc_virtio.c
> +++ b/drivers/remoteproc/remoteproc_virtio.c
> @@ -199,7 +199,7 @@ static u64 rproc_virtio_get_features(struct
> virtio_device *vdev)
> 
>  	rsc = (void *)rvdev->rproc->table_ptr + rvdev->rsc_offset;
> 
> -	return rsc->dfeatures;
> +	return rsc->dfeatures | (1ULL << VIRTIO_F_IOMMU_PLATFORM);
>  }
> 
>  static int rproc_virtio_finalize_features(struct virtio_device *vdev)
> @@ -213,7 +213,7 @@ static int rproc_virtio_finalize_features(struct
> virtio_device *vdev)
>  	vring_transport_features(vdev);
> 
>  	/* Make sure we don't have any features > 32 bits! */
> -	BUG_ON((u32)vdev->features != vdev->features);
> +	//BUG_ON((u32)vdev->features != vdev->features);
> 
>  	/*
>  	 * Remember the finalized features of our vdev, and provide it