Re: [Qemu-arm] [PATCH v7 13/20] hw/arm/smmuv3: Implement IOMMU memory region replay callback

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

Hi Tomasz,
On 15/09/2017 12:42, tn wrote:
> Hi Eric,
> 
> On 15.09.2017 09:30, Auger Eric wrote:
>> Hi Tomasz,
>>
>> On 14/09/2017 16:43, Tomasz Nowicki wrote:
>>> On 14.09.2017 16:31, Tomasz Nowicki wrote:
>>>> Hi Eric,
>>>>
>>>> On 14.09.2017 11:27, Linu Cherian wrote:
>>>>> Hi Eric,
>>>>>
>>>>> On Fri Sep 01, 2017 at 07:21:16PM +0200, Eric Auger wrote:
>>>>>> memory_region_iommu_replay() is used for VFIO integration.
>>>>>>
>>>>>> However its default implementation is not adapted to SMMUv3
>>>>>> IOMMU memory region. Indeed the input address range is too
>>>>>> huge and its execution is too slow as it calls the translate()
>>>>>> callback on each granule.
>>>>>>
>>>>>> Let's implement the replay callback which hierarchically walk
>>>>>> over the page table structure and notify only the segments
>>>>>> that are populated with valid entries.
>>>>>>
>>>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>>>> ---
>>>>>>    hw/arm/smmuv3.c     | 36 ++++++++++++++++++++++++++++++++++++
>>>>>>    hw/arm/trace-events |  1 +
>>>>>>    2 files changed, 37 insertions(+)
>>>>>>
>>>>>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>>>>>> index 8e7d10d..c43bd93 100644
>>>>>> --- a/hw/arm/smmuv3.c
>>>>>> +++ b/hw/arm/smmuv3.c
>>>>>> @@ -657,6 +657,41 @@ static int smmuv3_notify_entry(IOMMUTLBEntry
>>>>>> *entry, void *private)
>>>>>>        return 0;
>>>>>>    }
>>>>>> +/* Unmap the whole notifier's range */
>>>>>> +static void smmuv3_unmap_notifier_range(IOMMUNotifier *n)
>>>>>> +{
>>>>>> +    IOMMUTLBEntry entry;
>>>>>> +    hwaddr size = n->end - n->start + 1;
>>>>>> +
>>>>>> +    entry.target_as = &address_space_memory;
>>>>>> +    entry.iova = n->start & ~(size - 1);
>>>>>> +    entry.perm = IOMMU_NONE;
>>>>>> +    entry.addr_mask = size - 1;
>>>>>> +
>>>>>> +    memory_region_notify_one(n, &entry);
>>>>>> +}
>>>>>> +
>>>>>> +static void smmuv3_replay(IOMMUMemoryRegion *mr, IOMMUNotifier *n)
>>>>>> +{
>>>>>> +    SMMUTransCfg cfg = {};
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    trace_smmuv3_replay(mr->parent_obj.name, n, n->start, n->end);
>>>>>> +    smmuv3_unmap_notifier_range(n);
>>>>>> +
>>>>>> +    ret = smmuv3_decode_config(mr, &cfg);
>>>>>> +    if (ret) {
>>>>>> +        error_report("%s error decoding the configuration for iommu
>>>>>> mr=%s",
>>>>>> +                     __func__, mr->parent_obj.name);
>>>>>> +    }
>>>>>>
>>>>>
>>>>> On an invalid config being found, shouldnt we return rather than
>>>>> proceeding with
>>>>> page table walk. For example on an invalid Stream table entry.
>>>>
>>>> Indeed, without return here vhost case is not working for me.
>>>
>>> I was just lucky one time. return here has no influence. Vhost still not
>>> working. Sorry for noise.
>>
>> As far as I understand the replay() callback only is called in VFIO use
>> case. So this shouldn't impact vhost.
>>
>> I can't reproduce your vhost issue on my side. I will review the
>> invalidate code again and compare against the last version.
>>
>> What is the page size used by your guest?
> 
> 64K page size for guest as well as for host.
> 
> However, I've just checked 4K page size for guest and then vhost is
> working fine.
I can reproduce the issue with vhost on 64KB page guest. Currently
investigating...

Thanks!

Eric
> 
> Thanks,
> Tomasz