Re: [PATCH v6 01/11] arch, arm: domain build: let dom0 access I/O memory of mapped devices

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

On 04/29/2014 03:44 PM, Julien Grall wrote:
> On 04/29/2014 01:37 PM, Julien Grall wrote:
>> Hi Arianna,
>>
>> On 04/21/2014 02:44 PM, Arianna Avanzini wrote:
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index 187e071..1802b6e 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -11,6 +11,7 @@
>>>  #include <xen/device_tree.h>
>>>  #include <xen/libfdt/libfdt.h>
>>>  #include <xen/guest_access.h>
>>> +#include <xen/iocap.h>
>>>  #include <asm/setup.h>
>>>  #include <asm/platform.h>
>>>  #include <asm/psci.h>
>>> @@ -740,6 +741,16 @@ static int map_device(struct domain *d, const struct dt_device_node *dev)
>>>          DPRINT("addr %u = 0x%"PRIx64" - 0x%"PRIx64"\n",
>>>                 i, addr, addr + size - 1);
>>>  
>>> +        res = iomem_permit_access(d, paddr_to_pfn(addr & PAGE_MASK),
>>> +                                  paddr_to_pfn(PAGE_ALIGN(addr + size - 1)));
>>> +        if ( res )
>>> +        {
>>> +            printk(XENLOG_ERR "Unable to permit to dom%d access to"
>>> +                   " 0x%"PRIx64" - 0x%"PRIx64"\n",
>>> +                   d->domain_id,
>>> +                   addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
>>> +            return res;
>>> +        }
>>>          res = map_mmio_regions(d, addr & PAGE_MASK,
>>>                                 PAGE_ALIGN(addr + size) - 1,
>>>                                 addr & PAGE_MASK);
>>>
>>
>> I though a bit more about this patch. If the device is disabled (i.e
>> status="disabled"), Xen doesn't call map_device.
>> Futhermore, in your use case (e.g with iomem=) you might want to map
>> memory that is not describe to the device tree.
>>
>> I would either:
>> 	1) give a full access to the I/O range
>> 	2) give a full access to the I/O range except on the RAM region
>>
>> IHMO, the second solution might be better but I don't know if it's easy
>> to implement it.
> 
> I quickly wrote a follow-up of the series (see below). We can either merge
> in this patch or I can carry it with the device passthrough patch series.
> 

For me it is OK that the follow-up is kept in the device passthrough patch
series, if it's fine for you.
If you agree, I'll update the commit description of the 0001 patch in the
memory_mapping patchset and try to explain its limitations with regard to the
scenario you highlighted.

Also, sorry for the delay; I'll be surely sending a v7 of the memory_mapping
series in the next few days.


> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 2f6ffe9..3498549 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -717,9 +717,14 @@ static int make_timer_node(const struct domain *d, void *fdt,
>      return res;
>  }
>  
> -/* Map the device in the domain */
> -static int map_device(struct domain *d, struct kernel_info *kinfo,
> -                      struct dt_device_node *dev)
> +/* For a specific device node :
> + *  - Give access permission to the guest
> + * When the device is available:
> + *  - Assign the device to the guest if it's protected by an IOMMU
> + *  - Map the IRQs and iomem regions to DOM0
> + */
> +static int handle_device(struct domain *d, struct kernel_info *kinfo,
> +                         struct dt_device_node *dev, bool_t map)
>  {
>      unsigned int nirq;
>      unsigned int naddr;
> @@ -734,7 +739,7 @@ static int map_device(struct domain *d, struct kernel_info *kinfo,
>  
>      DPRINT("%s nirq = %d naddr = %u\n", dt_node_full_name(dev), nirq, naddr);
>  
> -    if ( dt_device_is_protected(dev) )
> +    if ( dt_device_is_protected(dev) && map )
>      {
>          DPRINT("%s setup iommu\n", dt_node_full_name(dev));
>          res = iommu_assign_dt_device(d, dev);
> @@ -778,12 +783,15 @@ static int map_device(struct domain *d, struct kernel_info *kinfo,
>          }
>  
>          DPRINT("irq %u = %u type = 0x%x\n", i, irq.irq, irq.type);
> -        res = route_dt_irq_to_guest(d, &irq, dt_node_name(dev));
> -        if ( res )
> +        if ( map )
>          {
> -            printk(XENLOG_ERR "Unable to route IRQ %u to domain %u\n",
> -                   irq.irq, d->domain_id);
> -            return res;
> +            res = route_dt_irq_to_guest(d, &irq, dt_node_name(dev));
> +            if ( res )
> +            {
> +                printk(XENLOG_ERR "Unable to route IRQ %u to domain %u\n",
> +                       irq.irq, d->domain_id);
> +                return res;
> +            }
>          }
>      }
>  
> @@ -811,17 +819,21 @@ static int map_device(struct domain *d, struct kernel_info *kinfo,
>                     addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
>              return res;
>          }
> -        res = map_mmio_regions(d,
> -                               paddr_to_pfn(addr & PAGE_MASK),
> -                               paddr_to_pfn_aligned(addr + size) - 1,
> -                               paddr_to_pfn(addr & PAGE_MASK));
> -        if ( res )
> +
> +        if ( map )
>          {
> -            printk(XENLOG_ERR "Unable to map 0x%"PRIx64
> -                   " - 0x%"PRIx64" in domain %d\n",
> -                   addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1,
> -                   d->domain_id);
> -            return res;
> +            res = map_mmio_regions(d,
> +                                   paddr_to_pfn(addr & PAGE_MASK),
> +                                   paddr_to_pfn_aligned(addr + size) - 1,
> +                                   paddr_to_pfn(addr & PAGE_MASK));
> +            if ( res )
> +            {
> +                printk(XENLOG_ERR "Unable to map 0x%"PRIx64
> +                       " - 0x%"PRIx64" in domain %d\n",
> +                       addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1,
> +                       d->domain_id);
> +                return res;
> +            }
>          }
>      }
>  
> @@ -902,10 +914,9 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
>       *  property. Therefore these device doesn't need to be mapped. This
>       *  solution can be use later for pass through.
>       */
> -    if ( !dt_device_type_is_equal(node, "memory") &&
> -         dt_device_is_available(node) )
> +    if ( !dt_device_type_is_equal(node, "memory") )
>      {
> -        res = map_device(d, kinfo, node);
> +        res = handle_device(d, kinfo, node, dt_device_is_available(node));
>  
>          if ( res )
>              return res;
> 
> 
> 


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