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

[Tim Deegan <tim@xxxxxxx>]

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.

Tim.

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