Re: [Xen-devel] [PATCH V3 7/8] iommu/arm: Introduce iommu_add_dt_device API

[Oleksandr <olekstysh@gmail.com>]

On 09.09.19 18:04, Julien Grall wrote:
> Hi Oleksandr,

Hi Julien


>
>
> On 8/20/19 7:09 PM, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> This patch adds new iommu_add_dt_device API for adding DT device
>> to the IOMMU using generic IOMMU DT bindings [1] and previously
>> added "iommu_fwspec" support and "add_device/of_xlate" callbacks.
>>
>> New function does the following:
>> - Parse the DT bindings according to the specification
>> - Provide DT IOMMU specifier which describes the IOMMU master
>>    interfaces of that device (device IDs, etc) to the driver
>> - Add master device to the IOMMU if latter is present and available
>>
>> The additional benefit here is to avoid to go through the whole DT
>> multiple times in IOMMU driver trying to locate master devices which
>> belong to each IOMMU device being probed.
>
> So the commit title/message describes the new function 
> iommu_add_dt_device, but not the main important thing i.e. "Why is it 
> called when building dom0".
>
> While I agree the new function is the big part of the function what 
> matter is we need to register device using the generic IOMMU bindings 
> before assigning the device to a domain. The split is to keep separate 
> "add" and "assign". The later can be called from dom0.

Good point. I will update description.


>
>
>>
>> [1] 
>> https://www.kernel.org/doc/Documentation/devicetree/bindings/iommu/iommu.txt
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> CC: Julien Grall <julien.grall@arm.com>
>>
>> ---
>> Changes V2 -> V3:
>>      - clarified patch description
>>      - clarified comments in code
>>      - modified to provide DT IOMMU specifier to the driver
>>        using "of_xlate" callback
>>      - documented function usage
>>      - modified to return an error if ops is not present/implemented,
>>      - added ability to return a possitive value to indicate
>>        that device doesn't need to be protected
>>      - removed check for the "iommu" property presence
>>        in the common code
>>      - included <asm/iommu_fwspec.h> directly
>> ---
>>   xen/arch/arm/domain_build.c         | 11 ++++++++
>>   xen/drivers/passthrough/arm/iommu.c | 55 
>> +++++++++++++++++++++++++++++++++++++
>>   xen/include/asm-arm/iommu.h         | 11 ++++++++
>>   3 files changed, 77 insertions(+)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index e79d4e2..159ea6a 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -1240,6 +1240,7 @@ static int __init map_device_children(struct 
>> domain *d,
>>     /*
>>    * For a given device node:
>> + *  - Try to call iommu_add_dt_device to protect the device by an IOMMU
>>    *  - 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:
>> @@ -1257,6 +1258,16 @@ static int __init handle_device(struct domain 
>> *d, struct dt_device_node *dev,
>>       u64 addr, size;
>>       bool need_mapping = !dt_device_for_passthrough(dev);
>>   +    dt_dprintk("%s add to iommu\n", dt_node_full_name(dev));
>
> This message is slightly confusing. You are not adding the device, you 
> are trying to. So how about "Check if %s is behind an IOMMU and add it".

Sounds reasonable.


>
>
>> +
>> +    res = iommu_add_dt_device(dev);
>> +    if ( res < 0 )
>> +    {
>> +        printk(XENLOG_ERR "Failed to add %s to the IOMMU\n",
>> +               dt_node_full_name(dev));
>> +        return res;
>> +    }
>> +
>>       nirq = dt_number_of_irq(dev);
>>       naddr = dt_number_of_address(dev);
>>   diff --git a/xen/drivers/passthrough/arm/iommu.c 
>> b/xen/drivers/passthrough/arm/iommu.c
>> index 72a30e0..47e4bc6 100644
>> --- a/xen/drivers/passthrough/arm/iommu.c
>> +++ b/xen/drivers/passthrough/arm/iommu.c
>> @@ -20,6 +20,7 @@
>>   #include <xen/lib.h>
>>     #include <asm/device.h>
>> +#include <asm/iommu_fwspec.h>
>>     /*
>>    * Deferred probe list is used to keep track of devices for which 
>> driver
>> @@ -139,3 +140,57 @@ int arch_iommu_populate_page_table(struct domain 
>> *d)
>>   void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
>>   {
>>   }
>> +
>> +int __init iommu_add_dt_device(struct dt_device_node *np)
>> +{
>> +    const struct iommu_ops *ops = iommu_get_ops();
>> +    struct dt_phandle_args iommu_spec;
>> +    struct device *dev = dt_to_dev(np);
>> +    int rc = 1, index = 0;
>> +
>> +    if ( !iommu_enabled )
>> +        return 1;
>> +
>> +    if ( !ops || !ops->add_device || !ops->of_xlate )
>
> The SMMU does not implement of_xlate(). It is actually only mandatory 
> if you are using the generic bindings. So I would only check 
> ops->of_xlate if "iommus" exists.

Agree. Will do.


>
>
>> +        return -EINVAL;
>> +
>> +    if ( dev_iommu_fwspec_get(dev) )
>> +        return -EEXIST; > +
>> +    /* According Documentation/devicetree/bindings/iommu/iommu.txt 
>> from Linux */
>
> s/According/According to/

ok


>
>
>> +    while ( !dt_parse_phandle_with_args(np, "iommus", "#iommu-cells",
>> +                                        index, &iommu_spec) )
>> +    {
>> +        if ( !dt_device_is_available(iommu_spec.np) )
>> +            break;
>> +
>> +        rc = iommu_fwspec_init(dev, &iommu_spec.np->dev);
>> +        if ( rc )
>> +            break;
>> +
>> +        /*
>> +         * Provide DT IOMMU specifier which describes the IOMMU master
>> +         * interfaces of that device (device IDs, etc) to the driver.
>> +         * The driver's responsibility is to decide how to interpret 
>> them.
>
> NIT: "The driver is responsible to decide...".

ok


>
>> +         * It should also initialize/verify that device.
>
> What do you mean? of_xlate should mostly translate the specifier to 
> fwspec.

Yes. Saying "initialize/verify that device" I meant to verify passed DT 
IOMMU specifier and initialize driver's private data for this device 
(iommu_priv). But, this is obvious.

I will remove confusing word "initialize" or even the whole sentence.


>
>
>> +         */
>> +        rc = ops->of_xlate(dev, &iommu_spec);
>> +        if ( rc )
>> +            break;
>> +
>> +        index++;
>> +    }
>> +
>> +    /*
>> +     * Add master device to the IOMMU if latter is present and 
>> available.
>> +     * The driver's responsibility is to check whether that device
>> +     * was initialized/verified before and mark that device as 
>> protected.
>
> I don't understand the last sentence. For me, "device" refers to 
> what's pointed by "dev". But the IOMMU driver is not responsible for 
> initializing the device.

Yes. The same as for comment above. I will remove confusing word 
"initialize".


-- 
Regards,

Oleksandr Tyshchenko


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel