Re: [Qemu-ppc] [PULL 06/21] spapr: Add a "no HPT" encoding to HTAB migration stream

[Thomas Huth]

On 18.07.2017 05:37, David Gibson wrote:
> On Mon, Jul 17, 2017 at 09:54:45PM +0200, Laurent Vivier wrote:
>> On 30/06/2017 12:46, David Gibson wrote:
>>> From: Bharata B Rao <address@hidden>
>>>
>>> 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>
>>> Signed-off-by: David Gibson <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 52f4e72..f3e0b9b 100644
>>> --- a/hw/ppc/spapr.c
>>> +++ b/hw/ppc/spapr.c
>>> @@ -1560,13 +1560,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());
>>> +        }
>>>      }
>>>  
>>>  
>>> @@ -1712,7 +1718,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());
>>> @@ -1746,7 +1757,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);
>>> +    }
>>>  
>>>      if (!spapr->htab) {
>>>          int rc;
>>> @@ -1790,6 +1806,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;
>>> +    }
>>> +
>>>      if (section_hdr) {
>>>          Error *local_err = NULL;
>>>  
>>>
>>
>> It seems there is a bug in this patch: when using from QEMU console the
>> command "savevm", it never stops and the qcow2 image grows without limit.
>>
>> I think this is because htab_save_iterate() and htab_save_complete()
>> should mark the end of the sequence (the empty one, -1) by returning 1
>> and not 0 (see kvmppc_save_htab()).
> 
> Ah, yes, I think you're right.  The end condition is one of the subtle
> differences between the savevm and migration paths.
> 
>> This fixes the problem for me:
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 970093e..fa01511 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -1827,7 +1827,7 @@ static int htab_save_iterate(QEMUFile *f, void
>> *opaque)
>>      /* Iteration header */
>>      if (!spapr->htab_shift) {
>>          qemu_put_be32(f, -1);
>> -        return 0;
>> +        return 1;
>>      } else {
>>          qemu_put_be32(f, 0);
>>      }
>> @@ -1866,7 +1866,7 @@ static int htab_save_complete(QEMUFile *f, void
>> *opaque)
>>      /* Iteration header */
>>      if (!spapr->htab_shift) {
>>          qemu_put_be32(f, -1);
>> -        return 0;
>> +        return 1;
>>      } else {
>>          qemu_put_be32(f, 0);
>>      }
> 
> Can you polish this up and submit for merge, please?

Could/should we maybe finally have proper #defines or an enum for these
return values? The raw numbers caused trouble in the past already, and
now they caused trouble again ... we should avoid to step into that trap
again in the future if possible.

 Thomas

signature.asc
Description: OpenPGP digital signature