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

From: Petre Ovidiu PIRCALABU <ppircalabu@bitdefender.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: "Stefano Stabellini" <sstabellini@kernel.org>,
	"Wei Liu" <wl@xen.org>,
	"Razvan Cojocaru" <rcojocaru@bitdefender.com>,
	"Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>,
	"George Dunlap" <George.Dunlap@eu.citrix.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Ian Jackson" <ian.jackson@eu.citrix.com>,
	"Tim Deegan" <tim@xen.org>, JulienGrall <julien.grall@arm.com>,
	"Paul Durrant" <Paul.Durrant@citrix.com>,
	"Tamas K Lengyel" <tamas@tklengyel.com>,
	"Alexandru Stefan ISAILA" <aisaila@bitdefender.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v2 07/10] vm_event: Add vm_event_ng interface
Date: Thu, 18 Jul 2019 14:55:41 +0000	[thread overview]
Message-ID: <54ffb078c4c4c5d793a4bed2e47b587237cb7b0d.camel@bitdefender.com> (raw)
In-Reply-To: <58a8a000-7dff-be57-7d75-f39abbd41f69@suse.com>

On Thu, 2019-07-18 at 14:44 +0000, Jan Beulich wrote:
> 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

Thank-you very much for explaining this to me. I will update the
cleanup procedure and let you know the outcome.

Best regards,
Petre

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