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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.11.13 at 01:03, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> 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.

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

Jan


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