Re: [Qemu-arm] [PATCH 15/27] iommu: Add IOMMU index argument to notifier APIs

[Alex Bennée]

Peter Maydell <address@hidden> writes:

> On 23 May 2018 at 10:08, Alex Bennée <address@hidden> wrote:
>>
>> Peter Maydell <address@hidden> writes:
>>
>>> Add support for multiple IOMMU indexes to the IOMMU notifier APIs.
>>> When initializing a notifier with iommu_notifier_init(), the caller
>>> must pass the IOMMU index that it is interested in. When a change
>>> happens, the IOMMU implementation must pass
>>> memory_region_notify_iommu() the IOMMU index that has changed and
>>> that notifiers must be called for.
>>>
>>> IOMMUs which support only a single index don't need to change.
>>> Callers which only really support working with IOMMUs with a single
>>> index can use the result of passing MEMTXATTRS_UNSPECIFIED to
>>> memory_region_iommu_attrs_to_index().
>>>
>>> Signed-off-by: Peter Maydell <address@hidden>
>>> ---
>>>  include/exec/memory.h    | 11 ++++++++++-
>>>  hw/i386/intel_iommu.c    |  4 ++--
>>>  hw/ppc/spapr_iommu.c     |  2 +-
>>>  hw/s390x/s390-pci-inst.c |  4 ++--
>>>  hw/vfio/common.c         |  6 +++++-
>>>  hw/virtio/vhost.c        |  7 ++++++-
>>>  memory.c                 |  8 +++++++-
>>>  7 files changed, 33 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>>> index f6226fb263..4e6b125add 100644
>>> --- a/include/exec/memory.h
>>> +++ b/include/exec/memory.h
>>> @@ -71,6 +71,7 @@ struct IOMMUTLBEntry {
>>>      hwaddr           iova;
>>>      hwaddr           translated_addr;
>>>      hwaddr           addr_mask;  /* 0xfff = 4k translation */
>>> +    int              iommu_idx;
>>>      IOMMUAccessFlags perm;
>>>  };
>>>
>>> @@ -98,18 +99,21 @@ struct IOMMUNotifier {
>>>      /* Notify for address space range start <= addr <= end */
>>>      hwaddr start;
>>>      hwaddr end;
>>> +    int iommu_idx;
>>
>> Its a minor thing but are we ever expecting iommu_idx to ever be
>> negative?
>
> Coming back to this one -- no, we don't expect negative iommu_idxs.
> But on the other hand we don't ever expect negative TCG mmu_indexes
> either, and we use 'int' for those...

That's a clean-up right there ;-)

As I said it's a minor thing but I generally favour clearest type for
the range you are going to see. That said the enum stuff on the older
clangs did confuse me so there is that...

--
Alex Bennée