On Mon, May 13, 2024 at 01:40:37PM +0000, Elias El Yandouzi wrote:
> From: Hongyan Xia <hongy...@amazon.com>
>
> When there is not an always-mapped direct map, xenheap allocations need
> to be mapped and unmapped on-demand.
>
> Signed-off-by: Hongyan Xia <hongy...@amazon.com>
> Signed-off-by: Julien Grall <jgr...@amazon.com>
> Signed-off-by: Elias El Yandouzi <elias...@amazon.com>
>
> ----
>
> 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.
I'm also concerned by this, did you test that
CONFIG_SEPARATE_XENHEAP=y works properly with the added {,un}map
calls?
If CONFIG_SEPARATE_XENHEAP=y I would expect the memory returned by
alloc_heap_pages(MEMZONE_XEN...) to already have the virtual mappings
created ahead?
The comment at the top of page_alloc.c also needs to be updated to
notice how the removal of the direct map affects xenheap allocations,
AFAICT a new combination is now possible:
CONFIG_SEPARATE_XENHEAP=n & CONFIG_NO_DIRECTMAP=y
> Changes in v2:
> * Fix remaining wrong indentation in alloc_xenheap_pages()
>
> Changes since Hongyan's version:
> * Rebase
> * Fix indentation in alloc_xenheap_pages()
> * Fix build for arm32
>
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 9b7e4721cd..dfb2c05322 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2242,6 +2242,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;
virt_addr maybe? ret is what I would expect to store the return value
of the function usually.
>
> ASSERT_ALLOC_CONTEXT();
>
> @@ -2250,17 +2251,36 @@ void *alloc_xenheap_pages(unsigned int order,
> unsigned int memflags)
> if ( unlikely(pg == NULL) )
> return NULL;
>
> + ret = page_to_virt(pg);
> +
> + if ( !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 ( !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);
> }
>
> @@ -2284,6 +2304,7 @@ void *alloc_xenheap_pages(unsigned int order, unsigned
> int memflags)
> {
> struct page_info *pg;
> unsigned int i;
> + void *ret;
>
> ASSERT_ALLOC_CONTEXT();
>
> @@ -2296,16 +2317,28 @@ void *alloc_xenheap_pages(unsigned int order,
> unsigned int memflags)
> if ( unlikely(pg == NULL) )
> return NULL;
>
> + ret = page_to_virt(pg);
> +
> + if ( !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();
>
> @@ -2317,6 +2350,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 ( !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);
I don't think this should be a dprintk(), leaving mappings behind
could be a severe issue given the point of this work is to prevent
leaking data by having everything mapped on the direct map.
This needs to be a printk() IMO, I'm unsure whether freeing the memory
would need to be avoided if destroying the mappings failed, I can't
think of how we could recover from this gracefully.
Thanks, Roger.
