On 24-07-10 05:17:42, CLEMENT MATHIEU--DRIF wrote:
>
>
> On 09/07/2024 23:17, 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-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--d...@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--d...@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.
>
> Would we store the ATC directly in the PCI subsytem?
Yes, endpoint device (e.g., svm.c) should call pci_* helpers in PCI subsystem
with `PCIDevice *pdev instance` which represents the endpoint device itself.
By the instance, we can look up the IOTLB entry from the ATC in the PCI
subsystem, not the current caller side.
> >
> >> 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().
> Hi,
>
> I think both strategies can coexist.
> Keeping control can be interesting for people who use Qemu for hardware
> prototyping and who generally want to experiment.
> We can keep the current PCI-level API for devices that want to
> reimplement the logic themselves
> and add a kind of "DMA module"/"ATS+PRI module" that works out of the box.
I think we should proivde hybrid mode on this. One for a `generic` cache
policy mode for every PCI endpoint devices which can be controlled in the PCI
subsystem for ATC, the other one is that device-specific cache policy mode
which let each device implement their own ATC lookup behaviors to optimize
their own caching impact.
> That module could be called "struct PciDmaModule" and expose a simple
> set of functions like pci_dma_module_init, pci_dma_module_read,
> pci_dma_module_write.
> I think it's important to keep existing DMA API as is to allow devices
> to do both "with ATS" and "without ATS" operations.
>
> Do you agree with that?
Indeed. Keeping the existing APIs is a good choice, but I would like to have
endpoint devices code much more simpler for the generic usages :)
> >
> >> 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
> >>
> >>>
