Re: [Xen-devel] [PATCH 03/18] PVH xen: create domctl_memory_mapping() function

[Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>]

On Fri, 31 May 2013 10:46:45 +0100
"Jan Beulich" <JBeulich@xxxxxxxx> wrote:

> >>> On 25.05.13 at 03:25, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
> >>> wrote:
> > Changes in V5:
> >   - Move iomem_access_permitted check to case statment from the
> > function as current doesn't point to dom0 during construct_dom0.
> > 
> > Changes in V6:
> >   - Move iomem_access_permitted back to domctl_memory_mapping() as
> > it should be after the sanity and wrap checks of mfns/gfns.
> 
> So you're undoing what you previously did? How can it work then?
> Or is the whole patch perhaps no longer needed with Dom0 code
> dropped for the time being?
> 
> Otherwise, rather than undoing what you did in v5, I'd think you'd
> want to keep the sanity/wrap checks in the caller as well, since if
> you need the function to be split out just for Dom0 building, you
> can assume the inputs to be sane. And if so, the XSM check could
> remain in the caller too. That would also make sure there really is
> no change in functionality:

Well, thats where I had it originally, but one of the reviewers early on
suggested moving it to the function, in case there's another caller in
future. 

It's only for dom0, so I can drop the check ' !is_idle_vcpu(current) ' and
add it during dom0 construction patch, or drop the whole patch and add it
to construct dom0 patch series, now phase 1.5. Please lmk.

> > +long domctl_memory_mapping(struct domain *d, unsigned long gfn,
> > +                           unsigned long mfn, unsigned long
> > nr_mfns,
> > +                           bool_t add_map)
> > +{
> > +    unsigned long i;
> > +    long ret;
> > +
> > +    if ( (mfn + nr_mfns - 1) < mfn || /* wrap? */
> > +         ((mfn | (mfn + nr_mfns - 1)) >> (paddr_bits -
> > PAGE_SHIFT)) ||
> > +         (gfn + nr_mfns - 1) < gfn ) /* wrap? */
> > +        return -EINVAL;
> > +
> > +    /* caller construct_dom0() runs on idle vcpu */
> > +    if ( !is_idle_vcpu(current) &&
> 
> This check is new, so this is not pure code motion.
> 
> > +         !iomem_access_permitted(current->domain, mfn, mfn +
> > nr_mfns - 1) )
> > +        return -EPERM;
> > +
> > +    ret = xsm_iomem_permission(XSM_HOOK, d, mfn, mfn + nr_mfns -
> > 1, add_map);
> 
> And this was xsm_iomem_mapping() in the original code. What's going
> on here?

Bad merge! That's why I like to do things in small steps and keep patchsets
small. Orig code had xsm_iomem_permission() at some point.

thanks
Mukesh

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