Re: [Xen-devel] [PATCH v5 21/22] xen/arm: Add a hypercall for device mmio mapping

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.03.16 at 07:15, <zhaoshenglong@xxxxxxxxxx> wrote:
> From: Shannon Zhao <shannon.zhao@xxxxxxxxxx>
> 
> It needs to map platform or amba device mmio to Dom0 on ARM. But when
> booting with ACPI, it can't get the mmio region in Xen due to lack of
> AML interpreter to parse DSDT table. Therefore, let Dom0 call a
> hypercall to map mmio region when it adds the devices.
> 
> Here we add a new map space like the XEN_DOMCTL_memory_mapping to map
> mmio region for Dom0.
> 
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Signed-off-by: Shannon Zhao <shannon.zhao@xxxxxxxxxx>

Same remark regarding the Cc list.

> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1138,6 +1138,9 @@ int xenmem_add_to_physmap_one(
>          rcu_unlock_domain(od);
>          break;
>      }
> +    case XENMAPSPACE_dev_mmio:
> +        rc = map_dev_mmio_region(d, gpfn, 1, idx);

This being the only caller, ...

> +int map_dev_mmio_region(struct domain *d,
> +                        unsigned long start_gfn,
> +                        unsigned long nr,
> +                        unsigned long mfn)
> +{

... what's the "nr" parameter good for? Or alternatively - wouldn't
you want to make it possible to have larger areas mapped by large
pages?

> +    int res;
> +
> +    if(!iomem_access_permitted(d, start_gfn, start_gfn + nr))
> +        return 0;

This would seem to belong into common code

Also - coding style.

> +    res = map_mmio_regions(d, start_gfn, nr, mfn);
> +    if ( res < 0 )
> +    {
> +        printk(XENLOG_ERR "Unable to map 0x%lx - 0x%lx in domain %d\n",

%#lx

> +               start_gfn << PAGE_SHIFT, (start_gfn + nr) << PAGE_SHIFT,

I see no reason for the shifts.

> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -980,6 +980,14 @@ long do_memory_op(unsigned long cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>          if ( d == NULL )
>              return -ESRCH;
>  
> +        /*
> +         * XENMAPSPACE_dev_mmio mapping is only supported for hardware Domain
> +         * to map this kind of space to itself.
> +         */
> +        if ( (xatp.space == XENMAPSPACE_dev_mmio) &&
> +             (!is_hardware_domain(current->domain) || (d != 
> current->domain)) )

Readability would benefit if you used "d" twice and "current->domain"
just once, preferable after swapping the two sides of the ||.

Overall I wonder whether this wouldn't help PVH on x86 too,
where we currently do some hackery to (not even completely)
map MMIO into Dom0's p2m. In such a case perhaps
map_dev_mmio_regions() should become a general per-arch
function right away (declared in a common header and stubbed
out in x86 code for now). Boris, Roger?

Jan


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