| Message ID | 20221216114853.8227-16-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Remove the directmap | expand |
On 16.12.2022 12:48, Julien Grall wrote: > From: Hongyan Xia <hongyxia@amazon.com> > > When there is not an always-mapped direct map, xenheap allocations need > to be mapped and unmapped on-demand. Hmm, that's still putting mappings in the directmap, which I thought we mean to be doing away with. If that's just a temporary step, then please say so here. > I have left the call to map_pages_to_xen() and destroy_xen_mappings() > in the split heap for now. I am not entirely convinced this is necessary > because in that setup only the xenheap would be always mapped and > this doesn't contain any guest memory (aside the grant-table). > So map/unmapping for every allocation seems unnecessary. But if you're unmapping, that heap won't be "always mapped" anymore. So why would it need mapping initially? > Changes since Hongyan's version: > * Rebase > * Fix indentation in alloc_xenheap_pages() Looks like you did in one of the two instances only, as ... > @@ -2230,17 +2231,36 @@ void *alloc_xenheap_pages(unsigned int order, unsigned int memflags) > if ( unlikely(pg == NULL) ) > return NULL; > > + ret = page_to_virt(pg); > + > + if ( !arch_has_directmap() && > + map_pages_to_xen((unsigned long)ret, page_to_mfn(pg), 1UL << order, > + PAGE_HYPERVISOR) ) > + { > + /* Failed to map xenheap pages. */ > + free_heap_pages(pg, order, false); > + return NULL; > + } ... this looks wrong. An important aspect here is that to be sure of no recursion, map_pages_to_xen() and destroy_xen_mappings() may no longer use Xen heap pages. May be worth saying explicitly in the description (I can't think of a good place in code where such a comment could be put _and_ be likely to be noticed at the right point in time). > void free_xenheap_pages(void *v, unsigned int order) > { > + unsigned long va = (unsigned long)v & PAGE_MASK; > + > ASSERT_ALLOC_CONTEXT(); > > if ( v == NULL ) > return; > > + if ( !arch_has_directmap() && > + destroy_xen_mappings(va, va + (1UL << (order + PAGE_SHIFT))) ) > + dprintk(XENLOG_WARNING, > + "Error while destroying xenheap mappings at %p, order %u\n", > + v, order); Doesn't failure here mean (intended) security henceforth isn't guaranteed anymore? If so, a mere dprintk() can't really be sufficient to "handle" the error. Jan
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index 0a950288e241..0c4af5a71407 100644 --- a/xen/common/page_alloc.c +++ b/xen/common/page_alloc.c @@ -2222,6 +2222,7 @@ void init_xenheap_pages(paddr_t ps, paddr_t pe) void *alloc_xenheap_pages(unsigned int order, unsigned int memflags) { struct page_info *pg; + void *ret; ASSERT_ALLOC_CONTEXT(); @@ -2230,17 +2231,36 @@ void *alloc_xenheap_pages(unsigned int order, unsigned int memflags) if ( unlikely(pg == NULL) ) return NULL; + ret = page_to_virt(pg); + + if ( !arch_has_directmap() && + map_pages_to_xen((unsigned long)ret, page_to_mfn(pg), 1UL << order, + PAGE_HYPERVISOR) ) + { + /* Failed to map xenheap pages. */ + free_heap_pages(pg, order, false); + return NULL; + } + return page_to_virt(pg); } void free_xenheap_pages(void *v, unsigned int order) { + unsigned long va = (unsigned long)v & PAGE_MASK; + ASSERT_ALLOC_CONTEXT(); if ( v == NULL ) return; + if ( !arch_has_directmap() && + destroy_xen_mappings(va, va + (1UL << (order + PAGE_SHIFT))) ) + dprintk(XENLOG_WARNING, + "Error while destroying xenheap mappings at %p, order %u\n", + v, order); + free_heap_pages(virt_to_page(v), order, false); } @@ -2264,6 +2284,7 @@ void *alloc_xenheap_pages(unsigned int order, unsigned int memflags) { struct page_info *pg; unsigned int i; + void *ret; ASSERT_ALLOC_CONTEXT(); @@ -2276,16 +2297,28 @@ void *alloc_xenheap_pages(unsigned int order, unsigned int memflags) if ( unlikely(pg == NULL) ) return NULL; + ret = page_to_virt(pg); + + if ( !arch_has_directmap() && + map_pages_to_xen((unsigned long)ret, page_to_mfn(pg), 1UL << order, + PAGE_HYPERVISOR) ) + { + /* Failed to map xenheap pages. */ + free_domheap_pages(pg, order); + return NULL; + } + for ( i = 0; i < (1u << order); i++ ) pg[i].count_info |= PGC_xen_heap; - return page_to_virt(pg); + return ret; } void free_xenheap_pages(void *v, unsigned int order) { struct page_info *pg; unsigned int i; + unsigned long va = (unsigned long)v & PAGE_MASK; ASSERT_ALLOC_CONTEXT(); @@ -2297,6 +2330,12 @@ void free_xenheap_pages(void *v, unsigned int order) for ( i = 0; i < (1u << order); i++ ) pg[i].count_info &= ~PGC_xen_heap; + if ( !arch_has_directmap() && + destroy_xen_mappings(va, va + (1UL << (order + PAGE_SHIFT))) ) + dprintk(XENLOG_WARNING, + "Error while destroying xenheap mappings at %p, order %u\n", + v, order); + free_heap_pages(pg, order, true); }