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.
>
> 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
