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

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

On 03/02/2014 10:56 AM, Julien Grall wrote:
> Hello Arianna,
> 

Hello,

thank you all for your comments. I'm currently trying to address at my best the
issues pointed out throughout the discussion. Also, sorry for the delay in replying.


> On 02/03/14 08:49, Arianna Avanzini wrote:
>> This commit introduces a first attempt of implementation of the
>> XEN_DOMCTL_memory_mapping hypercall for arm32. The following
>> warnings must be taken into consideration:
>> . currently, the hypercall simply performs an 1:1 mapping of I/O
>>    memory ranges (mfn == gfn);
>> . the range of I/O memory addresses is mapped all at once with
>>    map_mmio_regions();
>> . the hypercall takes for granted that any I/O memory range can
>>    be mapped to a domU if the current domain is dom0.
>>
>> 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: Eric Trudeau <etrudeau@broadcom.com>
>> Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
>> ---
>>   xen/arch/arm/arm32/domctl.c | 74 +++++++++++++++++++++++++++++++++++++++++++++
> 
> The implementation of XEN_DOMCTL_memory_mapping is not ARM32 specific. It can be
> implemented in arch/arm/domctl.c.
> 
>>   xen/arch/arm/p2m.c          |  9 ++++++
>>   xen/include/asm-arm/p2m.h   |  2 ++
>>   3 files changed, 85 insertions(+)
>>
>> diff --git a/xen/arch/arm/arm32/domctl.c b/xen/arch/arm/arm32/domctl.c
>> index c2ca4d3..67cf734 100644
>> --- a/xen/arch/arm/arm32/domctl.c
>> +++ b/xen/arch/arm/arm32/domctl.c
>> @@ -10,8 +10,11 @@
>>   #include <xen/errno.h>
>>   #include <xen/sched.h>
>>   #include <xen/hypercall.h>
>> +#include <xen/iocap.h>
>>   #include <public/domctl.h>
>>
>> +#include "cpu.h"
>> +
>>   long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>   {
>> @@ -19,6 +22,77 @@ long subarch_do_domctl(struct xen_domctl *domctl, struct
>> domain *d,
>>       {
>>       case XEN_DOMCTL_set_address_size:
>>           return domctl->u.address_size.size == 32 ? 0 : -EINVAL;
>> +    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;
>> diff --git a/xen/arch/arm/arm32/domctl.c b/xen/arch/arm/
>> +
>> +        ret = -EPERM;
>> +    /*
>> +     * NOTE: dom0 seems to have empty iomem_caps but to be however able to
>> +     *       access I/O memory ranges. The following check takes for granted
>> +     *       that any iomem range can be mapped to a domU if the current
>> +     *       domain is dom0.
>> +     */
>> +        if ( current->domain->domain_id != 0 &&
>> +             !iomem_access_permitted(current->domain, mfn, mfn + nr_mfns - 1) )
>> +            return ret;
> 
> This check can be removed by adding in construct_dom0 (arch/arm/domain_build.c)
> something like that:
>   /* DOM0 is permitted full I/O capabilities */
>   rc = iomem_permit_access(d, 0UL, ~OUL);
> 
> I'm wondering if we can even restrict dom0 I/0 access by only permit access on
> devices passthrough to it. Because dom0 should not be allowed to map I/O ranges
> which belong to device used by Xen e.g : GIC, RAM,...
> 
> I think we should at least restrict dom0 to use the hypercall for mapping device
> memory. Otherwise dom0 may be allowed to map Xen address range, do wrong thing
> with foreign mapping...
> 
>> +
>> +        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 )
>> +            {
>> +                /* 1:1 iomem mapping (gfn == mfn) */
>> +                ret = map_mmio_regions(d, gfn, gfn + nr_mfns - 1, mfn);
> 
> The comment "1:1 iomem mapping (gfn == mfn)" seems wrong here. The
> implementation you wrote allow gfn != mfn.
> 

Right, thank you.
Sorry if I bother you with one more question about this hunk; aside from the
error pointed out by Eric with the usage of pfn instead of paddr_t, is it OK to
map the address range all at once with map_mmio_regions() as proposed? I noticed
that the x86-related code seems to map the range one mfn at a time with
set_mmio_p2m_entry(), and I wonder if this different approach has any benefit I
didn't think about.



> Regards,
> 


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