linux-kernel - Re: [PATCH 1/1] iommu/vt-d: Make SVA and IOPF irrelevant

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <5a11d688-9245-9041-5183-3eb87914eb1f@linux.intel.com>
Date:   Mon, 12 Sep 2022 15:01:47 +0800
From:   Baolu Lu <baolu.lu@...ux.intel.com>
To:     "Tian, Kevin" <kevin.tian@...el.com>,
        "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>
Cc:     baolu.lu@...ux.intel.com, Joerg Roedel <joro@...tes.org>,
        Will Deacon <will@...nel.org>,
        Robin Murphy <robin.murphy@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/1] iommu/vt-d: Make SVA and IOPF irrelevant

On 2022/9/9 16:33, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@...ux.intel.com>
>> Sent: Thursday, September 8, 2022 10:36 AM
>>
>> The existing IOPF handling code relies on the per-PASID SVA data
>> structures. It does not apply to scenarios other than SVA. This
>> decouples the I/O page fault reporting and responding code from
>> SVA related data structures so that the PRQ handling code could
>> become generic.
> 
> I think the point is that it's unnecessary to access those SVA data
> in the fault path. otherwise 'decouple' reads like an alternative
> implementation is added instead of just removing the code.

Makes sense. I will rephrase the commit message like this:

iommu/vt-d: Remove unnecessary SVA data accesses in page fault path

The existing I/O page fault handling code accesses the per-PASID SVA data
structures. This is unnecessary and makes the fault handling code only
suitable for SVA scenarios. This removes the SVA data accesses from the
I/O page fault reporting and responding code, so that the fault handling
code could be generic.

> 
> Overall this is a nice cleanup, but a few nits here:
> 
>> -		/*
>> -		 * If prq is to be handled outside iommu driver via receiver of
>> -		 * the fault notifiers, we skip the page response here.
>> -		 */
> 
> I didn't understand what this comment is trying to say. But just want
> to confirm removing it is the desired thing given the original code below
> it is still kept below...

I carelessly removed this comment. Yes. It still makes sense. I will add
it back.

> 
>> -		if (intel_svm_prq_report(iommu, sdev->dev, req))
>> +		pdev = pci_get_domain_bus_and_slot(iommu->segment,
>> +						   PCI_BUS_NUM(req->rid),
>> +						   req->rid & 0xff);
>> +		if (!pdev || intel_svm_prq_report(iommu, &pdev->dev, req))
>>   			handle_bad_prq_event(iommu, req,
>> QI_RESP_INVALID);
>>
>> -		trace_prq_report(iommu, sdev->dev, req->qw_0, req->qw_1,
>> +		trace_prq_report(iommu, &pdev->dev, req->qw_0, req-
>>> qw_1,
>>   				 req->priv_data[0], req->priv_data[1],
>> -				 sdev->prq_seq_number);
>> +				 prq_seq_number++);
> 
> Previously this is counted per device but now becomes global. Could it
> be stored elsewhere in a per-device structure?

I think the per-IOMMU structure might be the right place to store it.
Those faults share a page fault queue.

Best regards,
baolu