>-----Original Message-----
>From: Eric Auger <eric.au...@redhat.com>
>Subject: Re: [RFC v2 4/7] virtio-iommu: Compute host reserved regions
>
>Hi Zhenzhong,
>
>On 6/11/24 05:25, Duan, Zhenzhong wrote:
>>
>>> -----Original Message-----
>>> From: Eric Auger <eric.au...@redhat.com>
>>> Subject: [RFC v2 4/7] virtio-iommu: Compute host reserved regions
>>>
>>> Compute the host reserved regions in virtio_iommu_set_iommu_device().
>>> The usable IOVA regions are retrieved from the HOSTIOMMUDevice.
>>> The virtio_iommu_set_host_iova_ranges() helper turns usable regions
>>> into complementary reserved regions while testing the inclusion
>>> into existing ones. virtio_iommu_set_host_iova_ranges() reuse the
>>> implementation of virtio_iommu_set_iova_ranges() which will be
>>> removed in subsequent patches. rebuild_resv_regions() is just moved.
>>>
>>> Signed-off-by: Eric Auger <eric.au...@redhat.com>
>>> ---
>>> hw/virtio/virtio-iommu.c | 151 ++++++++++++++++++++++++++++++----
>----
>>> -
>>> 1 file changed, 117 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
>>> index 0680a357f0..33e9682b83 100644
>>> --- a/hw/virtio/virtio-iommu.c
>>> +++ b/hw/virtio/virtio-iommu.c
>>> @@ -494,12 +494,114 @@ get_host_iommu_device(VirtIOIOMMU
>>> *viommu, PCIBus *bus, int devfn) {
>>> return g_hash_table_lookup(viommu->host_iommu_devices, &key);
>>> }
>>>
>>> +/**
>>> + * rebuild_resv_regions: rebuild resv regions with both the
>>> + * info of host resv ranges and property set resv ranges
>>> + */
>>> +static int rebuild_resv_regions(IOMMUDevice *sdev)
>>> +{
>>> + GList *l;
>>> + int i = 0;
>>> +
>>> + /* free the existing list and rebuild it from scratch */
>>> + g_list_free_full(sdev->resv_regions, g_free);
>>> + sdev->resv_regions = NULL;
>>> +
>>> + /* First add host reserved regions if any, all tagged as RESERVED */
>>> + for (l = sdev->host_resv_ranges; l; l = l->next) {
>>> + ReservedRegion *reg = g_new0(ReservedRegion, 1);
>>> + Range *r = (Range *)l->data;
>>> +
>>> + reg->type = VIRTIO_IOMMU_RESV_MEM_T_RESERVED;
>>> + range_set_bounds(®->range, range_lob(r), range_upb(r));
>>> + sdev->resv_regions = resv_region_list_insert(sdev->resv_regions,
>reg);
>>> + trace_virtio_iommu_host_resv_regions(sdev-
>>>> iommu_mr.parent_obj.name, i,
>>> + range_lob(®->range),
>>> + range_upb(®->range));
>>> + i++;
>>> + }
>>> + /*
>>> + * then add higher priority reserved regions set by the machine
>>> + * through properties
>>> + */
>>> + add_prop_resv_regions(sdev);
>>> + return 0;
>>> +}
>>> +
>>> +static int virtio_iommu_set_host_iova_ranges(VirtIOIOMMU *s, PCIBus
>>> *bus,
>>> + int devfn, GList *iova_ranges,
>>> + Error **errp)
>>> +{
>>> + 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) {
>>> + error_report("%s no sbus", __func__);
>>> + }
>>> +
>>> + sdev = sbus->pbdev[devfn];
>>> +
>>> + current_ranges = sdev->host_resv_ranges;
>>> +
>>> + if (sdev->probe_done) {
>> Will this still happen with new interface?
>no this shouldn't. Replaced by a g_assert(!sdev->probe_done) to make
>sure the i/f is used properly.
>>
>>> + error_setg(errp,
>>> + "%s: Notified about new host reserved regions after
>>> probe",
>>> + __func__);
>>> + goto out;
>>> + }
>>> +
>>> + /* check that each new resv region is included in an existing one */
>>> + if (sdev->host_resv_ranges) {
>> Same here.
>To me this one can still happen in case several devices belong to the
>same group.
If same slot is used to plug/unplug vfio devices with different reserved
ranges, the second device plug may check failed.
It looks sdev->host_resv_ranges is not freed after unplug.
>>
>>> + 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;
>>> + }
>>> +
>>> + range_inverse_array(iova_ranges,
>>> + &sdev->host_resv_ranges,
>>> + 0, UINT64_MAX);
>>> + 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 bool virtio_iommu_set_iommu_device(PCIBus *bus, void *opaque,
>>> int devfn,
>>> HostIOMMUDevice *hiod, Error
>>> **errp)
>>> {
>>> VirtIOIOMMU *viommu = opaque;
>>> VirtioHostIOMMUDevice *vhiod;
>>> + HostIOMMUDeviceClass *hiodc =
>>> HOST_IOMMU_DEVICE_GET_CLASS(hiod);
>>> struct hiod_key *new_key;
>>> + GList *host_iova_ranges = NULL;
>> g_autoptr(GList)?
>are you sure this frees all the elements of the list?
Not quite sure, just see some code in qemu use it that way.
> As of now I would
>be tempted to leave the code as is.
Sure, that's fine.
Thanks
Zhenzhong
