Messages in this thread | | | Subject | Re: [PATCH v1 04/10] powerpc/kernel/iommu: Add new iommu_table_in_use() helper | From | Alexey Kardashevskiy <> | Date | Fri, 28 Aug 2020 11:51:11 +1000 |
| |
On 28/08/2020 04:34, Leonardo Bras wrote: > On Sat, 2020-08-22 at 20:34 +1000, Alexey Kardashevskiy wrote: >>> + >>> + /*ignore reserved bit0*/ >> >> s/ignore reserved bit0/ ignore reserved bit0 / (add spaces) > > Fixed > >>> + if (tbl->it_offset == 0) >>> + p1_start = 1; >>> + >>> + /* Check if reserved memory is valid*/ >> >> A missing space here. > > Fixed > >> >>> + if (tbl->it_reserved_start >= tbl->it_offset && >>> + tbl->it_reserved_start <= (tbl->it_offset + tbl->it_size) && >>> + tbl->it_reserved_end >= tbl->it_offset && >>> + tbl->it_reserved_end <= (tbl->it_offset + tbl->it_size)) { >> >> Uff. What if tbl->it_reserved_end is bigger than tbl->it_offset + >> tbl->it_size? >> >> The reserved area is to preserve MMIO32 so it is for it_offset==0 only >> and the boundaries are checked in the only callsite, and it is unlikely >> to change soon or ever. >> >> Rather that bothering with fixing that, may be just add (did not test): >> >> if (WARN_ON(( >> (tbl->it_reserved_start || tbl->it_reserved_end) && (it_offset != 0)) >> (tbl->it_reserved_start > it_offset && tbl->it_reserved_end < it_offset >> + it_size) && (it_offset == 0)) ) >> return true; >> >> Or simply always look for it_offset..it_reserved_start and >> it_reserved_end..it_offset+it_size and if there is no reserved area, >> initialize it_reserved_start=it_reserved_end=it_offset so the first >> it_offset..it_reserved_start becomes a no-op. > > The problem here is that the values of it_reserved_{start,end} are not > necessarily valid. I mean, on iommu_table_reserve_pages() the values > are stored however they are given (bit reserving is done only if they > are valid). > > Having a it_reserved_{start,end} value outside the valid ranges would > cause find_next_bit() to run over memory outside the bitmap. > Even if the those values are < tbl->it_offset, the resulting > subtraction on unsigned would cause it to become a big value and run > over memory outside the bitmap. > > But I think you are right. That is not the place to check if the > reserved values are valid. It should just trust them here. > I intent to change iommu_table_reserve_pages() to only store the > parameters in it_reserved_{start,end} if they are in the range, and or > it_offset in both of them if they are not. > > What do you think?
This should work, yes.
> > Thanks for the feedback! > Leonardo Bras > > >
-- Alexey
| |