Re: [PATCH v2 05/12] virtio-iommu: Introduce per IOMMUDevice reserved regions

[Eric Auger <eric.auger@redhat.com>]

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.auger@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 = &reg->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(&reg->range, addr)) {
>>              switch (reg->type) {
>> -- 
>> 2.41.0
>>