Re: [Xen-devel] [PATCH v2 07/10] vm_event: Add vm_event_ng interface

[Jan Beulich <JBeulich@xxxxxxxx>]

On 18.07.2019 15:59, Petre Ovidiu PIRCALABU wrote:
> Before using xenforeignmemory_map_resource I investigated several
> different approaches:
> - Allocate the memory in hypervisor and xc_map_foreign_pages (doesn't
> work because you cannot "foreignmap" pages of your own domain.
> - Allocate the memory in guest, and map the allocated pages in XEN. To
> my knowledge there is no such API in linux to do this and the monitor
> application, as an userspace program, is not aware of the actual gfns
> for an allocated memory area.
> 
> So, at this point the most promising solution is allocating the memory
> in XEN, sharing it with ID using share_xen_page_with_guest, and mapping
> it with xenforeignmemory_map_resource (with the flag
> XENMEM_rsrc_acq_caller_owned set)

Which is fine - that's why Paul had introduced the generic interface.

> To my understanding the cleanup sequence from
> gnttab_unpopulate_status_frames basically boils down to:
> 1. guest_physmap_remove_page
> 2. if ( test_and_clear_bit(_PGC_allocated, &pg->count_info) )
>         put_page(pg);
> 3. free_xenheap_page

You're missing the crucial part of undoing step 2 if you find
that there are still references left to the page.

And then, because of your use of vzalloc(), you can't use
free_xenheap_pages(), as that takes a (direct map) linear address
as input. It has to be free_domheap_pages() in your case.

> My current implementation uses vzalloc instead of alloc_xenheap_pages
> and that's why I assumed vunmap and free_domheap_pages will suffice (I
> would have called vfree directly, but the temporary linked list that is
> used to hold the page references causes free_domheap_pages to crash)
> 
> Do I also have to call guest_physmap_remove_page and put_page? (steps
> 1. and 2.)

guest_physmap_remove_page() needs to be called for translated page-
owning domains if the page was ever added to their physmap. As long
as you avoid adding, you also don't need to remove. I don't recall
though whether a translated domain can access a resource without it
having a representation in its GFN space.

You definitely need to do step 2 (which is to undo the respective
part of share_xen_page_with_guest(), and you _also_ need to clear
the page owner (undoing the other part of what that function has
done). And then, as said above, you need to check that the page
has no references left on it, and correctly deal with the case when
it still has some.

On the whole gnttab_unpopulate_status_frames() is very unfortunate
to have the way it is, including the various domain_crash()-es. It
was merely the only way we could see when dealing with XSA-255. I
would strongly recommend against new code trying to go this same
route, unless we introduce a generic and clean/safe function
inverting share_xen_page_with_guest() (which I've tried in the past
and failed).

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel