Re: [PATCH] swiotlb: Update the comment of swiotlb

[Yisheng Xie]

Hi Christoph,

On 2018/5/9 15:38, Christoph Hellwig wrote:
> Thanks,
> 
> applied to the dma-mapping tree.

Thanks

BTW, should lib/swiotlb.c also add to DMA MAPPING HELPERS, or
add yourself as a maintainer of SWIOTLB SUBSYSTEM ? It will make
get_maintainer.pl get you :)

Thanks
Yisheng
> 
> .
> 

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

[PATCH] swiotlb: Update the comment of swiotlb

[Yisheng Xie]

swiotlb use physical address of bounce buffer when do map and unmap,
therefore, related comment should be updated.

Signed-off-by: Yisheng Xie <xieyishe...@huawei.com>
---
 lib/swiotlb.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index fece575..e3d9445 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -593,9 +593,8 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
 }
 
 /*
- * Allocates bounce buffer and returns its kernel virtual address.
+ * Allocates bounce buffer and returns its physical address.
  */
-
 static phys_addr_t
 map_single(struct device *hwdev, phys_addr_t phys, size_t size,
   enum dma_data_direction dir, unsigned long attrs)
@@ -614,7 +613,7 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
 }
 
 /*
- * dma_addr is the kernel virtual address of the bounce buffer to unmap.
+ * tlb_addr is the physical address of the bounce buffer to unmap.
  */
 void swiotlb_tbl_unmap_single(struct device *hwdev, phys_addr_t tlb_addr,
  size_t size, enum dma_data_direction dir,
-- 
1.7.12.4

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

Re: [RFC 1/2] iommu/arm-smmu-v3: Remove bypass in arm_smmu_reset_device

[Yisheng Xie]

Hi Robin,

Thanks for your comment!
On 2018/4/24 0:07, Robin Murphy wrote:
> On 23/04/18 12:45, Yisheng Xie wrote:
>> Add a bypass parameter in arm_smmu_device to keep whether smmu device
>> should pypass or not, so parameter bypass in arm_smmu_reset_device can
>> be removed.
> 
> Given that the GBPA configuration implied by the bypass argument here
> is only there to avoid initialising a full stream table when the firmware
> is terminally broken, I wonder if it would make sense to simply skip
> allocating a stream table at all in that case. Then we could just base
> the subsequent SMMUEN/GPBA decision on whether strtab_cfg.strtab is valid or 
> not.
> 
Yes, it may save many memory, and should be care about not access strtab when 
attach
device or other similar scenario, anyway software can achieve this after move 
bypass
to struct arm_smmu_device.

Thanks
Yisheng
> Robin.
> 

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

Re: [RFC 2/2] iommu/arm-smmu-v3: Support software retention for pm_resume

[Yisheng Xie]

Hi Robin,

Thanks for your comment.
On 2018/4/24 0:14, Robin Murphy wrote:
> On 23/04/18 12:45, Yisheng Xie wrote:
>> When system suspend, hisilicon's smmu will do power gating for smmu,
>> this time smmu's reg will be set to default value for not having
>> hardware retention, which means need software do the retention instead.
>>
>> The patch is to use arm_smmu_device_reset() to restore the register of
>> smmu. However, it need to save the msis setting at probe if smmu do not
>> support hardware retention.
>>
>> Signed-off-by: Yisheng Xie <xieyishe...@huawei.com>
>> ---
>>   drivers/iommu/arm-smmu-v3.c | 69 
>> +++--
>>   1 file changed, 66 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index 044df6e..6cb56d8 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -534,6 +534,11 @@ struct arm_smmu_strtab_cfg {
>>   u32strtab_base_cfg;
>>   };
>>   +struct arm_smmu_msi_val {
>> +u64doorbell;
>> +u32data;
>> +};
> 
> What does this do that struct msi_msg doesn't already (apart from take up 
> more space in an array)?

Right, struct msi_msg can be used instead, and memcpy can be used when save the 
msi_msg.

> 
>> +
>>   /* An SMMUv3 instance */
>>   struct arm_smmu_device {
>>   struct device*dev;
>> @@ -558,6 +563,7 @@ struct arm_smmu_device {
>> #define ARM_SMMU_OPT_SKIP_PREFETCH(1 << 0)
>>   #define ARM_SMMU_OPT_PAGE0_REGS_ONLY(1 << 1)
>> +#define ARM_SMMU_OPT_SW_RETENTION(1 << 2)
>>   u32options;
>> struct arm_smmu_cmdqcmdq;
>> @@ -587,6 +593,8 @@ struct arm_smmu_device {
>> u32sync_count;
>>   +struct arm_smmu_msi_val*msi;
>> +boolprobed;
> 
> This looks really hacky. I'm sure there's probably enough driver model 
> information
> to be able to identify the probe state from just the struct device, but 
> that's still
> not the right way to go. If you need to know this, then it can only mean 
> we've got
> one-time software state initialisation mixed in with the actual hardware 
> reset which
> programs the software state into the device. Thus there should be some 
> refactoring to
> properly separate those concerns.

So you mean status enum should use instead? Maybe something like:
enum arm_smmu_status {
ARM_SMMU_INACTIVE;
ARM_SMMU_ACTIVE;
ARM_SMMU_SUSPEND;
};
to indicate the status of device ?

> 
>>   boolbypass;
>> /* IOMMU core code handle */
>> @@ -630,6 +638,7 @@ struct arm_smmu_option_prop {
>>   static struct arm_smmu_option_prop arm_smmu_options[] = {
>>   { ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" },
>>   { ARM_SMMU_OPT_PAGE0_REGS_ONLY, "cavium,cn9900-broken-page1-regspace"},
>> +{ ARM_SMMU_OPT_SW_RETENTION, "hisilicon,broken-hardware-retention" },
> 
> That seems a bit over-specific - there are going to be any number of SMMU 
> implementations/integrations
> which may or may not implement hardware retention states. More crucially, 
> it's also backwards. Making
> the driver assume that *every* SMMU implements hardware retention unless this 
> new DT property is present
> is quite obviously completely wrong, especially for ACPI...
> 
I was thought about remove the word *hisilicon* for other implement may
also not have hardware retention. Anyway, this a wrong for ACPI cannot
works without hardware retention.

> The sensible thing to do is to implement suspend/resume support which works 
> in general, *then* consider
> optimising it for cases where explicitly restoring the hardware state may be 
> skipped (if indeed it makes
> a significant difference).

Yes, we should make it common, what I was doubt is that whether should enable it
by default or not, for someone who do not want do power gating at all when 
suspend,
it may suffer from device reset when resume ?

> Are there not already generic DT/ACPI properties for describing the retention
> levels of different power states, which could be made use of here?

AFAIK no, and I have just tried to find some clue from the document of DT/ACPI
but do not find any usefull information. Do you have any idea or clue about it?

>>   { 0, NULL},
>>   };
>>   @@ -2228,7 +2237,8 @@ static void arm_smmu_write_msi_msg(struct msi_desc 
>> *desc, struct msi_msg *msg)
>>   phys_addr_t doorbell;
>>  

[RFC 2/2] iommu/arm-smmu-v3: Support software retention for pm_resume

[Yisheng Xie]

When system suspend, hisilicon's smmu will do power gating for smmu,
this time smmu's reg will be set to default value for not having
hardware retention, which means need software do the retention instead.

The patch is to use arm_smmu_device_reset() to restore the register of
smmu. However, it need to save the msis setting at probe if smmu do not
support hardware retention.

Signed-off-by: Yisheng Xie <xieyishe...@huawei.com>
---
 drivers/iommu/arm-smmu-v3.c | 69 +++--
 1 file changed, 66 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 044df6e..6cb56d8 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -534,6 +534,11 @@ struct arm_smmu_strtab_cfg {
u32 strtab_base_cfg;
 };
 
+struct arm_smmu_msi_val {
+   u64 doorbell;
+   u32 data;
+};
+
 /* An SMMUv3 instance */
 struct arm_smmu_device {
struct device   *dev;
@@ -558,6 +563,7 @@ struct arm_smmu_device {
 
 #define ARM_SMMU_OPT_SKIP_PREFETCH (1 << 0)
 #define ARM_SMMU_OPT_PAGE0_REGS_ONLY   (1 << 1)
+#define ARM_SMMU_OPT_SW_RETENTION  (1 << 2)
u32 options;
 
struct arm_smmu_cmdqcmdq;
@@ -587,6 +593,8 @@ struct arm_smmu_device {
 
u32 sync_count;
 
+   struct arm_smmu_msi_val *msi;
+   boolprobed;
boolbypass;
 
/* IOMMU core code handle */
@@ -630,6 +638,7 @@ struct arm_smmu_option_prop {
 static struct arm_smmu_option_prop arm_smmu_options[] = {
{ ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" },
{ ARM_SMMU_OPT_PAGE0_REGS_ONLY, "cavium,cn9900-broken-page1-regspace"},
+   { ARM_SMMU_OPT_SW_RETENTION, "hisilicon,broken-hardware-retention" },
{ 0, NULL},
 };
 
@@ -2228,7 +2237,8 @@ static void arm_smmu_write_msi_msg(struct msi_desc *desc, 
struct msi_msg *msg)
phys_addr_t doorbell;
struct device *dev = msi_desc_to_dev(desc);
struct arm_smmu_device *smmu = dev_get_drvdata(dev);
-   phys_addr_t *cfg = arm_smmu_msi_cfg[desc->platform.msi_index];
+   int msi_index = desc->platform.msi_index;
+   phys_addr_t *cfg = arm_smmu_msi_cfg[msi_index];
 
doorbell = (((u64)msg->address_hi) << 32) | msg->address_lo;
doorbell &= MSI_CFG0_ADDR_MASK;
@@ -2236,6 +2246,28 @@ static void arm_smmu_write_msi_msg(struct msi_desc 
*desc, struct msi_msg *msg)
writeq_relaxed(doorbell, smmu->base + cfg[0]);
writel_relaxed(msg->data, smmu->base + cfg[1]);
writel_relaxed(ARM_SMMU_MEMATTR_DEVICE_nGnRE, smmu->base + cfg[2]);
+
+   if (smmu->options & ARM_SMMU_OPT_SW_RETENTION) {
+   smmu->msi[msi_index].doorbell = doorbell;
+   smmu->msi[msi_index].data = msg->data;
+   }
+}
+
+static void arm_smmu_restore_msis(struct arm_smmu_device *smmu)
+{
+   int nevc = ARM_SMMU_MAX_MSIS - 1;
+
+   if (!(smmu->features & ARM_SMMU_FEAT_PRI))
+   nevc--;
+
+   for (; nevc >= 0; nevc--) {
+   phys_addr_t *cfg = arm_smmu_msi_cfg[nevc];
+   struct arm_smmu_msi_val msi_val = smmu->msi[nevc];
+
+   writeq_relaxed(msi_val.doorbell, smmu->base + cfg[0]);
+   writel_relaxed(msi_val.data, smmu->base + cfg[1]);
+   writel_relaxed(ARM_SMMU_MEMATTR_DEVICE_nGnRE, smmu->base + 
cfg[2]);
+   }
 }
 
 static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
@@ -2261,6 +2293,16 @@ static void arm_smmu_setup_msis(struct arm_smmu_device 
*smmu)
return;
}
 
+   if (smmu->probed) {
+   BUG_ON(!(smmu->options & ARM_SMMU_OPT_SW_RETENTION));
+   arm_smmu_restore_msis(smmu);
+   return;
+   } else if (smmu->options & ARM_SMMU_OPT_SW_RETENTION) {
+   smmu->msi = devm_kmalloc_array(dev, nvec,
+  sizeof(*(smmu->msi)),
+  GFP_KERNEL);
+   }
+
/* Allocate MSIs for evtq, gerror and priq. Ignore cmdq */
ret = platform_msi_domain_alloc_irqs(dev, nvec, arm_smmu_write_msi_msg);
if (ret) {
@@ -2294,6 +2336,9 @@ static void arm_smmu_setup_unique_irqs(struct 
arm_smmu_device *smmu)
 
arm_smmu_setup_msis(smmu);
 
+   if (smmu->probed)
+   return;
+
/* Request interrupt lines */
irq = smmu->evtq.q.irq;
if (irq) {
@@ -2348,7 +2393,7 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device 
*smmu)
}
 
irq = smmu->combined_irq;
-   if (irq) {
+   if (irq &am

[RFC 0/2] iommu/arm-smmu-v3: Support software retention for pm_resume

[Yisheng Xie]

- Backgroud:
 Hisilicon's implement of smmuv3 do not support hardware retention if system
 do power gating when system suspend, however for embed system, we do need
 to do power gating at trust zone for lower power comsume. So software
 retention is need.

- Implement:
 From the process of smmu probe, it will get the feature of smmu from IDR
 registers and keep the status in struct arm_smmu_device, then
 arm_smmu_device_reset() will initial(set) most of the registers according
 to it. So we can use arm_smmu_device_reset() to re-initial the smmuv3
 device to make it can works again.
 
 To achieve this, patch 1 move the bypass parameter from arm_smmu_device_reset()
 to struct arm_smmu_device to make arm_smmu_device_reset() re-usable. Patch 2
 introduces probed parameter for struct arm_smmu_device, so once smmuv3 is 
probed
 we can avoid request irqs once more. It also introduce a struct 
arm_smmu_msi_val
 to keep the value of smmuv3's msi register which can be restore when reset 
device
 after probed.

Yisheng Xie (2):
  iommu/arm-smmu-v3: Remove bypass in arm_smmu_reset_device
  iommu/arm-smmu-v3: Support software retention for pm_resume

 drivers/iommu/arm-smmu-v3.c | 80 -
 1 file changed, 72 insertions(+), 8 deletions(-)

-- 
1.7.12.4

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

Re: [PATCH v9 5/5] iommu/arm-smmu: Add support for qcom,smmu-v2 variant

[Yisheng Xie]

Hi Vivek,

On 2018/4/11 13:15, Vivek Gautam wrote:
> Hi Yisheng,
> 
> 
> On 4/11/2018 6:52 AM, Yisheng Xie wrote:
>> Hi Tomasz,
>>
>> On 2018/4/10 21:14, Tomasz Figa wrote:
>>> Hi Yisheng,
>>>
>>> Sorry, I think we missed your question here.
>>>
>>> On Wed, Mar 28, 2018 at 3:12 PM Yisheng Xie <xieyishe...@huawei.com> wrote:
>>>
>>>> Hi Vivek,
>>>> On 2018/3/28 12:37, Vivek Gautam wrote:
>>>>> Hi Yisheng
>>>>>
>>>>>
>>>>> On 3/28/2018 6:54 AM, Yisheng Xie wrote:
>>>>>> Hi Vivek,
>>>>>>
>>>>>> On 2018/3/13 16:55, Vivek Gautam wrote:
>>>>>>> +- power-domains:  Specifiers for power domains required to be
>>> powered on for
>>>>>>> +  the SMMU to operate, as per generic power domain
>>> bindings.
>>>>>>> +
>>>>>> In this patchset, power-domains is not used right? And you just do the
>>> clock gating,
>>>>>> but not power gating, right?
>>>>> We are handling the power-domains too. Please see the example in this
>>> binding doc.
>>>
>>>> I see, but I do not find the point in code of these patchset, do you mean
>>> PMIC(e.g mmcc)
>>>> will gate the power domain of SMMU(e.g. MDSS_GDSC of mmcc) when PMIC
>>> suspend?
>>>
>>>
>>> If respective SoC power domains is registered as a standard genpd PM
>>> domain, then the runtime PM subsystem will take care of power domain
>>> control at runtime suspend and resume.
>>>
>> Get it, thanks for your explain, I should have learned about this.
> 
> Sorry, i missed your subsequent question, and Tomasz has explained it now.

Never mind about that.

> Let me know if you have further questions.

Presently, no other questions. Thanks for your kind help.

Thanks
Yisheng
> 
> regards
> Vivek
>>
>> Thanks
>> Yisheng
>>
>>> Best regards,
>>> Tomasz
>>>
>>> .
>>>
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> .
> 

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

Re: [PATCH v9 5/5] iommu/arm-smmu: Add support for qcom,smmu-v2 variant

[Yisheng Xie]

Hi Tomasz,

On 2018/4/10 21:14, Tomasz Figa wrote:
> Hi Yisheng,
> 
> Sorry, I think we missed your question here.
> 
> On Wed, Mar 28, 2018 at 3:12 PM Yisheng Xie <xieyishe...@huawei.com> wrote:
> 
>> Hi Vivek,
> 
>> On 2018/3/28 12:37, Vivek Gautam wrote:
>>> Hi Yisheng
>>>
>>>
>>> On 3/28/2018 6:54 AM, Yisheng Xie wrote:
>>>> Hi Vivek,
>>>>
>>>> On 2018/3/13 16:55, Vivek Gautam wrote:
>>>>> +- power-domains:  Specifiers for power domains required to be
> powered on for
>>>>> +  the SMMU to operate, as per generic power domain
> bindings.
>>>>> +
>>>> In this patchset, power-domains is not used right? And you just do the
> clock gating,
>>>> but not power gating, right?
>>>
>>> We are handling the power-domains too. Please see the example in this
> binding doc.
> 
>> I see, but I do not find the point in code of these patchset, do you mean
> PMIC(e.g mmcc)
>> will gate the power domain of SMMU(e.g. MDSS_GDSC of mmcc) when PMIC
> suspend?
> 
> 
> If respective SoC power domains is registered as a standard genpd PM
> domain, then the runtime PM subsystem will take care of power domain
> control at runtime suspend and resume.
> 

Get it, thanks for your explain, I should have learned about this.

Thanks
Yisheng

> Best regards,
> Tomasz
> 
> .
> 

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

Re: [PATCH v9 5/5] iommu/arm-smmu: Add support for qcom,smmu-v2 variant

[Yisheng Xie]

Hi Vivek,

On 2018/3/28 12:37, Vivek Gautam wrote:
> Hi Yisheng
> 
> 
> On 3/28/2018 6:54 AM, Yisheng Xie wrote:
>> Hi Vivek,
>>
>> On 2018/3/13 16:55, Vivek Gautam wrote:
>>> +- power-domains:  Specifiers for power domains required to be powered on 
>>> for
>>> +  the SMMU to operate, as per generic power domain 
>>> bindings.
>>> +
>> In this patchset, power-domains is not used right? And you just do the clock 
>> gating,
>> but not power gating, right?
> 
> We are handling the power-domains too. Please see the example in this binding 
> doc.

I see, but I do not find the point in code of these patchset, do you mean 
PMIC(e.g mmcc)
will gate the power domain of SMMU(e.g. MDSS_GDSC of mmcc) when PMIC suspend?

> 
>>
>> Another question is if smmu do power gating, it will reset some of its 
>> registers, so
>> it need save at suspend and restore at resume, right?
> 
> Qualcomm implementation of the arm-smmu has the retenetion enabled. So the 
> smmu doesn't
> loose state when power is pulled out of it.
> And now we are just selectively enabling the runtime pm. So only the 
> platforms that can really
> support runtime pm can enable it.

Get it, thanks for your explain.

Thanks
Yisheng
> 
> Thanks
> Vivek
>>
>> Thanks
>> Yisheng
>>
> 
> 
> 

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

Re: [PATCH 27/37] iommu/arm-smmu-v3: Register fault workqueue

[Yisheng Xie]

Hi Jean,

On 2018/3/21 21:24, Jean-Philippe Brucker wrote:
> Hi Yisheng,
> 
> On 19/03/18 11:03, Yisheng Xie wrote:
>> Hi Jean,
>>
>> [...]
>>> @@ -3168,6 +3260,13 @@ static int arm_smmu_device_probe(struct 
>>> platform_device *pdev)
>>> if (ret)
>>> return ret;
>>>  
>>> +   if (smmu->features & (ARM_SMMU_FEAT_STALLS | ARM_SMMU_FEAT_PRI)) {
>>> +   smmu->faultq_nb.notifier_call = arm_smmu_flush_queues;
>>> +   ret = iommu_fault_queue_register(>faultq_nb);
>>> +   if (ret)
>>> +   return ret;
>>
>> I find a case here: CONFIG_IOMMU_FAULT=n, and smmu support feature STALLS or 
>> PRI,
>> the device probe will failed here, is this what we want?
> 
> Since CONFIG_ARM_SMMU_V3 selects CONFIG_IOMMU_FAULT, I don't think it
> can happen. 

But CONFIG_IOMMU_FAULT can be changed after select, maybe can make it as 
unchangeable.

Thanks
Yisheng

> I'm not sure what the best practices are, but I feel like it
> would be too much work to guard against config combinations that violate
> an explicit "select" in Kconfig
> 
> Thanks,
> Jean
> 
> .
> 

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

Re: [PATCH 27/37] iommu/arm-smmu-v3: Register fault workqueue

[Yisheng Xie]

Hi Jean,

[...]
> @@ -3168,6 +3260,13 @@ static int arm_smmu_device_probe(struct 
> platform_device *pdev)
>   if (ret)
>   return ret;
>  
> + if (smmu->features & (ARM_SMMU_FEAT_STALLS | ARM_SMMU_FEAT_PRI)) {
> + smmu->faultq_nb.notifier_call = arm_smmu_flush_queues;
> + ret = iommu_fault_queue_register(>faultq_nb);
> + if (ret)
> + return ret;

I find a case here: CONFIG_IOMMU_FAULT=n, and smmu support feature STALLS or 
PRI,
the device probe will failed here, is this what we want?

Thanks
Yisheng

> + }
> +
>   /* And we're up. Go go go! */
>   ret = iommu_device_sysfs_add(>iommu, dev, NULL,
>"smmu3.%pa", );

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

Re: [PATCH 37/37] vfio: Add support for Shared Virtual Addressing

[Yisheng Xie]

Hi Jean,

vfio can be compiled as module, however you use some functions which are not
exported.

comment inline:

[...]
> Add two new ioctl for VFIO containers. VFIO_IOMMU_BIND_PROCESS creates a
> bond between a container and a process address space, identified by a
> device-specific ID named PASID. This allows the device to target DMA
> transactions at the process virtual addresses without a need for mapping
> and unmapping buffers explicitly in the IOMMU. The process page tables are
> shared with the IOMMU, and mechanisms such as PCI ATS/PRI are used to
> handle faults. VFIO_IOMMU_UNBIND_PROCESS removes a bond created with
> VFIO_IOMMU_BIND_PROCESS.
>
> Signed-off-by: Jean-Philippe Brucker 
> ---
[...]
> +static struct mm_struct *vfio_iommu_get_mm_by_vpid(pid_t vpid)
> +{
> + struct mm_struct *mm;
> + struct task_struct *task;
> +
> + rcu_read_lock();
> + task = find_task_by_vpid(vpid);

Maybe can use?
task = pid_task(find_vpid(params.vpid), PIDTYPE_PID)

> + if (task)
> + get_task_struct(task);
> + rcu_read_unlock();
> + if (!task)
> + return ERR_PTR(-ESRCH);
> +
> + /* Ensure that current has RW access on the mm */
> + mm = mm_access(task, PTRACE_MODE_ATTACH_REALCREDS);

You will try to export mm_access, I find Felix have tried to, but seems give up:

 https://patchwork.kernel.org/patch/9744281/

Thanks
Yisheng

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

Re: [PATCH 03/37] iommu/sva: Manage process address spaces

[Yisheng Xie]

Hi Jean,

On 2018/2/22 14:23, Jean-Philippe Brucker wrote:
> @@ -129,7 +439,10 @@ int iommu_sva_device_shutdown(struct device *dev)
>  int iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, int 
> *pasid,
> unsigned long flags, void *drvdata)
>  {
> + int i, ret;
> + struct io_mm *io_mm = NULL;
>   struct iommu_domain *domain;
> + struct iommu_bond *bond = NULL, *tmp;
>   struct iommu_param *dev_param = dev->iommu_param;
>  
>   domain = iommu_get_domain_for_dev(dev);
> @@ -145,7 +458,42 @@ int iommu_sva_bind_device(struct device *dev, struct 
> mm_struct *mm, int *pasid,
>   if (flags != (IOMMU_SVA_FEAT_PASID | IOMMU_SVA_FEAT_IOPF))
>   return -EINVAL;
>  
> - return -ENOSYS; /* TODO */
> + /* If an io_mm already exists, use it */
> + spin_lock(_sva_lock);
> + idr_for_each_entry(_pasid_idr, io_mm, i) {
> + if (io_mm->mm != mm || !io_mm_get_locked(io_mm))
> + continue;
> +
> + /* Is it already bound to this device? */
> + list_for_each_entry(tmp, _mm->devices, mm_head) {
> + if (tmp->dev != dev)
> + continue;
> +
> + bond = tmp;
> + refcount_inc(>refs);
> + io_mm_put_locked(io_mm);

Should io_mm->pasid still be set to *pasid when the device already bond? so 
driver can
always get the right pasid if it bond to a mm multi-times, without keeping the 
pasid itself?

Thanks
Yisheng
> + break;
> + }
> + break;
> + }
> + spin_unlock(_sva_lock);
> +
> + if (bond)
> + return 0;
> +
> + if (!io_mm) {
> + io_mm = io_mm_alloc(domain, dev, mm);
> + if (IS_ERR(io_mm))
> + return PTR_ERR(io_mm);
> + }
> +
> + ret = io_mm_attach(domain, dev, io_mm, drvdata);
> + if (ret)
> + io_mm_put(io_mm);
> + else
> + *pasid = io_mm->pasid;
> +
> + return ret;
>  }
>  EXPORT_SYMBOL_GPL(iommu_sva_bind_device);

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

Re: [RFCv2 PATCH 26/36] iommu/arm-smmu-v3: Add support for Hardware Translation Table Update

[Yisheng Xie]

Hi Jean,

On 2017/10/6 21:31, Jean-Philippe Brucker wrote:
> If the SMMU supports it and the kernel was built with HTTU support, enable
> + if (IS_ENABLED(CONFIG_ARM64_HW_AFDBM) && (reg & (IDR0_HA | IDR0_HD))) {
> + smmu->features |= ARM_SMMU_FEAT_HA;
> + if (reg & IDR0_HD)
> + smmu->features |= ARM_SMMU_FEAT_HD;
> + }

What is relationship of armv8.1 HW_AFDBM and SMMUv3 HTTU? I mean why we need
IS_ENABLED(CONFIG_ARM64_HW_AFDBM) ?

If CONFIG_ARM64_HW_AFDBM=y but the process do not support ARMv8.1, should it 
also
enable related feature for SMMUv3?

Thanks
Yisheng Xie
> +
>   /*
>* If the CPU is using VHE, but the SMMU doesn't support it, the SMMU
>* will create TLB entries for NH-EL1 world and will miss the
> 

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

Re: [RFCv2 PATCH 09/36] iommu/fault: Allow blocking fault handlers

[Yisheng Xie]

hi jean,

On 2017/11/29 23:01, Jean-Philippe Brucker wrote:
> Hello,
> 
> On 29/11/17 06:15, Yisheng Xie wrote:
>> Hi Jean,
>>
>> On 2017/10/6 21:31, Jean-Philippe Brucker wrote:
>>> -   if (domain->ext_handler) {
>>> +   if (domain->handler_flags & IOMMU_FAULT_HANDLER_ATOMIC) {
>>> +   fault->flags |= IOMMU_FAULT_ATOMIC;
>>
>> Why remove the condition of domain->ext_handler? should it be much better 
>> like:
>>   if ((domain->handler_flags & IOMMU_FAULT_HANDLER_ATOMIC) && 
>> domain->ext_handler)
>>
>> If domain->ext_handler is NULL, and (domain->handler_flags & 
>> IOMMU_FAULT_HANDLER_ATOMIC)
>> is true. It will oops, right?
> 
> I removed the check because ext_handler shouldn't be NULL if handler_flags
> has a bit set (as per iommu_set_ext_fault_handler). But you're right that
> this is fragile, and I overlooked the case where users could call
> set_ext_fault_handler to clear the fault handler.
> 
> (Note that this ext_handler will most likely be replaced by the fault
> infrastructure that Jacob is working on:
> https://patchwork.kernel.org/patch/10063385/ to which we should add the
> atomic/blocking flags)
> 

Get it, thanks for your explanation.

Thanks
Yisheng Xie

> Thanks,
> Jean
> 
> .
> 

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

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

[Yisheng Xie]

Hi, Jean,

On 2017/11/29 23:01, Jean-Philippe Brucker wrote:
> On 29/11/17 06:08, Yisheng Xie wrote:
>>
>>
>> On 2017/10/6 21:31, Jean-Philippe Brucker wrote:
>>> +int iommu_process_bind_device(struct device *dev, struct task_struct *task,
>>> + int *pasid, int flags)
>>> +{
>> [..]
>>> +   err = iommu_process_attach_locked(context, dev);
>>> +   if (err)
>>> +   iommu_process_put_locked(process);
>> one ref for a context is enough right? so also need call 
>> iommu_process_put_locked()
>> if attach ok, or will be leak if user call bind twice for the same device 
>> and task.
> 
> I wasn't sure, I think I prefer taking one ref for each bind. If user
> calls bind twice, it should call unbind twice as well (in case of leak we
> free the context on process exit).
> 
> Also with this implementation, user can call bind for two devices in the
> same domain, which will share the same context structure. So we have to
> take as many refs as bind() calls.


hmm, it has two ref, one for _context_ and the other for *process* (or maybe mm 
for
your next version), right? For each bind it will take a ref of context as 
present
design. but why also process ref need be taken for each bind? I mean it seems 
does
not break _user can call bind for two devices in the same domain_.

And if you really want to take a ref of *process* for echo bind, you should put 
it when
unbind, right? I just not find where you put the ref of process when unbind. 
But just put
the process ref when free context.

Maybe I just miss something.

Thanks
Yisheng Xie
> 
> Thanks,
> Jean
> 
> .
> 

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

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

[Yisheng Xie]

On 2017/10/6 21:31, Jean-Philippe Brucker wrote:
> +int iommu_process_bind_device(struct device *dev, struct task_struct *task,
> +   int *pasid, int flags)
> +{
[..]
> + err = iommu_process_attach_locked(context, dev);
> + if (err)
> + iommu_process_put_locked(process);
one ref for a context is enough right? so also need call 
iommu_process_put_locked()
if attach ok, or will be leak if user call bind twice for the same device and 
task.

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

Re: [RFCv2 PATCH 09/36] iommu/fault: Allow blocking fault handlers

[Yisheng Xie]

Hi Jean,

On 2017/10/6 21:31, Jean-Philippe Brucker wrote:
> - if (domain->ext_handler) {
> + if (domain->handler_flags & IOMMU_FAULT_HANDLER_ATOMIC) {
> + fault->flags |= IOMMU_FAULT_ATOMIC;

Why remove the condition of domain->ext_handler? should it be much better like:
  if ((domain->handler_flags & IOMMU_FAULT_HANDLER_ATOMIC) && 
domain->ext_handler)

If domain->ext_handler is NULL, and (domain->handler_flags & 
IOMMU_FAULT_HANDLER_ATOMIC)
is true. It will oops, right?

>   ret = domain->ext_handler(domain, dev, fault,
>             domain->handler_token);

Thanks
Yisheng Xie

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

Re: [RFCv2 PATCH 21/36] iommu/arm-smmu-v3: Implement process operations

[Yisheng Xie]

Hi Jean,

On 2017/10/6 21:31, Jean-Philippe Brucker wrote:
> Hook process operations to support PASID and page table sharing with the
> SMMUv3:
> 
> +
> +static void arm_smmu_process_exit(struct iommu_domain *domain,
> +   struct iommu_process *process)
> +{
> + struct arm_smmu_master_data *master;
> + struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> +
> + if (!domain->process_exit)
> + return;

If domain do not set process_exit handler, just return? smmu do not
need invalid ATC, clear cd entry, etc.? Maybe you should check when
call domain->process_exit?

> +
> + spin_lock(_domain->devices_lock);
> + list_for_each_entry(master, _domain->devices, list) {
> + if (!master->processes)
> + continue;
> +
> + master->processes--;
Add
if (domain->process_exit)
here?
> + domain->process_exit(domain, master->dev, process->pasid,
> +  domain->process_exit_token);
> +
> + /* TODO: inval ATC */
> + }
> + spin_unlock(_domain->devices_lock);
> +
> + arm_smmu_write_ctx_desc(smmu_domain, process->pasid, NULL);
> +
> + /* TODO: Invalidate all mappings if not DVM */
> +}
> +
Thanks
Yisheng Xie

___
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

[Yisheng Xie]

On 2017/11/3 17:37, Jean-Philippe Brucker wrote:
> On 03/11/17 05:45, Yisheng Xie wrote:
>> 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 <shameerali.kolothum.th...@huawei.com>
>>>> 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 <mark.rutl...@arm.com>; xieyisheng (A)
>>>> <xieyishe...@huawei.com>; Gabriele Paoloni
>>>> <gabriele.paol...@huawei.com>; Catalin Marinas
>>>> <catalin.mari...@arm.com>; Will Deacon <will.dea...@arm.com>;
>>>> ok...@codeaurora.org; yi.l@intel.com; Lorenzo Pieralisi
>>>> <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) <thunder.leiz...@huawei.com>;
>>>> bhelg...@google.com; dw...@infradead.org; liubo (CU)
>>>> <liub...@huawei.com>; r...@rjwysocki.net; robdcl...@gmail.com;
>>>> hanjun@linaro.org; Sudeep Holla <sudeep.ho...@arm.com>; Robin
>>>> Murphy <robin.mur...@arm.com>; nwatt...@codeaurora.org; Linuxarm
>>>> <linux...@huawei.com>
>>>> 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/i

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

[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/arm-smmu-v3: Avoid ILLEGAL setting of STE.S1STALLD and CD.S

[Yisheng Xie]

Hi Will,

On 2017/10/20 16:36, Will Deacon wrote:
> On Fri, Oct 20, 2017 at 03:00:01PM +0800, Yisheng Xie wrote:
>> Any comment about this version?
> 
> I have it queued up and plan to send a pull request to Joerg today for 4.15.

Fine, thanks.

Thanks
Yisheng Xie
> 
> Will
> 
> .
> 

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

Re: [PATCH v2] iommu/arm-smmu-v3: Avoid ILLEGAL setting of STE.S1STALLD and CD.S

[Yisheng Xie]

Hi Will & Jean,

Any comment about this version?

Thanks
Yisheng Xie

On 2017/9/21 20:36, Yisheng Xie wrote:
> According to Spec, it is ILLEGAL to set STE.S1STALLD if STALL_MODEL
> is not 0b00, which means we should not disable stall mode if stall
> or terminate mode is not configuable.
> 
> Meanwhile, it is also ILLEGAL when STALL_MODEL==0b10 && CD.S==0 which
> means if stall mode is force we should always set CD.S.
> 
> As Jean-Philippe's suggestion, this patch introduce a feature bit
> ARM_SMMU_FEAT_STALL_FORCE, which means smmu only supports stall force.
> Therefore, we can avoid the ILLEGAL setting of STE.S1STALLD.by checking
> ARM_SMMU_FEAT_STALL_FORCE.
> 
> This patch keeps the ARM_SMMU_FEAT_STALLS as the meaning of stall supported
> (force or configuable) to easy to expand the future function, i.e. we can
> only use ARM_SMMU_FEAT_STALLS to check whether we should register fault
> handle or enable master can_stall, etc to supporte platform SVM.
> 
> The feature bit, STE.S1STALLD and CD.S setting will be like:
> 
> STALL_MODEL  FEATURE S1STALLD CD.S
> 0b00 ARM_SMMU_FEAT_STALLS 0b1 0b0
> 0b01 !ARM_SMMU_FEAT_STALLS && !ARM_SMMU_FEAT_STALL_FORCE  0b0 0b0
> 0b10 ARM_SMMU_FEAT_STALLS && ARM_SMMU_FEAT_STALL_FORCE0b0 0b1
> 
> after apply this patch.
> 
> Signed-off-by: Yisheng Xie <xieyishe...@huawei.com>
> ---
> v2:
>  * Keep the feature bits backward compatible and add new one at the end
>  * Avoid ILLEGAL of CD.S - both as Jean's suggestion.
> 
>  drivers/iommu/arm-smmu-v3.c | 15 ---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index e67ba6c..22a6b08 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -316,6 +316,7 @@
>  #define ARM64_TCR_TBI0_MASK  0x1UL
>  
>  #define CTXDESC_CD_0_AA64(1UL << 41)
> +#define CTXDESC_CD_0_S   (1UL << 44)
>  #define CTXDESC_CD_0_R   (1UL << 45)
>  #define CTXDESC_CD_0_A   (1UL << 46)
>  #define CTXDESC_CD_0_ASET_SHIFT  47
> @@ -604,6 +605,7 @@ struct arm_smmu_device {
>  #define ARM_SMMU_FEAT_TRANS_S2   (1 << 10)
>  #define ARM_SMMU_FEAT_STALLS (1 << 11)
>  #define ARM_SMMU_FEAT_HYP(1 << 12)
> +#define ARM_SMMU_FEAT_STALL_FORCE(1 << 13)
>   u32 features;
>  
>  #define ARM_SMMU_OPT_SKIP_PREFETCH   (1 << 0)
> @@ -996,6 +998,11 @@ static void arm_smmu_write_ctx_desc(struct 
> arm_smmu_device *smmu,
> CTXDESC_CD_0_R | CTXDESC_CD_0_A | CTXDESC_CD_0_ASET_PRIVATE |
> CTXDESC_CD_0_AA64 | (u64)cfg->cd.asid << CTXDESC_CD_0_ASID_SHIFT |
> CTXDESC_CD_0_V;
> +
> + /* STALL_MODEL==0b10 && CD.S==0 is ILLEGAL */
> + if (smmu->features & ARM_SMMU_FEAT_STALL_FORCE)
> + val |= CTXDESC_CD_0_S;
> +
>   cfg->cdptr[0] = cpu_to_le64(val);
>  
>   val = cfg->cd.ttbr & CTXDESC_CD_1_TTB0_MASK << CTXDESC_CD_1_TTB0_SHIFT;
> @@ -1112,7 +1119,8 @@ static void arm_smmu_write_strtab_ent(struct 
> arm_smmu_device *smmu, u32 sid,
>  #endif
>STRTAB_STE_1_STRW_NSEL1 << STRTAB_STE_1_STRW_SHIFT);
>  
> - if (smmu->features & ARM_SMMU_FEAT_STALLS)
> + if (smmu->features & ARM_SMMU_FEAT_STALLS &&
> +!(smmu->features & ARM_SMMU_FEAT_STALL_FORCE))
>   dst[1] |= cpu_to_le64(STRTAB_STE_1_S1STALLD);
>  
>   val |= (ste->s1_cfg->cdptr_dma & STRTAB_STE_0_S1CTXPTR_MASK
> @@ -2536,9 +2544,10 @@ static int arm_smmu_device_hw_probe(struct 
> arm_smmu_device *smmu)
>coherent ? "true" : "false");
>  
>   switch (reg & IDR0_STALL_MODEL_MASK << IDR0_STALL_MODEL_SHIFT) {
> - case IDR0_STALL_MODEL_STALL:
> - /* Fallthrough */
>   case IDR0_STALL_MODEL_FORCE:
> + smmu->features |= ARM_SMMU_FEAT_STALL_FORCE;
> + /* Fallthrough */
> + case IDR0_STALL_MODEL_STALL:
>   smmu->features |= ARM_SMMU_FEAT_STALLS;
>   }
>  
> 

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

Re: [RFCv2 PATCH 00/36] Process management for IOMMU + SVM for SMMUv3

[Yisheng Xie]

Hi Jean,

Thanks for replying.
On 2017/10/9 19:36, Jean-Philippe Brucker wrote:
> Hi,
> 
> On 09/10/17 10:49, Yisheng Xie wrote:
>> Hi Jean,
>>
>> On 2017/10/6 21:31, Jean-Philippe Brucker wrote:
>>> Following discussions at plumbers and elsewhere, it seems like we need to
>>> unify some of the Shared Virtual Memory (SVM) code, in order to define
>>> clear semantics for the SVM API.
>>>
>>> My previous RFC [1] was centered on the SMMUv3, but some of this code will
>>> need to be reused by the SMMUv2 and virtio-iommu drivers. This second
>>> proposal focuses on abstracting a little more into the core IOMMU API, and
>>> also trying to find common ground for all SVM-capable IOMMUs.
>>>
>>> SVM is, in the context of the IOMMU, sharing page tables between a process
>>> and a device. Traditionally it requires IO Page Fault and Process Address
>>> Space ID capabilities in device and IOMMU.
>>>
>>> * A device driver can bind a process to a device, with iommu_process_bind.
>>>   Internally we hold on to the mm and get notified of its activity with an
>>>   mmu_notifier. The bond is removed by exit_mm, by a call to
>>>   iommu_process_unbind or iommu_detach_device.
>>>
>>> * iommu_process_bind returns a 20-bit PASID (PCI terminology) to the
>>>   device driver, which programs it into the device to access the process
>>>   address space.
>>>
>>> * The device and the IOMMU support recoverable page faults. This can be
>>>   either ATS+PRI for PCI, or platform-specific mechanisms such as Stall
>>>   for SMMU.
>>>
>>> Ideally systems wanting to use SVM have to support these three features,
>>> but in practice we'll see implementations supporting just a subset of
>>> them, especially in validation environments. So even if this particular
>>> patchset assumes all three capabilities, it should also be possible to
>>> support PASID without IOPF (by pinning everything, see non-system SVM in
>>> OpenCL)
>> How to pin everything? If user malloc anything we should pin it. It should
>> from user or driver?
> 
> For userspace drivers, I guess it would be via a VFIO ioctl, that does the
> same preparatory work as VFIO_IOMMU_MAP_DMA, but doesn't call iommu_map.
> For things like OpenCL SVM buffers, it's the kernel driver that does the
> pinning, just like VFIO does it, before launching the work on a SVM buffer.
> 
>>> , or IOPF without PASID (sharing the single device address space
>>> with a process, could be useful for DPDK+VFIO).
>>>
>>> Implementing both these cases would enable PT sharing alone. Some people
>>> would also like IOPF alone without SVM (covered by this series) or process
>>> management without shared PT (not covered). Using these features
>>> individually is also important for testing, as SVM is in its infancy and
>>> providing easy ways to test is essential to reduce the number of quirks
>>> down the line.
>>>
>>>   Process management
>>>   ==
>>>
>>> The first part of this series introduces boilerplate code for managing
>>> PASIDs and processes bound to devices. It's something any IOMMU driver
>>> that wants to support bind/unbind will have to do, and it is difficult to
>>> get right.
>>>
>>> Patches
>>> 1: iommu_process and PASID allocation, attach and release
>>> 2: process_exit callback for device drivers
>>> 3: iommu_process search by PASID
>>> 4: track process changes with an MMU notifiers
>>> 5: bind and unbind operations
>>>
>>> My proposal uses the following model:
>>>
>>> * The PASID space is system-wide. This means that a Linux process will
>>>   have a single PASID. I introduce the iommu_process structure and a
>>>   global IDR to manage this.
>>>
>>> * An iommu_process can be bound to multiple domains, and a domain can have
>>>   multiple iommu_process.
>> when bind a task to device, can we create a single domain for it? I am 
>> thinking
>> about process management without shared PT(for some device only support PASID
>> without pri ability), it seems hard to expand if a domain have multiple 
>> iommu_process?
>> Do you have any idea about this?
> 
> A device always has to be in a domain, as far as I know. Not supporting
> PRI forces you to pin down all user mappings (or just the ones you use for
> DMA) but you should sill be able to share PT. Now if you don't support
> shared PT either, but only PASID,

Re: [RFCv2 PATCH 00/36] Process management for IOMMU + SVM for SMMUv3

[Yisheng Xie]

Hi Jean,

On 2017/10/6 21:31, Jean-Philippe Brucker wrote:
> Following discussions at plumbers and elsewhere, it seems like we need to
> unify some of the Shared Virtual Memory (SVM) code, in order to define
> clear semantics for the SVM API.
> 
> My previous RFC [1] was centered on the SMMUv3, but some of this code will
> need to be reused by the SMMUv2 and virtio-iommu drivers. This second
> proposal focuses on abstracting a little more into the core IOMMU API, and
> also trying to find common ground for all SVM-capable IOMMUs.
> 
> SVM is, in the context of the IOMMU, sharing page tables between a process
> and a device. Traditionally it requires IO Page Fault and Process Address
> Space ID capabilities in device and IOMMU.
> 
> * A device driver can bind a process to a device, with iommu_process_bind.
>   Internally we hold on to the mm and get notified of its activity with an
>   mmu_notifier. The bond is removed by exit_mm, by a call to
>   iommu_process_unbind or iommu_detach_device.
> 
> * iommu_process_bind returns a 20-bit PASID (PCI terminology) to the
>   device driver, which programs it into the device to access the process
>   address space.
> 
> * The device and the IOMMU support recoverable page faults. This can be
>   either ATS+PRI for PCI, or platform-specific mechanisms such as Stall
>   for SMMU.
> 
> Ideally systems wanting to use SVM have to support these three features,
> but in practice we'll see implementations supporting just a subset of
> them, especially in validation environments. So even if this particular
> patchset assumes all three capabilities, it should also be possible to
> support PASID without IOPF (by pinning everything, see non-system SVM in
> OpenCL)
How to pin everything? If user malloc anything we should pin it. It should
from user or driver?

> , or IOPF without PASID (sharing the single device address space
> with a process, could be useful for DPDK+VFIO).
> 
> Implementing both these cases would enable PT sharing alone. Some people
> would also like IOPF alone without SVM (covered by this series) or process
> management without shared PT (not covered). Using these features
> individually is also important for testing, as SVM is in its infancy and
> providing easy ways to test is essential to reduce the number of quirks
> down the line.
> 
>   Process management
>   ==
> 
> The first part of this series introduces boilerplate code for managing
> PASIDs and processes bound to devices. It's something any IOMMU driver
> that wants to support bind/unbind will have to do, and it is difficult to
> get right.
> 
> Patches
> 1: iommu_process and PASID allocation, attach and release
> 2: process_exit callback for device drivers
> 3: iommu_process search by PASID
> 4: track process changes with an MMU notifiers
> 5: bind and unbind operations
> 
> My proposal uses the following model:
> 
> * The PASID space is system-wide. This means that a Linux process will
>   have a single PASID. I introduce the iommu_process structure and a
>   global IDR to manage this.
> 
> * An iommu_process can be bound to multiple domains, and a domain can have
>   multiple iommu_process.
when bind a task to device, can we create a single domain for it? I am thinking
about process management without shared PT(for some device only support PASID
without pri ability), it seems hard to expand if a domain have multiple 
iommu_process?
Do you have any idea about this?

> 
> * IOMMU groups share same PASID table. IOMMU groups are a convenient way
>   to cover various hardware weaknesses that prevent a group of device to
>   be isolated by the IOMMU (untrusted bridge, for instance). It's foolish
>   to assume that all PASID implementations will perfectly isolate devices
>   within a bus and functions within a device, so let's assume all devices
>   within an IOMMU group have to share PASID traffic as well. In general
>   there will be a single device per group.
> 
> * It's up to the driver implementation to decide where to implement the
>   PASID tables. For SMMU it's more convenient to have a single PASID table
>   per domain. And I think the model fits better with the existing IOMMU
>   API: IOVA traffic is shared by all devices in a domain, so should PASID
>   traffic.
What's the meaning of "share PASID traffic"? PASID space is system-wide,
and a domain can have multiple iommu_process , so a domain can have multiple
PASIDs , one PASID for a iommu_process, right?

Yisheng Xie
Thanks


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

[PATCH v2] iommu/arm-smmu-v3: Avoid ILLEGAL setting of STE.S1STALLD and CD.S

[Yisheng Xie]

According to Spec, it is ILLEGAL to set STE.S1STALLD if STALL_MODEL
is not 0b00, which means we should not disable stall mode if stall
or terminate mode is not configuable.

Meanwhile, it is also ILLEGAL when STALL_MODEL==0b10 && CD.S==0 which
means if stall mode is force we should always set CD.S.

As Jean-Philippe's suggestion, this patch introduce a feature bit
ARM_SMMU_FEAT_STALL_FORCE, which means smmu only supports stall force.
Therefore, we can avoid the ILLEGAL setting of STE.S1STALLD.by checking
ARM_SMMU_FEAT_STALL_FORCE.

This patch keeps the ARM_SMMU_FEAT_STALLS as the meaning of stall supported
(force or configuable) to easy to expand the future function, i.e. we can
only use ARM_SMMU_FEAT_STALLS to check whether we should register fault
handle or enable master can_stall, etc to supporte platform SVM.

The feature bit, STE.S1STALLD and CD.S setting will be like:

STALL_MODEL  FEATURE S1STALLD CD.S
0b00 ARM_SMMU_FEAT_STALLS 0b1 0b0
0b01 !ARM_SMMU_FEAT_STALLS && !ARM_SMMU_FEAT_STALL_FORCE  0b0 0b0
0b10 ARM_SMMU_FEAT_STALLS && ARM_SMMU_FEAT_STALL_FORCE0b0 0b1

after apply this patch.

Signed-off-by: Yisheng Xie <xieyishe...@huawei.com>
---
v2:
 * Keep the feature bits backward compatible and add new one at the end
 * Avoid ILLEGAL of CD.S - both as Jean's suggestion.

 drivers/iommu/arm-smmu-v3.c | 15 ---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index e67ba6c..22a6b08 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -316,6 +316,7 @@
 #define ARM64_TCR_TBI0_MASK0x1UL
 
 #define CTXDESC_CD_0_AA64  (1UL << 41)
+#define CTXDESC_CD_0_S (1UL << 44)
 #define CTXDESC_CD_0_R (1UL << 45)
 #define CTXDESC_CD_0_A (1UL << 46)
 #define CTXDESC_CD_0_ASET_SHIFT47
@@ -604,6 +605,7 @@ struct arm_smmu_device {
 #define ARM_SMMU_FEAT_TRANS_S2 (1 << 10)
 #define ARM_SMMU_FEAT_STALLS   (1 << 11)
 #define ARM_SMMU_FEAT_HYP  (1 << 12)
+#define ARM_SMMU_FEAT_STALL_FORCE  (1 << 13)
u32 features;
 
 #define ARM_SMMU_OPT_SKIP_PREFETCH (1 << 0)
@@ -996,6 +998,11 @@ static void arm_smmu_write_ctx_desc(struct arm_smmu_device 
*smmu,
  CTXDESC_CD_0_R | CTXDESC_CD_0_A | CTXDESC_CD_0_ASET_PRIVATE |
  CTXDESC_CD_0_AA64 | (u64)cfg->cd.asid << CTXDESC_CD_0_ASID_SHIFT |
  CTXDESC_CD_0_V;
+
+   /* STALL_MODEL==0b10 && CD.S==0 is ILLEGAL */
+   if (smmu->features & ARM_SMMU_FEAT_STALL_FORCE)
+   val |= CTXDESC_CD_0_S;
+
cfg->cdptr[0] = cpu_to_le64(val);
 
val = cfg->cd.ttbr & CTXDESC_CD_1_TTB0_MASK << CTXDESC_CD_1_TTB0_SHIFT;
@@ -1112,7 +1119,8 @@ static void arm_smmu_write_strtab_ent(struct 
arm_smmu_device *smmu, u32 sid,
 #endif
 STRTAB_STE_1_STRW_NSEL1 << STRTAB_STE_1_STRW_SHIFT);
 
-   if (smmu->features & ARM_SMMU_FEAT_STALLS)
+   if (smmu->features & ARM_SMMU_FEAT_STALLS &&
+  !(smmu->features & ARM_SMMU_FEAT_STALL_FORCE))
dst[1] |= cpu_to_le64(STRTAB_STE_1_S1STALLD);
 
val |= (ste->s1_cfg->cdptr_dma & STRTAB_STE_0_S1CTXPTR_MASK
@@ -2536,9 +2544,10 @@ static int arm_smmu_device_hw_probe(struct 
arm_smmu_device *smmu)
 coherent ? "true" : "false");
 
switch (reg & IDR0_STALL_MODEL_MASK << IDR0_STALL_MODEL_SHIFT) {
-   case IDR0_STALL_MODEL_STALL:
-   /* Fallthrough */
case IDR0_STALL_MODEL_FORCE:
+   smmu->features |= ARM_SMMU_FEAT_STALL_FORCE;
+   /* Fallthrough */
+   case IDR0_STALL_MODEL_STALL:
smmu->features |= ARM_SMMU_FEAT_STALLS;
}
 
-- 
1.7.12.4

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

Re: [PATCH] iommu/arm-smmu-v3: Avoid ILLEGAL setting of STE.S1STALLD

[Yisheng Xie]

Hi Jean-Philippe,

Thanks for reviewing.
On 2017/9/19 19:43, Jean-Philippe Brucker wrote:
> On 14/09/17 06:08, Yisheng Xie wrote:
>> According to Spec, it is ILLEGAL to set STE.S1STALLD if STALL_MODEL
>> is not 0b00, which means we should not disable stall mode if stall
>> or terminate mode is not configuable.
>>
>> As Jean-Philippe's suggestion, this patch introduce a feature bit
>> ARM_SMMU_FEAT_STALL_FORCE, which means smmu only supports stall force.
>> Therefore, we can avoid the ILLEGAL setting of STE.S1STALLD.by checking
>> ARM_SMMU_FEAT_STALL_FORCE.
>>
>> This patch keeps the ARM_SMMU_FEAT_STALLS as the meaning of stall supported
>> (force or configuable) to easy to expand the future function, i.e. we can
>> only use ARM_SMMU_FEAT_STALLS to check whether we should register fault
>> handle or enable master can_stall, etc to supporte platform SVM.
>>
>> After apply this patch, the feature bit and S1STALLD setting will be like:
>> STALL_MODEL  FEATURE  S1STALLD
>> 0b00 ARM_SMMU_FEAT_STALLS 0b1
>> 0b01 !ARM_SMMU_FEAT_STALLS && !ARM_SMMU_FEAT_STALL_FORCE  0b0
>> 0b10 ARM_SMMU_FEAT_STALLS && ARM_SMMU_FEAT_STALL_FORCE0b0
> 
> Thanks for the patch. Since it's the same problem, could you also fix the
> context descriptor value? The spec says, in 5.5 Fault configuration:
> 
> "A CD (Stage 1 translation enabled) is considered ILLEGAL if one of the
> following applies:
> * SMMU_(S_)IDR0.STALL_MODEL == 0b10 and CD.S == 0."
Sure, I will fix this it in next version.

> 
> So I think we should always set CD.S if the SMMU has STALL_FORCE.
> 
> As Will pointed out, more work is needed for STALL_FORCE. We can't enable
> translation at all for devices that don't support stalling (e.g. PCI). We
> should force them into bypass or abort mode depending on the config. Maybe
> we can fix that later, after the devicetree property is added.
It is fare.

> 
>> Signed-off-by: Yisheng Xie <xieyishe...@huawei.com>
>> ---
>>  drivers/iommu/arm-smmu-v3.c | 11 +++
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index e67ba6c..d2a3627 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -603,7 +603,8 @@ struct arm_smmu_device {
>>  #define ARM_SMMU_FEAT_TRANS_S1  (1 << 9)
>>  #define ARM_SMMU_FEAT_TRANS_S2  (1 << 10)
>>  #define ARM_SMMU_FEAT_STALLS(1 << 11)
>> -#define ARM_SMMU_FEAT_HYP   (1 << 12)
>> +#define ARM_SMMU_FEAT_STALL_FORCE   (1 << 12)
>> +#define ARM_SMMU_FEAT_HYP       (1 << 13)
> 
> We probably should keep the feature bits backward compatible and only add
> new ones at the end. It's not ABI, but it's printed at boot time and I
> sometimes use them when inspecting the kernel output to see what an SMMU
> supports.
Take it, also will fixed it in next version.

Thanks
Yisheng Xie
> 
> Thanks,
> Jean
> 
>>  u32 features;
>>  
>>  #define ARM_SMMU_OPT_SKIP_PREFETCH  (1 << 0)
>> @@ -1112,7 +1113,8 @@ static void arm_smmu_write_strtab_ent(struct 
>> arm_smmu_device *smmu, u32 sid,
>>  #endif
>>   STRTAB_STE_1_STRW_NSEL1 << STRTAB_STE_1_STRW_SHIFT);
>>  
>> -if (smmu->features & ARM_SMMU_FEAT_STALLS)
>> +if (smmu->features & ARM_SMMU_FEAT_STALLS &&
>> +   !(smmu->features & ARM_SMMU_FEAT_STALL_FORCE))
>>  dst[1] |= cpu_to_le64(STRTAB_STE_1_S1STALLD);
>>  
>>  val |= (ste->s1_cfg->cdptr_dma & STRTAB_STE_0_S1CTXPTR_MASK
>> @@ -2536,9 +2538,10 @@ static int arm_smmu_device_hw_probe(struct 
>> arm_smmu_device *smmu)
>>   coherent ? "true" : "false");
>>  
>>  switch (reg & IDR0_STALL_MODEL_MASK << IDR0_STALL_MODEL_SHIFT) {
>> -case IDR0_STALL_MODEL_STALL:
>> -/* Fallthrough */
>>  case IDR0_STALL_MODEL_FORCE:
>> +smmu->features |= ARM_SMMU_FEAT_STALL_FORCE;
>> +/* Fallthrough */
>> +case IDR0_STALL_MODEL_STALL:
>>  smmu->features |= ARM_SMMU_FEAT_STALLS;
>>  }
>>  
>>
> 
> 
> .
> 

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

[PATCH] iommu/arm-smmu-v3: Avoid ILLEGAL setting of STE.S1STALLD

[Yisheng Xie]

According to Spec, it is ILLEGAL to set STE.S1STALLD if STALL_MODEL
is not 0b00, which means we should not disable stall mode if stall
or terminate mode is not configuable.

As Jean-Philippe's suggestion, this patch introduce a feature bit
ARM_SMMU_FEAT_STALL_FORCE, which means smmu only supports stall force.
Therefore, we can avoid the ILLEGAL setting of STE.S1STALLD.by checking
ARM_SMMU_FEAT_STALL_FORCE.

This patch keeps the ARM_SMMU_FEAT_STALLS as the meaning of stall supported
(force or configuable) to easy to expand the future function, i.e. we can
only use ARM_SMMU_FEAT_STALLS to check whether we should register fault
handle or enable master can_stall, etc to supporte platform SVM.

After apply this patch, the feature bit and S1STALLD setting will be like:
STALL_MODEL  FEATURE  S1STALLD
0b00 ARM_SMMU_FEAT_STALLS 0b1
0b01 !ARM_SMMU_FEAT_STALLS && !ARM_SMMU_FEAT_STALL_FORCE  0b0
0b10 ARM_SMMU_FEAT_STALLS && ARM_SMMU_FEAT_STALL_FORCE0b0

Signed-off-by: Yisheng Xie <xieyishe...@huawei.com>
---
 drivers/iommu/arm-smmu-v3.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index e67ba6c..d2a3627 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -603,7 +603,8 @@ struct arm_smmu_device {
 #define ARM_SMMU_FEAT_TRANS_S1 (1 << 9)
 #define ARM_SMMU_FEAT_TRANS_S2 (1 << 10)
 #define ARM_SMMU_FEAT_STALLS   (1 << 11)
-#define ARM_SMMU_FEAT_HYP  (1 << 12)
+#define ARM_SMMU_FEAT_STALL_FORCE  (1 << 12)
+#define ARM_SMMU_FEAT_HYP  (1 << 13)
u32 features;
 
 #define ARM_SMMU_OPT_SKIP_PREFETCH (1 << 0)
@@ -1112,7 +1113,8 @@ static void arm_smmu_write_strtab_ent(struct 
arm_smmu_device *smmu, u32 sid,
 #endif
 STRTAB_STE_1_STRW_NSEL1 << STRTAB_STE_1_STRW_SHIFT);
 
-   if (smmu->features & ARM_SMMU_FEAT_STALLS)
+   if (smmu->features & ARM_SMMU_FEAT_STALLS &&
+  !(smmu->features & ARM_SMMU_FEAT_STALL_FORCE))
dst[1] |= cpu_to_le64(STRTAB_STE_1_S1STALLD);
 
val |= (ste->s1_cfg->cdptr_dma & STRTAB_STE_0_S1CTXPTR_MASK
@@ -2536,9 +2538,10 @@ static int arm_smmu_device_hw_probe(struct 
arm_smmu_device *smmu)
 coherent ? "true" : "false");
 
switch (reg & IDR0_STALL_MODEL_MASK << IDR0_STALL_MODEL_SHIFT) {
-   case IDR0_STALL_MODEL_STALL:
-   /* Fallthrough */
case IDR0_STALL_MODEL_FORCE:
+   smmu->features |= ARM_SMMU_FEAT_STALL_FORCE;
+   /* Fallthrough */
+   case IDR0_STALL_MODEL_STALL:
smmu->features |= ARM_SMMU_FEAT_STALLS;
}
 
-- 
1.7.12.4

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

Re: [RFC PATCH 6/6] iommu/arm-smmu-v3: Avoid ILLEGAL setting of STE.S1STALLD and CD.S

[Yisheng Xie]

Hi Will,

On 2017/9/13 11:06, Will Deacon wrote:
> On Tue, Sep 05, 2017 at 01:54:19PM +0100, Jean-Philippe Brucker wrote:
>> On 31/08/17 09:20, Yisheng Xie wrote:
>>> It is ILLEGAL to set STE.S1STALLD if STALL_MODEL is not 0b00, which
>>> means we should not disable stall mode if stall/terminate mode is not
>>> configuable.
>>>
>>> Meanwhile, it is also ILLEGAL when STALL_MODEL==0b10 && CD.S==0 which
>>> means if stall mode is force we should always set CD.S.
>>>
>>> This patch add ARM_SMMU_FEAT_TERMINATE feature bit for smmu, and use
>>> TERMINATE feature checking to ensue above ILLEGAL cases from happening.
>>>
>>> Signed-off-by: Yisheng Xie <xieyishe...@huawei.com>
>>> ---
>>>  drivers/iommu/arm-smmu-v3.c | 22 --
>>>  1 file changed, 16 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>>> index dbda2eb..0745522 100644
>>> --- a/drivers/iommu/arm-smmu-v3.c
>>> +++ b/drivers/iommu/arm-smmu-v3.c
>>> @@ -55,6 +55,7 @@
>>>  #define IDR0_STALL_MODEL_SHIFT 24
>>>  #define IDR0_STALL_MODEL_MASK  0x3
>>>  #define IDR0_STALL_MODEL_STALL (0 << IDR0_STALL_MODEL_SHIFT)
>>> +#define IDR0_STALL_MODEL_NS(1 << IDR0_STALL_MODEL_SHIFT)
>>>  #define IDR0_STALL_MODEL_FORCE (2 << IDR0_STALL_MODEL_SHIFT)
>>>  #define IDR0_TTENDIAN_SHIFT21
>>>  #define IDR0_TTENDIAN_MASK 0x3
>>> @@ -766,6 +767,7 @@ struct arm_smmu_device {
>>>  #define ARM_SMMU_FEAT_SVM  (1 << 15)
>>>  #define ARM_SMMU_FEAT_HA   (1 << 16)
>>>  #define ARM_SMMU_FEAT_HD   (1 << 17)
>>> +#define ARM_SMMU_FEAT_TERMINATE(1 << 18)
>>
>> I'd rather introduce something like "ARM_SMMU_FEAT_STALL_FORCE" instead.
>> Terminate model has another meaning, and is defined by a different bit in
>> IDR0.
> 
> Yes. What we need to do is:
> 
> - If STALL_MODEL is 0b00, then set S1STALLD

Yes, and within this case, we can only set the S1STALLD for masters which can
not stall in the future?

> - If STALL_MODEL is 0b01, then we're ok (in future, avoiding trying to use
>   stalls, even for masters that claim to support it)
> - If STALL_MODEL is 0b10, then force all PCI devices and any platform
>   devices that don't claim to support stalls into bypass (depending on
>   disable_bypass).
> 
> Reasonable? We could actually knock up a fix for mainline to do most of
> this already.
This sound reasonable to me. And I can be a volunteer to prepare this patch if
Jean-Philippe do not oppose :)

Thanks
Yisheng Xie

> 
> Will
> 
> .
> 

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

Re: [RFC PATCH 6/6] iommu/arm-smmu-v3: Avoid ILLEGAL setting of STE.S1STALLD and CD.S

[Yisheng Xie]

Hi Jean-Philippe,

On 2017/9/5 20:54, Jean-Philippe Brucker wrote:
> On 31/08/17 09:20, Yisheng Xie wrote:
>> It is ILLEGAL to set STE.S1STALLD if STALL_MODEL is not 0b00, which
>> means we should not disable stall mode if stall/terminate mode is not
>> configuable.
>>
>> Meanwhile, it is also ILLEGAL when STALL_MODEL==0b10 && CD.S==0 which
>> means if stall mode is force we should always set CD.S.
>>
>> This patch add ARM_SMMU_FEAT_TERMINATE feature bit for smmu, and use
>> TERMINATE feature checking to ensue above ILLEGAL cases from happening.
>>
>> Signed-off-by: Yisheng Xie <xieyishe...@huawei.com>
>> ---
>>  drivers/iommu/arm-smmu-v3.c | 22 --
>>  1 file changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index dbda2eb..0745522 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -55,6 +55,7 @@
>>  #define IDR0_STALL_MODEL_SHIFT  24
>>  #define IDR0_STALL_MODEL_MASK   0x3
>>  #define IDR0_STALL_MODEL_STALL  (0 << IDR0_STALL_MODEL_SHIFT)
>> +#define IDR0_STALL_MODEL_NS (1 << IDR0_STALL_MODEL_SHIFT)
>>  #define IDR0_STALL_MODEL_FORCE  (2 << IDR0_STALL_MODEL_SHIFT)
>>  #define IDR0_TTENDIAN_SHIFT 21
>>  #define IDR0_TTENDIAN_MASK  0x3
>> @@ -766,6 +767,7 @@ struct arm_smmu_device {
>>  #define ARM_SMMU_FEAT_SVM   (1 << 15)
>>  #define ARM_SMMU_FEAT_HA(1 << 16)
>>  #define ARM_SMMU_FEAT_HD(1 << 17)
>> +#define ARM_SMMU_FEAT_TERMINATE (1 << 18)
> 
> I'd rather introduce something like "ARM_SMMU_FEAT_STALL_FORCE" instead.
> Terminate model has another meaning, and is defined by a different bit in
> IDR0.

Ok, sound more reasonable.

Thanks
Yisheng Xie

> 
> Thanks,
> Jean
> 
> .
> 

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

Re: [RFC PATCH 4/6] iommu/arm-smmu-v3: Add SVM support for platform devices

[Yisheng Xie]

Hi Jean-Philippe,

On 2017/9/6 8:51, Bob Liu wrote:
> On 2017/9/5 20:53, Jean-Philippe Brucker wrote:
>> On 31/08/17 09:20, Yisheng Xie wrote:
>>> From: Jean-Philippe Brucker <jean-philippe.bruc...@arm.com>
>>>
>>> Platform device can realise SVM function by using the stall mode. That
>>> is to say, when device access a memory via iova which is not populated,
>>> it will stalled and when SMMU try to translate this iova, it also will
>>> stall and meanwhile send an event to CPU via MSI.
>>>
>>> After SMMU driver handle the event and populated the iova, it will send
>>> a RESUME command to SMMU to exit the stall mode, therefore the platform
>>> device can contiue access the memory.
>>>
>>> Signed-off-by: Jean-Philippe Brucker <jean-philippe.bruc...@arm.com>
>>
>> No. Please don't forge a signed-off-by under a commit message you wrote,

Sorry about that, it is my mistake.

> 
> Really sorry for that.
> We sent out the wrong version, I should take more careful review.
> 
> Regards,
> Liubo
> 
>> it's rude. I didn't sign it, didn't consider it fit for mainline or even
>> as an RFC, and wanted to have another read before sending. My mistake,
>> I'll think twice before sharing prototypes in the future.
>>
>> Thanks,
>> Jean
>>
> 
> 
> 
> 
> .
> 

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

Re: [RFC PATCH 0/6] Add platform device SVM support for ARM SMMUv3

[Yisheng Xie]

Hi Jean-Philippe,

On 2017/9/5 20:56, Jean-Philippe Brucker wrote:
> On 31/08/17 09:20, Yisheng Xie wrote:
>> Jean-Philippe has post a patchset for Adding PCIe SVM support to ARM SMMUv3:
>> https://www.spinics.net/lists/arm-kernel/msg565155.html
>>
>> But for some platform devices(aka on-chip integrated devices), there is also
>> SVM requirement, which works based on the SMMU stall mode.
>> Jean-Philippe has prepared a prototype patchset to support it:
>> git://linux-arm.org/linux-jpb.git svm/stall
> 
> Only meant for testing at that point, and unfit even for an RFC.

Sorry about that, I should ask you before send it out. It's my mistake. For I 
also
have some question about this patchset.

We have related device, and would like to do some help about it. Do you have
any plan about upstream ?

> 
>> We tested this patchset with some fixes on a on-chip integrated device. The
>> basic function is ok, so I just send them out for review, although this
>> patchset heavily depends on the former patchset (PCIe SVM support for ARM
>> SMMUv3), which is still under discussion.
>>
>> Patch Overview:
>> *1 to 3 prepare for device tree or acpi get the device stall ability and 
>> pasid bits
>> *4 is to realise the SVM function for platform device
>> *5 is fix a bug when test SVM function while SMMU donnot support this feature
>> *6 avoid ILLEGAL setting of STE and CD entry about stall
>>
>> Acctually here, I also have some questions about SVM on SMMUv3:
>>
>> 1. Why the SVM feature on SMMUv3 depends on BTM feature? when bind a task to 
>> device,
>>it will register a mmu_notify. Therefore, when a page range is invalid, 
>> we can
>>send TLBI or ATC invalid without BTM?
> 
> We could, but the end goal for SVM is to perfectly mirror the CPU page
> tables. So for platform SVM we would like to get rid of MMU notifiers
> entirely.

I see, but for some SMMU which do not support BTM, it cannot benefit from SVM.

Meanwhile, do you mean even with BTM feature, the PCI-e device also need to 
send a
ATC invalid by MMU notify? It seems not fair, why not hardware do the entirely 
work
in this case? It may costly for send ATC invalid and sync.

> 
>> 2. According to ACPI IORT spec, named component specific data has a node 
>> flags field
>>whoes bit0 is for Stall support. However, it do not have any field for 
>> pasid bit.
>>Can we use other 5 bits[5:1] for pasid bit numbers, so we can have 32 
>> pasid bit for
>>a single platform device which should be enough, because SMMU only 
>> support 20 bit pasid
>>
>> 3. Presently, the pasid is allocate for a task but not for a context, if a 
>> task is trying
>>to bind to 2 device A and B:
>>  a) A support 5 pasid bits
>>  b) B support 2 pasid bits
>>  c) when the task bind to device A, it allocate pasid = 16
>>  d) then it must be fail when trying to bind to task B, for its highest 
>> pasid is 4.
>>So it should allocate a single pasid for a context to avoid this?
> 
> Ideally yes, but the model chosen for the IOMMU API was one PASID per
> task, so I implemented this model (the PASID allocator will be common to
> IOMMU core in the future).
It is fair, for each IOMMU need PASID allocator to support SVM.

Thanks
Yisheng Xie

> 
> Therefore the PASID allocation will fail in your example, and there is no
> way around it. If you do (d) then (c), the task will have PASID 4.
> 
> Thanks,
> Jean
> 
> .
> 

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

[RFC PATCH 4/6] iommu/arm-smmu-v3: Add SVM support for platform devices

[Yisheng Xie]

From: Jean-Philippe Brucker 

Platform device can realise SVM function by using the stall mode. That
is to say, when device access a memory via iova which is not populated,
it will stalled and when SMMU try to translate this iova, it also will
stall and meanwhile send an event to CPU via MSI.

After SMMU driver handle the event and populated the iova, it will send
a RESUME command to SMMU to exit the stall mode, therefore the platform
device can contiue access the memory.

Signed-off-by: Jean-Philippe Brucker 
---
 drivers/iommu/arm-smmu-v3.c | 218 
 1 file changed, 181 insertions(+), 37 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index cafbeef..d44256a 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -359,6 +359,7 @@
 #define CTXDESC_CD_0_TCR_HA_SHIFT  43
 #define CTXDESC_CD_0_HD(1UL << 
CTXDESC_CD_0_TCR_HD_SHIFT)
 #define CTXDESC_CD_0_HA(1UL << 
CTXDESC_CD_0_TCR_HA_SHIFT)
+#define CTXDESC_CD_0_S (1UL << 44)
 #define CTXDESC_CD_0_R (1UL << 45)
 #define CTXDESC_CD_0_A (1UL << 46)
 #define CTXDESC_CD_0_ASET_SHIFT47
@@ -432,6 +433,15 @@
 #define CMDQ_PRI_1_RESP_FAIL   (1UL << CMDQ_PRI_1_RESP_SHIFT)
 #define CMDQ_PRI_1_RESP_SUCC   (2UL << CMDQ_PRI_1_RESP_SHIFT)
 
+#define CMDQ_RESUME_0_SID_SHIFT32
+#define CMDQ_RESUME_0_SID_MASK 0xUL
+#define CMDQ_RESUME_0_ACTION_SHIFT 12
+#define CMDQ_RESUME_0_ACTION_TERM  (0UL << CMDQ_RESUME_0_ACTION_SHIFT)
+#define CMDQ_RESUME_0_ACTION_RETRY (1UL << CMDQ_RESUME_0_ACTION_SHIFT)
+#define CMDQ_RESUME_0_ACTION_ABORT (2UL << CMDQ_RESUME_0_ACTION_SHIFT)
+#define CMDQ_RESUME_1_STAG_SHIFT   0
+#define CMDQ_RESUME_1_STAG_MASK0xUL
+
 #define CMDQ_SYNC_0_CS_SHIFT   12
 #define CMDQ_SYNC_0_CS_NONE(0UL << CMDQ_SYNC_0_CS_SHIFT)
 #define CMDQ_SYNC_0_CS_SEV (2UL << CMDQ_SYNC_0_CS_SHIFT)
@@ -443,6 +453,31 @@
 #define EVTQ_0_ID_SHIFT0
 #define EVTQ_0_ID_MASK 0xffUL
 
+#define EVT_ID_TRANSLATION_FAULT   0x10
+#define EVT_ID_ADDR_SIZE_FAULT 0x11
+#define EVT_ID_ACCESS_FAULT0x12
+#define EVT_ID_PERMISSION_FAULT0x13
+
+#define EVTQ_0_SSV (1UL << 11)
+#define EVTQ_0_SSID_SHIFT  12
+#define EVTQ_0_SSID_MASK   0xfUL
+#define EVTQ_0_SID_SHIFT   32
+#define EVTQ_0_SID_MASK0xUL
+#define EVTQ_1_STAG_SHIFT  0
+#define EVTQ_1_STAG_MASK   0xUL
+#define EVTQ_1_STALL   (1UL << 31)
+#define EVTQ_1_PRIV(1UL << 33)
+#define EVTQ_1_EXEC(1UL << 34)
+#define EVTQ_1_READ(1UL << 35)
+#define EVTQ_1_S2  (1UL << 39)
+#define EVTQ_1_CLASS_SHIFT 40
+#define EVTQ_1_CLASS_MASK  0x3UL
+#define EVTQ_1_TT_READ (1UL << 44)
+#define EVTQ_2_ADDR_SHIFT  0
+#define EVTQ_2_ADDR_MASK   0xUL
+#define EVTQ_3_IPA_SHIFT   12
+#define EVTQ_3_IPA_MASK0xffUL
+
 /* PRI queue */
 #define PRIQ_ENT_DWORDS2
 #define PRIQ_MAX_SZ_SHIFT  8
@@ -586,6 +621,13 @@ struct arm_smmu_cmdq_ent {
enum fault_status   resp;
} pri;
 
+   #define CMDQ_OP_RESUME  0x44
+   struct {
+   u32 sid;
+   u16 stag;
+   enum fault_status   resp;
+   } resume;
+
#define CMDQ_OP_CMD_SYNC0x46
};
 };
@@ -659,6 +701,7 @@ struct arm_smmu_s1_cfg {
 
struct list_headcontexts;
size_t  num_contexts;
+   boolcan_stall;
 
struct arm_smmu_ctx_desccd; /* Default context (SSID0) */
 };
@@ -682,6 +725,7 @@ struct arm_smmu_strtab_ent {
struct arm_smmu_s2_cfg  *s2_cfg;
 
boolprg_response_needs_ssid;
+   boolcan_stall;
 };
 
 struct arm_smmu_strtab_cfg {
@@ -816,6 +860,7 @@ struct arm_smmu_fault {
boolpriv;
 
boollast;
+   boolstall;
 
struct work_struct  work;
 };
@@ -1098,6 +1143,21 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct 
arm_smmu_cmdq_ent *ent)
return -EINVAL;
}
break;
+   case CMDQ_OP_RESUME:
+   switch 

[RFC PATCH 1/6] dt-bindings: document stall and PASID properties for IOMMU masters

[Yisheng Xie]

From: Jean-Philippe Brucker 

Document the bindings for stall and PASID capable platform devices.

Signed-off-by: Jean-Philippe Brucker 
---
 Documentation/devicetree/bindings/iommu/iommu.txt | 13 +
 1 file changed, 13 insertions(+)

diff --git a/Documentation/devicetree/bindings/iommu/iommu.txt 
b/Documentation/devicetree/bindings/iommu/iommu.txt
index 5a8b462..7355d7f 100644
--- a/Documentation/devicetree/bindings/iommu/iommu.txt
+++ b/Documentation/devicetree/bindings/iommu/iommu.txt
@@ -86,6 +86,19 @@ have a means to turn off translation. But it is invalid in 
such cases to
 disable the IOMMU's device tree node in the first place because it would
 prevent any driver from properly setting up the translations.
 
+Optional properties:
+
+- dma-can-stall: When present, the master can wait for a DMA transaction
+  to be handled by the IOMMU and can recover from a fault. For example, if
+  the page accessed by the DMA transaction isn't mapped, some IOMMUs are
+  able to stall the transaction and let the OS populate the page tables.
+  The IOMMU then performs the transaction if the fault was successfully
+  handled, or aborts the transaction otherwise.
+
+- pasid-bits: Some masters support multiple address spaces for DMA. By
+  tagging DMA transactions with an address space identifier. By default,
+  this is 0, which means that the device only has one address space.
+
 
 Notes:
 ==
-- 
1.7.12.4

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

[RFC PATCH 3/6] ACPI: IORT: Add stall and pasid properties to iommu_fwspec

[Yisheng Xie]

According to ACPI IORT spec, named component specific data has a node
flags field whoes bit0 is for Stall support. However, it do not have any
field for pasid bit.

As PCIe SMMU support 20 pasid bits, this patch suggest to use 5 bits[5:1]
in node flags field for pasid bits which means we can have 32 pasid bits.
And this should be enough for platform device. Anyway, this is just a RFC.

Signed-off-by: Yisheng Xie <xieyishe...@huawei.com>
---
 drivers/acpi/arm64/iort.c | 20 
 include/acpi/actbl2.h |  5 +
 2 files changed, 25 insertions(+)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index 4d9ccdd0..514caca3 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -710,6 +710,22 @@ static bool iort_pci_rc_supports_ats(struct acpi_iort_node 
*node)
return pci_rc->ats_attribute & ACPI_IORT_ATS_SUPPORTED;
 }
 
+static bool iort_platform_support_stall(struct acpi_iort_node *node)
+{
+   struct acpi_iort_named_component *ncomp;
+
+   ncomp = (struct acpi_iort_named_component *)node->node_data;
+   return ncomp->node_flags & ACPI_IORT_STALL_SUPPORTED;
+}
+
+static unsigned long iort_platform_get_pasid_bits(struct acpi_iort_node *node)
+{
+   struct acpi_iort_named_component *ncomp;
+
+   ncomp = (struct acpi_iort_named_component *)node->node_data;
+   return (ncomp->node_flags & ACPI_IORT_PASID_BITS_MASK) >> 
ACPI_IORT_PASID_BITS_SHIFT;
+}
+
 /**
  * iort_iommu_configure - Set-up IOMMU configuration for a device.
  *
@@ -772,6 +788,10 @@ const struct iommu_ops *iort_iommu_configure(struct device 
*dev)
   IORT_IOMMU_TYPE,
   i++);
}
+   if (!IS_ERR_OR_NULL(ops)) {
+   dev->iommu_fwspec->can_stall = 
iort_platform_support_stall(node);
+   dev->iommu_fwspec->num_pasid_bits = 
iort_platform_get_pasid_bits(node);
+   }
}
 
/*
diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
index 707dda74..125b150 100644
--- a/include/acpi/actbl2.h
+++ b/include/acpi/actbl2.h
@@ -749,6 +749,11 @@ struct acpi_iort_named_component {
char device_name[1];/* Path of namespace object */
 };
 
+#define ACPI_IORT_STALL_SUPPORTED  0x0001  /* The platform device 
supports stall */
+#define ACPI_IORT_STALL_UNSUPPORTED0x  /* The platform device 
doesn't support stall */
+#define ACPI_IORT_PASID_BITS_MASK  0x003e  /* 5 bits for PASID 
BITS */
+#define ACPI_IORT_PASID_BITS_SHIFT 1   /* PASID BITS numbers 
shift */
+
 struct acpi_iort_root_complex {
u64 memory_properties;  /* Memory access properties */
u32 ats_attribute;
-- 
1.7.12.4

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

[RFC PATCH 2/6] iommu/of: Add stall and pasid properties to iommu_fwspec

[Yisheng Xie]

From: Jean-Philippe Brucker 

Add stall and pasid properties to iommu_fwspec.

Signed-off-by: Jean-Philippe Brucker 
---
 drivers/iommu/of_iommu.c | 11 +++
 include/linux/iommu.h|  2 ++
 2 files changed, 12 insertions(+)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 84fa6b4..b926276 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -209,6 +209,16 @@ static bool of_pci_rc_supports_ats(struct device_node 
*rc_node)
break;
}
 
+   if (dev->iommu_fwspec) {
+   const __be32 *prop;
+
+   if (of_get_property(np, "dma-can-stall", NULL))
+   dev->iommu_fwspec->can_stall = true;
+
+   prop = of_get_property(np, "pasid-bits", NULL);
+   if (prop)
+   dev->iommu_fwspec->num_pasid_bits = be32_to_cpu(*prop);
+   }
+
return ops;
 }
 
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 1c5a216..5f580de 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -387,6 +387,8 @@ struct iommu_fwspec {
void*iommu_priv;
u32 flags;
unsigned intnum_ids;
+   unsigned long   num_pasid_bits;
+   boolcan_stall;
u32 ids[1];
 };
 
-- 
1.7.12.4

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

[RFC PATCH 5/6] iommu/arm-smmu-v3: fix panic when handle stall mode irq

[Yisheng Xie]

When SMMU do not support SVM feature, however the master support SVM,
which means matser can stall and with mult-pasid number, then the user
can bind a task to device using API like iommu_bind_task(). however,
when device trigger a stall mode fault i will cause panic:

[  106.996087] Unable to handle kernel NULL pointer dereference at virtual 
address 0100
[  106.996122] user pgtable: 4k pages, 48-bit VAs, pgd = 80003e023000
[  106.996150] [0100] *pgd=3e04a003, *pud=3e04b003, 
*pmd=
[  106.996201] Internal error: Oops: 9606 [#1] PREEMPT SM
[  106.996224] Modules linked in:
[  106.996256] CPU: 0 PID: 916 Comm: irq/14-arm-smmu Not tainted 
4.13.0-rc5-00035-g1235ddd-dirty #67
[  106.996288] Hardware name: Hisilicon PhosphorHi1383 ESL (DT)
[  106.996317] task: 80003adc1c00 task.stack: 80003a9f8000
[  106.996347] PC is at __queue_work+0x30/0x3a8
[  106.996374] LR is at queue_work_on+0x60/0x78
[  106.996401] pc : [] lr : [] pstate: 
40c001c9
[  106.996430] sp : 80003a9fbc20
[  106.996451] x29: 80003a9fbc20 x28: 80003adc1c00
[  106.996488] x27: 08d05080 x26: 80003ab0e028
[  106.996526] x25: 80003a9900ac x24: 0001
[  106.996562] x23: 0040 x22: 
[  106.996598] x21:  x20: 0140
[  106.996634] x19: 80003ab0e028 x18: 0010
[  106.996670] x17: a52a5040 x16: 0820f260
[  106.996708] x15: 0018e97629e0 x14: 80003fb89468
[  106.996744] x13:  x12: 80003abb0600
[  106.996781] x11:  x10: 01010100
[  106.996817] x9 : 85de5010 x8 : e4830001
[  106.996854] x7 : 80003a9fbcf8 x6 : 000fffe0
[  106.996890] x5 :  x4 : 0001
[  106.996926] x3 :  x2 : 80003ab0e028
[  106.996962] x1 :  x0 : 01c0
[  106.997002] Process irq/14-arm-smmu (pid: 916, stack limit 
=0x80003a9f8000)
[  106.997035] Stack: (0x80003a9fbc20 to 0x80003a9fc000)
[...]
[  106.998366] Call trace:
[  106.998842] [] __queue_work+0x30/0x3a8
[  106.998874] [] queue_work_on+0x60/0x78
[  106.998912] [] arm_smmu_handle_stall+0x104/0x138
[  106.998952] [] arm_smmu_evtq_thread+0xc0/0x158
[  106.998989] [] irq_thread_fn+0x28/0x68
[  106.999025] [] irq_thread+0x128/0x1d0
[  106.999060] [] kthread+0xfc/0x128
[  106.999093] [] ret_from_fork+0x10/0x50
[  106.999130] Code: a90153f3 a90573fb d53b4220 363814c0 (b94102a0)
[  106.999159] ---[ end trace 7e5c9f0cb1f2fecd ]---

And the resean is we donot init fault_queue while the fault handle need
to use it. Fix by return -EINVAL in arm_smmu_bind_task() when smmu do not
support the feature of SVM.

Signed-off-by: Yisheng Xie <xieyishe...@huawei.com>
---
 drivers/iommu/arm-smmu-v3.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index d44256a..dbda2eb 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -2922,6 +2922,8 @@ static int arm_smmu_bind_task(struct device *dev, struct 
task_struct *task,
return -EINVAL;
 
smmu = master->smmu;
+   if (!(smmu->features & ARM_SMMU_FEAT_SVM))
+   return -EINVAL;
 
domain = iommu_get_domain_for_dev(dev);
if (WARN_ON(!domain))
-- 
1.7.12.4

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

[RFC PATCH 0/6] Add platform device SVM support for ARM SMMUv3

[Yisheng Xie]

Jean-Philippe has post a patchset for Adding PCIe SVM support to ARM SMMUv3:
https://www.spinics.net/lists/arm-kernel/msg565155.html

But for some platform devices(aka on-chip integrated devices), there is also
SVM requirement, which works based on the SMMU stall mode.
Jean-Philippe has prepared a prototype patchset to support it:
git://linux-arm.org/linux-jpb.git svm/stall

We tested this patchset with some fixes on a on-chip integrated device. The
basic function is ok, so I just send them out for review, although this
patchset heavily depends on the former patchset (PCIe SVM support for ARM
SMMUv3), which is still under discussion.

Patch Overview:
*1 to 3 prepare for device tree or acpi get the device stall ability and pasid 
bits
*4 is to realise the SVM function for platform device
*5 is fix a bug when test SVM function while SMMU donnot support this feature
*6 avoid ILLEGAL setting of STE and CD entry about stall

Acctually here, I also have a question about SVM on SMMUv3:

1. Why the SVM feature on SMMUv3 depends on BTM feature? when bind a task to 
device,
   it will register a mmu_notify. Therefore, when a page range is invalid, we 
can
   send TLBI or ATC invalid without BTM?

2. According to ACPI IORT spec, named component specific data has a node flags 
field
   whoes bit0 is for Stall support. However, it do not have any field for pasid 
bit.
   Can we use other 5 bits[5:1] for pasid bit numbers, so we can have 32 pasid 
bit for
   a single platform device which should be enough, because SMMU only support 
20 bit pasid

3. Presently, the pasid is allocate for a task but not for a context, if a task 
is trying
   to bind to 2 device A and B:
 a) A support 5 pasid bits
 b) B support 2 pasid bits
 c) when the task bind to device A, it allocate pasid = 16
 d) then it must be fail when trying to bind to task B, for its highest 
pasid is 4.
   So it should allocate a single pasid for a context to avoid this?


Jean-Philippe Brucker (3):
  dt-bindings: document stall and PASID properties for IOMMU masters
  iommu/of: Add stall and pasid properties to iommu_fwspec
  iommu/arm-smmu-v3: Add SVM support for platform devices

Yisheng Xie (3):
  ACPI: IORT: Add stall and pasid properties to iommu_fwspec
  iommu/arm-smmu-v3: fix panic when handle stall mode irq
  iommu/arm-smmu-v3: Avoid ILLEGAL setting of STE.S1STALLD and CD.S

 Documentation/devicetree/bindings/iommu/iommu.txt |  13 ++
 drivers/acpi/arm64/iort.c |  20 ++
 drivers/iommu/arm-smmu-v3.c   | 230 ++
 drivers/iommu/of_iommu.c  |  11 +
 include/acpi/actbl2.h |   5 +
 include/linux/iommu.h |   2 +
 6 files changed, 244 insertions(+), 37 deletions(-)

--
1.7.12.4

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

[RFC PATCH 6/6] iommu/arm-smmu-v3: Avoid ILLEGAL setting of STE.S1STALLD and CD.S

[Yisheng Xie]

It is ILLEGAL to set STE.S1STALLD if STALL_MODEL is not 0b00, which
means we should not disable stall mode if stall/terminate mode is not
configuable.

Meanwhile, it is also ILLEGAL when STALL_MODEL==0b10 && CD.S==0 which
means if stall mode is force we should always set CD.S.

This patch add ARM_SMMU_FEAT_TERMINATE feature bit for smmu, and use
TERMINATE feature checking to ensue above ILLEGAL cases from happening.

Signed-off-by: Yisheng Xie <xieyishe...@huawei.com>
---
 drivers/iommu/arm-smmu-v3.c | 22 --
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index dbda2eb..0745522 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -55,6 +55,7 @@
 #define IDR0_STALL_MODEL_SHIFT 24
 #define IDR0_STALL_MODEL_MASK  0x3
 #define IDR0_STALL_MODEL_STALL (0 << IDR0_STALL_MODEL_SHIFT)
+#define IDR0_STALL_MODEL_NS(1 << IDR0_STALL_MODEL_SHIFT)
 #define IDR0_STALL_MODEL_FORCE (2 << IDR0_STALL_MODEL_SHIFT)
 #define IDR0_TTENDIAN_SHIFT21
 #define IDR0_TTENDIAN_MASK 0x3
@@ -766,6 +767,7 @@ struct arm_smmu_device {
 #define ARM_SMMU_FEAT_SVM  (1 << 15)
 #define ARM_SMMU_FEAT_HA   (1 << 16)
 #define ARM_SMMU_FEAT_HD   (1 << 17)
+#define ARM_SMMU_FEAT_TERMINATE(1 << 18)
u32 features;
 
 #define ARM_SMMU_OPT_SKIP_PREFETCH (1 << 0)
@@ -1402,6 +1404,7 @@ static void arm_smmu_write_ctx_desc(struct 
arm_smmu_domain *smmu_domain,
u64 val;
bool cd_live;
__u64 *cdptr = arm_smmu_get_cd_ptr(smmu_domain, ssid);
+   struct arm_smmu_device *smmu = smmu_domain->smmu;
 
/*
 * This function handles the following cases:
@@ -1468,9 +1471,11 @@ static void arm_smmu_write_ctx_desc(struct 
arm_smmu_domain *smmu_domain,
  CTXDESC_CD_0_V;
 
/*
-* FIXME: STALL_MODEL==0b10 && CD.S==0 is ILLEGAL
+* STALL_MODEL==0b10 && CD.S==0 is ILLEGAL
 */
-   if (ssid && smmu_domain->s1_cfg.can_stall)
+   if ((ssid && smmu_domain->s1_cfg.can_stall) ||
+   (!(smmu->features & ARM_SMMU_FEAT_TERMINATE) &&
+   smmu->features & ARM_SMMU_FEAT_STALLS))
val |= CTXDESC_CD_0_S;
 
cdptr[0] = cpu_to_le64(val);
@@ -1690,12 +1695,13 @@ static void arm_smmu_write_strtab_ent(struct 
arm_smmu_device *smmu, u32 sid,
dst[1] |= STRTAB_STE_1_PPAR;
 
/*
-* FIXME: it is illegal to set S1STALLD if STALL_MODEL=0b10
-* (force). But according to the spec, it *must* be set for
+* According to spec, it is illegal to set S1STALLD if
+* STALL_MODEL is not 0b00. And it *must* be set for
 * devices that aren't capable of stalling (notably pci!)
-* So we need a "STALL_MODEL=0b00" feature bit.
 */
-   if (smmu->features & ARM_SMMU_FEAT_STALLS && !ste->can_stall)
+   if (smmu->features & ARM_SMMU_FEAT_STALLS &&
+   smmu->features & ARM_SMMU_FEAT_TERMINATE &&
+   !ste->can_stall)
dst[1] |= cpu_to_le64(STRTAB_STE_1_S1STALLD);
 
val |= (s1ctxptr & STRTAB_STE_0_S1CTXPTR_MASK
@@ -4577,9 +4583,13 @@ static int arm_smmu_device_hw_probe(struct 
arm_smmu_device *smmu)
 
switch (reg & IDR0_STALL_MODEL_MASK << IDR0_STALL_MODEL_SHIFT) {
case IDR0_STALL_MODEL_STALL:
+   smmu->features |= ARM_SMMU_FEAT_TERMINATE;
/* Fallthrough */
case IDR0_STALL_MODEL_FORCE:
smmu->features |= ARM_SMMU_FEAT_STALLS;
+   break;
+   case IDR0_STALL_MODEL_NS:
+   smmu->features |= ARM_SMMU_FEAT_TERMINATE;
}
 
if (reg & IDR0_S1P)
-- 
1.7.12.4

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

Re: What differences and relations between SVM, HSA, HMM and Unified Memory?

[Yisheng Xie]

Hi Jérôme and Jean-Philippe ,

Get it, thanks for all of your detail explain.

Thanks
Yisheng Xie

On 2017/7/17 22:27, Jerome Glisse wrote:
> On Mon, Jul 17, 2017 at 07:57:23PM +0800, Yisheng Xie wrote:
>> Hi Jean-Philippe，
>>
>> On 2017/6/12 19:37, Jean-Philippe Brucker wrote:
>>> Hello,
>>>
>>> On 10/06/17 05:06, Wuzongyong (Cordius Wu, Euler Dept) wrote:
>>>> Hi,
>>>>
>>>> Could someone explain differences and relations between the SVM(Shared
>>>> Virtual Memory, by Intel), HSA(Heterogeneous System Architecture, by AMD),
>>>> HMM(Heterogeneous Memory Management, by Glisse) and UM(Unified Memory, by
>>>> NVIDIA) ? Are these in the substitutional relation?
>>>>
>>>> As I understand it, these aim to solve the same thing, sharing pointers
>>>> between CPU and GPU(implement with ATS/PASID/PRI/IOMMU support). So far,
>>>> SVM and HSA can only be used by integrated gpu. And, Intel declare that
>>>> the root ports doesn’t not have the required TLP prefix support, resulting
>>>>  that SVM can’t be used by discrete devices. So could someone tell me the
>>>> required TLP prefix means what specifically?>
>>>> With HMM, we can use allocator like malloc to manage host and device
>>>> memory. Does this mean that there is no need to use SVM and HSA with HMM,
>>>> or HMM is the basis of SVM and HAS to implement Fine-Grained system SVM
>>>> defined in the opencl spec?
>>>
>>> I can't provide an exhaustive answer, but I have done some work on SVM.
>>> Take it with a grain of salt though, I am not an expert.
>>>
>>> * HSA is an architecture that provides a common programming model for CPUs
>>> and accelerators (GPGPUs etc). It does have SVM requirement (I/O page
>>> faults, PASID and compatible address spaces), though it's only a small
>>> part of it.
>>>
>>> * Similarly, OpenCL provides an API for dealing with accelerators. OpenCL
>>> 2.0 introduced the concept of Fine-Grained System SVM, which allows to
>>> pass userspace pointers to devices. It is just one flavor of SVM, they
>>> also have coarse-grained and non-system. But they might have coined the
>>> name, and I believe that in the context of Linux IOMMU, when we talk about
>>> "SVM" it is OpenCL's fine-grained system SVM.
>>> [...]
>>>
>>> While SVM is only about virtual address space,
>> As you mentioned, SVM is only about virtual address space, I'd like to know 
>> how to
>> manage the physical address especially about device's RAM, before HMM?
>>
>> When OpenCL alloc a SVM pointer like:
>> void* p = clSVMAlloc (
>> context, // an OpenCL context where this buffer is available
>> CL_MEM_READ_WRITE | CL_MEM_SVM_FINE_GRAIN_BUFFER,
>> size, // amount of memory to allocate (in bytes)
>> 0 // alignment in bytes (0 means default)
>> );
>>
>> where this RAM come from， device RAM or host RAM?
>>
> 
> For SVM using ATS/PASID with FINE_GRAIN your allocation can only
> be inside the system memory (host RAM). You need a special system
> bus like CAPI or CCIX which both are step further than ATS/PASID
> to be able to allow fine grain to use device memory.
> 
> However that is where HMM can be usefull as HMM is a software
> solution to this problem. So with HMM and a device that can work
> with HMM, you can get fine grain allocation to also use device
> memory however any CPU access will happen in host RAM.
> 
> Jérôme
> 
> .
> 

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

Re: What differences and relations between SVM, HSA, HMM and Unified Memory?

[Yisheng Xie]

Hi Jean-Philippe，

On 2017/6/12 19:37, Jean-Philippe Brucker wrote:
> Hello,
> 
> On 10/06/17 05:06, Wuzongyong (Cordius Wu, Euler Dept) wrote:
>> Hi,
>>
>> Could someone explain differences and relations between the SVM(Shared
>> Virtual Memory, by Intel), HSA(Heterogeneous System Architecture, by AMD),
>> HMM(Heterogeneous Memory Management, by Glisse) and UM(Unified Memory, by
>> NVIDIA) ? Are these in the substitutional relation?
>>
>> As I understand it, these aim to solve the same thing, sharing pointers
>> between CPU and GPU(implement with ATS/PASID/PRI/IOMMU support). So far,
>> SVM and HSA can only be used by integrated gpu. And, Intel declare that
>> the root ports doesn’t not have the required TLP prefix support, resulting
>>  that SVM can’t be used by discrete devices. So could someone tell me the
>> required TLP prefix means what specifically?>
>> With HMM, we can use allocator like malloc to manage host and device
>> memory. Does this mean that there is no need to use SVM and HSA with HMM,
>> or HMM is the basis of SVM and HAS to implement Fine-Grained system SVM
>> defined in the opencl spec?
> 
> I can't provide an exhaustive answer, but I have done some work on SVM.
> Take it with a grain of salt though, I am not an expert.
> 
> * HSA is an architecture that provides a common programming model for CPUs
> and accelerators (GPGPUs etc). It does have SVM requirement (I/O page
> faults, PASID and compatible address spaces), though it's only a small
> part of it.
> 
> * Similarly, OpenCL provides an API for dealing with accelerators. OpenCL
> 2.0 introduced the concept of Fine-Grained System SVM, which allows to
> pass userspace pointers to devices. It is just one flavor of SVM, they
> also have coarse-grained and non-system. But they might have coined the
> name, and I believe that in the context of Linux IOMMU, when we talk about
> "SVM" it is OpenCL's fine-grained system SVM.
> [...]
> 
> While SVM is only about virtual address space,
As you mentioned, SVM is only about virtual address space, I'd like to know how 
to
manage the physical address especially about device's RAM, before HMM?

When OpenCL alloc a SVM pointer like:
void* p = clSVMAlloc (
context, // an OpenCL context where this buffer is available
CL_MEM_READ_WRITE | CL_MEM_SVM_FINE_GRAIN_BUFFER,
        size, // amount of memory to allocate (in bytes)
0 // alignment in bytes (0 means default)
);

where this RAM come from， device RAM or host RAM?

Thanks
Yisheng Xie

> HMM deals with physical
> storage. If I understand correctly, HMM allows to transparently use device
> RAM from userspace applications. So upon an I/O page fault, the mm
> subsystem will migrate data from system memory into device RAM. It would
> differ from "pure" SVM in that you would use different page directories on
> IOMMU and MMU sides, and synchronize them using MMU notifiers. But please
> don't take this at face value, I haven't had time to look into HMM yet.
> 
> Thanks,
> Jean
> 
> .
> 

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