linux-kernel - RE: [PATCH 2/2] iommu/vt-d: Move iopf code from SVA to IOPF enabling path

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]

Message-ID: <BN9PR11MB527659435BA073B21A1291588CDA9@BN9PR11MB5276.namprd11.prod.outlook.com>
Date:   Mon, 6 Feb 2023 03:28:52 +0000
From:   "Tian, Kevin" <kevin.tian@...el.com>
To:     Lu Baolu <baolu.lu@...ux.intel.com>,
        "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
        "dmaengine@...r.kernel.org" <dmaengine@...r.kernel.org>
CC:     Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>,
        "Robin Murphy" <robin.murphy@....com>,
        "Yu, Fenghua" <fenghua.yu@...el.com>,
        "Jiang, Dave" <dave.jiang@...el.com>,
        Vinod Koul <vkoul@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 2/2] iommu/vt-d: Move iopf code from SVA to IOPF enabling
 path

> From: Lu Baolu <baolu.lu@...ux.intel.com>
> Sent: Friday, February 3, 2023 4:45 PM
> 
> Generally enabling IOMMU_DEV_FEAT_SVA requires
> IOMMU_DEV_FEAT_IOPF, but
> some devices manage I/O Page Faults themselves instead of relying on the
> IOMMU. Move IOPF related code from SVA to IOPF enabling path to make
> the
> driver work for devices that manage IOPF themselves.
> 
> For the device drivers that relies on the IOMMU for IOPF through PCI/PRI,
> IOMMU_DEV_FEAT_IOPF must be enabled before and disabled after
> IOMMU_DEV_FEAT_SVA.

ARM still handles this differently:

arm_smmu_master_enable_sva()
  arm_smmu_master_sva_enable_iopf():
{
	/*
	 * Drivers for devices supporting PRI or stall should enable IOPF first.
	 * Others have device-specific fault handlers and don't need IOPF.
	 */
	if (!arm_smmu_master_iopf_supported(master))
		return 0;

	if (!master->iopf_enabled)
		return -EINVAL;
}

i.e. device specific IOPF is allowed only when PRI or stall is not supported.

it's different from what this patch does to allow device specific IOPF even
when PRI is supported.

should we make them consistent given SVA/IOPF capabilities are general
iommu definitions or fine to leave each iommu driver with different
restriction?

> 
> -	ret = iopf_queue_add_device(iommu->iopf_queue, dev);
> -	if (!ret)
> -		ret = iommu_register_device_fault_handler(dev,
> iommu_queue_iopf, dev);
> -
> -	return ret;
> +	return 0;
>  }

here and below...

> +	ret = iopf_queue_add_device(info->iommu->iopf_queue, dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = iommu_register_device_fault_handler(dev, iommu_queue_iopf,
> dev);
> +	if (ret)
> +		iopf_queue_remove_device(info->iommu->iopf_queue, dev);
> +
> +	return ret;
>  }

...indicate a bug fix on error handling. better to have the fix as
a separate patch and then move code.