From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [V6 PATCH 6.2/7] pvh dom0: Add and remove foreign pages Date: Mon, 16 Dec 2013 23:44:37 +0000 Message-ID: <52AF9065.6030205@linaro.org> References: <1386297524-15483-1-git-send-email-mukesh.rathor@oracle.com> <1386297524-15483-7-git-send-email-mukesh.rathor@oracle.com> <20131210162753.2e402081@mantra.us.oracle.com> <20131210164442.3879f6c0@mantra.us.oracle.com> <52A7C14C.2020504@linaro.org> <20131210174755.05e5550f@mantra.us.oracle.com> <20131211142903.GB6450@deinos.phlegethon.org> <20131211184606.4f0d9366@mantra.us.oracle.com> <20131212184449.77cc02db@mantra.us.oracle.com> <20131213112548.GA57692@deinos.phlegethon.org> <20131213184828.68a24d94@mantra.us.oracle.com> <52AECA99020000780010D7F3@nat28.tlf.novell.com> <20131216152722.4c074d8f@mantra.us.oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20131216152722.4c074d8f@mantra.us.oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Mukesh Rathor , Jan Beulich Cc: Xen-devel@lists.xensource.com, Ian Campbell , george.dunlap@eu.citrix.com, Tim Deegan , eddie.dong@intel.com, keir.xen@gmail.com, jun.nakajima@intel.com List-Id: xen-devel@lists.xenproject.org On 12/16/2013 11:27 PM, Mukesh Rathor wrote: > On Mon, 16 Dec 2013 08:40:41 +0000 > "Jan Beulich" wrote: > >>>>> On 14.12.13 at 03:48, Mukesh Rathor >>>>> wrote: >>>> Also, Jan may have an opinion about whether a teardown operation >>>> that has to walk each p2m entry would have to be made >>>> preemptible. I'm not sure where we draw the line on such things. >>> >>> Since at present teardown cleanup of foreign is not really that >>> important as its only applicable to dom0, let me submit another >>> patch for it on Mon with few ideas. That would also keep this patch >>> size reasonable, and keep you from having to look at the same code >>> over and over. >>> >>> So, please take a look at the version below with above two fixes. If >>> you approve it, i can resubmit the entire series rebased to latest >>> with your ack on Monday, and the series can go in while we resolve >>> the p2m teardown. >> >> Going through the patch again, I'm not seeing any loop being >> added. Am I missing something here? > > Yes. Since the destruction of p2m leaking foreign pages only applies > to control domain being destroyed, i don't think it is that critical > that part get into 4.4. So, I'm submitting a separate patch for it, > like said above. > >>> --- a/xen/arch/x86/mm/p2m-ept.c >>> +++ b/xen/arch/x86/mm/p2m-ept.c >>> @@ -36,8 +36,6 @@ >>> >>> #define >>> atomic_read_ept_entry(__pepte) \ >>> ( (ept_entry_t) { .epte = read_atomic(&(__pepte)->epte) } ) >>> -#define atomic_write_ept_entry(__pepte, >>> __epte) \ >>> - write_atomic(&(__pepte)->epte, (__epte).epte) >>> >>> #define is_epte_present(ept_entry) ((ept_entry)->epte & 0x7) >>> #define is_epte_superpage(ept_entry) ((ept_entry)->sp) >>> @@ -46,6 +44,25 @@ static inline bool_t is_epte_valid(ept_entry_t >>> *e) return (e->epte != 0 && e->sa_p2mt != p2m_invalid); >>> } >>> >>> +static inline void write_ept_entry(ept_entry_t *entryptr, >>> ept_entry_t *new) >> >> So why do you drop the "atomic_" prefix here? > > To distinguish it from the older atomic_* macro which did nothing but > atomically write the entry. But if it helps get your approval, I added > atomic prefix. > >> Also the second parameter could be "const"... > > Ok. > > Final version below: > > thanks > Mukesh > --------------------- > > In this patch, a new function, p2m_add_foreign(), is added > to map pages from foreign guest into current dom0 for domU creation. > Such pages are typed p2m_map_foreign. Another function > p2m_remove_foreign() is added to remove such pages. Note, it is the > nature of such pages that a refcnt is held during their stay in the p2m. > The refcnt is added and released in the low level ept code for convenience. > The cleanup of foreign pages from p2m upon it's destruction, is submitted > subsequently under a separate patch. > > Signed-off-by: Mukesh Rathor > --- > xen/arch/x86/mm.c | 23 +++++++--- > xen/arch/x86/mm/p2m-ept.c | 30 +++++++++++--- > xen/arch/x86/mm/p2m-pt.c | 9 ++++- > xen/arch/x86/mm/p2m.c | 96 +++++++++++++++++++++++++++++++++++++++++++++ > xen/common/memory.c | 12 +++++- > xen/include/asm-arm/p2m.h | 8 +++- > xen/include/asm-x86/p2m.h | 7 +++ > 7 files changed, 169 insertions(+), 16 deletions(-) Following the discussion we had on ARM thread (see https://patches.linaro.org/22361/), the approach is to remove specific patch for p2m foreign on common code. So get_page_from_gfn must handle reference on foreign mapping. The code is pretty simple on ARM, see: https://patches.linaro.org/22536/ and I don't see why this kind of modification can't go on x86 part. Also, can you remove all ARM specific code in this patch? Cheers, -- Julien Grall