Re: [PATCH ats_vtd v5 20/22] pci: add a pci-level API for ATS

[Minwoo Im]

On 24-07-09 11:58:53, CLEMENT MATHIEU--DRIF wrote:
> 
> 
> On 09/07/2024 12:15, Minwoo Im wrote:
> > Caution: External email. Do not open attachments or click links, unless 
> > this email comes from a known sender and you know the content is safe.
> >
> >
> > On 24-07-02 05:52:45, CLEMENT MATHIEU--DRIF wrote:
> >> From: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
> >>
> >> Devices implementing ATS can send translation requests using
> >> pci_ats_request_translation_pasid.
> >>
> >> The invalidation events are sent back to the device using the iommu
> >> notifier managed with pci_register_iommu_tlb_event_notifier and
> >> pci_unregister_iommu_tlb_event_notifier
> >>
> >> Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
> >> ---
> >>   hw/pci/pci.c         | 44 +++++++++++++++++++++++++++++++++++++
> >>   include/hw/pci/pci.h | 52 ++++++++++++++++++++++++++++++++++++++++++++
> >>   2 files changed, 96 insertions(+)
> >>
> >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >> index 7a483dd05d..93b816aff2 100644
> >> --- a/hw/pci/pci.c
> >> +++ b/hw/pci/pci.c
> >> @@ -2833,6 +2833,50 @@ void pci_device_unset_iommu_device(PCIDevice *dev)
> >>       }
> >>   }
> >>
> >> +ssize_t pci_ats_request_translation_pasid(PCIDevice *dev, uint32_t pasid,
> >> +                                          bool priv_req, bool exec_req,
> >> +                                          hwaddr addr, size_t length,
> >> +                                          bool no_write, IOMMUTLBEntry 
> >> *result,
> >> +                                          size_t result_length,
> >> +                                          uint32_t *err_count)
> >> +{
> >> +    assert(result_length);
> >> +    IOMMUMemoryRegion *iommu_mr = 
> >> pci_device_iommu_memory_region_pasid(dev,
> >> +                                                                        
> >> pasid);
> >> +    if (!iommu_mr || !pcie_ats_enabled(dev)) {
> >> +        return -EPERM;
> >> +    }
> >> +    return memory_region_iommu_ats_request_translation(iommu_mr, priv_req,
> >> +                                                       exec_req, addr, 
> >> length,
> >> +                                                       no_write, result,
> >> +                                                       result_length,
> >> +                                                       err_count);
> >> +}
> > Can we use this function not from the endpoint PCI device, but inside of 
> > the pci
> > subsystem (hw/pci/pci.c) to make transparent abstraction for ATS request 
> > from
> > PCI endpoint device POV?  I guess it would be better to have PCI subsystem 
> > to
> > issue ATS request if pcie_ats_enabled(dev) rather than calling from the 
> > endpoint
> > side.
> Hi,
> 
> This series aims to bring support for SVM (we are trying to integrate 
> the patches bit by bit).
>  From a spec point of view, I don't know if it would make sense to 
> implement the SVM logic at the PCI level
> as it's supposed to be implemented by endpoint devices.

Understood that this series is targeting the SVM usage.  But ATS feature is
something general to PCI devices, not only just for SVM, so I guess it would be
better to have caller to `pci_ats_request_translation_pasid()` in pci subsystem
like pci_dma_rw() to avoid duplicated implementation in the future for the
other PCI enpoint devices.

> However, we could consider providing a reference/reusable/encapsulated 
> implementation of SVM with a simplified API
> that would call the pci_* functions under the hood.

I would prefer that PCI devices which want to request ATS translation has no
additional implementation for ATS, but only pcie_ats_init().

> 
> Do you have a specific use case in mind?

ATS/PRI is the actual use case, and it's not that different what you are
targeting for :)

> 
>  >cmd
> 
> >