* [RFC PATCH v1] iommu/io-pgtable-arm-v7s: Check for leaf entry right after finding it
@ 2017-02-16 13:52 Oleksandr Tyshchenko
2017-02-16 13:58 ` Oleksandr Tyshchenko
2017-02-21 12:00 ` Robin Murphy
0 siblings, 2 replies; 5+ messages in thread
From: Oleksandr Tyshchenko @ 2017-02-16 13:52 UTC (permalink / raw)
To: linux-arm-kernel
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Do a check for already installed leaf entry at the current level before
performing any actions when trying to map.
This check is already present in arm_v7s_init_pte(), i.e. before
installing new leaf entry at the current level if conditions to do so
are met (num_entries > 0).
But, this might be insufficient in case when we have already
installed block mapping at this level and it is not time to
install new leaf entry (num_entries == 0).
In that case we continue walking the page table down with wrong pointer
to the next level.
So, move check from arm_v7s_init_pte() to __arm_v7s_map() in order to
avoid all cases.
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
This patch does similar things as the patch I have pushed a week ago
for long-descriptor format [1].
The reason why this patch is RFC is because I am not sure I did the right
thing and I even didn't test it.
[1] https://lists.linuxfoundation.org/pipermail/iommu/2017-February/020411.html
---
---
drivers/iommu/io-pgtable-arm-v7s.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/iommu/io-pgtable-arm-v7s.c b/drivers/iommu/io-pgtable-arm-v7s.c
index f50e51c..7f7594b 100644
--- a/drivers/iommu/io-pgtable-arm-v7s.c
+++ b/drivers/iommu/io-pgtable-arm-v7s.c
@@ -364,10 +364,6 @@ static int arm_v7s_init_pte(struct arm_v7s_io_pgtable *data,
if (WARN_ON(__arm_v7s_unmap(data, iova + i * sz,
sz, lvl, tblp) != sz))
return -EINVAL;
- } else if (ptep[i]) {
- /* We require an unmap first */
- WARN_ON(!selftest_running);
- return -EEXIST;
}
pte |= ARM_V7S_PTE_TYPE_PAGE;
@@ -392,11 +388,20 @@ static int __arm_v7s_map(struct arm_v7s_io_pgtable *data, unsigned long iova,
{
struct io_pgtable_cfg *cfg = &data->iop.cfg;
arm_v7s_iopte pte, *cptep;
- int num_entries = size >> ARM_V7S_LVL_SHIFT(lvl);
+ int i = 0, num_entries = size >> ARM_V7S_LVL_SHIFT(lvl);
/* Find our entry at the current level */
ptep += ARM_V7S_LVL_IDX(iova, lvl);
+ /* Check for already installed leaf entry */
+ do {
+ if (ptep[i] && !ARM_V7S_PTE_IS_TABLE(ptep[i], lvl)) {
+ /* We require an unmap first */
+ WARN_ON(!selftest_running);
+ return -EEXIST;
+ }
+ } while (++i < num_entries);
+
/* If we can install a leaf entry@this level, then do so */
if (num_entries)
return arm_v7s_init_pte(data, iova, paddr, prot,
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [RFC PATCH v1] iommu/io-pgtable-arm-v7s: Check for leaf entry right after finding it
2017-02-16 13:52 [RFC PATCH v1] iommu/io-pgtable-arm-v7s: Check for leaf entry right after finding it Oleksandr Tyshchenko
@ 2017-02-16 13:58 ` Oleksandr Tyshchenko
2017-02-21 12:00 ` Robin Murphy
1 sibling, 0 replies; 5+ messages in thread
From: Oleksandr Tyshchenko @ 2017-02-16 13:58 UTC (permalink / raw)
To: linux-arm-kernel
Hi, Robin.
It would be greatly appreciated if you could test this patch if nobody
has objections to the patch itself.
On Thu, Feb 16, 2017 at 3:52 PM, Oleksandr Tyshchenko
<olekstysh@gmail.com> wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> Do a check for already installed leaf entry at the current level before
> performing any actions when trying to map.
>
> This check is already present in arm_v7s_init_pte(), i.e. before
> installing new leaf entry at the current level if conditions to do so
> are met (num_entries > 0).
>
> But, this might be insufficient in case when we have already
> installed block mapping at this level and it is not time to
> install new leaf entry (num_entries == 0).
> In that case we continue walking the page table down with wrong pointer
> to the next level.
>
> So, move check from arm_v7s_init_pte() to __arm_v7s_map() in order to
> avoid all cases.
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
> This patch does similar things as the patch I have pushed a week ago
> for long-descriptor format [1].
> The reason why this patch is RFC is because I am not sure I did the right
> thing and I even didn't test it.
>
> [1] https://lists.linuxfoundation.org/pipermail/iommu/2017-February/020411.html
> ---
> ---
> drivers/iommu/io-pgtable-arm-v7s.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iommu/io-pgtable-arm-v7s.c b/drivers/iommu/io-pgtable-arm-v7s.c
> index f50e51c..7f7594b 100644
> --- a/drivers/iommu/io-pgtable-arm-v7s.c
> +++ b/drivers/iommu/io-pgtable-arm-v7s.c
> @@ -364,10 +364,6 @@ static int arm_v7s_init_pte(struct arm_v7s_io_pgtable *data,
> if (WARN_ON(__arm_v7s_unmap(data, iova + i * sz,
> sz, lvl, tblp) != sz))
> return -EINVAL;
> - } else if (ptep[i]) {
> - /* We require an unmap first */
> - WARN_ON(!selftest_running);
> - return -EEXIST;
> }
>
> pte |= ARM_V7S_PTE_TYPE_PAGE;
> @@ -392,11 +388,20 @@ static int __arm_v7s_map(struct arm_v7s_io_pgtable *data, unsigned long iova,
> {
> struct io_pgtable_cfg *cfg = &data->iop.cfg;
> arm_v7s_iopte pte, *cptep;
> - int num_entries = size >> ARM_V7S_LVL_SHIFT(lvl);
> + int i = 0, num_entries = size >> ARM_V7S_LVL_SHIFT(lvl);
>
> /* Find our entry at the current level */
> ptep += ARM_V7S_LVL_IDX(iova, lvl);
>
> + /* Check for already installed leaf entry */
> + do {
> + if (ptep[i] && !ARM_V7S_PTE_IS_TABLE(ptep[i], lvl)) {
> + /* We require an unmap first */
> + WARN_ON(!selftest_running);
> + return -EEXIST;
> + }
> + } while (++i < num_entries);
> +
> /* If we can install a leaf entry at this level, then do so */
> if (num_entries)
> return arm_v7s_init_pte(data, iova, paddr, prot,
> --
> 2.7.4
>
--
Regards,
Oleksandr Tyshchenko
^ permalink raw reply [flat|nested] 5+ messages in thread
* [RFC PATCH v1] iommu/io-pgtable-arm-v7s: Check for leaf entry right after finding it
2017-02-16 13:52 [RFC PATCH v1] iommu/io-pgtable-arm-v7s: Check for leaf entry right after finding it Oleksandr Tyshchenko
2017-02-16 13:58 ` Oleksandr Tyshchenko
@ 2017-02-21 12:00 ` Robin Murphy
2017-02-21 13:31 ` Oleksandr Tyshchenko
1 sibling, 1 reply; 5+ messages in thread
From: Robin Murphy @ 2017-02-21 12:00 UTC (permalink / raw)
To: linux-arm-kernel
On 16/02/17 13:52, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> Do a check for already installed leaf entry at the current level before
> performing any actions when trying to map.
>
> This check is already present in arm_v7s_init_pte(), i.e. before
> installing new leaf entry at the current level if conditions to do so
> are met (num_entries > 0).
>
> But, this might be insufficient in case when we have already
> installed block mapping at this level and it is not time to
> install new leaf entry (num_entries == 0).
> In that case we continue walking the page table down with wrong pointer
> to the next level.
>
> So, move check from arm_v7s_init_pte() to __arm_v7s_map() in order to
> avoid all cases.
Would it not be more logical (and simpler) to just check that the thing
we dereference is valid to dereference when we dereference it? i.e.:
-----8<-----
diff --git a/drivers/iommu/io-pgtable-arm-v7s.c
b/drivers/iommu/io-pgtable-arm-v7s.c
index 0769276c0537..f3112f9ff494 100644
--- a/drivers/iommu/io-pgtable-arm-v7s.c
+++ b/drivers/iommu/io-pgtable-arm-v7s.c
@@ -418,8 +418,10 @@ static int __arm_v7s_map(struct arm_v7s_io_pgtable
*data, unsigned long iova,
pte |= ARM_V7S_ATTR_NS_TABLE;
__arm_v7s_set_pte(ptep, pte, 1, cfg);
- } else {
+ } else if (ARM_V7S_PTE_IS_TABLE(pte, lvl)) {
cptep = iopte_deref(pte, lvl);
+ } else {
+ return -EEXIST;
}
/* Rinse, repeat */
----->8-----
I think the equivalent could be done in LPAE as well.
Robin.
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
> This patch does similar things as the patch I have pushed a week ago
> for long-descriptor format [1].
> The reason why this patch is RFC is because I am not sure I did the right
> thing and I even didn't test it.
>
> [1] https://lists.linuxfoundation.org/pipermail/iommu/2017-February/020411.html
> ---
> ---
> drivers/iommu/io-pgtable-arm-v7s.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iommu/io-pgtable-arm-v7s.c b/drivers/iommu/io-pgtable-arm-v7s.c
> index f50e51c..7f7594b 100644
> --- a/drivers/iommu/io-pgtable-arm-v7s.c
> +++ b/drivers/iommu/io-pgtable-arm-v7s.c
> @@ -364,10 +364,6 @@ static int arm_v7s_init_pte(struct arm_v7s_io_pgtable *data,
> if (WARN_ON(__arm_v7s_unmap(data, iova + i * sz,
> sz, lvl, tblp) != sz))
> return -EINVAL;
> - } else if (ptep[i]) {
> - /* We require an unmap first */
> - WARN_ON(!selftest_running);
> - return -EEXIST;
> }
>
> pte |= ARM_V7S_PTE_TYPE_PAGE;
> @@ -392,11 +388,20 @@ static int __arm_v7s_map(struct arm_v7s_io_pgtable *data, unsigned long iova,
> {
> struct io_pgtable_cfg *cfg = &data->iop.cfg;
> arm_v7s_iopte pte, *cptep;
> - int num_entries = size >> ARM_V7S_LVL_SHIFT(lvl);
> + int i = 0, num_entries = size >> ARM_V7S_LVL_SHIFT(lvl);
>
> /* Find our entry at the current level */
> ptep += ARM_V7S_LVL_IDX(iova, lvl);
>
> + /* Check for already installed leaf entry */
> + do {
> + if (ptep[i] && !ARM_V7S_PTE_IS_TABLE(ptep[i], lvl)) {
> + /* We require an unmap first */
> + WARN_ON(!selftest_running);
> + return -EEXIST;
> + }
> + } while (++i < num_entries);
> +
> /* If we can install a leaf entry at this level, then do so */
> if (num_entries)
> return arm_v7s_init_pte(data, iova, paddr, prot,
>
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [RFC PATCH v1] iommu/io-pgtable-arm-v7s: Check for leaf entry right after finding it
2017-02-21 12:00 ` Robin Murphy
@ 2017-02-21 13:31 ` Oleksandr Tyshchenko
2017-02-24 15:18 ` Will Deacon
0 siblings, 1 reply; 5+ messages in thread
From: Oleksandr Tyshchenko @ 2017-02-21 13:31 UTC (permalink / raw)
To: linux-arm-kernel
Hi, Robin.
On Tue, Feb 21, 2017 at 2:00 PM, Robin Murphy <robin.murphy@arm.com> wrote:
> On 16/02/17 13:52, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> Do a check for already installed leaf entry at the current level before
>> performing any actions when trying to map.
>>
>> This check is already present in arm_v7s_init_pte(), i.e. before
>> installing new leaf entry at the current level if conditions to do so
>> are met (num_entries > 0).
>>
>> But, this might be insufficient in case when we have already
>> installed block mapping at this level and it is not time to
>> install new leaf entry (num_entries == 0).
>> In that case we continue walking the page table down with wrong pointer
>> to the next level.
>>
>> So, move check from arm_v7s_init_pte() to __arm_v7s_map() in order to
>> avoid all cases.
>
> Would it not be more logical (and simpler) to just check that the thing
> we dereference is valid to dereference when we dereference it? i.e.:
>
> -----8<-----
> diff --git a/drivers/iommu/io-pgtable-arm-v7s.c
> b/drivers/iommu/io-pgtable-arm-v7s.c
> index 0769276c0537..f3112f9ff494 100644
> --- a/drivers/iommu/io-pgtable-arm-v7s.c
> +++ b/drivers/iommu/io-pgtable-arm-v7s.c
> @@ -418,8 +418,10 @@ static int __arm_v7s_map(struct arm_v7s_io_pgtable
> *data, unsigned long iova,
> pte |= ARM_V7S_ATTR_NS_TABLE;
>
> __arm_v7s_set_pte(ptep, pte, 1, cfg);
> - } else {
> + } else if (ARM_V7S_PTE_IS_TABLE(pte, lvl)) {
> cptep = iopte_deref(pte, lvl);
> + } else {
> + return -EEXIST;
> }
>
> /* Rinse, repeat */
> ----->8-----
Agree. Sounds reasonable.
>
> I think the equivalent could be done in LPAE as well.
OK.
I will resend both modified patches without RFC prefix, right?
>
> Robin.
>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> ---
>> This patch does similar things as the patch I have pushed a week ago
>> for long-descriptor format [1].
>> The reason why this patch is RFC is because I am not sure I did the right
>> thing and I even didn't test it.
>>
>> [1] https://lists.linuxfoundation.org/pipermail/iommu/2017-February/020411.html
>> ---
>> ---
>> drivers/iommu/io-pgtable-arm-v7s.c | 15 ++++++++++-----
>> 1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/iommu/io-pgtable-arm-v7s.c b/drivers/iommu/io-pgtable-arm-v7s.c
>> index f50e51c..7f7594b 100644
>> --- a/drivers/iommu/io-pgtable-arm-v7s.c
>> +++ b/drivers/iommu/io-pgtable-arm-v7s.c
>> @@ -364,10 +364,6 @@ static int arm_v7s_init_pte(struct arm_v7s_io_pgtable *data,
>> if (WARN_ON(__arm_v7s_unmap(data, iova + i * sz,
>> sz, lvl, tblp) != sz))
>> return -EINVAL;
>> - } else if (ptep[i]) {
>> - /* We require an unmap first */
>> - WARN_ON(!selftest_running);
>> - return -EEXIST;
>> }
>>
>> pte |= ARM_V7S_PTE_TYPE_PAGE;
>> @@ -392,11 +388,20 @@ static int __arm_v7s_map(struct arm_v7s_io_pgtable *data, unsigned long iova,
>> {
>> struct io_pgtable_cfg *cfg = &data->iop.cfg;
>> arm_v7s_iopte pte, *cptep;
>> - int num_entries = size >> ARM_V7S_LVL_SHIFT(lvl);
>> + int i = 0, num_entries = size >> ARM_V7S_LVL_SHIFT(lvl);
>>
>> /* Find our entry at the current level */
>> ptep += ARM_V7S_LVL_IDX(iova, lvl);
>>
>> + /* Check for already installed leaf entry */
>> + do {
>> + if (ptep[i] && !ARM_V7S_PTE_IS_TABLE(ptep[i], lvl)) {
>> + /* We require an unmap first */
>> + WARN_ON(!selftest_running);
>> + return -EEXIST;
>> + }
>> + } while (++i < num_entries);
>> +
>> /* If we can install a leaf entry at this level, then do so */
>> if (num_entries)
>> return arm_v7s_init_pte(data, iova, paddr, prot,
>>
>
--
Regards,
Oleksandr Tyshchenko
^ permalink raw reply [flat|nested] 5+ messages in thread
* [RFC PATCH v1] iommu/io-pgtable-arm-v7s: Check for leaf entry right after finding it
2017-02-21 13:31 ` Oleksandr Tyshchenko
@ 2017-02-24 15:18 ` Will Deacon
0 siblings, 0 replies; 5+ messages in thread
From: Will Deacon @ 2017-02-24 15:18 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Feb 21, 2017 at 03:31:26PM +0200, Oleksandr Tyshchenko wrote:
> On Tue, Feb 21, 2017 at 2:00 PM, Robin Murphy <robin.murphy@arm.com> wrote:
> > Would it not be more logical (and simpler) to just check that the thing
> > we dereference is valid to dereference when we dereference it? i.e.:
> >
> > -----8<-----
> > diff --git a/drivers/iommu/io-pgtable-arm-v7s.c
> > b/drivers/iommu/io-pgtable-arm-v7s.c
> > index 0769276c0537..f3112f9ff494 100644
> > --- a/drivers/iommu/io-pgtable-arm-v7s.c
> > +++ b/drivers/iommu/io-pgtable-arm-v7s.c
> > @@ -418,8 +418,10 @@ static int __arm_v7s_map(struct arm_v7s_io_pgtable
> > *data, unsigned long iova,
> > pte |= ARM_V7S_ATTR_NS_TABLE;
> >
> > __arm_v7s_set_pte(ptep, pte, 1, cfg);
> > - } else {
> > + } else if (ARM_V7S_PTE_IS_TABLE(pte, lvl)) {
> > cptep = iopte_deref(pte, lvl);
> > + } else {
> > + return -EEXIST;
> > }
> >
> > /* Rinse, repeat */
> > ----->8-----
>
> Agree. Sounds reasonable.
>
> >
> > I think the equivalent could be done in LPAE as well.
>
> OK.
>
> I will resend both modified patches without RFC prefix, right?
Sounds good to me.
Will
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-02-24 15:18 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-02-16 13:52 [RFC PATCH v1] iommu/io-pgtable-arm-v7s: Check for leaf entry right after finding it Oleksandr Tyshchenko
2017-02-16 13:58 ` Oleksandr Tyshchenko
2017-02-21 12:00 ` Robin Murphy
2017-02-21 13:31 ` Oleksandr Tyshchenko
2017-02-24 15:18 ` Will Deacon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).