Re: [PATCH v3 1/1] PCI: Add translated request only flag for pci_enable_pasid()

[Jason Gunthorpe]

On Tue, Jan 31, 2023 at 05:50:52PM -0600, Bjorn Helgaas wrote:
> On Mon, Jan 30, 2023 at 02:47:32PM -0400, Jason Gunthorpe wrote:
> > On Mon, Jan 30, 2023 at 12:38:10PM -0600, Bjorn Helgaas wrote:
> > 
> > > Sorry, I'm still confused.  PCI_PASID_XLATED_REQ_ONLY is a
> > > device-specific property, and you want to opt-in AMD graphics devices.
> > > Where's the AMD graphics-specific change?  The current patch does
> > > this:
> > > 
> > >   pdev_pri_ats_enable
> > >     pci_enable_pasid(pdev, 0, PCI_PASID_XLATED_REQ_ONLY)
> > > 
> > > which looks like it does it for *all* devices below an AMD IOMMU,
> > > without any device or driver input.
> > 
> > AMD GPU has a private interface to AMD IOMMU to support PASID support
> > that only it uses.
> 
> What is it that makes this a private interface? 

The symbol names start with "amd"

drivers/iommu/amd/init.c:EXPORT_SYMBOL(amd_iommu_snp_en);
drivers/iommu/amd/init.c:EXPORT_SYMBOL(amd_iommu_v2_supported);
drivers/iommu/amd/init.c:EXPORT_SYMBOL(amd_iommu_pc_get_max_banks);
drivers/iommu/amd/init.c:EXPORT_SYMBOL(amd_iommu_pc_supported);
drivers/iommu/amd/init.c:EXPORT_SYMBOL(amd_iommu_pc_get_max_counters);
drivers/iommu/amd/iommu.c:EXPORT_SYMBOL(amd_iommu_register_ga_log_notifier);
drivers/iommu/amd/iommu.c:EXPORT_SYMBOL_GPL(amd_iommu_is_attach_deferred);
drivers/iommu/amd/iommu.c:EXPORT_SYMBOL(amd_iommu_register_ppr_notifier);
drivers/iommu/amd/iommu.c:EXPORT_SYMBOL(amd_iommu_unregister_ppr_notifier);
drivers/iommu/amd/iommu.c:EXPORT_SYMBOL(amd_iommu_domain_direct_map);
drivers/iommu/amd/iommu.c:EXPORT_SYMBOL(amd_iommu_domain_enable_v2);
drivers/iommu/amd/iommu.c:EXPORT_SYMBOL(amd_iommu_flush_page);
drivers/iommu/amd/iommu.c:EXPORT_SYMBOL(amd_iommu_flush_tlb);
drivers/iommu/amd/iommu.c:EXPORT_SYMBOL(amd_iommu_domain_set_gcr3);
drivers/iommu/amd/iommu.c:EXPORT_SYMBOL(amd_iommu_domain_clear_gcr3);
drivers/iommu/amd/iommu.c:EXPORT_SYMBOL(amd_iommu_complete_ppr);
drivers/iommu/amd/iommu.c:EXPORT_SYMBOL(amd_iommu_device_info);
drivers/iommu/amd/iommu.c:EXPORT_SYMBOL(amd_iommu_activate_guest_mode);
drivers/iommu/amd/iommu.c:EXPORT_SYMBOL(amd_iommu_deactivate_guest_mode);
drivers/iommu/amd/iommu.c:EXPORT_SYMBOL(amd_iommu_update_ga);
drivers/iommu/amd/iommu_v2.c:EXPORT_SYMBOL(amd_iommu_bind_pasid);
drivers/iommu/amd/iommu_v2.c:EXPORT_SYMBOL(amd_iommu_unbind_pasid);
drivers/iommu/amd/iommu_v2.c:EXPORT_SYMBOL(amd_iommu_init_device);
drivers/iommu/amd/iommu_v2.c:EXPORT_SYMBOL(amd_iommu_free_device);
drivers/iommu/amd/iommu_v2.c:EXPORT_SYMBOL(amd_iommu_set_invalid_ppr_cb);
drivers/iommu/amd/iommu_v2.c:EXPORT_SYMBOL(amd_iommu_set_invalidate_ctx_cb);

A driver should not be using EXPORT_SYMBOL at all, this is all
superseded by the core code that has now been created, but this 
has not been cleaned up.

So the troublesome PASID bit is here:

drivers/gpu/drm/amd/amdkfd/kfd_iommu.c: err = amd_iommu_bind_pasid(dev->adev->pdev, p->pasid, p->lead_thread);
drivers/gpu/drm/amd/amdkfd/kfd_iommu.c:         err = amd_iommu_bind_pasid(kfd->adev->pdev, p->pasid,

And the logic AMD iommu uses to call pci_enable_pasid() is in the
wrong place, it should be in drm/amd someplace not in the iommu
drivers.

This is all more stuff to fix

> But amd_iommu_domain_alloc() also leads to domain_enable_v2(), and
> that's pretty generic, so it looks like we set PD_IOMMUV2_MASK
> whenever the IOMMU supports it.

Yes, it is all sort of messy still.

AMD and ARM have a requirement that the RID page table format be in a
certain way to be able to enable the PASID decoded in the iommu

So the iommu drivers are trying to guess what page table format to use
based on the PCI caps, and wrongly turning on PASID mode at the same
time.

> I guess I'm trying to convince myself that no harm in enabling PASID
> for any device below an AMD v2 IOMMU.  But I don't think a device is
> *required* to use translated addresses with PASID, and if it uses
> untranslated addresses with PASID, don't we need ACS to avoid
> misrouting?

PASID enabling via config space doesn't actually do much - it is
attaching a PASID at the iommu and attempting to operate the device
with a PASID that is the key item.

So right now, the only thing in the kernel which can do that is amdkfd
because of the private interface. AMD says amdkfd HW always issues ATS
with a PASID and never a MemRd/Wr, which is why it works at all.

Jason