Re: [Xen-devel] [PATCH v3 07/24] xen/arm: Introduce xen, passthrough property

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 13 Jan 2015, Julien Grall wrote:
> When a device is marked for passthrough (via the new property 
> "xen,passthrough"),
> dom0 must not access to the device (i.e not loading a driver), but should
> be able to manage the MMIO/interrupt of the passthrough device.
> 
> The latter part will allow the toolstack to map MMIO/IRQ when a device
> is pass through to a guest.
> 
> The property "xen,passthrough" will be translated as 'status="disabled"'
> in the device tree to avoid DOM0 using the device. We assume that DOM0 is
> able to cope with this property (already the case for Linux).
> 
> 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
> * When the device is not marked for guest passthrough:
>     - Assign the device to the guest if it's protected by an IOMMU
>     - Map the IRQs and MMIOs regions to the guest
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> 
> ---
>     Changes in v3:
>         - This patch was formely "xen/arm: Follow-up to allow DOM0
>         manage IRQ and MMIO". It has been split in 2 parts [1].
>         - Update commit title and improve message
>         - Remove spurious change
> 
> [1] https://patches.linaro.org/34669/
> ---
>  docs/misc/arm/device-tree/passthrough.txt |   7 ++
>  xen/arch/arm/device.c                     |   2 +-
>  xen/arch/arm/domain_build.c               | 102 
> ++++++++++++++++++++++--------
>  xen/common/device_tree.c                  |   6 ++
>  xen/include/xen/device_tree.h             |  11 ++++
>  5 files changed, 100 insertions(+), 28 deletions(-)
>  create mode 100644 docs/misc/arm/device-tree/passthrough.txt
> 
> diff --git a/docs/misc/arm/device-tree/passthrough.txt 
> b/docs/misc/arm/device-tree/passthrough.txt
> new file mode 100644
> index 0000000..04645b3
> --- /dev/null
> +++ b/docs/misc/arm/device-tree/passthrough.txt
> @@ -0,0 +1,7 @@
> +Device passthrough
> +===================
> +
> +Any device that will be passthrough to a guest should have a property
> +"xen,passthrough" in their device tree node.
> +
> +The device won't be exposed to DOM0 and therefore no driver will be loaded.
> diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
> index 1993929..1a01793 100644
> --- a/xen/arch/arm/device.c
> +++ b/xen/arch/arm/device.c
> @@ -30,7 +30,7 @@ int __init device_init(struct dt_device_node *dev, enum 
> device_match type,
>  
>      ASSERT(dev != NULL);
>  
> -    if ( !dt_device_is_available(dev) )
> +    if ( !dt_device_is_available(dev) || dt_device_for_passthrough(dev) )
>          return  -ENODEV;
>  
>      for ( desc = _sdevice; desc != _edevice; desc++ )
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index f68755f..b48b5d0 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -402,7 +402,7 @@ static int write_properties(struct domain *d, struct 
> kernel_info *kinfo,
>                              const struct dt_device_node *node)
>  {
>      const char *bootargs = NULL;
> -    const struct dt_property *prop;
> +    const struct dt_property *prop, *status = NULL;
>      int res = 0;
>      int had_dom0_bootargs = 0;
>  
> @@ -457,6 +457,17 @@ static int write_properties(struct domain *d, struct 
> kernel_info *kinfo,
>              }
>          }
>  
> +        /* Don't expose the property "xen,passthrough" to the guest */
> +        if ( dt_property_name_is_equal(prop, "xen,passthrough") )
> +            continue;
> +
> +        /* Remember and skip the status property as Xen may modify it later 
> */
> +        if ( dt_property_name_is_equal(prop, "status") )
> +        {
> +            status = prop;
> +            continue;
> +        }
> +
>          res = fdt_property(kinfo->fdt, prop->name, prop_data, prop_len);
>  
>          xfree(new_data);
> @@ -465,6 +476,19 @@ static int write_properties(struct domain *d, struct 
> kernel_info *kinfo,
>              return res;
>      }
>  
> +    /*
> +     * Override the property "status" to disable the device when it's
> +     * marked for passthrough.
> +     */
> +    if ( dt_device_for_passthrough(node) )
> +        res = fdt_property_string(kinfo->fdt, "status", "disabled");
> +    else if ( status )
> +        res = fdt_property(kinfo->fdt, "status", status->value,
> +                           status->length);

Why is the "else" needed? Wouldn't the status property for
non-passtrough devices already be covered by the
dt_for_each_property_node loop above?


> +    if ( res )
> +        return res;
> +
>      if ( dt_node_path_is_equal(node, "/chosen") )
>      {
>          const struct bootmodule *mod = kinfo->initrd_bootmodule;
> @@ -919,8 +943,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 dt_device_node *dev)
> +/* For a given device node:
> + *  - Give permission to the guest to manage IRQ and MMIO range
> + *  - Retrieve the IRQ configuration (i.e edge/level) from device tree
> + * When the device is not marked for guest passthrough:
> + *  - 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 dt_device_node *dev)
>  {
>      unsigned int nirq;
>      unsigned int naddr;
> @@ -929,13 +959,15 @@ static int map_device(struct domain *d, struct 
> dt_device_node *dev)
>      unsigned int irq;
>      struct dt_raw_irq rirq;
>      u64 addr, size;
> +    bool_t need_mapping = !dt_device_for_passthrough(dev);
>
>      nirq = dt_number_of_irq(dev);
>      naddr = dt_number_of_address(dev);
>  
> -    DPRINT("%s nirq = %d naddr = %u\n", dt_node_full_name(dev), nirq, naddr);
> +    DPRINT("%s passthrough = %d nirq = %d naddr = %u\n", 
> dt_node_full_name(dev),
> +           need_mapping, nirq, naddr);
>  
> -    if ( dt_device_is_protected(dev) )
> +    if ( dt_device_is_protected(dev) && need_mapping )
>      {
>          DPRINT("%s setup iommu\n", dt_node_full_name(dev));
>          res = iommu_assign_dt_device(d, dev);
> @@ -947,7 +979,7 @@ static int map_device(struct domain *d, struct 
> dt_device_node *dev)
>          }
>      }
>  
> -    /* Map IRQs */
> +    /* Give permission and  map IRQs */
>      for ( i = 0; i < nirq; i++ )
>      {
>          res = dt_device_get_raw_irq(dev, i, &rirq);
> @@ -980,22 +1012,34 @@ static int map_device(struct domain *d, struct 
> dt_device_node *dev)
>          irq = res;
>  
>          DPRINT("irq %u = %u\n", i, irq);
> -        /*
> -         * Checking the return of vgic_reserve_virq is not
> -         * necessary. It should not fail except when we try to map
> -         * twice the IRQ. This can happen if the IRQ is shared
> -         */
> -        vgic_reserve_virq(d, 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 ( need_mapping )
> +        {
> +            /*
> +             * Checking the return of vgic_reserve_virq is not
> +             * necessary. It should not fail except when we try to map
> +             * twice the IRQ. This can happen if the IRQ is shared
> +             */
> +            vgic_reserve_virq(d, irq);
> +            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 */
>      for ( i = 0; i < naddr; i++ )
>      {
>          res = dt_device_get_address(dev, i, &addr, &size);
> @@ -1019,17 +1063,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 ( need_mapping )
>          {
> -            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;
> +            }
>          }
>      }
>  
> @@ -1104,7 +1152,7 @@ static int handle_node(struct domain *d, struct 
> kernel_info *kinfo,
>          return 0;
>      }
>  
> -    res = map_device(d, node);
> +    res = handle_device(d, node);
>      if ( res)
>          return res;
>  
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index bb9d7ce..ce10574 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -1103,6 +1103,12 @@ bool_t dt_device_is_available(const struct 
> dt_device_node *device)
>      return 0;
>  }
>  
> +bool_t dt_device_for_passthrough(const struct dt_device_node *device)
> +{
> +    return (dt_find_property(device, "xen,passthrough", NULL) != NULL);
> +
> +}
> +
>  static int __dt_parse_phandle_with_args(const struct dt_device_node *np,
>                                          const char *list_name,
>                                          const char *cells_name,
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 890d356..caaf65f 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -555,6 +555,17 @@ int dt_n_addr_cells(const struct dt_device_node *np);
>  bool_t dt_device_is_available(const struct dt_device_node *device);
>  
>  /**
> + * dt_device_for_passthrough - Check if a device will be used for
> + * passthrough later
> + *
> + * @device: Node to check
> + *
> + * Return true if the property "xen,passthrough" is present in the node,
> + * false otherwise.
> + */
> +bool_t dt_device_for_passthrough(const struct dt_device_node *device);
> +
> +/**
>   * dt_match_node - Tell if a device_node has a matching of dt_device_match
>   * @matches: array of dt_device_match structures to search in
>   * @node: the dt_device_node structure to match against
> -- 
> 2.1.4
> 

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