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

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

Hi Peter,

On 6/14/19 3:45 PM, Peter Maydell wrote:
> On Fri, 14 Jun 2019 at 14:40, Auger Eric <eric.auger@redhat.com> wrote:
>>
>> Hi Peter,
>>
>> On 6/14/19 3:26 PM, Peter Maydell wrote:
>>> On Tue, 11 Jun 2019 at 15:29, Eric Auger <eric.auger@redhat.com> 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".
> 
>>>> +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?
> 
> I dunno, but if the API isn't supposed to behave the way we've
> documented it to, we should fix the documentation...

fair enough

 Since
> the only user of memory_region_iommu_replay() is the vfio code
> I guess we can define it however is most convenient for vfio,
> but we should document what the method has to do to make things
> work.

OK I need to think about it. Maybe an alternative is to call
memory_region_iommu_replay() only when it makes sense.
> 
> PS: we have a memory_region_iommu_replay_all() which currently
> appears to be not used by anybody, could we get rid of it?

I will do that

Thanks

Eric
> 
> thanks
> -- PMM
>