Re: [Qemu-devel] [PATCH qemu v16 10/19] spapr_iommu: Migrate full state

From: David Gibson <david@gibson.dropbear.id.au>
To: Alexey Kardashevskiy <aik@ozlabs.ru>
Cc: qemu-devel@nongnu.org, qemu-ppc@nongnu.org,
	Alexander Graf <agraf@suse.de>,
	Alex Williamson <alex.williamson@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [Qemu-devel] [PATCH qemu v16 10/19] spapr_iommu: Migrate full state
Date: Thu, 26 May 2016 14:01:05 +1000	[thread overview]
Message-ID: <20160526040105.GB17226@voom.fritz.box> (raw)
In-Reply-To: <1462344751-28281-11-git-send-email-aik@ozlabs.ru>

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

On Wed, May 04, 2016 at 04:52:22PM +1000, Alexey Kardashevskiy wrote:
> The source guest could have reallocated the default TCE table and
> migrate bigger/smaller table. This adds reallocation in post_load()
> if the default table size is different on source and destination.
> 
> This adds @bus_offset, @page_shift, @enabled to the migration stream.
> These cannot change without dynamic DMA windows so no change in
> behavior is expected now.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> David Gibson <david@gibson.dropbear.id.au>
> ---
> Changes:
> v15:
> * squashed "migrate full state" into this
> * added missing tcet->mig_nb_table initialization in spapr_tce_table_pre_save()
> * instead of bumping the version, moved extra parameters to subsection
> 
> v14:
> * new to the series
> ---
>  hw/ppc/spapr_iommu.c   | 67 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  include/hw/ppc/spapr.h |  2 ++
>  trace-events           |  2 ++
>  3 files changed, 69 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> index 9bcd3f6..52b1e0d 100644
> --- a/hw/ppc/spapr_iommu.c
> +++ b/hw/ppc/spapr_iommu.c
> @@ -137,33 +137,96 @@ static IOMMUTLBEntry spapr_tce_translate_iommu(MemoryRegion *iommu, hwaddr addr,
>      return ret;
>  }
>  
> +static void spapr_tce_table_pre_save(void *opaque)
> +{
> +    sPAPRTCETable *tcet = SPAPR_TCE_TABLE(opaque);
> +
> +    tcet->mig_table = tcet->table;
> +    tcet->mig_nb_table = tcet->nb_table;
> +
> +    trace_spapr_iommu_pre_save(tcet->liobn, tcet->mig_nb_table,
> +                               tcet->bus_offset, tcet->page_shift);
> +}
> +
> +static void spapr_tce_table_do_enable(sPAPRTCETable *tcet);
> +static void spapr_tce_table_do_disable(sPAPRTCETable *tcet);
> +
>  static int spapr_tce_table_post_load(void *opaque, int version_id)
>  {
>      sPAPRTCETable *tcet = SPAPR_TCE_TABLE(opaque);
> +    uint32_t old_nb_table = tcet->nb_table;
>  
>      if (tcet->vdev) {
>          spapr_vio_set_bypass(tcet->vdev, tcet->bypass);
>      }
>  
> +    if (tcet->enabled) {
> +        if (tcet->nb_table != tcet->mig_nb_table) {
> +            if (tcet->nb_table) {
> +                spapr_tce_table_do_disable(tcet);
> +            }
> +            tcet->nb_table = tcet->mig_nb_table;
> +            spapr_tce_table_do_enable(tcet);
> +        }
> +
> +        memcpy(tcet->table, tcet->mig_table,
> +               tcet->nb_table * sizeof(tcet->table[0]));
> +
> +        free(tcet->mig_table);
> +        tcet->mig_table = NULL;
> +    } else if (tcet->table) {
> +        /* Destination guest has a default table but source does not -> free */
> +        spapr_tce_table_do_disable(tcet);
> +    }
> +
> +    trace_spapr_iommu_post_load(tcet->liobn, old_nb_table, tcet->nb_table,
> +                                tcet->bus_offset, tcet->page_shift);
> +
>      return 0;
>  }
>  
> +static bool spapr_tce_table_ex_needed(void *opaque)
> +{
> +    sPAPRTCETable *tcet = opaque;
> +
> +    return tcet->bus_offset || tcet->page_shift != 0xC;

	|| !tcet->enabled ??

AFAICT you're assuming that the existing tcet on the destination will
be enabled prior to an incoming migration.

> +}
> +
> +static const VMStateDescription vmstate_spapr_tce_table_ex = {
> +    .name = "spapr_iommu_ex",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = spapr_tce_table_ex_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_BOOL(enabled, sPAPRTCETable),

..or could you encode enabled as !!mig_nb_table?

> +        VMSTATE_UINT64(bus_offset, sPAPRTCETable),
> +        VMSTATE_UINT32(page_shift, sPAPRTCETable),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
>  static const VMStateDescription vmstate_spapr_tce_table = {
>      .name = "spapr_iommu",
>      .version_id = 2,
>      .minimum_version_id = 2,
> +    .pre_save = spapr_tce_table_pre_save,
>      .post_load = spapr_tce_table_post_load,
>      .fields      = (VMStateField []) {
>          /* Sanity check */
>          VMSTATE_UINT32_EQUAL(liobn, sPAPRTCETable),
> -        VMSTATE_UINT32_EQUAL(nb_table, sPAPRTCETable),
>  
>          /* IOMMU state */
> +        VMSTATE_UINT32(mig_nb_table, sPAPRTCETable),
>          VMSTATE_BOOL(bypass, sPAPRTCETable),
> -        VMSTATE_VARRAY_UINT32(table, sPAPRTCETable, nb_table, 0, vmstate_info_uint64, uint64_t),
> +        VMSTATE_VARRAY_UINT32_ALLOC(mig_table, sPAPRTCETable, mig_nb_table, 0,
> +                                    vmstate_info_uint64, uint64_t),
>  
>          VMSTATE_END_OF_LIST()
>      },
> +    .subsections = (const VMStateDescription*[]) {
> +        &vmstate_spapr_tce_table_ex,
> +        NULL
> +    }
>  };
>  
>  static MemoryRegionIOMMUOps spapr_iommu_ops = {
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 0140810..d36dda2 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -540,6 +540,8 @@ struct sPAPRTCETable {
>      uint64_t bus_offset;
>      uint32_t page_shift;
>      uint64_t *table;
> +    uint32_t mig_nb_table;
> +    uint64_t *mig_table;
>      bool bypass;
>      bool need_vfio;
>      int fd;
> diff --git a/trace-events b/trace-events
> index d96d344..dd50005 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -1432,6 +1432,8 @@ spapr_iommu_pci_indirect(uint64_t liobn, uint64_t ioba, uint64_t tce, uint64_t i
>  spapr_iommu_pci_stuff(uint64_t liobn, uint64_t ioba, uint64_t tce_value, uint64_t npages, uint64_t ret) "liobn=%"PRIx64" ioba=0x%"PRIx64" tcevalue=0x%"PRIx64" npages=%"PRId64" ret=%"PRId64
>  spapr_iommu_xlate(uint64_t liobn, uint64_t ioba, uint64_t tce, unsigned perm, unsigned pgsize) "liobn=%"PRIx64" 0x%"PRIx64" -> 0x%"PRIx64" perm=%u mask=%x"
>  spapr_iommu_new_table(uint64_t liobn, void *table, int fd) "liobn=%"PRIx64" table=%p fd=%d"
> +spapr_iommu_pre_save(uint64_t liobn, uint32_t nb, uint64_t offs, uint32_t ps) "liobn=%"PRIx64" %"PRIx32" bus_offset=%"PRIx64" ps=%"PRIu32
> +spapr_iommu_post_load(uint64_t liobn, uint32_t pre_nb, uint32_t post_nb, uint64_t offs, uint32_t ps) "liobn=%"PRIx64" %"PRIx32" => %"PRIx32" bus_offset=%"PRIx64" ps=%"PRIu32
>  
>  # hw/ppc/ppc.c
>  ppc_tb_adjust(uint64_t offs1, uint64_t offs2, int64_t diff, int64_t seconds) "adjusted from 0x%"PRIx64" to 0x%"PRIx64", diff %"PRId64" (%"PRId64"s)"

-- 
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: 819 bytes --]