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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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?

Ian.


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