| Message ID | 1419246673-7222-1-git-send-email-oded.gabbay@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Dec 22, 2014 at 6:11 AM, Oded Gabbay <oded.gabbay@amd.com> wrote: > amdkfd driver can be compiled only in 64-bit kernel. Therefore, there is no > point in trying to initialize amdkfd in 32-bit kernel. > > In addition, in case of specific configuration of 32-bit kernel, no modules and > random kernel base, the symbol_request function doesn't work as expected - It > doesn't return NULL if the symbol doesn't exists. That makes the kernel panic. > Therefore, the as amdkfd doesn't compile in 32-bit kernel, the best way is just > to return false immediately. > > Signed-off-by: Oded Gabbay <oded.gabbay@amd.com> Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > --- > drivers/gpu/drm/radeon/radeon_kfd.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/radeon/radeon_kfd.c b/drivers/gpu/drm/radeon/radeon_kfd.c > index 242fd8b..cb77e5c 100644 > --- a/drivers/gpu/drm/radeon/radeon_kfd.c > +++ b/drivers/gpu/drm/radeon/radeon_kfd.c > @@ -101,6 +101,7 @@ static const struct kgd2kfd_calls *kgd2kfd; > > bool radeon_kfd_init(void) > { > +#ifdef CONFIG_X86_64 > bool (*kgd2kfd_init_p)(unsigned, const struct kfd2kgd_calls*, > const struct kgd2kfd_calls**); > > @@ -117,6 +118,9 @@ bool radeon_kfd_init(void) > } > > return true; > +#else > + return false; > +#endif > } > > void radeon_kfd_fini(void) > -- > 1.9.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, Dec 22, 2014 at 11:58:43AM -0500, Alex Deucher wrote: > On Mon, Dec 22, 2014 at 6:11 AM, Oded Gabbay <oded.gabbay@amd.com> wrote: > > amdkfd driver can be compiled only in 64-bit kernel. Therefore, there is no > > point in trying to initialize amdkfd in 32-bit kernel. > > > > In addition, in case of specific configuration of 32-bit kernel, no modules and > > random kernel base, the symbol_request function doesn't work as expected - It > > doesn't return NULL if the symbol doesn't exists. That makes the kernel panic. > > Therefore, the as amdkfd doesn't compile in 32-bit kernel, the best way is just > > to return false immediately. > > > > Signed-off-by: Oded Gabbay <oded.gabbay@amd.com> > > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> Sorry but the patch is just bogus. X-bit only code is usually a very bad sign for the code. This is not windows programing after all. Even if you wanted to do a 64bit only driver -- which you probably don't -- the standard way would be to exclude it in Kconfig. Please root-cause why symbol_request doesn't work on 32bit and fix it properly. +rusty. -Andi
On Mon, Dec 22, 2014 at 10:49:40AM -0800, Andi Kleen wrote: > On Mon, Dec 22, 2014 at 11:58:43AM -0500, Alex Deucher wrote: > > On Mon, Dec 22, 2014 at 6:11 AM, Oded Gabbay <oded.gabbay@amd.com> wrote: > > > amdkfd driver can be compiled only in 64-bit kernel. Therefore, there is no > > > point in trying to initialize amdkfd in 32-bit kernel. > > > > > > In addition, in case of specific configuration of 32-bit kernel, no modules and > > > random kernel base, the symbol_request function doesn't work as expected - It > > > doesn't return NULL if the symbol doesn't exists. That makes the kernel panic. > > > Therefore, the as amdkfd doesn't compile in 32-bit kernel, the best way is just > > > to return false immediately. > > > > > > Signed-off-by: Oded Gabbay <oded.gabbay@amd.com> > > > > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > > Sorry but the patch is just bogus. X-bit only code is usually > a very bad sign for the code. This is not windows programing after all. > > Even if you wanted to do a 64bit only driver -- which > you probably don't -- the standard way would be to exclude > it in Kconfig. > > Please root-cause why symbol_request doesn't work on 32bit > and fix it properly. > > +rusty. And also with correct email. -Andi
On 12/22/2014 09:00 PM, Andi Kleen wrote: > On Mon, Dec 22, 2014 at 10:49:40AM -0800, Andi Kleen wrote: >> On Mon, Dec 22, 2014 at 11:58:43AM -0500, Alex Deucher wrote: >>> On Mon, Dec 22, 2014 at 6:11 AM, Oded Gabbay <oded.gabbay@amd.com> wrote: >>>> amdkfd driver can be compiled only in 64-bit kernel. Therefore, there is no >>>> point in trying to initialize amdkfd in 32-bit kernel. >>>> >>>> In addition, in case of specific configuration of 32-bit kernel, no modules and >>>> random kernel base, the symbol_request function doesn't work as expected - It >>>> doesn't return NULL if the symbol doesn't exists. That makes the kernel panic. >>>> Therefore, the as amdkfd doesn't compile in 32-bit kernel, the best way is just >>>> to return false immediately. >>>> >>>> Signed-off-by: Oded Gabbay <oded.gabbay@amd.com> >>> >>> Reviewed-by: Alex Deucher <alexander.deucher@amd.com> >> >> Sorry but the patch is just bogus. X-bit only code is usually >> a very bad sign for the code. This is not windows programing after all. Hi Andi, Strange, I have never programmed for Windows in my life (except maybe in a few courses during my degree) :) >> >> Even if you wanted to do a 64bit only driver -- which >> you probably don't -- the standard way would be to exclude >> it in Kconfig. So amdkfd actually *only* supports 64bit user processes, because AMD's HSA stack on Linux supports *only* 64bit user processes. So, yes, I definitely want to do a 64bit only driver. If you look at kfd_open(), it fails the open of /dev/kfd if the process is 32bit. In addition, in Kconfig of amdkfd, it is written: "depends on DRM_RADEON && AMD_IOMMU_V2 && X86_64" The problem here is that there is code in radeon, which is a driver that can compile in 32bit, which tries to load amdkfd. I didn't see a point in trying to load a driver which can't be compiled in 32bit. >> >> Please root-cause why symbol_request doesn't work on 32bit >> and fix it properly. I didn't say it doesn't always work. The actual thing that doesn't work is the define symbol_get and only in a specific case of 32bit kernel AND CONFIG_MODULES is unset AND CONFIG_RANDOMIZE_BASE is set. The define in that case is: #define symbol_get(x) ({ extern typeof(x) x __attribute__((weak)); &(x); }) Why it doesn't work (doesn't return NULL when symbol doesn't exists) ? I don't know, probably because of some elf/makefile/c language magic. I'm not that big of an expert on those issues, and I wanted to provide a fix for this problem during the -rc stages. If someone can help me solving the root cause, I would be more than happy. Oded >> >> +rusty. > > And also with correct email. > > -Andi >
Am 22.12.2014 um 20:18 schrieb Oded Gabbay: > > On 12/22/2014 09:00 PM, Andi Kleen wrote: >> On Mon, Dec 22, 2014 at 10:49:40AM -0800, Andi Kleen wrote: >>> On Mon, Dec 22, 2014 at 11:58:43AM -0500, Alex Deucher wrote: >>>> On Mon, Dec 22, 2014 at 6:11 AM, Oded Gabbay <oded.gabbay@amd.com> wrote: >>>>> amdkfd driver can be compiled only in 64-bit kernel. Therefore, there is no >>>>> point in trying to initialize amdkfd in 32-bit kernel. >>>>> >>>>> In addition, in case of specific configuration of 32-bit kernel, no modules and >>>>> random kernel base, the symbol_request function doesn't work as expected - It >>>>> doesn't return NULL if the symbol doesn't exists. That makes the kernel panic. >>>>> Therefore, the as amdkfd doesn't compile in 32-bit kernel, the best way is just >>>>> to return false immediately. >>>>> >>>>> Signed-off-by: Oded Gabbay <oded.gabbay@amd.com> >>>> Reviewed-by: Alex Deucher <alexander.deucher@amd.com> >>> Sorry but the patch is just bogus. X-bit only code is usually >>> a very bad sign for the code. This is not windows programing after all. > Hi Andi, > > Strange, I have never programmed for Windows in my life (except maybe in a > few courses during my degree) :) >>> Even if you wanted to do a 64bit only driver -- which >>> you probably don't -- the standard way would be to exclude >>> it in Kconfig. > So amdkfd actually *only* supports 64bit user processes, because AMD's HSA > stack on Linux supports *only* 64bit user processes. So, yes, I definitely > want to do a 64bit only driver. > If you look at kfd_open(), it fails the open of /dev/kfd if the process is > 32bit. > In addition, in Kconfig of amdkfd, it is written: > "depends on DRM_RADEON && AMD_IOMMU_V2 && X86_64" > > The problem here is that there is code in radeon, which is a driver that can > compile in 32bit, which tries to load amdkfd. I didn't see a point in trying > to load a driver which can't be compiled in 32bit. Well in this case couldn't we make the code in radeon depend on whether or not the KFD driver is compiled in or not instead of checking the system architecture? Regards, Christian. > >>> Please root-cause why symbol_request doesn't work on 32bit >>> and fix it properly. > I didn't say it doesn't always work. > The actual thing that doesn't work is the define symbol_get and only in a > specific case of 32bit kernel AND CONFIG_MODULES is unset AND > CONFIG_RANDOMIZE_BASE is set. > The define in that case is: > #define symbol_get(x) ({ extern typeof(x) x __attribute__((weak)); &(x); }) > > Why it doesn't work (doesn't return NULL when symbol doesn't exists) ? > I don't know, probably because of some elf/makefile/c language magic. I'm > not that big of an expert on those issues, and I wanted to provide a fix for > this problem during the -rc stages. If someone can help me solving the root > cause, I would be more than happy. > > Oded >>> +rusty. >> And also with correct email. >> >> -Andi >> > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 12/25/2014 02:31 PM, Christian König wrote: > Am 22.12.2014 um 20:18 schrieb Oded Gabbay: >> >> On 12/22/2014 09:00 PM, Andi Kleen wrote: >>> On Mon, Dec 22, 2014 at 10:49:40AM -0800, Andi Kleen wrote: >>>> On Mon, Dec 22, 2014 at 11:58:43AM -0500, Alex Deucher wrote: >>>>> On Mon, Dec 22, 2014 at 6:11 AM, Oded Gabbay <oded.gabbay@amd.com> wrote: >>>>>> amdkfd driver can be compiled only in 64-bit kernel. Therefore, there is no >>>>>> point in trying to initialize amdkfd in 32-bit kernel. >>>>>> >>>>>> In addition, in case of specific configuration of 32-bit kernel, no >>>>>> modules and >>>>>> random kernel base, the symbol_request function doesn't work as expected - It >>>>>> doesn't return NULL if the symbol doesn't exists. That makes the kernel >>>>>> panic. >>>>>> Therefore, the as amdkfd doesn't compile in 32-bit kernel, the best way is >>>>>> just >>>>>> to return false immediately. >>>>>> >>>>>> Signed-off-by: Oded Gabbay <oded.gabbay@amd.com> >>>>> Reviewed-by: Alex Deucher <alexander.deucher@amd.com> >>>> Sorry but the patch is just bogus. X-bit only code is usually >>>> a very bad sign for the code. This is not windows programing after all. >> Hi Andi, >> >> Strange, I have never programmed for Windows in my life (except maybe in a >> few courses during my degree) :) >>>> Even if you wanted to do a 64bit only driver -- which >>>> you probably don't -- the standard way would be to exclude >>>> it in Kconfig. >> So amdkfd actually *only* supports 64bit user processes, because AMD's HSA >> stack on Linux supports *only* 64bit user processes. So, yes, I definitely >> want to do a 64bit only driver. >> If you look at kfd_open(), it fails the open of /dev/kfd if the process is >> 32bit. >> In addition, in Kconfig of amdkfd, it is written: >> "depends on DRM_RADEON && AMD_IOMMU_V2 && X86_64" >> >> The problem here is that there is code in radeon, which is a driver that can >> compile in 32bit, which tries to load amdkfd. I didn't see a point in trying >> to load a driver which can't be compiled in 32bit. > > Well in this case couldn't we make the code in radeon depend on whether or not > the KFD driver is compiled in or not instead of checking the system architecture? > > Regards, > Christian. > If we are going down that path, we need something like: bool radeon_kfd_init(void) { #if defined(CONFIG_HSA_AMD_MODULE) current code (symbol request and call to symbol) #elif defined(CONFIG_HSA_AMD) direct call to kgd2kfd_init #else return false; #endif } Now, the original concept of the symbol_request call was to prevent writing something like the above pseudo-code, but because symbol_request is not currently working in all cases, I think that this is a good band-aid as any. Oded >> >>>> Please root-cause why symbol_request doesn't work on 32bit >>>> and fix it properly. >> I didn't say it doesn't always work. >> The actual thing that doesn't work is the define symbol_get and only in a >> specific case of 32bit kernel AND CONFIG_MODULES is unset AND >> CONFIG_RANDOMIZE_BASE is set. >> The define in that case is: >> #define symbol_get(x) ({ extern typeof(x) x __attribute__((weak)); &(x); }) >> >> Why it doesn't work (doesn't return NULL when symbol doesn't exists) ? >> I don't know, probably because of some elf/makefile/c language magic. I'm >> not that big of an expert on those issues, and I wanted to provide a fix for >> this problem during the -rc stages. If someone can help me solving the root >> cause, I would be more than happy. >> >> Oded >>>> +rusty. >>> And also with correct email. >>> >>> -Andi >>> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel >
diff --git a/drivers/gpu/drm/radeon/radeon_kfd.c b/drivers/gpu/drm/radeon/radeon_kfd.c index 242fd8b..cb77e5c 100644 --- a/drivers/gpu/drm/radeon/radeon_kfd.c +++ b/drivers/gpu/drm/radeon/radeon_kfd.c @@ -101,6 +101,7 @@ static const struct kgd2kfd_calls *kgd2kfd; bool radeon_kfd_init(void) { +#ifdef CONFIG_X86_64 bool (*kgd2kfd_init_p)(unsigned, const struct kfd2kgd_calls*, const struct kgd2kfd_calls**); @@ -117,6 +118,9 @@ bool radeon_kfd_init(void) } return true; +#else + return false; +#endif } void radeon_kfd_fini(void)
amdkfd driver can be compiled only in 64-bit kernel. Therefore, there is no point in trying to initialize amdkfd in 32-bit kernel. In addition, in case of specific configuration of 32-bit kernel, no modules and random kernel base, the symbol_request function doesn't work as expected - It doesn't return NULL if the symbol doesn't exists. That makes the kernel panic. Therefore, the as amdkfd doesn't compile in 32-bit kernel, the best way is just to return false immediately. Signed-off-by: Oded Gabbay <oded.gabbay@amd.com> --- drivers/gpu/drm/radeon/radeon_kfd.c | 4 ++++ 1 file changed, 4 insertions(+)