Re: [PATCH v2] iommu/ipmmu-vmsa: Rereserving a free context before setting up a pagetable
On Wed, Aug 23, 2017 at 05:31:42PM +0300, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko > > Reserving a free context is both quicker and more likely to fail > (due to limited hardware resources) than setting up a pagetable. > What is more the pagetable init/cleanup code could require > the context to be set up. > > Signed-off-by: Oleksandr Tyshchenko > CC: Robin Murphy > CC: Laurent Pinchart > CC: Joerg Roedel Applied, thanks. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] iommu/ipmmu-vmsa: Rereserving a free context before setting up a pagetable
On Tue, Aug 29, 2017 at 04:30:55PM +0300, Oleksandr Tyshchenko wrote: > Thank you for the review. > Shall I resend patch with added R-bs? No, I add them when applying the patch. Thanks, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] iommu/ipmmu-vmsa: Rereserving a free context before setting up a pagetable
Hi Laurent, Robin
On Tue, Aug 29, 2017 at 4:01 PM, Laurent Pinchart
wrote:
> Hi Oleksandr,
>
> Thank you for the patch.
>
> On Wednesday, 23 August 2017 17:31:42 EEST Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko
>>
>> Reserving a free context is both quicker and more likely to fail
>> (due to limited hardware resources) than setting up a pagetable.
>> What is more the pagetable init/cleanup code could require
>> the context to be set up.
>>
>> Signed-off-by: Oleksandr Tyshchenko
>> CC: Robin Murphy
>> CC: Laurent Pinchart
>> CC: Joerg Roedel
>
> Reviewed-by: Laurent Pinchart
>
>> ---
>> This patch fixes a bug during rollback logic:
>> In ipmmu_domain_init_context() we are trying to find an unused context
>> and if operation fails we will call free_io_pgtable_ops(),
>> but "domain->context_id" hasn't been initialized yet (likely it is 0
>> because of kzalloc). Having the following call stack:
>> free_io_pgtable_ops() -> io_pgtable_tlb_flush_all() ->
>> ipmmu_tlb_flush_all() -> ipmmu_tlb_invalidate()
>> we will get a mistaken cache flush for a context pointed by
>> uninitialized "domain->context_id".
>>
>>
>> v1 here:
>> https://lists.linuxfoundation.org/pipermail/iommu/2017-August/023857.html
>> ---
>> drivers/iommu/ipmmu-vmsa.c | 42 +-
>> 1 file changed, 21 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
>> index 2a38aa1..382f387 100644
>> --- a/drivers/iommu/ipmmu-vmsa.c
>> +++ b/drivers/iommu/ipmmu-vmsa.c
>> @@ -341,6 +341,19 @@ static int ipmmu_domain_allocate_context(struct
>> ipmmu_vmsa_device *mmu, return ret;
>> }
>>
>> +static void ipmmu_domain_free_context(struct ipmmu_vmsa_device *mmu,
>> + unsigned int context_id)
>> +{
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&mmu->lock, flags);
>> +
>> + clear_bit(context_id, mmu->ctx);
>> + mmu->domains[context_id] = NULL;
>> +
>> + spin_unlock_irqrestore(&mmu->lock, flags);
>> +}
>> +
>> static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
>> {
>> u64 ttbr;
>> @@ -370,22 +383,22 @@ static int ipmmu_domain_init_context(struct
>> ipmmu_vmsa_domain *domain) */
>> domain->cfg.iommu_dev = domain->mmu->dev;
>>
>> - domain->iop = alloc_io_pgtable_ops(ARM_32_LPAE_S1, &domain->cfg,
>> -domain);
>> - if (!domain->iop)
>> - return -EINVAL;
>> -
>> /*
>>* Find an unused context.
>>*/
>> ret = ipmmu_domain_allocate_context(domain->mmu, domain);
>> - if (ret == IPMMU_CTX_MAX) {
>> - free_io_pgtable_ops(domain->iop);
>> + if (ret == IPMMU_CTX_MAX)
>> return -EBUSY;
>> - }
>>
>> domain->context_id = ret;
>>
>> + domain->iop = alloc_io_pgtable_ops(ARM_32_LPAE_S1, &domain->cfg,
>> +domain);
>> + if (!domain->iop) {
>> + ipmmu_domain_free_context(domain->mmu, domain->context_id);
>> + return -EINVAL;
>> + }
>> +
>> /* TTBR0 */
>> ttbr = domain->cfg.arm_lpae_s1_cfg.ttbr[0];
>> ipmmu_ctx_write(domain, IMTTLBR0, ttbr);
>> @@ -426,19 +439,6 @@ static int ipmmu_domain_init_context(struct
>> ipmmu_vmsa_domain *domain) return 0;
>> }
>>
>> -static void ipmmu_domain_free_context(struct ipmmu_vmsa_device *mmu,
>> - unsigned int context_id)
>> -{
>> - unsigned long flags;
>> -
>> - spin_lock_irqsave(&mmu->lock, flags);
>> -
>> - clear_bit(context_id, mmu->ctx);
>> - mmu->domains[context_id] = NULL;
>> -
>> - spin_unlock_irqrestore(&mmu->lock, flags);
>> -}
>> -
>> static void ipmmu_domain_destroy_context(struct ipmmu_vmsa_domain *domain)
>> {
>> /*
>
>
> --
> Regards,
>
> Laurent Pinchart
>
Thank you for the review.
Shall I resend patch with added R-bs?
--
Regards,
Oleksandr Tyshchenko
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] iommu/ipmmu-vmsa: Rereserving a free context before setting up a pagetable
Hi Oleksandr,
Thank you for the patch.
On Wednesday, 23 August 2017 17:31:42 EEST Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko
>
> Reserving a free context is both quicker and more likely to fail
> (due to limited hardware resources) than setting up a pagetable.
> What is more the pagetable init/cleanup code could require
> the context to be set up.
>
> Signed-off-by: Oleksandr Tyshchenko
> CC: Robin Murphy
> CC: Laurent Pinchart
> CC: Joerg Roedel
Reviewed-by: Laurent Pinchart
> ---
> This patch fixes a bug during rollback logic:
> In ipmmu_domain_init_context() we are trying to find an unused context
> and if operation fails we will call free_io_pgtable_ops(),
> but "domain->context_id" hasn't been initialized yet (likely it is 0
> because of kzalloc). Having the following call stack:
> free_io_pgtable_ops() -> io_pgtable_tlb_flush_all() ->
> ipmmu_tlb_flush_all() -> ipmmu_tlb_invalidate()
> we will get a mistaken cache flush for a context pointed by
> uninitialized "domain->context_id".
>
>
> v1 here:
> https://lists.linuxfoundation.org/pipermail/iommu/2017-August/023857.html
> ---
> drivers/iommu/ipmmu-vmsa.c | 42 +-
> 1 file changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index 2a38aa1..382f387 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -341,6 +341,19 @@ static int ipmmu_domain_allocate_context(struct
> ipmmu_vmsa_device *mmu, return ret;
> }
>
> +static void ipmmu_domain_free_context(struct ipmmu_vmsa_device *mmu,
> + unsigned int context_id)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&mmu->lock, flags);
> +
> + clear_bit(context_id, mmu->ctx);
> + mmu->domains[context_id] = NULL;
> +
> + spin_unlock_irqrestore(&mmu->lock, flags);
> +}
> +
> static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
> {
> u64 ttbr;
> @@ -370,22 +383,22 @@ static int ipmmu_domain_init_context(struct
> ipmmu_vmsa_domain *domain) */
> domain->cfg.iommu_dev = domain->mmu->dev;
>
> - domain->iop = alloc_io_pgtable_ops(ARM_32_LPAE_S1, &domain->cfg,
> -domain);
> - if (!domain->iop)
> - return -EINVAL;
> -
> /*
>* Find an unused context.
>*/
> ret = ipmmu_domain_allocate_context(domain->mmu, domain);
> - if (ret == IPMMU_CTX_MAX) {
> - free_io_pgtable_ops(domain->iop);
> + if (ret == IPMMU_CTX_MAX)
> return -EBUSY;
> - }
>
> domain->context_id = ret;
>
> + domain->iop = alloc_io_pgtable_ops(ARM_32_LPAE_S1, &domain->cfg,
> +domain);
> + if (!domain->iop) {
> + ipmmu_domain_free_context(domain->mmu, domain->context_id);
> + return -EINVAL;
> + }
> +
> /* TTBR0 */
> ttbr = domain->cfg.arm_lpae_s1_cfg.ttbr[0];
> ipmmu_ctx_write(domain, IMTTLBR0, ttbr);
> @@ -426,19 +439,6 @@ static int ipmmu_domain_init_context(struct
> ipmmu_vmsa_domain *domain) return 0;
> }
>
> -static void ipmmu_domain_free_context(struct ipmmu_vmsa_device *mmu,
> - unsigned int context_id)
> -{
> - unsigned long flags;
> -
> - spin_lock_irqsave(&mmu->lock, flags);
> -
> - clear_bit(context_id, mmu->ctx);
> - mmu->domains[context_id] = NULL;
> -
> - spin_unlock_irqrestore(&mmu->lock, flags);
> -}
> -
> static void ipmmu_domain_destroy_context(struct ipmmu_vmsa_domain *domain)
> {
> /*
--
Regards,
Laurent Pinchart
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] iommu/ipmmu-vmsa: Rereserving a free context before setting up a pagetable
On 28/08/17 14:23, Oleksandr Tyshchenko wrote:
> Hi, all.
>
> Any comments?
>
> On Wed, Aug 23, 2017 at 5:31 PM, Oleksandr Tyshchenko
> wrote:
>> From: Oleksandr Tyshchenko
>>
>> Reserving a free context is both quicker and more likely to fail
>> (due to limited hardware resources) than setting up a pagetable.
>> What is more the pagetable init/cleanup code could require
>> the context to be set up.
Looks OK to me,
Reviewed-by: Robin Murphy
>> Signed-off-by: Oleksandr Tyshchenko
>> CC: Robin Murphy
>> CC: Laurent Pinchart
>> CC: Joerg Roedel
>>
>> ---
>> This patch fixes a bug during rollback logic:
>> In ipmmu_domain_init_context() we are trying to find an unused context
>> and if operation fails we will call free_io_pgtable_ops(),
>> but "domain->context_id" hasn't been initialized yet (likely it is 0
>> because of kzalloc). Having the following call stack:
>> free_io_pgtable_ops() -> io_pgtable_tlb_flush_all() ->
>> ipmmu_tlb_flush_all() -> ipmmu_tlb_invalidate()
>> we will get a mistaken cache flush for a context pointed by
>> uninitialized "domain->context_id".
>>
>>
>> v1 here:
>> https://lists.linuxfoundation.org/pipermail/iommu/2017-August/023857.html
>> ---
>> drivers/iommu/ipmmu-vmsa.c | 42 +-
>> 1 file changed, 21 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
>> index 2a38aa1..382f387 100644
>> --- a/drivers/iommu/ipmmu-vmsa.c
>> +++ b/drivers/iommu/ipmmu-vmsa.c
>> @@ -341,6 +341,19 @@ static int ipmmu_domain_allocate_context(struct
>> ipmmu_vmsa_device *mmu,
>> return ret;
>> }
>>
>> +static void ipmmu_domain_free_context(struct ipmmu_vmsa_device *mmu,
>> + unsigned int context_id)
>> +{
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&mmu->lock, flags);
>> +
>> + clear_bit(context_id, mmu->ctx);
>> + mmu->domains[context_id] = NULL;
>> +
>> + spin_unlock_irqrestore(&mmu->lock, flags);
>> +}
>> +
>> static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
>> {
>> u64 ttbr;
>> @@ -370,22 +383,22 @@ static int ipmmu_domain_init_context(struct
>> ipmmu_vmsa_domain *domain)
>> */
>> domain->cfg.iommu_dev = domain->mmu->dev;
>>
>> - domain->iop = alloc_io_pgtable_ops(ARM_32_LPAE_S1, &domain->cfg,
>> - domain);
>> - if (!domain->iop)
>> - return -EINVAL;
>> -
>> /*
>> * Find an unused context.
>> */
>> ret = ipmmu_domain_allocate_context(domain->mmu, domain);
>> - if (ret == IPMMU_CTX_MAX) {
>> - free_io_pgtable_ops(domain->iop);
>> + if (ret == IPMMU_CTX_MAX)
>> return -EBUSY;
>> - }
>>
>> domain->context_id = ret;
>>
>> + domain->iop = alloc_io_pgtable_ops(ARM_32_LPAE_S1, &domain->cfg,
>> + domain);
>> + if (!domain->iop) {
>> + ipmmu_domain_free_context(domain->mmu, domain->context_id);
>> + return -EINVAL;
>> + }
>> +
>> /* TTBR0 */
>> ttbr = domain->cfg.arm_lpae_s1_cfg.ttbr[0];
>> ipmmu_ctx_write(domain, IMTTLBR0, ttbr);
>> @@ -426,19 +439,6 @@ static int ipmmu_domain_init_context(struct
>> ipmmu_vmsa_domain *domain)
>> return 0;
>> }
>>
>> -static void ipmmu_domain_free_context(struct ipmmu_vmsa_device *mmu,
>> - unsigned int context_id)
>> -{
>> - unsigned long flags;
>> -
>> - spin_lock_irqsave(&mmu->lock, flags);
>> -
>> - clear_bit(context_id, mmu->ctx);
>> - mmu->domains[context_id] = NULL;
>> -
>> - spin_unlock_irqrestore(&mmu->lock, flags);
>> -}
>> -
>> static void ipmmu_domain_destroy_context(struct ipmmu_vmsa_domain *domain)
>> {
>> /*
>> --
>> 2.7.4
>>
>
>
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] iommu/ipmmu-vmsa: Rereserving a free context before setting up a pagetable
Hi, all.
Any comments?
On Wed, Aug 23, 2017 at 5:31 PM, Oleksandr Tyshchenko
wrote:
> From: Oleksandr Tyshchenko
>
> Reserving a free context is both quicker and more likely to fail
> (due to limited hardware resources) than setting up a pagetable.
> What is more the pagetable init/cleanup code could require
> the context to be set up.
>
> Signed-off-by: Oleksandr Tyshchenko
> CC: Robin Murphy
> CC: Laurent Pinchart
> CC: Joerg Roedel
>
> ---
> This patch fixes a bug during rollback logic:
> In ipmmu_domain_init_context() we are trying to find an unused context
> and if operation fails we will call free_io_pgtable_ops(),
> but "domain->context_id" hasn't been initialized yet (likely it is 0
> because of kzalloc). Having the following call stack:
> free_io_pgtable_ops() -> io_pgtable_tlb_flush_all() ->
> ipmmu_tlb_flush_all() -> ipmmu_tlb_invalidate()
> we will get a mistaken cache flush for a context pointed by
> uninitialized "domain->context_id".
>
>
> v1 here:
> https://lists.linuxfoundation.org/pipermail/iommu/2017-August/023857.html
> ---
> drivers/iommu/ipmmu-vmsa.c | 42 +-
> 1 file changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index 2a38aa1..382f387 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -341,6 +341,19 @@ static int ipmmu_domain_allocate_context(struct
> ipmmu_vmsa_device *mmu,
> return ret;
> }
>
> +static void ipmmu_domain_free_context(struct ipmmu_vmsa_device *mmu,
> + unsigned int context_id)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&mmu->lock, flags);
> +
> + clear_bit(context_id, mmu->ctx);
> + mmu->domains[context_id] = NULL;
> +
> + spin_unlock_irqrestore(&mmu->lock, flags);
> +}
> +
> static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
> {
> u64 ttbr;
> @@ -370,22 +383,22 @@ static int ipmmu_domain_init_context(struct
> ipmmu_vmsa_domain *domain)
> */
> domain->cfg.iommu_dev = domain->mmu->dev;
>
> - domain->iop = alloc_io_pgtable_ops(ARM_32_LPAE_S1, &domain->cfg,
> - domain);
> - if (!domain->iop)
> - return -EINVAL;
> -
> /*
> * Find an unused context.
> */
> ret = ipmmu_domain_allocate_context(domain->mmu, domain);
> - if (ret == IPMMU_CTX_MAX) {
> - free_io_pgtable_ops(domain->iop);
> + if (ret == IPMMU_CTX_MAX)
> return -EBUSY;
> - }
>
> domain->context_id = ret;
>
> + domain->iop = alloc_io_pgtable_ops(ARM_32_LPAE_S1, &domain->cfg,
> + domain);
> + if (!domain->iop) {
> + ipmmu_domain_free_context(domain->mmu, domain->context_id);
> + return -EINVAL;
> + }
> +
> /* TTBR0 */
> ttbr = domain->cfg.arm_lpae_s1_cfg.ttbr[0];
> ipmmu_ctx_write(domain, IMTTLBR0, ttbr);
> @@ -426,19 +439,6 @@ static int ipmmu_domain_init_context(struct
> ipmmu_vmsa_domain *domain)
> return 0;
> }
>
> -static void ipmmu_domain_free_context(struct ipmmu_vmsa_device *mmu,
> - unsigned int context_id)
> -{
> - unsigned long flags;
> -
> - spin_lock_irqsave(&mmu->lock, flags);
> -
> - clear_bit(context_id, mmu->ctx);
> - mmu->domains[context_id] = NULL;
> -
> - spin_unlock_irqrestore(&mmu->lock, flags);
> -}
> -
> static void ipmmu_domain_destroy_context(struct ipmmu_vmsa_domain *domain)
> {
> /*
> --
> 2.7.4
>
--
Regards,
Oleksandr Tyshchenko
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2] iommu/ipmmu-vmsa: Rereserving a free context before setting up a pagetable
From: Oleksandr Tyshchenko
Reserving a free context is both quicker and more likely to fail
(due to limited hardware resources) than setting up a pagetable.
What is more the pagetable init/cleanup code could require
the context to be set up.
Signed-off-by: Oleksandr Tyshchenko
CC: Robin Murphy
CC: Laurent Pinchart
CC: Joerg Roedel
---
This patch fixes a bug during rollback logic:
In ipmmu_domain_init_context() we are trying to find an unused context
and if operation fails we will call free_io_pgtable_ops(),
but "domain->context_id" hasn't been initialized yet (likely it is 0
because of kzalloc). Having the following call stack:
free_io_pgtable_ops() -> io_pgtable_tlb_flush_all() ->
ipmmu_tlb_flush_all() -> ipmmu_tlb_invalidate()
we will get a mistaken cache flush for a context pointed by
uninitialized "domain->context_id".
v1 here:
https://lists.linuxfoundation.org/pipermail/iommu/2017-August/023857.html
---
drivers/iommu/ipmmu-vmsa.c | 42 +-
1 file changed, 21 insertions(+), 21 deletions(-)
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 2a38aa1..382f387 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -341,6 +341,19 @@ static int ipmmu_domain_allocate_context(struct
ipmmu_vmsa_device *mmu,
return ret;
}
+static void ipmmu_domain_free_context(struct ipmmu_vmsa_device *mmu,
+ unsigned int context_id)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&mmu->lock, flags);
+
+ clear_bit(context_id, mmu->ctx);
+ mmu->domains[context_id] = NULL;
+
+ spin_unlock_irqrestore(&mmu->lock, flags);
+}
+
static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
{
u64 ttbr;
@@ -370,22 +383,22 @@ static int ipmmu_domain_init_context(struct
ipmmu_vmsa_domain *domain)
*/
domain->cfg.iommu_dev = domain->mmu->dev;
- domain->iop = alloc_io_pgtable_ops(ARM_32_LPAE_S1, &domain->cfg,
- domain);
- if (!domain->iop)
- return -EINVAL;
-
/*
* Find an unused context.
*/
ret = ipmmu_domain_allocate_context(domain->mmu, domain);
- if (ret == IPMMU_CTX_MAX) {
- free_io_pgtable_ops(domain->iop);
+ if (ret == IPMMU_CTX_MAX)
return -EBUSY;
- }
domain->context_id = ret;
+ domain->iop = alloc_io_pgtable_ops(ARM_32_LPAE_S1, &domain->cfg,
+ domain);
+ if (!domain->iop) {
+ ipmmu_domain_free_context(domain->mmu, domain->context_id);
+ return -EINVAL;
+ }
+
/* TTBR0 */
ttbr = domain->cfg.arm_lpae_s1_cfg.ttbr[0];
ipmmu_ctx_write(domain, IMTTLBR0, ttbr);
@@ -426,19 +439,6 @@ static int ipmmu_domain_init_context(struct
ipmmu_vmsa_domain *domain)
return 0;
}
-static void ipmmu_domain_free_context(struct ipmmu_vmsa_device *mmu,
- unsigned int context_id)
-{
- unsigned long flags;
-
- spin_lock_irqsave(&mmu->lock, flags);
-
- clear_bit(context_id, mmu->ctx);
- mmu->domains[context_id] = NULL;
-
- spin_unlock_irqrestore(&mmu->lock, flags);
-}
-
static void ipmmu_domain_destroy_context(struct ipmmu_vmsa_domain *domain)
{
/*
--
2.7.4
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
