Re: [V2 PATCH 2/8] PVH dom0: create update_memory_mapping() function

[Mukesh Rathor <mukesh.rathor@oracle.com>]

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