Re: [Qemu-devel] [PATCH V3] spapr: Fix stale HTAB during live migration

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Samuel Mendoza-Jonas (sam.mj@au1.ibm.com) wrote:
> If a guest reboots during a running migration, changes to the
> hash page table are not necessarily updated on the destination.
> Opening a new file descriptor to the HTAB forces the migration
> handler to resend the entire table.

Yes I think that's safe.

> Signed-off-by: Samuel Mendoza-Jonas <sam.mj@au1.ibm.com>
> ---
> Changes in v3: Pointed out by David, htab_save_iterate could
> 	potentially try to read before htab_fd is open again.
> 	Leave opening the fd to the functions trying to read.
> Changes in v2: Forgot check on kvmppc_get_htab_fd return value
>  hw/ppc/spapr.c         | 25 +++++++++++++++++++++++++
>  include/hw/ppc/spapr.h |  1 +
>  2 files changed, 26 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 3a6d26d..5b41318 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -997,6 +997,10 @@ static void spapr_reset_htab(sPAPREnvironment *spapr)
>          /* Kernel handles htab, we don't need to allocate one */
>          spapr->htab_shift = shift;
>          kvmppc_kern_htab = true;
> +
> +        /* Check if we are overlapping a migration */
> +        if (spapr->htab_fd > 0)
> +            spapr->need_reset = true;
>      } else {
>          if (!spapr->htab) {
>              /* Allocate an htab if we don't yet have one */
> @@ -1156,6 +1160,7 @@ static int htab_save_setup(QEMUFile *f, void *opaque)
>      } else {
>          assert(kvm_enabled());
>  
> +        spapr->need_reset = false;
>          spapr->htab_fd = kvmppc_get_htab_fd(false);
>          if (spapr->htab_fd < 0) {
>              fprintf(stderr, "Unable to open fd for reading hash table from KVM: %s\n",
> @@ -1309,6 +1314,16 @@ static int htab_save_iterate(QEMUFile *f, void *opaque)
>      if (!spapr->htab) {
>          assert(kvm_enabled());
>  
> +        if (atomic_cmpxchg(&spapr->need_reset, true, false) == true) {
> +            close(spapr->htab_fd);
> +            spapr->htab_fd = kvmppc_get_htab_fd(false);
> +            if (spapr->htab_fd < 0) {
> +                fprintf(stderr, "Unable to open fd for reading hash table from KVM: %s\n",
> +                        strerror(errno));

Either perror or error_report() with the strerror would seem better.

> +                return -1;
> +            }
> +        }
> +

Why not make a little function for this; it seems a bad idea to have two copies of
it.
Also, add a comment saying why you're reopening it.

Dave

>          rc = kvmppc_save_htab(f, spapr->htab_fd,
>                                MAX_KVM_BUF_SIZE, MAX_ITERATION_NS);
>          if (rc < 0) {
> @@ -1340,6 +1355,16 @@ static int htab_save_complete(QEMUFile *f, void *opaque)
>  
>          assert(kvm_enabled());
>  
> +        if (atomic_cmpxchg(&spapr->need_reset, true, false) == true) {
> +            close(spapr->htab_fd);
> +            spapr->htab_fd = kvmppc_get_htab_fd(false);
> +            if (spapr->htab_fd < 0) {
> +                fprintf(stderr, "Unable to open fd for reading hash table from KVM: %s\n",
> +                        strerror(errno));
> +                return -1;
> +            }
> +        }
> +
>          rc = kvmppc_save_htab(f, spapr->htab_fd, MAX_KVM_BUF_SIZE, -1);
>          if (rc < 0) {
>              return rc;
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 0c2e3c5..9ab9827 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -71,6 +71,7 @@ typedef struct sPAPREnvironment {
>      int htab_save_index;
>      bool htab_first_pass;
>      int htab_fd;
> +    bool need_reset;
>  
>      /* state for Dynamic Reconfiguration Connectors */
>      sPAPRDrcEntry drc_table[SPAPR_DRC_TABLE_SIZE];
> -- 
> 1.9.3
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK