Re: [RFC PATCH v2 2/3] arch, arm: add the XEN_DOMCTL_memory_mapping hypercall

[Arianna Avanzini <avanzini.arianna@gmail.com>]

On 03/10/2014 01:03 PM, Julien Grall wrote:
> Hello Arianna,
> 
> On 03/10/2014 08:25 AM, Arianna Avanzini wrote:
>> This commit introduces a first attempt of implementation of the
>> XEN_DOMCTL_memory_mapping hypercall for ARM. The range of I/O
>> memory addresses is mapped all at once with map_mmio_regions().
>>
>> Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
>> Cc: Dario Faggioli <dario.faggioli@citrix.com>
>> Cc: Paolo Valente <paolo.valente@unimore.it>
>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> Cc: Julien Grall <julien.grall@citrix.com>
>> Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>
>> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> Cc: Eric Trudeau <etrudeau@broadcom.com>
>> Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
>> ---
>>  xen/arch/arm/domctl.c     | 69 +++++++++++++++++++++++++++++++++++++++++++++++
>>  xen/arch/arm/p2m.c        |  9 +++++++
>>  xen/include/asm-arm/p2m.h |  2 ++
>>  3 files changed, 80 insertions(+)
>>
>> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
>> index 45974e7..078b165 100644
>> --- a/xen/arch/arm/domctl.c
>> +++ b/xen/arch/arm/domctl.c
>> @@ -10,6 +10,7 @@
>>  #include <xen/errno.h>
>>  #include <xen/sched.h>
>>  #include <xen/hypercall.h>
>> +#include <xen/iocap.h>
>>  #include <public/domctl.h>
>>  
>>  long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>> @@ -30,7 +31,75 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>  
>>          return p2m_cache_flush(d, s, e);
>>      }
>> +    case XEN_DOMCTL_memory_mapping:
>> +    {
>> +        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;
>> +        int add = domctl->u.memory_mapping.add_mapping;
>> +        long int ret;
>> +
>> +        ret = -EINVAL;
>> +        if ( (mfn + nr_mfns - 1) < mfn || /* wrap? */
>> +             ((mfn | (mfn + nr_mfns - 1)) >> (PADDR_BITS - PAGE_SHIFT)) ||
>> +             (gfn + nr_mfns - 1) < gfn ) /* wrap? */
>> +            return ret;
>> +
>> +        ret = -EPERM;
>> +        if ( !iomem_access_permitted(current->domain, mfn, mfn + nr_mfns - 1) )
>> +            return ret;
>>  
>> +        ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn + nr_mfns - 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 + nr_mfns - 1);
>> +            if ( !ret )
>> +            {
>> +                ret = map_mmio_regions(d, PAGE_ALIGN(pfn_to_paddr(gfn)),
>> +                                       PAGE_ALIGN(
>> +                                           pfn_to_paddr(gfn + nr_mfns)) - 1,
>> +                                       PAGE_ALIGN(pfn_to_paddr(mfn)));
>> +                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 + nr_mfns - 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 + nr_mfns - 1);
>> +                }
>> +            }
>> +        }
>> +	else
>> +        {
>> +            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, PAGE_ALIGN(pfn_to_paddr(gfn)),
>> +                                     PAGE_ALIGN(
>> +                                         pfn_to_paddr(gfn + nr_mfns)) - 1,
>> +                                     PAGE_ALIGN(pfn_to_paddr(mfn)));
>> +            ret = iomem_deny_access(d, mfn, mfn + nr_mfns - 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 + nr_mfns - 1);
> 
> 
> The unmap part doesn't seem correct to me. With your solution, you are
> allowed to remove any gfn as long as the current domain is permitted to
> modify the mfn. No matter if gfn is effectively mapped to the mfn or not.
> 
> You should at least check that gfn is typed p2m_mmio_direct (This is
> done by clean_mmio_p2m_entry on x86). You can also check that the gfn is
> mapped to the mfn.
> 
> I would do that in the switch REMOVE in apply_p2m_changes.
> 

OK, thank you for the detailed feedback and for the suggestions. I'll certainly
try to implement the checks you indicated.


>> +	}
>> +        return ret;
>> +    }
>>      default:
>>          return subarch_do_domctl(domctl, d, u_domctl);
>>      }
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index d00c882..710f74e 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -461,6 +461,15 @@ int map_mmio_regions(struct domain *d,
>>                               maddr, MATTR_DEV, p2m_mmio_direct);
>>  }
>>  
>> +int unmap_mmio_regions(struct domain *d,
>> +                       paddr_t start_gaddr,
>> +                       paddr_t end_gaddr,
>> +                       paddr_t maddr)
>> +{
> 
> Can you use pfn instead of physical address?
> 

Sure, thank you for the feedback.
Sorry if I bother you with another question, unmap_mmio_regions() is a wrapper
to apply_p2m_changes(), which takes paddr_t as parameters. Is it OK to just have
unmap_mmio_regions() take pfn as parameters and then convert them to paddr_t
when calling apply_p2m_changes(), or would you prefer that changes are performed
also to apply_p2m_changes()?


> Regards,
> 


-- 
/*
 * Arianna Avanzini
 * avanzini.arianna@gmail.com
 * 73628@studenti.unimore.it
 */