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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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:
> 
> > >>> On 28.01.14 at 02:55, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
> > >>> wrote:
> > > --- a/xen/common/memory.c
> > > +++ b/xen/common/memory.c
> ....
> > 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).
> > 
> > In the latter case I would then also wonder whether the x86
> > implementation shouldn't be moved into common code.
> 
> Stefano/Ian:
> 
> If you have use for get_pg_owner() I can stub it out for now and
> have it return 1, as NULL would result in error. Otherwise, I can
> change the function prototype to return rc with ARM always returning 
> 0 and not doing anything, like:
> 
>         if ( xatpb.space == XENMAPSPACE_gmfn_foreign )
>         {
>             if ( (rc = get_pg_owner(xatpb.foreign_domid, &fd)) )
>             {
>                 rcu_unlock_domain(d);
>                 return rc;
>             }
>         }
> 
> which on ARM would always return 0, setting fd to NULL.
> 
> If you think it would be needed in ARM, I can just leave the function
> prototype the same and you guys can implement whenever as I don't have the
> insight into ARM, and if it looks the same as x86 you can commonise it too.

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))

Thanks,
Ian.



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