| Message ID | 20220102070623.24009-1-rdunlap@infradead.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [1/2] IB/qib: don't use qib_wc_x86_64 for UML | expand |
On Sat, Jan 01, 2022 at 11:06:23PM -0800, Randy Dunlap wrote: > When building rdmavt for ARCH=um, qp.c has a build error on a reference > to the x86-specific cpuinfo field 'x86_cache_size'. This value is then > used to determine whether to use cacheless_memcpy() or not. > Provide a fake value to LLC for CONFIG_UML. Then provide a separate > verison of cacheless_memcpy() for CONFIG_UML that is just a plain > memcpy(), like the calling code uses. > > Prevents these build errors: > > ../drivers/infiniband/sw/rdmavt/qp.c: In function ‘rvt_wss_llc_size’: > ../drivers/infiniband/sw/rdmavt/qp.c:88:23: error: ‘struct cpuinfo_um’ has no member named ‘x86_cache_size’; did you mean ‘x86_capability’? > return boot_cpu_data.x86_cache_size; > > ../drivers/infiniband/sw/rdmavt/qp.c: In function ‘cacheless_memcpy’: > ../drivers/infiniband/sw/rdmavt/qp.c:100:2: error: implicit declaration of function ‘__copy_user_nocache’; did you mean ‘copy_user_page’? [-Werror=implicit-function-declaration] > __copy_user_nocache(dst, (void __user *)src, n, 0); > > Fixes: 68f5d3f3b654 ("um: add PCI over virtio emulation driver") > Signed-off-by: Randy Dunlap <rdunlap@infradead.org> > drivers/infiniband/sw/rdmavt/qp.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > +++ linux-next-20211224/drivers/infiniband/sw/rdmavt/qp.c > @@ -84,10 +84,15 @@ EXPORT_SYMBOL(ib_rvt_state_ops); > /* platform specific: return the last level cache (llc) size, in KiB */ > static int rvt_wss_llc_size(void) > { > +#if !defined(CONFIG_UML) > /* assume that the boot CPU value is universal for all CPUs */ > return boot_cpu_data.x86_cache_size; > +#else /* CONFIG_UML */ > + return 1024; /* fake 1 MB LLC size */ > +#endif > } > > +#if !defined(CONFIG_UML) > /* platform specific: cacheless copy */ > static void cacheless_memcpy(void *dst, void *src, size_t n) > { > @@ -99,6 +104,13 @@ static void cacheless_memcpy(void *dst, > */ > __copy_user_nocache(dst, (void __user *)src, n, 0); > } > +#else > +/* for CONFIG_UML, this is just a plain memcpy() */ > +static void cacheless_memcpy(void *dst, void *src, size_t n) > +{ > + memcpy(dst, src, n); > +} > +#endif memcpy is not the same thing as __copy_user - the hint is in the __user cast.. It should by copy_from_user(), I think, and this is all just somehow broken to not check the return code. Why are you trying to make a HW driver compile on UML? Is there any way to even use a driver like this in a UML environment? Jason
On 1/3/22 15:04, Jason Gunthorpe wrote: > On Sat, Jan 01, 2022 at 11:06:23PM -0800, Randy Dunlap wrote: >> When building rdmavt for ARCH=um, qp.c has a build error on a reference >> to the x86-specific cpuinfo field 'x86_cache_size'. This value is then >> used to determine whether to use cacheless_memcpy() or not. >> Provide a fake value to LLC for CONFIG_UML. Then provide a separate >> verison of cacheless_memcpy() for CONFIG_UML that is just a plain >> memcpy(), like the calling code uses. >> >> Prevents these build errors: >> >> ../drivers/infiniband/sw/rdmavt/qp.c: In function ‘rvt_wss_llc_size’: >> ../drivers/infiniband/sw/rdmavt/qp.c:88:23: error: ‘struct cpuinfo_um’ has no member named ‘x86_cache_size’; did you mean ‘x86_capability’? >> return boot_cpu_data.x86_cache_size; >> >> ../drivers/infiniband/sw/rdmavt/qp.c: In function ‘cacheless_memcpy’: >> ../drivers/infiniband/sw/rdmavt/qp.c:100:2: error: implicit declaration of function ‘__copy_user_nocache’; did you mean ‘copy_user_page’? [-Werror=implicit-function-declaration] >> __copy_user_nocache(dst, (void __user *)src, n, 0); >> >> Fixes: 68f5d3f3b654 ("um: add PCI over virtio emulation driver") >> Signed-off-by: Randy Dunlap <rdunlap@infradead.org> >> drivers/infiniband/sw/rdmavt/qp.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> +++ linux-next-20211224/drivers/infiniband/sw/rdmavt/qp.c >> @@ -84,10 +84,15 @@ EXPORT_SYMBOL(ib_rvt_state_ops); >> /* platform specific: return the last level cache (llc) size, in KiB */ >> static int rvt_wss_llc_size(void) >> { >> +#if !defined(CONFIG_UML) >> /* assume that the boot CPU value is universal for all CPUs */ >> return boot_cpu_data.x86_cache_size; >> +#else /* CONFIG_UML */ >> + return 1024; /* fake 1 MB LLC size */ >> +#endif >> } >> >> +#if !defined(CONFIG_UML) >> /* platform specific: cacheless copy */ >> static void cacheless_memcpy(void *dst, void *src, size_t n) >> { >> @@ -99,6 +104,13 @@ static void cacheless_memcpy(void *dst, >> */ >> __copy_user_nocache(dst, (void __user *)src, n, 0); >> } >> +#else >> +/* for CONFIG_UML, this is just a plain memcpy() */ >> +static void cacheless_memcpy(void *dst, void *src, size_t n) >> +{ >> + memcpy(dst, src, n); >> +} >> +#endif > > memcpy is not the same thing as __copy_user - the hint is in the > __user cast.. > > It should by copy_from_user(), I think, and this is all just somehow > broken to not check the return code. Thanks. > Why are you trying to make a HW driver compile on UML? Is there any > way to even use a driver like this in a UML environment? I'm just trying to clean up lots of UML build errors. I'm quite happy just making the driver depend on !UML. UML maintainers, what do you think? Thanks again.
Yikes, this rdmavt code is completely fucked up and a very good example why people should not use __user casts or random internal helpers. The right fix is to remove this crap entirely, and if the rdmavt maintainer think that they absolutely need a non-temporal memcpy they need to work to add a core API for it.
On 04/01/2022 01:00, Randy Dunlap wrote: > > > On 1/3/22 15:04, Jason Gunthorpe wrote: >> On Sat, Jan 01, 2022 at 11:06:23PM -0800, Randy Dunlap wrote: >>> When building rdmavt for ARCH=um, qp.c has a build error on a reference >>> to the x86-specific cpuinfo field 'x86_cache_size'. This value is then >>> used to determine whether to use cacheless_memcpy() or not. >>> Provide a fake value to LLC for CONFIG_UML. Then provide a separate >>> verison of cacheless_memcpy() for CONFIG_UML that is just a plain >>> memcpy(), like the calling code uses. >>> >>> Prevents these build errors: >>> >>> ../drivers/infiniband/sw/rdmavt/qp.c: In function ‘rvt_wss_llc_size’: >>> ../drivers/infiniband/sw/rdmavt/qp.c:88:23: error: ‘struct cpuinfo_um’ has no member named ‘x86_cache_size’; did you mean ‘x86_capability’? >>> return boot_cpu_data.x86_cache_size; >>> >>> ../drivers/infiniband/sw/rdmavt/qp.c: In function ‘cacheless_memcpy’: >>> ../drivers/infiniband/sw/rdmavt/qp.c:100:2: error: implicit declaration of function ‘__copy_user_nocache’; did you mean ‘copy_user_page’? [-Werror=implicit-function-declaration] >>> __copy_user_nocache(dst, (void __user *)src, n, 0); >>> >>> Fixes: 68f5d3f3b654 ("um: add PCI over virtio emulation driver") >>> Signed-off-by: Randy Dunlap <rdunlap@infradead.org> >>> drivers/infiniband/sw/rdmavt/qp.c | 12 ++++++++++++ >>> 1 file changed, 12 insertions(+) >>> >>> +++ linux-next-20211224/drivers/infiniband/sw/rdmavt/qp.c >>> @@ -84,10 +84,15 @@ EXPORT_SYMBOL(ib_rvt_state_ops); >>> /* platform specific: return the last level cache (llc) size, in KiB */ >>> static int rvt_wss_llc_size(void) >>> { >>> +#if !defined(CONFIG_UML) >>> /* assume that the boot CPU value is universal for all CPUs */ >>> return boot_cpu_data.x86_cache_size; >>> +#else /* CONFIG_UML */ >>> + return 1024; /* fake 1 MB LLC size */ >>> +#endif >>> } >>> >>> +#if !defined(CONFIG_UML) >>> /* platform specific: cacheless copy */ >>> static void cacheless_memcpy(void *dst, void *src, size_t n) >>> { >>> @@ -99,6 +104,13 @@ static void cacheless_memcpy(void *dst, >>> */ >>> __copy_user_nocache(dst, (void __user *)src, n, 0); >>> } >>> +#else >>> +/* for CONFIG_UML, this is just a plain memcpy() */ >>> +static void cacheless_memcpy(void *dst, void *src, size_t n) >>> +{ >>> + memcpy(dst, src, n); >>> +} >>> +#endif >> >> memcpy is not the same thing as __copy_user - the hint is in the >> __user cast.. >> >> It should by copy_from_user(), I think, and this is all just somehow >> broken to not check the return code. > > Thanks. > >> Why are you trying to make a HW driver compile on UML? Is there any >> way to even use a driver like this in a UML environment? > > I'm just trying to clean up lots of UML build errors. > I'm quite happy just making the driver depend on !UML. > > UML maintainers, what do you think? > > Thanks again. > I would suggest that we just !UML this driver.
On Tue, 2022-01-04 at 08:03 +0000, Anton Ivanov wrote: > > > > > Why are you trying to make a HW driver compile on UML? Is there any > > > way to even use a driver like this in a UML environment? > > > > I'm just trying to clean up lots of UML build errors. > > I'm quite happy just making the driver depend on !UML. > > > > UML maintainers, what do you think? > > > > Thanks again. > > > > I would suggest that we just !UML this driver. > Agree, unless some of the maintainers of this driver actually wants to build simulation for it for testing or something, it's almost certainly completely useless. After all, the reason I enabled PCI on UML was to be able to test - in simulation - PCI driver code in UML... Most certainly nobody wants to do that here, so it's pointless to let the driver be compiled. OTOH, as Christoph points out, that seems like a band-aid for some really strange code, but it's probably the easiest way to get the build issue fixed in the short term. johannes
--- linux-next-20211224.orig/drivers/infiniband/sw/rdmavt/qp.c +++ linux-next-20211224/drivers/infiniband/sw/rdmavt/qp.c @@ -84,10 +84,15 @@ EXPORT_SYMBOL(ib_rvt_state_ops); /* platform specific: return the last level cache (llc) size, in KiB */ static int rvt_wss_llc_size(void) { +#if !defined(CONFIG_UML) /* assume that the boot CPU value is universal for all CPUs */ return boot_cpu_data.x86_cache_size; +#else /* CONFIG_UML */ + return 1024; /* fake 1 MB LLC size */ +#endif } +#if !defined(CONFIG_UML) /* platform specific: cacheless copy */ static void cacheless_memcpy(void *dst, void *src, size_t n) { @@ -99,6 +104,13 @@ static void cacheless_memcpy(void *dst, */ __copy_user_nocache(dst, (void __user *)src, n, 0); } +#else +/* for CONFIG_UML, this is just a plain memcpy() */ +static void cacheless_memcpy(void *dst, void *src, size_t n) +{ + memcpy(dst, src, n); +} +#endif void rvt_wss_exit(struct rvt_dev_info *rdi) {
When building rdmavt for ARCH=um, qp.c has a build error on a reference to the x86-specific cpuinfo field 'x86_cache_size'. This value is then used to determine whether to use cacheless_memcpy() or not. Provide a fake value to LLC for CONFIG_UML. Then provide a separate verison of cacheless_memcpy() for CONFIG_UML that is just a plain memcpy(), like the calling code uses. Prevents these build errors: ../drivers/infiniband/sw/rdmavt/qp.c: In function ‘rvt_wss_llc_size’: ../drivers/infiniband/sw/rdmavt/qp.c:88:23: error: ‘struct cpuinfo_um’ has no member named ‘x86_cache_size’; did you mean ‘x86_capability’? return boot_cpu_data.x86_cache_size; ../drivers/infiniband/sw/rdmavt/qp.c: In function ‘cacheless_memcpy’: ../drivers/infiniband/sw/rdmavt/qp.c:100:2: error: implicit declaration of function ‘__copy_user_nocache’; did you mean ‘copy_user_page’? [-Werror=implicit-function-declaration] __copy_user_nocache(dst, (void __user *)src, n, 0); Fixes: 68f5d3f3b654 ("um: add PCI over virtio emulation driver") Signed-off-by: Randy Dunlap <rdunlap@infradead.org> --- drivers/infiniband/sw/rdmavt/qp.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)