* [PATCH v2 1/2] virtio_ring: add a struce device forward declaration @ 2023-04-10 11:28 Shunsuke Mie 2023-04-10 11:28 ` [PATCH v2 2/2] tools/virtio: fix build caused by virtio_ring changes Shunsuke Mie 2023-04-10 11:49 ` [PATCH v2 1/2] virtio_ring: add a struce device forward declaration Michael S. Tsirkin 0 siblings, 2 replies; 9+ messages in thread From: Shunsuke Mie @ 2023-04-10 11:28 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Rafael J. Wysocki, Greg Kroah-Hartman, linux-kernel, Matthew Wilcox (Oracle), virtualization, Eugenio Pérez, Shunsuke Mie, Sakari Ailus, Andy Shevchenko The virtio_ring header file uses the struct device without a forward declaration. Signed-off-by: Shunsuke Mie <mie@igel.co.jp> --- include/linux/virtio_ring.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h index 8b95b69ef694..77a9c2f52919 100644 --- a/include/linux/virtio_ring.h +++ b/include/linux/virtio_ring.h @@ -58,6 +58,7 @@ do { \ struct virtio_device; struct virtqueue; +struct device; /* * Creates a virtqueue and allocates the descriptor ring. If -- 2.25.1 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 2/2] tools/virtio: fix build caused by virtio_ring changes 2023-04-10 11:28 [PATCH v2 1/2] virtio_ring: add a struce device forward declaration Shunsuke Mie @ 2023-04-10 11:28 ` Shunsuke Mie 2023-04-10 12:00 ` Michael S. Tsirkin 2023-04-10 11:49 ` [PATCH v2 1/2] virtio_ring: add a struce device forward declaration Michael S. Tsirkin 1 sibling, 1 reply; 9+ messages in thread From: Shunsuke Mie @ 2023-04-10 11:28 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Rafael J. Wysocki, Greg Kroah-Hartman, linux-kernel, Matthew Wilcox (Oracle), virtualization, Eugenio Pérez, Shunsuke Mie, Sakari Ailus, Andy Shevchenko Fix the build dependency for virtio_test. The virtio_ring that is used from the test requires container_of_const(). Change to use container_of.h kernel header directly and adapt related codes. Signed-off-by: Shunsuke Mie <mie@igel.co.jp> --- tools/include/linux/types.h | 1 - tools/virtio/linux/compiler.h | 2 ++ tools/virtio/linux/kernel.h | 5 +---- tools/virtio/linux/module.h | 1 - tools/virtio/linux/uaccess.h | 11 ++--------- 5 files changed, 5 insertions(+), 15 deletions(-) diff --git a/tools/include/linux/types.h b/tools/include/linux/types.h index 051fdeaf2670..f1896b70a8e5 100644 --- a/tools/include/linux/types.h +++ b/tools/include/linux/types.h @@ -49,7 +49,6 @@ typedef __s8 s8; #endif #define __force -#define __user #define __must_check #define __cold diff --git a/tools/virtio/linux/compiler.h b/tools/virtio/linux/compiler.h index 2c51bccb97bb..1f3a15b954b9 100644 --- a/tools/virtio/linux/compiler.h +++ b/tools/virtio/linux/compiler.h @@ -2,6 +2,8 @@ #ifndef LINUX_COMPILER_H #define LINUX_COMPILER_H +#include "../../../include/linux/compiler_types.h" + #define WRITE_ONCE(var, val) \ (*((volatile typeof(val) *)(&(var))) = (val)) diff --git a/tools/virtio/linux/kernel.h b/tools/virtio/linux/kernel.h index 8b877167933d..6702008f7f5c 100644 --- a/tools/virtio/linux/kernel.h +++ b/tools/virtio/linux/kernel.h @@ -10,6 +10,7 @@ #include <stdarg.h> #include <linux/compiler.h> +#include "../../../include/linux/container_of.h" #include <linux/log2.h> #include <linux/types.h> #include <linux/overflow.h> @@ -107,10 +108,6 @@ static inline void free_page(unsigned long addr) free((void *)addr); } -#define container_of(ptr, type, member) ({ \ - const typeof( ((type *)0)->member ) *__mptr = (ptr); \ - (type *)( (char *)__mptr - offsetof(type,member) );}) - # ifndef likely # define likely(x) (__builtin_expect(!!(x), 1)) # endif diff --git a/tools/virtio/linux/module.h b/tools/virtio/linux/module.h index 9dfa96fea2b2..5cf39167d47a 100644 --- a/tools/virtio/linux/module.h +++ b/tools/virtio/linux/module.h @@ -4,4 +4,3 @@ #define MODULE_LICENSE(__MODULE_LICENSE_value) \ static __attribute__((unused)) const char *__MODULE_LICENSE_name = \ __MODULE_LICENSE_value - diff --git a/tools/virtio/linux/uaccess.h b/tools/virtio/linux/uaccess.h index 991dfb263998..cde2c344b260 100644 --- a/tools/virtio/linux/uaccess.h +++ b/tools/virtio/linux/uaccess.h @@ -6,15 +6,10 @@ extern void *__user_addr_min, *__user_addr_max; -static inline void __chk_user_ptr(const volatile void *p, size_t size) -{ - assert(p >= __user_addr_min && p + size <= __user_addr_max); -} - #define put_user(x, ptr) \ ({ \ typeof(ptr) __pu_ptr = (ptr); \ - __chk_user_ptr(__pu_ptr, sizeof(*__pu_ptr)); \ + __chk_user_ptr(__pu_ptr); \ WRITE_ONCE(*(__pu_ptr), x); \ 0; \ }) @@ -22,7 +17,7 @@ static inline void __chk_user_ptr(const volatile void *p, size_t size) #define get_user(x, ptr) \ ({ \ typeof(ptr) __pu_ptr = (ptr); \ - __chk_user_ptr(__pu_ptr, sizeof(*__pu_ptr)); \ + __chk_user_ptr(__pu_ptr); \ x = READ_ONCE(*(__pu_ptr)); \ 0; \ }) @@ -37,7 +32,6 @@ static void volatile_memcpy(volatile char *to, const volatile char *from, static inline int copy_from_user(void *to, const void __user volatile *from, unsigned long n) { - __chk_user_ptr(from, n); volatile_memcpy(to, from, n); return 0; } @@ -45,7 +39,6 @@ static inline int copy_from_user(void *to, const void __user volatile *from, static inline int copy_to_user(void __user volatile *to, const void *from, unsigned long n) { - __chk_user_ptr(to, n); volatile_memcpy(to, from, n); return 0; } -- 2.25.1 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] tools/virtio: fix build caused by virtio_ring changes 2023-04-10 11:28 ` [PATCH v2 2/2] tools/virtio: fix build caused by virtio_ring changes Shunsuke Mie @ 2023-04-10 12:00 ` Michael S. Tsirkin 2023-04-10 18:40 ` Greg Kroah-Hartman 0 siblings, 1 reply; 9+ messages in thread From: Michael S. Tsirkin @ 2023-04-10 12:00 UTC (permalink / raw) To: Shunsuke Mie Cc: Rafael J. Wysocki, Greg Kroah-Hartman, linux-kernel, Matthew Wilcox (Oracle), virtualization, Eugenio Pérez, Sakari Ailus, Andy Shevchenko On Mon, Apr 10, 2023 at 08:28:45PM +0900, Shunsuke Mie wrote: > Fix the build dependency for virtio_test. The virtio_ring that is used from > the test requires container_of_const(). Change to use container_of.h kernel > header directly and adapt related codes. > > Signed-off-by: Shunsuke Mie <mie@igel.co.jp> This is only for next right? That's where container_of_const things are I think ... > --- > tools/include/linux/types.h | 1 - > tools/virtio/linux/compiler.h | 2 ++ > tools/virtio/linux/kernel.h | 5 +---- > tools/virtio/linux/module.h | 1 - > tools/virtio/linux/uaccess.h | 11 ++--------- > 5 files changed, 5 insertions(+), 15 deletions(-) > > diff --git a/tools/include/linux/types.h b/tools/include/linux/types.h > index 051fdeaf2670..f1896b70a8e5 100644 > --- a/tools/include/linux/types.h > +++ b/tools/include/linux/types.h > @@ -49,7 +49,6 @@ typedef __s8 s8; > #endif > > #define __force > -#define __user > #define __must_check > #define __cold > > diff --git a/tools/virtio/linux/compiler.h b/tools/virtio/linux/compiler.h > index 2c51bccb97bb..1f3a15b954b9 100644 > --- a/tools/virtio/linux/compiler.h > +++ b/tools/virtio/linux/compiler.h > @@ -2,6 +2,8 @@ > #ifndef LINUX_COMPILER_H > #define LINUX_COMPILER_H > > +#include "../../../include/linux/compiler_types.h" > + > #define WRITE_ONCE(var, val) \ > (*((volatile typeof(val) *)(&(var))) = (val)) > > diff --git a/tools/virtio/linux/kernel.h b/tools/virtio/linux/kernel.h > index 8b877167933d..6702008f7f5c 100644 > --- a/tools/virtio/linux/kernel.h > +++ b/tools/virtio/linux/kernel.h > @@ -10,6 +10,7 @@ > #include <stdarg.h> > > #include <linux/compiler.h> > +#include "../../../include/linux/container_of.h" > #include <linux/log2.h> > #include <linux/types.h> > #include <linux/overflow.h> > @@ -107,10 +108,6 @@ static inline void free_page(unsigned long addr) > free((void *)addr); > } > > -#define container_of(ptr, type, member) ({ \ > - const typeof( ((type *)0)->member ) *__mptr = (ptr); \ > - (type *)( (char *)__mptr - offsetof(type,member) );}) > - > # ifndef likely > # define likely(x) (__builtin_expect(!!(x), 1)) > # endif > diff --git a/tools/virtio/linux/module.h b/tools/virtio/linux/module.h > index 9dfa96fea2b2..5cf39167d47a 100644 > --- a/tools/virtio/linux/module.h > +++ b/tools/virtio/linux/module.h > @@ -4,4 +4,3 @@ > #define MODULE_LICENSE(__MODULE_LICENSE_value) \ > static __attribute__((unused)) const char *__MODULE_LICENSE_name = \ > __MODULE_LICENSE_value > - > diff --git a/tools/virtio/linux/uaccess.h b/tools/virtio/linux/uaccess.h > index 991dfb263998..cde2c344b260 100644 > --- a/tools/virtio/linux/uaccess.h > +++ b/tools/virtio/linux/uaccess.h > @@ -6,15 +6,10 @@ > > extern void *__user_addr_min, *__user_addr_max; > > -static inline void __chk_user_ptr(const volatile void *p, size_t size) > -{ > - assert(p >= __user_addr_min && p + size <= __user_addr_max); > -} > - > #define put_user(x, ptr) \ > ({ \ > typeof(ptr) __pu_ptr = (ptr); \ > - __chk_user_ptr(__pu_ptr, sizeof(*__pu_ptr)); \ > + __chk_user_ptr(__pu_ptr); \ > WRITE_ONCE(*(__pu_ptr), x); \ > 0; \ > }) > @@ -22,7 +17,7 @@ static inline void __chk_user_ptr(const volatile void *p, size_t size) > #define get_user(x, ptr) \ > ({ \ > typeof(ptr) __pu_ptr = (ptr); \ > - __chk_user_ptr(__pu_ptr, sizeof(*__pu_ptr)); \ > + __chk_user_ptr(__pu_ptr); \ > x = READ_ONCE(*(__pu_ptr)); \ > 0; \ > }) > @@ -37,7 +32,6 @@ static void volatile_memcpy(volatile char *to, const volatile char *from, > static inline int copy_from_user(void *to, const void __user volatile *from, > unsigned long n) > { > - __chk_user_ptr(from, n); > volatile_memcpy(to, from, n); > return 0; > } > @@ -45,7 +39,6 @@ static inline int copy_from_user(void *to, const void __user volatile *from, > static inline int copy_to_user(void __user volatile *to, const void *from, > unsigned long n) > { > - __chk_user_ptr(to, n); > volatile_memcpy(to, from, n); > return 0; > } > -- > 2.25.1 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] tools/virtio: fix build caused by virtio_ring changes 2023-04-10 12:00 ` Michael S. Tsirkin @ 2023-04-10 18:40 ` Greg Kroah-Hartman 2023-04-10 19:44 ` Michael S. Tsirkin 2023-04-11 3:17 ` Shunsuke Mie 0 siblings, 2 replies; 9+ messages in thread From: Greg Kroah-Hartman @ 2023-04-10 18:40 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Rafael J. Wysocki, linux-kernel, Matthew Wilcox (Oracle), virtualization, Eugenio Pérez, Shunsuke Mie, Sakari Ailus, Andy Shevchenko On Mon, Apr 10, 2023 at 08:00:33AM -0400, Michael S. Tsirkin wrote: > On Mon, Apr 10, 2023 at 08:28:45PM +0900, Shunsuke Mie wrote: > > Fix the build dependency for virtio_test. The virtio_ring that is used from > > the test requires container_of_const(). Change to use container_of.h kernel > > header directly and adapt related codes. > > > > Signed-off-by: Shunsuke Mie <mie@igel.co.jp> > > This is only for next right? That's where container_of_const > things are I think ... container_of_const() is in 6.2. > > > --- > > tools/include/linux/types.h | 1 - > > tools/virtio/linux/compiler.h | 2 ++ > > tools/virtio/linux/kernel.h | 5 +---- > > tools/virtio/linux/module.h | 1 - > > tools/virtio/linux/uaccess.h | 11 ++--------- > > 5 files changed, 5 insertions(+), 15 deletions(-) > > > > diff --git a/tools/include/linux/types.h b/tools/include/linux/types.h > > index 051fdeaf2670..f1896b70a8e5 100644 > > --- a/tools/include/linux/types.h > > +++ b/tools/include/linux/types.h > > @@ -49,7 +49,6 @@ typedef __s8 s8; > > #endif > > > > #define __force > > -#define __user Why is this needed? > > #define __must_check > > #define __cold > > > > diff --git a/tools/virtio/linux/compiler.h b/tools/virtio/linux/compiler.h > > index 2c51bccb97bb..1f3a15b954b9 100644 > > --- a/tools/virtio/linux/compiler.h > > +++ b/tools/virtio/linux/compiler.h > > @@ -2,6 +2,8 @@ > > #ifndef LINUX_COMPILER_H > > #define LINUX_COMPILER_H > > > > +#include "../../../include/linux/compiler_types.h" While I understand your need to not want to duplicate code, what in the world is this doing? Why not use the in-kernel compiler.h instead? Why are you copying loads of .h files into tools/virtio/? What is this for and why not just use the real files so you don't have to even attempt to try to keep things in sync (hint, they will always be out of sync.) > > + > > #define WRITE_ONCE(var, val) \ > > (*((volatile typeof(val) *)(&(var))) = (val)) > > > > diff --git a/tools/virtio/linux/kernel.h b/tools/virtio/linux/kernel.h > > index 8b877167933d..6702008f7f5c 100644 > > --- a/tools/virtio/linux/kernel.h > > +++ b/tools/virtio/linux/kernel.h > > @@ -10,6 +10,7 @@ > > #include <stdarg.h> > > > > #include <linux/compiler.h> > > +#include "../../../include/linux/container_of.h" Either do this for all .h files, or not, don't pick and choose. > > #include <linux/log2.h> > > #include <linux/types.h> > > #include <linux/overflow.h> > > @@ -107,10 +108,6 @@ static inline void free_page(unsigned long addr) > > free((void *)addr); > > } > > > > -#define container_of(ptr, type, member) ({ \ > > - const typeof( ((type *)0)->member ) *__mptr = (ptr); \ > > - (type *)( (char *)__mptr - offsetof(type,member) );}) > > - > > # ifndef likely > > # define likely(x) (__builtin_expect(!!(x), 1)) > > # endif > > diff --git a/tools/virtio/linux/module.h b/tools/virtio/linux/module.h > > index 9dfa96fea2b2..5cf39167d47a 100644 > > --- a/tools/virtio/linux/module.h > > +++ b/tools/virtio/linux/module.h > > @@ -4,4 +4,3 @@ > > #define MODULE_LICENSE(__MODULE_LICENSE_value) \ > > static __attribute__((unused)) const char *__MODULE_LICENSE_name = \ > > __MODULE_LICENSE_value > > - This change has nothing to do with what you said was happening in this patch :( Please be more careful. > > diff --git a/tools/virtio/linux/uaccess.h b/tools/virtio/linux/uaccess.h > > index 991dfb263998..cde2c344b260 100644 > > --- a/tools/virtio/linux/uaccess.h > > +++ b/tools/virtio/linux/uaccess.h > > @@ -6,15 +6,10 @@ > > > > extern void *__user_addr_min, *__user_addr_max; > > > > -static inline void __chk_user_ptr(const volatile void *p, size_t size) > > -{ > > - assert(p >= __user_addr_min && p + size <= __user_addr_max); > > -} > > - What does this function have to do with container_of()? > > #define put_user(x, ptr) \ > > ({ \ > > typeof(ptr) __pu_ptr = (ptr); \ > > - __chk_user_ptr(__pu_ptr, sizeof(*__pu_ptr)); \ > > + __chk_user_ptr(__pu_ptr); \ Why are you trying to duplicate in-kernel .h files? This all doesn't look ok, sorry. greg k-h _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] tools/virtio: fix build caused by virtio_ring changes 2023-04-10 18:40 ` Greg Kroah-Hartman @ 2023-04-10 19:44 ` Michael S. Tsirkin 2023-04-11 3:25 ` Shunsuke Mie 2023-04-11 3:17 ` Shunsuke Mie 1 sibling, 1 reply; 9+ messages in thread From: Michael S. Tsirkin @ 2023-04-10 19:44 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Rafael J. Wysocki, linux-kernel, Matthew Wilcox (Oracle), virtualization, Eugenio Pérez, Shunsuke Mie, Sakari Ailus, Andy Shevchenko On Mon, Apr 10, 2023 at 08:40:34PM +0200, Greg Kroah-Hartman wrote: > On Mon, Apr 10, 2023 at 08:00:33AM -0400, Michael S. Tsirkin wrote: > > On Mon, Apr 10, 2023 at 08:28:45PM +0900, Shunsuke Mie wrote: > > > Fix the build dependency for virtio_test. The virtio_ring that is used from > > > the test requires container_of_const(). Change to use container_of.h kernel > > > header directly and adapt related codes. > > > > > > Signed-off-by: Shunsuke Mie <mie@igel.co.jp> > > > > This is only for next right? That's where container_of_const > > things are I think ... > > container_of_const() is in 6.2. Absolutely but the patch switching virtio to that is not there. > > > > > > --- > > > tools/include/linux/types.h | 1 - > > > tools/virtio/linux/compiler.h | 2 ++ > > > tools/virtio/linux/kernel.h | 5 +---- > > > tools/virtio/linux/module.h | 1 - > > > tools/virtio/linux/uaccess.h | 11 ++--------- > > > 5 files changed, 5 insertions(+), 15 deletions(-) > > > > > > diff --git a/tools/include/linux/types.h b/tools/include/linux/types.h > > > index 051fdeaf2670..f1896b70a8e5 100644 > > > --- a/tools/include/linux/types.h > > > +++ b/tools/include/linux/types.h > > > @@ -49,7 +49,6 @@ typedef __s8 s8; > > > #endif > > > > > > #define __force > > > -#define __user > > Why is this needed? > > > > #define __must_check > > > #define __cold > > > > > > diff --git a/tools/virtio/linux/compiler.h b/tools/virtio/linux/compiler.h > > > index 2c51bccb97bb..1f3a15b954b9 100644 > > > --- a/tools/virtio/linux/compiler.h > > > +++ b/tools/virtio/linux/compiler.h > > > @@ -2,6 +2,8 @@ > > > #ifndef LINUX_COMPILER_H > > > #define LINUX_COMPILER_H > > > > > > +#include "../../../include/linux/compiler_types.h" > > While I understand your need to not want to duplicate code, what in the > world is this doing? Why not use the in-kernel compiler.h instead? Why > are you copying loads of .h files into tools/virtio/? What is this for > and why not just use the real files so you don't have to even attempt to > try to keep things in sync (hint, they will always be out of sync.) Because it's doing a very weird hack: building some kernel code into a userspace binary, used for just for testing. This is all not part of kernel build intentionally, no one is supposed to use this binary in production, but it was helpful in finding bugs in virtio core in the past so I keep it around. -- MST _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] tools/virtio: fix build caused by virtio_ring changes 2023-04-10 19:44 ` Michael S. Tsirkin @ 2023-04-11 3:25 ` Shunsuke Mie 2023-04-11 6:37 ` Michael S. Tsirkin 0 siblings, 1 reply; 9+ messages in thread From: Shunsuke Mie @ 2023-04-11 3:25 UTC (permalink / raw) To: Michael S. Tsirkin, Greg Kroah-Hartman Cc: Rafael J. Wysocki, linux-kernel, Matthew Wilcox (Oracle), virtualization, Eugenio Pérez, Sakari Ailus, Andy Shevchenko On 2023/04/11 4:44, Michael S. Tsirkin wrote: > On Mon, Apr 10, 2023 at 08:40:34PM +0200, Greg Kroah-Hartman wrote: >> On Mon, Apr 10, 2023 at 08:00:33AM -0400, Michael S. Tsirkin wrote: >>> On Mon, Apr 10, 2023 at 08:28:45PM +0900, Shunsuke Mie wrote: >>>> Fix the build dependency for virtio_test. The virtio_ring that is used from >>>> the test requires container_of_const(). Change to use container_of.h kernel >>>> header directly and adapt related codes. >>>> >>>> Signed-off-by: Shunsuke Mie <mie@igel.co.jp> >>> This is only for next right? That's where container_of_const >>> things are I think ... >> container_of_const() is in 6.2. > > Absolutely but the patch switching virtio to that is not there. > > >>>> --- >>>> tools/include/linux/types.h | 1 - >>>> tools/virtio/linux/compiler.h | 2 ++ >>>> tools/virtio/linux/kernel.h | 5 +---- >>>> tools/virtio/linux/module.h | 1 - >>>> tools/virtio/linux/uaccess.h | 11 ++--------- >>>> 5 files changed, 5 insertions(+), 15 deletions(-) >>>> >>>> diff --git a/tools/include/linux/types.h b/tools/include/linux/types.h >>>> index 051fdeaf2670..f1896b70a8e5 100644 >>>> --- a/tools/include/linux/types.h >>>> +++ b/tools/include/linux/types.h >>>> @@ -49,7 +49,6 @@ typedef __s8 s8; >>>> #endif >>>> >>>> #define __force >>>> -#define __user >> Why is this needed? >> >>>> #define __must_check >>>> #define __cold >>>> >>>> diff --git a/tools/virtio/linux/compiler.h b/tools/virtio/linux/compiler.h >>>> index 2c51bccb97bb..1f3a15b954b9 100644 >>>> --- a/tools/virtio/linux/compiler.h >>>> +++ b/tools/virtio/linux/compiler.h >>>> @@ -2,6 +2,8 @@ >>>> #ifndef LINUX_COMPILER_H >>>> #define LINUX_COMPILER_H >>>> >>>> +#include "../../../include/linux/compiler_types.h" >> While I understand your need to not want to duplicate code, what in the >> world is this doing? Why not use the in-kernel compiler.h instead? Why >> are you copying loads of .h files into tools/virtio/? What is this for >> and why not just use the real files so you don't have to even attempt to >> try to keep things in sync (hint, they will always be out of sync.) > Because it's doing a very weird hack: building some kernel > code into a userspace binary, used for just for testing. > This is all not part of kernel build intentionally, no > one is supposed to use this binary in production, but > it was helpful in finding bugs in virtio core in the past > so I keep it around. Would it be possible to switch to in-kernel tests, such as KUnit? If it's possible, we could use the kernel headers and implementations as they are, and we could address the concerns that Greg raised I think. Best regards, Shunsuke _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] tools/virtio: fix build caused by virtio_ring changes 2023-04-11 3:25 ` Shunsuke Mie @ 2023-04-11 6:37 ` Michael S. Tsirkin 0 siblings, 0 replies; 9+ messages in thread From: Michael S. Tsirkin @ 2023-04-11 6:37 UTC (permalink / raw) To: Shunsuke Mie Cc: Rafael J. Wysocki, Greg Kroah-Hartman, linux-kernel, Matthew Wilcox (Oracle), virtualization, Eugenio Pérez, Sakari Ailus, Andy Shevchenko On Tue, Apr 11, 2023 at 12:25:39PM +0900, Shunsuke Mie wrote: > > On 2023/04/11 4:44, Michael S. Tsirkin wrote: > > On Mon, Apr 10, 2023 at 08:40:34PM +0200, Greg Kroah-Hartman wrote: > > > On Mon, Apr 10, 2023 at 08:00:33AM -0400, Michael S. Tsirkin wrote: > > > > On Mon, Apr 10, 2023 at 08:28:45PM +0900, Shunsuke Mie wrote: > > > > > Fix the build dependency for virtio_test. The virtio_ring that is used from > > > > > the test requires container_of_const(). Change to use container_of.h kernel > > > > > header directly and adapt related codes. > > > > > > > > > > Signed-off-by: Shunsuke Mie <mie@igel.co.jp> > > > > This is only for next right? That's where container_of_const > > > > things are I think ... > > > container_of_const() is in 6.2. > > > > Absolutely but the patch switching virtio to that is not there. > > > > > > > > > --- > > > > > tools/include/linux/types.h | 1 - > > > > > tools/virtio/linux/compiler.h | 2 ++ > > > > > tools/virtio/linux/kernel.h | 5 +---- > > > > > tools/virtio/linux/module.h | 1 - > > > > > tools/virtio/linux/uaccess.h | 11 ++--------- > > > > > 5 files changed, 5 insertions(+), 15 deletions(-) > > > > > > > > > > diff --git a/tools/include/linux/types.h b/tools/include/linux/types.h > > > > > index 051fdeaf2670..f1896b70a8e5 100644 > > > > > --- a/tools/include/linux/types.h > > > > > +++ b/tools/include/linux/types.h > > > > > @@ -49,7 +49,6 @@ typedef __s8 s8; > > > > > #endif > > > > > #define __force > > > > > -#define __user > > > Why is this needed? > > > > > > > > #define __must_check > > > > > #define __cold > > > > > diff --git a/tools/virtio/linux/compiler.h b/tools/virtio/linux/compiler.h > > > > > index 2c51bccb97bb..1f3a15b954b9 100644 > > > > > --- a/tools/virtio/linux/compiler.h > > > > > +++ b/tools/virtio/linux/compiler.h > > > > > @@ -2,6 +2,8 @@ > > > > > #ifndef LINUX_COMPILER_H > > > > > #define LINUX_COMPILER_H > > > > > +#include "../../../include/linux/compiler_types.h" > > > While I understand your need to not want to duplicate code, what in the > > > world is this doing? Why not use the in-kernel compiler.h instead? Why > > > are you copying loads of .h files into tools/virtio/? What is this for > > > and why not just use the real files so you don't have to even attempt to > > > try to keep things in sync (hint, they will always be out of sync.) > > Because it's doing a very weird hack: building some kernel > > code into a userspace binary, used for just for testing. > > This is all not part of kernel build intentionally, no > > one is supposed to use this binary in production, but > > it was helpful in finding bugs in virtio core in the past > > so I keep it around. > Would it be possible to switch to in-kernel tests, such as KUnit? If it's > possible, we could use the kernel headers and implementations as they are, > and we could address the concerns that Greg raised I think. > > > Best regards, > > Shunsuke I think your patch is fine as is. Using kunit is certainly possible, but won't be a small project. tools/virtio was always a quick hack to help experiment quickly, so I'm not worried by it being broken often - whoever wants to play with it next, fixes it up. -- MST _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] tools/virtio: fix build caused by virtio_ring changes 2023-04-10 18:40 ` Greg Kroah-Hartman 2023-04-10 19:44 ` Michael S. Tsirkin @ 2023-04-11 3:17 ` Shunsuke Mie 1 sibling, 0 replies; 9+ messages in thread From: Shunsuke Mie @ 2023-04-11 3:17 UTC (permalink / raw) To: Greg Kroah-Hartman, Michael S. Tsirkin Cc: Rafael J. Wysocki, linux-kernel, Matthew Wilcox (Oracle), virtualization, Eugenio Pérez, Sakari Ailus, Andy Shevchenko On 2023/04/11 3:40, Greg Kroah-Hartman wrote: > On Mon, Apr 10, 2023 at 08:00:33AM -0400, Michael S. Tsirkin wrote: >> On Mon, Apr 10, 2023 at 08:28:45PM +0900, Shunsuke Mie wrote: >>> Fix the build dependency for virtio_test. The virtio_ring that is used from >>> the test requires container_of_const(). Change to use container_of.h kernel >>> header directly and adapt related codes. >>> >>> Signed-off-by: Shunsuke Mie <mie@igel.co.jp> >> This is only for next right? That's where container_of_const >> things are I think ... > container_of_const() is in 6.2. > > >>> --- >>> tools/include/linux/types.h | 1 - >>> tools/virtio/linux/compiler.h | 2 ++ >>> tools/virtio/linux/kernel.h | 5 +---- >>> tools/virtio/linux/module.h | 1 - >>> tools/virtio/linux/uaccess.h | 11 ++--------- >>> 5 files changed, 5 insertions(+), 15 deletions(-) >>> >>> diff --git a/tools/include/linux/types.h b/tools/include/linux/types.h >>> index 051fdeaf2670..f1896b70a8e5 100644 >>> --- a/tools/include/linux/types.h >>> +++ b/tools/include/linux/types.h >>> @@ -49,7 +49,6 @@ typedef __s8 s8; >>> #endif >>> >>> #define __force >>> -#define __user > Why is this needed? > >>> #define __must_check >>> #define __cold >>> >>> diff --git a/tools/virtio/linux/compiler.h b/tools/virtio/linux/compiler.h >>> index 2c51bccb97bb..1f3a15b954b9 100644 >>> --- a/tools/virtio/linux/compiler.h >>> +++ b/tools/virtio/linux/compiler.h >>> @@ -2,6 +2,8 @@ >>> #ifndef LINUX_COMPILER_H >>> #define LINUX_COMPILER_H >>> >>> +#include "../../../include/linux/compiler_types.h" > While I understand your need to not want to duplicate code, what in the > world is this doing? Why not use the in-kernel compiler.h instead? Why > are you copying loads of .h files into tools/virtio/? What is this for > and why not just use the real files so you don't have to even attempt to > try to keep things in sync (hint, they will always be out of sync.) > >>> + >>> #define WRITE_ONCE(var, val) \ >>> (*((volatile typeof(val) *)(&(var))) = (val)) >>> >>> diff --git a/tools/virtio/linux/kernel.h b/tools/virtio/linux/kernel.h >>> index 8b877167933d..6702008f7f5c 100644 >>> --- a/tools/virtio/linux/kernel.h >>> +++ b/tools/virtio/linux/kernel.h >>> @@ -10,6 +10,7 @@ >>> #include <stdarg.h> >>> >>> #include <linux/compiler.h> >>> +#include "../../../include/linux/container_of.h" > Either do this for all .h files, or not, don't pick and choose. > > > >>> #include <linux/log2.h> >>> #include <linux/types.h> >>> #include <linux/overflow.h> >>> @@ -107,10 +108,6 @@ static inline void free_page(unsigned long addr) >>> free((void *)addr); >>> } >>> >>> -#define container_of(ptr, type, member) ({ \ >>> - const typeof( ((type *)0)->member ) *__mptr = (ptr); \ >>> - (type *)( (char *)__mptr - offsetof(type,member) );}) >>> - >>> # ifndef likely >>> # define likely(x) (__builtin_expect(!!(x), 1)) >>> # endif >>> diff --git a/tools/virtio/linux/module.h b/tools/virtio/linux/module.h >>> index 9dfa96fea2b2..5cf39167d47a 100644 >>> --- a/tools/virtio/linux/module.h >>> +++ b/tools/virtio/linux/module.h >>> @@ -4,4 +4,3 @@ >>> #define MODULE_LICENSE(__MODULE_LICENSE_value) \ >>> static __attribute__((unused)) const char *__MODULE_LICENSE_name = \ >>> __MODULE_LICENSE_value >>> - > This change has nothing to do with what you said was happening in this > patch :( > > Please be more careful. > >>> diff --git a/tools/virtio/linux/uaccess.h b/tools/virtio/linux/uaccess.h >>> index 991dfb263998..cde2c344b260 100644 >>> --- a/tools/virtio/linux/uaccess.h >>> +++ b/tools/virtio/linux/uaccess.h >>> @@ -6,15 +6,10 @@ >>> >>> extern void *__user_addr_min, *__user_addr_max; >>> >>> -static inline void __chk_user_ptr(const volatile void *p, size_t size) >>> -{ >>> - assert(p >= __user_addr_min && p + size <= __user_addr_max); >>> -} >>> - > What does this function have to do with container_of()? > > >>> #define put_user(x, ptr) \ >>> ({ \ >>> typeof(ptr) __pu_ptr = (ptr); \ >>> - __chk_user_ptr(__pu_ptr, sizeof(*__pu_ptr)); \ >>> + __chk_user_ptr(__pu_ptr); \ > Why are you trying to duplicate in-kernel .h files? > > This all doesn't look ok, sorry. > > greg k-h Thank you. I'll modify problems you commented, but we have to discuss the design of tools/virtio. Best regards, Shunsuke. _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] virtio_ring: add a struce device forward declaration 2023-04-10 11:28 [PATCH v2 1/2] virtio_ring: add a struce device forward declaration Shunsuke Mie 2023-04-10 11:28 ` [PATCH v2 2/2] tools/virtio: fix build caused by virtio_ring changes Shunsuke Mie @ 2023-04-10 11:49 ` Michael S. Tsirkin 1 sibling, 0 replies; 9+ messages in thread From: Michael S. Tsirkin @ 2023-04-10 11:49 UTC (permalink / raw) To: Shunsuke Mie Cc: Rafael J. Wysocki, Greg Kroah-Hartman, linux-kernel, Matthew Wilcox (Oracle), virtualization, Eugenio Pérez, Sakari Ailus, Andy Shevchenko s/struce/struct/ ? On Mon, Apr 10, 2023 at 08:28:44PM +0900, Shunsuke Mie wrote: > The virtio_ring header file uses the struct device without a forward > declaration. > > Signed-off-by: Shunsuke Mie <mie@igel.co.jp> > --- > include/linux/virtio_ring.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h > index 8b95b69ef694..77a9c2f52919 100644 > --- a/include/linux/virtio_ring.h > +++ b/include/linux/virtio_ring.h > @@ -58,6 +58,7 @@ do { \ > > struct virtio_device; > struct virtqueue; > +struct device; > > /* > * Creates a virtqueue and allocates the descriptor ring. If > -- > 2.25.1 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-04-11 6:38 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-04-10 11:28 [PATCH v2 1/2] virtio_ring: add a struce device forward declaration Shunsuke Mie 2023-04-10 11:28 ` [PATCH v2 2/2] tools/virtio: fix build caused by virtio_ring changes Shunsuke Mie 2023-04-10 12:00 ` Michael S. Tsirkin 2023-04-10 18:40 ` Greg Kroah-Hartman 2023-04-10 19:44 ` Michael S. Tsirkin 2023-04-11 3:25 ` Shunsuke Mie 2023-04-11 6:37 ` Michael S. Tsirkin 2023-04-11 3:17 ` Shunsuke Mie 2023-04-10 11:49 ` [PATCH v2 1/2] virtio_ring: add a struce device forward declaration Michael S. Tsirkin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).