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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.03.14 at 21:11, Arianna Avanzini <avanzini.arianna@xxxxxxxxx> wrote:
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1591,6 +1591,45 @@ unsigned long paging_gva_to_gfn(struct vcpu *v,
>      return hostmode->gva_to_gfn(v, hostp2m, va, pfec);
>  }
>  
> +int map_mmio_regions(struct domain *d,
> +                     paddr_t start_gaddr,
> +                     paddr_t end_gaddr,
> +                     paddr_t maddr)
> +{
> +    int i, ret = 0;

"i" was unsigned long in the original code, and I don't see any proof
of either the signedness or the width change being correct.

> +    unsigned long gfn = paddr_to_pfn(start_gaddr);
> +    unsigned long nr_mfns = paddr_to_pfn(end_gaddr) - 
> paddr_to_pfn(start_gaddr);

So this implies the range is exclusive of end_gaddr.

> +    unsigned long mfn = paddr_to_pfn(maddr);
> +
> +    if ( !paging_mode_translate(d) )
> +        return 0;
> +
> +    for ( i = 0; !ret && i < nr_mfns; i++ )
> +        if ( !set_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i)) )
> +            ret = -EIO;
> +    while ( i-- )
> +        clear_mmio_p2m_entry(d, gfn + i);

So you're clearing all entries unconditionally again right away? This
surely wants to be inside an "if ( ret )".

> +int unmap_mmio_regions(struct domain *d,
> +                       unsigned long start_gfn,
> +                       unsigned long end_gfn,
> +                       unsigned long mfn)
> +{
> +    unsigned long nr_mfns = end_gfn - start_gfn;
> +    int i, ret = 0;

See above.

> +
> +    if ( !paging_mode_translate(d) )
> +        return 0;
> +
> +    for ( i = 0; i < nr_mfns; i++ )
> +        ret |= !clear_mmio_p2m_entry(d, start_gfn + i);

With this you pretty clearly want the function to have bool_t as its
return type.

> +    case XEN_DOMCTL_memory_mapping:
> +    {
> +        unsigned long gfn = op->u.memory_mapping.first_gfn;
> +        unsigned long mfn = op->u.memory_mapping.first_mfn;
> +        unsigned long nr_mfns = op->u.memory_mapping.nr_mfns;
> +        unsigned long mfn_end = mfn + nr_mfns;
> +        unsigned long gfn_end = gfn + nr_mfns;
> +        int add = op->u.memory_mapping.add_mapping;
> +        long int ret;
> +
> +        ret = -EINVAL;
> +        if ( (mfn_end - 1) < mfn || /* wrap? */
> +             ((mfn | (mfn_end - 1)) >> (paddr_bits - PAGE_SHIFT)) ||
> +             (gfn_end - 1) < gfn ) /* wrap? */
> +            return ret;
> +
> +        ret = -EPERM;
> +        if ( !iomem_access_permitted(current->domain, mfn, mfn_end - 1) )
> +            return ret;
> +
> +        ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn_end - 1, add);
> +        if ( ret )
> +            return ret;
> +
> +        if ( add )
> +        {
> +            printk(XENLOG_G_INFO
> +                   "memory_map: add: dom%d gfn=%lx mfn=%lx nr=%lx\n",
> +                   d->domain_id, gfn, mfn, nr_mfns);
> +            ret = iomem_permit_access(d, mfn, mfn_end - 1);
> +            if ( !ret )
> +            {
> +                ret = map_mmio_regions(d, PAGE_ALIGN(pfn_to_paddr(gfn)),
> +                                       PAGE_ALIGN(
> +                                           pfn_to_paddr(gfn_end)) - 1,

Contrary to the above, this implies the range to inclusive of the
ending address.

Furthermore, PAGE_ALIGN(pfn_to_paddr()) is rather pointless a
construct - the outer PAGE_ALIGN() can't possibly have any effect.

> +/* 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);

Please have the two interfaces be consistent: Either both take
frame numbers only (preferred), or both take addresses only.

Jan

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