Re: [PATCH] intel-iommu: Quiesce devices before disabling IOMMU

[Takao Indoh <indou.takao@jp.fujitsu.com>]

(2013/09/08 20:47), Baoquan He wrote:
> Hi,
> Patch is great and works on my HP-z420.

Thank you for your test!

> There are several small concerns, please see inline comments.
> 
> On 08/21/13 at 04:15pm, Takao Indoh wrote:
>> This patch quiesces devices before disabling IOMMU on boot to stop
>> ongoing DMA. In intel_iommu_init(), check context entries and if there
>> is entry whose present bit is set then reset corresponding device.
>>
>> When IOMMU is already enabled on boot, it is disabled and new DMAR table
>> is created and then re-enabled in intel_iommu_init(). This causes DMAR
>> faults if there are in-flight DMAs.
>>
>> This causes problem on kdump. Devices are working in first kernel, and
>> after switching to second kernel and initializing IOMMU, many DMAR faults
>> occur and it causes problems like driver error or PCI SERR, at last
>> kdump fails. This patch fixes this problem.
>>
>> Signed-off-by: Takao Indoh <indou.takao@jp.fujitsu.com>
>>
>>
>> NOTE:
>> To reset devices this patch uses bus reset interface introduced by
>> following commits in PCI "next" branch.
>>
>> 64e8674fbe6bc848333a9b7e19f8cc019dde9eab
>> 5c32b35b004f5ef70dcf62bbc42b8bed1e50b471
>> 2e35afaefe64946caaecfacaf7fb568e46185e88
>> 608c388122c72e1bf11ba8113434eb3d0c40c32d
>> 77cb985ad4acbe66a92ead1bb826deffa47dd33f
>> 090a3c5322e900f468b3205b76d0837003ad57b2
>> a6cbaadea0af9b4aa6eee2882f2aa761ab91a4f8
>> de0c548c33429cc78fd47a3c190c6d00b0e4e441
>> 1b95ce8fc9c12fdb60047f2f9950f29e76e7c66d
>> ---
>>   drivers/iommu/intel-iommu.c |   55 ++++++++++++++++++++++++++++++++++++++++++-
>>   1 files changed, 54 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
>> index eec0d3e..efb98eb 100644
>> --- a/drivers/iommu/intel-iommu.c
>> +++ b/drivers/iommu/intel-iommu.c
>> @@ -3663,6 +3663,56 @@ static struct notifier_block device_nb = {
>>   	.notifier_call = device_notifier,
>>   };
>>   
>> +/* Reset PCI devices if its entry exists in DMAR table */
>> +static void __init iommu_reset_devices(struct intel_iommu *iommu, u16 segment)
>> +{
>> +	u64 addr;
>> +	struct root_entry *root;
>> +	struct context_entry *context;
>> +	int bus, devfn;
>> +	struct pci_dev *dev;
>> +
>> +	addr = dmar_readq(iommu->reg + DMAR_RTADDR_REG);
>> +	if (!addr)
>> +		return;
>> +
>> +	/*
>> +	 *  In the case of kdump, ioremap is needed because root-entry table
>> +	 *  exists in first kernel's memory area which is not mapped in second
>> +	 *  kernel
>> +	 */
>> +	root = (struct root_entry *)ioremap(addr, PAGE_SIZE);
>> +	if (!root)
>> +		return;
>> +
>> +	for (bus = 0; bus < ROOT_ENTRY_NR; bus++) {
>> +		if (!root_present(&root[bus]))
>> +			continue;
>> +
>> +		context = (struct context_entry *)ioremap(
>> +			root[bus].val & VTD_PAGE_MASK, PAGE_SIZE);
>> +		if (!context)
>> +			continue;
>> +
>> +		for (devfn = 0; devfn < ROOT_ENTRY_NR; devfn++) {
> For context_entry table, the context_entry has the same size as
> root_entry currently, so it's also correct to use ROOT_ENTRY_NR. But for
> future extention and removing confusion, can we define a new MACRO on
> calculating the size of context_entry table, e.g CONTEXT_ENTRY_NR?

That makes sense, will do in next version.


> 
>> +			if (!context_present(&context[devfn]))
>> +				continue;
>> +
>> +			dev = pci_get_domain_bus_and_slot(segment, bus, devfn);
>> +			if (!dev)
>> +				continue;
>> +
>> +			if (!pci_reset_bus(dev->bus)) /* go to next bus */
> 
> Here, we have got the specific dev, why don't we just call
> pci_reset_function? If call pci_reset_bus here, will it repeat resetting
> devices on the same bus many times?

pci_reset_bus() can reset all devices on the same bus at the same time.
I think it is better than calling pci_reset_function() many times for
each device/function.

When pci_reset_bus() returns 0 (reset succeeded), we can immediately go
out of devfn loop by "break" and go to next bus loop.

> 
>> +				break;
>> +			else /* Try per-function reset */
>> +				pci_reset_function(dev);
>> +
>> +		}
>> +		iounmap(context);
>> +	}
>> +	iounmap(root);
>> +}
>> +
>>   int __init intel_iommu_init(void)
>>   {
>>   	int ret = 0;
>> @@ -3687,8 +3737,11 @@ int __init intel_iommu_init(void)
>>   			continue;
>>   
>>   		iommu = drhd->iommu;
>> -		if (iommu->gcmd & DMA_GCMD_TE)
>> +		if (iommu->gcmd & DMA_GCMD_TE) {
>> +			if (reset_devices)
>> +				iommu_reset_devices(iommu, drhd->segment);
> 
> I remember the double reset issue vivek concerned in the old patch. Here
> the iommu reset is done at the very beginning of pci_iommu_init, it's
> after the bus subsystem init, it means here only iommu reset, am I
> right?

Yes, exactly.

> 
> If yes, I think this patch is clear and logic is simple.
> 
> BTW, what's the status of Alex's PCI patchset which this patch depends
> on?

It is merged to Bjorn's tree, will be in v3.12.

Thanks,
Takao Indoh


> 
> Baoquan
> Thanks
> 
>>   			iommu_disable_translation(iommu);
>> +		}
>>   	}
>>   
>>   	if (dmar_dev_scope_init() < 0) {
>> -- 
>> 1.7.1
>>
>>
>>
>> _______________________________________________
>> kexec mailing list
>> kexec@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/kexec
> 
> 



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec