RE: [RFC 4/7] virtio-iommu: Implement PCIIOMMUOps set_host_resv_regions

[Duan, Zhenzhong]

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.auger@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.auger@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.auger@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(&reg->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(&reg->range),
>>>> +                                             range_upb(&reg->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