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

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

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@linaro.org>
>>
>> ---
>>     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.


Regards,

-- 
Julien Grall