Re: Possible error in dtrace
Le 25/07/2018 à 19:56, Siddharth Muralee a écrit : I guess that makes sense. Should I submit a patch for the same as you suggested? I've committed it. But in the end I used VM_MIN_KERNEL_ADDRESS, because the variable being "kernelbase", it was less confusing. I guess we could remove the variable and inline its content, plain and simple.
Re: Possible error in dtrace
On Wed, Jul 25, 2018, 7:48 PM Maxime Villard wrote: > Le 25/07/2018 à 16:09, Kamil Rytarowski a écrit : > > On 25.07.2018 16:01, Maxime Villard wrote: > >> Le 25/07/2018 à 15:54, Siddharth Muralee a écrit : > >>> Hello, > >>> While going through sys/arch/amd64/pmap.h, I came across > >>> KERN_BASE[1] and confused it for KERNBASE[2]. Looking deeper into the > >>> same I think someone made the same mistake in dtrace_isa.c{3](I might > >>> be wrong). > >>> I compared how the same variable was used in amd64[4] and > >>> i386[5]. There seems to be a mismatch. I computed the values for > >>> KERN_BASE and KERNBASE for amd64 and > >>> > >>>* KERN_BASE = 0x8000 > >>>* KERNBASE = 0x8000 > >>> > >>> I think this may be an error. However, the names are definitely a bit > >>> confusing and there is no documentation to suggest what they are for. > >> > >> They are not the same. KERNBASE is the base where .text etc are located. > >> KERN_BASE is where the kernel map begins (VM_MIN_KERNEL_ADDRESS). > >> > >> In fact, KERN_BASE is incorrect, there should be a VA_SIGN_NEG() around > the > >> multiplication. I've always known this, but I don't know if it's really > >> harmful, so by default I've never fixed it. > > > > I propose to either rename or fix KERN_BASE according to VA_SIGN_NET(). > > > > These names are confusing. > > The confusion likely comes from i386. On i386, VM_MIN_KERNEL_ADDRESS and > KERNBASE are equal. > > The only dtrace user I see is dtrace_toxic_ranges(). I'm not sure what's > going on down there, but I think we want to exclude all of userland. So > the call should probably just be: > > (*func)(0, VM_MAX_ADDRESS) > > Then we remove KERN_BASE. > KERN_BASE is used only once in dtrace and no where else - if it indeed is the same as VM_MIN_KERNEL_ADDRESS then we can just substitute it. I didn't understand how VM_MAX_ADDRESS comes there. Also why is KERNBASE > VM_MAX_KERNEL_ADDRESS in amd64 and equal to VM_MIN_KERNEL_ADDRESS in i386(or did you mean VM_MAX_KERNEL_ADDRESS)
Re: Possible error in dtrace
I guess that makes sense. Should I submit a patch for the same as you suggested? On Wed, Jul 25, 2018, 11:15 PM Maxime Villard wrote: > Le 25/07/2018 à 19:33, Siddharth Muralee a écrit : > > KERN_BASE is used only once in dtrace and no where else - if it indeed is > > the same as VM_MIN_KERNEL_ADDRESS then we can just substitute it. > > I didn't understand how VM_MAX_ADDRESS comes there. > > Instead of taking the kernel min address, you can take the userland max > address. This userland max address is VM_MAX_ADDRESS. > > > Also why is KERNBASE > VM_MAX_KERNEL_ADDRESS in amd64 and equal to > > VM_MIN_KERNEL_ADDRESS in i386(or did you mean VM_MAX_KERNEL_ADDRESS) > > I did mean VM_MIN_KERNEL_ADDRESS. The reason is that on amd64 we are > required to put the kernel at the last 2GB of the VA space, because of the > mcmodel. So we have KERNBASE that is near the end of the VA space, and > VM_MIN_KERNEL_ADDRESS that is located before. The two areas are separate. > > On i386 however we can put the kernel wherever we want, and by default we > chose to put it at VM_MIN_KERNEL_ADDRESS. So there, VM_MIN_KERNEL_ADDRESS > equals KERNBASE. >
Re: Possible error in dtrace
Le 25/07/2018 à 19:33, Siddharth Muralee a écrit : KERN_BASE is used only once in dtrace and no where else - if it indeed is the same as VM_MIN_KERNEL_ADDRESS then we can just substitute it. I didn't understand how VM_MAX_ADDRESS comes there. Instead of taking the kernel min address, you can take the userland max address. This userland max address is VM_MAX_ADDRESS. Also why is KERNBASE > VM_MAX_KERNEL_ADDRESS in amd64 and equal to VM_MIN_KERNEL_ADDRESS in i386(or did you mean VM_MAX_KERNEL_ADDRESS) I did mean VM_MIN_KERNEL_ADDRESS. The reason is that on amd64 we are required to put the kernel at the last 2GB of the VA space, because of the mcmodel. So we have KERNBASE that is near the end of the VA space, and VM_MIN_KERNEL_ADDRESS that is located before. The two areas are separate. On i386 however we can put the kernel wherever we want, and by default we chose to put it at VM_MIN_KERNEL_ADDRESS. So there, VM_MIN_KERNEL_ADDRESS equals KERNBASE.
Re: Possible error in dtrace
Le 25/07/2018 à 16:09, Kamil Rytarowski a écrit : On 25.07.2018 16:01, Maxime Villard wrote: Le 25/07/2018 à 15:54, Siddharth Muralee a écrit : Hello, While going through sys/arch/amd64/pmap.h, I came across KERN_BASE[1] and confused it for KERNBASE[2]. Looking deeper into the same I think someone made the same mistake in dtrace_isa.c{3](I might be wrong). I compared how the same variable was used in amd64[4] and i386[5]. There seems to be a mismatch. I computed the values for KERN_BASE and KERNBASE for amd64 and * KERN_BASE = 0x8000 * KERNBASE = 0x8000 I think this may be an error. However, the names are definitely a bit confusing and there is no documentation to suggest what they are for. They are not the same. KERNBASE is the base where .text etc are located. KERN_BASE is where the kernel map begins (VM_MIN_KERNEL_ADDRESS). In fact, KERN_BASE is incorrect, there should be a VA_SIGN_NEG() around the multiplication. I've always known this, but I don't know if it's really harmful, so by default I've never fixed it. I propose to either rename or fix KERN_BASE according to VA_SIGN_NET(). These names are confusing. The confusion likely comes from i386. On i386, VM_MIN_KERNEL_ADDRESS and KERNBASE are equal. The only dtrace user I see is dtrace_toxic_ranges(). I'm not sure what's going on down there, but I think we want to exclude all of userland. So the call should probably just be: (*func)(0, VM_MAX_ADDRESS) Then we remove KERN_BASE.
Re: Possible error in dtrace
On 25.07.2018 16:01, Maxime Villard wrote: > Le 25/07/2018 à 15:54, Siddharth Muralee a écrit : >> Hello, >> While going through sys/arch/amd64/pmap.h, I came across >> KERN_BASE[1] and confused it for KERNBASE[2]. Looking deeper into the >> same I think someone made the same mistake in dtrace_isa.c{3](I might >> be wrong). >> I compared how the same variable was used in amd64[4] and >> i386[5]. There seems to be a mismatch. I computed the values for >> KERN_BASE and KERNBASE for amd64 and >> >> * KERN_BASE = 0x8000 >> * KERNBASE = 0x8000 >> >> I think this may be an error. However, the names are definitely a bit >> confusing and there is no documentation to suggest what they are for. > > They are not the same. KERNBASE is the base where .text etc are located. > KERN_BASE is where the kernel map begins (VM_MIN_KERNEL_ADDRESS). > > In fact, KERN_BASE is incorrect, there should be a VA_SIGN_NEG() around the > multiplication. I've always known this, but I don't know if it's really > harmful, so by default I've never fixed it. I propose to either rename or fix KERN_BASE according to VA_SIGN_NET(). These names are confusing. signature.asc Description: OpenPGP digital signature
Re: Possible error in dtrace
Le 25/07/2018 à 15:54, Siddharth Muralee a écrit : Hello, While going through sys/arch/amd64/pmap.h, I came across KERN_BASE[1] and confused it for KERNBASE[2]. Looking deeper into the same I think someone made the same mistake in dtrace_isa.c{3](I might be wrong). I compared how the same variable was used in amd64[4] and i386[5]. There seems to be a mismatch. I computed the values for KERN_BASE and KERNBASE for amd64 and * KERN_BASE = 0x8000 * KERNBASE = 0x8000 I think this may be an error. However, the names are definitely a bit confusing and there is no documentation to suggest what they are for. They are not the same. KERNBASE is the base where .text etc are located. KERN_BASE is where the kernel map begins (VM_MIN_KERNEL_ADDRESS). In fact, KERN_BASE is incorrect, there should be a VA_SIGN_NEG() around the multiplication. I've always known this, but I don't know if it's really harmful, so by default I've never fixed it.