On Thu, 2012-09-27 at 17:50 -0400, Linda Knippers wrote: > Alex Williamson wrote: > > On Thu, 2012-09-27 at 21:10 +0000, Mingarelli, Thomas wrote: > >> Alex, > >> > >> Are you suggesting that a solution is to prevent devices with RMRRs > >> from being placed in the SI Domain in the first place (when pt mode is > >> used)? > > > > No, it seems like it's preferable that devices with RMRRs stay in the si > > domain if the device supports 64bit DMA. They're likely to cause less > > problems there and we can ignore the RMRRs in the si domain. The > > problem I'm trying to address is that the domain you're setting up with > > RMRRs is only used for dma_ops (ie. host driver use). If the device is > > attached to a guest, that domain gets discarded and the device is added > > to yet another domain, potentially with other devices. That domain is > > missing RMRRs, so now your device is going to generate VT-d faults. The > > device can then get removed from that domain, in which case the RMRRs > > should be removed, and again a new dma_ops domain needs to get created > > with RMRRs. > > > > It really seems like RMRRs are incompatible with IOMMU API use though. > > If an RMRR is setup for a VM domain, that's bad because a) it gives the > > VM direct access to that range of host memory, and b) it interferes with > > the guest use of the address space. a) is also bad for isolating > > devices on the host, but the spec makes it available for abuse. For b), > > it's not hard to imagine an RMRR range on the host that overlaps with > > DMA'able space on the guest. Data is read or written to the host memory > > instead of the guest memory. So maybe the right answer is to make > > intel_iommu_attach_device return error if requested to act on a device > > with RMRR ranges. > > That sounds like the right answer to me. Tom, can you make that > change when you address the rest of the comments? Or should it > be a separate patch?
Please include a pr_info when intel_iommu_attach_device takes this path or we're going to have a really hard time figuring out why a device won't attach to a guest. Thanks, Alex > >> -----Original Message----- > >> From: Alex Williamson [mailto:[email protected]] > >> Sent: Thursday, September 27, 2012 3:37 PM > >> To: Mingarelli, Thomas > >> Cc: [email protected]; Knippers, Linda; Khan, Shuah; Don > >> Dutile; David Woodhouse > >> Subject: Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info > >> > >> [adding David Woodhouse] > >> > >> On Tue, 2012-09-18 at 16:49 +0000, Tom Mingarelli wrote: > >>> When a 32bit PCI device is removed from the SI Domain, the RMRR > >>> information > >>> for this device becomes invalid and needs to be reprocessed to avoid DMA > >>> Read errors. These errors are evidenced by the Present bit being cleared > >>> in > >>> the device's context entry. Fixing this problem with an enhancement to > >>> process > >>> the RMRR info when the device is assigned to another domain. The Bus > >>> Master bit > >>> is cleared during the move to another domain and during the reprocessing > >>> of > >>> the RMRR info so no DMA can take place at this time. > >>> ---- > >>> PATCH v1: https://lkml.org/lkml/2012/6/15/204 > >>> > >>> drivers/iommu/intel-iommu.c | 47 > >>> ++++++++++++++++++++++++++++++++++++++++-- > >>> 1 files changed, 44 insertions(+), 3 deletions(-) > >>> > >>> Signed-off-by: Thomas Mingarelli <[email protected]> > >>> > >>> diff -up ./drivers/iommu/intel-iommu.c.ORIG ./drivers/iommu/intel-iommu.c > >>> --- ./drivers/iommu/intel-iommu.c.ORIG 2012-09-18 09:58:25.147976889 > >>> -0500 > >>> +++ ./drivers/iommu/intel-iommu.c 2012-09-18 10:39:43.286672765 -0500 > >>> @@ -2706,11 +2706,39 @@ static int iommu_dummy(struct pci_dev *p > >>> return pdev->dev.archdata.iommu == DUMMY_DEVICE_DOMAIN_INFO; > >>> } > >>> > >>> +static int reprocess_rmrr(struct device *dev) > >>> +{ > >>> + struct dmar_rmrr_unit *rmrr; > >>> + struct pci_dev *pdev; > >>> + int i, ret; > >>> + > >>> + pdev = to_pci_dev(dev); > >>> + > >>> + for_each_rmrr_units(rmrr) { > >>> + for (i = 0; i < rmrr->devices_cnt; i++) { > >>> + /* > >>> + * Here we are just concerned with > >>> + * finding the one device that was > >>> + * removed from the si_domain and > >>> + * re-evaluating its RMRR info. > >>> + */ > >> I'm still not sure why this comment is wrapped so tightly. > >> > >>> + if (rmrr->devices[i] != pdev) > >>> + continue; > >>> + pr_info("IOMMU: Reprocess RMRR information for device > >>> %s.\n", > >>> + pci_name(pdev)); > >>> + ret = iommu_prepare_rmrr_dev(rmrr, pdev); > >>> + if (ret) > >>> + pr_err("IOMMU: Reprocessing RMRR reserved > >>> region for device failed"); > >> This could be "if (iommu_prepare_rmrr...)" because... > >> > >>> + } > >>> + } > >>> +return 0; > >> Why does return anything? Looks like it could be a void function since > >> you're not returning the only possible error case above and not checking > >> the return value below. You're missing an indent here anyway. > >> > >>> +} > >>> + > >>> /* Check if the pdev needs to go through non-identity map and unmap > >>> process.*/ > >>> static int iommu_no_mapping(struct device *dev) > >>> { > >>> struct pci_dev *pdev; > >>> - int found; > >>> + int found, current_bus_master; > >>> > >>> if (unlikely(dev->bus != &pci_bus_type)) > >>> return 1; > >>> @@ -2731,9 +2759,22 @@ static int iommu_no_mapping(struct devic > >>> * 32 bit DMA is removed from si_domain and fall back > >>> * to non-identity mapping. > >>> */ > >>> - domain_remove_one_dev_info(si_domain, pdev); > >>> printk(KERN_INFO "32bit %s uses non-identity mapping\n", > >>> - pci_name(pdev)); > >>> + pci_name(pdev)); > >> White space damage? Change this to a pr_info if you really want to > >> touch it. > >> > >>> + /* > >>> + * If a device gets this far we need to clear the Bus > >>> + * Master bit before we start moving devices from domain > >>> + * to domain. We will also reset the Bus Master bit > >>> + * after reprocessing the RMRR info. However, we only > >>> + * do both the clearing and setting if needed. > >>> + */ > >>> + current_bus_master = pdev->is_busmaster; > >>> + if (current_bus_master) > >>> + pci_clear_master(pdev); > >>> + domain_remove_one_dev_info(si_domain, pdev); > >>> + reprocess_rmrr(dev); > >>> + if (current_bus_master) > >>> + pci_set_master(pdev); > >> I don't know any better way to halt DMA since we can't move the device > >> to a new domain atomically, but what about the other cases where we > >> switch domains? For instance, what if some unsuspecting user tries to > >> assign the device to a guest? I think it's generally inadvisable to > >> assign a devices with RMRRs to a guest, but if they do, they're going to > >> run into the same problem. The RMRRs aren't reprocessed for the VM > >> domain and again aren't reprocessed when returned to a standard device > >> domain. Even more fun, if assigned to a VM domain, RMRRs should be > >> added with the device and removed if the device is later detached from > >> the domain. > >> > >> The lazy solution might be to disallow devices with RMRRs from being > >> attached via the IOMMU API interfaces (do we need more reasons not to > >> use RMRRs?). Otherwise we need to be proactive about setting up a new > >> domain with RMRR entries for every case and correctly tracking RMRRs for > >> VM domains. Thanks, > >> > >> Alex > >> > >>> return 0; > >>> } > >>> } else { > >> > >> > > > > > > > _______________________________________________ iommu mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/iommu
