> -----Original Message----- > From: linux-remoteproc-owner at vger.kernel.org [mailto:linux-remoteproc- > owner at vger.kernel.org] On Behalf Of Anup Patel > Sent: Sunday, April 22, 2018 6:08 AM > To: Michael S. Tsirkin <mst at redhat.com> > Cc: linux-remoteproc at vger.kernel.org; Ohad Ben-Cohen > <ohad at wizery.com>; Bjorn Andersson <bjorn.andersson at linaro.org>; > virtualization at lists.linux-foundation.org > Subject: Re: virtio remoteproc device > > On Fri, Apr 20, 2018 at 10:23 PM, Michael S. Tsirkin <mst at 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 at 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 at vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 23, 2018 at 08:55:57AM +0000, Loic PALLARDY wrote:> > > > -----Original Message----- > > From: linux-remoteproc-owner at vger.kernel.org [mailto:linux-remoteproc- > > owner at vger.kernel.org] On Behalf Of Anup Patel > > Sent: Sunday, April 22, 2018 6:08 AM > > To: Michael S. Tsirkin <mst at redhat.com> > > Cc: linux-remoteproc at vger.kernel.org; Ohad Ben-Cohen > > <ohad at wizery.com>; Bjorn Andersson <bjorn.andersson at linaro.org>; > > virtualization at lists.linux-foundation.org > > Subject: Re: virtio remoteproc device > > > > On Fri, Apr 20, 2018 at 10:23 PM, Michael S. Tsirkin <mst at 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 at 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/644Thanks! Based on that patch, can remoteproc force VIRTIO_F_IOMMU_PLATFORM in features and then drop special-casing of dma completely?> > > > Regards, > > Anup > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in > > the body of a message to majordomo at vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 23, 2018 at 08:55:57AM +0000, Loic PALLARDY wrote:> > > > -----Original Message----- > > From: linux-remoteproc-owner at vger.kernel.org [mailto:linux-remoteproc- > > owner at vger.kernel.org] On Behalf Of Anup Patel > > Sent: Sunday, April 22, 2018 6:08 AM > > To: Michael S. Tsirkin <mst at redhat.com> > > Cc: linux-remoteproc at vger.kernel.org; Ohad Ben-Cohen > > <ohad at wizery.com>; Bjorn Andersson <bjorn.andersson at linaro.org>; > > virtualization at lists.linux-foundation.org > > Subject: Re: virtio remoteproc device > > > > On Fri, Apr 20, 2018 at 10:23 PM, Michael S. Tsirkin <mst at 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 at 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 at vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.htmlSo 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? 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
> -----Original Message----- > From: Michael S. Tsirkin [mailto:mst at redhat.com] > Sent: Monday, April 23, 2018 9:41 PM > To: Loic PALLARDY <loic.pallardy at st.com> > Cc: Anup Patel <anup at brainfault.org>; linux-remoteproc at vger.kernel.org; > Ohad Ben-Cohen <ohad at wizery.com>; Bjorn Andersson > <bjorn.andersson at linaro.org>; virtualization at 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 at vger.kernel.org [mailto:linux- > remoteproc- > > > owner at vger.kernel.org] On Behalf Of Anup Patel > > > Sent: Sunday, April 22, 2018 6:08 AM > > > To: Michael S. Tsirkin <mst at redhat.com> > > > Cc: linux-remoteproc at vger.kernel.org; Ohad Ben-Cohen > > > <ohad at wizery.com>; Bjorn Andersson <bjorn.andersson at linaro.org>; > > > virtualization at lists.linux-foundation.org > > > Subject: Re: virtio remoteproc device > > > > > > On Fri, Apr 20, 2018 at 10:23 PM, Michael S. Tsirkin <mst at 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 at 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 at 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