Re: [PATCH v1 15/19] memory: Add an API for ATS support

[CLEMENT MATHIEU--DRIF]

On 08/01/2025 19:10, Michael S. Tsirkin 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 Fri, Nov 29, 2024 at 07:44:00AM +0000, 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.
>>
>> We decided not to use the existing translation operation for 2 reasons.
>> First, ATS is designed to translate ranges and not isolated addresses.
>> Second, we need ATS-specific parameters.
>>
>> 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 c0d064dbd8..14166e2874 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).
>> @@ -525,6 +529,20 @@ struct IOMMUMemoryRegionClass {
>>        * @iommu: the IOMMUMemoryRegion
>>        */
>>       int (*num_indexes)(IOMMUMemoryRegion *iommu);
>> +
>> +    /**
>> +     * @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);
>>   };
>>
>>   typedef struct RamDiscardListener RamDiscardListener;
>> @@ -1883,6 +1901,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 85f6834cb3..7f6f3798e6 100644
>> --- a/system/memory.c
>> +++ b/system/memory.c
>> @@ -2011,6 +2011,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);
>> +
>
> line too long - just call the variable mr.

It's still too long with mr, will do this instead:

IOMMUMemoryRegionClass *imrc =
     memory_region_get_iommu_class_nocheck(iommu_mr);

By the way, I think I figured out why patch encoding is not correct.
Will fix this in the next spin.

>
>> +    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,
>>                                       const IOMMUTLBEvent *event)
>>   {
>> --
>> 2.47.0