From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mukesh Rathor Subject: Re: [PATCH 03/18] PVH xen: create domctl_memory_mapping() function Date: Tue, 4 Jun 2013 17:47:45 -0700 Message-ID: <20130604174745.157c9011@mantra.us.oracle.com> References: <1369445137-19755-1-git-send-email-mukesh.rathor@oracle.com> <1369445137-19755-4-git-send-email-mukesh.rathor@oracle.com> <51A88DA502000078000D9F3A@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <51A88DA502000078000D9F3A@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel List-Id: xen-devel@lists.xenproject.org On Fri, 31 May 2013 10:46:45 +0100 "Jan Beulich" wrote: > >>> On 25.05.13 at 03:25, Mukesh Rathor > >>> 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