Re: [Qemu-ppc] [PATCH v6 1/2] spapr: Add a "no HPT" encoding to HTAB migration stream

[Bharata B Rao]

On Mon, Jun 12, 2017 at 05:10:44PM +0800, David Gibson wrote:
> On Mon, Jun 12, 2017 at 11:02:34AM +0530, Bharata B Rao wrote:
> > Add a "no HPT" encoding (using value -1) to the HTAB migration
> > stream (in the place of HPT size) when the guest doesn't allocate HPT.
> > This will help the target side to match target HPT with the source HPT
> > and thus enable successful migration.
> > 
> > Suggested-by: David Gibson <address@hidden>
> > Signed-off-by: Bharata B Rao <address@hidden>
> > ---
> >  hw/ppc/spapr.c | 29 +++++++++++++++++++++++++----
> >  1 file changed, 25 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 8b541d9..c425499 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1558,13 +1558,19 @@ static int htab_save_setup(QEMUFile *f, void 
> > *opaque)
> >      sPAPRMachineState *spapr = opaque;
> >  
> >      /* "Iteration" header */
> > -    qemu_put_be32(f, spapr->htab_shift);
> > +    if (!spapr->htab_shift) {
> > +        qemu_put_be32(f, -1);
> > +    } else {
> > +        qemu_put_be32(f, spapr->htab_shift);
> > +    }
> >  
> >      if (spapr->htab) {
> >          spapr->htab_save_index = 0;
> >          spapr->htab_first_pass = true;
> >      } else {
> > -        assert(kvm_enabled());
> > +        if (spapr->htab_shift) {
> > +            assert(kvm_enabled());
> > +        }
> >      }
> >  
> >  
> > @@ -1710,7 +1716,12 @@ static int htab_save_iterate(QEMUFile *f, void 
> > *opaque)
> >      int rc = 0;
> >  
> >      /* Iteration header */
> > -    qemu_put_be32(f, 0);
> > +    if (!spapr->htab_shift) {
> > +        qemu_put_be32(f, -1);
> > +        return 0;
> > +    } else {
> > +        qemu_put_be32(f, 0);
> > +    }
> >  
> >      if (!spapr->htab) {
> >          assert(kvm_enabled());
> > @@ -1744,7 +1755,12 @@ static int htab_save_complete(QEMUFile *f, void 
> > *opaque)
> >      int fd;
> >  
> >      /* Iteration header */
> > -    qemu_put_be32(f, 0);
> > +    if (!spapr->htab_shift) {
> > +        qemu_put_be32(f, -1);
> > +        return 0;
> > +    } else {
> > +        qemu_put_be32(f, 0);
> > +    }
> 
> Do you actually need the modifications for _iterate and _complete?  I
> would have thought you just wouldn't need to send any more of the HPT
> stream at all after sending the -1 header.  

_setup, _iterate, _complete handler routines for HTAB always get interspersed
with similar routines for ram savevm handlers as per what I have seen.
And moreover these are called by the core migration code and hence we they get
called, we need these changes to ensure that we don't attempt to send HPT
stream.

> 
> We should also adjust the downtime estimation logic so we don't allow
> for transferring an HPT that isn't there.

I will have to check that out.

> 
> >      if (!spapr->htab) {
> >          int rc;
> > @@ -1788,6 +1804,11 @@ static int htab_load(QEMUFile *f, void *opaque, int 
> > version_id)
> >  
> >      section_hdr = qemu_get_be32(f);
> >  
> > +    if (section_hdr == -1) {
> > +        spapr_free_hpt(spapr);
> > +        return 0;
> > +    }
> 
> Strictly speaking we probably shouldn't just return here.  We should
> wait and see if there is more data in the stream.

Because of the way the source sends data (with HPT data getting
interspersed with ram data, I don't think we can say for sure if
we got or will get any HPT data following the no-HPT indication.

> Any actual content
> (i.e. section_hdr == 0 sections) would be an error at this point.
> However, a reset to an HPT guest would be represented by a new
> non-zero section header, then more data.  This isn't urgent, but it
> would be nice to fix at some point.

Sure.

Regards,
Bharata.