Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/2] spapr: Fix stale HTAB during live migration (TCG)

[Samuel Mendoza-Jonas]

On 05/11/14 19:05, Alexander Graf wrote:
> 
> 
> On 05.11.14 07:17, Samuel Mendoza-Jonas wrote:
>> If a TCG guest reboots during a running migration HTAB entries are not
>> marked dirty, and the destination boots with an invalid HTAB.
>>
>> When a reboot occurs reset the state of HTAB migration, and explicitly
>> inform the destination of invalid entries.
>>
>> Signed-off-by: Samuel Mendoza-Jonas <address@hidden>
>> ---
>>  hw/ppc/spapr.c         | 59 
>> +++++++++++++++++++++++++++++++++++---------------
>>  include/hw/ppc/spapr.h |  1 +
>>  2 files changed, 42 insertions(+), 18 deletions(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 1610c28..9f419e8 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -829,26 +829,30 @@ static void spapr_reset_htab(sPAPREnvironment *spapr)
>>  
>>      shift = kvmppc_reset_htab(spapr->htab_shift);
>>  
>> +    pthread_mutex_lock(&spapr->htab_mutex);
>>      if (shift > 0) {
>>          /* Kernel handles htab, we don't need to allocate one */
>>          spapr->htab_shift = shift;
>>          kvmppc_kern_htab = true;
>>  
>>          /* Tell readers to update their file descriptor */
>> -        pthread_mutex_lock(&spapr->htab_mutex);
>>          if (spapr->htab_fd > 0) {
>>              spapr->htab_fd_stale = true;
>>          }
>> -        pthread_mutex_unlock(&spapr->htab_mutex);
>>      } else {
>>          if (!spapr->htab) {
>>              /* Allocate an htab if we don't yet have one */
>>              spapr->htab = qemu_memalign(HTAB_SIZE(spapr), HTAB_SIZE(spapr));
>> +        } else {
>> +            spapr->htab_mig_full = true;
>> +            spapr->htab_first_pass = true;
>> +            spapr->htab_save_index = 0;
> 
> You could just set the dirty bitmap to "all dirty" here, no? Then you
> don't need all the changes belong I presume?

Yes, then you just need to reset htab_save_index to zero. The idea of this 
approach
was to avoid walking the HTAB twice (once to dirty and once to read it). But it 
is
a lot of changes for a fairly small benefit. If setting it dirty is preferred 
I'll
test and send that version. Thanks!
> 
>>          }
>>  
>>          /* And clear it */
>>          memset(spapr->htab, 0, HTAB_SIZE(spapr));
> 
> ... so instead of memset(0)ing it, you could just
> 
>   ppc_hash64_store_hpte(env, i, HPTE64_V_HPTE_DIRTY, 0);
> 
> the HTAB in a loop.
> 
> 
> Alex
> 


-- 
-----------
LTC Ozlabs
IBM