From mboxrd@z Thu Jan  1 00:00:00 1970
From: Mukesh Rathor <mukesh.rathor@oracle.com>
Subject: Re: [V2 PATCH 2/8] PVH dom0: create
 update_memory_mapping() function
Date: Mon, 25 Nov 2013 15:20:42 -0800
Message-ID: <20131125152042.2e18ab05@mantra.us.oracle.com>
References: <1385165018-25933-1-git-send-email-mukesh.rathor@oracle.com>
	<1385165018-25933-3-git-send-email-mukesh.rathor@oracle.com>
	<52931BC402000078001065DC@nat28.tlf.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta5.messagelabs.com ([195.245.231.135])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <mukesh.rathor@oracle.com>) id 1Vl5SW-0004Wf-8i
	for xen-devel@lists.xenproject.org; Mon, 25 Nov 2013 23:20:52 +0000
In-Reply-To: <52931BC402000078001065DC@nat28.tlf.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>
Cc: xen-devel <xen-devel@lists.xenproject.org>, keir.xen@gmail.com, tim@xen.org
List-Id: xen-devel@lists.xenproject.org

On Mon, 25 Nov 2013 08:43:32 +0000
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 23.11.13 at 01:03, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> wrote:
> > --- a/xen/arch/x86/domctl.c
> > +++ b/xen/arch/x86/domctl.c
> > @@ -46,6 +46,66 @@ static int gdbsx_guest_mem_io(
> >      return (iop->remain ? -EFAULT : 0);
> >  }
> >  
> > +static int update_memory_mapping(struct domain *d, unsigned long
> > gfn,
> > +                          unsigned long mfn, unsigned long nr_mfns,
> > +                          bool_t add)
> > +{
> > +    unsigned long i;
> > +    int ret;
> > +
> > +    ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn + nr_mfns - 1,
> > add);
> > +    if ( ret )
> > +        return ret;
> 
> I think I raised this point more than once before: What is this good
> for when the function (later) gets called during Dom0 construction?
> If it denied access, the system would fail to boot. Hence denying
> access here is pointless, and hence the check should remain in the
> caller of this function.

Well, the point to was to stop unauthorized use. but anyways, moved.

> > +    if ( add )
> > +    {
> 
> Furthermore I assume that the Dom0 construction code path
> would call the function with "add" set only, hence only that
> portion of the code really needs breaking out.
> 
> > +        printk(XENLOG_G_INFO
> > +               "memory_map:add: dom%d gfn=%lx mfn=%lx nr=%lx\n",
> > +               d->domain_id, gfn, mfn, nr_mfns);
> 
> And this printk is surely going to be rather noisy for Dom0, so
> should remain in the caller too.
> 
> > +
> > +        ret = iomem_permit_access(d, mfn, mfn + nr_mfns - 1);
> > +        if ( !ret && paging_mode_translate(d) )
> > +        {
> > +            for ( i = 0; !ret && i < nr_mfns; i++ )
> > +                if ( !set_mmio_p2m_entry(d, gfn + i, _mfn(mfn +
> > i)) )
> > +                    ret = -EIO;
> > +            if ( ret )
> > +            {
> > +                printk(XENLOG_G_WARNING
> > +                       "memory_map:fail: dom%d gfn=%lx mfn=%lx\n",
> > +                       d->domain_id, gfn + i, mfn + i);
> 
> Whereas this should perhaps have its G_WARNING be conditional,
> with it being plain XENLOG_ERR when is_hardware_domain(d) (if
> being a better fit, you could even avoid recovery here for Dom0,
> perhaps by calling panic() instead of printk() in that case).

panic sounds good. Done.

thanks
mukesh