Re: [Qemu-devel] [PATCH qemu v7 13/14] spapr_pci/spapr_pci_vfio: Support Dynamic DMA Windows (DDW)

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

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

On Thu, Jun 18, 2015 at 09:35:44PM +1000, Alexey Kardashevskiy wrote:
> On 05/05/2015 10:49 PM, David Gibson wrote:
> >On Sat, Apr 25, 2015 at 10:24:43PM +1000, Alexey Kardashevskiy wrote:
> >>This adds support for Dynamic DMA Windows (DDW) option defined by
> >>the SPAPR specification which allows to have additional DMA window(s)
> >>
> >>This implements DDW for emulated and VFIO devices. As all TCE root regions
> >>are mapped at 0 and 64bit long (and actual tables are child regions),
> >>this replaces memory_region_add_subregion() with _overlap() to make
> >>QEMU memory API happy.
> >>
> >>This reserves RTAS token numbers for DDW calls.
> >>
> >>This implements helpers to interact with VFIO kernel interface.
> >>
> >>This changes the TCE table migration descriptor to support dynamic
> >>tables as from now on, PHB will create as many stub TCE table objects
> >>as PHB can possibly support but not all of them might be initialized at
> >>the time of migration because DDW might or might not be requested by
> >>the guest.
> >>
> >>The "ddw" property is enabled by default on a PHB but for compatibility
> >>the pseries-2.3 machine and older disable it.
> >>
> >>This implements DDW for VFIO. The host kernel support is required.
> >>This adds a "levels" property to PHB to control the number of levels
> >>in the actual TCE table allocated by the host kernel, 0 is the default
> >>value to tell QEMU to calculate the correct value. Current hardware
> >>supports up to 5 levels.
> >>
> >>The existing linux guests try creating one additional huge DMA window
> >>with 64K or 16MB pages and map the entire guest RAM to. If succeeded,
> >>the guest switches to dma_direct_ops and never calls TCE hypercalls
> >>(H_PUT_TCE,...) again. This enables VFIO devices to use the entire RAM
> >>and not waste time on map/unmap later.
> >>
> >>This adds 4 RTAS handlers:
> >>* ibm,query-pe-dma-window
> >>* ibm,create-pe-dma-window
> >>* ibm,remove-pe-dma-window
> >>* ibm,reset-pe-dma-window
> >>These are registered from type_init() callback.
> >>
> >>These RTAS handlers are implemented in a separate file to avoid polluting
> >>spapr_iommu.c with PCI.
> >>
> >>Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >
> >Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> 
> I saw this and decided there are no more coments but I was wrong :)

Right.  Note that if I add a Reviewed-by but also make comments, then
those comments are seeking clarification and maybe suggesting later
cleanups, but I think the problems are small enough that the patch is
still ready to go as it is.

[snip]
> >>+static void spapr_tce_table_do_enable(sPAPRTCETable *tcet);
> >>+
> >>  static int spapr_tce_table_post_load(void *opaque, int version_id)
> >>  {
> >>      sPAPRTCETable *tcet = SPAPR_TCE_TABLE(opaque);
> >>@@ -98,22 +107,42 @@ static int spapr_tce_table_post_load(void *opaque, int version_id)
> >>          spapr_vio_set_bypass(tcet->vdev, tcet->bypass);
> >>      }
> >>
> >>+    if (!tcet->migtable) {
> >
> >What's the case where migtable will be NULL?  IIUC an old->new
> >migration will result in the data saved for "table" being loaded into
> >"migtable".
> >
> >So "migtable" should only be NULL, when tce->enabled is also false?
> 
> 
> Seems to be true and this is just extra precaution. Remove?

Yes, remove as a cleanup, but that can be done later, I won't hold up
the main patch series for this.

-- 
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: Type: application/pgp-signature, Size: 819 bytes --]