RE: [PATCH 3/6] virtio-iommu: Free [host_]resv_ranges on unset_iommu_devices

[Duan, Zhenzhong]

>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>Subject: Re: [PATCH 3/6] virtio-iommu: Free [host_]resv_ranges on
>unset_iommu_devices
>
>Hi Zhenzhong,
>
>On 7/17/24 05:06, Duan, Zhenzhong wrote:
>>
>>> -----Original Message-----
>>> From: Eric Auger <eric.auger@redhat.com>
>>> Subject: [PATCH 3/6] virtio-iommu: Free [host_]resv_ranges on
>>> unset_iommu_devices
>>>
>>> We are currently missing the deallocation of the [host_]resv_regions
>>> in case of hot unplug. Also to make things more simple let's rule
>>> out the case where multiple HostIOMMUDevices would be aliased and
>>> attached to the same IOMMUDevice. This allows to remove the handling
>>> of conflicting Host reserved regions. Anyway this is not properly
>>> supported at guest kernel level. On hotunplug the reserved regions
>>> are reset to the ones set by virtio-iommu property.
>>>
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>> ---
>>> hw/virtio/virtio-iommu.c | 62 ++++++++++++++++++----------------------
>>> 1 file changed, 28 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
>>> index 2c54c0d976..2de41ab412 100644
>>> --- a/hw/virtio/virtio-iommu.c
>>> +++ b/hw/virtio/virtio-iommu.c
>>> @@ -538,8 +538,6 @@ static int
>>> virtio_iommu_set_host_iova_ranges(VirtIOIOMMU *s, PCIBus *bus,
>>> {
>>>     IOMMUPciBus *sbus = g_hash_table_lookup(s->as_by_busptr, bus);
>>>     IOMMUDevice *sdev;
>>> -    GList *current_ranges;
>>> -    GList *l, *tmp, *new_ranges = NULL;
>>>     int ret = -EINVAL;
>>>
>>>     if (!sbus) {
>>> @@ -553,33 +551,10 @@ static int
>>> virtio_iommu_set_host_iova_ranges(VirtIOIOMMU *s, PCIBus *bus,
>>>         return ret;
>>>     }
>>>
>>> -    current_ranges = sdev->host_resv_ranges;
>>> -
>>> -    /* check that each new resv region is included in an existing one */
>>>     if (sdev->host_resv_ranges) {
>>> -        range_inverse_array(iova_ranges,
>>> -                            &new_ranges,
>>> -                            0, UINT64_MAX);
>>> -
>>> -        for (tmp = new_ranges; tmp; tmp = tmp->next) {
>>> -            Range *newr = (Range *)tmp->data;
>>> -            bool included = false;
>>> -
>>> -            for (l = current_ranges; l; l = l->next) {
>>> -                Range * r = (Range *)l->data;
>>> -
>>> -                if (range_contains_range(r, newr)) {
>>> -                    included = true;
>>> -                    break;
>>> -                }
>>> -            }
>>> -            if (!included) {
>>> -                goto error;
>>> -            }
>>> -        }
>>> -        /* all new reserved ranges are included in existing ones */
>>> -        ret = 0;
>>> -        goto out;
>>> +        error_setg(errp, "%s virtio-iommu does not support aliased BDF",
>>> +                   __func__);
>>> +        return ret;
>>>     }
>>>
>>>     range_inverse_array(iova_ranges,
>>> @@ -588,14 +563,31 @@ static int
>>> virtio_iommu_set_host_iova_ranges(VirtIOIOMMU *s, PCIBus *bus,
>>>     rebuild_resv_regions(sdev);
>>>
>>>     return 0;
>>> -error:
>>> -    error_setg(errp, "%s Conflicting host reserved ranges set!",
>>> -               __func__);
>>> -out:
>>> -    g_list_free_full(new_ranges, g_free);
>>> -    return ret;
>>> }
>>>
>>> +static void virtio_iommu_unset_host_iova_ranges(VirtIOIOMMU *s,
>>> PCIBus *bus,
>>> +                                                int devfn)
>>> +{
>>> +    IOMMUPciBus *sbus = g_hash_table_lookup(s->as_by_busptr, bus);
>>> +    IOMMUDevice *sdev;
>>> +
>>> +    if (!sbus) {
>>> +        return;
>>> +    }
>>> +
>>> +    sdev = sbus->pbdev[devfn];
>>> +    if (!sdev) {
>>> +        return;
>>> +    }
>>> +
>>> +    g_list_free_full(g_steal_pointer(&sdev->host_resv_ranges), g_free);
>>> +    g_list_free_full(sdev->resv_regions, g_free);
>>> +    sdev->host_resv_ranges = NULL;
>>> +    sdev->resv_regions = NULL;
>>> +    add_prop_resv_regions(sdev);
>> Is this necessary? rebuild_resv_regions() will do that again.
>My goal was to reset the state that existed before the
>
>virtio_iommu_set_host_iova_ranges() was called. prop resv regions were
>originally added in virtio_iommu_find_add_as.
>The next device to be hotplugged at this aliased bdf is not necessarily a VFIO
>device (may be a virtio one), in which case we would miss the prop resv
>regions.

Yeah, you are right, we must have it.

Thanks
Zhenzhong

>
>>
>> Other than that, for the whole series,
>>
>> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>
>Thanks!
>
>Eric
>>
>> Thanks
>> Zhenzhong
>>
>>> +}
>>> +
>>> +
>>> static bool check_page_size_mask(VirtIOIOMMU *viommu, uint64_t
>>> new_mask,
>>>                                  Error **errp)
>>> {
>>> @@ -704,6 +696,8 @@ virtio_iommu_unset_iommu_device(PCIBus
>*bus,
>>> void *opaque, int devfn)
>>>     if (!hiod) {
>>>         return;
>>>     }
>>> +    virtio_iommu_unset_host_iova_ranges(viommu, hiod->aliased_bus,
>>> +                                        hiod->aliased_devfn);
>>>
>>>     g_hash_table_remove(viommu->host_iommu_devices, &key);
>>> }
>>> --
>>> 2.41.0