Re: [PATCH v3 4/5] x86/mm: optimize static_protection() by using overlap()

[Yang, Bin]

On Tue, 2018-09-04 at 14:22 +0200, Thomas Gleixner wrote:
> On Tue, 21 Aug 2018, Bin Yang wrote:
> >  
> > +static inline bool
> > +overlap(unsigned long start1, unsigned long end1,
> > +		unsigned long start2, unsigned long end2)
> > +{
> > +	/* Is 'start2' within area 1? */
> > +	if (start1 <= start2 && end1 > start2)
> > +		return true;
> > +
> > +	/* Is 'start1' within area 2? */
> > +	if (start2 <= start1 && end2 > start1)
> > +		return true;
> > +
> > +	return false;
> >  static inline unsigned long highmap_start_pfn(void)
> > @@ -293,7 +308,7 @@ static void cpa_flush_array(unsigned long *start, int numpages, int cache,
> >   * checks and fixes these known static required protection bits.
> >   */
> >  static inline pgprot_t static_protections(pgprot_t prot, unsigned long address,
> > -				   unsigned long pfn)
> > +				   unsigned long len, unsigned long pfn)
> >  {
> >  	pgprot_t forbidden = __pgprot(0);
> >  
> > @@ -302,7 +317,9 @@ static inline pgprot_t static_protections(pgprot_t prot, unsigned long address,
> >  	 * PCI BIOS based config access (CONFIG_PCI_GOBIOS) support.
> >  	 */
> >  #ifdef CONFIG_PCI_BIOS
> > -	if (pcibios_enabled && within(pfn, BIOS_BEGIN >> PAGE_SHIFT, BIOS_END >> PAGE_SHIFT))
> > +	if (pcibios_enabled &&
> > +	    overlap(pfn, pfn + PFN_DOWN(len),
> > +		    PFN_DOWN(BIOS_BEGIN), PFN_DOWN(BIOS_END)))
> 
> This is completely unreadable and aside of that it is wrong. You cannot do
> an overlap check with the following constraints:
> 
>    	   range1_end = range1_start + size;
>    	   range2_end = range2_start + size;
> 
> See the definition of BIOS_END. It's 0x100000, i.e. 1MB, so the following
> overlap check will give you the false result:
> 
> 	overlap(256, 258, 0x000a0000 >> 12, 0x0010000 >> 12)
> 
> because
> 
> 	0x0010000 >> 12 = 256
> 
> ergo will overlap return true. All of your overlap checks are broken.
> 
> Oh well.

I just write a test.c to compare the result between overlap() and
original within(). 

8<--------- test.c ----------------

#include <stdio.h>

#define bool int
#define true 1
#define false 0
#define PAGE_SHIFT 12
#define BIOS_BEGIN              0x000a0000
#define BIOS_END                0x00100000 

static inline int
within(unsigned long addr, unsigned long start, unsigned long end)
{
        printf("addr=%ld, start=%ld, end=%ld\n",
                addr, start, end);

        return addr >= start && addr < end;
}

static inline bool
overlap(unsigned long start1, unsigned long end1,
             unsigned long start2, unsigned long end2)
{
        printf("start1=%ld, end1=%ld, start2=%ld, end2=%ld\n",
                start1, end1, start2, end2);

        /* Is 'start2' within area 1? */
        if (start1 <= start2 && end1 > start2)
                return true;

        /* Is 'start1' within area 2? */
        if (start2 <= start1 && end2 > start1)
                return true;

        return false;
}

int main(void)
{       
        int ret;
        int pfn;
        
        for (pfn = 256; pfn < 258; pfn ++) { 
                ret = within(pfn, BIOS_BEGIN >> PAGE_SHIFT, BIOS_END >>
PAGE_SHIFT);
                printf("pfn = %d, within() return: %d\n", pfn, ret);
        }
        
        ret = overlap(256, 258, BIOS_BEGIN >> PAGE_SHIFT, BIOS_END >>
PAGE_SHIFT);
        printf("overlap() return: %d\n", ret);
}


8<------ output ------

addr=256, start=160, end=256
pfn = 256, within() return: 0
addr=257, start=160, end=256
pfn = 257, within() return: 0
start1=256, end1=258, start2=160, end2=256
overlap() return: 0


it looks the overlap() result is same as original one.


> 
> Thanks,
> 
> 	tglx
> 
>