[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [Xen-devel] Re: [PATCH 17/18] Nested Virtualization: p2m infrastructure
This was mostly OK, with a few oddities (below), and assuming that we can agree that later patches make it necessary. If you resubmit, please send it as two patches, one of which does _only_ the change from passing struct domains to passing p2m structs and nothing else - no comment changes, no format tidying, nothing. I don't want to have to comb through another 4000 line patch looking for hidden surprises. > @@ -1391,8 +1390,9 @@ out: > return rv; > } > > +/* Read p2m table (through the linear mapping). */ That's exactly wrong. The gfn_to_mfn_current() function that you removed was the one that used the linear mapping. This one maps and unmaps pages as it goes. > static mfn_t > -p2m_gfn_to_mfn(struct domain *d, unsigned long gfn, p2m_type_t *t, > +p2m_gfn_to_mfn(struct p2m_domain *p2m, unsigned long gfn, p2m_type_t *t, > p2m_query_t q) > { mfn_t mfn; ... > @@ -1531,8 +1531,7 @@ pod_retry_l1: > return (p2m_is_valid(*t) || p2m_is_grant(*t)) ? mfn : _mfn(INVALID_MFN); > } > > -/* Init the datastructures for later use by the p2m code */ > -int p2m_init(struct domain *d) > +static int p2m_allocp2m(struct p2m_domain **_p2m) Since the only error this routine can return is -ENOMEM, why not just return the p2m pointer or NULL? > { > struct p2m_domain *p2m; > > @@ -1540,39 +1539,68 @@ int p2m_init(struct domain *d) > if ( p2m == NULL ) > return -ENOMEM; > > - d->arch.p2m = p2m; > - > + *_p2m = p2m; > + return 0; > +} > + > +/* Init the datastructures for later use by the p2m code */ > +static void p2m_initialise(struct domain *d, struct p2m_domain *p2m, > + bool_t preserve_allocfree) > +{ > + void *alloc, *free; These are function pointers; there's no need to hack them into void pointers. > + alloc = free = NULL; > + if (preserve_allocfree) { > + alloc = p2m->alloc_page; > + free = p2m->free_page; > + } > memset(p2m, 0, sizeof(*p2m)); > p2m_lock_init(p2m); > INIT_PAGE_LIST_HEAD(&p2m->pages); > INIT_PAGE_LIST_HEAD(&p2m->pod.super); > INIT_PAGE_LIST_HEAD(&p2m->pod.single); > > + p2m->domain = d; > p2m->set_entry = p2m_set_entry; > p2m->get_entry = p2m_gfn_to_mfn; > p2m->change_entry_type_global = p2m_change_type_global; > + if (preserve_allocfree) { > + p2m->alloc_page = alloc; > + p2m->free_page = free; > + } > > if ( is_hvm_domain(d) && d->arch.hvm_domain.hap_enabled && > (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) ) > ept_p2m_init(d); > > + return; > +} ... > void p2m_final_teardown(struct domain *d) > { > + /* Iterate over all p2m tables per domain */ Even when the matching code arrives this comment's not really helpful. > xfree(d->arch.p2m); > d->arch.p2m = NULL; > } > > #if P2M_AUDIT > -static void audit_p2m(struct domain *d) > +static void audit_p2m(struct p2m_domain *p2m) > { > struct page_info *page; > struct domain *od; > @@ -1735,6 +1761,7 @@ static void audit_p2m(struct domain *d) > unsigned long orphans_d = 0, orphans_i = 0, mpbad = 0, pmbad = 0; > int test_linear; > p2m_type_t type; > + struct domain *d = p2m->domain; > > if ( !paging_mode_translate(d) ) > return; > @@ -1789,7 +1816,7 @@ static void audit_p2m(struct domain *d) > continue; > } > > - p2mfn = gfn_to_mfn_type_foreign(d, gfn, &type, p2m_query); > + p2mfn = gfn_to_mfn_type_p2m(p2m, gfn, &type, p2m_query); > if ( mfn_x(p2mfn) != mfn ) > { > mpbad++; > @@ -1805,9 +1832,9 @@ static void audit_p2m(struct domain *d) > set_gpfn_from_mfn(mfn, INVALID_M2P_ENTRY); > } > > - if ( test_linear && (gfn <= d->arch.p2m->max_mapped_pfn) ) > + if ( test_linear && (gfn <= p2m->max_mapped_pfn) ) > { > - lp2mfn = mfn_x(gfn_to_mfn_query(d, gfn, &type)); > + lp2mfn = mfn_x(gfn_to_mfn_query(p2m, gfn, &type)); > if ( lp2mfn != mfn_x(p2mfn) ) > { > P2M_PRINTK("linear mismatch gfn %#lx -> mfn %#lx " > @@ -1822,21 +1849,19 @@ static void audit_p2m(struct domain *d) > spin_unlock(&d->page_alloc_lock); > > /* Audit part two: walk the domain's p2m table, checking the entries. */ > - if ( pagetable_get_pfn(p2m_get_pagetable(p2m_get_hostp2m(d)) != 0 ) > + if ( pagetable_get_pfn(p2m_get_pagetable(p2m)) != 0 ) > { > + l3_pgentry_t *l3e; > l2_pgentry_t *l2e; > l1_pgentry_t *l1e; > - int i1, i2; > + int i1, i2, i3; Why are you moving these definitions around? You don't change the use of the variables anywhere. > #if CONFIG_PAGING_LEVELS == 4 > l4_pgentry_t *l4e; > - l3_pgentry_t *l3e; > - int i3, i4; > - l4e = > map_domain_page(mfn_x(pagetable_get_mfn(p2m_get_pagetable(p2m_get_hostp2m(d))))); > + int i4; > + l4e = > map_domain_page(mfn_x(pagetable_get_mfn(p2m_get_pagetable(p2m)))); > #else /* CONFIG_PAGING_LEVELS == 3 */ > - l3_pgentry_t *l3e; > - int i3; Ditto. > - l3e = > map_domain_page(mfn_x(pagetable_get_mfn(p2m_get_pagetable(p2m_get_hostp2m(d))))); > + l3e = > map_domain_page(mfn_x(pagetable_get_mfn(p2m_get_pagetable(p2m)))); > #endif > > gfn = 0; ... > diff -r f96f6f2cd19a -r 3f48b73b0a30 xen/common/grant_table.c > --- a/xen/common/grant_table.c > +++ b/xen/common/grant_table.c > @@ -109,7 +109,7 @@ static unsigned inline int max_nr_maptra > #define gfn_to_mfn_private(_d, _gfn) ({ \ > p2m_type_t __p2mt; \ > unsigned long __x; \ > - __x = mfn_x(gfn_to_mfn_unshare(_d, _gfn, &__p2mt, 1)); \ > + __x = mfn_x(gfn_to_mfn_unshare(p2m_get_hostp2m(_d), _gfn, &__p2mt, 1)); > \ > if ( !p2m_is_valid(__p2mt) ) \ > __x = INVALID_MFN; \ > __x; }) > @@ -1059,7 +1059,7 @@ gnttab_unpopulate_status_frames(struct d > > for ( i = 0; i < nr_status_frames(gt); i++ ) > { > - page_set_owner(virt_to_page(gt->status[i]), dom_xen); > + page_set_owner(virt_to_page(gt->status[i]), > p2m_get_hostp2m(dom_xen)); That seems very unlikely to be correct. Did you get carried away? > free_xenheap_page(gt->status[i]); > gt->status[i] = NULL; > } > @@ -1498,7 +1498,7 @@ gnttab_transfer( > if ( unlikely(e->tot_pages++ == 0) ) > get_knownalive_domain(e); > page_list_add_tail(page, &e->page_list); > - page_set_owner(page, e); > + page_set_owner(page, p2m_get_hostp2m(e)); Likewise. > > spin_unlock(&e->page_alloc_lock); > ... > diff -r f96f6f2cd19a -r 3f48b73b0a30 xen/include/asm-x86/mem_sharing.h > --- a/xen/include/asm-x86/mem_sharing.h > +++ b/xen/include/asm-x86/mem_sharing.h > @@ -23,22 +23,22 @@ > #define __MEM_SHARING_H__ > > #define sharing_supported(_d) \ > - (is_hvm_domain(_d) && (_d)->arch.hvm_domain.hap_enabled) > + (is_hvm_domain(_d) && paging_mode_hap(_d)) > Okay, but it really doesn't belong in this patch. Cheers, Tim. -- Tim Deegan <Tim.Deegan@xxxxxxxxxx> Principal Software Engineer, XenServer Engineering Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxx http://lists.xensource.com/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |