Re: [Xen-devel] [V7 PATCH 5/7] pvh: change xsm_add_to_physmap

[Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>]

On Wed, 29 Jan 2014 11:51:35 +0000
Ian Campbell <Ian.Campbell@xxxxxxxxxx> 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" <JBeulich@xxxxxxxx> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel