Hi Jean,
On 9/29/23 17:52, Jean-Philippe Brucker wrote:
> Hi Eric,
>
> On Wed, Sep 13, 2023 at 10:01:40AM +0200, Eric Auger wrote:
>> For the time being the per device reserved regions are
>> just a duplicate of IOMMU wide reserved regions. Subsequent
>> patches will combine those with host reserved regions, if any.
>>
>> Signed-off-by: Eric Auger <eric.au...@redhat.com>
>> ---
>> include/hw/virtio/virtio-iommu.h | 1 +
>> hw/virtio/virtio-iommu.c | 42 ++++++++++++++++++++++++++------
>> 2 files changed, 35 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/hw/virtio/virtio-iommu.h
>> b/include/hw/virtio/virtio-iommu.h
>> index eea4564782..70b8ace34d 100644
>> --- a/include/hw/virtio/virtio-iommu.h
>> +++ b/include/hw/virtio/virtio-iommu.h
>> @@ -39,6 +39,7 @@ typedef struct IOMMUDevice {
>> AddressSpace as;
>> MemoryRegion root; /* The root container of the device */
>> MemoryRegion bypass_mr; /* The alias of shared memory MR */
>> + GList *resv_regions;
>> } IOMMUDevice;
>>
>> typedef struct IOMMUPciBus {
>> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
>> index 979cdb5648..ea359b586a 100644
>> --- a/hw/virtio/virtio-iommu.c
>> +++ b/hw/virtio/virtio-iommu.c
>> @@ -624,22 +624,48 @@ static int virtio_iommu_unmap(VirtIOIOMMU *s,
>> return ret;
>> }
>>
>> +static int consolidate_resv_regions(IOMMUDevice *sdev)
>> +{
>> + VirtIOIOMMU *s = sdev->viommu;
>> + int i;
>> +
>> + for (i = 0; i < s->nr_prop_resv_regions; i++) {
>> + ReservedRegion *reg = g_new0(ReservedRegion, 1);
>> +
>> + *reg = s->prop_resv_regions[i];
>> + sdev->resv_regions = g_list_append(sdev->resv_regions, reg);
>> + }
>> + return 0;
>> +}
>> +
>> static ssize_t virtio_iommu_fill_resv_mem_prop(VirtIOIOMMU *s, uint32_t ep,
>> uint8_t *buf, size_t free)
>> {
>> struct virtio_iommu_probe_resv_mem prop = {};
>> size_t size = sizeof(prop), length = size - sizeof(prop.head), total;
>> - int i;
>> + IOMMUDevice *sdev;
>> + GList *l;
>> + int ret;
>>
>> - total = size * s->nr_prop_resv_regions;
>> + sdev = container_of(virtio_iommu_mr(s, ep), IOMMUDevice, iommu_mr);
>> + if (!sdev) {
>> + return -EINVAL;
>> + }
>>
>> + ret = consolidate_resv_regions(sdev);
>> + if (ret) {
>> + return ret;
>> + }
>> +
>> + total = size * g_list_length(sdev->resv_regions);
>> if (total > free) {
>> return -ENOSPC;
>> }
>>
>> - for (i = 0; i < s->nr_prop_resv_regions; i++) {
>> - unsigned subtype = s->prop_resv_regions[i].type;
>> - Range *range = &s->prop_resv_regions[i].range;
>> + for (l = sdev->resv_regions; l; l = l->next) {
>> + ReservedRegion *reg = l->data;
>> + unsigned subtype = reg->type;
>> + Range *range = ®->range;
>>
>> assert(subtype == VIRTIO_IOMMU_RESV_MEM_T_RESERVED ||
>> subtype == VIRTIO_IOMMU_RESV_MEM_T_MSI);
>> @@ -857,7 +883,7 @@ static IOMMUTLBEntry
>> virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>> bool bypass_allowed;
>> int granule;
>> bool found;
>> - int i;
>> + GList *l;
>>
>> interval.low = addr;
>> interval.high = addr + 1;
>> @@ -895,8 +921,8 @@ static IOMMUTLBEntry
>> virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>> goto unlock;
>> }
>>
>> - for (i = 0; i < s->nr_prop_resv_regions; i++) {
>> - ReservedRegion *reg = &s->prop_resv_regions[i];
>> + for (l = sdev->resv_regions; l; l = l->next) {
>> + ReservedRegion *reg = l->data;
> This means translate() now only takes reserved regions into account after
> the guest issues a probe request, which only happens if the guest actually
> supports the probe feature. It may be better to build the list earlier
> (like when creating the IOMMUDevice), and complete it in
> set_iova_ranges(). I guess both could call consolidate() which would
> rebuild the whole list, for example
you're totally right, I completely missed that point. Will try to follow
your suggestion.
Thanks!
Eric
>
> Thanks,
> Jean
>
>>
>> if (range_contains(®->range, addr)) {
>> switch (reg->type) {
>> --
>> 2.41.0
>>
