Re: [Xen-devel] [PATCH v3 3/9] xen/mm: move modify_identity_mmio to global file and drop __init

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.04.17 at 16:35, <roger.pau@xxxxxxxxxx> wrote:
> And also allow it to do non-identity mappings by adding a new parameter. This
> function will be needed in other parts apart from PVH Dom0 build. While there
> fix the function to use gfn_t and mfn_t instead of unsigned long for memory
> addresses.

I'm afraid both title and description don't (or no longer) properly reflect
what the patch does. I'm also afraid the reason the new parameter as
well as the placement in common/memory.c aren't sufficiently explained.
For example, what use is the function going to be without
CONFIG_HAS_PCI?

> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -64,27 +64,7 @@ static struct acpi_madt_nmi_source __initdata *nmisrc;
>  static int __init modify_identity_mmio(struct domain *d, unsigned long pfn,
>                                         unsigned long nr_pages, const bool 
> map)
>  {
> -    int rc;
> -
> -    for ( ; ; )
> -    {
> -        rc = (map ? map_mmio_regions : unmap_mmio_regions)
> -             (d, _gfn(pfn), nr_pages, _mfn(pfn));
> -        if ( rc == 0 )
> -            break;
> -        if ( rc < 0 )
> -        {
> -            printk(XENLOG_WARNING
> -                   "Failed to identity %smap [%#lx,%#lx) for d%d: %d\n",
> -                   map ? "" : "un", pfn, pfn + nr_pages, d->domain_id, rc);
> -            break;
> -        }
> -        nr_pages -= rc;
> -        pfn += rc;
> -        process_pending_softirqs();
> -    }
> -
> -    return rc;
> +    return modify_mmio(d, _gfn(pfn), _mfn(pfn), nr_pages, map);
>  }

I don't see the value of retaining this wrapper.

> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1438,6 +1438,42 @@ int prepare_ring_for_helper(
>      return 0;
>  }
>  
> +int modify_mmio(struct domain *d, gfn_t gfn, mfn_t mfn, unsigned long 
> nr_pages,
> +                const bool map)
> +{
> +    int rc;
> +
> +    /*
> +     * Make sure this function is only used by the hardware domain, because 
> it
> +     * can take an arbitrary long time, and could DoS the whole system.
> +     */
> +    ASSERT(is_hardware_domain(d));

If that can happen arbitrarily at run time (rather than just at boot,
as suggested by the removal of __init), it definitely can't remain as
is and will instead need to make use of continuations. I'm therefore
unconvinced you really want to move this code instead of simply
calling {,un}map_mmio_regions() while taking care of preemption
needs.

Jan


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