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

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

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

On Wed, Jun 07, 2017 at 01:23:32PM +0530, Bharata B Rao wrote:
> 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.

Yeah.. but you can change that, right.  Have htab_load() remove the
HPT instead, if section_hdr == 0.

Older qemus that don't understand that certainly weren't going to be
able to take an incoming RPT guest anyway.

> 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.

But that makes no sense anyway.  How the destination goes about
allocating or not allocating the HPT shouldn't be the choice of the
source.  Since the source doesn't know what the destination has
already allocated, it can't possibly know if it's of a matching size,
likewise if the destination doesn't get a size, it can't know if the
current size is correct.

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

I really think we can..

> 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.
> 

-- 
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 --]