[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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.

Ian.


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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.