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

[Julien Grall <julien.grall@linaro.org>]

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.

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;



-- 
Julien Grall