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

[Ian Campbell <Ian.Campbell@citrix.com>]

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")

> 
> 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.

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.

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.

> +        {
> +            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?

>      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.

>          {
> -            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),
> +                                   DIV_ROUND_UP(size, PAGE_SIZE),
> +                                   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;
> +            }
>          }
>      }
>