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

[Peter Maydell <peter.maydell@linaro.org>]

On 24 May 2018 at 16:29, Auger Eric <eric.auger@redhat.com> wrote:
> On 05/21/2018 04:03 PM, Peter Maydell wrote:
>> 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;
> I don't get why ne need iommu_idx field here. On translate the caller
> has it. On notify the notifier has it?

I think this is an accidental leftover from some earlier draft;
nothing in the patchset actually reads or writes this field, and
it should be deleted.

>>      IOMMUAccessFlags perm;
>>  };
>>

>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 8e57265edf..fb396cf00a 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -507,6 +507,7 @@ static void vfio_listener_region_add(MemoryListener *listener,
>>      if (memory_region_is_iommu(section->mr)) {
>>          VFIOGuestIOMMU *giommu;
>>          IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(section->mr);
>> +        int iommu_idx;
>>
>>          trace_vfio_listener_region_add_iommu(iova, end);
>>          /*
>> @@ -523,10 +524,13 @@ static void vfio_listener_region_add(MemoryListener *listener,
>>          llend = int128_add(int128_make64(section->offset_within_region),
>>                             section->size);
>>          llend = int128_sub(llend, int128_one());
>> +        iommu_idx = memory_region_iommu_attrs_to_index(iommu_mr,
>> +                                                       MEMTXATTRS_UNSPECIFIED);
> In that case VFIO ideally wants to be notified for any guest update
> (whatever the page set) to reprogram the physical IOMMU corresponding
> entries and doesn't want to register a notifier per iommu_idx. Also it
> does not know which ones are supported. Is there a corresponding
> iommu_idx value? MEMTXATTRS_ANY?

If VFIO is actually dealing with an IOMMU that needs to handle
multiple possible transactions for different tx attributes, then
it needs to know about all of them, because how it programs
the physical IOMMU needs to be different for "map X to Y for
Secure transactions" versus "map X to Y for NonSecure" (say).
(This would require new kernel APIs, I assume.)

If, as is currently the case, the VFIO infrastructure assumes that
the IOMMU will always translate any transaction from a device
identically regardless of its transaction attributes, then it
should just use MEMTXATTRS_UNSPECIFIED, and trust that the
emulated IOMMU will translate those correctly. (There might be
scope for VFIO checking that the IOMMU really does, eg that
it is only using one iommu index?)

Basically, VFIO is shadowing the behaviour of the emulated
IOMMU to reflect it into the kernel; if the IOMMU it's shadowing
is complicated then VFIO is going to need to be similarly
complicated, and "merge updates for different page tables
down into one" is not going to be the right behaviour.

thanks
-- PMM