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

[Alexander Graf <agraf@suse.de>]

On 05.11.14 23:00, Samuel Mendoza-Jonas wrote:
> 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 <sam.mj@au1.ibm.com>
>>> ---
>>>  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!

Yes, please. I would prefer to keep this code as simple as I can ;). And
the less corner cases we need to watch out for, the easier it becomes.


Alex