Re: [PATCH v6 11/15] iommu/amd: Add IO page fault notifier handler

[Jason Gunthorpe <jgg@ziepe.ca>]

On Fri, Feb 09, 2024 at 11:29:26AM +0000, Vasant Hegde wrote:

> +static bool ppr_is_valid(struct amd_iommu *iommu, u64 *raw)
> +{
> +	struct device *dev = iommu->iommu.dev;
> +	u16 devid = PPR_DEVID(raw[0]);
> +
> +	if (!(PPR_FLAGS(raw[0]) & PPR_FLAG_GN)) {
> +		dev_dbg(dev, "PPR logged [Request ignored due to GN=0 (device=%04x:%02x:%02x.%x "
> +			"pasid=0x%05llx address=0x%llx flags=0x%04llx tag=0x%03llx]\n",
> +			iommu->pci_seg->id, PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
> +			PPR_PASID(raw[0]), raw[1], PPR_FLAGS(raw[0]), PPR_TAG(raw[0]));
> +		return false;

Someday this will not be an error..

> +static void iommu_call_iopf_notifier(struct amd_iommu *iommu, u64 *raw)
> +{
> +	struct iommu_dev_data *dev_data;
> +	struct iopf_fault event;
> +	struct pci_dev *pdev;
> +	u16 devid = PPR_DEVID(raw[0]);
> +
> +	if (PPR_REQ_TYPE(raw[0]) != PPR_REQ_FAULT) {
> +		pr_info_ratelimited("Unknown PPR request received\n");
> +		return;
> +	}
> +
> +	pdev = pci_get_domain_bus_and_slot(iommu->pci_seg->id,
> +					   PCI_BUS_NUM(devid), devid & 0xff);
> +	if (!pdev)
> +		return;
> +
> +	if (!ppr_is_valid(iommu, raw))
> +		goto out;
> +
> +	memset(&event, 0, sizeof(struct iopf_fault));
> +
> +	event.fault.type = IOMMU_FAULT_PAGE_REQ;
> +	event.fault.prm.perm = ppr_flag_to_fault_perm(PPR_FLAGS(raw[0]));
> +	event.fault.prm.addr = (u64)(raw[1] & PAGE_MASK);
> +	event.fault.prm.pasid = PPR_PASID(raw[0]);
> +	event.fault.prm.grpid = PPR_TAG(raw[0]) & 0x1FF;
> +
> +	/*
> +	 * PASID zero is used for requests from the I/O device without
> +	 * a PASID
> +	 */
> +	dev_data = dev_iommu_priv_get(&pdev->dev);
> +	if (event.fault.prm.pasid == 0 ||
> +	    event.fault.prm.pasid >= dev_data->max_pasids) {
> +		pr_info_ratelimited("Invalid PASID : 0x%x, device : 0x%x\n",
> +				    event.fault.prm.pasid, pdev->dev.id);
> +		goto out;
> +	}

Why even do this check? The core code is perfectly fine to take in a
big pasid value, it will not match anything in the xarray and just be
completed with error anyhow.

Looks fine otherwise

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason