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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.05.14 at 17:54, <avanzini.arianna@xxxxxxxxx> wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -646,19 +646,21 @@ 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;
>  
>          ret = -EINVAL;
> -        if ( (mfn + nr_mfns - 1) < mfn || /* wrap? */
> -             ((mfn | (mfn + nr_mfns - 1)) >> (paddr_bits - PAGE_SHIFT)) ||
> -             (gfn + nr_mfns - 1) < gfn ) /* wrap? */
> +        if ( mfn_end < mfn || /* wrap? */
> +             ((mfn | mfn_end) >> (paddr_bits - PAGE_SHIFT)) ||
> +             gfn_end < gfn ) /* wrap? */
>              break;
>  
>          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;
>  

With the switch to passing nr_mfns to the helper functions all of the
above and some of the change further down (i.e. anything substituting
[gm]fn_end are now unrelated to the patch. If you feel strongly about
doing this replacement, this should be a separate patch, making the
review of this one easier.

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1650,6 +1650,47 @@ 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 nr_mfns,
> +                     mfn_t mfn)
> +{
> +    int ret = 0;
> +    unsigned long i;
> +
> +    if ( !paging_mode_translate(d) )
> +        return 0;
> +
> +    for ( i = 0; !ret && i < nr_mfns; i++ )
> +        ret |= set_mmio_p2m_entry(d, start_gfn + i, _mfn(mfn_x(mfn) + i));

The |= it pointless considering the loop termination condition. Just
use =.

> +    if ( ret )
> +    {
> +        unmap_mmio_regions(d, start_gfn, start_gfn + i, mfn);
> +        ret = -EIO;

And without using |= there's then also no point in overriding the
return value from set_mmio_p2m_entry().

> +int unmap_mmio_regions(struct domain *d,
> +                       unsigned long start_gfn,
> +                       unsigned long nr_mfns,
> +                       mfn_t mfn)
> +{
> +    int ret = 0;
> +    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_x(mfn) + i));
> +
> +    if ( ret )
> +        return -EIO;

Same here regarding the return value override: Avoid the |= and
instead latch either the first or last error (if any), just like done by
the original code.

Jan


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