| Message ID | 20200624091732.23944-1-peng.fan@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: introduce xen_vring_use_dma | expand |
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 <peng.fan@nxp.com> Isn't there some way to use VIRTIO_F_IOMMU_PLATFORM for this? Xen was there first, but everyone else is using that now. > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > index a2de775801af..768afd79f67a 100644 > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -252,7 +252,7 @@ static bool vring_use_dma_api(struct virtio_device *vdev) > * the DMA API if we're a Xen guest, which at least allows > * all of the sensible Xen configurations to work correctly. > */ > - if (xen_domain()) > + if (xen_vring_use_dma()) > return true; > > return false; The comment above this should probably be fixed.
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 <peng.fan@nxp.com> > > 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. 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. In this regards I have two more comments: - the comment on top of the check in vring_use_dma_api is still valid - the patch looks broken on x86: it should always return true, but it returns false instead > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > index a2de775801af..768afd79f67a 100644 > > --- a/drivers/virtio/virtio_ring.c > > +++ b/drivers/virtio/virtio_ring.c > > @@ -252,7 +252,7 @@ static bool vring_use_dma_api(struct virtio_device *vdev) > > * the DMA API if we're a Xen guest, which at least allows > > * all of the sensible Xen configurations to work correctly. > > */ > > - if (xen_domain()) > > + if (xen_vring_use_dma()) > > return true; > > > > return false; > > > The comment above this should probably be fixed. >
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 <peng.fan@nxp.com> > > > > 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 would like to make Xen do what everyone else is doing which is just set VIRTIO_F_IOMMU_PLATFORM and then put platform specific hacks inside DMA API. Then we can think about deprecating the Xen hack in a distance future, or hiding it behind a static branch, or something like this. > 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? > > In this regards I have two more comments: > > - the comment on top of the check in vring_use_dma_api is still valid > - the patch looks broken on x86: it should always return true, but it > returns false instead > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > > index a2de775801af..768afd79f67a 100644 > > > --- a/drivers/virtio/virtio_ring.c > > > +++ b/drivers/virtio/virtio_ring.c > > > @@ -252,7 +252,7 @@ static bool vring_use_dma_api(struct virtio_device *vdev) > > > * the DMA API if we're a Xen guest, which at least allows > > > * all of the sensible Xen configurations to work correctly. > > > */ > > > - if (xen_domain()) > > > + if (xen_vring_use_dma()) > > > return true; > > > > > > return false; > > > > > > The comment above this should probably be fixed. > > >
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 <peng.fan@nxp.com> > > > > > > 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, it is a detail of the way Linux does Xen address translations. swiotlb-xen is used to do these translations and it is hooked into the dma_ops framework. It would be possible to have a device in hardware that is virtio-compatible and doesn't set VIRTIO_F_IOMMU_PLATFORM. The device could be directly assigned (passthrough) to a DomU. We would still have to use swiotlb_xen if Xen is running. You should think of swiotlb-xen as only internal to Linux and not related to whether the (virtual or non-virtual) hardware comes with an IOMMU or not. > > 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 problem. Alternatively we could try to work-around it from swiotlb-xen. We could enable swiotlb-xen for Xen/ARM DomUs with a different implementation so that we could leave the vring_use_dma_api check unmodified. It would be ugly because we would have to figure out from the new swiotlb-xen functions if the device is a normal device, so we have to call the regular dma_ops functions, or if the device is a virtio device, in which case there is nothing to do. I think it is undesirable but could probably be made to work.
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 <peng.fan@nxp.com> > > > > > > > > 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. > it is a detail of > the way Linux does Xen address translations. swiotlb-xen is used to do > these translations and it is hooked into the dma_ops framework. > > It would be possible to have a device in hardware that is > virtio-compatible and doesn't set VIRTIO_F_IOMMU_PLATFORM. That device would be basically broken, since hardware can't know whether it can access all memory or not. > The device > could be directly assigned (passthrough) to a DomU. We would still > have to use swiotlb_xen if Xen is running. > > You should think of swiotlb-xen as only internal to Linux and not > related to whether the (virtual or non-virtual) hardware comes with an > IOMMU or not. IOMMU is a misnomer here. Virtio spec now calls this bit VIRTIO_F_ACCESS_PLATFORM. We should have done the same a while ago - I'll send a patch. > > > > 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 problem. Why is it a problem? It just makes virtio use DMA API. If that in turn works, problem solved. > > Alternatively we could try to work-around it from swiotlb-xen. We could > enable swiotlb-xen for Xen/ARM DomUs with a different implementation so > that we could leave the vring_use_dma_api check unmodified. > > It would be ugly because we would have to figure out from the new > swiotlb-xen functions if the device is a normal device, so we have to > call the regular dma_ops functions, or if the device is a virtio device, > in which case there is nothing to do. I think it is undesirable but > could probably be made to work.
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 <peng.fan@nxp.com> > > > > > > > > > > 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? > > > > 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 problem. > > Why is it a problem? It just makes virtio use DMA API. > If that in turn works, problem solved. You are correct in the sense that it would work. However I do think it is wrong for vring_use_dma_api to enable dma_ops/swiotlb-xen for Xen/ARM DomUs that don't need it. There are many different types of Xen guests, Xen x86 is drastically different from Xen ARM, it seems wrong to treat them the same way. 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? [1] https://lore.kernel.org/patchwork/patch/1033801/#1222404
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 <peng.fan@nxp.com> > > > > > > > > > > > > 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. > > > > > > 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 problem. > > > > Why is it a problem? It just makes virtio use DMA API. > > If that in turn works, problem solved. > > You are correct in the sense that it would work. However I do think it > is wrong for vring_use_dma_api to enable dma_ops/swiotlb-xen for Xen/ARM > DomUs that don't need it. There are many different types of Xen guests, > Xen x86 is drastically different from Xen ARM, it seems wrong to treat > them the same way. I could imagine some future Xen hosts setting a flag somewhere in the platform capability saying "no xen specific flag, rely on "VIRTIO_F_ACCESS_PLATFORM". Then you set that accordingly in QEMU. How about that? > > > 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? > > > [1] https://lore.kernel.org/patchwork/patch/1033801/#1222404
> 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 <peng.fan@nxp.com> > > > > > > > > > > > > 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 problem. > > > > Why is it a problem? It just makes virtio use DMA API. > > If that in turn works, problem solved. > > You are correct in the sense that it would work. However I do think it is wrong > for vring_use_dma_api to enable dma_ops/swiotlb-xen for Xen/ARM DomUs > that don't need it. There are many different types of Xen guests, Xen x86 is > drastically different from Xen ARM, it seems wrong to treat them the same > way. > > > > 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? > > > [1] > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.ke > rnel.org%2Fpatchwork%2Fpatch%2F1033801%2F%231222404&data=02 > %7C01%7Cpeng.fan%40nxp.com%7C27edb29c11da49a2249008d8192d98cc > %7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637287030912707 > 092&sdata=MsF%2FLmBmJ1V%2BoOQ%2FmdhEJ3PFzH55DaSNvorRUU > QvBvQ%3D&reserved=0
> 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 <peng.fan@nxp.com> > > > > > > > > > > > > > > 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. > > > > > > > > > > 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 problem. > > > > > > Why is it a problem? It just makes virtio use DMA API. > > > If that in turn works, problem solved. > > > > You are correct in the sense that it would work. However I do think it > > is wrong for vring_use_dma_api to enable dma_ops/swiotlb-xen for > > Xen/ARM DomUs that don't need it. There are many different types of > > Xen guests, Xen x86 is drastically different from Xen ARM, it seems > > wrong to treat them the same way. > > I could imagine some future Xen hosts setting a flag somewhere in the > platform capability saying "no xen specific flag, rely on > "VIRTIO_F_ACCESS_PLATFORM". Then you set that accordingly in QEMU. > How about that? > Michael, Stefano, So what's your suggestion here, that we could avoid similar issue for virtio drivers in ARM DomU? Thanks, Peng. > > > > > > > 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. > > > > > > [1] > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore > > .kernel.org%2Fpatchwork%2Fpatch%2F1033801%2F%231222404&dat > a=02%7C0 > > > 1%7Cpeng.fan%40nxp.com%7C08ba48d3b3d54e775a8108d819e62fd0%7C68 > 6ea1d3bc > > > 2b4c6fa92cd99c5c301635%7C0%7C0%7C637287823721994475&sdata > =Cw4FHWrH > > uVKBCn3%2BKS2VM7cWuGoTI6R7SHJrJSLY5Io%3D&reserved=0
On Mon, Jun 29, 2020 at 03:05:19AM +0000, 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 <peng.fan@nxp.com> > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > 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 problem. > > > > > > > > Why is it a problem? It just makes virtio use DMA API. > > > > If that in turn works, problem solved. > > > > > > You are correct in the sense that it would work. However I do think it > > > is wrong for vring_use_dma_api to enable dma_ops/swiotlb-xen for > > > Xen/ARM DomUs that don't need it. There are many different types of > > > Xen guests, Xen x86 is drastically different from Xen ARM, it seems > > > wrong to treat them the same way. > > > > I could imagine some future Xen hosts setting a flag somewhere in the > > platform capability saying "no xen specific flag, rely on > > "VIRTIO_F_ACCESS_PLATFORM". Then you set that accordingly in QEMU. > > How about that? > > > > Michael, Stefano, > > So what's your suggestion here, that we could avoid similar issue > for virtio drivers in ARM DomU? > > Thanks, > Peng. > > > > > > > > > > > > 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. > > > > > > > > > [1] > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore > > > .kernel.org%2Fpatchwork%2Fpatch%2F1033801%2F%231222404&dat > > a=02%7C0 > > > > > 1%7Cpeng.fan%40nxp.com%7C08ba48d3b3d54e775a8108d819e62fd0%7C68 > > 6ea1d3bc > > > > > 2b4c6fa92cd99c5c301635%7C0%7C0%7C637287823721994475&sdata > > =Cw4FHWrH > > > uVKBCn3%2BKS2VM7cWuGoTI6R7SHJrJSLY5Io%3D&reserved=0
> Subject: Re: [PATCH] xen: introduce xen_vring_use_dma > > On Mon, Jun 29, 2020 at 03:05:19AM +0000, 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 <peng.fan@nxp.com> > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > 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 problem. > > > > > > > > > > Why is it a problem? It just makes virtio use DMA API. > > > > > If that in turn works, problem solved. > > > > > > > > You are correct in the sense that it would work. However I do > > > > think it is wrong for vring_use_dma_api to enable > > > > dma_ops/swiotlb-xen for Xen/ARM DomUs that don't need it. There > > > > are many different types of Xen guests, Xen x86 is drastically > > > > different from Xen ARM, it seems wrong to treat them the same way. > > > > > > I could imagine some future Xen hosts setting a flag somewhere in > > > the platform capability saying "no xen specific flag, rely on > > > "VIRTIO_F_ACCESS_PLATFORM". Then you set that accordingly in QEMU. > > > How about that? > > > > > > > Michael, Stefano, > > > > So what's your suggestion here, that we could avoid similar issue for > > virtio drivers in ARM DomU? > > > > Thanks, > > Peng. > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > [1] > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F > > > > lore > > > > .kernel.org%2Fpatchwork%2Fpatch%2F1033801%2F%231222404& > dat > > > a=02%7C0 > > > > > > > > 1%7Cpeng.fan%40nxp.com%7C08ba48d3b3d54e775a8108d819e62fd0%7C68 > > > 6ea1d3bc > > > > > > > > 2b4c6fa92cd99c5c301635%7C0%7C0%7C637287823721994475&sdata > > > =Cw4FHWrH > > > > uVKBCn3%2BKS2VM7cWuGoTI6R7SHJrJSLY5Io%3D&reserved=0
On Mon, Jun 29, 2020 at 06:25:41AM +0000, 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.
> Subject: Re: [PATCH] xen: introduce xen_vring_use_dma > > On Mon, Jun 29, 2020 at 06:25:41AM +0000, 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
On Fri, 26 Jun 2020, Michael S. Tsirkin wrote: > 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 <peng.fan@nxp.com> > > > > > > > > > > > > > > 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). Ok thanks. I understand and it makes sense to me. > > > > > > 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 problem. > > > > > > Why is it a problem? It just makes virtio use DMA API. > > > If that in turn works, problem solved. > > > > You are correct in the sense that it would work. However I do think it > > is wrong for vring_use_dma_api to enable dma_ops/swiotlb-xen for Xen/ARM > > DomUs that don't need it. There are many different types of Xen guests, > > Xen x86 is drastically different from Xen ARM, it seems wrong to treat > > them the same way. > > I could imagine some future Xen hosts setting a flag somewhere in the > platform capability saying "no xen specific flag, rely on > "VIRTIO_F_ACCESS_PLATFORM". Then you set that accordingly in QEMU. > How about that? Yes, that would be fine and there is no problem implementing something like that when we get virtio support in Xen. Today there are still no virtio interfaces provided by Xen to ARM guests (no virtio-block/net, etc.) In fact, in both cases we are discussing virtio is *not* provided by Xen; it is a firmware interface to something entirely different: 1) virtio is used to talk to a remote AMP processor (RPMesg) 2) virtio is used to talk to a secure-world firmware/OS (Trusty) VIRTIO_F_ACCESS_PLATFORM is not set by Xen in these cases but by RPMesg and by Trusty respectively. I don't know if Trusty should or should not set VIRTIO_F_ACCESS_PLATFORM, but I think Linux should still work without issues. The xen_domain() check in Linux makes it so that vring_use_dma_api returns the opposite value on native Linux compared to Linux as Xen/ARM DomU by "accident". By "accident" because there is no architectural reason why Linux Xen/ARM DomU should behave differently compared to native Linux in this regard. I hope that now it is clearer why I think the if (xen_domain()) check needs to be improved anyway, even if we fix generic dma_ops with virtio interfaces missing VIRTIO_F_ACCESS_PLATFORM.
On Mon, 29 Jun 2020, Peng Fan wrote: > > > 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. Is the stack trace above for the RPMesg issue or for the Trusty issue? If it is the stack trace for RPMesg, can you also post the Trusty stack trace? Or are they indentical?
> Subject: RE: [PATCH] xen: introduce xen_vring_use_dma > > On Mon, 29 Jun 2020, Peng Fan wrote: > > > > 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. > > Is the stack trace above for the RPMesg issue or for the Trusty issue? The stack trace you pasted is rpmsg issue. > If it is the stack trace for RPMesg, can you also post the Trusty stack trace? Or > are they indentical? There is no stack dump here. It successfully using swiotlb to do a map, but we actually no need swiotlb in domu to do the map. Thanks, Peng.
On Mon, Jun 29, 2020 at 04:46:09PM -0700, Stefano Stabellini wrote: > > I could imagine some future Xen hosts setting a flag somewhere in the > > platform capability saying "no xen specific flag, rely on > > "VIRTIO_F_ACCESS_PLATFORM". Then you set that accordingly in QEMU. > > How about that? > > Yes, that would be fine and there is no problem implementing something > like that when we get virtio support in Xen. Today there are still no > virtio interfaces provided by Xen to ARM guests (no virtio-block/net, > etc.) > > In fact, in both cases we are discussing virtio is *not* provided by > Xen; it is a firmware interface to something entirely different: > > 1) virtio is used to talk to a remote AMP processor (RPMesg) > 2) virtio is used to talk to a secure-world firmware/OS (Trusty) > > VIRTIO_F_ACCESS_PLATFORM is not set by Xen in these cases but by RPMesg > and by Trusty respectively. I don't know if Trusty should or should not > set VIRTIO_F_ACCESS_PLATFORM, but I think Linux should still work > without issues. > Any virtio implementation that is not in control of the memory map (aka not the hypervisor) absolutely must set VIRTIO_F_ACCESS_PLATFORM, else it is completely broken. > The xen_domain() check in Linux makes it so that vring_use_dma_api > returns the opposite value on native Linux compared to Linux as Xen/ARM > DomU by "accident". By "accident" because there is no architectural > reason why Linux Xen/ARM DomU should behave differently compared to > native Linux in this regard. > > I hope that now it is clearer why I think the if (xen_domain()) check > needs to be improved anyway, even if we fix generic dma_ops with virtio > interfaces missing VIRTIO_F_ACCESS_PLATFORM. IMHO that Xen quirk should never have been added in this form..
On Wed, 1 Jul 2020, Christoph Hellwig wrote: > On Mon, Jun 29, 2020 at 04:46:09PM -0700, Stefano Stabellini wrote: > > > I could imagine some future Xen hosts setting a flag somewhere in the > > > platform capability saying "no xen specific flag, rely on > > > "VIRTIO_F_ACCESS_PLATFORM". Then you set that accordingly in QEMU. > > > How about that? > > > > Yes, that would be fine and there is no problem implementing something > > like that when we get virtio support in Xen. Today there are still no > > virtio interfaces provided by Xen to ARM guests (no virtio-block/net, > > etc.) > > > > In fact, in both cases we are discussing virtio is *not* provided by > > Xen; it is a firmware interface to something entirely different: > > > > 1) virtio is used to talk to a remote AMP processor (RPMesg) > > 2) virtio is used to talk to a secure-world firmware/OS (Trusty) > > > > VIRTIO_F_ACCESS_PLATFORM is not set by Xen in these cases but by RPMesg > > and by Trusty respectively. I don't know if Trusty should or should not > > set VIRTIO_F_ACCESS_PLATFORM, but I think Linux should still work > > without issues. > > > > Any virtio implementation that is not in control of the memory map > (aka not the hypervisor) absolutely must set VIRTIO_F_ACCESS_PLATFORM, > else it is completely broken. Lots of broken virtio implementations out there it would seem :-( > > The xen_domain() check in Linux makes it so that vring_use_dma_api > > returns the opposite value on native Linux compared to Linux as Xen/ARM > > DomU by "accident". By "accident" because there is no architectural > > reason why Linux Xen/ARM DomU should behave differently compared to > > native Linux in this regard. > > > > I hope that now it is clearer why I think the if (xen_domain()) check > > needs to be improved anyway, even if we fix generic dma_ops with virtio > > interfaces missing VIRTIO_F_ACCESS_PLATFORM. > > IMHO that Xen quirk should never have been added in this form.. Would you be in favor of a more flexible check along the lines of the one proposed in the patch that started this thread: if (xen_vring_use_dma()) return true; xen_vring_use_dma would be implemented so that it returns true when xen_swiotlb is required and false otherwise.
On Wed, Jul 01, 2020 at 10:34:53AM -0700, Stefano Stabellini wrote: > On Wed, 1 Jul 2020, Christoph Hellwig wrote: > > On Mon, Jun 29, 2020 at 04:46:09PM -0700, Stefano Stabellini wrote: > > > > I could imagine some future Xen hosts setting a flag somewhere in the > > > > platform capability saying "no xen specific flag, rely on > > > > "VIRTIO_F_ACCESS_PLATFORM". Then you set that accordingly in QEMU. > > > > How about that? > > > > > > Yes, that would be fine and there is no problem implementing something > > > like that when we get virtio support in Xen. Today there are still no > > > virtio interfaces provided by Xen to ARM guests (no virtio-block/net, > > > etc.) > > > > > > In fact, in both cases we are discussing virtio is *not* provided by > > > Xen; it is a firmware interface to something entirely different: > > > > > > 1) virtio is used to talk to a remote AMP processor (RPMesg) > > > 2) virtio is used to talk to a secure-world firmware/OS (Trusty) > > > > > > VIRTIO_F_ACCESS_PLATFORM is not set by Xen in these cases but by RPMesg > > > and by Trusty respectively. I don't know if Trusty should or should not > > > set VIRTIO_F_ACCESS_PLATFORM, but I think Linux should still work > > > without issues. > > > > > > > Any virtio implementation that is not in control of the memory map > > (aka not the hypervisor) absolutely must set VIRTIO_F_ACCESS_PLATFORM, > > else it is completely broken. > > Lots of broken virtio implementations out there it would seem :-( Not really, most of virtio implementations are in full control of memory, being part of the hypervisor. > > > > The xen_domain() check in Linux makes it so that vring_use_dma_api > > > returns the opposite value on native Linux compared to Linux as Xen/ARM > > > DomU by "accident". By "accident" because there is no architectural > > > reason why Linux Xen/ARM DomU should behave differently compared to > > > native Linux in this regard. > > > > > > I hope that now it is clearer why I think the if (xen_domain()) check > > > needs to be improved anyway, even if we fix generic dma_ops with virtio > > > interfaces missing VIRTIO_F_ACCESS_PLATFORM. > > > > IMHO that Xen quirk should never have been added in this form.. > > Would you be in favor of a more flexible check along the lines of the > one proposed in the patch that started this thread: > > if (xen_vring_use_dma()) > return true; > > > xen_vring_use_dma would be implemented so that it returns true when > xen_swiotlb is required and false otherwise. I'll need to think about it. Sounds reasonable on the surface ...
On Wed, Jul 01, 2020 at 10:34:53AM -0700, Stefano Stabellini wrote: > Would you be in favor of a more flexible check along the lines of the > one proposed in the patch that started this thread: > > if (xen_vring_use_dma()) > return true; > > > xen_vring_use_dma would be implemented so that it returns true when > xen_swiotlb is required and false otherwise. Just to stress - with a patch like this virtio can *still* use DMA API if PLATFORM_ACCESS is set. So if DMA API is broken on some platforms as you seem to be saying, you guys should fix it before doing something like this..
Sorry for the late reply -- a couple of conferences kept me busy. On Wed, 1 Jul 2020, Michael S. Tsirkin wrote: > On Wed, Jul 01, 2020 at 10:34:53AM -0700, Stefano Stabellini wrote: > > Would you be in favor of a more flexible check along the lines of the > > one proposed in the patch that started this thread: > > > > if (xen_vring_use_dma()) > > return true; > > > > > > xen_vring_use_dma would be implemented so that it returns true when > > xen_swiotlb is required and false otherwise. > > Just to stress - with a patch like this virtio can *still* use DMA API > if PLATFORM_ACCESS is set. So if DMA API is broken on some platforms > as you seem to be saying, you guys should fix it before doing something > like this.. Yes, DMA API is broken with some interfaces (specifically: rpmesg and trusty), but for them PLATFORM_ACCESS is never set. That is why the errors weren't reported before. Xen special case aside, there is no problem under normal circumstances. If you are OK with this patch (after a little bit of clean-up), Peng, are you OK with sending an update or do you want me to?
On Fri, Jul 10, 2020 at 10:23:22AM -0700, Stefano Stabellini wrote: > Sorry for the late reply -- a couple of conferences kept me busy. > > > On Wed, 1 Jul 2020, Michael S. Tsirkin wrote: > > On Wed, Jul 01, 2020 at 10:34:53AM -0700, Stefano Stabellini wrote: > > > Would you be in favor of a more flexible check along the lines of the > > > one proposed in the patch that started this thread: > > > > > > if (xen_vring_use_dma()) > > > return true; > > > > > > > > > xen_vring_use_dma would be implemented so that it returns true when > > > xen_swiotlb is required and false otherwise. > > > > Just to stress - with a patch like this virtio can *still* use DMA API > > if PLATFORM_ACCESS is set. So if DMA API is broken on some platforms > > as you seem to be saying, you guys should fix it before doing something > > like this.. > > Yes, DMA API is broken with some interfaces (specifically: rpmesg and > trusty), but for them PLATFORM_ACCESS is never set. That is why the > errors weren't reported before. Xen special case aside, there is no > problem under normal circumstances. So why not fix DMA API? Then this patch is not needed. > > If you are OK with this patch (after a little bit of clean-up), Peng, > are you OK with sending an update or do you want me to?
> Subject: Re: [PATCH] xen: introduce xen_vring_use_dma > > Sorry for the late reply -- a couple of conferences kept me busy. > > > On Wed, 1 Jul 2020, Michael S. Tsirkin wrote: > > On Wed, Jul 01, 2020 at 10:34:53AM -0700, Stefano Stabellini wrote: > > > Would you be in favor of a more flexible check along the lines of > > > the one proposed in the patch that started this thread: > > > > > > if (xen_vring_use_dma()) > > > return true; > > > > > > > > > xen_vring_use_dma would be implemented so that it returns true when > > > xen_swiotlb is required and false otherwise. > > > > Just to stress - with a patch like this virtio can *still* use DMA API > > if PLATFORM_ACCESS is set. So if DMA API is broken on some platforms > > as you seem to be saying, you guys should fix it before doing > > something like this.. > > Yes, DMA API is broken with some interfaces (specifically: rpmesg and trusty), > but for them PLATFORM_ACCESS is never set. That is why the errors weren't > reported before. Xen special case aside, there is no problem under normal > circumstances. > > > If you are OK with this patch (after a little bit of clean-up), Peng, are you OK > with sending an update or do you want me to? If you could help, that would be great. You have more expertise in knowing the whole picture. Thanks, Peng.
On Sat, 11 Jul 2020, Michael S. Tsirkin wrote: > On Fri, Jul 10, 2020 at 10:23:22AM -0700, Stefano Stabellini wrote: > > Sorry for the late reply -- a couple of conferences kept me busy. > > > > > > On Wed, 1 Jul 2020, Michael S. Tsirkin wrote: > > > On Wed, Jul 01, 2020 at 10:34:53AM -0700, Stefano Stabellini wrote: > > > > Would you be in favor of a more flexible check along the lines of the > > > > one proposed in the patch that started this thread: > > > > > > > > if (xen_vring_use_dma()) > > > > return true; > > > > > > > > > > > > xen_vring_use_dma would be implemented so that it returns true when > > > > xen_swiotlb is required and false otherwise. > > > > > > Just to stress - with a patch like this virtio can *still* use DMA API > > > if PLATFORM_ACCESS is set. So if DMA API is broken on some platforms > > > as you seem to be saying, you guys should fix it before doing something > > > like this.. > > > > Yes, DMA API is broken with some interfaces (specifically: rpmesg and > > trusty), but for them PLATFORM_ACCESS is never set. That is why the > > errors weren't reported before. Xen special case aside, there is no > > problem under normal circumstances. > > So why not fix DMA API? Then this patch is not needed. It looks like the conversation is going in circles :-) I tried to explain the reason why, even if we fixed the DMA API to work with rpmesg and trusty, we still need this patch with the following email: https://marc.info/?l=linux-kernel&m=159347446709625&w=2 At that time it looked like you agreed that we needed to improve this check? (https://marc.info/?l=linux-kernel&m=159363662418250&w=2)
Hi Stefano, First off, apologies for bumping this dead thread. I came across this patch signed off by you recently https://github.com/Xilinx/linux-xlnx/commit/72cb5514953be3aa2ac00c57c9eaa100ecc67176 and was wondering if a patch replacing xen_domain() with xen_pv_domain() in vring_use_dma_api() can be sent upstream? If not, is there a reason why? Regards Amneesh
On Tue, 30 Jul 2024, Amneesh Singh wrote: > Hi Stefano, > > First off, apologies for bumping this dead thread. > > I came across this patch signed off by you recently > https://github.com/Xilinx/linux-xlnx/commit/72cb5514953be3aa2ac00c57c9eaa100ecc67176 > > and was wondering if a patch replacing xen_domain() with xen_pv_domain() in > vring_use_dma_api() can be sent upstream? If not, is there a reason why? Hi Amneesh, Yes this patch is still required for Xen on ARM and it fixes a real bug. Unfortunately the upstreaming process stalled so we had to keep the fix in our Xilinx Linux branch. Here is the link to the start of this conversation and to the last message: https://marc.info/?i=20200624091732.23944-1-peng.fan%40nxp.com https://marc.info/?i=alpine.DEB.2.21.2007151001140.4124%40sstabellini-ThinkPad-T480s Hi Michael, I see you are CCed to this email. Would you be open to acking this patch now that a 3rd independent engineer from a different company (NXP first, then AMD/Xilinx, now TI) has reported this as a bug fix?
diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c index d40e9e5fc52b..6a493ea087f0 100644 --- a/arch/arm/xen/mm.c +++ b/arch/arm/xen/mm.c @@ -139,6 +139,7 @@ static int __init xen_mm_init(void) struct gnttab_cache_flush cflush; if (!xen_initial_domain()) return 0; + xen_swiotlb = 1; xen_swiotlb_init(1, false); cflush.op = 0; diff --git a/arch/x86/include/asm/xen/swiotlb-xen.h b/arch/x86/include/asm/xen/swiotlb-xen.h index 6b56d0d45d15..bb5ce02b4e20 100644 --- a/arch/x86/include/asm/xen/swiotlb-xen.h +++ b/arch/x86/include/asm/xen/swiotlb-xen.h @@ -3,12 +3,10 @@ #define _ASM_X86_SWIOTLB_XEN_H #ifdef CONFIG_SWIOTLB_XEN -extern int xen_swiotlb; extern int __init pci_xen_swiotlb_detect(void); extern void __init pci_xen_swiotlb_init(void); extern int pci_xen_swiotlb_init_late(void); #else -#define xen_swiotlb (0) static inline int __init pci_xen_swiotlb_detect(void) { return 0; } static inline void __init pci_xen_swiotlb_init(void) { } static inline int pci_xen_swiotlb_init_late(void) { return -ENXIO; } diff --git a/arch/x86/xen/pci-swiotlb-xen.c b/arch/x86/xen/pci-swiotlb-xen.c index 33293ce01d8d..071fbe0e1a91 100644 --- a/arch/x86/xen/pci-swiotlb-xen.c +++ b/arch/x86/xen/pci-swiotlb-xen.c @@ -18,8 +18,6 @@ #endif #include <linux/export.h> -int xen_swiotlb __read_mostly; - /* * pci_xen_swiotlb_detect - set xen_swiotlb to 1 if necessary * diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index a2de775801af..768afd79f67a 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -252,7 +252,7 @@ static bool vring_use_dma_api(struct virtio_device *vdev) * the DMA API if we're a Xen guest, which at least allows * all of the sensible Xen configurations to work correctly. */ - if (xen_domain()) + if (xen_vring_use_dma()) return true; return false; diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index b6d27762c6f8..25747e72e6fe 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -40,6 +40,9 @@ #include <trace/events/swiotlb.h> #define MAX_DMA_BITS 32 + +int xen_swiotlb __read_mostly; + /* * Used to do a quick range check in swiotlb_tbl_unmap_single and * swiotlb_tbl_sync_single_*, to see if the memory was in fact allocated by this diff --git a/include/xen/swiotlb-xen.h b/include/xen/swiotlb-xen.h index ffc0d3902b71..235babcde848 100644 --- a/include/xen/swiotlb-xen.h +++ b/include/xen/swiotlb-xen.h @@ -12,4 +12,10 @@ void xen_dma_sync_for_device(dma_addr_t handle, phys_addr_t paddr, size_t size, extern int xen_swiotlb_init(int verbose, bool early); extern const struct dma_map_ops xen_swiotlb_dma_ops; +#ifdef CONFIG_SWIOTLB_XEN +extern int xen_swiotlb; +#else +#define xen_swiotlb (0) +#endif + #endif /* __LINUX_SWIOTLB_XEN_H */ diff --git a/include/xen/xen.h b/include/xen/xen.h index 19a72f591e2b..c51c46f5d739 100644 --- a/include/xen/xen.h +++ b/include/xen/xen.h @@ -52,4 +52,10 @@ bool xen_biovec_phys_mergeable(const struct bio_vec *vec1, extern u64 xen_saved_max_mem_size; #endif +#include <xen/swiotlb-xen.h> +static inline int xen_vring_use_dma(void) +{ + return !!xen_swiotlb; +} + #endif /* _XEN_XEN_H */
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 <peng.fan@nxp.com> --- V2: This is a modified version from Stefano's patch https://lore.kernel.org/patchwork/patch/1033801/#1222404 Note: This is not to address rpmsg virtio issue, this is to let DomU virtio not using xen swiotlb could use non dma vring on ARM64 platforms. arch/arm/xen/mm.c | 1 + arch/x86/include/asm/xen/swiotlb-xen.h | 2 -- arch/x86/xen/pci-swiotlb-xen.c | 2 -- drivers/virtio/virtio_ring.c | 2 +- drivers/xen/swiotlb-xen.c | 3 +++ include/xen/swiotlb-xen.h | 6 ++++++ include/xen/xen.h | 6 ++++++ 7 files changed, 17 insertions(+), 5 deletions(-)