Re: [Xen-devel] [PATCH for 4.6 13/13] xen/iommu: smmu: Add Xen specific code to be able to use the driver

["Jaggi, Manish" <Manish.Jaggi@xxxxxxxxxxxxxxxxxx>]

_______________________________________
From: Julien Grall <julien.grall@xxxxxxxxxx>
Sent: Wednesday, February 18, 2015 5:24 PM
To: Jaggi, Manish; xen-devel@xxxxxxxxxxxxxxxxxxxx >> xen-devel
Cc: ian.campbell@xxxxxxxxxx; tim@xxxxxxx; stefano.stabellini@xxxxxxxxxx; Jaggi, 
Manish
Subject: Re: [PATCH for 4.6 13/13] xen/iommu: smmu: Add Xen specific code to be 
able to use the driver

BTW, I have sent few versions of this series since then. Please comment
on the latest series as the code may have change.

[manish] Somehow I could not find your recent revision, but I am looking at 
your den-unstable tree assuming it has your latest version.

Nonetheless, you are comment is still valid for the v3 :).

[manish] There are general comments on the data structures
(a) I don't see a use case where for same domain (VM) there would be different 
context banks , so linked list may not be required. 
(b) Also iommu group may not be relevant for the same reason.
I am curious to find the use cases.


Regards,

On 18/02/2015 11:47, Julien Grall wrote:
> Hi Manish,
>
> On 18/02/2015 01:02, Manish wrote:
>>
>> On 17/12/14 1:38 am, Julien Grall wrote:
>>> The main goal is to modify as little the Linux code to be able to port
>>> easily new feature added in Linux repo for the driver.
>>>
>>> To achieve that we:
>>>      - Add helpers to Linux function not implemented on Xen
>>>      - Add callbacks used by Xen to do our own stuff and call Linux ones
>>>      - Only modify when required the code which comes from Linux. If
>>> so a
>>>      comment has been added with /* Xen: ... */ explaining why it's
>>>      necessary.
>>>
>>> The support for PCI has been commented because it's not yet supported by
>>> Xen ARM and therefore won't compile.
>>>
>>> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
>>> ---
>>>   xen/drivers/passthrough/arm/Makefile |   1 +
>>>   xen/drivers/passthrough/arm/smmu.c   | 668
>>> +++++++++++++++++++++++++++++++----
>>>   2 files changed, 602 insertions(+), 67 deletions(-)
>>>
>>> diff --git a/xen/drivers/passthrough/arm/Makefile
>>> b/xen/drivers/passthrough/arm/Makefile
>>> index 0484b79..f4cd26e 100644
>>> --- a/xen/drivers/passthrough/arm/Makefile
>>> +++ b/xen/drivers/passthrough/arm/Makefile
>>> @@ -1 +1,2 @@
>>>   obj-y += iommu.o
>>> +obj-y += smmu.o
>>> diff --git a/xen/drivers/passthrough/arm/smmu.c
>>> b/xen/drivers/passthrough/arm/smmu.c
>>> index 8a6514f..3cf1773 100644
>>> --- a/xen/drivers/passthrough/arm/smmu.c
>>> +++ b/xen/drivers/passthrough/arm/smmu.c
>>> @@ -18,6 +18,13 @@
>>>    *
>>>    * Author: Will Deacon <will.deacon@xxxxxxx>
>>>    *
>>> + * Based on Linux drivers/iommu/arm-smmu.c
>>> + *    => commit e6b5be2be4e30037eb551e0ed09dd97bd00d85d3
>>> + *
>>> + * Xen modification:
>>> + * Julien Grall <julien.grall@xxxxxxxxxx>
>>> + * Copyright (C) 2014 Linaro Limited.
>>> + *
>>>    * This driver currently supports:
>>>    *    - SMMUv1 and v2 implementations
>>>    *    - Stream-matching and stream-indexing
>>> @@ -28,26 +35,154 @@
>>>    *    - Context fault reporting
>>>    */
>>
>> <<<snip >>>
>>
>>> +/* Xen: Dummy iommu_domain */
>>> +struct iommu_domain
>>> +{
>>> +    struct arm_smmu_domain        *priv;
>>> +
>>> +    /* Used to link domain contexts for a same domain */
>>> +    struct list_head        list;
>>> +};
>>> +
>>> +/* Xen: Describes informations required for a Xen domain */
>>> +struct arm_smmu_xen_domain {
>>> +    spinlock_t            lock;
>>> +    /* List of context (i.e iommu_domain) associated to this domain */
>>> +    struct list_head        contexts;
>>> +};
>>> +
>>> +/* Xen: Information about each device stored in dev->archdata.iommu */
>>> +struct arm_smmu_xen_device {
>>> +    struct iommu_domain *domain;
>>> +    struct iommu_group *group;
>>> +};
>>> +
>>> +#define dev_archdata(dev) ((struct arm_smmu_xen_device
>>> *)dev->archdata.iommu)
>>> +#define dev_iommu_domain(dev) (dev_archdata(dev)->domain)
>>> +#define dev_iommu_group(dev) (dev_archdata(dev)->group)
>>> +
>>> +/* Xen: Dummy iommu_group */
>>> +struct iommu_group
>>> +{
>>> +    struct arm_smmu_master_cfg *cfg;
>>> +
>>> +    atomic_t ref;
>>> +};
>>> +
>> The naming needs to be revisited in this patch. Original driver from
>> Will has arm_smmu_domain. This patch adds  iommu_domain,
>> arm_smmu_xen_domain, iommu_group.
>
> I can't change the naming of the structure. iommu_domain and iommu_group
> are from Linux. As we don't have it on Xen, I have to add dummy
> structure for it.
>
>> Could you please add some description about the relation and hierarchy
>> of these data structures.
>
> Good point, I will try to add more comment and explain why we have to do
> it.
>
> Regards,
>

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