Hi Eric,
>-----Original Message-----
>From: Duan, Zhenzhong
>Subject: RE: [RFC 4/7] virtio-iommu: Implement PCIIOMMUOps
>set_host_resv_regions
>
>
>
>>-----Original Message-----
>>From: Eric Auger <eric.au...@redhat.com>
>>Subject: Re: [RFC 4/7] virtio-iommu: Implement PCIIOMMUOps
>>set_host_resv_regions
>>
>>Hi Zhenzhong,
>>On 1/18/24 08:43, Duan, Zhenzhong wrote:
>>> Hi Eric,
>>>
>>>> -----Original Message-----
>>>> From: Eric Auger <eric.au...@redhat.com>
>>>> Subject: [RFC 4/7] virtio-iommu: Implement PCIIOMMUOps
>>>> set_host_resv_regions
>>>>
>>>> Reuse the implementation of virtio_iommu_set_iova_ranges() which
>>>> will be removed in subsequent patches.
>>>>
>>>> Signed-off-by: Eric Auger <eric.au...@redhat.com>
>>>> ---
>>>> hw/virtio/virtio-iommu.c | 134 +++++++++++++++++++++++++++++----
>--
>>---
>>>> -
>>>> 1 file changed, 101 insertions(+), 33 deletions(-)
>>>>
>>>> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
>>>> index 8a4bd933c6..716a3fcfbf 100644
>>>> --- a/hw/virtio/virtio-iommu.c
>>>> +++ b/hw/virtio/virtio-iommu.c
>>>> @@ -461,8 +461,109 @@ static AddressSpace
>>>> *virtio_iommu_find_add_as(PCIBus *bus, void *opaque,
>>>> return &sdev->as;
>>>> }
>>>>
>>>> +/**
>>>> + * 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(PCIBus *bus, void
>>*opaque,
>>>> + int devfn, GList
>>>> *iova_ranges,
>>>> + Error **errp)
>>>> +{
>>>> + VirtIOIOMMU *s = opaque;
>>>> + 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__);
>>>> + }
>>> Do we plan to support multiple devices in same iommu group?
>>> as_by_busptr is hashed by bus which is an aliased bus of the device.
>>> So sbus may be NULL if device's bus is different from aliased bus.
>>If I understand you remark properly this means that
>>
>>virtio_iommu_set_host_iova_ranges should take as arg the aliased bus and
>>not the bus, right?
>>I think we shall support non singleton groups too.
>
>Not exactly. I think we should pass device's real BDF, not aliased BDF. Or else
>we setup reserved ranges of all devices in same group to aliased BDF.
>
>I'm just suspecting that the hash lookup with real BDF index will return NULL
>if
>multiple devices are in same group. If that’s true, then iova_ranges is never
>passed to guest.
I guess https://lists.gnu.org/archive/html/qemu-devel/2024-01/msg04153.html
may help on the discussion here, it's a complementation to this series to make
multiple devices in same IOMMU group works. Comments welcome.
Thanks
Zhenzhong
