Re: [Xen-devel] [PATCH 06 of 14] Don't lose track of the log dirty bitmap

["Andres Lagar-Cavilla" <andres@xxxxxxxxxxxxxxxx>]

Certainly, looks good. Let me give it a quick test. I was
enabling-dsabling-enablig log dirty, and that was crashing my box on the
ASSERT that was checking no dirty bitmap pages were left when tearing
down.

Andres
> At 16:11 -0500 on 23 Nov (1322064673), Andres Lagar-Cavilla wrote:
>> hap_log_dirty_init unconditionally sets the top of the log dirty
>> bitmap to INVALID_MFN. If there had been a bitmap allocated, it is
>> then leaked, and the host crashes on an ASSERT when the domain is
>> cleaned up. Fixing it here.
>>
>> Signed-off-by: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>
>>
>> diff -r e8f0709af2b7 -r e22610ef339a xen/arch/x86/mm/hap/hap.c
>> --- a/xen/arch/x86/mm/hap/hap.c
>> +++ b/xen/arch/x86/mm/hap/hap.c
>> @@ -224,11 +224,18 @@ static void hap_clean_dirty_bitmap(struc
>>  void hap_logdirty_init(struct domain *d)
>>  {
>>      struct sh_dirty_vram *dirty_vram = d->arch.hvm_domain.dirty_vram;
>> -    if ( paging_mode_log_dirty(d) && dirty_vram )
>> +    if ( paging_mode_log_dirty(d) )
>>      {
>> -        paging_log_dirty_disable(d);
>> -        xfree(dirty_vram);
>> -        dirty_vram = d->arch.hvm_domain.dirty_vram = NULL;
>> +        if ( dirty_vram )
>> +        {
>> +            paging_log_dirty_disable(d);
>> +            xfree(dirty_vram);
>> +            dirty_vram = d->arch.hvm_domain.dirty_vram = NULL;
>> +        } else {
>> +            /* We still need to clean up the bitmap, because
>> +             * init below will overwrite it */
>> +            paging_free_log_dirty_bitmap(d);
>
> That's not safe - in the particular case you're worried about there are
> concurrent users of this bitmap.  I think the only sane way to deal with
> this is to make sure the initialization of log_dirty.top to INVALID_MFN
> happens once at start of day, and remove it from the functions that get
> called mulitple times.
>
> Something like this:
>
> diff -r f71ff2f7b604 xen/arch/x86/mm/paging.c
> --- a/xen/arch/x86/mm/paging.c        Thu Nov 24 15:20:57 2011 +0000
> +++ b/xen/arch/x86/mm/paging.c        Thu Nov 24 16:05:46 2011 +0000
> @@ -595,7 +595,6 @@ void paging_log_dirty_init(struct domain
>      d->arch.paging.log_dirty.enable_log_dirty = enable_log_dirty;
>      d->arch.paging.log_dirty.disable_log_dirty = disable_log_dirty;
>      d->arch.paging.log_dirty.clean_dirty_bitmap = clean_dirty_bitmap;
> -    d->arch.paging.log_dirty.top = _mfn(INVALID_MFN);
>  }
>
>  /* This function fress log dirty bitmap resources. */
> @@ -617,6 +616,11 @@ int paging_domain_init(struct domain *d,
>
>      mm_lock_init(&d->arch.paging.lock);
>
> +    /* This must be initialized separately from the rest of the
> +     * log-dirty init code as that can be called more than once and we
> +     * don't want to leak any active log-dirty bitmaps */
> +    d->arch.paging.log_dirty.top = _mfn(INVALID_MFN);
> +
>      /* The order of the *_init calls below is important, as the later
>       * ones may rewrite some common fields.  Shadow pagetables are the
>       * default... */
>
> Does that make sense to you?
>
> Tim.
>



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel