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] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 9 Sep 2022 08:33:02 +0000
From:   "Tian, Kevin" <kevin.tian@...el.com>
To:     Lu Baolu <baolu.lu@...ux.intel.com>,
        "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>
CC:     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

> 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.

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...

> -		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?