Re: [Xen-devel] [PATCH V2 5/6] iommu/arm: Introduce iommu_add_dt_device API

[Oleksandr <olekstysh@gmail.com>]

On 13.08.19 16:49, Julien Grall wrote:
> Hi Oleksandr,

Hi Julien


>
> On 8/2/19 5:39 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 binding [1] and previously
>> added "iommu_fwspec" support.
>>
>> New function parses the DT binding, prepares "dev->iommu_fwspec"
>> with correct information and calls the IOMMU driver using "add_device"
>> callback to register new DT device.
>> The IOMMU driver's responsibility is to check whether 
>> "dev->iommu_fwspec"
>> is initialized and mark that device as protected.
>>
>> 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.
>>
>> The upcoming IPMMU driver will have "add_device" callback implemented.
>>
>> I hope, this patch won't break SMMU driver's functionality,
>> which doesn't have this callback implemented.
>
> The last two sentence does not really belong to the commit message. So 
> I think they should go after ---.
>
> Anyway, the only concern for the SMMU is to not break the old 
> bindings. New bindings are not supported, so it does not matter 
> whether they are broken or not. Once this series is merged, we can 
> have a look how new bindings for the SMMU can be supported.

Sounds reasonable.


>
>>
>> [1] 
>> https://www.kernel.org/doc/Documentation/devicetree/bindings/iommu/iommu.txt
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> ---
>>   xen/arch/arm/domain_build.c         | 12 ++++++++++
>>   xen/drivers/passthrough/arm/iommu.c | 45 
>> +++++++++++++++++++++++++++++++++++++
>>   xen/include/asm-arm/iommu.h         |  3 +++
>>   3 files changed, 60 insertions(+)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index d983677..d67f7d4 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -1241,6 +1241,18 @@ static int __init handle_device(struct domain 
>> *d, struct dt_device_node *dev,
>>       u64 addr, size;
>>       bool need_mapping = !dt_device_for_passthrough(dev);
>>   +    if ( dt_parse_phandle(dev, "iommus", 0) )
>
> I don't particularly like this check. dt_parse_phandle is non-trivial 
> to execute.
>
> TBH, what we should do is trying to call iommu_add_dt_device if IOMMU 
> is enabled. We can then return a recognizable value to tell the device 
> is not protected.

Well. Don't really mind.


>
>> +    {
>> +        dt_dprintk("%s add to iommu\n", dt_node_full_name(dev));
>> +        res = iommu_add_dt_device(dev);
>> +        if ( res )
>> +        {
>> +            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 3195919..19516af 100644
>> --- a/xen/drivers/passthrough/arm/iommu.c
>> +++ b/xen/drivers/passthrough/arm/iommu.c
>> @@ -113,3 +113,48 @@ 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 || !ops || !ops->add_device )
>> +        return 0;
>
> While I agree that !iommu_enabled should return 0, for the two others 
> I am not entirely sure this is the right thing to do.
>
> !ops is definitely an error because if you have the IOMMU enabled then 
> you should have ops installed.

Agree.


>
>
> !ops->add_device means that you will not be able to add any device 
> using the new bindings. IOW, the device will be unusable later one as 
> most likely the IOMMU was configured to deny any transaction. 
> Therefore, this should return an error.

The initial reason *was* to not break SMMU which hasn't had add_device 
callback implemented yet. But, I got your point regarding SMMU above 
(the only concern for the SMMU is to not break the old bindings), so 
agree here.


>
>> +
>> +    if ( dev_iommu_fwspec_get(dev) )
>> +        return -EEXIST;
>> +
>> +    /* According to the 
>> Documentation/devicetree/bindings/iommu/iommu.txt */
>
> This file does not exist in Xen, so Linux should at least be mentioned 
> in the comment.

Will do.


>
>> +    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;
>> +
>> +        rc = iommu_fwspec_add_ids(dev, iommu_spec.args, 1);
>
> Here you assume that there will at least always be one cells and the 
> first cell is the IDs. For a first, #iommu-cells may be 0 (and 
> therefore no cells) when the master IOMMU device cannot be configured.
>
> Furthermore, the content of the #iommu-cells depends on the driver. 
> This is why Linux provides a callback of_xlate to let the driver 
> decide how to interpret it.
>
> For instance, the SMMU can support either 1 or 2 cells. It also may 
> need to look-up for other properties in the node (e.g stream-match-mask).
>
> So I think we also want to provide the of_xlate in Xen.


Hmm, I was thinking how to end up with only one callback re-used 
(add_device), really didn't want to add a new one (of_xlate). But, I 
didn't take into the account that this stuff is a really 
driver-depended. So, likely yes, we need to provide of_xlate callback.


May I ask some questions to clarify:

1. Do you want me to introduce of_xlate callback in a separate patch 
(under CONFIG_ARM?)?

2. Can we avoid introducing new API for that callback? 
iommu_add_dt_device will be able to call it directly.


-- 
Regards,

Oleksandr Tyshchenko


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