Re: [PATCH 3/5] iommu/amd: Fix missing CMD_COMPLETE_PPR response for invalid PPR requests

On Mon, Jul 27, 2026 at 05:39:05AM +0000, Vasant Hegde wrote:
> The AMD IOMMU spec, requires the host to respond with a CMD_COMPLETE_PPR
> command when an EVENT_TYPE_INV_PPR_REQ event is received with the RX bit
> cleared. This response was missing in the current implementation, leaving
> invalid PPR requests unacknowledged.
> 
> Introduce amd_iommu_report_ppr_err() to handle EVENT_TYPE_INV_PPR_REQ
> events. The new function logs the invalid PPR request and when the RX
> bit is cleared, sends CMD_COMPLETE_PPR response.
> 
> Reported-by: Gaultier Delbarre <[email protected]>
> Co-developed-by: Wei Huang <[email protected]>
> Signed-off-by: Wei Huang <[email protected]>
> Signed-off-by: Vasant Hegde <[email protected]>
> ---
>  drivers/iommu/amd/amd_iommu_types.h |  1 +
>  drivers/iommu/amd/iommu.c           | 38 ++++++++++++++++++++++++-----
>  2 files changed, 33 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
> index a7aa9411b2e9..500952e752b4 100644
> --- a/drivers/iommu/amd/amd_iommu_types.h
> +++ b/drivers/iommu/amd/amd_iommu_types.h
> @@ -159,6 +159,7 @@
>  #define EVENT_FLAGS_SHIFT	0x10
>  #define EVENT_FLAG_RW		0x020
>  #define EVENT_FLAG_I		0x008
> +#define EVENT_FLAG_PPR_RX	0x001
>  
>  /* feature control bits */
>  #define CONTROL_IOMMU_EN	0
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 563f9c2672d5..4372d3908e67 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -905,10 +905,40 @@ static void amd_iommu_report_page_fault(struct amd_iommu *iommu,
>  		pci_dev_put(pdev);
>  }
>  
> +static void amd_iommu_report_ppr_err(struct amd_iommu *iommu, volatile u32 *event,
> +				     u16 devid, u64 address, int flags)
> +{
> +	struct pci_dev *pdev;
> +	struct device *dev = iommu->iommu.dev;
> +	u32 pasid = PPR_PASID(*((u64 *)event));
> +	int tag = event[1] & 0x03FF;
> +
> +	dev_err(dev, "Event logged [INVALID_PPR_REQUEST device=%04x:%02x:%02x.%x pasid=0x%05x address=0x%llx flags=0x%04x tag=0x%03x]\n",
> +		iommu->pci_seg->id, PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
> +		pasid, address, flags, tag);
> +
> +	/* Skip COMPLETE_PPR_REQUEST response if RX=1 */
> +	if (flags & EVENT_FLAG_PPR_RX)
> +		return;
> +
> +	pdev = pci_get_domain_bus_and_slot(iommu->pci_seg->id, PCI_BUS_NUM(devid),
> +					   devid & 0xff);
> +	if (!pdev)
> +		return;
> +
> +	if (!dev_iommu_priv_get(&pdev->dev)) {

Hi Vasant,
May be corener case, we return without sending COMPLETE_PPR. Since the 
goal of this patch is to always respond when RX=0, and the completion only
needs the devid (already available from the event) and iommu, should these
paths still issue the completion directly via 
build_complete_ppr()/iommu_queue_command() rather than just return?

-Ankit

> +		pci_dev_put(pdev);
> +		return;
> +	}
> +
> +	amd_iommu_complete_ppr(&pdev->dev, pasid, IOMMU_PAGE_RESP_FAILURE, tag);
> +	pci_dev_put(pdev);
> +}
> +
>  static void iommu_print_event(struct amd_iommu *iommu, void *__evt)
>  {
>  	struct device *dev = iommu->iommu.dev;
> -	int type, devid, flags, tag;
> +	int type, devid, flags;
>  	volatile u32 *event = __evt;
>  	int count = 0;
>  	u64 address, ctrl;
> @@ -982,11 +1012,7 @@ static void iommu_print_event(struct amd_iommu *iommu, void *__evt)
>  		amd_iommu_report_rmp_hw_error(iommu, event);
>  		break;
>  	case EVENT_TYPE_INV_PPR_REQ:
> -		pasid = PPR_PASID(*((u64 *)__evt));
> -		tag = event[1] & 0x03FF;
> -		dev_err(dev, "Event logged [INVALID_PPR_REQUEST device=%04x:%02x:%02x.%x pasid=0x%05x address=0x%llx flags=0x%04x tag=0x%03x]\n",
> -			iommu->pci_seg->id, PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
> -			pasid, address, flags, tag);
> +		amd_iommu_report_ppr_err(iommu, event, devid, address, flags);
>  		break;
>  	default:
>  		dev_err(dev, "Event logged [UNKNOWN event[0]=0x%08x event[1]=0x%08x event[2]=0x%08x event[3]=0x%08x\n",
> -- 
> 2.31.1
>