On 24/01/17 16:29, Marc Zyngier wrote: > On 24/01/17 16:14, Shameerali Kolothum Thodi wrote: >>>>>> Let's contemplate this for a moment. If we're on the affected ITS, >>>>> we're >>>>>> using the physical address of the GITS_TRANSLATER register. What >>>>>> guarantees that this is not going to conflict with an IOVA that DMA >>>>> is >>>>>> going to use? From looking at these patches, my feeling is "not >>>>> much". >>>>>> >>>>>> So if I'm right, you're opening the door to some interesting memory >>>>>> corruption if the two regions ever intersect. >>>>>> >>>>>> Robin, what do you think? >>>>> >>>>> Yup. Unless the ITS physical address is actually reserved from the >>>>> IOVA domain, it's still free to be allocated for DMA mappings, and >>> if >>>>> that ever happens then you'll get odd bits of data landing in the >>> ITS >>>>> instead of RAM, and maybe even locked-up devices or worse if the >>>>> doorbell gives back decode errors on read attempts. It's essentially >>>>> the exact same problem as we have with memory-mapped PCI windows, >>> and >>>>> needs to be solved in the same fashion, i.e. between the SMMU and >>> the >>>>> IOMMU-DMA code. >>>> >>>> Is this something that can incorporated in Eric's latest patch >>> series[1]? >>>> It does mentions reserved regions can be: >>>> - directly mapped regions >>>> - regions that cannot be iommu mapped (PCI host bridge windows, ...) >>>> - MSI regions (because they belong to another address space or >>> because >>>> they are not translated by the IOMMU and need special handling) >>>> >>>> Though I am not clear our case comes under "the MSI regions that are >>>> not translated by the IOMMU and need special handling" or not. >>> >>> Well, given that in your case, the IOMMU never sees the MSI write, it >>> definitely falls into the "not translated" category. >>> >>> As for handling it on top of Eric's series, that's probably the most >>> reasonable thing to do, which also means that none of this should >>> appear in the ITS driver. Robin seems to have an idea on how to >>> approach this. >> >> Ok. Thanks for that Marc/Robin. >> >> But I am not sure we can get away with ITS driver. Because current vfio >> patch series[1] treats GICV3 ITS as irq safe and is setting >> IRQ_DOMAIN_FLAG_MSI_REMAP in ITS driver. But this is not the case with >> our ITS. > > The ITS itself is perfectly safe, as it does perform device isolation > just fine (at least as far as I can tell from this bug description). > > There is two things we need to take care of: > - When the device is used on the host, the hardwired MSI region must be > excluded from the DMA IOVA allocator, and the iommu_dma_map_msi_msg() > call becomes a NOP. > - When the device is assigned to VFIO, the MSI region must be exposed to > userspace through /sys so that it knows that the guest RAM cannot alias > with this region (or face the corruption we've talked about above). > > None of that actually involves the ITS. Eric's stuff has some of the > initial infrastructure, but there is of course more to it. I'll let > Robin chime in and correct me if I've missed something (very likely). That's pretty much it. Once Eric's patches for the iommu_resv_regions interface have been merged, I'm planning to convert IOMMU-DMA over to using those instead of the internal PCI-specific hook it currently has. Then we would simply need the SMMU driver to expose this hardwired MSI region where necessary so that IOMMU-DMA can directly insert 1:1 iommu_dma_msi_page entries to cover it up-front in iommu_dma_init_domain(). Robin. > > Thanks, > > M. > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
