On 2021-04-13 10:53:39 Tue, Oliver O'Halloran wrote:
> On Mon, Apr 12, 2021 at 5:52 PM Mahesh Salgaonkar <mahesh@xxxxxxxxxxxxx> wrote:
> >
> > During the EEH MMIO error checking, the current implementation fails to map
> > the (virtual) MMIO address back to the pci device on radix with hugepage
> > mappings for I/O. This results into failure to dispatch EEH event with no
> > recovery even when EEH capability has been enabled on the device.
> >
> > eeh_check_failure(token) # token = virtual MMIO address
> > addr = eeh_token_to_phys(token);
> > edev = eeh_addr_cache_get_dev(addr);
> > if (!edev)
> > return 0;
> > eeh_dev_check_failure(edev); <= Dispatch the EEH event
> >
> > In case of hugepage mappings, eeh_token_to_phys() has a bug in virt -> phys
> > translation that results in wrong physical address, which is then passed to
> > eeh_addr_cache_get_dev() to match it against cached pci I/O address ranges
> > to get to a PCI device. Hence, it fails to find a match and the EEH event
> > never gets dispatched leaving the device in failed state.
> >
> > The commit 33439620680be ("powerpc/eeh: Handle hugepages in ioremap space")
> > introduced following logic to translate virt to phys for hugepage mappings:
> >
> > eeh_token_to_phys():
> > + pa = pte_pfn(*ptep);
> > +
> > + /* On radix we can do hugepage mappings for io, so handle that */
> > + if (hugepage_shift) {
> > + pa <<= hugepage_shift; <= This is wrong
> > + pa |= token & ((1ul << hugepage_shift) - 1);
> > + }
>
> I think I vaguely remember thinking "is this right?" at the time.
> Apparently not!
>
> Reviewed-by: Oliver O'Halloran <oohall@xxxxxxxxx>
>
Thanks for the review.
>
> It would probably be a good idea to add a debugfs interface to help
> with testing the address translation. Maybe something like:
>
> echo <mmio addr> > /sys/kernel/debug/powerpc/eeh_addr_check
>
> Then in the kernel:
>
> struct resource *r = lookup_resource(mmio_addr);
> void *virt = ioremap_resource(r);
> ret = eeh_check_failure(virt);
> iounmap(virt)
>
> return ret;
>
> A little tedious, but then you can write a selftest :)
Sure, will give a try.
Thanks,
-Mahesh.
--
Mahesh J Salgaonkar
