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

[Auger Eric]

Hi Peter,

On 6/14/19 3:26 PM, Peter Maydell wrote:
> 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...
The situation is a bit odd. SMMUv3 is not integrated with VFIO so VFIO
devices will not work anyway (we are not able to notify on MAP). There
is a warning already reporting the issue. However the default
implementation of memory_region_iommu_replay() prevents the guest from
booting. So what would you advise?

Thanks

Eric
> 
> thanks
> -- PMM
>