Re: [PATCH v3 3/5] xen, common: add the XEN_DOMCTL_memory_mapping hypercall

[Julien Grall <julien.grall@linaro.org>]

Hello Arianna,

Thank for the patch.

On 15/03/14 20:11, Arianna Avanzini wrote:
> diff --git a/xen/arch/arm/cpu.c b/xen/arch/arm/cpu.c
> index afc564f..28f2611 100644
> --- a/xen/arch/arm/cpu.c
> +++ b/xen/arch/arm/cpu.c
> @@ -17,6 +17,8 @@
>
>   #include <asm/processor.h>
>
> +unsigned int paddr_bits __read_mostly = 40;
> +

I would prefer a define for paddr_bits on ARM rather than creating a new 
variable. Something like #define paddr_bits PADDR_BITS in asm-arm/p2m.h

Furthermore, you should not hardcode 40, you must use PADDR_BITS.

[..]

> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 7cf610a..b18fd46 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -818,6 +818,75 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)

[..]

> +                if ( ret )
> +                {
> +                    printk(XENLOG_G_WARNING
> +                           "memory_map: fail: dom%d gfn=%lx mfn=%lx\n",
> +                           d->domain_id, gfn, mfn);
> +                    if ( iomem_deny_access(d, mfn, mfn_end - 1) &&
> +                         is_hardware_domain(current->domain) )
> +                        printk(XENLOG_ERR
> +                               "memory_map: failed to deny dom%d access "
> +                               "to [%lx,%lx]\n",
> +                               d->domain_id, mfn, mfn_end - 1);
> +                }
> +            }
> +        }
> +	else

Hard tab here. You should use space.

> +        {
> +            printk(XENLOG_G_INFO
> +                   "memory_map: remove: dom%d gfn=%lx mfn=%lx nr=%lx\n",
> +                   d->domain_id, gfn, mfn, nr_mfns);
> +
> +            add = unmap_mmio_regions(d, gfn, gfn_end - 1, mfn);
> +            ret = iomem_deny_access(d, mfn, mfn_end - 1);
> +            if ( ret && add )
> +                ret = -EIO;
> +            if ( ret && is_hardware_domain(current->domain) )
> +                printk(XENLOG_ERR
> +                       "memory_map: error %ld %s dom%d access to [%lx,%lx]\n",
> +                       ret, add ? "removing" : "denying", d->domain_id,
> +                       mfn, mfn_end - 1);
> +	}
> +    }
> +    break;
> +
>       case XEN_DOMCTL_settimeoffset:
>       {
>           domain_set_time_offset(d, op->u.settimeoffset.time_offset_seconds);
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 3b39c45..00e3bba 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -88,6 +88,10 @@ int p2m_populate_ram(struct domain *d, paddr_t start, paddr_t end);
>    * address maddr. */
>   int map_mmio_regions(struct domain *d, paddr_t start_gaddr,
>                        paddr_t end_gaddr, paddr_t maddr);
> +int unmap_mmio_regions(struct domain *d,
> +                       unsigned long start_gfn,
> +                       unsigned long end_gfn,
> +                       unsigned long mfn);
>
>   int guest_physmap_add_entry(struct domain *d,
>                               unsigned long gfn,
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index 06e638f..4869a75 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -119,6 +119,8 @@
>
>   #include <xen/types.h>
>
> +extern unsigned int paddr_bits;
> +
>   struct cpuinfo_arm {
>       union {
>           uint32_t bits;
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index f4e7253..91ef110 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -657,6 +657,16 @@ void p2m_flush(struct vcpu *v, struct p2m_domain *p2m);
>   /* Flushes all nested p2m tables */
>   void p2m_flush_nestedp2m(struct domain *d);
>
> +/* Map MMIO regions in the p2m: start_gaddr and end_gaddr is the range
> + * in the guest physical address space to map, starting from the machine
> + * address maddr. */
> +int map_mmio_regions(struct domain *d, paddr_t start_gaddr,
> +                     paddr_t end_gaddr, paddr_t maddr);
> +int unmap_mmio_regions(struct domain *d,
> +                       unsigned long start_gfn,
> +                       unsigned long end_gfn,
> +                       unsigned long mfn);
> +

The both functions are used on common code. I would prefer to define the 
prototype only once in common header (e.g in include/xen).

At the same time, it's not necessary to give the full addresses on 
argument. GFN, MFN is enough.

I think we already talk about it on V1. I said it wasn't urgent, but as 
the function is common now ... :)

Regards,

-- 
Julien Grall