| Message ID | 20220404061948.2111820-9-masahiroy@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | UAPI: make more exported headers self-contained, and put them into test coverage | expand |
On Mon, Apr 04, 2022 at 03:19:48PM +0900, Masahiro Yamada wrote: > vr->num = num; > vr->desc = p; > vr->avail = (struct vring_avail *)((char *)p + num * sizeof(struct vring_desc)); > - vr->used = (void *)(((uintptr_t)&vr->avail->ring[num] + sizeof(__virtio16) > + vr->used = (void *)(((__kernel_uintptr_t)&vr->avail->ring[num] + sizeof(__virtio16) > + align-1) & ~(align - 1)); This really does not look like it should be in a uapi header to start with.
On Mon, Apr 4, 2022 at 4:44 PM Christoph Hellwig <hch@infradead.org> wrote: > > On Mon, Apr 04, 2022 at 03:19:48PM +0900, Masahiro Yamada wrote: > > vr->num = num; > > vr->desc = p; > > vr->avail = (struct vring_avail *)((char *)p + num * sizeof(struct vring_desc)); > > - vr->used = (void *)(((uintptr_t)&vr->avail->ring[num] + sizeof(__virtio16) > > + vr->used = (void *)(((__kernel_uintptr_t)&vr->avail->ring[num] + sizeof(__virtio16) > > + align-1) & ~(align - 1)); > > This really does not look like it should be in a uapi header to start > with. Then, could you send a proper fix? I have no idea how to do this...
On Mon, Apr 4, 2022 at 9:44 AM Christoph Hellwig <hch@infradead.org> wrote: > On Mon, Apr 04, 2022 at 03:19:48PM +0900, Masahiro Yamada wrote: > > vr->num = num; > > vr->desc = p; > > vr->avail = (struct vring_avail *)((char *)p + num * sizeof(struct vring_desc)); > > - vr->used = (void *)(((uintptr_t)&vr->avail->ring[num] + sizeof(__virtio16) > > + vr->used = (void *)(((__kernel_uintptr_t)&vr->avail->ring[num] + sizeof(__virtio16) > > + align-1) & ~(align - 1)); > > This really does not look like it should be in a uapi header to start > with. The header is shared between kernel and other projects using virtio, such as qemu and any boot loaders booting from virtio devices. It's not technically a /kernel/ ABI, but it is an ABI and for practical reasons the kernel version is maintained as the master copy if I understand it correctly. Arnd
On Mon, Apr 04, 2022 at 10:04:02AM +0200, Arnd Bergmann wrote: > The header is shared between kernel and other projects using virtio, such as > qemu and any boot loaders booting from virtio devices. It's not technically a > /kernel/ ABI, but it is an ABI and for practical reasons the kernel version is > maintained as the master copy if I understand it correctly. Besides that fact that as you correctly states these are not a UAPI at all, qemu and bootloades are not specific to Linux and can't require a specific kernel version. So the same thing we do for file system formats or network protocols applies here: just copy the damn header. And as stated above any reasonably portable userspace needs to have a copy anyway. If it is just as a "master copy" it can live in drivers/virtio/, just like we do for other formats.
On Tue, Apr 5, 2022 at 7:35 AM Christoph Hellwig <hch@infradead.org> wrote: > > On Mon, Apr 04, 2022 at 10:04:02AM +0200, Arnd Bergmann wrote: > > The header is shared between kernel and other projects using virtio, such as > > qemu and any boot loaders booting from virtio devices. It's not technically a > > /kernel/ ABI, but it is an ABI and for practical reasons the kernel version is > > maintained as the master copy if I understand it correctly. > > Besides that fact that as you correctly states these are not a UAPI at > all, qemu and bootloades are not specific to Linux and can't require a > specific kernel version. So the same thing we do for file system > formats or network protocols applies here: just copy the damn header. > And as stated above any reasonably portable userspace needs to have a > copy anyway. I think the users all have their own copies, at least the ones I could find on codesearch.debian.org. However, there are 27 virtio_*.h files in include/uapi/linux that probably should stay together for the purpose of defining the virtio protocol, and some others might be uapi relevant. I see that at least include/uapi/linux/vhost.h has ioctl() definitions in it, and includes the virtio_ring.h header indirectly. Adding the virtio maintainers to Cc to see if they can provide more background on this. > If it is just as a "master copy" it can live in drivers/virtio/, just > like we do for other formats. It has to be in include/linux/ at least because it's used by a number of drivers outside of drivers/virtio/. Arnd
On Tue, Apr 05, 2022 at 08:29:36AM +0200, Arnd Bergmann wrote: > I think the users all have their own copies, at least the ones I could > find on codesearch.debian.org. However, there are 27 virtio_*.h > files in include/uapi/linux that probably should stay together for > the purpose of defining the virtio protocol, and some others might > be uapi relevant. > > I see that at least include/uapi/linux/vhost.h has ioctl() definitions > in it, and includes the virtio_ring.h header indirectly. Uhh. We had a somilar mess (but at a smaller scale) in nvme, where the uapi nvme.h contained both the UAPI and the protocol definition. We took a hard break to only have a nvme_ioctl.h in the uapi header and linux/nvme.h for the protocol. This did break a bit of userspace compilation (but not running obviously) at the time, but really made the headers much easier to main. Some userspace keeps on copying nvme.h with the protocol definitions.
On Tue, Apr 05, 2022 at 12:01:35AM -0700, Christoph Hellwig wrote: > On Tue, Apr 05, 2022 at 08:29:36AM +0200, Arnd Bergmann wrote: > > I think the users all have their own copies, at least the ones I could > > find on codesearch.debian.org. However, there are 27 virtio_*.h > > files in include/uapi/linux that probably should stay together for > > the purpose of defining the virtio protocol, and some others might > > be uapi relevant. > > > > I see that at least include/uapi/linux/vhost.h has ioctl() definitions > > in it, and includes the virtio_ring.h header indirectly. > > Uhh. We had a somilar mess (but at a smaller scale) in nvme, where > the uapi nvme.h contained both the UAPI and the protocol definition. > We took a hard break to only have a nvme_ioctl.h in the uapi header > and linux/nvme.h for the protocol. This did break a bit of userspace > compilation (but not running obviously) at the time, but really made > the headers much easier to main. Some userspace keeps on copying > nvme.h with the protocol definitions. So far we are quite happy with the status quo, I don't see any issues maintaining the headers. And yes, through vhost and vringh they are part of UAPI. Yes users have their own copies but they synch with the kernel. That's generally. Specifically the vring_init thing is a legacy thingy used by kvmtool and maybe others, and it inits the ring in the way that vring/virtio expect. Has been there since day 1 and we are careful not to add more stuff like that, so I don't see a lot of gain from incurring this pain for users.
On Tue, Apr 05, 2022 at 08:29:36AM +0200, Arnd Bergmann wrote: > On Tue, Apr 5, 2022 at 7:35 AM Christoph Hellwig <hch@infradead.org> wrote: > > > > On Mon, Apr 04, 2022 at 10:04:02AM +0200, Arnd Bergmann wrote: > > > The header is shared between kernel and other projects using virtio, such as > > > qemu and any boot loaders booting from virtio devices. It's not technically a > > > /kernel/ ABI, but it is an ABI and for practical reasons the kernel version is > > > maintained as the master copy if I understand it correctly. > > > > Besides that fact that as you correctly states these are not a UAPI at > > all, qemu and bootloades are not specific to Linux and can't require a > > specific kernel version. So the same thing we do for file system > > formats or network protocols applies here: just copy the damn header. > > And as stated above any reasonably portable userspace needs to have a > > copy anyway. > > I think the users all have their own copies, at least the ones I could > find on codesearch.debian.org. kvmtool does not seem to have its own copy, just grep vring_init. > However, there are 27 virtio_*.h > files in include/uapi/linux that probably should stay together for > the purpose of defining the virtio protocol, and some others might > be uapi relevant. > > I see that at least include/uapi/linux/vhost.h has ioctl() definitions > in it, and includes the virtio_ring.h header indirectly. > > Adding the virtio maintainers to Cc to see if they can provide > more background on this. > > > If it is just as a "master copy" it can live in drivers/virtio/, just > > like we do for other formats. > > It has to be in include/linux/ at least because it's used by a number > of drivers outside of drivers/virtio/. > > Arnd > _______________________________________________ > Virtualization mailing list > Virtualization@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/virtualization >
On Mon, Apr 04, 2022 at 03:19:48PM +0900, Masahiro Yamada wrote: > Arnd mentioned a limitation when including <stdint.h> from UAPI > headers. [1] > > Besides, I'd like exported headers to be as compliant with the > traditional C as possible. > > In fact, the UAPI headers are compile-tested with -std=c90 (see > usr/include/Makefile) even though the kernel itself is now built > with -std=gnu11. > > Currently, include/uapi/linux/virtio_ring.h includes <stdint.h> > presumably because it uses uintptr_t. > > Replace it with __kernel_uintptr_t, and stop including <stdint.h>. > > [1]: https://lore.kernel.org/all/CAK8P3a0bz8XYJOsmND2=CT_oTDmGMJGaRo9+QMroEhpekSMEaQ@mail.gmail.com/ > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> Acked-by: Michael S. Tsirkin <mst@redhat.com> > --- > > include/uapi/linux/virtio_ring.h | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h > index 476d3e5c0fe7..6329e4ff35f4 100644 > --- a/include/uapi/linux/virtio_ring.h > +++ b/include/uapi/linux/virtio_ring.h > @@ -31,9 +31,7 @@ > * SUCH DAMAGE. > * > * Copyright Rusty Russell IBM Corporation 2007. */ > -#ifndef __KERNEL__ > -#include <stdint.h> > -#endif > + > #include <linux/types.h> > #include <linux/virtio_types.h> > > @@ -196,7 +194,7 @@ static inline void vring_init(struct vring *vr, unsigned int num, void *p, > vr->num = num; > vr->desc = p; > vr->avail = (struct vring_avail *)((char *)p + num * sizeof(struct vring_desc)); > - vr->used = (void *)(((uintptr_t)&vr->avail->ring[num] + sizeof(__virtio16) > + vr->used = (void *)(((__kernel_uintptr_t)&vr->avail->ring[num] + sizeof(__virtio16) > + align-1) & ~(align - 1)); > } > > -- > 2.32.0 > >
On Mon, Apr 04, 2022 at 12:44:30AM -0700, Christoph Hellwig wrote: > On Mon, Apr 04, 2022 at 03:19:48PM +0900, Masahiro Yamada wrote: > > vr->num = num; > > vr->desc = p; > > vr->avail = (struct vring_avail *)((char *)p + num * sizeof(struct vring_desc)); > > - vr->used = (void *)(((uintptr_t)&vr->avail->ring[num] + sizeof(__virtio16) > > + vr->used = (void *)(((__kernel_uintptr_t)&vr->avail->ring[num] + sizeof(__virtio16) > > + align-1) & ~(align - 1)); > > This really does not look like it should be in a uapi header to start > with. It's a way to document a complex structure layout of virtio 0.9. It's ugly but it's been like this for years. For virtio 1.0 we moved away from this but a bunch of tools keep using legacy.
diff --git a/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h index 476d3e5c0fe7..6329e4ff35f4 100644 --- a/include/uapi/linux/virtio_ring.h +++ b/include/uapi/linux/virtio_ring.h @@ -31,9 +31,7 @@ * SUCH DAMAGE. * * Copyright Rusty Russell IBM Corporation 2007. */ -#ifndef __KERNEL__ -#include <stdint.h> -#endif + #include <linux/types.h> #include <linux/virtio_types.h> @@ -196,7 +194,7 @@ static inline void vring_init(struct vring *vr, unsigned int num, void *p, vr->num = num; vr->desc = p; vr->avail = (struct vring_avail *)((char *)p + num * sizeof(struct vring_desc)); - vr->used = (void *)(((uintptr_t)&vr->avail->ring[num] + sizeof(__virtio16) + vr->used = (void *)(((__kernel_uintptr_t)&vr->avail->ring[num] + sizeof(__virtio16) + align-1) & ~(align - 1)); }
Arnd mentioned a limitation when including <stdint.h> from UAPI headers. [1] Besides, I'd like exported headers to be as compliant with the traditional C as possible. In fact, the UAPI headers are compile-tested with -std=c90 (see usr/include/Makefile) even though the kernel itself is now built with -std=gnu11. Currently, include/uapi/linux/virtio_ring.h includes <stdint.h> presumably because it uses uintptr_t. Replace it with __kernel_uintptr_t, and stop including <stdint.h>. [1]: https://lore.kernel.org/all/CAK8P3a0bz8XYJOsmND2=CT_oTDmGMJGaRo9+QMroEhpekSMEaQ@mail.gmail.com/ Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> --- include/uapi/linux/virtio_ring.h | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)