Re: [PATCH v2 10/11] iommu/amd: Add IO page fault notifier handler

[Vasant Hegde <vasant.hegde@amd.com>]

Jason,


On 9/13/2023 12:16 AM, Jason Gunthorpe wrote:
> On Mon, Sep 11, 2023 at 12:10:45PM +0000, Vasant Hegde wrote:
> 
>> @@ -285,7 +286,7 @@ static struct iommu_dev_data *find_dev_data(struct amd_iommu *iommu, u16 devid)
>>  {
>>  	struct iommu_dev_data *dev_data;
>>  
>> -	dev_data = search_dev_data(iommu, devid);
>> +	dev_data = amd_iommu_search_dev_data(iommu, devid);
> 
>> +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_warn(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;
>> +	}
>> +
>> +	if (PPR_FLAGS(raw[0]) & PPR_FLAG_RVSD) {
>> +		dev_warn(dev, "PPR logged [Invalid request format (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;
>> +	}
> 
> Please be careful that no guest can trigger these warnings..

sure.

> 
>> +
>> +	return true;
>> +}
>> +
>> +static void iommu_call_iopf_notifier(struct amd_iommu *iommu, u64 *raw)
>> +{
>> +	struct iopf_fault event;
>> +	struct pci_dev *pdev;
>> +	int ret = -EINVAL;
>> +	u16 devid = PPR_DEVID(raw[0]);
>> +
>> +	if (PPR_REQ_TYPE(raw[0]) != PPR_REQ_FAULT) {
>> +		pr_err_ratelimited("Unknown PPR request received\n");
>> +		return;
>> +	}
>> +
>> +	if (!ppr_is_valid(iommu, raw))
>> +		goto out;
>> +
>> +	pdev = pci_get_domain_bus_and_slot(iommu->pci_seg->id, PCI_BUS_NUM(devid),
>> +					   devid & 0xff);
>> +	if (!pdev)
>> +		goto out;
> 
> Lu, here is another case where the core PRI code could make use of a
> core helper for a getting from the RID to the iommu world.
> 
>> +
>> +	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
>> +	 */
>> +	if (event.fault.prm.pasid == 0 ||
>> +	    event.fault.prm.pasid >= pdev->dev.iommu->max_pasids) {
>> +		pr_info_ratelimited("Invalid PASID : 0x%x, device : 0x%x\n",
>> +				    event.fault.prm.pasid, pdev->dev.id);
>> +		goto out;
>> +	}
>> +
>> +
>> +	event.fault.prm.flags |= IOMMU_FAULT_PAGE_RESPONSE_NEEDS_PASID;
>> +	event.fault.prm.flags |= IOMMU_FAULT_PAGE_REQUEST_PASID_VALID;
>> +	if (PPR_TAG(raw[0]) & 0x200)
>> +		event.fault.prm.flags |= IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE;
>> +
>> +	/* Submit event */
>> +	ret = iommu_report_device_fault(&pdev->dev, &event);
>> +
>> +out:
>> +	if (ret) {
>> +		/* Nobody cared, abort */
>> +		struct iommu_page_response resp = {
>> +			.pasid = PPR_PASID(raw[0]),
>> +			.grpid = PPR_TAG(raw[0]) & 0x1FF,
>> +			.code = IOMMU_PAGE_RESP_FAILURE,
>> +		};
>> +		amd_iommu_page_response(&pdev->dev, &event, &resp);
> 
> Just to call amd_iommu_complete_ppr(), this already has the pci_dev,
> we don't need amd_iommu_page_response() to get it.

Makes sense. Will fix it.

-Vasant