[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v2 05/21] xen/arm: follow-up to allow DOM0 manage IRQ and MMIO



Hi Ian,

On 09/09/14 06:07, Ian Campbell wrote:
On Thu, 2014-07-31 at 16:00 +0100, Julien Grall wrote:

Please make the subject line reflect what this patches function is, not
that it is a follow up to some other patch.

The commit 33233c2 "arch/arm: domain build: let dom0 access I/O memory of
mapped series" fill the iomem_caps to allow DOM0 managing MMIO of mapped
device.

"fills in ... to allow DOM0 to manage MMIO of mapped devices" (I think
that is what was meant")

Yes, I will change in the next version.


A device can be disabled (i.e by adding a property status="disabled" in the
device tree) because the user may want to passthrough this device to a guest.
This will avoid DOM0 loading (and few minutes after unloading) the driver to

s/after/later/

handle this device.

Even though, we don't want to let DOM0 using this device, the domain needs

"use". And I think s/the domain/it/ since you are referring to DOM0
(just mentioned, so "it" is fine in the context) not the domain as in
the guest.

to be able to manage the MMIO/IRQ range. This will allow the toolstack
to map MMIO/IRQ to a guest explicitly via "iomem" and "irqs" VM config
property or implicitly by passthrough a device.

"by passing through"

Rework the function map_device (renamed into handle_device) to:

* For a given device node:
     - Give permission to manage IRQ/MMIO for this device
     - Retrieve the IRQ configuration (i.e edge/level) from the device
     tree
* For available device (i.e status != disabled in the DT)
     - Assign the device to the guest if it's protected by an IOMMU
     - Map the IRQs and MMIOs regions to the guest

So based on that I think a suitable $subject would be something like
"give domain 0 permission to manage IRQ/MMIO for disabled devices" or
something like that.

It's a follow-up in the sense that it improves Arianna patch to handle MMIO and IRQ.

Your suggestion for the title sounds good. I will use it.

Is it a good idea to override status="disabled" in this way or should be
add our own xen,passthrough as a boolean DT property?

My concern is that we are also doing this for devices which are disabled
for some other reason (buggy? security sensitive? not present on this
partiocular board etc). It also leaves no way to mark a device as "not
for any domain, including dom0 or passthrough". Quite a few .dtsi files
define loads of stuff as disabled and then let the board files override
what they actually have as okay.

So I think keeping status="disabled" as more of a firmware thing and
having our own for devices which are to be passed through makes sense.

Using status="disabled" was an easy solution because Linux, and most of the other OS, knows the device should not be used by DOM0.

The current solution doesn't map the device into DOM0 memory but the device is still described in the device tree. That would let us the possibility to retrieve from DOM0 the properties/compatible of the device via a driver (and would avoid the bunch of hypercalls introduced later).

With the new property "xen,passthrough", we would have to remove the node from DOM0, or teach DOM0 that the device should not be used.

Overall, I don't think dropping the node in DOM0 device tree will impact it. If it's the case that would mean the device should not be passthrough to another guest. So I will give a look to introduce this new property. Shall I send a patch to the device tree bindings ML?

BTW, I don't think the new property should be a boolean. Use only the name should be enough here.

Since I expect the naming of the property isn't going to have a major
impact on the code itself outside of a few predicate functions I took a
look and it looks good, apart from a few nits which I'll mention.

@@ -957,7 +965,7 @@ static int map_device(struct domain *d, struct 
dt_device_node *dev)
          }
      }

-    /* Map IRQs */
+    /* Give permission and  map IRQs */

double space.

      for ( i = 0; i < nirq; i++ )
      {
          res = dt_device_get_raw_irq(dev, i, &rirq);
@@ -990,16 +998,28 @@ static int map_device(struct domain *d, struct 
dt_device_node *dev)
          irq = res;

          DPRINT("irq %u = %u\n", i, irq);
-        res = route_irq_to_guest(d, irq, dt_node_name(dev));
+
+        res = irq_permit_access(d, irq);
          if ( res )
          {
-            printk(XENLOG_ERR "Unable to route IRQ %u to domain %u\n",
-                   irq, d->domain_id);
+            printk(XENLOG_ERR "Unable to permit to dom%u access to IRQ %u\n",
+                   d->domain_id, irq);
              return res;
          }
+
+        if ( available )

if ( !avaiable ) \n continue would save you an indentation level.

Ok.

+        {
+            res = route_irq_to_guest(d, irq, dt_node_name(dev));
+            if ( res )
+            {
+                printk(XENLOG_ERR "Unable to route IRQ %u to domain %u\n",
+                       irq, d->domain_id);
+                return res;
+            }
+        }
      }

-    /* Map the address ranges */
+    /* Give permission and map MMIOs */

Permissions are now given above, not below, aren't they?

Only permission for IRQ. The MMIO ones are given few lines below.

      for ( i = 0; i < naddr; i++ )
      {
          res = dt_device_get_address(dev, i, &addr, &size);
@@ -1023,17 +1043,21 @@ static int map_device(struct domain *d, struct 
dt_device_node *dev)
                     addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
              return res;
          }
-        res = map_mmio_regions(d,
-                               paddr_to_pfn(addr & PAGE_MASK),
-                               DIV_ROUND_UP(size, PAGE_SIZE),
-                               paddr_to_pfn(addr & PAGE_MASK));
-        if ( res )
+
+        if ( available )

if ( !avaiable ) \n continue again.

Ok.

Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.