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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 28 Jan 2015, Julien Grall wrote:
> On 28/01/15 16:36, Stefano Stabellini wrote:
> > 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?
> 
> 
> Because the property "status" is skipped earlier in any case.

I see that now.

Acked-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>

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