[Qemu-devel] [PATCH] memory.h: Improve IOMMU related documentation

[Qemu-devel] [PATCH] memory.h: Improve IOMMU related documentation

[Peter Maydell]

Add more detail to the documentation for memory_region_init_iommu()
and other IOMMU-related functions and data structures.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I've been trying to understand how the IOMMU support works,
with a vague idea of extending it to be able to use it to
implement the Arm TrustZone Memory Protection Controller (which
is a bit of bus fabric that can configurably allow or deny
NonSecure access to RAM that sits behind it). I found the API
documentation comments a bit brief, so I thought I'd start by
expanding them as I figured out how the bits fit together.
Please check for inaccuracies :-)

I haven't documented the 'replay' method because I don't understand
that bit.

 include/exec/memory.h | 66 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 60 insertions(+), 6 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 4402ba6c0d..e62965a0c8 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -194,6 +194,14 @@ enum IOMMUMemoryRegionAttr {
     IOMMU_ATTR_SPAPR_TCE_FD
 };
 
+/**
+ * IOMMUMemoryRegionClass:
+ *
+ * All IOMMU implementations need to subclass TYPE_IOMMU_MEMORY_REGION
+ * and provide implementations of at least some of the methods here
+ * to handle requests to the memory region. The minimum requirement
+ * is a @translate method.
+ */
 typedef struct IOMMUMemoryRegionClass {
     /* private */
     struct DeviceClass parent_class;
@@ -203,20 +211,58 @@ typedef struct IOMMUMemoryRegionClass {
      * be the access permission of this translation operation. We can
      * set flag to IOMMU_NONE to mean that we don't need any
      * read/write permission checks, like, when for region replay.
+     *
+     * Once the IOMMU has returned a TLB entry, it must notify
+     * the IOMMU's users if that TLB entry changes, using
+     * memory_region_notify_iommu() (or, if necessary, by calling
+     * memory_region_notify_one() for each registered notifier).
+     *
+     * @iommu: the IOMMUMemoryRegion
+     * @hwaddr: address to be translated within the memory region
+     * @flag: requested access permissions
      */
     IOMMUTLBEntry (*translate)(IOMMUMemoryRegion *iommu, hwaddr addr,
                                IOMMUAccessFlags flag);
-    /* Returns minimum supported page size */
+    /* Returns minimum supported page size in bytes.
+     * If this method is not provided then the minimum is assumed to
+     * be TARGET_PAGE_SIZE.
+     *
+     * @iommu: the IOMMUMemoryRegion
+     */
     uint64_t (*get_min_page_size)(IOMMUMemoryRegion *iommu);
-    /* Called when IOMMU Notifier flag changed */
+    /* Called when IOMMU Notifier flag changes (ie when the set of
+     * events which IOMMU users are requesting notification for changes).
+     * Optional method -- need not be provided if the IOMMU does not
+     * need to know exactly which events must be notified.
+     *
+     * @iommu: the IOMMUMemoryRegion
+     * @old_flags: events which previously needed to be notified
+     * @new_flags: events which now need to be notified
+     */
     void (*notify_flag_changed)(IOMMUMemoryRegion *iommu,
                                 IOMMUNotifierFlag old_flags,
                                 IOMMUNotifierFlag new_flags);
-    /* Set this up to provide customized IOMMU replay function */
+    /* Set this up to provide customized IOMMU replay function.
+     * Optional method.
+     */
     void (*replay)(IOMMUMemoryRegion *iommu, IOMMUNotifier *notifier);
 
-    /* Get IOMMU misc attributes */
-    int (*get_attr)(IOMMUMemoryRegion *iommu, enum IOMMUMemoryRegionAttr,
+    /* Get IOMMU misc attributes. This is an optional method that
+     * can be used to allow users of the IOMMU to get implementation-specific
+     * information. The IOMMU implements this method to handle calls
+     * by IOMMU users to memory_region_iommu_get_attr() by filling in
+     * the arbitrary data pointer for any IOMMUMemoryRegionAttr valuess that
+     * the IOMMU supports. If the method is unimplemented then
+     * memory_region_iommu_get_attr() will always return -EINVAL.
+     *
+     * @iommu: the IOMMUMemoryRegion
+     * @attr: attribute being queried
+     * @data: memory to fill in with the attribute data
+     *
+     * Returns 0 on success, or a negative errno; in particular
+     * returns -EINVAL for unrecognized or unimplemented attribute types.
+     */
+    int (*get_attr)(IOMMUMemoryRegion *iommu, enum IOMMUMemoryRegionAttr attr,
                     void *data);
 } IOMMUMemoryRegionClass;
 
@@ -705,6 +751,14 @@ static inline void memory_region_init_reservation(MemoryRegion *mr,
  * An IOMMU region translates addresses and forwards accesses to a target
  * memory region.
  *
+ * The IOMMU implementation must define a subclass of TYPE_IOMMU_MEMORY_REGION.
+ * @_iommu_mr should be a pointer to enough memory for an instance of
+ * that subclass, @instance_size is the size of that subclass, and
+ * @mrtypename is its name. This function will initialize @_iommu_mr as an
+ * instance of the subclass, and its methods will then be called to handle
+ * accesses to the memory region. See the documentation of
+ * #IOMMUMemoryRegionClass for further details.
+ *
  * @_iommu_mr: the #IOMMUMemoryRegion to be initialized
  * @instance_size: the IOMMUMemoryRegion subclass instance size
  * @mrtypename: the type name of the #IOMMUMemoryRegion
@@ -981,7 +1035,7 @@ void memory_region_unregister_iommu_notifier(MemoryRegion *mr,
  * memory_region_iommu_get_attr: return an IOMMU attr if get_attr() is
  * defined on the IOMMU.
  *
- * Returns 0 if succeded, error code otherwise.
+ * Returns 0 on success, or a negative errno otherwise.
  *
  * @iommu_mr: the memory region
  * @attr: the requested attribute
-- 
2.17.0

Re: [Qemu-devel] [PATCH] memory.h: Improve IOMMU related documentation

[Paolo Bonzini]

On 30/04/2018 14:24, Peter Maydell wrote:
> -    /* Set this up to provide customized IOMMU replay function */
> +    /* Set this up to provide customized IOMMU replay function.
> +     * Optional method.
> +     */
>      void (*replay)(IOMMUMemoryRegion *iommu, IOMMUNotifier *notifier);

replay is needed if you want to support IOMMU notifiers.  After
memory_region_register_iommu_notifier you're only notified about future
changes to the mappings; memory_region_iommu_replay calls the replay
method so that the IOMMUNotifier is called for each existing mapping.

Paolo

Re: [Qemu-devel] [PATCH] memory.h: Improve IOMMU related documentation

[Peter Maydell]

On 30 April 2018 at 13:54, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 30/04/2018 14:24, Peter Maydell wrote:
>> -    /* Set this up to provide customized IOMMU replay function */
>> +    /* Set this up to provide customized IOMMU replay function.
>> +     * Optional method.
>> +     */
>>      void (*replay)(IOMMUMemoryRegion *iommu, IOMMUNotifier *notifier);
>
> replay is needed if you want to support IOMMU notifiers.  After
> memory_region_register_iommu_notifier you're only notified about future
> changes to the mappings; memory_region_iommu_replay calls the replay
> method so that the IOMMUNotifier is called for each existing mapping.

Is it then unrelated to record-and-replay ? That's what I guessed
it was for... Also, some IOMMUs (eg spapr_iommu.c) seem to support
notifiers but don't implement it.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] memory.h: Improve IOMMU related documentation

[Paolo Bonzini]

On 30/04/2018 14:57, Peter Maydell wrote:
> On 30 April 2018 at 13:54, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 30/04/2018 14:24, Peter Maydell wrote:
>>> -    /* Set this up to provide customized IOMMU replay function */
>>> +    /* Set this up to provide customized IOMMU replay function.
>>> +     * Optional method.
>>> +     */
>>>      void (*replay)(IOMMUMemoryRegion *iommu, IOMMUNotifier *notifier);
>>
>> replay is needed if you want to support IOMMU notifiers.  After
>> memory_region_register_iommu_notifier you're only notified about future
>> changes to the mappings; memory_region_iommu_replay calls the replay
>> method so that the IOMMUNotifier is called for each existing mapping.
> 
> Is it then unrelated to record-and-replay ? That's what I guessed
> it was for... Also, some IOMMUs (eg spapr_iommu.c) seem to support
> notifiers but don't implement it.

Yes, it's completely unrelated.  I have no idea why spapr_iommu.c
doesn't need it, so I am CCing the sPAPR and VFIO experts...

Paolo

Re: [Qemu-devel] [PATCH] memory.h: Improve IOMMU related documentation

[Peter Maydell]

On 30 April 2018 at 13:24, Peter Maydell <peter.maydell@linaro.org> wrote:
> Add more detail to the documentation for memory_region_init_iommu()
> and other IOMMU-related functions and data structures.

> @@ -203,20 +211,58 @@ typedef struct IOMMUMemoryRegionClass {
>       * be the access permission of this translation operation. We can
>       * set flag to IOMMU_NONE to mean that we don't need any
>       * read/write permission checks, like, when for region replay.
> +     *
> +     * Once the IOMMU has returned a TLB entry, it must notify
> +     * the IOMMU's users if that TLB entry changes, using
> +     * memory_region_notify_iommu() (or, if necessary, by calling
> +     * memory_region_notify_one() for each registered notifier).
> +     *
> +     * @iommu: the IOMMUMemoryRegion
> +     * @hwaddr: address to be translated within the memory region
> +     * @flag: requested access permissions
>       */
>      IOMMUTLBEntry (*translate)(IOMMUMemoryRegion *iommu, hwaddr addr,
>                                 IOMMUAccessFlags flag);

A question I just thought of -- how long is the IOMMUTLBEntry I get
back guaranteed to be valid for? For instance if I'm in an RCU
critical section can I assume it won't become invalid before I
leave the critical section? Or must I always register an IOMMU
notifier before I call translate? I'm guessing not the latter
because flatview_do_translate() does not, but is the guarantee
only RCU-critical section or while-holding-big-QEMU-lock or
something else?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] memory.h: Improve IOMMU related documentation

[Peter Maydell]

On 30 April 2018 at 14:08, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 30/04/2018 14:57, Peter Maydell wrote:
>> On 30 April 2018 at 13:54, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> On 30/04/2018 14:24, Peter Maydell wrote:
>>>> -    /* Set this up to provide customized IOMMU replay function */
>>>> +    /* Set this up to provide customized IOMMU replay function.
>>>> +     * Optional method.
>>>> +     */
>>>>      void (*replay)(IOMMUMemoryRegion *iommu, IOMMUNotifier *notifier);
>>>
>>> replay is needed if you want to support IOMMU notifiers.  After
>>> memory_region_register_iommu_notifier you're only notified about future
>>> changes to the mappings; memory_region_iommu_replay calls the replay
>>> method so that the IOMMUNotifier is called for each existing mapping.
>>
>> Is it then unrelated to record-and-replay ? That's what I guessed
>> it was for... Also, some IOMMUs (eg spapr_iommu.c) seem to support
>> notifiers but don't implement it.
>
> Yes, it's completely unrelated.  I have no idea why spapr_iommu.c
> doesn't need it, so I am CCing the sPAPR and VFIO experts...

There does seem to be a default implementation in
memory_region_iommu_replay(), maybe that is sufficient for spapr?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] memory.h: Improve IOMMU related documentation

[Alex Williamson]

On Mon, 30 Apr 2018 14:35:20 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 30 April 2018 at 14:08, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > On 30/04/2018 14:57, Peter Maydell wrote:  
> >> On 30 April 2018 at 13:54, Paolo Bonzini <pbonzini@redhat.com> wrote:  
> >>> On 30/04/2018 14:24, Peter Maydell wrote:  
> >>>> -    /* Set this up to provide customized IOMMU replay function */
> >>>> +    /* Set this up to provide customized IOMMU replay function.
> >>>> +     * Optional method.
> >>>> +     */
> >>>>      void (*replay)(IOMMUMemoryRegion *iommu, IOMMUNotifier *notifier);  
> >>>
> >>> replay is needed if you want to support IOMMU notifiers.  After
> >>> memory_region_register_iommu_notifier you're only notified about future
> >>> changes to the mappings; memory_region_iommu_replay calls the replay
> >>> method so that the IOMMUNotifier is called for each existing mapping.  
> >>
> >> Is it then unrelated to record-and-replay ? That's what I guessed
> >> it was for... Also, some IOMMUs (eg spapr_iommu.c) seem to support
> >> notifiers but don't implement it.  
> >
> > Yes, it's completely unrelated.  I have no idea why spapr_iommu.c
> > doesn't need it, so I am CCing the sPAPR and VFIO experts...  
> 
> There does seem to be a default implementation in
> memory_region_iommu_replay(), maybe that is sufficient for spapr?

AIUI, the default implementation is used by spapr, it was added here:

commit a788f227ef7bd2912fcaacdfe13d13ece2998149
Author: David Gibson <david@gibson.dropbear.id.au>
Date:   Wed Sep 30 12:13:55 2015 +1000

    memory: Allow replay of IOMMU mapping notifications
    
    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_iommu_replay() function to handle this case.  It
    replays any existing mappings in an IOMMU memory region to a specified
    notifier.  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 <david@gibson.dropbear.id.au>
    Reviewed-by: Laurent Vivier <lvivier@redhat.com>
    Acked-by: Paolo Bonzini <pbonzini@redhat.com>
    Signed-off-by: Alex Williamson <alex.williamson@redhat.com>

VT-d emulation then needed its own replay and that hook was later added
here:

commit faa362e3cc94bf739a89b457693e3fbd7a4b95c4
Author: Peter Xu <peterx@redhat.com>
Date:   Fri Apr 7 18:59:11 2017 +0800

    memory: add MemoryRegionIOMMUOps.replay() callback
    
    Originally we have one memory_region_iommu_replay() function, which is
    the default behavior to replay the translations of the whole IOMMU
    region. However, on some platform like x86, we may want our own replay
    logic for IOMMU regions. This patch adds one more hook for IOMMUOps for
    the callback, and it'll override the default if set.
    
    Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
    Reviewed-by: Eric Auger <eric.auger@redhat.com>
    Reviewed-by: \"Michael S. Tsirkin\" <mst@redhat.com>
    Signed-off-by: Peter Xu <peterx@redhat.com>
    Message-Id: <1491562755-23867-6-git-send-email-peterx@redhat.com>
    Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Thanks,
Alex

Re: [Qemu-devel] [PATCH] memory.h: Improve IOMMU related documentation

[Paolo Bonzini]

On 30/04/2018 15:34, Peter Maydell wrote:
>>       */
>>      IOMMUTLBEntry (*translate)(IOMMUMemoryRegion *iommu, hwaddr addr,
>>                                 IOMMUAccessFlags flag);
> A question I just thought of -- how long is the IOMMUTLBEntry I get
> back guaranteed to be valid for? For instance if I'm in an RCU
> critical section can I assume it won't become invalid before I
> leave the critical section? Or must I always register an IOMMU
> notifier before I call translate? I'm guessing not the latter
> because flatview_do_translate() does not, but is the guarantee
> only RCU-critical section or while-holding-big-QEMU-lock or
> something else?

The only pointer inside is to the target AddressSpace, which (see
address_space_destroy) is safe to access inside either of those
condition.  In practice we use the former.

Paolo

Re: [Qemu-devel] [PATCH] memory.h: Improve IOMMU related documentation

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3999 bytes --]

On Mon, Apr 30, 2018 at 08:28:35AM -0600, Alex Williamson wrote:
> On Mon, 30 Apr 2018 14:35:20 +0100
> Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> > On 30 April 2018 at 14:08, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > > On 30/04/2018 14:57, Peter Maydell wrote:  
> > >> On 30 April 2018 at 13:54, Paolo Bonzini <pbonzini@redhat.com> wrote:  
> > >>> On 30/04/2018 14:24, Peter Maydell wrote:  
> > >>>> -    /* Set this up to provide customized IOMMU replay function */
> > >>>> +    /* Set this up to provide customized IOMMU replay function.
> > >>>> +     * Optional method.
> > >>>> +     */
> > >>>>      void (*replay)(IOMMUMemoryRegion *iommu, IOMMUNotifier *notifier);  
> > >>>
> > >>> replay is needed if you want to support IOMMU notifiers.  After
> > >>> memory_region_register_iommu_notifier you're only notified about future
> > >>> changes to the mappings; memory_region_iommu_replay calls the replay
> > >>> method so that the IOMMUNotifier is called for each existing mapping.  
> > >>
> > >> Is it then unrelated to record-and-replay ? That's what I guessed
> > >> it was for... Also, some IOMMUs (eg spapr_iommu.c) seem to support
> > >> notifiers but don't implement it.  
> > >
> > > Yes, it's completely unrelated.  I have no idea why spapr_iommu.c
> > > doesn't need it, so I am CCing the sPAPR and VFIO experts...  
> > 
> > There does seem to be a default implementation in
> > memory_region_iommu_replay(), maybe that is sufficient for spapr?
> 
> AIUI, the default implementation is used by spapr, it was added here:
> 
> commit a788f227ef7bd2912fcaacdfe13d13ece2998149
> Author: David Gibson <david@gibson.dropbear.id.au>
> Date:   Wed Sep 30 12:13:55 2015 +1000
> 
>     memory: Allow replay of IOMMU mapping notifications
>     
>     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_iommu_replay() function to handle this case.  It
>     replays any existing mappings in an IOMMU memory region to a specified
>     notifier.  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 <david@gibson.dropbear.id.au>
>     Reviewed-by: Laurent Vivier <lvivier@redhat.com>
>     Acked-by: Paolo Bonzini <pbonzini@redhat.com>
>     Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> 
> VT-d emulation then needed its own replay and that hook was later added
> here:

Yes, that's right.  We actually introduced the replay for spapr's
benefit, initially with just the default implementation not a hook.
When x86 needed it, the default implementation although correct, was
grossly inefficient so the hook was added.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]