Re: [PATCH v2 04/17] vfio/common: Extract vIOMMU code from vfio_sync_dirty_bitmap()

["Cédric Le Goater" <clg@redhat.com>]

On 6/17/24 4:00 PM, Eric Auger wrote:
> Hi Cédric,
> 
> On 6/17/24 08:33, Cédric Le Goater wrote:
>> From: Avihai Horon <avihaih@nvidia.com>
>>
>> Extract vIOMMU code from vfio_sync_dirty_bitmap() to a new function and
>> restructure the code.
>>
>> This is done in preparation for optimizing vIOMMU deviice dirty page
> device

fixed.


>> tracking. No functional changes intended.
>>
>> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> [ clg: - Rebased on upstream ]
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   hw/vfio/common.c | 63 +++++++++++++++++++++++++++++-------------------
>>   1 file changed, 38 insertions(+), 25 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index fe215918bdf66ddbe3c5db803e10ce1aa9756b90..f28641bad5cf4b71fcdc0a6c9d42b24c8d786248 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -1302,37 +1302,50 @@ vfio_sync_ram_discard_listener_dirty_bitmap(VFIOContainerBase *bcontainer,
>>                                                   &vrdl);
>>   }
>>   
>> +static int vfio_sync_iommu_dirty_bitmap(VFIOContainerBase *bcontainer,
>> +                                        MemoryRegionSection *section)
>> +{
>> +    VFIOGuestIOMMU *giommu;
>> +    bool found = false;
>> +    Int128 llend;
>> +    vfio_giommu_dirty_notifier gdn;
>> +    int idx;
>> +
>> +    QLIST_FOREACH(giommu, &bcontainer->giommu_list, giommu_next) {
>> +        if (MEMORY_REGION(giommu->iommu_mr) == section->mr &&
>> +            giommu->n.start == section->offset_within_region) {
>> +            found = true;
>> +            break;
>> +        }
>> +    }
>> +
>> +    if (!found) {
>> +        return 0;
>> +    }
>> +
>> +    gdn.giommu = giommu;
>> +    idx = memory_region_iommu_attrs_to_index(giommu->iommu_mr,
>> +                                             MEMTXATTRS_UNSPECIFIED);
>> +
>> +    llend = int128_add(int128_make64(section->offset_within_region),
>> +                       section->size);
>> +    llend = int128_sub(llend, int128_one());
>> +
>> +    iommu_notifier_init(&gdn.n, vfio_iommu_map_dirty_notify, IOMMU_NOTIFIER_MAP,
>> +                        section->offset_within_region, int128_get64(llend),
>> +                        idx);
>> +    memory_region_iommu_replay(giommu->iommu_mr, &gdn.n);
>> +
>> +    return 0;
> if this does not return anything else than 0, shouldn't it be void?

Yes it could. The return value is practical for the caller below though. Let's
keep it that way for now, it's harmless.


Thanks,

C.


> 
> Thanks
> 
> Eric
>> +}
>> +
>>   static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer,
>>                                     MemoryRegionSection *section, Error **errp)
>>   {
>>       ram_addr_t ram_addr;
>>   
>>       if (memory_region_is_iommu(section->mr)) {
>> -        VFIOGuestIOMMU *giommu;
>> -
>> -        QLIST_FOREACH(giommu, &bcontainer->giommu_list, giommu_next) {
>> -            if (MEMORY_REGION(giommu->iommu_mr) == section->mr &&
>> -                giommu->n.start == section->offset_within_region) {
>> -                Int128 llend;
>> -                vfio_giommu_dirty_notifier gdn = { .giommu = giommu };
>> -                int idx = memory_region_iommu_attrs_to_index(giommu->iommu_mr,
>> -                                                       MEMTXATTRS_UNSPECIFIED);
>> -
>> -                llend = int128_add(int128_make64(section->offset_within_region),
>> -                                   section->size);
>> -                llend = int128_sub(llend, int128_one());
>> -
>> -                iommu_notifier_init(&gdn.n,
>> -                                    vfio_iommu_map_dirty_notify,
>> -                                    IOMMU_NOTIFIER_MAP,
>> -                                    section->offset_within_region,
>> -                                    int128_get64(llend),
>> -                                    idx);
>> -                memory_region_iommu_replay(giommu->iommu_mr, &gdn.n);
>> -                break;
>> -            }
>> -        }
>> -        return 0;
>> +        return vfio_sync_iommu_dirty_bitmap(bcontainer, section);
>>       } else if (memory_region_has_ram_discard_manager(section->mr)) {
>>           int ret;
>>   
>