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?
>
>> 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.
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?
>
>> 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
>>
>>>
