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

[CLEMENT MATHIEU--DRIF]

On 11/07/2024 10:04, 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-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--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.
>> 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 :)
That's a good point, we will se what we can do once the current work is 
integrated.
Thanks for your comment :)
>
>>>> 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
>>>>
>>>>>