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

[David Gibson <david@gibson.dropbear.id.au>]

[-- 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 --]