Re: [Qemu-devel] [RFC PATCH 05/10] memory: Allow replay of IOMMU mapping notifications

[Laurent Vivier]

On 23/09/2015 12:40, Thomas Huth wrote:
> On 17/09/15 15:09, David Gibson wrote:
>> When we have guest visible IOMMUs, we allow notifiers to be registered
>> which will be informed of all changes to IOMMU mappings.  This is used by
>> vfio to keep the host IOMMU mappings in sync with guest IOMMU mappings.
>>
>> However, unlike with a memory region listener, an iommu notifier won't be
>> told about any mappings which already exist in the (guest) IOMMU at the
>> time it is registered.  This can cause problems if hotplugging a VFIO
>> device onto a guest bus which had existing guest IOMMU mappings, but didn't
>> previously have an VFIO devices (and hence no host IOMMU mappings).
>>
>> This adds a memory_region_register_iommu_notifier_replay() function to
>> handle this case.  As well as registering the new notifier it replays
>> existing mappings.  Because the IOMMU memory region doesn't internally
>> remember the granularity of the guest IOMMU it has a small hack where the
>> caller must specify a granularity at which to replay mappings.
>>
>> If there are finer mappings in the guest IOMMU these will be reported in
>> the iotlb structures passed to the notifier which it must handle (probably
>> causing it to flag an error).  This isn't new - the VFIO iommu notifier
>> must already handle notifications about guest IOMMU mappings too short
>> for it to represent in the host IOMMU.
>>
>> Signed-off-by: David Gibson <address@hidden>
>> ---
>>  include/exec/memory.h | 16 ++++++++++++++++
>>  memory.c              | 18 ++++++++++++++++++
>>  2 files changed, 34 insertions(+)
> 
>> diff --git a/memory.c b/memory.c
>> index 0d8b2d9..6b5a2f1 100644
>> --- a/memory.c
>> +++ b/memory.c
>> @@ -1403,6 +1403,24 @@ void 
>> memory_region_register_iommu_notifier(MemoryRegion *mr, Notifier *n)
>>      notifier_list_add(&mr->iommu_notify, n);
>>  }
>>  
>> +void memory_region_register_iommu_notifier_replay(MemoryRegion *mr, 
>> Notifier *n,
>> +                                                  hwaddr granularity, bool 
>> is_write)
> 
> granularity itself is not an address, but a size, isn't it? So using
> "hwaddr" sounds wrong here.

I disagree: in memory.c,  all size are defined with hwaddr.

>> +{
>> +    hwaddr addr;
> 
> dma_addr_t ?
> 
>> +    IOMMUTLBEntry iotlb;
>> +
>> +    memory_region_register_iommu_notifier(mr, n);
>> +
>> +    for (addr = 0;
>> +         int128_lt(int128_make64(addr), mr->size);
>> +         addr += granularity) {
>> +
>> +        iotlb = mr->iommu_ops->translate(mr, addr, is_write);
>> +        if (iotlb.perm != IOMMU_NONE)
>> +            n->notify(n, &iotlb);
> 
> Missing curly braces.
> 
>> +    }
>> +}
>> +
> 
>  Thomas
>