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

[Jean-Philippe Brucker <jean-philippe@linaro.org>]

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

Thanks,
Jean

>  
>          if (range_contains(&reg->range, addr)) {
>              switch (reg->type) {
> -- 
> 2.41.0
>