Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info

[Linda Knippers <linda.knippers-VXdhtT5mjnY@public.gmane.org>]

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?

-- ljk

> Thanks,
> 
> Alex
> 
>> -----Original Message-----
>> From: Alex Williamson [mailto:alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org] 
>> Sent: Thursday, September 27, 2012 3:37 PM
>> To: Mingarelli, Thomas
>> Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; 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 <thomas.mingarelli-VXdhtT5mjnY@public.gmane.org>
>>>
>>> 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 {
>>
>>
> 
> 
>