[Xen-devel,RFC,06/11] fwnode xen spacific changes

Message ID 20180102092809.1841-7-manish.jaggi@linaro.org
State New
Headers show
Series
  • acpi: arm: IORT Support for Xen
Related show

Commit Message

Manish Jaggi Jan. 2, 2018, 9:28 a.m.
From: Manish Jaggi <manish.jaggi@linaro.org>

Merge few more changes from linux kernel code (v4.14) into iommu.c
Modify code specifc to xen.

Signed-off-by: Manish Jaggi <manish.jaggi@linaro.org>
---
 xen/drivers/passthrough/iommu.c | 75 +++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/device.h    | 11 ++++--
 xen/include/xen/iommu.h         | 22 ++++++++++++
 3 files changed, 106 insertions(+), 2 deletions(-)

Comments

Julien Grall Jan. 18, 2018, 6:51 p.m. | #1
Hi Manish,

Please use scripts/get_maintainers.pl to CC relevant maintainers. I have 
done it for you this time.


Title: s/spacific/specific/

On 02/01/18 09:28, manish.jaggi@linaro.org wrote:
> From: Manish Jaggi <manish.jaggi@linaro.org>
> 
> Merge few more changes from linux kernel code (v4.14) into iommu.c
> Modify code specifc to xen.

I appreciate you pick-up the series from Sameer. I would also have 
appreciated if you have addressed my remarks from there.

Sameer explain why he imported fwnode. This has been dropped here. Also,
I think you probably want a bit more context in the commit message about 
implement fwnode.h in common code.

Within this series, fwnode seems to only be used by Arm. So what would 
be the advantage to get that in xen/? Is it going to be used by x86 or 
taken advantage in common code?

> 
> Signed-off-by: Manish Jaggi <manish.jaggi@linaro.org>
> ---
>   xen/drivers/passthrough/iommu.c | 75 +++++++++++++++++++++++++++++++++++++++++
>   xen/include/asm-arm/device.h    | 11 ++++--
>   xen/include/xen/iommu.h         | 22 ++++++++++++
>   3 files changed, 106 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index 1aecf7cf34..408f44106d 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -13,6 +13,7 @@
>    */
>   
>   #include <xen/sched.h>
> +#include <xen/fwnode.h>
>   #include <xen/iommu.h>
>   #include <xen/paging.h>
>   #include <xen/guest_access.h>
> @@ -507,6 +508,80 @@ static void iommu_dump_p2m_table(unsigned char key)
>       }
>   }
>   
> +/**
> + * fwnode_handle_put - Drop reference to a device node
> + * @fwnode: Pointer to the device node to drop the reference to.
> + *
> + * This has to be used when terminating device_for_each_child_node() iteration
> + * with break or return to prevent stale device node references from being left
> + * behind.
> + */
> +void fwnode_handle_put(struct fwnode_handle *fwnode)
> +{
> +        fwnode_call_void_op(fwnode, put);

This file is following Xen coding style. And therefore you should use 
Xen coding.

> +}
> +
> +const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
> +{
> +       return iommu_get_ops();
> +}
> +
> +int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
> +                     const struct iommu_ops *ops)
> +{
> +       struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> +
> +       if (fwspec)
> +               return ops == fwspec->ops ? 0 : -EINVAL;
> +
> +       fwspec = kzalloc(sizeof(*fwspec), GFP_KERNEL);

You define kzalloc in a later patch and hence break the build. *All* the 
patches should build one by one to help bisectability.

But given the side of the code and the fact you are going to fix the 
coding style. It might be easier to use Xen name here.

> +       if (!fwspec)
> +               return -ENOMEM;
> +#if 0
> +       of_node_get(to_of_node(iommu_fwnode));
> +#endif
> +       fwspec->iommu_fwnode = iommu_fwnode; > +       fwspec->ops = ops;
> +       dev->iommu_fwspec = fwspec;
> +       return 0;
> +}
> +
> +void iommu_fwspec_free(struct device *dev)
> +{
> +       struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> +
> +       if (fwspec) {
> +               fwnode_handle_put(fwspec->iommu_fwnode);
> +               kfree(fwspec);
> +               dev->iommu_fwspec = NULL;
> +       }
> +}
> +
> +int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids)
> +{
> +  struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> +        size_t size;
> +        int i;
> +
> +        if (!fwspec)
> +                return -EINVAL;
> +
> +        size = offsetof(struct iommu_fwspec, ids[fwspec->num_ids + num_ids]);
> +        if (size > sizeof(*fwspec)) {
> +                //TBD: fwspec = krealloc(dev->iommu_fwspec, size, GFP_KERNEL);

Hmmm?

> +                if (!fwspec)
> +                        return -ENOMEM;
> +
> +                dev->iommu_fwspec = fwspec;
> +        }
> +
> +        for (i = 0; i < num_ids; i++)
> +                fwspec->ids[fwspec->num_ids + i] = ids[i];
> +
> +        fwspec->num_ids += num_ids;
> +        return 0;
> +
> +}
>   /*
>    * Local variables:
>    * mode: C
> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
> index 6734ae8efd..f78482ca0c 100644
> --- a/xen/include/asm-arm/device.h
> +++ b/xen/include/asm-arm/device.h
> @@ -6,6 +6,8 @@
>   enum device_type
>   {
>       DEV_DT,
> +    DEV_ACPI,

You don't use DEV_ACPI in this patch. So why is there?

> +    DEV_PCI,
>   };
>   
>   struct dev_archdata {
> @@ -18,8 +20,13 @@ struct device
>       enum device_type type;
>   #ifdef CONFIG_HAS_DEVICE_TREE
>       struct dt_device_node *of_node; /* Used by drivers imported from Linux */

As said on Sameer's patches, I was expecting a todo in the code after 
the discussion about leave of_node here.

> +#endif
> +#ifdef CONFIG_ACPI
> +    void *acpi_node;

You don't use acpi_node in this patch. So why is it there?

>   #endif
>       struct dev_archdata archdata;
> +    struct fwnode_handle *fwnode; /* firmware device node */

Ditto.

> +    struct iommu_fwspec *iommu_fwspec;
>   };
>   
>   typedef struct device device_t;
> @@ -27,8 +34,8 @@ typedef struct device device_t;
>   #include <xen/device_tree.h>
>   
>   /* TODO: Correctly implement dev_is_pci when PCI is supported on ARM */
> -#define dev_is_pci(dev) ((void)(dev), 0)
> -#define dev_is_dt(dev)  ((dev->type == DEV_DT)
> +#define dev_is_pci(dev) (dev->type == DEV_PCI)
> +#define dev_is_dt(dev)  (dev->type == DEV_DT)

Those two changes does not belong to this patch. It is likely 2 separate 
patches:
     1# fixing dev_is_dt because of the missing parenthese
     2# implementing dev_is_dt

>   
>   enum device_class
>   {
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 33c8b221dc..56b169bae9 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -208,4 +208,26 @@ DECLARE_PER_CPU(bool_t, iommu_dont_flush_iotlb);
>   extern struct spinlock iommu_pt_cleanup_lock;
>   extern struct page_list_head iommu_pt_cleanup_list;
>   
> +/**
> + * struct iommu_fwspec - per-device IOMMU instance data
> + * @ops: ops for this device's IOMMU
> + * @iommu_fwnode: firmware handle for this device's IOMMU
> + * @iommu_priv: IOMMU driver private data for this device
> + * @num_ids: number of associated device IDs
> + * @ids: IDs which this device may present to the IOMMU
> + */
> +struct iommu_fwspec {
> +        const struct iommu_ops  *ops;
> +        struct fwnode_handle    *iommu_fwnode;
> +        void                    *iommu_priv;
> +        unsigned int            num_ids;
> +        u32                     ids[1];
> +};
> +
> +int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
> +                      const struct iommu_ops *ops);
> +void iommu_fwspec_free(struct device *dev);
> +int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
> +const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode);
> +
>   #endif /* _IOMMU_H_ */
> 

Cheers,
Manish Jaggi March 6, 2018, 10:27 a.m. | #2
On 01/19/2018 12:21 AM, Julien Grall wrote:
> Hi Manish,
>
> Please use scripts/get_maintainers.pl to CC relevant maintainers. I 
> have done it for you this time.
>
>
> Title: s/spacific/specific/
>
> On 02/01/18 09:28, manish.jaggi@linaro.org wrote:
>> From: Manish Jaggi <manish.jaggi@linaro.org>
>>
>> Merge few more changes from linux kernel code (v4.14) into iommu.c
>> Modify code specifc to xen.
>
> I appreciate you pick-up the series from Sameer. I would also have 
> appreciated if you have addressed my remarks from there.
>
> Sameer explain why he imported fwnode. This has been dropped here. Also,
> I think you probably want a bit more context in the commit message 
> about implement fwnode.h in common code.
>
> Within this series, fwnode seems to only be used by Arm. So what would 
> be the advantage to get that in xen/? Is it going to be used by x86 or 
> taken advantage in common code?
Do you want patch code (iommu.h iommu.c) to be moved to xen/arch/arm
In patch 4, i have created a new file xen/include/xen/fwnode.h
Should I move it to asm-arm ?
>
>>
>> Signed-off-by: Manish Jaggi <manish.jaggi@linaro.org>
>> ---
>>   xen/drivers/passthrough/iommu.c | 75 
>> +++++++++++++++++++++++++++++++++++++++++
>>   xen/include/asm-arm/device.h    | 11 ++++--
>>   xen/include/xen/iommu.h         | 22 ++++++++++++
>>   3 files changed, 106 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/drivers/passthrough/iommu.c 
>> b/xen/drivers/passthrough/iommu.c
>> index 1aecf7cf34..408f44106d 100644
>> --- a/xen/drivers/passthrough/iommu.c
>> +++ b/xen/drivers/passthrough/iommu.c
>> @@ -13,6 +13,7 @@
>>    */
>>     #include <xen/sched.h>
>> +#include <xen/fwnode.h>
>>   #include <xen/iommu.h>
>>   #include <xen/paging.h>
>>   #include <xen/guest_access.h>
>> @@ -507,6 +508,80 @@ static void iommu_dump_p2m_table(unsigned char key)
>>       }
>>   }
>>   +/**
>> + * fwnode_handle_put - Drop reference to a device node
>> + * @fwnode: Pointer to the device node to drop the reference to.
>> + *
>> + * This has to be used when terminating device_for_each_child_node() 
>> iteration
>> + * with break or return to prevent stale device node references from 
>> being left
>> + * behind.
>> + */
>> +void fwnode_handle_put(struct fwnode_handle *fwnode)
>> +{
>> +        fwnode_call_void_op(fwnode, put);
>
> This file is following Xen coding style. And therefore you should use 
> Xen coding.
>
>> +}
>> +
>> +const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle 
>> *fwnode)
>> +{
>> +       return iommu_get_ops();
>> +}
>> +
>> +int iommu_fwspec_init(struct device *dev, struct fwnode_handle 
>> *iommu_fwnode,
>> +                     const struct iommu_ops *ops)
>> +{
>> +       struct iommu_fwspec *fwspec = dev->iommu_fwspec;
>> +
>> +       if (fwspec)
>> +               return ops == fwspec->ops ? 0 : -EINVAL;
>> +
>> +       fwspec = kzalloc(sizeof(*fwspec), GFP_KERNEL);
>
> You define kzalloc in a later patch and hence break the build. *All* 
> the patches should build one by one to help bisectability.
>
> But given the side of the code and the fact you are going to fix the 
> coding style. It might be easier to use Xen name here.
>
>> +       if (!fwspec)
>> +               return -ENOMEM;
>> +#if 0
>> +       of_node_get(to_of_node(iommu_fwnode));
>> +#endif
>> +       fwspec->iommu_fwnode = iommu_fwnode; > + fwspec->ops = ops;
>> +       dev->iommu_fwspec = fwspec;
>> +       return 0;
>> +}
>> +
>> +void iommu_fwspec_free(struct device *dev)
>> +{
>> +       struct iommu_fwspec *fwspec = dev->iommu_fwspec;
>> +
>> +       if (fwspec) {
>> +               fwnode_handle_put(fwspec->iommu_fwnode);
>> +               kfree(fwspec);
>> +               dev->iommu_fwspec = NULL;
>> +       }
>> +}
>> +
>> +int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids)
>> +{
>> +  struct iommu_fwspec *fwspec = dev->iommu_fwspec;
>> +        size_t size;
>> +        int i;
>> +
>> +        if (!fwspec)
>> +                return -EINVAL;
>> +
>> +        size = offsetof(struct iommu_fwspec, ids[fwspec->num_ids + 
>> num_ids]);
>> +        if (size > sizeof(*fwspec)) {
>> +                //TBD: fwspec = krealloc(dev->iommu_fwspec, size, 
>> GFP_KERNEL);
>
> Hmmm?
>
>> +                if (!fwspec)
>> +                        return -ENOMEM;
>> +
>> +                dev->iommu_fwspec = fwspec;
>> +        }
>> +
>> +        for (i = 0; i < num_ids; i++)
>> +                fwspec->ids[fwspec->num_ids + i] = ids[i];
>> +
>> +        fwspec->num_ids += num_ids;
>> +        return 0;
>> +
>> +}
>>   /*
>>    * Local variables:
>>    * mode: C
>> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
>> index 6734ae8efd..f78482ca0c 100644
>> --- a/xen/include/asm-arm/device.h
>> +++ b/xen/include/asm-arm/device.h
>> @@ -6,6 +6,8 @@
>>   enum device_type
>>   {
>>       DEV_DT,
>> +    DEV_ACPI,
>
> You don't use DEV_ACPI in this patch. So why is there?
>
>> +    DEV_PCI,
>>   };
>>     struct dev_archdata {
>> @@ -18,8 +20,13 @@ struct device
>>       enum device_type type;
>>   #ifdef CONFIG_HAS_DEVICE_TREE
>>       struct dt_device_node *of_node; /* Used by drivers imported 
>> from Linux */
>
> As said on Sameer's patches, I was expecting a todo in the code after 
> the discussion about leave of_node here.
I might have missed your comment on sameers patch, could you please restate
>
>> +#endif
>> +#ifdef CONFIG_ACPI
>> +    void *acpi_node;
>
> You don't use acpi_node in this patch. So why is it there?
>
>>   #endif
>>       struct dev_archdata archdata;
>> +    struct fwnode_handle *fwnode; /* firmware device node */
>
> Ditto.
>
>> +    struct iommu_fwspec *iommu_fwspec;
>>   };
>>     typedef struct device device_t;
>> @@ -27,8 +34,8 @@ typedef struct device device_t;
>>   #include <xen/device_tree.h>
>>     /* TODO: Correctly implement dev_is_pci when PCI is supported on 
>> ARM */
>> -#define dev_is_pci(dev) ((void)(dev), 0)
>> -#define dev_is_dt(dev)  ((dev->type == DEV_DT)
>> +#define dev_is_pci(dev) (dev->type == DEV_PCI)
>> +#define dev_is_dt(dev)  (dev->type == DEV_DT)
>
> Those two changes does not belong to this patch. It is likely 2 
> separate patches:
>     1# fixing dev_is_dt because of the missing parenthese
>     2# implementing dev_is_dt
>
>>     enum device_class
>>   {
>> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
>> index 33c8b221dc..56b169bae9 100644
>> --- a/xen/include/xen/iommu.h
>> +++ b/xen/include/xen/iommu.h
>> @@ -208,4 +208,26 @@ DECLARE_PER_CPU(bool_t, iommu_dont_flush_iotlb);
>>   extern struct spinlock iommu_pt_cleanup_lock;
>>   extern struct page_list_head iommu_pt_cleanup_list;
>>   +/**
>> + * struct iommu_fwspec - per-device IOMMU instance data
>> + * @ops: ops for this device's IOMMU
>> + * @iommu_fwnode: firmware handle for this device's IOMMU
>> + * @iommu_priv: IOMMU driver private data for this device
>> + * @num_ids: number of associated device IDs
>> + * @ids: IDs which this device may present to the IOMMU
>> + */
>> +struct iommu_fwspec {
>> +        const struct iommu_ops  *ops;
>> +        struct fwnode_handle    *iommu_fwnode;
>> +        void                    *iommu_priv;
>> +        unsigned int            num_ids;
>> +        u32                     ids[1];
>> +};
>> +
>> +int iommu_fwspec_init(struct device *dev, struct fwnode_handle 
>> *iommu_fwnode,
>> +                      const struct iommu_ops *ops);
>> +void iommu_fwspec_free(struct device *dev);
>> +int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
>> +const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle 
>> *fwnode);
>> +
>>   #endif /* _IOMMU_H_ */
>>
>
> Cheers,
>
Manish Jaggi March 6, 2018, 1:43 p.m. | #3
Hi Julien,


On 01/19/2018 12:21 AM, Julien Grall wrote:
>> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h

>> index 6734ae8efd..f78482ca0c 100644

>> --- a/xen/include/asm-arm/device.h

>> +++ b/xen/include/asm-arm/device.h

>> @@ -6,6 +6,8 @@

>>   enum device_type

>>   {

>>       DEV_DT,

>> +    DEV_ACPI,

>

> You don't use DEV_ACPI in this patch. So why is there?

>

>> +    DEV_PCI,

>>   };

>>     struct dev_archdata {

>> @@ -18,8 +20,13 @@ struct device

>>       enum device_type type;

>>   #ifdef CONFIG_HAS_DEVICE_TREE

>>       struct dt_device_node *of_node; /* Used by drivers imported 

>> from Linux */

>

> As said on Sameer's patches, I was expecting a todo in the code after 

> the discussion about leave of_node here. 

I think you are referring to https://patchwork.kernel.org/patch/9963109/
Could you please add what TODO you wish to add ?

I could not find  any discussion on of_node in the mail chain


-Regards
Manish
<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    <p>Hi Julien,<br>
    </p>
    <br>
    <div class="moz-cite-prefix">On 01/19/2018 12:21 AM, Julien Grall
      wrote:<br>
    </div>
    <blockquote type="cite"
      cite="mid:521998b8-1179-bc87-db7b-a4e3aee0c644@linaro.org">
      <blockquote type="cite" style="color: #000000;">diff --git
        a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
        <br>
        index 6734ae8efd..f78482ca0c 100644
        <br>
        --- a/xen/include/asm-arm/device.h
        <br>
        +++ b/xen/include/asm-arm/device.h
        <br>
        @@ -6,6 +6,8 @@
        <br>
          enum device_type
        <br>
          {
        <br>
              DEV_DT,
        <br>
        +    DEV_ACPI,
        <br>
      </blockquote>
      <br>
      You don't use DEV_ACPI in this patch. So why is there?
      <br>
      <br>
      <blockquote type="cite" style="color: #000000;">+    DEV_PCI,
        <br>
          };
        <br>
            struct dev_archdata {
        <br>
        @@ -18,8 +20,13 @@ struct device
        <br>
              enum device_type type;
        <br>
          #ifdef CONFIG_HAS_DEVICE_TREE
        <br>
              struct dt_device_node *of_node; /* Used by drivers
        imported from Linux */
        <br>
      </blockquote>
      <br>
      As said on Sameer's patches, I was expecting a todo in the code
      after the discussion about leave of_node here.
    </blockquote>
    I think you are referring to
    <a class="moz-txt-link-freetext" href="https://patchwork.kernel.org/patch/9963109/">https://patchwork.kernel.org/patch/9963109/</a><br>
    Could you please add what TODO you wish to add ?<br>
    <br>
    I could not find  any discussion on of_node in the mail chain<br>
    <br>
    <br>
    -Regards<br>
    Manish<br>
  </body>
</html>
Manish Jaggi March 6, 2018, 1:44 p.m. | #4
Hi Julien,


On 01/19/2018 12:21 AM, Julien Grall wrote:
>> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
>> index 6734ae8efd..f78482ca0c 100644
>> --- a/xen/include/asm-arm/device.h
>> +++ b/xen/include/asm-arm/device.h
>> @@ -6,6 +6,8 @@
>>   enum device_type
>>   {
>>       DEV_DT,
>> +    DEV_ACPI,
>
> You don't use DEV_ACPI in this patch. So why is there?
>
>> +    DEV_PCI,
>>   };
>>     struct dev_archdata {
>> @@ -18,8 +20,13 @@ struct device
>>       enum device_type type;
>>   #ifdef CONFIG_HAS_DEVICE_TREE
>>       struct dt_device_node *of_node; /* Used by drivers imported 
>> from Linux */
>
> As said on Sameer's patches, I was expecting a todo in the code after 
> the discussion about leave of_node here. 
I think you are referring to https://patchwork.kernel.org/patch/9963109/
Could you please add what TODO you wish to add ?

I could not find  any discussion on of_node in the mail chain


-Regards
Manish
Julien Grall March 6, 2018, 2:22 p.m. | #5
On 06/03/18 13:44, Manish Jaggi wrote:
> Hi Julien,
> 
> 
> On 01/19/2018 12:21 AM, Julien Grall wrote:
>>> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
>>> index 6734ae8efd..f78482ca0c 100644
>>> --- a/xen/include/asm-arm/device.h
>>> +++ b/xen/include/asm-arm/device.h
>>> @@ -6,6 +6,8 @@
>>>   enum device_type
>>>   {
>>>       DEV_DT,
>>> +    DEV_ACPI,
>>
>> You don't use DEV_ACPI in this patch. So why is there?
>>
>>> +    DEV_PCI,
>>>   };
>>>     struct dev_archdata {
>>> @@ -18,8 +20,13 @@ struct device
>>>       enum device_type type;
>>>   #ifdef CONFIG_HAS_DEVICE_TREE
>>>       struct dt_device_node *of_node; /* Used by drivers imported 
>>> from Linux */
>>
>> As said on Sameer's patches, I was expecting a todo in the code after 
>> the discussion about leave of_node here. 
> I think you are referring to https://patchwork.kernel.org/patch/9963109/
> Could you please add what TODO you wish to add ?
> 
> I could not find  any discussion on of_node in the mail chain
Usually when I say: "I was expecting ..." it means that was discussed on 
a previous version. In that case it is "[RFC 2/6] arm64: Add definitions 
for fwnode_handle".

Below the conversation:

Me: I am a bit surprised you don't rework struct dev. As of_node is now 
redundant with fwnode.

Sameer: I agree that this will eventually be removed. I have kept this 
in now just to maintain compatibility
(compilation and otherwise) with smmuv2 driver. I will add a comment to 
indicate this. So that it can
be easily identified and remove when we do a final cleanup. Can I prefix 
the comment with with XEN:TODO:?

Me: A TODO would be nice, but who is going to do the rework?

Cheers,
Julien Grall March 6, 2018, 2:29 p.m. | #6
On 06/03/18 10:27, Manish Jaggi wrote:
> 
> 
> On 01/19/2018 12:21 AM, Julien Grall wrote:
>> Hi Manish,
>>
>> Please use scripts/get_maintainers.pl to CC relevant maintainers. I 
>> have done it for you this time.
>>
>>
>> Title: s/spacific/specific/
>>
>> On 02/01/18 09:28, manish.jaggi@linaro.org wrote:
>>> From: Manish Jaggi <manish.jaggi@linaro.org>
>>>
>>> Merge few more changes from linux kernel code (v4.14) into iommu.c
>>> Modify code specifc to xen.
>>
>> I appreciate you pick-up the series from Sameer. I would also have 
>> appreciated if you have addressed my remarks from there.
>>
>> Sameer explain why he imported fwnode. This has been dropped here. Also,
>> I think you probably want a bit more context in the commit message 
>> about implement fwnode.h in common code.
>>
>> Within this series, fwnode seems to only be used by Arm. So what would 
>> be the advantage to get that in xen/? Is it going to be used by x86 or 
>> taken advantage in common code?
> Do you want patch code (iommu.h iommu.c) to be moved to xen/arch/arm
> In patch 4, i have created a new file xen/include/xen/fwnode.h
> Should I move it to asm-arm ?

Please read my e-mail and answer the question. If you answer by no, then 
move it to asm-arm. If you answer by yes, then write the rationale in 
the commit message.

It looks like to me we might only need that in Arm, but I wanted your 
input as the author of the patch. You likely have a reason to put this 
code here, am I right?

Cheers,

>>
>>>
>>> Signed-off-by: Manish Jaggi <manish.jaggi@linaro.org>
>>> ---
>>>   xen/drivers/passthrough/iommu.c | 75 
>>> +++++++++++++++++++++++++++++++++++++++++
>>>   xen/include/asm-arm/device.h    | 11 ++++--
>>>   xen/include/xen/iommu.h         | 22 ++++++++++++
>>>   3 files changed, 106 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/xen/drivers/passthrough/iommu.c 
>>> b/xen/drivers/passthrough/iommu.c
>>> index 1aecf7cf34..408f44106d 100644
>>> --- a/xen/drivers/passthrough/iommu.c
>>> +++ b/xen/drivers/passthrough/iommu.c
>>> @@ -13,6 +13,7 @@
>>>    */
>>>     #include <xen/sched.h>
>>> +#include <xen/fwnode.h>
>>>   #include <xen/iommu.h>
>>>   #include <xen/paging.h>
>>>   #include <xen/guest_access.h>
>>> @@ -507,6 +508,80 @@ static void iommu_dump_p2m_table(unsigned char key)
>>>       }
>>>   }
>>>   +/**
>>> + * fwnode_handle_put - Drop reference to a device node
>>> + * @fwnode: Pointer to the device node to drop the reference to.
>>> + *
>>> + * This has to be used when terminating device_for_each_child_node() 
>>> iteration
>>> + * with break or return to prevent stale device node references from 
>>> being left
>>> + * behind.
>>> + */
>>> +void fwnode_handle_put(struct fwnode_handle *fwnode)
>>> +{
>>> +        fwnode_call_void_op(fwnode, put);
>>
>> This file is following Xen coding style. And therefore you should use 
>> Xen coding.
>>
>>> +}
>>> +
>>> +const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle 
>>> *fwnode)
>>> +{
>>> +       return iommu_get_ops();
>>> +}
>>> +
>>> +int iommu_fwspec_init(struct device *dev, struct fwnode_handle 
>>> *iommu_fwnode,
>>> +                     const struct iommu_ops *ops)
>>> +{
>>> +       struct iommu_fwspec *fwspec = dev->iommu_fwspec;
>>> +
>>> +       if (fwspec)
>>> +               return ops == fwspec->ops ? 0 : -EINVAL;
>>> +
>>> +       fwspec = kzalloc(sizeof(*fwspec), GFP_KERNEL);
>>
>> You define kzalloc in a later patch and hence break the build. *All* 
>> the patches should build one by one to help bisectability.
>>
>> But given the side of the code and the fact you are going to fix the 
>> coding style. It might be easier to use Xen name here.
>>
>>> +       if (!fwspec)
>>> +               return -ENOMEM;
>>> +#if 0
>>> +       of_node_get(to_of_node(iommu_fwnode));
>>> +#endif
>>> +       fwspec->iommu_fwnode = iommu_fwnode; > + fwspec->ops = ops;
>>> +       dev->iommu_fwspec = fwspec;
>>> +       return 0;
>>> +}
>>> +
>>> +void iommu_fwspec_free(struct device *dev)
>>> +{
>>> +       struct iommu_fwspec *fwspec = dev->iommu_fwspec;
>>> +
>>> +       if (fwspec) {
>>> +               fwnode_handle_put(fwspec->iommu_fwnode);
>>> +               kfree(fwspec);
>>> +               dev->iommu_fwspec = NULL;
>>> +       }
>>> +}
>>> +
>>> +int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids)
>>> +{
>>> +  struct iommu_fwspec *fwspec = dev->iommu_fwspec;
>>> +        size_t size;
>>> +        int i;
>>> +
>>> +        if (!fwspec)
>>> +                return -EINVAL;
>>> +
>>> +        size = offsetof(struct iommu_fwspec, ids[fwspec->num_ids + 
>>> num_ids]);
>>> +        if (size > sizeof(*fwspec)) {
>>> +                //TBD: fwspec = krealloc(dev->iommu_fwspec, size, 
>>> GFP_KERNEL);
>>
>> Hmmm?
>>
>>> +                if (!fwspec)
>>> +                        return -ENOMEM;
>>> +
>>> +                dev->iommu_fwspec = fwspec;
>>> +        }
>>> +
>>> +        for (i = 0; i < num_ids; i++)
>>> +                fwspec->ids[fwspec->num_ids + i] = ids[i];
>>> +
>>> +        fwspec->num_ids += num_ids;
>>> +        return 0;
>>> +
>>> +}
>>>   /*
>>>    * Local variables:
>>>    * mode: C
>>> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
>>> index 6734ae8efd..f78482ca0c 100644
>>> --- a/xen/include/asm-arm/device.h
>>> +++ b/xen/include/asm-arm/device.h
>>> @@ -6,6 +6,8 @@
>>>   enum device_type
>>>   {
>>>       DEV_DT,
>>> +    DEV_ACPI,
>>
>> You don't use DEV_ACPI in this patch. So why is there?
>>
>>> +    DEV_PCI,
>>>   };
>>>     struct dev_archdata {
>>> @@ -18,8 +20,13 @@ struct device
>>>       enum device_type type;
>>>   #ifdef CONFIG_HAS_DEVICE_TREE
>>>       struct dt_device_node *of_node; /* Used by drivers imported 
>>> from Linux */
>>
>> As said on Sameer's patches, I was expecting a todo in the code after 
>> the discussion about leave of_node here.
> I might have missed your comment on sameers patch, could you please restate
>>
>>> +#endif
>>> +#ifdef CONFIG_ACPI
>>> +    void *acpi_node;
>>
>> You don't use acpi_node in this patch. So why is it there?
>>
>>>   #endif
>>>       struct dev_archdata archdata;
>>> +    struct fwnode_handle *fwnode; /* firmware device node */
>>
>> Ditto.
>>
>>> +    struct iommu_fwspec *iommu_fwspec;
>>>   };
>>>     typedef struct device device_t;
>>> @@ -27,8 +34,8 @@ typedef struct device device_t;
>>>   #include <xen/device_tree.h>
>>>     /* TODO: Correctly implement dev_is_pci when PCI is supported on 
>>> ARM */
>>> -#define dev_is_pci(dev) ((void)(dev), 0)
>>> -#define dev_is_dt(dev)  ((dev->type == DEV_DT)
>>> +#define dev_is_pci(dev) (dev->type == DEV_PCI)
>>> +#define dev_is_dt(dev)  (dev->type == DEV_DT)
>>
>> Those two changes does not belong to this patch. It is likely 2 
>> separate patches:
>>     1# fixing dev_is_dt because of the missing parenthese
>>     2# implementing dev_is_dt
>>
>>>     enum device_class
>>>   {
>>> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
>>> index 33c8b221dc..56b169bae9 100644
>>> --- a/xen/include/xen/iommu.h
>>> +++ b/xen/include/xen/iommu.h
>>> @@ -208,4 +208,26 @@ DECLARE_PER_CPU(bool_t, iommu_dont_flush_iotlb);
>>>   extern struct spinlock iommu_pt_cleanup_lock;
>>>   extern struct page_list_head iommu_pt_cleanup_list;
>>>   +/**
>>> + * struct iommu_fwspec - per-device IOMMU instance data
>>> + * @ops: ops for this device's IOMMU
>>> + * @iommu_fwnode: firmware handle for this device's IOMMU
>>> + * @iommu_priv: IOMMU driver private data for this device
>>> + * @num_ids: number of associated device IDs
>>> + * @ids: IDs which this device may present to the IOMMU
>>> + */
>>> +struct iommu_fwspec {
>>> +        const struct iommu_ops  *ops;
>>> +        struct fwnode_handle    *iommu_fwnode;
>>> +        void                    *iommu_priv;
>>> +        unsigned int            num_ids;
>>> +        u32                     ids[1];
>>> +};
>>> +
>>> +int iommu_fwspec_init(struct device *dev, struct fwnode_handle 
>>> *iommu_fwnode,
>>> +                      const struct iommu_ops *ops);
>>> +void iommu_fwspec_free(struct device *dev);
>>> +int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
>>> +const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle 
>>> *fwnode);
>>> +
>>>   #endif /* _IOMMU_H_ */
>>>
>>
>> Cheers,
>>
>

Patch

diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 1aecf7cf34..408f44106d 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -13,6 +13,7 @@ 
  */
 
 #include <xen/sched.h>
+#include <xen/fwnode.h>
 #include <xen/iommu.h>
 #include <xen/paging.h>
 #include <xen/guest_access.h>
@@ -507,6 +508,80 @@  static void iommu_dump_p2m_table(unsigned char key)
     }
 }
 
+/**
+ * fwnode_handle_put - Drop reference to a device node
+ * @fwnode: Pointer to the device node to drop the reference to.
+ *
+ * This has to be used when terminating device_for_each_child_node() iteration
+ * with break or return to prevent stale device node references from being left
+ * behind.
+ */
+void fwnode_handle_put(struct fwnode_handle *fwnode)
+{
+        fwnode_call_void_op(fwnode, put);
+}
+
+const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
+{
+       return iommu_get_ops();
+}
+
+int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
+                     const struct iommu_ops *ops)
+{
+       struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+
+       if (fwspec)
+               return ops == fwspec->ops ? 0 : -EINVAL;
+
+       fwspec = kzalloc(sizeof(*fwspec), GFP_KERNEL);
+       if (!fwspec)
+               return -ENOMEM;
+#if 0
+       of_node_get(to_of_node(iommu_fwnode));
+#endif
+       fwspec->iommu_fwnode = iommu_fwnode;
+       fwspec->ops = ops;
+       dev->iommu_fwspec = fwspec;
+       return 0;
+}
+
+void iommu_fwspec_free(struct device *dev)
+{
+       struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+
+       if (fwspec) {
+               fwnode_handle_put(fwspec->iommu_fwnode);
+               kfree(fwspec);
+               dev->iommu_fwspec = NULL;
+       }
+}
+
+int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids)
+{
+  struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+        size_t size;
+        int i;
+
+        if (!fwspec)
+                return -EINVAL;
+
+        size = offsetof(struct iommu_fwspec, ids[fwspec->num_ids + num_ids]);
+        if (size > sizeof(*fwspec)) {
+                //TBD: fwspec = krealloc(dev->iommu_fwspec, size, GFP_KERNEL);
+                if (!fwspec)
+                        return -ENOMEM;
+
+                dev->iommu_fwspec = fwspec;
+        }
+
+        for (i = 0; i < num_ids; i++)
+                fwspec->ids[fwspec->num_ids + i] = ids[i];
+
+        fwspec->num_ids += num_ids;
+        return 0;
+
+}
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
index 6734ae8efd..f78482ca0c 100644
--- a/xen/include/asm-arm/device.h
+++ b/xen/include/asm-arm/device.h
@@ -6,6 +6,8 @@ 
 enum device_type
 {
     DEV_DT,
+    DEV_ACPI,
+    DEV_PCI,
 };
 
 struct dev_archdata {
@@ -18,8 +20,13 @@  struct device
     enum device_type type;
 #ifdef CONFIG_HAS_DEVICE_TREE
     struct dt_device_node *of_node; /* Used by drivers imported from Linux */
+#endif
+#ifdef CONFIG_ACPI
+    void *acpi_node;
 #endif
     struct dev_archdata archdata;
+    struct fwnode_handle *fwnode; /* firmware device node */
+    struct iommu_fwspec *iommu_fwspec;
 };
 
 typedef struct device device_t;
@@ -27,8 +34,8 @@  typedef struct device device_t;
 #include <xen/device_tree.h>
 
 /* TODO: Correctly implement dev_is_pci when PCI is supported on ARM */
-#define dev_is_pci(dev) ((void)(dev), 0)
-#define dev_is_dt(dev)  ((dev->type == DEV_DT)
+#define dev_is_pci(dev) (dev->type == DEV_PCI)
+#define dev_is_dt(dev)  (dev->type == DEV_DT)
 
 enum device_class
 {
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 33c8b221dc..56b169bae9 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -208,4 +208,26 @@  DECLARE_PER_CPU(bool_t, iommu_dont_flush_iotlb);
 extern struct spinlock iommu_pt_cleanup_lock;
 extern struct page_list_head iommu_pt_cleanup_list;
 
+/**
+ * struct iommu_fwspec - per-device IOMMU instance data
+ * @ops: ops for this device's IOMMU
+ * @iommu_fwnode: firmware handle for this device's IOMMU
+ * @iommu_priv: IOMMU driver private data for this device
+ * @num_ids: number of associated device IDs
+ * @ids: IDs which this device may present to the IOMMU
+ */
+struct iommu_fwspec {
+        const struct iommu_ops  *ops;
+        struct fwnode_handle    *iommu_fwnode;
+        void                    *iommu_priv;
+        unsigned int            num_ids;
+        u32                     ids[1];
+};
+
+int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
+                      const struct iommu_ops *ops);
+void iommu_fwspec_free(struct device *dev);
+int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
+const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode);
+
 #endif /* _IOMMU_H_ */