Re: [PATCH v2 3/9] xen/mm: move modify_identity_mmio to global file and drop __init

[Julien Grall <julien.grall@arm.com>]

Hi Roger,

On 20/04/17 16:17, Roger Pau Monne 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.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  xen/arch/x86/hvm/dom0_build.c | 22 +---------------------
>  xen/common/memory.c           | 34 ++++++++++++++++++++++++++++++++++
>  xen/include/xen/p2m-common.h  |  4 ++++
>  3 files changed, 39 insertions(+), 21 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
> index ca88c5835e..65f606d33a 100644
> --- 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, pfn, pfn, nr_pages, map);
>  }
>
>  /* Populate a HVM memory range using the biggest possible order. */
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 52879e7438..0d970482cb 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1438,6 +1438,40 @@ int prepare_ring_for_helper(
>      return 0;
>  }
>
> +int modify_mmio(struct domain *d, unsigned long gfn, unsigned long pfn,

Whilst you introduce this new function, please use mfn_t and gfn_t.

Also s/pfn/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));

What would be the plan for guest if we decide to use vpci?

> +
> +    for ( ; ; )
> +    {
> +        rc = (map ? map_mmio_regions : unmap_mmio_regions)

On ARM, map_mmio_regions and unmap_mmio_regions will map the MMIO with 
very strict attribute. I think we would need an extra argument to know 
the wanted memory attribute (maybe p2m_type_t?).

> +             (d, _gfn(gfn), nr_pages, _mfn(pfn));
> +        if ( rc == 0 )
> +            break;
> +        if ( rc < 0 )
> +        {
> +            printk(XENLOG_WARNING

I would probably use XENLOG_G_WARNING.

> +                   "Failed to %smap [%#lx, %#lx) -> [%#lx,%#lx) for d%d: %d\n",
> +                   map ? "" : "un", gfn, gfn + nr_pages, pfn, pfn + nr_pages,
> +                   d->domain_id, rc);
> +            break;
> +        }
> +        nr_pages -= rc;
> +        pfn += rc;
> +        gfn += rc;
> +        process_pending_softirqs();
> +    }
> +
> +    return rc;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h
> index 8cd5a6b503..1308da44e7 100644
> --- a/xen/include/xen/p2m-common.h
> +++ b/xen/include/xen/p2m-common.h
> @@ -13,4 +13,8 @@ int unmap_mmio_regions(struct domain *d,
>                         unsigned long nr,
>                         mfn_t mfn);
>
> +

Spurious newline.

> +int modify_mmio(struct domain *d, unsigned long gfn, unsigned long pfn,
> +                unsigned long nr_pages, const bool map);
> +
>  #endif /* _XEN_P2M_COMMON_H */
>

Cheers,

-- 
Julien Grall

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