| Message ID | 1-v1-a805b607f1fb+17b-vfio_container_split_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio: Split the container code into a clean layer and dedicated file | expand |
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Wednesday, August 31, 2022 9:02 AM > > As is normal for headers. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/vfio/vfio.h | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h > index 503bea6c843d56..093784f1dea7a9 100644 > --- a/drivers/vfio/vfio.h > +++ b/drivers/vfio/vfio.h > @@ -3,6 +3,14 @@ > * Copyright (C) 2012 Red Hat, Inc. All rights reserved. > * Author: Alex Williamson <alex.williamson@redhat.com> > */ > +#ifndef __VFIO_VFIO_H__ > +#define __VFIO_VFIO_H__ > + > +#include <linux/device.h> > +#include <linux/cdev.h> > +#include <linux/module.h> Curious what is the criteria for which header inclusions should be placed here. If it is for everything required by the definitions in this file then the list is not complete, e.g. <linux/iommu.h> is obviously missing. btw while they are moved here the inclusions in vfio_main.c are not removed in patch8. > + > +struct iommu_group; > > enum vfio_group_type { > /* > @@ -69,3 +77,5 @@ struct vfio_iommu_driver_ops { > > int vfio_register_iommu_driver(const struct vfio_iommu_driver_ops *ops); > void vfio_unregister_iommu_driver(const struct vfio_iommu_driver_ops > *ops); > + > +#endif > -- > 2.37.2
On Wed, Aug 31, 2022 at 08:42:17AM +0000, Tian, Kevin wrote: > > diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h > > index 503bea6c843d56..093784f1dea7a9 100644 > > --- a/drivers/vfio/vfio.h > > +++ b/drivers/vfio/vfio.h > > @@ -3,6 +3,14 @@ > > * Copyright (C) 2012 Red Hat, Inc. All rights reserved. > > * Author: Alex Williamson <alex.williamson@redhat.com> > > */ > > +#ifndef __VFIO_VFIO_H__ > > +#define __VFIO_VFIO_H__ > > + > > +#include <linux/device.h> > > +#include <linux/cdev.h> > > +#include <linux/module.h> > > Curious what is the criteria for which header inclusions should be > placed here. If it is for everything required by the definitions in > this file then the list is not complete, e.g. <linux/iommu.h> is > obviously missing. It isn't missing: $ clang-14 -Wp,-MMD,drivers/vfio/.vfio_main.o.d -nostdinc -I../arch/x86/include -I./arch/x86/include/generated -I../include -I./include -I../arch/x86/include/uapi -I./arch/x86/include/generated/uapi -I../include/uapi -I./include/generated/uapi -include ../include/linux/compiler-version.h -include ../include/linux/kconfig.h -include ../include/linux/compiler_types.h -D__KERNEL__ -Qunused-arguments -fmacro-prefix-map=../= -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE -Werror=implicit-function-declaration -Werror=implicit-int -Werror=return-type -Wno-format-security -std=gnu11 --target=x86_64-linux-gnu -fintegrated-as -Werror=unknown-warning-option -Werror=ignored-optimization-argument -mno-sse -mno-mmx -mno-sse2 -mno-3dnow -mno-avx -fcf-protection=none -m64 -falign-loops=1 -mno-80387 -mno-fp-ret-in-387 -mstack-alignment=8 -mskip-rax-setup -mtune=generic -mno-red-zone -mcmodel=kernel -Wno-sign-compare -fno-asynchronous-unwind-tables -fno-delete-null-pointer-checks -Wno-frame-address -Wno-address-of-packed-member -O2 -Wframe-larger-than=2048 -fno-stack-protector -Wimplicit-fallthrough -Wno-gnu -Wno-unused-but-set-variable -Wno-unused-const-variable -fomit-frame-pointer -fno-stack-clash-protection -Wdeclaration-after-statement -Wvla -Wno-pointer-sign -Wcast-function-type -fno-strict-overflow -fno-stack-check -Werror=date-time -Werror=incompatible-pointer-types -Wno-initializer-overrides -Wno-format -Wno-sign-compare -Wno-pointer-to-enum-cast -Wno-tautological-constant-out-of-range-compare -Wno-unaligned-access -I ../drivers/vfio -I ./drivers/vfio -DMODULE -DKBUILD_BASENAME='"vfio_main"' -DKBUILD_MODNAME='"vfio"' -D__KBUILD_MODNAME=kmod_vfio -c -o /tmp/jnk.o ../drivers/vfio/vfio.h [no error] The criteria I like to use is if the header is able to compile stand-alone. > btw while they are moved here the inclusions in vfio_main.c are > not removed in patch8. ? I'm not sure I understand this Jason
On Wed, 31 Aug 2022 12:36:02 -0300 Jason Gunthorpe <jgg@nvidia.com> wrote: > On Wed, Aug 31, 2022 at 08:42:17AM +0000, Tian, Kevin wrote: > > > > diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h > > > index 503bea6c843d56..093784f1dea7a9 100644 > > > --- a/drivers/vfio/vfio.h > > > +++ b/drivers/vfio/vfio.h > > > @@ -3,6 +3,14 @@ > > > * Copyright (C) 2012 Red Hat, Inc. All rights reserved. > > > * Author: Alex Williamson <alex.williamson@redhat.com> > > > */ > > > +#ifndef __VFIO_VFIO_H__ > > > +#define __VFIO_VFIO_H__ > > > + > > > +#include <linux/device.h> > > > +#include <linux/cdev.h> > > > +#include <linux/module.h> > > > > Curious what is the criteria for which header inclusions should be > > placed here. If it is for everything required by the definitions in > > this file then the list is not complete, e.g. <linux/iommu.h> is > > obviously missing. > > It isn't missing: > > $ clang-14 -Wp,-MMD,drivers/vfio/.vfio_main.o.d -nostdinc -I../arch/x86/include -I./arch/x86/include/generated -I../include -I./include -I../arch/x86/include/uapi -I./arch/x86/include/generated/uapi -I../include/uapi -I./include/generated/uapi -include ../include/linux/compiler-version.h -include ../include/linux/kconfig.h -include ../include/linux/compiler_types.h -D__KERNEL__ -Qunused-arguments -fmacro-prefix-map=../= -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE -Werror=implicit-function-declaration -Werror=implicit-int -Werror=return-type -Wno-format-security -std=gnu11 --target=x86_64-linux-gnu -fintegrated-as -Werror=unknown-warning-option -Werror=ignored-optimization-argument -mno-sse -mno-mmx -mno-sse2 -mno-3dnow -mno-avx -fcf-protection=none -m64 -falign-loops=1 -mno-80387 -mno-fp-ret-in-387 -mstack-alignment=8 -mskip-rax-setup -mtune=generic -mno-red-zone -mcmodel=kernel -Wno-sign-compare -fno-asynchronous-u nwind-tables -fno-delete-null-pointer-checks -Wno-frame-address -Wno-address-of-packed-member -O2 -Wframe-larger-than=2048 -fno-stack-protector -Wimplicit-fallthrough -Wno-gnu -Wno-unused-but-set-variable -Wno-unused-const-variable -fomit-frame-pointer -fno-stack-clash-protection -Wdeclaration-after-statement -Wvla -Wno-pointer-sign -Wcast-function-type -fno-strict-overflow -fno-stack-check -Werror=date-time -Werror=incompatible-pointer-types -Wno-initializer-overrides -Wno-format -Wno-sign-compare -Wno-pointer-to-enum-cast -Wno-tautological-constant-out-of-range-compare -Wno-unaligned-access -I ../drivers/vfio -I ./drivers/vfio -DMODULE -DKBUILD_BASENAME='"vfio_main"' -DKBUILD_MODNAME='"vfio"' -D__KBUILD_MODNAME=kmod_vfio -c -o /tmp/jnk.o ../drivers/vfio/vfio.h > [no error] > > The criteria I like to use is if the header is able to compile > stand-alone. Is this stream of consciousness or is there some tooling for this? ;) > > btw while they are moved here the inclusions in vfio_main.c are > > not removed in patch8. > > ? I'm not sure I understand this I think Kevin is asking why these includes were not also removed from vfio_main.c when adding them to vfio.h. Thanks, Alex
On Wed, Aug 31, 2022 at 12:02:31PM -0600, Alex Williamson wrote: > > The criteria I like to use is if the header is able to compile > > stand-alone. > > Is this stream of consciousness or is there some tooling for this? ;) In my case I'm using clangd https://clangd.llvm.org/ in the editor, which checks header files for self-consistency. But there is also https://include-what-you-use.org/ (though I have never tried it) But the above wack of text is just the normal compiler invocation of vfio_main.c with main.c replaced by the header. > > > btw while they are moved here the inclusions in vfio_main.c are > > > not removed in patch8. > > > > ? I'm not sure I understand this > > I think Kevin is asking why these includes were not also removed from > vfio_main.c when adding them to vfio.h. Thanks, Oh, I am actually unclear what is policy/preference/consensus in that area. I know a strong camp is to avoid implicit includes, so the duplicated includes are welcomed. include-what-you-use for instance is that philosophy. Do you have a preference? Jason
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Thursday, September 1, 2022 2:25 AM > > On Wed, Aug 31, 2022 at 12:02:31PM -0600, Alex Williamson wrote: > > > > The criteria I like to use is if the header is able to compile > > > stand-alone. > > > > Is this stream of consciousness or is there some tooling for this? ;) > > In my case I'm using clangd https://clangd.llvm.org/ in the editor, > which checks header files for self-consistency. > > But there is also https://include-what-you-use.org/ (though I have > never tried it) > > But the above wack of text is just the normal compiler invocation of > vfio_main.c with main.c replaced by the header. Thanks. This is a good learning. > > > > > btw while they are moved here the inclusions in vfio_main.c are > > > > not removed in patch8. > > > > > > ? I'm not sure I understand this > > > > I think Kevin is asking why these includes were not also removed from > > vfio_main.c when adding them to vfio.h. Thanks, yes > > Oh, I am actually unclear what is policy/preference/consensus in that > area. me either. That is why I raised this open in case there is already a consensus in place.
diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h index 503bea6c843d56..093784f1dea7a9 100644 --- a/drivers/vfio/vfio.h +++ b/drivers/vfio/vfio.h @@ -3,6 +3,14 @@ * Copyright (C) 2012 Red Hat, Inc. All rights reserved. * Author: Alex Williamson <alex.williamson@redhat.com> */ +#ifndef __VFIO_VFIO_H__ +#define __VFIO_VFIO_H__ + +#include <linux/device.h> +#include <linux/cdev.h> +#include <linux/module.h> + +struct iommu_group; enum vfio_group_type { /* @@ -69,3 +77,5 @@ struct vfio_iommu_driver_ops { int vfio_register_iommu_driver(const struct vfio_iommu_driver_ops *ops); void vfio_unregister_iommu_driver(const struct vfio_iommu_driver_ops *ops); + +#endif
As is normal for headers. Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/vfio/vfio.h | 10 ++++++++++ 1 file changed, 10 insertions(+)