>-----Original Message-----
>From: Eric Auger <eric.au...@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.au...@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.au...@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.d...@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
