date:20171102

Re: [RFCv2 PATCH 14/36] iommu/arm-smmu-v3: Add support for Substream IDs

2017-11-02 Thread Yisheng Xie

Hi Jean,

On 2017/11/3 1:02, Shameerali Kolothum Thodi wrote:
> 
> 
>> -Original Message-
>> From: Jean-Philippe Brucker [mailto:jean-philippe.bruc...@arm.com]
>> Sent: Thursday, November 02, 2017 3:52 PM
>> To: Shameerali Kolothum Thodi 
>> Cc: linux-arm-ker...@lists.infradead.org; linux-...@vger.kernel.org; linux-
>> a...@vger.kernel.org; devicet...@vger.kernel.org; iommu@lists.linux-
>> foundation.org; Mark Rutland ; xieyisheng (A)
>> ; Gabriele Paoloni
>> ; Catalin Marinas
>> ; Will Deacon ;
>> ok...@codeaurora.org; yi.l@intel.com; Lorenzo Pieralisi
>> ; ashok@intel.com; t...@semihalf.com;
>> j...@8bytes.org; rfr...@cavium.com; l...@kernel.org;
>> jacob.jun@linux.intel.com; alex.william...@redhat.com;
>> robh...@kernel.org; Leizhen (ThunderTown) ;
>> bhelg...@google.com; dw...@infradead.org; liubo (CU)
>> ; r...@rjwysocki.net; robdcl...@gmail.com;
>> hanjun@linaro.org; Sudeep Holla ; Robin
>> Murphy ; nwatt...@codeaurora.org; Linuxarm
>> 
>> Subject: Re: [RFCv2 PATCH 14/36] iommu/arm-smmu-v3: Add support for
>> Substream IDs
>>
>> Hi Shameer,
>>
>> On Thu, Nov 02, 2017 at 12:49:32PM +, Shameerali Kolothum Thodi wrote:
>>> We had a go with this series on HiSIlicon D05 platform which doesn't have
>>> support for ssids/ATS/PRI, to make sure it generally works.
>>>
>>> But observed the below crash on boot,
>>>
>>> [   16.009084] WARNING: CPU: 59 PID: 391 at mm/page_alloc.c:3883
>> __alloc_pages_nodemask+0x19c/0xc48
>>> [   16.026797] Modules linked in:
>>> [   16.032944] CPU: 59 PID: 391 Comm: kworker/59:1 Not tainted 4.14.0-rc1-
>> 159539-ge42aca3 #236
>>> [...]
>>> [   16.068206] Workqueue: events deferred_probe_work_func
>>> [   16.078557] task: 8017d38a task.stack: 0b198000
>>> [   16.090486] PC is at __alloc_pages_nodemask+0x19c/0xc48
>>> [   16.101013] LR is at __alloc_pages_nodemask+0xe0/0xc48
>>> [   16.469220] [] __alloc_pages_nodemask+0x19c/0xc48
>>> [   16.481854] [] alloc_pages_current+0x80/0xcc
>>> [   16.493607] [] __get_free_pages+0xc/0x38
>>> [   16.504661] [] swiotlb_alloc_coherent+0x64/0x190
>>> [   16.517117] [] __dma_alloc+0x110/0x204
>>> [   16.527820] [] dmam_alloc_coherent+0x88/0xf0
>>> [   16.539575] []
>> arm_smmu_domain_finalise_s1+0x60/0x248
>>> [   16.552909] [] arm_smmu_attach_dev+0x264/0x300
>>> [   16.565013] [] __iommu_attach_device+0x48/0x5c
>>> [   16.577117] [] iommu_group_add_device+0x144/0x3a4
>>> [   16.589746] [] iommu_group_get_for_dev+0x70/0xf8
>>> [   16.602201] [] arm_smmu_add_device+0x1a4/0x418
>>> [   16.614308] [] iort_iommu_configure+0xf0/0x16c
>>> [   16.626416] [] acpi_dma_configure+0x30/0x70
>>> [   16.637994] [] dma_configure+0xa8/0xd4
>>> [   16.648695] [] driver_probe_device+0x1a4/0x2dc
>>> [   16.673081] [] bus_for_each_drv+0x54/0x94
>>> [   16.684307] [] __device_attach+0xc4/0x12c
>>> [   16.695533] [] device_initial_probe+0x10/0x18
>>> [   16.707462] [] bus_probe_device+0x90/0x98
>>>
>>> After a bit of debug it looks like on platforms where ssid is not supported,
>>> s1_cfg.num_contexts is set to zero and it eventually results in this crash
>>> in,
>>> arm_smmu_domain_finalise_s1() -->arm_smmu_alloc_cd_tables()-->
>>> arm_smmu_alloc_cd_leaf_table() as num_leaf_entries is zero.
>>>
>>> With the below fix, it works on D05 now,
>>>
>>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>>> index 8ad90e2..51f5821 100644
>>> --- a/drivers/iommu/arm-smmu-v3.c
>>> +++ b/drivers/iommu/arm-smmu-v3.c
>>> @@ -2433,7 +2433,10 @@ static int arm_smmu_domain_finalise(struct
>> iommu_domain *domain,
>>> domain->min_pasid = 1;
>>> domain->max_pasid = master->num_ssids - 1;
>>> smmu_domain->s1_cfg.num_contexts = 
>>> master->num_ssids;
>>> +   } else {
>>> +   smmu_domain->s1_cfg.num_contexts = 1;
>>> }
>>> +
>>> smmu_domain->s1_cfg.can_stall = master->ste.can_stall;
>>> break;
>>> case ARM_SMMU_DOMAIN_NESTED:
>>>
>>>
>>> I am not sure this is right place do this. Please take a look.
>>
>> Thanks for testing the series and reporting the bug. I added the
>> following patch to branch svm/current, does it work for you?
> 
> Yes, it does.
> 
> Thanks,
> Shameer
>  
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index 42c8378624ed..edda466adc81 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -3169,9 +3169,7 @@ static int arm_smmu_add_device(struct device *dev)
>> }
>> }
>>
>> -   if (smmu->ssid_bits)
>> -   master->num_ssids = 1 << min(smmu->ssid_bits,

Re: [PATCH v2] iommu/iova: Use raw_cpu_ptr() instead of get_cpu_ptr() for ->fq

2017-11-02 Thread Sebastian Andrzej Siewior

On 2017-09-21 17:21:40 [+0200], Sebastian Andrzej Siewior wrote:
> get_cpu_ptr() disabled preemption and returns the ->fq object of the
> current CPU. raw_cpu_ptr() does the same except that it not disable
> preemption which means the scheduler can move it to another CPU after it
> obtained the per-CPU object.
> In this case this is not bad because the data structure itself is
> protected with a spin_lock. This change shouldn't matter however on RT
> it does because the sleeping lock can't be accessed with disabled
> preemption.

Did this make to your tree Jörg?

> Cc: Joerg Roedel 
> Cc: iommu@lists.linux-foundation.org
> Reported-by: vina...@gmail.com
> Signed-off-by: Sebastian Andrzej Siewior 
> ---
> On 2017-09-19 11:41:19 [+0200], Joerg Roedel wrote:
> > Hi Sebastian,
> Hi Jörg,
> 
> > I moved the flushing to driver/iommu/iova.c to share it with the Intel
> > IOMMU and possibly other drivers too, so this patch does no longer apply
> > to v4.14-rc1. Can you update the patch to these changes?
> 
> Sure.
> 
> v1…v2: move the change from amd_iommu.c to iova.c
> 
>  drivers/iommu/iova.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
> index 33edfa794ae9..b30900025c62 100644
> --- a/drivers/iommu/iova.c
> +++ b/drivers/iommu/iova.c
> @@ -570,7 +570,7 @@ void queue_iova(struct iova_domain *iovad,
>   unsigned long pfn, unsigned long pages,
>   unsigned long data)
>  {
> - struct iova_fq *fq = get_cpu_ptr(iovad->fq);
> + struct iova_fq *fq = raw_cpu_ptr(iovad->fq);
>   unsigned long flags;
>   unsigned idx;
>  
> @@ -600,8 +600,6 @@ void queue_iova(struct iova_domain *iovad,
>   if (atomic_cmpxchg(>fq_timer_on, 0, 1) == 0)
>   mod_timer(>fq_timer,
> jiffies + msecs_to_jiffies(IOVA_FQ_TIMEOUT));
> -
> - put_cpu_ptr(iovad->fq);
>  }
>  EXPORT_SYMBOL_GPL(queue_iova);
>  
> -- 
> 2.14.1
> 

Sebastian
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [RFCv2 PATCH 14/36] iommu/arm-smmu-v3: Add support for Substream IDs

2017-11-02 Thread Shameerali Kolothum Thodi



> -Original Message-
> From: Jean-Philippe Brucker [mailto:jean-philippe.bruc...@arm.com]
> Sent: Thursday, November 02, 2017 3:52 PM
> To: Shameerali Kolothum Thodi 
> Cc: linux-arm-ker...@lists.infradead.org; linux-...@vger.kernel.org; linux-
> a...@vger.kernel.org; devicet...@vger.kernel.org; iommu@lists.linux-
> foundation.org; Mark Rutland ; xieyisheng (A)
> ; Gabriele Paoloni
> ; Catalin Marinas
> ; Will Deacon ;
> ok...@codeaurora.org; yi.l@intel.com; Lorenzo Pieralisi
> ; ashok@intel.com; t...@semihalf.com;
> j...@8bytes.org; rfr...@cavium.com; l...@kernel.org;
> jacob.jun@linux.intel.com; alex.william...@redhat.com;
> robh...@kernel.org; Leizhen (ThunderTown) ;
> bhelg...@google.com; dw...@infradead.org; liubo (CU)
> ; r...@rjwysocki.net; robdcl...@gmail.com;
> hanjun@linaro.org; Sudeep Holla ; Robin
> Murphy ; nwatt...@codeaurora.org; Linuxarm
> 
> Subject: Re: [RFCv2 PATCH 14/36] iommu/arm-smmu-v3: Add support for
> Substream IDs
> 
> Hi Shameer,
> 
> On Thu, Nov 02, 2017 at 12:49:32PM +, Shameerali Kolothum Thodi wrote:
> > We had a go with this series on HiSIlicon D05 platform which doesn't have
> > support for ssids/ATS/PRI, to make sure it generally works.
> >
> > But observed the below crash on boot,
> >
> > [   16.009084] WARNING: CPU: 59 PID: 391 at mm/page_alloc.c:3883
> __alloc_pages_nodemask+0x19c/0xc48
> > [   16.026797] Modules linked in:
> > [   16.032944] CPU: 59 PID: 391 Comm: kworker/59:1 Not tainted 4.14.0-rc1-
> 159539-ge42aca3 #236
> > [...]
> > [   16.068206] Workqueue: events deferred_probe_work_func
> > [   16.078557] task: 8017d38a task.stack: 0b198000
> > [   16.090486] PC is at __alloc_pages_nodemask+0x19c/0xc48
> > [   16.101013] LR is at __alloc_pages_nodemask+0xe0/0xc48
> > [   16.469220] [] __alloc_pages_nodemask+0x19c/0xc48
> > [   16.481854] [] alloc_pages_current+0x80/0xcc
> > [   16.493607] [] __get_free_pages+0xc/0x38
> > [   16.504661] [] swiotlb_alloc_coherent+0x64/0x190
> > [   16.517117] [] __dma_alloc+0x110/0x204
> > [   16.527820] [] dmam_alloc_coherent+0x88/0xf0
> > [   16.539575] []
> arm_smmu_domain_finalise_s1+0x60/0x248
> > [   16.552909] [] arm_smmu_attach_dev+0x264/0x300
> > [   16.565013] [] __iommu_attach_device+0x48/0x5c
> > [   16.577117] [] iommu_group_add_device+0x144/0x3a4
> > [   16.589746] [] iommu_group_get_for_dev+0x70/0xf8
> > [   16.602201] [] arm_smmu_add_device+0x1a4/0x418
> > [   16.614308] [] iort_iommu_configure+0xf0/0x16c
> > [   16.626416] [] acpi_dma_configure+0x30/0x70
> > [   16.637994] [] dma_configure+0xa8/0xd4
> > [   16.648695] [] driver_probe_device+0x1a4/0x2dc
> > [   16.673081] [] bus_for_each_drv+0x54/0x94
> > [   16.684307] [] __device_attach+0xc4/0x12c
> > [   16.695533] [] device_initial_probe+0x10/0x18
> > [   16.707462] [] bus_probe_device+0x90/0x98
> >
> > After a bit of debug it looks like on platforms where ssid is not supported,
> > s1_cfg.num_contexts is set to zero and it eventually results in this crash
> > in,
> > arm_smmu_domain_finalise_s1() -->arm_smmu_alloc_cd_tables()-->
> > arm_smmu_alloc_cd_leaf_table() as num_leaf_entries is zero.
> >
> > With the below fix, it works on D05 now,
> >
> > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> > index 8ad90e2..51f5821 100644
> > --- a/drivers/iommu/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm-smmu-v3.c
> > @@ -2433,7 +2433,10 @@ static int arm_smmu_domain_finalise(struct
> iommu_domain *domain,
> > domain->min_pasid = 1;
> > domain->max_pasid = master->num_ssids - 1;
> > smmu_domain->s1_cfg.num_contexts = 
> > master->num_ssids;
> > +   } else {
> > +   smmu_domain->s1_cfg.num_contexts = 1;
> > }
> > +
> > smmu_domain->s1_cfg.can_stall = master->ste.can_stall;
> > break;
> > case ARM_SMMU_DOMAIN_NESTED:
> >
> >
> > I am not sure this is right place do this. Please take a look.
> 
> Thanks for testing the series and reporting the bug. I added the
> following patch to branch svm/current, does it work for you?

Yes, it does.

Thanks,
Shameer
 
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 42c8378624ed..edda466adc81 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -3169,9 +3169,7 @@ static int arm_smmu_add_device(struct device *dev)
> }
> }
> 
> -   if (smmu->ssid_bits)
> -   master->num_ssids = 1 << min(smmu->ssid_bits,
> -fwspec->num_pasid_bits);
> +   master->num_ssids = 1 <<

Re: [RFCv2 PATCH 05/36] iommu/process: Bind and unbind process to and from devices

2017-11-02 Thread Jean-Philippe Brucker

On 21/10/17 16:47, Sinan Kaya wrote:
> Just some improvement suggestions.
> 
> On 10/6/2017 9:31 AM, Jean-Philippe Brucker wrote:
>> +spin_lock(_process_lock);
>> +idr_for_each_entry(_process_idr, process, i) {
>> +if (process->pid != pid)
>> +continue;
> if you see this construct a lot, this could be a for_each_iommu_process.
> 
>> +
>> +if (!iommu_process_get_locked(process)) {
>> +/* Process is defunct, create a new one */
>> +process = NULL;
>> +break;
>> +}
>> +
>> +/* Great, is it also bound to this domain? */
>> +list_for_each_entry(cur_context, >domains,
>> +process_head) {
>> +if (cur_context->domain != domain)
>> +continue;
> if you see this construct a lot, this could be a 
> for_each_iommu_process_domain.
> 
>> +
>> +context = cur_context;
>> +*pasid = process->pasid;
>> +
>> +/* Splendid, tell the driver and increase the ref */
>> +err = iommu_process_attach_locked(context, dev);
>> +if (err)
>> +iommu_process_put_locked(process);
>> +
>> +break;
>> +}
>> +break;
>> +}
>> +spin_unlock(_process_lock);
>> +put_pid(pid);
>> +
>> +if (context)
>> +return err;
> 
> I think you should make the section above a independent function and return 
> here when the
> context is found.

Hopefully this code will only be needed for bind(), but moving it to a
separate function should look better.

Thanks,
Jean

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFCv2 PATCH 01/36] iommu: Keep track of processes and PASIDs

2017-11-02 Thread Jean-Philippe Brucker

Hi Sinan,

Sorry for the delay, thanks for reviewing this!

On 21/10/17 00:32, Sinan Kaya wrote:
> few nits below.
> 
>> +/*
>> + * Allocate a iommu_process structure for the given task.
>> + *
>> + * Ideally we shouldn't need the domain parameter, since iommu_process is
>> + * system-wide, but we use it to retrieve the driver's allocation ops and a
>> + * PASID range.
>> + */
>> +static struct iommu_process *
>> +iommu_process_alloc(struct iommu_domain *domain, struct task_struct *task)
>> +{
>> +int err;
>> +int pasid;
>> +struct iommu_process *process;
>> +
>> +if (WARN_ON(!domain->ops->process_alloc || !domain->ops->process_free))
>> +return ERR_PTR(-ENODEV);
>> +
>> +process = domain->ops->process_alloc(task);
>> +if (IS_ERR(process))
>> +return process;
>> +if (!process)
>> +return ERR_PTR(-ENOMEM);
>> +
>> +process->pid= get_task_pid(task, PIDTYPE_PID);
>> +process->release= domain->ops->process_free;
>> +INIT_LIST_HEAD(>domains);
>> +kref_init(>kref);
>> +
> nit, I think you should place this check right after the pid assignment.

Sure

>> +if (!process->pid) {
>> +err = -EINVAL;
>> +goto err_free_process;
>> +}
>> +
>> +idr_preload(GFP_KERNEL);
>> +spin_lock(_process_lock);
>> +pasid = idr_alloc_cyclic(_process_idr, process, domain->min_pasid,
>> + domain->max_pasid + 1, GFP_ATOMIC);
>> +process->pasid = pasid;
>> +spin_unlock(_process_lock);
>> +idr_preload_end();
>> +
> 
> nit, You can maybe return here if pasid is not negative.

Ok

>> +if (pasid < 0) {
>> +err = pasid;
>> +goto err_put_pid;
>> +}
>> +
>> +return process;
>> +
>> +err_put_pid:
>> +put_pid(process->pid);
>> +
>> +err_free_process:
>> +domain->ops->process_free(process);
>> +
>> +return ERR_PTR(err);
>> +}
>> +
>> +static void iommu_process_release(struct kref *kref)
>> +{
>> +struct iommu_process *process;
>> +void (*release)(struct iommu_process *);
>> +
>> +assert_spin_locked(_process_lock);
>> +
>> +process = container_of(kref, struct iommu_process, kref);
> 
> if we are concerned about things going wrong (assert above), we should
> also add some pointer check here (WARN) for process and release pointers as 
> well.

We can probably get rid of this assert, as any external users releasing
the process will have to go through iommu_process_put() which takes the
lock. process_alloc() ensures that release isn't NULL, and process should
always be valid here since we're being called from kref_put, but I should
check the value in process_put.

>> +release = process->release;
>> +
>> +WARN_ON(!list_empty(>domains));
>> +
>> +idr_remove(_process_idr, process->pasid);
>> +put_pid(process->pid);
>> +release(process);
>> +}
>> +
>> +/*
>> + * Returns non-zero if a reference to the process was successfully taken.
>> + * Returns zero if the process is being freed and should not be used.
>> + */
>> +static int iommu_process_get_locked(struct iommu_process *process)
>> +{
>> +assert_spin_locked(_process_lock);
>> +
>> +if (process)
>> +return kref_get_unless_zero(>kref);
>> +
>> +return 0;
>> +}
>> +
>> +static void iommu_process_put_locked(struct iommu_process *process)
>> +{
>> +assert_spin_locked(_process_lock);
>> +
>> +kref_put(>kref, iommu_process_release);
>> +}
>> +
>> +static int iommu_process_attach(struct iommu_domain *domain, struct device 
>> *dev,
>> +struct iommu_process *process)
>> +{
>> +int err;
>> +int pasid = process->pasid;
>> +struct iommu_context *context;
>> +
>> +if (WARN_ON(!domain->ops->process_attach || 
>> !domain->ops->process_detach))
>> +return -ENODEV;
>> +
>> +if (pasid > domain->max_pasid || pasid < domain->min_pasid)
>> +return -ENOSPC;
>> +
>> +context = kzalloc(sizeof(*context), GFP_KERNEL);
>> +if (!context)
>> +return -ENOMEM;
>> +
> 
> devm_kzalloc maybe?

I don't think we can ever leak contexts. Before the device is released, it
has to detach() from the domain, which will unbind from any process (call
unbind_dev_all()).

Thanks,
Jean
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFCv2 PATCH 14/36] iommu/arm-smmu-v3: Add support for Substream IDs

2017-11-02 Thread Jean-Philippe Brucker

Hi Shameer,

On Thu, Nov 02, 2017 at 12:49:32PM +, Shameerali Kolothum Thodi wrote:
> We had a go with this series on HiSIlicon D05 platform which doesn't have
> support for ssids/ATS/PRI, to make sure it generally works.
> 
> But observed the below crash on boot,
> 
> [   16.009084] WARNING: CPU: 59 PID: 391 at mm/page_alloc.c:3883 
> __alloc_pages_nodemask+0x19c/0xc48
> [   16.026797] Modules linked in:
> [   16.032944] CPU: 59 PID: 391 Comm: kworker/59:1 Not tainted 
> 4.14.0-rc1-159539-ge42aca3 #236
> [...]
> [   16.068206] Workqueue: events deferred_probe_work_func
> [   16.078557] task: 8017d38a task.stack: 0b198000
> [   16.090486] PC is at __alloc_pages_nodemask+0x19c/0xc48
> [   16.101013] LR is at __alloc_pages_nodemask+0xe0/0xc48
> [   16.469220] [] __alloc_pages_nodemask+0x19c/0xc48
> [   16.481854] [] alloc_pages_current+0x80/0xcc
> [   16.493607] [] __get_free_pages+0xc/0x38
> [   16.504661] [] swiotlb_alloc_coherent+0x64/0x190
> [   16.517117] [] __dma_alloc+0x110/0x204
> [   16.527820] [] dmam_alloc_coherent+0x88/0xf0
> [   16.539575] [] arm_smmu_domain_finalise_s1+0x60/0x248
> [   16.552909] [] arm_smmu_attach_dev+0x264/0x300
> [   16.565013] [] __iommu_attach_device+0x48/0x5c
> [   16.577117] [] iommu_group_add_device+0x144/0x3a4
> [   16.589746] [] iommu_group_get_for_dev+0x70/0xf8
> [   16.602201] [] arm_smmu_add_device+0x1a4/0x418
> [   16.614308] [] iort_iommu_configure+0xf0/0x16c
> [   16.626416] [] acpi_dma_configure+0x30/0x70
> [   16.637994] [] dma_configure+0xa8/0xd4
> [   16.648695] [] driver_probe_device+0x1a4/0x2dc
> [   16.673081] [] bus_for_each_drv+0x54/0x94
> [   16.684307] [] __device_attach+0xc4/0x12c
> [   16.695533] [] device_initial_probe+0x10/0x18
> [   16.707462] [] bus_probe_device+0x90/0x98
> 
> After a bit of debug it looks like on platforms where ssid is not supported,
> s1_cfg.num_contexts is set to zero and it eventually results in this crash 
> in,
> arm_smmu_domain_finalise_s1() -->arm_smmu_alloc_cd_tables()-->
> arm_smmu_alloc_cd_leaf_table() as num_leaf_entries is zero.
> 
> With the below fix, it works on D05 now,
> 
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 8ad90e2..51f5821 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -2433,7 +2433,10 @@ static int arm_smmu_domain_finalise(struct 
> iommu_domain *domain,
> domain->min_pasid = 1;
> domain->max_pasid = master->num_ssids - 1;
> smmu_domain->s1_cfg.num_contexts = master->num_ssids;
> +   } else {
> +   smmu_domain->s1_cfg.num_contexts = 1;
> }
> +
> smmu_domain->s1_cfg.can_stall = master->ste.can_stall;
> break;
> case ARM_SMMU_DOMAIN_NESTED:
> 
> 
> I am not sure this is right place do this. Please take a look.

Thanks for testing the series and reporting the bug. I added the
following patch to branch svm/current, does it work for you?

Thanks,
Jean

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 42c8378624ed..edda466adc81 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -3169,9 +3169,7 @@ static int arm_smmu_add_device(struct device *dev)
}
}

-   if (smmu->ssid_bits)
-   master->num_ssids = 1 << min(smmu->ssid_bits,
-fwspec->num_pasid_bits);
+   master->num_ssids = 1 << min(smmu->ssid_bits, fwspec->num_pasid_bits);

if (fwspec->can_stall && smmu->features & ARM_SMMU_FEAT_STALLS) {
master->can_fault = true;

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [RFCv2 PATCH 14/36] iommu/arm-smmu-v3: Add support for Substream IDs

2017-11-02 Thread Shameerali Kolothum Thodi

Hi Jean,

> -Original Message-
> From: linux-arm-kernel [mailto:linux-arm-kernel-boun...@lists.infradead.org]
> On Behalf Of Jean-Philippe Brucker
> Sent: Friday, October 06, 2017 2:32 PM
> To: linux-arm-ker...@lists.infradead.org; linux-...@vger.kernel.org; linux-
> a...@vger.kernel.org; devicet...@vger.kernel.org; iommu@lists.linux-
> foundation.org
> Cc: mark.rutl...@arm.com; xieyisheng (A) ;
> Gabriele Paoloni ; catalin.mari...@arm.com;
> will.dea...@arm.com; ok...@codeaurora.org; yi.l@intel.com;
> lorenzo.pieral...@arm.com; ashok@intel.com; t...@semihalf.com;
> j...@8bytes.org; rfr...@cavium.com; l...@kernel.org;
> jacob.jun@linux.intel.com; alex.william...@redhat.com;
> robh...@kernel.org; Leizhen (ThunderTown) ;
> bhelg...@google.com; dw...@infradead.org; liubo (CU)
> ; r...@rjwysocki.net; robdcl...@gmail.com;
> hanjun@linaro.org; sudeep.ho...@arm.com; robin.mur...@arm.com;
> nwatt...@codeaurora.org
> Subject: [RFCv2 PATCH 14/36] iommu/arm-smmu-v3: Add support for
> Substream IDs
> 
> At the moment, the SMMUv3 driver offers only one stage-1 or stage-2
> address space to each device. SMMUv3 allows to associate multiple address
> spaces per device. In addition to the Stream ID (SID), that identifies a
> device, we can now have Substream IDs (SSID) identifying an address space.
> In PCIe lingo, SID is called Requester ID (RID) and SSID is called Process
> Address-Space ID (PASID).

We had a go with this series on HiSIlicon D05 platform which doesn't have
support for ssids/ATS/PRI, to make sure it generally works.

But observed the below crash on boot,

[   16.009084] WARNING: CPU: 59 PID: 391 at mm/page_alloc.c:3883 
__alloc_pages_nodemask+0x19c/0xc48
[   16.026797] Modules linked in:
[   16.032944] CPU: 59 PID: 391 Comm: kworker/59:1 Not tainted 
4.14.0-rc1-159539-ge42aca3 #236
[...]
[   16.068206] Workqueue: events deferred_probe_work_func
[   16.078557] task: 8017d38a task.stack: 0b198000
[   16.090486] PC is at __alloc_pages_nodemask+0x19c/0xc48
[   16.101013] LR is at __alloc_pages_nodemask+0xe0/0xc48
[   16.469220] [] __alloc_pages_nodemask+0x19c/0xc48
[   16.481854] [] alloc_pages_current+0x80/0xcc
[   16.493607] [] __get_free_pages+0xc/0x38
[   16.504661] [] swiotlb_alloc_coherent+0x64/0x190
[   16.517117] [] __dma_alloc+0x110/0x204
[   16.527820] [] dmam_alloc_coherent+0x88/0xf0
[   16.539575] [] arm_smmu_domain_finalise_s1+0x60/0x248
[   16.552909] [] arm_smmu_attach_dev+0x264/0x300
[   16.565013] [] __iommu_attach_device+0x48/0x5c
[   16.577117] [] iommu_group_add_device+0x144/0x3a4
[   16.589746] [] iommu_group_get_for_dev+0x70/0xf8
[   16.602201] [] arm_smmu_add_device+0x1a4/0x418
[   16.614308] [] iort_iommu_configure+0xf0/0x16c
[   16.626416] [] acpi_dma_configure+0x30/0x70
[   16.637994] [] dma_configure+0xa8/0xd4
[   16.648695] [] driver_probe_device+0x1a4/0x2dc
[   16.673081] [] bus_for_each_drv+0x54/0x94
[   16.684307] [] __device_attach+0xc4/0x12c
[   16.695533] [] device_initial_probe+0x10/0x18
[   16.707462] [] bus_probe_device+0x90/0x98

After a bit of debug it looks like on platforms where ssid is not supported,
s1_cfg.num_contexts is set to zero and it eventually results in this crash 
in,
arm_smmu_domain_finalise_s1() -->arm_smmu_alloc_cd_tables()-->
arm_smmu_alloc_cd_leaf_table() as num_leaf_entries is zero.

With the below fix, it works on D05 now,

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 8ad90e2..51f5821 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -2433,7 +2433,10 @@ static int arm_smmu_domain_finalise(struct iommu_domain 
*domain,
domain->min_pasid = 1;
domain->max_pasid = master->num_ssids - 1;
smmu_domain->s1_cfg.num_contexts = master->num_ssids;
+   } else {
+   smmu_domain->s1_cfg.num_contexts = 1;
}
+
smmu_domain->s1_cfg.can_stall = master->ste.can_stall;
break;
case ARM_SMMU_DOMAIN_NESTED:


I am not sure this is right place do this. Please take a look.

Thanks,
Shameer

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFCv2 PATCH 14/36] iommu/arm-smmu-v3: Add support for Substream IDs

Re: [PATCH v2] iommu/iova: Use raw_cpu_ptr() instead of get_cpu_ptr() for ->fq

RE: [RFCv2 PATCH 14/36] iommu/arm-smmu-v3: Add support for Substream IDs

Re: [RFCv2 PATCH 05/36] iommu/process: Bind and unbind process to and from devices

Re: [RFCv2 PATCH 01/36] iommu: Keep track of processes and PASIDs

Re: [RFCv2 PATCH 14/36] iommu/arm-smmu-v3: Add support for Substream IDs

RE: [RFCv2 PATCH 14/36] iommu/arm-smmu-v3: Add support for Substream IDs

7 matches

Site Navigation

Mail list logo

Footer information