Re: [PATCH v4 2/5] vfio/spapr_tce: Normalize EEH IOA error injection addresses

[Sourabh Jain <sourabhjain@linux.ibm.com>]

On 31/08/26 12:24, Narayana Murty N wrote:
> VFIO_EEH_PE_INJECT_ERR receives a target address from userspace.  For
> userspace such as QEMU, this address may be derived from the host Linux
> BAR resource.  The platform EEH backends, however, expect the PCI/IOA
> bus address used by firmware error-injection interfaces.
>
> Normalize IOA/MMIO error-injection addresses in the sPAPR VFIO EEH
> ioctl path before dispatching to eeh_pe_inject_err().  If the supplied
> address is already a PCI bus BAR address it is left unchanged.  If it is
> a Linux resource address, translate it to the corresponding PCI bus
> address using the BAR-relative offset.
>
> Keep the helper local to VFIO so the address semantics of other
> in-kernel EEH callers are unchanged.  This provides common handling for
> both pseries and PowerNV backends.
>
> Signed-off-by: Narayana Murty N <nnmlinux@linux.ibm.com>
> ---
>   drivers/vfio/vfio_iommu_spapr_tce.c | 90 +++++++++++++++++++++++++++++
>   1 file changed, 90 insertions(+)
>
> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> index 1c0eec228cc6..8a6cc856d7da 100644
> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> @@ -774,6 +774,85 @@ static long tce_iommu_create_default_window(struct tce_container *container)
>   	return ret;
>   }
>   
> +static bool vfio_spapr_eeh_err_needs_addr_normalize(unsigned int type)
> +{
> +	if (type != EEH_ERR_TYPE_32 && type != EEH_ERR_TYPE_64)
> +		return false;
> +
> +	return true;
> +}

Do we really need this function? It is only used once.

> +
> +static int vfio_spapr_eeh_normalize_addr(struct eeh_pe *pe,
> +					 unsigned long addr,
> +					 unsigned long *normalized)
> +{
> +	struct pci_bus_region region;
> +	struct eeh_dev *edev, *tmp;
> +	struct pci_dev *pdev;
> +	struct resource *res;
> +	resource_size_t pci_start, pci_len;
> +	resource_size_t res_start, res_len;
> +	resource_size_t offset;
> +	int bar;
> +
> +	if (!pe || !normalized)
> +		return -EINVAL;
> +
> +	if (!addr) {
> +		*normalized = addr;
> +		return 0;
> +	}

This silently bypasses normalization for addr == 0. A comment on
why zero is exempted would be helpful.

> +
> +	eeh_pe_for_each_dev(pe, edev, tmp) {
> +		pdev = eeh_dev_to_pci_dev(edev);
> +		if (!pdev)
> +			continue;
> +
> +		for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> +			res = &pdev->resource[bar];

I'm not very familiar with this PCI resource regions, so quick question.
is pdev->resource[] limited to PCI_STD_NUM_BARS?

Also doesn't this need locking around the traversal? This dereferences
pdev->resource with no lock held, what stops hot-removal or BAR
reassignment racing this?

> +
> +			if (!resource_size(res))
> +				continue;
> +
> +			if (!(res->flags & (IORESOURCE_MEM | IORESOURCE_IO)))
> +				continue;
> +
> +			pcibios_resource_to_bus(pdev->bus, &region, res);
> +
> +			pci_start = region.start;
> +			pci_len = resource_size(res);
> +
> +			/*
> +			 * Case 1: userspace already supplied PCI/IOA
> +			 * bus address.
> +			 */
> +			if ((resource_size_t)addr >= pci_start &&
> +			    ((resource_size_t)addr - pci_start) < pci_len) {
> +				*normalized = addr;
> +				return 0;
> +			}
> +
> +			/*
> +			 * Case 2: userspace supplied Linux resource/CPU
> +			 * address Convert it back to PCI/IOA bus address
> +			 * before calling the platform EEH backend.
> +			 */
> +			res_start = res->start;
> +			res_len = resource_size(res);
> +
> +			if ((resource_size_t)addr >= res_start &&
> +			    ((resource_size_t)addr - res_start) < res_len) {
> +				offset = (resource_size_t)addr - res_start;
> +				*normalized = region.start + offset;
> +
> +				return 0;
> +			}
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +
>   static long vfio_spapr_ioctl_eeh_pe_op(struct iommu_group *group,
>   				       unsigned long arg)
>   {
> @@ -818,6 +897,17 @@ static long vfio_spapr_ioctl_eeh_pe_op(struct iommu_group *group,
>   		if (copy_from_user(&op, (void __user *)arg, minsz))
>   			return -EFAULT;
>   
> +		if (vfio_spapr_eeh_err_needs_addr_normalize(op.err.type)) {

At this point eeh_pe_inject_err is still using eeh_pe_inject_mmio_error, 
so why to normalize the address in this patch itself? Isn't this is a 
good candidate for 4/5 patch?
> +			unsigned long normalized;
> +			long ret;
> +
> +			ret = vfio_spapr_eeh_normalize_addr(pe, op.err.addr, &normalized);
> +			if (ret)
> +				return ret;

We should have error/debug log to tell eeh is not going through.

> +
> +			op.err.addr = normalized;
> +		}
> +
>   		return eeh_pe_inject_err(pe, op.err.type, op.err.func,
>   					 op.err.addr, op.err.mask);
>   	default: