[PATCH 00 of 14] MM Fixes

[PATCH 00 of 14] MM Fixes

[Andres Lagar-Cavilla]

This patch series includes a number of bug fixes, targetting 
primarily the mm layer. Many of these fixes also lay groundwork
for future patches.

Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
Signed-off-by: Adin Scannell <adin@scannell.ca>

 xen/arch/x86/mm/p2m-pod.c          |   1 -
 xen/arch/x86/mm/p2m-pt.c           |   9 +++-
 xen/arch/x86/mm/p2m.c              |   5 +-
 xen/arch/x86/mm/hap/hap.c          |   2 +
 tools/libxc/xc_domain.c            |   2 +
 xen/arch/x86/mm/shadow/common.c    |   4 +-
 xen/arch/x86/mm/hap/hap.c          |  15 ++++++--
 xen/arch/x86/mm/hap/hap.c          |   2 +-
 xen/drivers/char/ns16550.c         |   2 +-
 xen/arch/x86/mm/p2m.c              |   4 +-
 xen/include/asm-x86/p2m.h          |   3 +-
 xen/arch/x86/hvm/hvm.c             |  10 ++++-
 xen/arch/x86/mm.c                  |   5 ++-
 xen/arch/x86/hvm/hvm.c             |  69 ++++++++++++++++++++++++-------------
 xen/arch/x86/hvm/nestedhvm.c       |   4 +-
 xen/arch/x86/hvm/svm/nestedsvm.c   |  23 ++++++------
 xen/arch/x86/hvm/vmx/vvmx.c        |  36 +++++++++++--------
 xen/include/asm-x86/hvm/hvm.h      |   9 +++-
 xen/include/asm-x86/hvm/vcpu.h     |   1 +
 xen/include/asm-x86/hvm/vmx/vvmx.h |   1 +
 xen/arch/x86/mm/guest_walk.c       |  16 ++++++--
 xen/common/grant_table.c           |  69 ++++++++++++++++++++-----------------
 22 files changed, 182 insertions(+), 110 deletions(-)

[PATCH 01 of 14] The PoD code may split a 1GB superpage in a potentially unlocked way

[Andres Lagar-Cavilla]

 xen/arch/x86/mm/p2m-pod.c |  1 -
 xen/arch/x86/mm/p2m-pt.c  |  9 ++++++---
 2 files changed, 6 insertions(+), 4 deletions(-)


The path p2m-lookup -> p2m-pt->get_entry -> 1GB PoD superpage ->
pod_demand_populate ends in the pod code performing a p2m_set_entry with
no locks held (in order to split the 1GB superpage into 512 2MB ones)

Further, it calls p2m_unlock after that, which will break the spinlock.

This patch attempts to fix that.

Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
Acked-by: George Dunlap <george.dunlap@eu.citrix.com>

diff -r 909f13636400 -r 00c1834b2882 xen/arch/x86/mm/p2m-pod.c
--- a/xen/arch/x86/mm/p2m-pod.c
+++ b/xen/arch/x86/mm/p2m-pod.c
@@ -987,7 +987,6 @@ p2m_pod_demand_populate(struct p2m_domai
         set_p2m_entry(p2m, gfn_aligned, _mfn(0), PAGE_ORDER_2M,
                       p2m_populate_on_demand, p2m->default_access);
         audit_p2m(p2m, 1);
-        p2m_unlock(p2m);
         return 0;
     }
 
diff -r 909f13636400 -r 00c1834b2882 xen/arch/x86/mm/p2m-pt.c
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -542,10 +542,11 @@ pod_retry_l3:
             /* The read has succeeded, so we know that mapping exists */
             if ( q != p2m_query )
             {
-                if ( !p2m_pod_demand_populate(p2m, gfn, PAGE_ORDER_1G, q) )
+                if ( !p2m_pod_check_and_populate(p2m, gfn,
+                                      (l1_pgentry_t *) &l3e, PAGE_ORDER_1G, q) )
                     goto pod_retry_l3;
                 p2mt = p2m_invalid;
-                printk("%s: Allocate 1GB failed!\n", __func__);
+                gdprintk(XENLOG_ERR, "%s: Allocate 1GB failed!\n", __func__);
                 goto out;
             }
             else
@@ -743,8 +744,10 @@ pod_retry_l3:
             {
                 if ( q != p2m_query )
                 {
-                    if ( !p2m_pod_demand_populate(p2m, gfn, PAGE_ORDER_1G, q) )
+                    if ( !p2m_pod_check_and_populate(p2m, gfn,
+                                      (l1_pgentry_t *) l3e, PAGE_ORDER_1G, q) )
                         goto pod_retry_l3;
+                    gdprintk(XENLOG_ERR, "%s: Allocate 1GB failed!\n", __func__);
                 }
                 else
                     *t = p2m_populate_on_demand;

[PATCH 02 of 14] Fix handling of m2p map in set_shared_p2m_entry

[Andres Lagar-Cavilla]

 xen/arch/x86/mm/p2m.c |  5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)


When updating a p2m mapping to shared, previous code
unconditionally set the m2p entry for the old mfn to invalid.
We now check that the old mfn does not remain shared.

Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

diff -r 00c1834b2882 -r b85ecdc5aa83 xen/arch/x86/mm/p2m.c
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -714,8 +714,9 @@ set_shared_p2m_entry(struct domain *d, u
      * sharable first */
     ASSERT(p2m_is_shared(ot));
     ASSERT(mfn_valid(omfn));
-    /* XXX: M2P translations have to be handled properly for shared pages */
-    set_gpfn_from_mfn(mfn_x(omfn), INVALID_M2P_ENTRY);
+    if ( ((mfn_to_page(omfn)->u.inuse.type_info & PGT_type_mask) 
+                    != PGT_shared_page) )
+        set_gpfn_from_mfn(mfn_x(omfn), INVALID_M2P_ENTRY);
 
     P2M_DEBUG("set shared %lx %lx\n", gfn, mfn_x(mfn));
     rc = set_p2m_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2m_ram_shared, p2m->default_access);

[PATCH 03 of 14] Make HAP log dirty disable return the correct rc

[Andres Lagar-Cavilla]

 xen/arch/x86/mm/hap/hap.c |  2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)


Disabling log dirty mode in HAP always returns -EINVAL. Make it
return the correct rc on success.

Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

diff -r b85ecdc5aa83 -r 58eae300fa63 xen/arch/x86/mm/hap/hap.c
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -709,6 +709,8 @@ int hap_domctl(struct domain *d, xen_dom
         return rc;
     case XEN_DOMCTL_SHADOW_OP_GET_ALLOCATION:
         sc->mb = hap_get_allocation(d);
+        /* Fall through */
+    case XEN_DOMCTL_SHADOW_OP_OFF:
         return 0;
     default:
         HAP_ERROR("Bad hap domctl op %u\n", sc->op);

[PATCH 04 of 14] When passing no bitmap for the shadow log dirty bitmap clean up, we should not get EFAULT

[Andres Lagar-Cavilla]

 tools/libxc/xc_domain.c |  2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)


This is due to a stale check for guest_handle_null in the hypervisor, which doesn't
necessarily work with the hypercall buffers.

Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

diff -r 58eae300fa63 -r 93066bdc1e1c tools/libxc/xc_domain.c
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -430,6 +430,8 @@ int xc_shadow_control(xc_interface *xch,
     DECLARE_DOMCTL;
     DECLARE_HYPERCALL_BUFFER_ARGUMENT(dirty_bitmap);
 
+    memset(&domctl, 0, sizeof(domctl));
+
     domctl.cmd = XEN_DOMCTL_shadow_op;
     domctl.domain = (domid_t)domid;
     domctl.u.shadow_op.op     = sop;

[PATCH 05 of 14] Don't trigger unnecessary shadow scans on p2m entry update

[Andres Lagar-Cavilla]

 xen/arch/x86/mm/shadow/common.c |  4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)


When updating a p2m entry, the hypervisor scans all shadow pte's to find
mappings of that gfn and tear them down. This is avoided if the page count
reveals that there are no additional mappings. The current test ignores the
PGC_allocated flag and its effect on the page count.

Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
Signed-off-by: Adin Scannell <adin@scannell.ca>

diff -r 93066bdc1e1c -r e8f0709af2b7 xen/arch/x86/mm/shadow/common.c
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -2501,7 +2501,8 @@ int sh_remove_all_mappings(struct vcpu *
         ;
 
     perfc_incr(shadow_mappings);
-    if ( (page->count_info & PGC_count_mask) == 0 )
+    expected_count = (page->count_info & PGC_allocated) ? 1 : 0;
+    if ( (page->count_info & PGC_count_mask) == expected_count )
         return 0;
 
     /* Although this is an externally visible function, we do not know
@@ -2517,7 +2518,6 @@ int sh_remove_all_mappings(struct vcpu *
     hash_foreach(v, callback_mask, callbacks, gmfn);
 
     /* If that didn't catch the mapping, something is very wrong */
-    expected_count = (page->count_info & PGC_allocated) ? 1 : 0;
     if ( (page->count_info & PGC_count_mask) != expected_count )
     {
         /* Don't complain if we're in HVM and there are some extra mappings: 

Re: [PATCH 05 of 14] Don't trigger unnecessary shadow scans on p2m entry update

[Jan Beulich]

 >>> On 23.11.11 at 22:11, Andres Lagar-Cavilla <andres@lagarcavilla.org> wrote:
> xen/arch/x86/mm/shadow/common.c |  4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> 
> When updating a p2m entry, the hypervisor scans all shadow pte's to find
> mappings of that gfn and tear them down. This is avoided if the page count
> reveals that there are no additional mappings. The current test ignores the
> PGC_allocated flag and its effect on the page count.
> 
> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> Signed-off-by: Adin Scannell <adin@scannell.ca>
> 
> diff -r 93066bdc1e1c -r e8f0709af2b7 xen/arch/x86/mm/shadow/common.c
> --- a/xen/arch/x86/mm/shadow/common.c
> +++ b/xen/arch/x86/mm/shadow/common.c
> @@ -2501,7 +2501,8 @@ int sh_remove_all_mappings(struct vcpu *
>          ;
>  
>      perfc_incr(shadow_mappings);
> -    if ( (page->count_info & PGC_count_mask) == 0 )
> +    expected_count = (page->count_info & PGC_allocated) ? 1 : 0;
> +    if ( (page->count_info & PGC_count_mask) == expected_count )

Is that really valid outside the locked region?

>          return 0;
>  
>      /* Although this is an externally visible function, we do not know
> @@ -2517,7 +2518,6 @@ int sh_remove_all_mappings(struct vcpu *
>      hash_foreach(v, callback_mask, callbacks, gmfn);
>  
>      /* If that didn't catch the mapping, something is very wrong */
> -    expected_count = (page->count_info & PGC_allocated) ? 1 : 0;

This certainly isn't right - iiuc the count would normally have changed
during the hash_foreach() above this.

Jan

>      if ( (page->count_info & PGC_count_mask) != expected_count )
>      {
>          /* Don't complain if we're in HVM and there are some extra 
> mappings: 

Re: [PATCH 05 of 14] Don't trigger unnecessary shadow scans on p2m entry update

[Jan Beulich]

 >>> On 24.11.11 at 10:48, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>> On 23.11.11 at 22:11, Andres Lagar-Cavilla <andres@lagarcavilla.org> wrote:
>> xen/arch/x86/mm/shadow/common.c |  4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>> 
>> 
>> When updating a p2m entry, the hypervisor scans all shadow pte's to find
>> mappings of that gfn and tear them down. This is avoided if the page count
>> reveals that there are no additional mappings. The current test ignores the
>> PGC_allocated flag and its effect on the page count.
>> 
>> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
>> Signed-off-by: Adin Scannell <adin@scannell.ca>
>> 
>> diff -r 93066bdc1e1c -r e8f0709af2b7 xen/arch/x86/mm/shadow/common.c
>> --- a/xen/arch/x86/mm/shadow/common.c
>> +++ b/xen/arch/x86/mm/shadow/common.c
>> @@ -2501,7 +2501,8 @@ int sh_remove_all_mappings(struct vcpu *
>>          ;
>>  
>>      perfc_incr(shadow_mappings);
>> -    if ( (page->count_info & PGC_count_mask) == 0 )
>> +    expected_count = (page->count_info & PGC_allocated) ? 1 : 0;
>> +    if ( (page->count_info & PGC_count_mask) == expected_count )
> 
> Is that really valid outside the locked region?
> 
>>          return 0;
>>  
>>      /* Although this is an externally visible function, we do not know
>> @@ -2517,7 +2518,6 @@ int sh_remove_all_mappings(struct vcpu *
>>      hash_foreach(v, callback_mask, callbacks, gmfn);
>>  
>>      /* If that didn't catch the mapping, something is very wrong */
>> -    expected_count = (page->count_info & PGC_allocated) ? 1 : 0;
> 
> This certainly isn't right - iiuc the count would normally have changed
> during the hash_foreach() above this.

Hmm, I was wrong here, you aren't caching the page count. Still, is it
certain that the state of PGC_allocated doesn't change from where
you set expected_count now to where it was set before?

Jan

Re: [PATCH 05 of 14] Don't trigger unnecessary shadow scans on p2m entry update

[Andres Lagar-Cavilla]

Jan,
>  >>> On 24.11.11 at 10:48, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>>> On 23.11.11 at 22:11, Andres Lagar-Cavilla <andres@lagarcavilla.org>
>>>>> wrote:
>>> xen/arch/x86/mm/shadow/common.c |  4 ++--
>>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>>
>>> When updating a p2m entry, the hypervisor scans all shadow pte's to
>>> find
>>> mappings of that gfn and tear them down. This is avoided if the page
>>> count
>>> reveals that there are no additional mappings. The current test ignores
>>> the
>>> PGC_allocated flag and its effect on the page count.
>>>
>>> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
>>> Signed-off-by: Adin Scannell <adin@scannell.ca>
>>>
>>> diff -r 93066bdc1e1c -r e8f0709af2b7 xen/arch/x86/mm/shadow/common.c
>>> --- a/xen/arch/x86/mm/shadow/common.c
>>> +++ b/xen/arch/x86/mm/shadow/common.c
>>> @@ -2501,7 +2501,8 @@ int sh_remove_all_mappings(struct vcpu *
>>>          ;
>>>
>>>      perfc_incr(shadow_mappings);
>>> -    if ( (page->count_info & PGC_count_mask) == 0 )
>>> +    expected_count = (page->count_info & PGC_allocated) ? 1 : 0;
>>> +    if ( (page->count_info & PGC_count_mask) == expected_count )
>>
>> Is that really valid outside the locked region?
We can move it below to be protected by the lock.

>>
>>>          return 0;
>>>
>>>      /* Although this is an externally visible function, we do not know
>>> @@ -2517,7 +2518,6 @@ int sh_remove_all_mappings(struct vcpu *
>>>      hash_foreach(v, callback_mask, callbacks, gmfn);
>>>
>>>      /* If that didn't catch the mapping, something is very wrong */
>>> -    expected_count = (page->count_info & PGC_allocated) ? 1 : 0;
>>
>> This certainly isn't right - iiuc the count would normally have changed
>> during the hash_foreach() above this.

I don't think that's a problem. The expected count is still the same. The
shadow scan callback will not change the PGC_allocated flag in the target
page. Further, the sh_rm_mappings_from_l1 should also be updated. It
breaks out of the loop on a count of zero, but it should also break out on
1 | PGC_allocated.

I'll resend this one.
Andres
>
> Hmm, I was wrong here, you aren't caching the page count. Still, is it
> certain that the state of PGC_allocated doesn't change from where
> you set expected_count now to where it was set before?
>
> Jan
>
>

Re: [PATCH 05 of 14] Don't trigger unnecessary shadow scans on p2m entry update

[Tim Deegan]

At 07:24 -0800 on 24 Nov (1322119466), Andres Lagar-Cavilla wrote:
> >>> @@ -2501,7 +2501,8 @@ int sh_remove_all_mappings(struct vcpu *
> >>>          ;
> >>>
> >>>      perfc_incr(shadow_mappings);
> >>> -    if ( (page->count_info & PGC_count_mask) == 0 )
> >>> +    expected_count = (page->count_info & PGC_allocated) ? 1 : 0;
> >>> +    if ( (page->count_info & PGC_count_mask) == expected_count )
> >>
> >> Is that really valid outside the locked region?
> We can move it below to be protected by the lock.

It doesn't need to be protected by the lock.  All we care about is that
at some point after the p2m update, the count has fallen to the correct
value.

It would probably be better to explicitly snapshot 
(page->count_info & (PGC_count_mask | PGC_allocated) and make sure that
that is either 0 or (1 | PGC_allocated).  (Yes, the code you're copying
from does it the other way, and it probably shouldn't either).

> >>
> >>>          return 0;
> >>>
> >>>      /* Although this is an externally visible function, we do not know
> >>> @@ -2517,7 +2518,6 @@ int sh_remove_all_mappings(struct vcpu *
> >>>      hash_foreach(v, callback_mask, callbacks, gmfn);
> >>>
> >>>      /* If that didn't catch the mapping, something is very wrong */
> >>> -    expected_count = (page->count_info & PGC_allocated) ? 1 : 0;
> >>
> >> This certainly isn't right - iiuc the count would normally have changed
> >> during the hash_foreach() above this.
> 
> I don't think that's a problem. The expected count is still the same. The
> shadow scan callback will not change the PGC_allocated flag in the target
> page.

I don't think there's any interlock stopping another CPU from changing
PGC_allocated, though.  

> Further, the sh_rm_mappings_from_l1 should also be updated. It
> breaks out of the loop on a count of zero, but it should also break out on
> 1 | PGC_allocated.

True.  A helper function that does the snapshot-and-test and is called
from all three places would be an improvement. 

> I'll resend this one.

Thanks.

Tim.

[PATCH 06 of 14] Don't lose track of the log dirty bitmap

[Andres Lagar-Cavilla]

 xen/arch/x86/mm/hap/hap.c |  15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)


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@lagarcavilla.org>

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);
+        }
     }
 
     /* Reinitialize logdirty mechanism */

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

[Tim Deegan]

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@lagarcavilla.org>
> 
> 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.

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

[Andres Lagar-Cavilla]

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@lagarcavilla.org>
>>
>> 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.
>

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

[Andres Lagar-Cavilla]

Confirmed that your (simpler) patch solves the issue
Acked-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

Thanks
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@lagarcavilla.org>
>>
>> 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.
>

[PATCH 07 of 14] Trivial fix for rc val in hap track dirty vram

[Andres Lagar-Cavilla]

 xen/arch/x86/mm/hap/hap.c |  2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)


Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

diff -r e22610ef339a -r 25d27decfdcc xen/arch/x86/mm/hap/hap.c
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -141,7 +141,7 @@ int hap_track_dirty_vram(struct domain *
         }
         else if ( !paging_mode_log_dirty(d) && !dirty_vram )
         {
-            rc -ENOMEM;
+            rc = -ENOMEM;
             if ( (dirty_vram = xmalloc(struct sh_dirty_vram)) == NULL )
                 goto param_fail;

Re: [PATCH 07 of 14] Trivial fix for rc val in hap track dirty vram

[Jan Beulich]

 >>> On 23.11.11 at 22:11, Andres Lagar-Cavilla <andres@lagarcavilla.org> wrote:
> xen/arch/x86/mm/hap/hap.c |  2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> 
> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> 
> diff -r e22610ef339a -r 25d27decfdcc xen/arch/x86/mm/hap/hap.c
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -141,7 +141,7 @@ int hap_track_dirty_vram(struct domain *
>          }
>          else if ( !paging_mode_log_dirty(d) && !dirty_vram )
>          {
> -            rc -ENOMEM;
> +            rc = -ENOMEM;
>              if ( (dirty_vram = xmalloc(struct sh_dirty_vram)) == NULL )
>                  goto param_fail;
>  

This would have been caught by the compiler if we weren't forcing
-Wno-unused-value. Keir, shouldn't we revisit this 5 year old decision
(c/s 11762:db3d58d30e9d)?

Jan

Re: [PATCH 07 of 14] Trivial fix for rc val in hap track dirty vram

[Keir Fraser]

On 24/11/2011 09:59, "Jan Beulich" <JBeulich@suse.com> wrote:

>>>> On 23.11.11 at 22:11, Andres Lagar-Cavilla <andres@lagarcavilla.org> wrote:
>> xen/arch/x86/mm/hap/hap.c |  2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>> 
>> 
>> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
>> 
>> diff -r e22610ef339a -r 25d27decfdcc xen/arch/x86/mm/hap/hap.c
>> --- a/xen/arch/x86/mm/hap/hap.c
>> +++ b/xen/arch/x86/mm/hap/hap.c
>> @@ -141,7 +141,7 @@ int hap_track_dirty_vram(struct domain *
>>          }
>>          else if ( !paging_mode_log_dirty(d) && !dirty_vram )
>>          {
>> -            rc -ENOMEM;
>> +            rc = -ENOMEM;
>>              if ( (dirty_vram = xmalloc(struct sh_dirty_vram)) == NULL )
>>                  goto param_fail;
>>  
> 
> This would have been caught by the compiler if we weren't forcing
> -Wno-unused-value. Keir, shouldn't we revisit this 5 year old decision
> (c/s 11762:db3d58d30e9d)?

Fine with me to allow the warnings now.

 -- Keir

> Jan
> 

[PATCH 08 of 14] Properly compare "pci" token when groking serial port config

[Andres Lagar-Cavilla]

 xen/drivers/char/ns16550.c |  2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)


Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

diff -r 25d27decfdcc -r 7e8211d0f41d xen/drivers/char/ns16550.c
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -554,7 +554,7 @@ static void __init ns16550_parse_port_co
     if ( *conf == ',' )
     {
         conf++;
-        if ( strncmp(conf, "pci", 5) == 0 )
+        if ( strncmp(conf, "pci", 3) == 0 )
         {
             if ( pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com) )
                 return;

Re: [PATCH 08 of 14] Properly compare "pci" token when groking serial port config

[Jan Beulich]

 >>> On 23.11.11 at 22:11, Andres Lagar-Cavilla <andres@lagarcavilla.org> wrote:
> xen/drivers/char/ns16550.c |  2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> 
> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

This regression fix (from c/s 23679:cf7e1746bdb7) should probably
go in right away.

Acked-by: Jan Beulich <jbeulich@suse.com>

> diff -r 25d27decfdcc -r 7e8211d0f41d xen/drivers/char/ns16550.c
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -554,7 +554,7 @@ static void __init ns16550_parse_port_co
>      if ( *conf == ',' )
>      {
>          conf++;
> -        if ( strncmp(conf, "pci", 5) == 0 )
> +        if ( strncmp(conf, "pci", 3) == 0 )
>          {
>              if ( pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com) 
> )
>                  return;

[PATCH 09 of 14] Allow log dirty mode to be used in conjunction with paging

[Andres Lagar-Cavilla]

 xen/arch/x86/mm/p2m.c     |  4 +++-
 xen/include/asm-x86/p2m.h |  3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)


Allow pages typed log dirty to be paged out, and the proper type to
restored when paging pages back in.

Signed-off-by: Andres lagar-Cavilla <andres@lagarcavilla.org>
Signed-off-by: Adin Scannell <adin@scannell.ca>

diff -r 7e8211d0f41d -r 76802e649c2c xen/arch/x86/mm/p2m.c
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1061,7 +1061,9 @@ void p2m_mem_paging_resume(struct domain
         if ( mfn_valid(mfn) && 
              (p2mt == p2m_ram_paging_in || p2mt == p2m_ram_paging_in_start) )
         {
-            set_p2m_entry(p2m, rsp.gfn, mfn, PAGE_ORDER_4K, p2m_ram_rw, a);
+            set_p2m_entry(p2m, rsp.gfn, mfn, PAGE_ORDER_4K, 
+                            paging_mode_log_dirty(d) ? p2m_ram_logdirty : p2m_ram_rw, 
+                            a);
             set_gpfn_from_mfn(mfn_x(mfn), rsp.gfn);
             audit_p2m(p2m, 1);
         }
diff -r 7e8211d0f41d -r 76802e649c2c xen/include/asm-x86/p2m.h
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -150,7 +150,8 @@ typedef enum {
 #define P2M_MAGIC_TYPES (p2m_to_mask(p2m_populate_on_demand))
 
 /* Pageable types */
-#define P2M_PAGEABLE_TYPES (p2m_to_mask(p2m_ram_rw))
+#define P2M_PAGEABLE_TYPES (p2m_to_mask(p2m_ram_rw) \
+                            | p2m_to_mask(p2m_ram_logdirty) )
 
 #define P2M_PAGING_TYPES (p2m_to_mask(p2m_ram_paging_out)        \
                           | p2m_to_mask(p2m_ram_paged)           \

[PATCH 10 of 14] Prevent the hypervisor from BUGging if xc_hvm_modified_memory is called on a shared page

[Andres Lagar-Cavilla]

 xen/arch/x86/hvm/hvm.c |  10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)


Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
Signed-off-by: Adin Scannell <adin@scannell.ca>

diff -r 76802e649c2c -r 667e53a7ad34 xen/arch/x86/hvm/hvm.c
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3755,7 +3755,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
         for ( pfn = a.first_pfn; pfn < a.first_pfn + a.nr; pfn++ )
         {
             p2m_type_t t;
-            mfn_t mfn = get_gfn(d, pfn, &t);
+            mfn_t mfn = get_gfn_unshare(d, pfn, &t);
             if ( p2m_is_paging(t) )
             {
                 p2m_mem_paging_populate(d, pfn);
@@ -3764,8 +3764,16 @@ long do_hvm_op(unsigned long op, XEN_GUE
                 goto param_fail3;
             }
             if( p2m_is_shared(t) )
+            {
+                /* If it insists on not unsharing itself, crash the domain 
+                 * rather than crashing the host down in mark dirty */
                 gdprintk(XENLOG_WARNING,
                          "shared pfn 0x%lx modified?\n", pfn);
+                domain_crash(d);
+                put_gfn(d, pfn);
+                rc = -EINVAL;
+                goto param_fail3;
+            }
             
             if ( mfn_x(mfn) != INVALID_MFN )
             {

[PATCH 11 of 14] ASSERT we are putting the right gfn in the XNEMAPSPACE_gmfn* cases

[Andres Lagar-Cavilla]

 xen/arch/x86/mm.c |  5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)


Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

diff -r 667e53a7ad34 -r a64f63ecfc57 xen/arch/x86/mm.c
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4773,13 +4773,16 @@ static int xenmem_add_to_physmap_once(
     /* Unmap from old location, if any. */
     gpfn = get_gpfn_from_mfn(mfn);
     ASSERT( gpfn != SHARED_M2P_ENTRY );
+    if ( xatp->space == XENMAPSPACE_gmfn ||
+         xatp->space == XENMAPSPACE_gmfn_range )
+        ASSERT( gpfn == gfn );
     if ( gpfn != INVALID_M2P_ENTRY )
         guest_physmap_remove_page(d, gpfn, mfn, PAGE_ORDER_4K);
 
     /* Map at new location. */
     rc = guest_physmap_add_page(d, xatp->gpfn, mfn, PAGE_ORDER_4K);
 
-    /* In the XENMAPSPACE_gmfn, we took a ref and locked the p2m at the top */
+    /* In the XENMAPSPACE_gmfn, we took a ref of the gfn at the top */
     if ( xatp->space == XENMAPSPACE_gmfn ||
          xatp->space == XENMAPSPACE_gmfn_range )
         put_gfn(d, gfn);

Re: [PATCH 00 of 14] MM Fixes

[Jan Beulich]

>>> On 23.11.11 at 22:11, Andres Lagar-Cavilla <andres@lagarcavilla.org> wrote:
> This patch series includes a number of bug fixes, targetting 
> primarily the mm layer. Many of these fixes also lay groundwork
> for future patches.

Only 10 (or 11) of the advertised 14 patches actually arrived - in the
mailing list archive patches 4 and 12-14 are missing, and in my mailbox
I got directly addressed (due to being on Cc) 1-10 and through
xen-devel 1-11.

What's going on here?

Jan

> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> Signed-off-by: Adin Scannell <adin@scannell.ca>
> 
>  xen/arch/x86/mm/p2m-pod.c          |   1 -
>  xen/arch/x86/mm/p2m-pt.c           |   9 +++-
>  xen/arch/x86/mm/p2m.c              |   5 +-
>  xen/arch/x86/mm/hap/hap.c          |   2 +
>  tools/libxc/xc_domain.c            |   2 +
>  xen/arch/x86/mm/shadow/common.c    |   4 +-
>  xen/arch/x86/mm/hap/hap.c          |  15 ++++++--
>  xen/arch/x86/mm/hap/hap.c          |   2 +-
>  xen/drivers/char/ns16550.c         |   2 +-
>  xen/arch/x86/mm/p2m.c              |   4 +-
>  xen/include/asm-x86/p2m.h          |   3 +-
>  xen/arch/x86/hvm/hvm.c             |  10 ++++-
>  xen/arch/x86/mm.c                  |   5 ++-
>  xen/arch/x86/hvm/hvm.c             |  69 ++++++++++++++++++++++++-------------
>  xen/arch/x86/hvm/nestedhvm.c       |   4 +-
>  xen/arch/x86/hvm/svm/nestedsvm.c   |  23 ++++++------
>  xen/arch/x86/hvm/vmx/vvmx.c        |  36 +++++++++++--------
>  xen/include/asm-x86/hvm/hvm.h      |   9 +++-
>  xen/include/asm-x86/hvm/vcpu.h     |   1 +
>  xen/include/asm-x86/hvm/vmx/vvmx.h |   1 +
>  xen/arch/x86/mm/guest_walk.c       |  16 ++++++--
>  xen/common/grant_table.c           |  69 ++++++++++++++++++++-----------------
>  22 files changed, 182 insertions(+), 110 deletions(-)

Re: [PATCH 00 of 14] MM Fixes

[Ian Jackson]

Andres Lagar-Cavilla writes ("[Xen-devel] [PATCH 00 of 14] MM Fixes"):
> This patch series includes a number of bug fixes, targetting 
> primarily the mm layer. Many of these fixes also lay groundwork
> for future patches.

Thanks.  Something seems to have eaten patches 12,13,14.

Can you please confirm that you sent them, and tell me their
messageids, and any information you can tell me about their
transmission ?

Ideally I would like log entries from the final hop mail server on
your side showing the messages being handed over to the MX's for
lists.xensource.com, but looking at the headers of your other messages
they came through "homiemail-***.***.dreamhost.com" so that may
involve talking to whoever they are.

Thanks,
Ian.

Re: [PATCH 00 of 14] MM Fixes

[Andres Lagar-Cavilla]

Not a problem on your side. smtp quota on my side. Resending the final
three separately.
Thanks
Andres
> Andres Lagar-Cavilla writes ("[Xen-devel] [PATCH 00 of 14] MM Fixes"):
>> This patch series includes a number of bug fixes, targetting
>> primarily the mm layer. Many of these fixes also lay groundwork
>> for future patches.
>
> Thanks.  Something seems to have eaten patches 12,13,14.
>
> Can you please confirm that you sent them, and tell me their
> messageids, and any information you can tell me about their
> transmission ?
>
> Ideally I would like log entries from the final hop mail server on
> your side showing the messages being handed over to the MX's for
> lists.xensource.com, but looking at the headers of your other messages
> they came through "homiemail-***.***.dreamhost.com" so that may
> involve talking to whoever they are.
>
> Thanks,
> Ian.
>

Re: [PATCH 00 of 14] MM Fixes

[Tim Deegan]

At 16:11 -0500 on 23 Nov (1322064667), Andres Lagar-Cavilla wrote:
> This patch series includes a number of bug fixes, targetting 
> primarily the mm layer. Many of these fixes also lay groundwork
> for future patches.
> 
> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> Signed-off-by: Adin Scannell <adin@scannell.ca>

Thanks for these
 - 1-4 I addressed in the last round without noticing they'd been reposted
 - 5 and 6 I've commented on separately
 - 7-11 are applied.

Tim.