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
>
>
