Re: [Qemu-devel] [PATCH 2/2] hw/arm/smmuv3: Implement dummy replay

[Peter Maydell]

On Tue, 11 Jun 2019 at 15:29, Eric Auger <address@hidden> wrote:
>
> On ARM we currently do not support VFIO-PCI devices protected
> by the IOMMU. Any attempt to run such use case results in this
> kind of warning:
>
> "-device vfio-pci,host=0004:01:00.0,id=hostdev0,bus=pci.1,addr=0x0:
> warning: SMMUv3 does not support notification on MAP: device vfio-pci
> will not function properly".
>
> However this is just a warning and this should not prevent the
> guest from booting in a reasonable amount of time. This does not
> happen currently.
>
> This is due to the fact the VFIO vfio_listener_region_add() calls
> memory_region_iommu_replay(). As the SMMUv3 IOMMUMemoryRegionClass
> currently does not implement the replay() callback, the default
> memory_region_iommu_replay() implementation is used. This latter
> loops on the whole notifier's range (48b address space), translates
> each page and call the notifier on the resulting entry. This totally
> freezes the guest.
>
> The Intel IOMMU implements the replay() function which only
> notifies valid page table entries.
>
> In the looming SMMUv3 nested stage VFIO integration, there will be
> no need to replay() anything as there will not be any shadow page
> tables: the stage 1 page tables are owned by the guest.
>
> So let's implement a void replay() which satisfies both cases.
>
> Signed-off-by: Eric Auger <address@hidden>
> ---
>  hw/arm/smmuv3.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index e2f07d2864..1f578365ef 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -1489,6 +1489,11 @@ static void 
> smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
>      }
>  }
>
> +static inline void
> +smmuv3_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n)
> +{
> +}

This doesn't seem like a valid implementation of the replay
method to me. The API doc comment says
     * The default implementation of memory_region_iommu_replay() is to
     * call the IOMMU translate method for every page in the address space
     * with flag == IOMMU_NONE and then call the notifier if translate
     * returns a valid mapping. If this method is implemented then it
     * overrides the default behaviour, and must provide the full semantics
     * of memory_region_iommu_replay(), by calling @notifier for every
     * translation present in the IOMMU.

This empty function is definitely not going to call the notifier
for every IOMMU translation...

thanks
-- PMM