Re: [PATCH v8 09/14] xen/x86: factor out map and unmap from the memory_mapping DOMCTL

["Jan Beulich" <JBeulich@suse.com>]

>>> On 25.05.14 at 12:51, <avanzini.arianna@gmail.com> wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -670,17 +670,14 @@ long arch_do_domctl(
>                     d->domain_id, gfn, mfn, nr_mfns);
>  
>              ret = iomem_permit_access(d, mfn, mfn_end);
> -            if ( !ret && paging_mode_translate(d) )
> +            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, nr_mfns, _mfn(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));
> +                           d->domain_id, gfn, mfn, ret);

I'm not sure how useful retaining this message is now that doesn't
indicated the precise GFN/MFN anymore. At the very least you
should make this explicit by also printing nr_mfns.

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1693,6 +1693,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));
> +        if ( ret )
> +        {
> +            unmap_mmio_regions(d, start_gfn, start_gfn + i, mfn);

Wrong 3rd argument (should be just i).

> +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));
> +
> +    return ret;

You're still discarding an eventual error here if the last operation
succeeds.

Also, now that these two are standalone functions, I think it would
make sense to rename "nr_mfns" to just "nr", as the value
expresses both the number of MFNs and GFNs.

Jan