MM Fixes
PATCH 1/1 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.
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 1/1 The PoD code may split a 1GB superpage in a potentially unlocked way
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.
xen/arch/x86/mm/p2m-pod.c +0 -1
@@ -987,7 +987,6 @@ p2m_pod_demand_populate(struct p2m_domaiset_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 ; }
xen/arch/x86/mm/p2m-pt.c +6 -3
@@ -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 1/1 Fix handling of m2p map in set_shared_p2m_entry
In reply to Andres Lagar-Cavillaxen/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.
xen/arch/x86/mm/p2m.c +3 -2
@@ -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 1/1 Make HAP log dirty disable return the correct rc
In reply to Andres Lagar-Cavillaxen/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.
xen/arch/x86/mm/hap/hap.c +2 -0
@@ -709,6 +709,8 @@ int hap_domctl(struct domain *d, xen_domreturn 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 1/1 When passing no bitmap for the shadow log dirty bitmap clean up, we should not get EFAULT
In reply to Andres Lagar-Cavillatools/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.
tools/libxc/xc_domain.c +2 -0
@@ -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 1/1 Don't trigger unnecessary shadow scans on p2m entry update
In reply to Andres Lagar-Cavillaxen/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.
xen/arch/x86/mm/shadow/common.c +2 -2
@@ -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
Quoted 17 lines
Quoted 1 line
Quoted 1 line · 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_allocatedflag and its effect on the page count.Signed-off-by Andres Lagar-Cavilla <[email protected]>Signed-off-by Adin Scannell <[email protected]>diff -r
93066bdc1e1c-re8f0709af2b7xen/arch/x86/mm/shadow/common.c
xen/arch/x86/mm/shadow/common.c int sh_remove_all_mappings(struct vcpu * · line 2505
Quoted 6 lines
xen/arch/x86/mm/shadow/common.c +2
-1@@ -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?
xen/arch/x86/mm/shadow/common.c int sh_remove_all_mappings(struct vcpu * · line 2520
Quoted 8 lines
xen/arch/x86/mm/shadow/common.c +0
-1return 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
Quoted 4 lines
xen/arch/x86/mm/shadow/common.c +0
-0if ( (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
Quoted 33 lines
Quoted 18 lines
Quoted 2 lines · Jan Beulich
Quoted 1 line · 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_allocatedflag and its effect on the page count.Signed-off-by Andres Lagar-Cavilla <[email protected]>Signed-off-by Adin Scannell <[email protected]>diff -r
93066bdc1e1c-re8f0709af2b7xen/arch/x86/mm/shadow/common.cxen/arch/x86/mm/shadow/common.c int sh_remove_all_mappings(struct vcpu * · line 2505
Quoted 6 lines
xen/arch/x86/mm/shadow/common.c +2
-1@@ -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?
Quoted 6 lines
xen/arch/x86/mm/shadow/common.c +0
-0return 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);
Quoted 5 lines
xen/arch/x86/mm/shadow/common.c int sh_remove_all_mappings(struct vcpu * · line 2520
Quoted 2 lines
xen/arch/x86/mm/shadow/common.c +0
-1/* 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
Jan,
Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
Quoted 3 lines · Jan Beulich
Quoted 2 lines
On 23.11.11 at 22:11, Andres Lagar-Cavilla <[email protected]> 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 thePGC_allocatedflag and its effect on the page count.Signed-off-by Andres Lagar-Cavilla <[email protected]>Signed-off-by Adin Scannell <[email protected]>diff -r
93066bdc1e1c-re8f0709af2b7xen/arch/x86/mm/shadow/common.cxen/arch/x86/mm/shadow/common.c +0
-0@@ -2501,7 +2501,8 @@ int sh_remove_all_mappings(struct vcpu * ;
Quoted 6 lines
Quoted 6 lines
xen/arch/x86/mm/shadow/common.c int sh_remove_all_mappings(struct vcpu * · line 2505
Quoted 4 lines
xen/arch/x86/mm/shadow/common.c +2
-1perfc_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.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
xen/arch/x86/mm/shadow/common.c +0
-0return 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);
Quoted 5 lines
Quoted 5 lines
xen/arch/x86/mm/shadow/common.c int sh_remove_all_mappings(struct vcpu * · line 2520
Quoted 2 lines
xen/arch/x86/mm/shadow/common.c +0
-1/* 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
Quoted 8 lines
Hmm, I was wrong here, you aren't caching the page count. Still, is it certain that the state of
PGC_allocateddoesn't change from where you setexpected_countnow to where it was set before?Jan
Re: [PATCH 05 of 14] Don't trigger unnecessary shadow scans on p2m entry update
Quoted 10 lines · Andres Lagar-Cavilla
Quoted 9 lines
Quoted 9 lines
xen/arch/x86/mm/shadow/common.c int sh_remove_all_mappings(struct vcpu * · line 2505
Quoted 7 lines
xen/arch/x86/mm/shadow/common.c +2
-1@@ -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).
Quoted 13 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
xen/arch/x86/mm/shadow/common.c +0
-0return 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);Quoted 5 lines
Quoted 5 lines
xen/arch/x86/mm/shadow/common.c int sh_remove_all_mappings(struct vcpu * · line 2520
Quoted 2 lines
xen/arch/x86/mm/shadow/common.c +0
-1/* 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.
Quoted 3 lines
I don't think that's a problem. The expected count is still the same. The shadow scan callback will not change the
PGC_allocatedflag in the target page.
I don't think there's any interlock stopping another CPU from changing PGC_allocated, though.
Quoted 3 lines
Further, the
sh_rm_mappings_from_l1should 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.
Quoted 1 line
I'll resend this one.
Thanks.
Tim.
PATCH 1/1 Don't lose track of the log dirty bitmap
In reply to Andres Lagar-Cavillaxen/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.
xen/arch/x86/mm/hap/hap.c +11 -4
@@ -224,11 +224,18 @@ static void hap_clean_dirty_bitmap(strucvoid 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
Quoted 19 lines · Andres Lagar-Cavilla
hap_log_dirty_initunconditionally sets the top of the log dirty bitmap toINVALID_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 <[email protected]>diff -r
e8f0709af2b7-re22610ef339axen/arch/x86/mm/hap/hap.cxen/arch/x86/mm/hap/hap.c +1
-3@@ -224,11 +224,18 @@ static void hap_clean_dirty_bitmap(strucvoid 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);
xen/arch/x86/mm/hap/hap.c static void hap_clean_dirty_bitmap(struc · line 237
Quoted 10 lines
xen/arch/x86/mm/hap/hap.c +9
-1- 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:
xen/arch/x86/mm/paging.c +5 -1
@@ -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
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
Quoted 67 lines
Quoted 19 lines · Andres Lagar-Cavilla
hap_log_dirty_initunconditionally sets the top of the log dirty bitmap toINVALID_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 <[email protected]>diff -r
e8f0709af2b7-re22610ef339axen/arch/x86/mm/hap/hap.cxen/arch/x86/mm/hap/hap.c +1
-3@@ -224,11 +224,18 @@ static void hap_clean_dirty_bitmap(strucvoid 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);xen/arch/x86/mm/hap/hap.c static void hap_clean_dirty_bitmap(struc · line 237
Quoted 10 lines
xen/arch/x86/mm/hap/hap.c +9
-1- 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_MFNhappens once at start of day, and remove it from the functions that get called mulitple times.Something like this:
diff -r
f71ff2f7b604xen/arch/x86/mm/paging.cxen/arch/x86/mm/paging.c +5
-1@@ -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.
Andres Lagar-Cavilla
Acked-byRe: [PATCH 06 of 14] Don't lose track of the log dirty bitmap
In reply to Tim DeeganConfirmed that your (simpler) patch solves the issue
Thanks
Andres
Quoted 67 lines
Quoted 19 lines · Andres Lagar-Cavilla
hap_log_dirty_initunconditionally sets the top of the log dirty bitmap toINVALID_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 <[email protected]>diff -r
e8f0709af2b7-re22610ef339axen/arch/x86/mm/hap/hap.cxen/arch/x86/mm/hap/hap.c +1
-3@@ -224,11 +224,18 @@ static void hap_clean_dirty_bitmap(strucvoid 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);xen/arch/x86/mm/hap/hap.c static void hap_clean_dirty_bitmap(struc · line 237
Quoted 10 lines
xen/arch/x86/mm/hap/hap.c +9
-1- 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_MFNhappens once at start of day, and remove it from the functions that get called mulitple times.Something like this:
diff -r
f71ff2f7b604xen/arch/x86/mm/paging.cxen/arch/x86/mm/paging.c +5
-1@@ -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 1/1 Trivial fix for rc val in hap track dirty vram
In reply to Andres Lagar-Cavillaxen/arch/x86/mm/hap/hap.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
xen/arch/x86/mm/hap/hap.c +1 -1
@@ -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
Quoted 11 lines
Quoted 1 line
Quoted 1 line · Andres Lagar-Cavilla
xen/arch/x86/mm/hap/hap.c| 2 +-1 files changed, 1 insertions(+), 1 deletions(-)Signed-off-by Andres Lagar-Cavilla <[email protected]>diff -r
e22610ef339a-r25d27decfdccxen/arch/x86/mm/hap/hap.c
xen/arch/x86/mm/hap/hap.c int hap_track_dirty_vram(struct domain * · line 147
Quoted 8 lines
xen/arch/x86/mm/hap/hap.c +1
-1@@ -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
Quoted 7 lines · Jan Beulich
Quoted 7 lines
Quoted 1 line
Quoted 1 line · Andres Lagar-Cavilla
xen/arch/x86/mm/hap/hap.c| 2 +-1 files changed, 1 insertions(+), 1 deletions(-)Signed-off-by Andres Lagar-Cavilla <[email protected]>
Quoted 16 lines
xen/arch/x86/mm/hap/hap.c int hap_track_dirty_vram(struct domain * · line 147
Quoted 12 lines
diff -r
e22610ef339a-r25d27decfdccxen/arch/x86/mm/hap/hap.cxen/arch/x86/mm/hap/hap.c +1
-1@@ -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
Quoted 2 lines
Jan
PATCH 1/1 Properly compare "pci" token when groking serial port config
In reply to Andres Lagar-Cavillaxen/drivers/char/ns16550.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
xen/drivers/char/ns16550.c +1 -1
@@ -554,7 +554,7 @@ static void __init ns16550_parse_port_coif ( *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
Quoted 6 lines
Quoted 1 line
Quoted 1 line · Andres Lagar-Cavilla
xen/drivers/char/ns16550.c| 2 +-1 files changed, 1 insertions(+), 1 deletions(-)Signed-off-by Andres Lagar-Cavilla <[email protected]>
This regression fix (from c/s 23679:cf7e1746bdb7) should probably go in right away.
Quoted 13 lines
diff -r
25d27decfdcc-r7e8211d0f41dxen/drivers/char/ns16550.cxen/drivers/char/ns16550.c +1
-1@@ -554,7 +554,7 @@ static void __init ns16550_parse_port_coif ( *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 1/1 Allow log dirty mode to be used in conjunction with paging
In reply to Andres Lagar-Cavillaxen/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.
xen/arch/x86/mm/p2m.c +3 -1
@@ -1061,7 +1061,9 @@ void p2m_mem_paging_resume(struct domainif (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 ); }
xen/include/asm-x86/p2m.h +2 -1
@@ -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 1/1 Prevent the hypervisor from BUGging if xc_hvm_modified_memory is called on a shared page
In reply to Andres Lagar-Cavillaxen/arch/x86/hvm/hvm.c | 10 +++++++++- 1 files changed, 9 insertions(+), 1 deletions(-)
xen/arch/x86/hvm/hvm.c +9 -1
@@ -3755,7 +3755,7 @@ long do_hvm_op(unsigned long op, XEN_GUEfor ( 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 1/1 ASSERT we are putting the right gfn in the XNEMAPSPACE_gmfn* cases
In reply to Andres Lagar-Cavillaxen/arch/x86/mm.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
xen/arch/x86/mm.c +4 -1
@@ -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);
Quoted 4 lines
Quoted 1 line
Quoted 1 line · 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.
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
Quoted 26 lines
Signed-off-by Andres Lagar-Cavilla <[email protected]>Signed-off-by Adin Scannell <[email protected]>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(-)
Andres Lagar-Cavilla writes ("[Xen-devel] [PATCH 00 of 14] MM Fixes"):
Quoted 3 lines
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.
Not a problem on your side. smtp quota on my side. Resending the final three separately.
Thanks
Andres
Quoted 20 lines
Andres Lagar-Cavilla writes ("[Xen-devel] [PATCH 00 of 14] MM Fixes"):
Quoted 3 lines
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.
Quoted 6 lines · 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 <[email protected]>Signed-off-by Adin Scannell <[email protected]>
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.