From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH v2 1/2] altp2m: Merge p2m_set_altp2m_mem_access and p2m_set_mem_access Date: Fri, 29 Jan 2016 09:19:45 -0700 Message-ID: <56AB9F3102000078000CC78F@prv-mh.provo.novell.com> References: <1454014688-25060-1-git-send-email-tlengyel@novetta.com> <56AB552202000078000CC5E5@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1aPBm0-0007oj-EF for xen-devel@lists.xenproject.org; Fri, 29 Jan 2016 16:19:48 +0000 In-Reply-To: Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tamas Lengyel Cc: Wei Liu , Ian Campbell , Razvan Cojocaru , Stefano Stabellini , George Dunlap , Andrew Cooper , Ian Jackson , Stefano Stabellini , xen-devel@lists.xenproject.org, Keir Fraser List-Id: xen-devel@lists.xenproject.org >>> On 29.01.16 at 17:12, wrote: > On Fri, Jan 29, 2016 at 4:03 AM, Jan Beulich wrote: >> >>> On 28.01.16 at 21:58, wrote: >> > --- a/xen/arch/x86/mm/p2m.c >> > +++ b/xen/arch/x86/mm/p2m.c >> > @@ -1777,14 +1777,57 @@ bool_t p2m_mem_access_check(paddr_t gpa, >> unsigned long gla, >> > return (p2ma == p2m_access_n2rwx); >> > } >> > >> > +static int p2m_set_altp2m_mem_access(struct domain *d, struct >> p2m_domain *hp2m, >> > + struct p2m_domain *ap2m, >> p2m_access_t a, >> > + unsigned long gfn) >> >> I think new functions would better not use "unsigned long" for frame >> numbers. >> > > The only place this is called from the gfn is already converted to unsigned > long. I don't see much point in converting it back to gfn_t and then back > to unsigned long again.. If you recall, the goal is to have all frame numbers passed around have distinguishable types. > I was thinking this function may even be declared as inline? This is orthogonal (and I really don't care much). >> > +{ >> > + mfn_t mfn; >> > + p2m_type_t t; >> > + p2m_access_t old_a; >> > + unsigned int page_order; >> > + int rc; >> > + >> > + mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL); >> > + >> > + /* Check host p2m if no valid entry in alternate */ >> > + if ( !mfn_valid(mfn) ) >> > + { >> > + mfn = hp2m->get_entry(hp2m, gfn, &t, &old_a, >> > + P2M_ALLOC | P2M_UNSHARE, &page_order, >> NULL); >> > + >> > + rc = -ESRCH; >> > + if ( !mfn_valid(mfn) || t != p2m_ram_rw ) >> > + goto out; >> > + >> > + /* If this is a superpage, copy that first */ >> > + if ( page_order != PAGE_ORDER_4K ) >> > + { >> > + unsigned long mask = ~((1UL << page_order) - 1); >> > + gfn_t gfn2 = _gfn(gfn & mask); >> > + mfn_t mfn2 = _mfn(mfn_x(mfn) & mask); >> > + >> > + rc = ap2m->set_entry(ap2m, gfn_x(gfn2), mfn2, page_order, >> t, old_a, 1); >> > + if ( rc ) >> > + goto out; >> > + } >> > + } >> > + >> > + rc = ap2m->set_entry(ap2m, gfn, mfn, PAGE_ORDER_4K, t, a, >> > + (current->domain != d)); >> > + >> > + out: >> > + return rc; >> > +} >> >> With there not being any involved error handling here, I don't think >> using a label and goto is warranted here. But I'll leave the ultimate >> decision to George, of course. > > RIght, this is a remnant from the previous version of this function where > out also had the p2m_unlock. Now that it is just a return I could do the > return in place of the gotos. Let me know which one is preferred. Since you sent this to me - my general request is to avoid goto wherever possible. >> > --- a/xen/include/public/memory.h >> > +++ b/xen/include/public/memory.h >> > @@ -423,11 +423,14 @@ struct xen_mem_access_op { >> > /* xenmem_access_t */ >> > uint8_t access; >> > domid_t domid; >> > + uint16_t altp2m_idx; >> > + uint16_t _pad; >> > /* >> > * Number of pages for set op >> > * Ignored on setting default access and other ops >> > */ >> > uint32_t nr; >> > + uint32_t _pad2; >> >> Repeating what I had said on v1: So this is a tools only interface, >> yes. But it's not versioned (other than e.g. domctl and sysctl), so >> altering the interface structure is at least fragile. > > Not sure what I can do to address this. Deprecate the old interface and introduce a new one. But other maintainers' opinions would be welcome. Jan