Re: [Xen-devel] [PATCH v6 06/11] xen, x86: factor out map and unmap from the memory_mapping DOMCTL

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.04.14 at 15:44, <avanzini.arianna@xxxxxxxxx> wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -646,46 +646,45 @@ long arch_do_domctl(
>          unsigned long gfn = domctl->u.memory_mapping.first_gfn;
>          unsigned long mfn = domctl->u.memory_mapping.first_mfn;
>          unsigned long nr_mfns = domctl->u.memory_mapping.nr_mfns;
> +        unsigned long mfn_end = mfn + nr_mfns - 1;
> +        unsigned long gfn_end = gfn + nr_mfns - 1;
>          int add = domctl->u.memory_mapping.add_mapping;
> -        unsigned long i;
>  
>          ret = -EINVAL;
> -        if ( (mfn + nr_mfns - 1) < mfn || /* wrap? */
> -             ((mfn | (mfn + nr_mfns - 1)) >> (paddr_bits - PAGE_SHIFT)) ||
> -             (gfn + nr_mfns - 1) < gfn ) /* wrap? */
> -            break;
> +        if ( mfn_end < mfn || /* wrap? */
> +             ((mfn | mfn_end) >> (paddr_bits - PAGE_SHIFT)) ||
> +             gfn_end < gfn ) /* wrap? */
> +            return ret;
>  
>          ret = -EPERM;
> -        if ( !iomem_access_permitted(current->domain, mfn, mfn + nr_mfns - 
> 1) )
> +        if ( !iomem_access_permitted(current->domain, mfn, mfn_end) )
>              break;
>  
> -        ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn + nr_mfns - 1, add);
> +        ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn_end, add);
>          if ( ret )
>              break;
>  
>          if ( add )
>          {
>              printk(XENLOG_G_INFO
> -                   "memory_map:add: dom%d gfn=%lx mfn=%lx nr=%lx\n",
> +                   "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 + nr_mfns - 1);
> -            if ( !ret && paging_mode_translate(d) )
> +            ret = iomem_permit_access(d, mfn, mfn_end);
> +            if ( !ret )
>              {
> -                for ( i = 0; !ret && i < nr_mfns; i++ )
> -                    ret = set_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i));
> +                ret = map_mmio_regions(d, gfn, gfn_end, mfn);
>                  if ( ret )
>                  {
>                      printk(XENLOG_G_WARNING
> -                           "memory_map:fail: dom%d gfn=%lx mfn=%lx 
> ret:%ld\n",
> -                           d->domain_id, gfn + i, mfn + i, ret);
> -                    while ( i-- )
> -                        clear_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i));
> -                    if ( iomem_deny_access(d, mfn, mfn + nr_mfns - 1) &&
> +                           "memory_map: fail: dom%d gfn=%lx mfn=%lx 
> ret:%ld\n",
> +                           d->domain_id, gfn, mfn, ret);
> +                    if ( iomem_deny_access(d, mfn, mfn_end) &&
>                           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 + nr_mfns - 1);
> +                               "memory_map: failed to deny dom%d access "
> +                               "to [%lx,%lx]\n",

Please don't split format strings like this, unless they become _really_
long (such that when grep-ing for any part of the string it will be
found).

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1657,6 +1657,48 @@ 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,
> +                     unsigned long start_gfn,
> +                     unsigned long end_gfn,
> +                     unsigned long mfn)

The last parameter her should be mfn_t.

> +int unmap_mmio_regions(struct domain *d,
> +                       unsigned long start_gfn,
> +                       unsigned long end_gfn,
> +                       unsigned long mfn)
> +{
> +    int ret = 0;
> +    unsigned long nr_mfns = end_gfn - start_gfn + 1;
> +    unsigned long i;
> +
> +    if ( !paging_mode_translate(d) )
> +        return 0;
> +
> +    for ( i = 0; i < nr_mfns; i++ )
> +        ret |= !clear_mmio_p2m_entry(d, start_gfn + i, _mfn(mfn + i));

Elsewhere you updated to Mukesh's recent changes, yet this one
certainly is wrong now - clear_mmio_p2m_entry() now returns 0 on
success. When you re-base patches, you really should be rather
careful when you have to adjust code being removed in one place and
added back in another: In almost all cases you'll have to adjust both,
even if the patch applies fine without changes on the adding-back side.

Also, now that you have this as a separate function, perhaps it
would be better to use this for the error recovery path in
map_mmio_regions()?

Jan

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