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

[Narayana Murty N <nnmlinux@linux.ibm.com>]

Hi Sourabh,

On 23/09/26 5:10 PM, Sourabh Jain wrote:
> 
> 
> 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.
Good point.

In v5 I made the no-address case explicit as addr == 0 && mask == 0, 
rather than bypassing normalization based on the address alone.

I also added a comment explaining that this represents an injection with 
no specific IOA target and therefore does not require BAR translation.
> 
>> +
>> +    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?
pdev->resource[] is not limited to the standard BARs. The lookup 
intentionally searches only BAR0..BAR5 using PCI_STD_NUM_BARS; ROM and 
bridge resources are not considered IOA bus-error targets.

For the lifetime issue, agreed. In v5 I hold pci_rescan_remove_lock 
while walking the PE devices and accessing their BAR resources so device 
removal cannot race the traversal.
> 
>> +
>> +            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?
Agreed.

I reordered the series in v5 so that the pSeries RTAS error-injection 
implementation is added before the VFIO address-normalization patch.

The normalization patch is now 4/5, after the pSeries backend has 
switched from the legacy MMIO injection path to RTAS.

>> +            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.
> 
Agreed. I added a pr_debug() in v5 when the supplied address cannot be 
matched against any PCI bus BAR of the PE.

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