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(-)
Andres Lagar-Cavilla
2011-Nov-23 21:11 UTC
[PATCH 01 of 14] 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.
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;
Andres Lagar-Cavilla
2011-Nov-23 21:11 UTC
[PATCH 02 of 14] Fix handling of m2p map in set_shared_p2m_entry
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);
Andres Lagar-Cavilla
2011-Nov-23 21:11 UTC
[PATCH 03 of 14] Make HAP log dirty disable return the correct rc
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);
Andres Lagar-Cavilla
2011-Nov-23 21:11 UTC
[PATCH 04 of 14] When passing no bitmap for the shadow log dirty bitmap clean up, we should not get EFAULT
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;
Andres Lagar-Cavilla
2011-Nov-23 21:11 UTC
[PATCH 05 of 14] Don''t trigger unnecessary shadow scans on p2m entry update
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:
Andres Lagar-Cavilla
2011-Nov-23 21:11 UTC
[PATCH 06 of 14] Don''t lose track of the log dirty bitmap
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 */
Andres Lagar-Cavilla
2011-Nov-23 21:11 UTC
[PATCH 07 of 14] Trivial fix for rc val in hap track dirty vram
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;
Andres Lagar-Cavilla
2011-Nov-23 21:11 UTC
[PATCH 08 of 14] Properly compare "pci" token when groking serial port config
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;
Andres Lagar-Cavilla
2011-Nov-23 21:11 UTC
[PATCH 09 of 14] Allow log dirty mode to be used in conjunction with paging
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) \
Andres Lagar-Cavilla
2011-Nov-23 21:11 UTC
[PATCH 10 of 14] Prevent the hypervisor from BUGging if xc_hvm_modified_memory is called on a shared page
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 )
{
Andres Lagar-Cavilla
2011-Nov-23 21:11 UTC
[PATCH 11 of 14] ASSERT we are putting the right gfn in the XNEMAPSPACE_gmfn* cases
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);
>>> 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(-)
Jan Beulich
2011-Nov-24 09:48 UTC
Re: [PATCH 05 of 14] Don''t trigger unnecessary shadow scans on p2m entry update
>>> 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:
Jan Beulich
2011-Nov-24 09:51 UTC
Re: [PATCH 05 of 14] Don''t trigger unnecessary shadow scans on p2m entry update
>>> 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
Jan Beulich
2011-Nov-24 09:59 UTC
Re: [PATCH 07 of 14] Trivial fix for rc val in hap track dirty vram
>>> 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
Jan Beulich
2011-Nov-24 10:03 UTC
Re: [PATCH 08 of 14] Properly compare "pci" token when groking serial port config
>>> 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;
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.
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. >
Andres Lagar-Cavilla
2011-Nov-24 15:24 UTC
Re: [PATCH 05 of 14] Don''t trigger unnecessary shadow scans on p2m entry update
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 > >
Keir Fraser
2011-Nov-24 15:38 UTC
Re: [PATCH 07 of 14] Trivial fix for rc val in hap track dirty vram
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 >
Tim Deegan
2011-Nov-24 15:53 UTC
Re: [PATCH 05 of 14] Don''t trigger unnecessary shadow scans on p2m entry update
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.
Tim Deegan
2011-Nov-24 16:07 UTC
Re: [PATCH 06 of 14] Don''t lose track of the log dirty bitmap
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.
Andres Lagar-Cavilla
2011-Nov-24 16:11 UTC
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> 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. >
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.
Andres Lagar-Cavilla
2011-Nov-24 17:46 UTC
Re: [PATCH 06 of 14] Don''t lose track of the log dirty bitmap
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. >