Re: [PATCH 2/6] virtio-iommu: Remove probe_done

[Cédric Le Goater]

On 7/16/24 11:45, Eric Auger wrote:
> Now we have switched to PCIIOMMUOps to convey host IOMMU information,
> the host reserved regions are transmitted when the PCIe topology is
> built. This happens way before the virtio-iommu driver calls the probe
> request. So let's remove the probe_done flag that allowed to check
> the probe was not done before the IOMMU MR got enabled. Besides this
> probe_done flag had a flaw wrt migration since it was not saved/restored.
>
> The only case at risk is if 2 devices were plugged to a
> PCIe to PCI bridge and thus aliased. First of all we
> discovered in the past this case was not properly supported for
> neither SMMU nor virtio-iommu on guest kernel side: see
>
> [RFC] virtio-iommu: Take into account possible aliasing in virtio_iommu_mr()
> https://lore.kernel.org/all/20230116124709.793084-1-eric.auger@redhat.com/
>
> If this were supported by the guest kernel, it is unclear what the call
> sequence would be from a virtio-iommu driver point of view.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   include/hw/virtio/virtio-iommu.h | 1 -
>   hw/virtio/virtio-iommu.c         | 3 ---
>   2 files changed, 4 deletions(-)
>
> diff --git a/include/hw/virtio/virtio-iommu.h b/include/hw/virtio/virtio-iommu.h
> index bdb3da72d0..7db4210b16 100644
> --- a/include/hw/virtio/virtio-iommu.h
> +++ b/include/hw/virtio/virtio-iommu.h
> @@ -43,7 +43,6 @@ typedef struct IOMMUDevice {
>       MemoryRegion bypass_mr;     /* The alias of shared memory MR */
>       GList *resv_regions;
>       GList *host_resv_ranges;
> -    bool probe_done;
>   } IOMMUDevice;
>   
>   typedef struct IOMMUPciBus {
> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> index 4e34dacd6e..2c54c0d976 100644
> --- a/hw/virtio/virtio-iommu.c
> +++ b/hw/virtio/virtio-iommu.c
> @@ -555,8 +555,6 @@ static int virtio_iommu_set_host_iova_ranges(VirtIOIOMMU 
> *s, PCIBus *bus,
>   
>       current_ranges = sdev->host_resv_ranges;
>   
> -    g_assert(!sdev->probe_done);
> -
>       /* check that each new resv region is included in an existing one */
>       if (sdev->host_resv_ranges) {
>           range_inverse_array(iova_ranges,
> @@ -956,7 +954,6 @@ static int virtio_iommu_probe(VirtIOIOMMU *s,
>       }
>       buf += count;
>       free -= count;
> -    sdev->probe_done = true;
>   
>       return VIRTIO_IOMMU_S_OK;
>   }