* [PATCH RESEND] virtio: Fix typecast of pointer in vring_init()
@ 2015-07-02 7:21 Thomas Huth
2015-07-05 10:58 ` Michael S. Tsirkin
0 siblings, 1 reply; 6+ messages in thread
From: Thomas Huth @ 2015-07-02 7:21 UTC (permalink / raw)
To: Michael S. Tsirkin, virtualization; +Cc: linux-api, linux-kernel
The virtio_ring.h header is used in userspace programs (ie. QEMU),
too. Here we can not assume that sizeof(pointer) is the same as
sizeof(long), e.g. when compiling for Windows, so the typecast in
vring_init() should be done with (uintptr_t) instead of (unsigned long).
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
include/uapi/linux/virtio_ring.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h
index 915980a..8682551 100644
--- a/include/uapi/linux/virtio_ring.h
+++ b/include/uapi/linux/virtio_ring.h
@@ -143,7 +143,7 @@ static inline void vring_init(struct vring *vr, unsigned int num, void *p,
vr->num = num;
vr->desc = p;
vr->avail = p + num*sizeof(struct vring_desc);
- vr->used = (void *)(((unsigned long)&vr->avail->ring[num] + sizeof(__virtio16)
+ vr->used = (void *)(((uintptr_t)&vr->avail->ring[num] + sizeof(__virtio16)
+ align-1) & ~(align - 1));
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH RESEND] virtio: Fix typecast of pointer in vring_init()
2015-07-02 7:21 [PATCH RESEND] virtio: Fix typecast of pointer in vring_init() Thomas Huth
@ 2015-07-05 10:58 ` Michael S. Tsirkin
2015-07-05 12:59 ` Michael S. Tsirkin
0 siblings, 1 reply; 6+ messages in thread
From: Michael S. Tsirkin @ 2015-07-05 10:58 UTC (permalink / raw)
To: Thomas Huth; +Cc: virtualization, linux-api, linux-kernel
On Thu, Jul 02, 2015 at 09:21:22AM +0200, Thomas Huth wrote:
> The virtio_ring.h header is used in userspace programs (ie. QEMU),
> too. Here we can not assume that sizeof(pointer) is the same as
> sizeof(long), e.g. when compiling for Windows, so the typecast in
> vring_init() should be done with (uintptr_t) instead of (unsigned long).
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
This seems to break some userspace too:
INSTALL usr/include/linux/ (413 files)
CHECK usr/include/linux/ (413 files)
HOSTCC Documentation/accounting/getdelays
HOSTCC Documentation/connector/ucon
HOSTCC Documentation/mic/mpssd/mpssd.o
In file included from Documentation/mic/mpssd/mpssd.c:36:0:
./usr/include/linux/virtio_ring.h: In function ‘vring_init’:
./usr/include/linux/virtio_ring.h:146:24: error: ‘uintptr_t’ undeclared
(first use in this function)
vr->used = (void *)(((uintptr_t)&vr->avail->ring[num] +
sizeof(__virtio16)
^
./usr/include/linux/virtio_ring.h:146:24: note: each undeclared
identifier is reported only once for each function it appears in
scripts/Makefile.host:108: recipe for target
'Documentation/mic/mpssd/mpssd.o' failed
make[3]: *** [Documentation/mic/mpssd/mpssd.o] Error 1
scripts/Makefile.build:403: recipe for target 'Documentation/mic/mpssd'
failed
make[2]: *** [Documentation/mic/mpssd] Error 2
scripts/Makefile.build:403: recipe for target 'Documentation/mic' failed
make[1]: *** [Documentation/mic] Error 2
E.g. fuse.h has this code:
#ifdef __KERNEL__
#include <linux/types.h>
#else
#include <stdint.h>
#endif
Maybe we need something similar.
> ---
> include/uapi/linux/virtio_ring.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h
> index 915980a..8682551 100644
> --- a/include/uapi/linux/virtio_ring.h
> +++ b/include/uapi/linux/virtio_ring.h
> @@ -143,7 +143,7 @@ static inline void vring_init(struct vring *vr, unsigned int num, void *p,
> vr->num = num;
> vr->desc = p;
> vr->avail = p + num*sizeof(struct vring_desc);
> - vr->used = (void *)(((unsigned long)&vr->avail->ring[num] + sizeof(__virtio16)
> + vr->used = (void *)(((uintptr_t)&vr->avail->ring[num] + sizeof(__virtio16)
> + align-1) & ~(align - 1));
> }
>
> --
> 1.8.3.1
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH RESEND] virtio: Fix typecast of pointer in vring_init()
2015-07-05 10:58 ` Michael S. Tsirkin
@ 2015-07-05 12:59 ` Michael S. Tsirkin
2015-07-06 9:24 ` Thomas Huth
0 siblings, 1 reply; 6+ messages in thread
From: Michael S. Tsirkin @ 2015-07-05 12:59 UTC (permalink / raw)
To: Thomas Huth; +Cc: virtualization, linux-api, linux-kernel
On Sun, Jul 05, 2015 at 12:58:53PM +0200, Michael S. Tsirkin wrote:
> On Thu, Jul 02, 2015 at 09:21:22AM +0200, Thomas Huth wrote:
> > The virtio_ring.h header is used in userspace programs (ie. QEMU),
> > too. Here we can not assume that sizeof(pointer) is the same as
> > sizeof(long), e.g. when compiling for Windows, so the typecast in
> > vring_init() should be done with (uintptr_t) instead of (unsigned long).
> >
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
>
> This seems to break some userspace too:
>
> INSTALL usr/include/linux/ (413 files)
> CHECK usr/include/linux/ (413 files)
> HOSTCC Documentation/accounting/getdelays
> HOSTCC Documentation/connector/ucon
> HOSTCC Documentation/mic/mpssd/mpssd.o
> In file included from Documentation/mic/mpssd/mpssd.c:36:0:
> ./usr/include/linux/virtio_ring.h: In function ‘vring_init’:
> ./usr/include/linux/virtio_ring.h:146:24: error: ‘uintptr_t’ undeclared
> (first use in this function)
> vr->used = (void *)(((uintptr_t)&vr->avail->ring[num] +
> sizeof(__virtio16)
> ^
> ./usr/include/linux/virtio_ring.h:146:24: note: each undeclared
> identifier is reported only once for each function it appears in
> scripts/Makefile.host:108: recipe for target
> 'Documentation/mic/mpssd/mpssd.o' failed
> make[3]: *** [Documentation/mic/mpssd/mpssd.o] Error 1
> scripts/Makefile.build:403: recipe for target 'Documentation/mic/mpssd'
> failed
> make[2]: *** [Documentation/mic/mpssd] Error 2
> scripts/Makefile.build:403: recipe for target 'Documentation/mic' failed
> make[1]: *** [Documentation/mic] Error 2
>
>
> E.g. fuse.h has this code:
> #ifdef __KERNEL__
> #include <linux/types.h>
> #else
> #include <stdint.h>
> #endif
>
> Maybe we need something similar.
The following seems to help. Does it help the windows build?
---
include/uapi/linux/virtio_ring.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h
index 8682551..c072959 100644
--- a/include/uapi/linux/virtio_ring.h
+++ b/include/uapi/linux/virtio_ring.h
@@ -31,6 +31,9 @@
* SUCH DAMAGE.
*
* Copyright Rusty Russell IBM Corporation 2007. */
+#ifndef __KERNEL__
+#include <stdint.h>
+#endif
#include <linux/types.h>
#include <linux/virtio_types.h>
--
MST
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH RESEND] virtio: Fix typecast of pointer in vring_init()
2015-07-05 12:59 ` Michael S. Tsirkin
@ 2015-07-06 9:24 ` Thomas Huth
2015-07-06 9:50 ` Michael S. Tsirkin
0 siblings, 1 reply; 6+ messages in thread
From: Thomas Huth @ 2015-07-06 9:24 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtualization, linux-api, linux-kernel
On Sun, 5 Jul 2015 14:59:54 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Sun, Jul 05, 2015 at 12:58:53PM +0200, Michael S. Tsirkin wrote:
> > On Thu, Jul 02, 2015 at 09:21:22AM +0200, Thomas Huth wrote:
> > > The virtio_ring.h header is used in userspace programs (ie. QEMU),
> > > too. Here we can not assume that sizeof(pointer) is the same as
> > > sizeof(long), e.g. when compiling for Windows, so the typecast in
> > > vring_init() should be done with (uintptr_t) instead of (unsigned long).
> > >
> > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> >
> > This seems to break some userspace too:
> >
> > INSTALL usr/include/linux/ (413 files)
> > CHECK usr/include/linux/ (413 files)
> > HOSTCC Documentation/accounting/getdelays
> > HOSTCC Documentation/connector/ucon
> > HOSTCC Documentation/mic/mpssd/mpssd.o
> > In file included from Documentation/mic/mpssd/mpssd.c:36:0:
> > ./usr/include/linux/virtio_ring.h: In function ‘vring_init’:
> > ./usr/include/linux/virtio_ring.h:146:24: error: ‘uintptr_t’ undeclared
> > (first use in this function)
> > vr->used = (void *)(((uintptr_t)&vr->avail->ring[num] +
> > sizeof(__virtio16)
> > ^
> > ./usr/include/linux/virtio_ring.h:146:24: note: each undeclared
> > identifier is reported only once for each function it appears in
> > scripts/Makefile.host:108: recipe for target
D'oh, I only built the kernel for powerpc, that's why I did not notice
this breakage in the MIC code. Sorry!
> > E.g. fuse.h has this code:
> > #ifdef __KERNEL__
> > #include <linux/types.h>
> > #else
> > #include <stdint.h>
> > #endif
> >
> > Maybe we need something similar.
>
> The following seems to help. Does it help the windows build?
...
> diff --git a/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h
> index 8682551..c072959 100644
> --- a/include/uapi/linux/virtio_ring.h
> +++ b/include/uapi/linux/virtio_ring.h
> @@ -31,6 +31,9 @@
> * SUCH DAMAGE.
> *
> * Copyright Rusty Russell IBM Corporation 2007. */
> +#ifndef __KERNEL__
> +#include <stdint.h>
> +#endif
> #include <linux/types.h>
> #include <linux/virtio_types.h>
Since the update-linux-headers.sh script from QEMU replaces the
"#include <linux/types.h>" in virtio_ring.h to include a file that again
includes stdint.h already, your above patch should not do any
difference for compiling QEMU for Windows, I think. It's really just
about replacing the typecast there.
But you're right of course for other user space applications which
include this header by other means. So I'm fine if you change my patch
with your hunk above (or add it as an additional patch). Or shall I
rather resend my patch with your above hunk included?
Thomas
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RESEND] virtio: Fix typecast of pointer in vring_init()
2015-07-06 9:24 ` Thomas Huth
@ 2015-07-06 9:50 ` Michael S. Tsirkin
2015-07-06 10:05 ` Thomas Huth
0 siblings, 1 reply; 6+ messages in thread
From: Michael S. Tsirkin @ 2015-07-06 9:50 UTC (permalink / raw)
To: Thomas Huth; +Cc: virtualization, linux-api, linux-kernel
On Mon, Jul 06, 2015 at 11:24:42AM +0200, Thomas Huth wrote:
> On Sun, 5 Jul 2015 14:59:54 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Sun, Jul 05, 2015 at 12:58:53PM +0200, Michael S. Tsirkin wrote:
> > > On Thu, Jul 02, 2015 at 09:21:22AM +0200, Thomas Huth wrote:
> > > > The virtio_ring.h header is used in userspace programs (ie. QEMU),
> > > > too. Here we can not assume that sizeof(pointer) is the same as
> > > > sizeof(long), e.g. when compiling for Windows, so the typecast in
> > > > vring_init() should be done with (uintptr_t) instead of (unsigned long).
> > > >
> > > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > >
> > > This seems to break some userspace too:
> > >
> > > INSTALL usr/include/linux/ (413 files)
> > > CHECK usr/include/linux/ (413 files)
> > > HOSTCC Documentation/accounting/getdelays
> > > HOSTCC Documentation/connector/ucon
> > > HOSTCC Documentation/mic/mpssd/mpssd.o
> > > In file included from Documentation/mic/mpssd/mpssd.c:36:0:
> > > ./usr/include/linux/virtio_ring.h: In function ‘vring_init’:
> > > ./usr/include/linux/virtio_ring.h:146:24: error: ‘uintptr_t’ undeclared
> > > (first use in this function)
> > > vr->used = (void *)(((uintptr_t)&vr->avail->ring[num] +
> > > sizeof(__virtio16)
> > > ^
> > > ./usr/include/linux/virtio_ring.h:146:24: note: each undeclared
> > > identifier is reported only once for each function it appears in
> > > scripts/Makefile.host:108: recipe for target
>
> D'oh, I only built the kernel for powerpc, that's why I did not notice
> this breakage in the MIC code. Sorry!
>
> > > E.g. fuse.h has this code:
> > > #ifdef __KERNEL__
> > > #include <linux/types.h>
> > > #else
> > > #include <stdint.h>
> > > #endif
> > >
> > > Maybe we need something similar.
> >
> > The following seems to help. Does it help the windows build?
> ...
> > diff --git a/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h
> > index 8682551..c072959 100644
> > --- a/include/uapi/linux/virtio_ring.h
> > +++ b/include/uapi/linux/virtio_ring.h
> > @@ -31,6 +31,9 @@
> > * SUCH DAMAGE.
> > *
> > * Copyright Rusty Russell IBM Corporation 2007. */
> > +#ifndef __KERNEL__
> > +#include <stdint.h>
> > +#endif
> > #include <linux/types.h>
> > #include <linux/virtio_types.h>
>
> Since the update-linux-headers.sh script from QEMU replaces the
> "#include <linux/types.h>" in virtio_ring.h to include a file that again
> includes stdint.h already, your above patch should not do any
> difference for compiling QEMU for Windows, I think. It's really just
> about replacing the typecast there.
>
> But you're right of course for other user space applications which
> include this header by other means. So I'm fine if you change my patch
> with your hunk above (or add it as an additional patch). Or shall I
> rather resend my patch with your above hunk included?
>
> Thomas
You don't have to, but could you confirm that stdint has uintptr_t on
mingw?
--
MST
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RESEND] virtio: Fix typecast of pointer in vring_init()
2015-07-06 9:50 ` Michael S. Tsirkin
@ 2015-07-06 10:05 ` Thomas Huth
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Huth @ 2015-07-06 10:05 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtualization, linux-api, linux-kernel
On Mon, 6 Jul 2015 12:50:22 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, Jul 06, 2015 at 11:24:42AM +0200, Thomas Huth wrote:
> > On Sun, 5 Jul 2015 14:59:54 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > > On Sun, Jul 05, 2015 at 12:58:53PM +0200, Michael S. Tsirkin wrote:
> > > > On Thu, Jul 02, 2015 at 09:21:22AM +0200, Thomas Huth wrote:
> > > > > The virtio_ring.h header is used in userspace programs (ie. QEMU),
> > > > > too. Here we can not assume that sizeof(pointer) is the same as
> > > > > sizeof(long), e.g. when compiling for Windows, so the typecast in
> > > > > vring_init() should be done with (uintptr_t) instead of (unsigned long).
> > > > >
> > > > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > >
> > > > This seems to break some userspace too:
> > > >
> > > > INSTALL usr/include/linux/ (413 files)
> > > > CHECK usr/include/linux/ (413 files)
> > > > HOSTCC Documentation/accounting/getdelays
> > > > HOSTCC Documentation/connector/ucon
> > > > HOSTCC Documentation/mic/mpssd/mpssd.o
> > > > In file included from Documentation/mic/mpssd/mpssd.c:36:0:
> > > > ./usr/include/linux/virtio_ring.h: In function ‘vring_init’:
> > > > ./usr/include/linux/virtio_ring.h:146:24: error: ‘uintptr_t’ undeclared
> > > > (first use in this function)
> > > > vr->used = (void *)(((uintptr_t)&vr->avail->ring[num] +
> > > > sizeof(__virtio16)
> > > > ^
> > > > ./usr/include/linux/virtio_ring.h:146:24: note: each undeclared
> > > > identifier is reported only once for each function it appears in
> > > > scripts/Makefile.host:108: recipe for target
> >
> > D'oh, I only built the kernel for powerpc, that's why I did not notice
> > this breakage in the MIC code. Sorry!
> >
> > > > E.g. fuse.h has this code:
> > > > #ifdef __KERNEL__
> > > > #include <linux/types.h>
> > > > #else
> > > > #include <stdint.h>
> > > > #endif
> > > >
> > > > Maybe we need something similar.
> > >
> > > The following seems to help. Does it help the windows build?
> > ...
> > > diff --git a/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h
> > > index 8682551..c072959 100644
> > > --- a/include/uapi/linux/virtio_ring.h
> > > +++ b/include/uapi/linux/virtio_ring.h
> > > @@ -31,6 +31,9 @@
> > > * SUCH DAMAGE.
> > > *
> > > * Copyright Rusty Russell IBM Corporation 2007. */
> > > +#ifndef __KERNEL__
> > > +#include <stdint.h>
> > > +#endif
> > > #include <linux/types.h>
> > > #include <linux/virtio_types.h>
> >
> > Since the update-linux-headers.sh script from QEMU replaces the
> > "#include <linux/types.h>" in virtio_ring.h to include a file that again
> > includes stdint.h already, your above patch should not do any
> > difference for compiling QEMU for Windows, I think. It's really just
> > about replacing the typecast there.
> >
> > But you're right of course for other user space applications which
> > include this header by other means. So I'm fine if you change my patch
> > with your hunk above (or add it as an additional patch). Or shall I
> > rather resend my patch with your above hunk included?
> >
> > Thomas
>
> You don't have to, but could you confirm that stdint has uintptr_t on
> mingw?
>
$ grep typedef.*uintptr /usr/x86_64-w64-mingw32/sys-root/mingw/include/*.h
...
/usr/x86_64-w64-mingw32/sys-root/mingw/include/crtdefs.h:__MINGW_EXTENSION typedef unsigned __int64 uintptr_t;
...
And the stdint.h from my MinGW installation includes crtdefs.h.
So yes, confirmed that stdint.h provides uintptr_t there, too.
Thomas
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-07-06 10:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-07-02 7:21 [PATCH RESEND] virtio: Fix typecast of pointer in vring_init() Thomas Huth
2015-07-05 10:58 ` Michael S. Tsirkin
2015-07-05 12:59 ` Michael S. Tsirkin
2015-07-06 9:24 ` Thomas Huth
2015-07-06 9:50 ` Michael S. Tsirkin
2015-07-06 10:05 ` Thomas Huth
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox