From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mukesh Rathor Subject: Re: [V7 PATCH 5/7] pvh: change xsm_add_to_physmap Date: Wed, 29 Jan 2014 17:33:15 -0800 Message-ID: <20140129173315.592e593e@mantra.us.oracle.com> References: <1387247911-28846-1-git-send-email-mukesh.rathor@oracle.com> <1387247911-28846-6-git-send-email-mukesh.rathor@oracle.com> <20140127175550.4cc67171@mantra.us.oracle.com> <52E7951802000078001177F6@nat28.tlf.novell.com> <20140128180802.152b3f8d@mantra.us.oracle.com> <1390992026.31814.63.camel@kazak.uk.xensource.com> <20140129113846.GA54797@deinos.phlegethon.org> <1390995662.31814.76.camel@kazak.uk.xensource.com> <20140129114837.GB54797@deinos.phlegethon.org> <1390996295.31814.84.camel@kazak.uk.xensource.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 1W8gWS-0001dZ-FQ for xen-devel@lists.xenproject.org; Thu, 30 Jan 2014 01:34:28 +0000 In-Reply-To: <1390996295.31814.84.camel@kazak.uk.xensource.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: Ian Campbell Cc: xen-devel , Tim Deegan , Jan Beulich , "stefano.stabellini@eu.citrix.com" List-Id: xen-devel@lists.xenproject.org On Wed, 29 Jan 2014 11:51:35 +0000 Ian Campbell wrote: > On Wed, 2014-01-29 at 12:48 +0100, Tim Deegan wrote: > > At 11:41 +0000 on 29 Jan (1390992062), Ian Campbell wrote: > > > On Wed, 2014-01-29 at 12:38 +0100, Tim Deegan wrote: > > > > At 10:40 +0000 on 29 Jan (1390988426), Ian Campbell wrote: > > > > > On Tue, 2014-01-28 at 18:08 -0800, Mukesh Rathor wrote: > > > > > > On Tue, 28 Jan 2014 10:31:36 +0000 > > > > > > "Jan Beulich" wrote: > > > > > > > The only think x86-specific here is that > > > > > > > {get,put}_pg_owner() may not exist on ARM. But the > > > > > > > general operation isn't x86-specific, so there shouldn't > > > > > > > be any CONFIG_X86 dependency here. Instead you ought to > > > > > > > work out with the ARM maintainers whether to stub out > > > > > > > those two functions, or whether the functionality is > > > > > > > useful there too (and hence proper implementations would > > > > > > > be needed). > > > > [...] > > > > > Yes, please just make get/put_pg_owner common. > > > > > > > > > > The only required change would be to: > > > > > if ( unlikely(paging_mode_translate(curr)) ) > > > > > { > > > > > MEM_LOG("Cannot mix foreign mappings with translated > > > > > domains"); goto out; > > > > > } > > > > > > > > > > which is not needed for ARM, and I suspect needs adjusting > > > > > for PVH too (ah, there it is in the next patch). I think the > > > > > best solution there would be a new predicate e.g. > > > > > paging_mode_supports_foreign(curr) (or some better name, I > > > > > don't especially like my suggestion) > > > > > > > > > > on ARM: > > > > > > > > > > #define paging_mode_supports_foreign(d) (1) > > > > > > > > > > on x86: > > > > > > > > > > #define paging_mode_support_foreign(d) (is_pvh_domain(curr) > > > > > || !(paging_mode_translate(curr)) > > > > > > > > > > > > > Hmmm. That's likely to have unintended consequences > > > > somewhere. (And if that check is really not needed for PVH > > > > maybe it's not needed for HVM either, given that they share all > > > > their paging support code). > > > > > > > > But I don't think we need to tinker with it anyway - AFAICS, > > > > get_pg_owner() isn't really what's wanted in the XATP code. > > > > All the other uses of get_pg_owner() are in the x86 PV MMU > > > > code, which this is definitely not, and it handles cases (like > > > > mmio) that we don't want here anyway. How about using > > > > rcu_lock_live_remote_domain_by_id()? > > > > > > We have a struct page in our hand -- don't we need to lookup the > > > owner and lock it somewhat atomically? > > > > I'm not sure what you mean: > > - the code that Mukesh is adding doesn't have a struct page, it's > > just grabbing the foreign domid from the hypercall arg; > > - if we did have a struct page, we'd just need to take a ref to > > stop the owner changing underfoot; and > > - get_pg_owner() takes a domid anyway. > > Sorry, I was confused/mislead by the name... > > rcu_lock_live_remote_domain_by_id does look like what is needed. Yup, that will do it. Thanks Tim. Mukesh