Re: [PATCH v9 03/16] iommu/exynos: fix page table maintenance

[Cho KyongHo]

On Fri, 09 Aug 2013 09:47:28 +0200, Tomasz Figa wrote:
> Hi KyongHo,
> 
> On Friday 09 of August 2013 13:15:20 Cho KyongHo wrote:
> > On Thu, 08 Aug 2013 15:54:50 +0200, Tomasz Figa wrote:
> > > On Thursday 08 of August 2013 18:37:43 Cho KyongHo wrote:
> > > > This prevents allocating lv2 page table for the lv1 page table entry
> > > > 
> > >   ^ What this is this this about? :)
> > 
> > As you might indicate, 'this' means this patch :)
> 
> Yep, I was just nitpicking, but still I would go with something among 
> following lines:
> 
> 8<---
> Currently if lv2 page table allocation is requested for a lv1 page table 
> entry that already has 1MB page mapping, the driver returns -EADDRINUSE. 
> However this case should not happen, unless there is a bug in the generic 
> IOMMU allocator, so BUG_ON() is the right error handling here, which is 
> implemented by this patch.
> --->8
> 
> > > > that already has 1MB page mapping. In addition, changed to BUG_ON
> > > > instead of returning -EADDRINUSE.
> > > 
> > > The change mentioned in last sentence should be a separate patch.
> > 
> > Ok :)
> 
> Well, actually I was confused by subject of this patch.
> 
> It looks like this change is actually the main part of it and the only 
> unrelated changes are using defined macros for page masks and introduction 
> of clear_page_table() function. Since we both agreed that the latter can 
> be dropped only the former should be separated from this patch.
> 
> Maybe you could use following patch subject:
> 
> iommu/exynos: fix handling of possible BUG cases in page table setup code
> 

Your title looks much better. Thanks.

Initially, this patch is created for handling -EADDRINUSE correctly but
it is changed :). I missed to change the subject.

> > > > Signed-off-by: Cho KyongHo <pullip.cho@xxxxxxxxxxx>
> > > > ---
> > > > 
> > > >  drivers/iommu/exynos-iommu.c |   68
> > > > 
> > > > ++++++++++++++++++++++++----------------- 1 files changed, 40
> > > > insertions(+), 28 deletions(-)
> > > > 
> > > > diff --git a/drivers/iommu/exynos-iommu.c
> > > > b/drivers/iommu/exynos-iommu.c index d545a25..d90e6fa 100644
> > > > --- a/drivers/iommu/exynos-iommu.c
> > > > +++ b/drivers/iommu/exynos-iommu.c
> > > > @@ -52,11 +52,11 @@
> > > > 
> > > >  #define lv2ent_large(pent) ((*(pent) & 3) == 1)
> > > >  
> > > >  #define section_phys(sent) (*(sent) & SECT_MASK)
> > > > 
> > > > -#define section_offs(iova) ((iova) & 0xFFFFF)
> > > > +#define section_offs(iova) ((iova) & ~SECT_MASK)
> > > > 
> > > >  #define lpage_phys(pent) (*(pent) & LPAGE_MASK)
> > > > 
> > > > -#define lpage_offs(iova) ((iova) & 0xFFFF)
> > > > +#define lpage_offs(iova) ((iova) & ~LPAGE_MASK)
> > > > 
> > > >  #define spage_phys(pent) (*(pent) & SPAGE_MASK)
> > > > 
> > > > -#define spage_offs(iova) ((iova) & 0xFFF)
> > > > +#define spage_offs(iova) ((iova) & ~SPAGE_MASK)
> > > > 
> > > >  #define lv1ent_offset(iova) ((iova) >> SECT_ORDER)
> > > >  #define lv2ent_offset(iova) (((iova) & 0xFF000) >> SPAGE_ORDER)
> > > > 
> > > > @@ -856,13 +856,15 @@ finish:
> > > >  static unsigned long *alloc_lv2entry(unsigned long *sent, unsigned
> > > >  long
> > > > 
> > > > iova, short *pgcounter)
> > > > 
> > > >  {
> > > > 
> > > > +	BUG_ON(lv1ent_section(sent));
> > > 
> > > Is this condition really a critical one, to the point that the system
> > > should not continue execution?
> > 
> > Discussed with Grant. He thought that creating mapping on a valid
> > mapping is just a BUG and I finally agreed with him. Is there a case
> > that the condition in BUG_ON is true intentionally?
> 
> Yes, he explained this as well. It's fine for me then.
> 
> > > > +
> > > > 
> > > >  	if (lv1ent_fault(sent)) {
> > > >  	
> > > >  		unsigned long *pent;
> > > >  		
> > > >  		pent = kzalloc(LV2TABLE_SIZE, GFP_ATOMIC);
> > > >  		BUG_ON((unsigned long)pent & (LV2TABLE_SIZE - 1));
> > > >  		if (!pent)
> > > > 
> > > > -			return NULL;
> > > > +			return ERR_PTR(-ENOMEM);
> > > > 
> > > >  		*sent = mk_lv1ent_page(__pa(pent));
> > > >  		*pgcounter = NUM_LV2ENTRIES;
> > > > 
> > > > @@ -875,15 +877,11 @@ static unsigned long *alloc_lv2entry(unsigned
> > > > long *sent, unsigned long iova,
> > > > 
> > > >  static int lv1set_section(unsigned long *sent, phys_addr_t paddr,
> > > >  short
> > > > 
> > > > *pgcnt) {
> > > > -	if (lv1ent_section(sent))
> > > > -		return -EADDRINUSE;
> > > > +	BUG_ON(lv1ent_section(sent));
> > > 
> > > Ditto.
> > > 
> > > >  	if (lv1ent_page(sent)) {
> > > > 
> > > > -		if (*pgcnt != NUM_LV2ENTRIES)
> > > > -			return -EADDRINUSE;
> > > > -
> > > > +		BUG_ON(*pgcnt != NUM_LV2ENTRIES);
> > > 
> > > Ditto.
> > > 
> > > >  		kfree(page_entry(sent, 0));
> > > > 
> > > > -
> > > > 
> > > >  		*pgcnt = 0;
> > > >  	
> > > >  	}
> > > > 
> > > > @@ -894,24 +892,24 @@ static int lv1set_section(unsigned long *sent,
> > > > phys_addr_t paddr, short *pgcnt) return 0;
> > > > 
> > > >  }
> > > > 
> > > > +static void clear_page_table(unsigned long *ent, int n)
> > > > +{
> > > > +	if (n > 0)
> > > > +		memset(ent, 0, sizeof(*ent) * n);
> > > > +}
> > > 
> > > I don't see the point of creating this function. It seems to be used
> > > only once, in addition with a constant as n, so the check for n > 0
> > > is unnecessary.
> > > 
> > > And even if there is a need for this change, it should be done in
> > > separate patch, as this one is not about stylistic changes, but
> > > fixing page table maintenance (at least based on your commit
> > > message).
> > 
> > I know what you are concerning about.
> > It was introduced in v8 patches to recover previous fault entries before
> > returning -EADDRINUSE. It is still remained though "return -EADDRINUSE"
> > is changed into BUG_ON().
> > I also think that it needs to be removed.
> 
> OK, thanks.
> 
> Best regards,
> Tomasz
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/