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

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

Hi Peter,
On 05/24/2018 07:03 PM, Peter Maydell wrote:
> 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.)

Hum agreed. In any case the iommu_idx must be passed to VFIO along with
the notification, either as part of the notifier itself or in the
IOMMUTLBEntry. So VFIO may need to enumerate the supported iommu_idx and
register a notifier for relevant ones.

Thanks

Eric
> 
> 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
>