Re: [PATCH ats_vtd v5 19/22] memory: add an API for ATS support

[Yi Liu <yi.l.liu@intel.com>]

On 2024/7/4 12:30, CLEMENT MATHIEU--DRIF wrote:
> 
> On 03/07/2024 14:14, Yi Liu 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 2024/7/2 13:52, CLEMENT MATHIEU--DRIF wrote:
>>> From: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
>>>
>>> IOMMU have to implement iommu_ats_request_translation to support ATS.
>>>
>>> Devices can use IOMMU_TLB_ENTRY_TRANSLATION_ERROR to check the tlb
>>> entries returned by a translation request.
>>>
>>> Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
>>> ---
>>>    include/exec/memory.h | 26 ++++++++++++++++++++++++++
>>>    system/memory.c       | 20 ++++++++++++++++++++
>>>    2 files changed, 46 insertions(+)
>>>
>>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>>> index 003ee06610..48555c87c6 100644
>>> --- a/include/exec/memory.h
>>> +++ b/include/exec/memory.h
>>> @@ -148,6 +148,10 @@ struct IOMMUTLBEntry {
>>>        uint32_t         pasid;
>>>    };
>>>
>>> +/* Check if an IOMMU TLB entry indicates a translation error */
>>> +#define IOMMU_TLB_ENTRY_TRANSLATION_ERROR(entry) ((((entry)->perm) &
>>> IOMMU_RW) \
>>> +                                                    == IOMMU_NONE)
>>> +
>>>    /*
>>>     * Bitmap for different IOMMUNotifier capabilities. Each notifier can
>>>     * register with one or multiple IOMMU Notifier capability bit(s).
>>> @@ -571,6 +575,20 @@ struct IOMMUMemoryRegionClass {
>>>         int (*iommu_set_iova_ranges)(IOMMUMemoryRegion *iommu,
>>>                                      GList *iova_ranges,
>>>                                      Error **errp);
>>> +
>>> +    /**
>>> +     * @iommu_ats_request_translation:
>>> +     * This method must be implemented if the IOMMU has ATS enabled
>>> +     *
>>> +     * @see pci_ats_request_translation_pasid
>>> +     */
>>> +    ssize_t (*iommu_ats_request_translation)(IOMMUMemoryRegion *iommu,
>>> +                                             bool priv_req, bool
>>> exec_req,
>>> +                                             hwaddr addr, size_t
>>> length,
>>> +                                             bool no_write,
>>> +                                             IOMMUTLBEntry *result,
>>> +                                             size_t result_length,
>>> +                                             uint32_t *err_count);
>>>    };
>>>
>>
>> I'm not quite understanding why the existing translate() does not work.
>> Could you elaborate?
> We need more parameters than what the existing translation function has.
> This one is designed to get translations for a range instead of just a
> single address.
> The main purpose is to expose an API that has the same parameters as a
> PCIe translation request message
> and to give all the information the IOMMU needs to process the request.

ok. Please make the reason clear in commit message as well. Let's see if
any other opinion on it.

>>
>>>    typedef struct RamDiscardListener RamDiscardListener;
>>> @@ -1926,6 +1944,14 @@ void
>>> memory_region_iommu_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier
>>> *n);
>>>    void memory_region_unregister_iommu_notifier(MemoryRegion *mr,
>>>                                                 IOMMUNotifier *n);
>>>
>>> +ssize_t
>>> memory_region_iommu_ats_request_translation(IOMMUMemoryRegion *iommu_mr,
>>> +                                                bool priv_req, bool
>>> exec_req,
>>> +                                                hwaddr addr, size_t
>>> length,
>>> +                                                bool no_write,
>>> +                                                IOMMUTLBEntry *result,
>>> +                                                size_t result_length,
>>> +                                                uint32_t *err_count);
>>> +
>>>    /**
>>>     * memory_region_iommu_get_attr: return an IOMMU attr if get_attr() is
>>>     * defined on the IOMMU.
>>> diff --git a/system/memory.c b/system/memory.c
>>> index 74cd73ebc7..8268df7bf5 100644
>>> --- a/system/memory.c
>>> +++ b/system/memory.c
>>> @@ -2005,6 +2005,26 @@ void
>>> memory_region_unregister_iommu_notifier(MemoryRegion *mr,
>>>        memory_region_update_iommu_notify_flags(iommu_mr, NULL);
>>>    }
>>>
>>> +ssize_t
>>> memory_region_iommu_ats_request_translation(IOMMUMemoryRegion *iommu_mr,
>>> +                                                    bool priv_req,
>>> +                                                    bool exec_req,
>>> +                                                    hwaddr addr,
>>> size_t length,
>>> +                                                    bool no_write,
>>> + IOMMUTLBEntry *result,
>>> +                                                    size_t
>>> result_length,
>>> +                                                    uint32_t
>>> *err_count)
>>> +{
>>> +    IOMMUMemoryRegionClass *imrc =
>>> memory_region_get_iommu_class_nocheck(iommu_mr);
>>> +
>>> +    if (!imrc->iommu_ats_request_translation) {
>>> +        return -ENODEV;
>>> +    }
>>> +
>>> +    return imrc->iommu_ats_request_translation(iommu_mr, priv_req,
>>> exec_req,
>>> +                                               addr, length,
>>> no_write, result,
>>> +                                               result_length,
>>> err_count);
>>> +}
>>> +
>>>    void memory_region_notify_iommu_one(IOMMUNotifier *notifier,
>>>                                        IOMMUTLBEvent *event)
>>>    {
>>
>> -- 
>> Regards,
>> Yi Liu

-- 
Regards,
Yi Liu