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

[Bharata B Rao <bharata@linux.vnet.ibm.com>]

On Thu, Jun 01, 2017 at 02:54:48PM +1000, David Gibson wrote:
> On Wed, May 31, 2017 at 04:56:44PM +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.
> > 
> > A few more fixes to enable TCG migration to work correctly are also
> > included in this commit:
> > 
> > - HTAB savevm handlers have a few asserts on kvm_enabled() when
> >   spapr->htab != 0. Convert these into conditional checks as it is now
> >   possible to have no HTAB with TCG radix guests.
> > - htab_save_setup() asserts for kvm_enabled() when spapr->htab != 0.
> >   Remove this as we can't assert this for TCG radix guests.
> > 
> > Suggested-by: David Gibson <david@gibson.dropbear.id.au>
> >               [no HPT encoding suggestion]
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> 
> Looks basically ok, but there are still some details to address.
> 
> > ---
> >  hw/ppc/spapr.c | 31 +++++++++++++++++--------------
> >  1 file changed, 17 insertions(+), 14 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index ab3aab1..b589ed4 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1559,17 +1559,18 @@ static int htab_save_setup(QEMUFile *f, void *opaque)
> >  {
> >      sPAPRMachineState *spapr = opaque;
> >  
> > -    /* "Iteration" header */
> > -    qemu_put_be32(f, spapr->htab_shift);
> > +    /* "Iteration" header: no-HPT or HPT size encoding */
> > +    if (!spapr->htab_shift) {
> > +        qemu_put_be32(f, -1);
> 
> We're already using htab_shift == 0 to represent no HPT in the runtime
> structure; we might as well do the same on the wire.  As a bonus it
> slightly simplifies the logic here.

Non-zero value of iteration header (which is htab_shift) results in
htab_load() at the target to reallocate HTAB.

zero value of iteration header is used by htab_save_iterate() and
htab_save_complete() to tell htab_load() not to freshly allocate HTAB
at the target.

Hence we can't use 0 value to mean no-HPT.

I have addressed the rest of the comments on asserts by ensuring that
those code paths are taken only when HPT is present. v5 has those
changes.

Regards,
Bharata.