RE: [PATCH] xen: introduce xen_vring_use_dma

2020-06-28 Thread Peng Fan
> Subject: Re: [PATCH] xen: introduce xen_vring_use_dma
> 
> On Mon, Jun 29, 2020 at 06:25:41AM +, Peng Fan wrote:
> > > > > > Anyway, re-reading the last messages of the original thread
> > > > > > [1], it looks like Peng had a clear idea on how to fix the general 
> > > > > > issue.
> > > > > > Peng, what happened with that?
> > > >
> > > > We shrinked the rpmsg reserved area to workaround the issue.
> > > > So still use the dma apis in rpmsg.
> > > >
> > > > But here I am going to address domu android trusty issue using virtio.
> > >
> > > My suggestion is to first of all fix DMA API so it works properly.
> >
> > Could you please elaborate more details?
> >
> > You mean the DMA API usage of rpmsg? Or xen domu dma_ops?
> >
> > Thanks,
> > Peng.
> 
> Not 100% sure but I think xen dma ops.

Sorry to ask again, could you explain more details about what issues
do you see of current xen ARM domu dma_ops? 
Or Dom0 swiotlb xen dma_ops?

Thanks,
Pen.g

> 
> --
> MST

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH] xen: introduce xen_vring_use_dma

2020-06-28 Thread Michael S. Tsirkin
On Mon, Jun 29, 2020 at 06:25:41AM +, Peng Fan wrote:
> > > > > Anyway, re-reading the last messages of the original thread [1],
> > > > > it looks like Peng had a clear idea on how to fix the general issue.
> > > > > Peng, what happened with that?
> > >
> > > We shrinked the rpmsg reserved area to workaround the issue.
> > > So still use the dma apis in rpmsg.
> > >
> > > But here I am going to address domu android trusty issue using virtio.
> > 
> > My suggestion is to first of all fix DMA API so it works properly.
> 
> Could you please elaborate more details?
> 
> You mean the DMA API usage of rpmsg? Or xen domu dma_ops?
> 
> Thanks,
> Peng. 

Not 100% sure but I think xen dma ops.

-- 
MST

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


RE: [PATCH] xen: introduce xen_vring_use_dma

2020-06-28 Thread Peng Fan
> Subject: Re: [PATCH] xen: introduce xen_vring_use_dma
> 
> On Mon, Jun 29, 2020 at 03:05:19AM +, Peng Fan wrote:
> > > Subject: Re: [PATCH] xen: introduce xen_vring_use_dma
> > >
> > > On Thu, Jun 25, 2020 at 10:31:27AM -0700, Stefano Stabellini wrote:
> > > > On Wed, 24 Jun 2020, Michael S. Tsirkin wrote:
> > > > > On Wed, Jun 24, 2020 at 02:53:54PM -0700, Stefano Stabellini wrote:
> > > > > > On Wed, 24 Jun 2020, Michael S. Tsirkin wrote:
> > > > > > > On Wed, Jun 24, 2020 at 10:59:47AM -0700, Stefano Stabellini
> wrote:
> > > > > > > > On Wed, 24 Jun 2020, Michael S. Tsirkin wrote:
> > > > > > > > > On Wed, Jun 24, 2020 at 05:17:32PM +0800, Peng Fan wrote:
> > > > > > > > > > Export xen_swiotlb for all platforms using xen swiotlb
> > > > > > > > > >
> > > > > > > > > > Use xen_swiotlb to determine when vring should use dma
> > > > > > > > > > APIs to map the
> > > > > > > > > > ring: when xen_swiotlb is enabled the dma API is required.
> > > > > > > > > > When it is disabled, it is not required.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Peng Fan 
> > > > > > > > >
> > > > > > > > > Isn't there some way to use VIRTIO_F_IOMMU_PLATFORM for
> > > this?
> > > > > > > > > Xen was there first, but everyone else is using that now.
> > > > > > > >
> > > > > > > > Unfortunately it is complicated and it is not related to
> > > > > > > > VIRTIO_F_IOMMU_PLATFORM :-(
> > > > > > > >
> > > > > > > >
> > > > > > > > The Xen subsystem in Linux uses dma_ops via swiotlb_xen to
> > > > > > > > translate foreign mappings (memory coming from other VMs)
> > > > > > > > to
> > > physical addresses.
> > > > > > > > On x86, it also uses dma_ops to translate Linux's idea of
> > > > > > > > a physical address into a real physical address (this is
> > > > > > > > unneeded on ARM.)
> > > > > > > >
> > > > > > > >
> > > > > > > > So regardless of VIRTIO_F_IOMMU_PLATFORM, dma_ops should
> > > > > > > > be used on Xen/x86 always and on Xen/ARM if Linux is Dom0
> > > > > > > > (because it has foreign
> > > > > > > > mappings.) That is why we have the if (xen_domain) return
> > > > > > > > true; in vring_use_dma_api.
> > > > > > >
> > > > > > > VIRTIO_F_IOMMU_PLATFORM makes guest always use DMA ops.
> > > > > > >
> > > > > > > Xen hack predates VIRTIO_F_IOMMU_PLATFORM so it *also*
> > > > > > > forces DMA ops even if VIRTIO_F_IOMMU_PLATFORM is clear.
> > > > > > >
> > > > > > > Unfortunately as a result Xen never got around to properly
> > > > > > > setting VIRTIO_F_IOMMU_PLATFORM.
> > > > > >
> > > > > > I don't think VIRTIO_F_IOMMU_PLATFORM would be correct for
> > > > > > this because the usage of swiotlb_xen is not a property of
> > > > > > virtio,
> > > > >
> > > > >
> > > > > Basically any device without VIRTIO_F_ACCESS_PLATFORM (that is
> > > > > it's name in latest virtio spec, VIRTIO_F_IOMMU_PLATFORM is what
> > > > > linux calls it) is declared as "special, don't follow normal
> > > > > rules for access".
> > > > >
> > > > > So yes swiotlb_xen is not a property of virtio, but what *is* a
> > > > > property of virtio is that it's not special, just a regular
> > > > > device from DMA
> > > POV.
> > > >
> > > > I am trying to understand what you meant but I think I am missing
> > > > something.
> > > >
> > > > Are you saying that modern virtio should always have
> > > > VIRTIO_F_ACCESS_PLATFORM, hence use normal dma_ops as any other
> > > devices?
> > >
> > > I am saying it's a safe default. Clear VIRTIO_F_ACCESS_PLATFORM if
> > > you have some special needs e.g. you are very sure it's ok to bypass
> > > DMA ops, or you need to support a legacy guest (produced in the
> > > window between virtio 1 support in 2014 and support for
> VIRTIO_F_ACCESS_PLATFORM in 2016).
> > >
> > >
> > > > If that is the case, how is it possible that virtio breaks on ARM
> > > > using the default dma_ops? The breakage is not Xen related (except
> > > > that Xen turns dma_ops on). The original message from Peng was:
> > > >
> > > >   vring_map_one_sg -> vring_use_dma_api
> > > >-> dma_map_page
> > > >-> __swiotlb_map_page
> > > > ->swiotlb_map_page
> > > > 
> > > > ->__dma_map_area(phys_to_virt(dma_to_phys(dev,
> > > dev_addr)), size, dir);
> > > >   However we are using per device dma area for rpmsg, phys_to_virt
> > > >   could not return a correct virtual address for virtual address in
> > > >   vmalloc area. Then kernel panic.
> > > >
> > > > I must be missing something. Maybe it is because it has to do with
> RPMesg?
> > >
> > > I think it's an RPMesg bug, yes
> >
> > rpmsg bug is another issue, it should not use dma_alloc_coherent for
> > reserved area, and use vmalloc_to_page.
> >
> > Anyway here using dma api will also trigger issue.
> >
> > >
> > > >
> > > > > > > > You might have noticed that I missed one possible case above:
> > > > > > > > Xen/ARM DomU :-)
> > > > > > > >
> > > > > > > > Xen/ARM domUs don't need swiot

Re: [PATCH] xen: introduce xen_vring_use_dma

2020-06-28 Thread Michael S. Tsirkin
On Mon, Jun 29, 2020 at 03:05:19AM +, Peng Fan wrote:
> > Subject: Re: [PATCH] xen: introduce xen_vring_use_dma
> > 
> > On Thu, Jun 25, 2020 at 10:31:27AM -0700, Stefano Stabellini wrote:
> > > On Wed, 24 Jun 2020, Michael S. Tsirkin wrote:
> > > > On Wed, Jun 24, 2020 at 02:53:54PM -0700, Stefano Stabellini wrote:
> > > > > On Wed, 24 Jun 2020, Michael S. Tsirkin wrote:
> > > > > > On Wed, Jun 24, 2020 at 10:59:47AM -0700, Stefano Stabellini wrote:
> > > > > > > On Wed, 24 Jun 2020, Michael S. Tsirkin wrote:
> > > > > > > > On Wed, Jun 24, 2020 at 05:17:32PM +0800, Peng Fan wrote:
> > > > > > > > > Export xen_swiotlb for all platforms using xen swiotlb
> > > > > > > > >
> > > > > > > > > Use xen_swiotlb to determine when vring should use dma
> > > > > > > > > APIs to map the
> > > > > > > > > ring: when xen_swiotlb is enabled the dma API is required.
> > > > > > > > > When it is disabled, it is not required.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Peng Fan 
> > > > > > > >
> > > > > > > > Isn't there some way to use VIRTIO_F_IOMMU_PLATFORM for
> > this?
> > > > > > > > Xen was there first, but everyone else is using that now.
> > > > > > >
> > > > > > > Unfortunately it is complicated and it is not related to
> > > > > > > VIRTIO_F_IOMMU_PLATFORM :-(
> > > > > > >
> > > > > > >
> > > > > > > The Xen subsystem in Linux uses dma_ops via swiotlb_xen to
> > > > > > > translate foreign mappings (memory coming from other VMs) to
> > physical addresses.
> > > > > > > On x86, it also uses dma_ops to translate Linux's idea of a
> > > > > > > physical address into a real physical address (this is
> > > > > > > unneeded on ARM.)
> > > > > > >
> > > > > > >
> > > > > > > So regardless of VIRTIO_F_IOMMU_PLATFORM, dma_ops should be
> > > > > > > used on Xen/x86 always and on Xen/ARM if Linux is Dom0
> > > > > > > (because it has foreign
> > > > > > > mappings.) That is why we have the if (xen_domain) return
> > > > > > > true; in vring_use_dma_api.
> > > > > >
> > > > > > VIRTIO_F_IOMMU_PLATFORM makes guest always use DMA ops.
> > > > > >
> > > > > > Xen hack predates VIRTIO_F_IOMMU_PLATFORM so it *also* forces
> > > > > > DMA ops even if VIRTIO_F_IOMMU_PLATFORM is clear.
> > > > > >
> > > > > > Unfortunately as a result Xen never got around to properly
> > > > > > setting VIRTIO_F_IOMMU_PLATFORM.
> > > > >
> > > > > I don't think VIRTIO_F_IOMMU_PLATFORM would be correct for this
> > > > > because the usage of swiotlb_xen is not a property of virtio,
> > > >
> > > >
> > > > Basically any device without VIRTIO_F_ACCESS_PLATFORM (that is it's
> > > > name in latest virtio spec, VIRTIO_F_IOMMU_PLATFORM is what linux
> > > > calls it) is declared as "special, don't follow normal rules for
> > > > access".
> > > >
> > > > So yes swiotlb_xen is not a property of virtio, but what *is* a
> > > > property of virtio is that it's not special, just a regular device from 
> > > > DMA
> > POV.
> > >
> > > I am trying to understand what you meant but I think I am missing
> > > something.
> > >
> > > Are you saying that modern virtio should always have
> > > VIRTIO_F_ACCESS_PLATFORM, hence use normal dma_ops as any other
> > devices?
> > 
> > I am saying it's a safe default. Clear VIRTIO_F_ACCESS_PLATFORM if you have
> > some special needs e.g. you are very sure it's ok to bypass DMA ops, or you
> > need to support a legacy guest (produced in the window between virtio 1
> > support in 2014 and support for VIRTIO_F_ACCESS_PLATFORM in 2016).
> > 
> > 
> > > If that is the case, how is it possible that virtio breaks on ARM
> > > using the default dma_ops? The breakage is not Xen related (except
> > > that Xen turns dma_ops on). The original message from Peng was:
> > >
> > >   vring_map_one_sg -> vring_use_dma_api
> > >-> dma_map_page
> > >  -> __swiotlb_map_page
> > >   ->swiotlb_map_page
> > >   
> > > ->__dma_map_area(phys_to_virt(dma_to_phys(dev,
> > dev_addr)), size, dir);
> > >   However we are using per device dma area for rpmsg, phys_to_virt
> > >   could not return a correct virtual address for virtual address in
> > >   vmalloc area. Then kernel panic.
> > >
> > > I must be missing something. Maybe it is because it has to do with RPMesg?
> > 
> > I think it's an RPMesg bug, yes
> 
> rpmsg bug is another issue, it should not use dma_alloc_coherent for reserved 
> area,
> and use vmalloc_to_page.
> 
> Anyway here using dma api will also trigger issue.
> 
> > 
> > >
> > > > > > > You might have noticed that I missed one possible case above:
> > > > > > > Xen/ARM DomU :-)
> > > > > > >
> > > > > > > Xen/ARM domUs don't need swiotlb_xen, it is not even
> > > > > > > initialized. So if
> > > > > > > (xen_domain) return true; would give the wrong answer in that 
> > > > > > > case.
> > > > > > > Linux would end up calling the "normal" dma_ops, not
> > > > > > > swiotlb-xen, and the "normal" dma_ops fail.
> > > > 

RE: [PATCH] xen: introduce xen_vring_use_dma

2020-06-28 Thread Peng Fan
> Subject: Re: [PATCH] xen: introduce xen_vring_use_dma
> 
> On Wed, 24 Jun 2020, Michael S. Tsirkin wrote:
> > On Wed, Jun 24, 2020 at 02:53:54PM -0700, Stefano Stabellini wrote:
> > > On Wed, 24 Jun 2020, Michael S. Tsirkin wrote:
> > > > On Wed, Jun 24, 2020 at 10:59:47AM -0700, Stefano Stabellini wrote:
> > > > > On Wed, 24 Jun 2020, Michael S. Tsirkin wrote:
> > > > > > On Wed, Jun 24, 2020 at 05:17:32PM +0800, Peng Fan wrote:
> > > > > > > Export xen_swiotlb for all platforms using xen swiotlb
> > > > > > >
> > > > > > > Use xen_swiotlb to determine when vring should use dma APIs
> > > > > > > to map the
> > > > > > > ring: when xen_swiotlb is enabled the dma API is required.
> > > > > > > When it is disabled, it is not required.
> > > > > > >
> > > > > > > Signed-off-by: Peng Fan 
> > > > > >
> > > > > > Isn't there some way to use VIRTIO_F_IOMMU_PLATFORM for this?
> > > > > > Xen was there first, but everyone else is using that now.
> > > > >
> > > > > Unfortunately it is complicated and it is not related to
> > > > > VIRTIO_F_IOMMU_PLATFORM :-(
> > > > >
> > > > >
> > > > > The Xen subsystem in Linux uses dma_ops via swiotlb_xen to
> > > > > translate foreign mappings (memory coming from other VMs) to
> physical addresses.
> > > > > On x86, it also uses dma_ops to translate Linux's idea of a
> > > > > physical address into a real physical address (this is unneeded
> > > > > on ARM.)
> > > > >
> > > > >
> > > > > So regardless of VIRTIO_F_IOMMU_PLATFORM, dma_ops should be
> used
> > > > > on Xen/x86 always and on Xen/ARM if Linux is Dom0 (because it
> > > > > has foreign
> > > > > mappings.) That is why we have the if (xen_domain) return true;
> > > > > in vring_use_dma_api.
> > > >
> > > > VIRTIO_F_IOMMU_PLATFORM makes guest always use DMA ops.
> > > >
> > > > Xen hack predates VIRTIO_F_IOMMU_PLATFORM so it *also* forces
> DMA
> > > > ops even if VIRTIO_F_IOMMU_PLATFORM is clear.
> > > >
> > > > Unfortunately as a result Xen never got around to properly setting
> > > > VIRTIO_F_IOMMU_PLATFORM.
> > >
> > > I don't think VIRTIO_F_IOMMU_PLATFORM would be correct for this
> > > because the usage of swiotlb_xen is not a property of virtio,
> >
> >
> > Basically any device without VIRTIO_F_ACCESS_PLATFORM (that is it's
> > name in latest virtio spec, VIRTIO_F_IOMMU_PLATFORM is what linux
> > calls it) is declared as "special, don't follow normal rules for
> > access".
> >
> > So yes swiotlb_xen is not a property of virtio, but what *is* a
> > property of virtio is that it's not special, just a regular device from DMA 
> > POV.
> 
> I am trying to understand what you meant but I think I am missing something.
> 
> Are you saying that modern virtio should always have
> VIRTIO_F_ACCESS_PLATFORM, hence use normal dma_ops as any other
> devices?
> 
> If that is the case, how is it possible that virtio breaks on ARM using the
> default dma_ops? The breakage is not Xen related (except that Xen turns
> dma_ops on). The original message from Peng was:
> 
>   vring_map_one_sg -> vring_use_dma_api
>-> dma_map_page
>  -> __swiotlb_map_page
>   ->swiotlb_map_page
>   ->__dma_map_area(phys_to_virt(dma_to_phys(dev,
> dev_addr)), size, dir);
>   However we are using per device dma area for rpmsg, phys_to_virt
>   could not return a correct virtual address for virtual address in
>   vmalloc area. Then kernel panic.
> 
> I must be missing something. Maybe it is because it has to do with RPMesg?

I am not going to fix the rpmsg issue with this patch. It is when ARM DomU
Android os communicate with secure world trusty os using virtio, the
vring_use_dma_api will return true for xen domu, but I no need it return
true and fall into swiotlb.

Thanks,
Peng.

> 
> 
> > > > > You might have noticed that I missed one possible case above:
> > > > > Xen/ARM DomU :-)
> > > > >
> > > > > Xen/ARM domUs don't need swiotlb_xen, it is not even
> > > > > initialized. So if
> > > > > (xen_domain) return true; would give the wrong answer in that case.
> > > > > Linux would end up calling the "normal" dma_ops, not
> > > > > swiotlb-xen, and the "normal" dma_ops fail.
> > > > >
> > > > >
> > > > > The solution I suggested was to make the check in
> > > > > vring_use_dma_api more flexible by returning true if the
> > > > > swiotlb_xen is supposed to be used, not in general for all Xen
> > > > > domains, because that is what the check was really meant to do.
> > > >
> > > > Why not fix DMA ops so they DTRT (nop) on Xen/ARM DomU? What is
> wrong with that?
> > >
> > > swiotlb-xen is not used on Xen/ARM DomU, the default dma_ops are the
> > > ones that are used. So you are saying, why don't we fix the default
> > > dma_ops to work with virtio?
> > >
> > > It is bad that the default dma_ops crash with virtio, so yes I think
> > > it would be good to fix that. However, even if we fixed that, the if
> > > (xen_domain()) check in vring_use_dma_api is still a

RE: [PATCH] xen: introduce xen_vring_use_dma

2020-06-28 Thread Peng Fan
> Subject: Re: [PATCH] xen: introduce xen_vring_use_dma
> 
> On Thu, Jun 25, 2020 at 10:31:27AM -0700, Stefano Stabellini wrote:
> > On Wed, 24 Jun 2020, Michael S. Tsirkin wrote:
> > > On Wed, Jun 24, 2020 at 02:53:54PM -0700, Stefano Stabellini wrote:
> > > > On Wed, 24 Jun 2020, Michael S. Tsirkin wrote:
> > > > > On Wed, Jun 24, 2020 at 10:59:47AM -0700, Stefano Stabellini wrote:
> > > > > > On Wed, 24 Jun 2020, Michael S. Tsirkin wrote:
> > > > > > > On Wed, Jun 24, 2020 at 05:17:32PM +0800, Peng Fan wrote:
> > > > > > > > Export xen_swiotlb for all platforms using xen swiotlb
> > > > > > > >
> > > > > > > > Use xen_swiotlb to determine when vring should use dma
> > > > > > > > APIs to map the
> > > > > > > > ring: when xen_swiotlb is enabled the dma API is required.
> > > > > > > > When it is disabled, it is not required.
> > > > > > > >
> > > > > > > > Signed-off-by: Peng Fan 
> > > > > > >
> > > > > > > Isn't there some way to use VIRTIO_F_IOMMU_PLATFORM for
> this?
> > > > > > > Xen was there first, but everyone else is using that now.
> > > > > >
> > > > > > Unfortunately it is complicated and it is not related to
> > > > > > VIRTIO_F_IOMMU_PLATFORM :-(
> > > > > >
> > > > > >
> > > > > > The Xen subsystem in Linux uses dma_ops via swiotlb_xen to
> > > > > > translate foreign mappings (memory coming from other VMs) to
> physical addresses.
> > > > > > On x86, it also uses dma_ops to translate Linux's idea of a
> > > > > > physical address into a real physical address (this is
> > > > > > unneeded on ARM.)
> > > > > >
> > > > > >
> > > > > > So regardless of VIRTIO_F_IOMMU_PLATFORM, dma_ops should be
> > > > > > used on Xen/x86 always and on Xen/ARM if Linux is Dom0
> > > > > > (because it has foreign
> > > > > > mappings.) That is why we have the if (xen_domain) return
> > > > > > true; in vring_use_dma_api.
> > > > >
> > > > > VIRTIO_F_IOMMU_PLATFORM makes guest always use DMA ops.
> > > > >
> > > > > Xen hack predates VIRTIO_F_IOMMU_PLATFORM so it *also* forces
> > > > > DMA ops even if VIRTIO_F_IOMMU_PLATFORM is clear.
> > > > >
> > > > > Unfortunately as a result Xen never got around to properly
> > > > > setting VIRTIO_F_IOMMU_PLATFORM.
> > > >
> > > > I don't think VIRTIO_F_IOMMU_PLATFORM would be correct for this
> > > > because the usage of swiotlb_xen is not a property of virtio,
> > >
> > >
> > > Basically any device without VIRTIO_F_ACCESS_PLATFORM (that is it's
> > > name in latest virtio spec, VIRTIO_F_IOMMU_PLATFORM is what linux
> > > calls it) is declared as "special, don't follow normal rules for
> > > access".
> > >
> > > So yes swiotlb_xen is not a property of virtio, but what *is* a
> > > property of virtio is that it's not special, just a regular device from 
> > > DMA
> POV.
> >
> > I am trying to understand what you meant but I think I am missing
> > something.
> >
> > Are you saying that modern virtio should always have
> > VIRTIO_F_ACCESS_PLATFORM, hence use normal dma_ops as any other
> devices?
> 
> I am saying it's a safe default. Clear VIRTIO_F_ACCESS_PLATFORM if you have
> some special needs e.g. you are very sure it's ok to bypass DMA ops, or you
> need to support a legacy guest (produced in the window between virtio 1
> support in 2014 and support for VIRTIO_F_ACCESS_PLATFORM in 2016).
> 
> 
> > If that is the case, how is it possible that virtio breaks on ARM
> > using the default dma_ops? The breakage is not Xen related (except
> > that Xen turns dma_ops on). The original message from Peng was:
> >
> >   vring_map_one_sg -> vring_use_dma_api
> >-> dma_map_page
> >-> __swiotlb_map_page
> > ->swiotlb_map_page
> > ->__dma_map_area(phys_to_virt(dma_to_phys(dev,
> dev_addr)), size, dir);
> >   However we are using per device dma area for rpmsg, phys_to_virt
> >   could not return a correct virtual address for virtual address in
> >   vmalloc area. Then kernel panic.
> >
> > I must be missing something. Maybe it is because it has to do with RPMesg?
> 
> I think it's an RPMesg bug, yes

rpmsg bug is another issue, it should not use dma_alloc_coherent for reserved 
area,
and use vmalloc_to_page.

Anyway here using dma api will also trigger issue.

> 
> >
> > > > > > You might have noticed that I missed one possible case above:
> > > > > > Xen/ARM DomU :-)
> > > > > >
> > > > > > Xen/ARM domUs don't need swiotlb_xen, it is not even
> > > > > > initialized. So if
> > > > > > (xen_domain) return true; would give the wrong answer in that case.
> > > > > > Linux would end up calling the "normal" dma_ops, not
> > > > > > swiotlb-xen, and the "normal" dma_ops fail.
> > > > > >
> > > > > >
> > > > > > The solution I suggested was to make the check in
> > > > > > vring_use_dma_api more flexible by returning true if the
> > > > > > swiotlb_xen is supposed to be used, not in general for all Xen
> > > > > > domains, because that is what the check was really meant to do.
> > > > >
> > > > 

Re: [PATCH RFC 4/5] vhost-vdpa: support IOTLB batching hints

2020-06-28 Thread Michael S. Tsirkin
On Thu, Jun 18, 2020 at 01:56:25PM +0800, Jason Wang wrote:
> This patches extend the vhost IOTLB API to accept batch updating hints
> form userspace. When userspace wants update the device IOTLB in a
> batch, it may do:
> 
> 1) Write vhost_iotlb_msg with VHOST_IOTLB_BATCH_BEGIN flag
> 2) Perform a batch of IOTLB updating via VHOST_IOTLB_UPDATE/INVALIDATE
> 3) Write vhost_iotlb_msg with VHOST_IOTLB_BATCH_END flag

As long as we are extending the interface,
is there some way we could cut down the number of system calls needed
here?


> 
> Vhost-vdpa may decide to batch the IOMMU/IOTLB updating in step 3 when
> vDPA device support set_map() ops. This is useful for the vDPA device
> that want to know all the mappings to tweak their own DMA translation
> logic.
> 
> For vDPA device that doesn't require set_map(), no behavior changes.
> 
> This capability is advertised via VHOST_BACKEND_F_IOTLB_BATCH capability.
> 
> Signed-off-by: Jason Wang 
> ---
>  drivers/vhost/vdpa.c | 30 +++---
>  include/uapi/linux/vhost.h   |  2 ++
>  include/uapi/linux/vhost_types.h |  7 +++
>  3 files changed, 32 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 453057421f80..8f624bbafee7 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -56,7 +56,9 @@ enum {
>  };
>  
>  enum {
> - VHOST_VDPA_BACKEND_FEATURES = (1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2)
> + VHOST_VDPA_BACKEND_FEATURES =
> + (1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2) |
> + (1ULL << VHOST_BACKEND_F_IOTLB_BATCH),
>  };
>  
>  /* Currently, only network backend w/o multiqueue is supported. */
> @@ -77,6 +79,7 @@ struct vhost_vdpa {
>   int virtio_id;
>   int minor;
>   struct eventfd_ctx *config_ctx;
> + int in_batch;
>  };
>  
>  static DEFINE_IDA(vhost_vdpa_ida);
> @@ -125,6 +128,7 @@ static void vhost_vdpa_reset(struct vhost_vdpa *v)
>   const struct vdpa_config_ops *ops = vdpa->config;
>  
>   ops->set_status(vdpa, 0);
> + v->in_batch = 0;
>  }
>  
>  static long vhost_vdpa_get_device_id(struct vhost_vdpa *v, u8 __user *argp)
> @@ -540,9 +544,10 @@ static int vhost_vdpa_map(struct vhost_vdpa *v,
>  
>   if (ops->dma_map)
>   r = ops->dma_map(vdpa, iova, size, pa, perm);
> - else if (ops->set_map)
> - r = ops->set_map(vdpa, dev->iotlb);
> - else
> + else if (ops->set_map) {
> + if (!v->in_batch)
> + r = ops->set_map(vdpa, dev->iotlb);
> + } else
>   r = iommu_map(v->domain, iova, pa, size,
> perm_to_iommu_flags(perm));
>  
> @@ -559,9 +564,10 @@ static void vhost_vdpa_unmap(struct vhost_vdpa *v, u64 
> iova, u64 size)
>  
>   if (ops->dma_map)
>   ops->dma_unmap(vdpa, iova, size);
> - else if (ops->set_map)
> - ops->set_map(vdpa, dev->iotlb);
> - else
> + else if (ops->set_map) {
> + if (!v->in_batch)
> + ops->set_map(vdpa, dev->iotlb);
> + } else
>   iommu_unmap(v->domain, iova, size);
>  }
>  
> @@ -655,6 +661,8 @@ static int vhost_vdpa_process_iotlb_msg(struct vhost_dev 
> *dev,
>   struct vhost_iotlb_msg *msg)
>  {
>   struct vhost_vdpa *v = container_of(dev, struct vhost_vdpa, vdev);
> + struct vdpa_device *vdpa = v->vdpa;
> + const struct vdpa_config_ops *ops = vdpa->config;
>   int r = 0;
>  
>   r = vhost_dev_check_owner(dev);
> @@ -668,6 +676,14 @@ static int vhost_vdpa_process_iotlb_msg(struct vhost_dev 
> *dev,
>   case VHOST_IOTLB_INVALIDATE:
>   vhost_vdpa_unmap(v, msg->iova, msg->size);
>   break;
> + case VHOST_IOTLB_BATCH_BEGIN:
> + v->in_batch = true;
> + break;
> + case VHOST_IOTLB_BATCH_END:
> + if (v->in_batch && ops->set_map)
> + ops->set_map(vdpa, dev->iotlb);
> + v->in_batch = false;
> + break;
>   default:
>   r = -EINVAL;
>   break;
> diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> index 0c2349612e77..565da96f55d5 100644
> --- a/include/uapi/linux/vhost.h
> +++ b/include/uapi/linux/vhost.h
> @@ -91,6 +91,8 @@
>  
>  /* Use message type V2 */
>  #define VHOST_BACKEND_F_IOTLB_MSG_V2 0x1
> +/* IOTLB can accpet batching hints */

typo

> +#define VHOST_BACKEND_F_IOTLB_BATCH  0x2
>  
>  #define VHOST_SET_BACKEND_FEATURES _IOW(VHOST_VIRTIO, 0x25, __u64)
>  #define VHOST_GET_BACKEND_FEATURES _IOR(VHOST_VIRTIO, 0x26, __u64)
> diff --git a/include/uapi/linux/vhost_types.h 
> b/include/uapi/linux/vhost_types.h
> index 669457ce5c48..5c12faffdde9 100644
> --- a/include/uapi/linux/vhost_types.h
> +++ b/include/uapi/linux/vhost_types.h
> @@ -60,6 +60,13 @@ struct vhost_iotlb_msg {
>  #define VHOST_IOTLB_UPDATE 2
>  #define VHOST_IOTLB_INVALIDATE 3
>  #define VHOST_IOTLB_ACCESS_F