* [Qemu-devel] [PATCH] xen-usb.c/usbif.h: fix ARM64 build issue
@ 2016-10-18 19:09 Stefano Stabellini
2016-10-19 8:50 ` Wei Liu
0 siblings, 1 reply; 7+ messages in thread
From: Stefano Stabellini @ 2016-10-18 19:09 UTC (permalink / raw)
To: xen-devel
Cc: wei.liu2, jgross, sstabellini, qemu-devel, anthony.perard,
julien.grall
Hi all,
While I was investigating the recent libxl ARM64 build issue, I found
another ARM64 build problem. This one is in QEMU:
CC hw/usb/xen-usb.o
hw/usb/xen-usb.c: In function ‘usbback_gnttab_map’:
hw/usb/xen-usb.c:163:56: error: ‘PAGE_SIZE’ undeclared (first use in this function)
(unsigned)usbback_req->req.seg[i].length > PAGE_SIZE) {
^
hw/usb/xen-usb.c:163:56: note: each undeclared identifier is reported only once for each function it appears in
In file included from hw/usb/xen-usb.c:36:0:
hw/usb/xen-usb.c: In function ‘usbback_alloc’:
/users/stefanos/xen/tools/../tools/include/xen/io/usbif.h:229:56: error: ‘PAGE_SIZE’ undeclared (first use in this function)
#define USB_URB_RING_SIZE __CONST_RING_SIZE(usbif_urb, PAGE_SIZE)
^
/users/stefanos/xen/tools/../tools/include/xen/io/ring.h:45:23: note: in definition of macro ‘__RD32’
#define __RD32(_x) (((_x) & 0xffff0000) ? __RD16((_x)>>16)<<16 : __RD16(_x))
^
/users/stefanos/xen/tools/../tools/include/xen/io/usbif.h:229:27: note: in expansion of macro ‘__CONST_RING_SIZE’
#define USB_URB_RING_SIZE __CONST_RING_SIZE(usbif_urb, PAGE_SIZE)
^
hw/usb/xen-usb.c:1029:51: note: in expansion of macro ‘USB_URB_RING_SIZE’
max_grants = USBIF_MAX_SEGMENTS_PER_REQUEST * USB_URB_RING_SIZE + 2;
^
make: *** [hw/usb/xen-usb.o] Error 1
It is caused by PAGE_SIZE being utilized in
xen/include/public/io/usbif.h, but undefined on ARM64 (in userspace).
QEMU includes xen/include/public/io/usbif.h directly, therefore
hw/usb/xen-usb.c doesn't build.
I don't think we should be using "PAGE_SIZE" in public/io/usbif.h, but
that file is present in too many Xen releases to ignore now. So I am
going to work around the issue in QEMU.
---
xen-usb.c does not build on ARM64 because it includes xen/io/usbif.h
which uses PAGE_SIZE without defining it. The header file has been
shipped with too many Xen releases to be able to solve the problem at
the source.
This patch define PAGE_SIZE as XC_PAGE_SIZE when undefined.
Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
diff --git a/hw/usb/xen-usb.c b/hw/usb/xen-usb.c
index 174d715..ca9df87 100644
--- a/hw/usb/xen-usb.c
+++ b/hw/usb/xen-usb.c
@@ -34,6 +34,11 @@
#include "qapi/qmp/qstring.h"
#include <xen/io/ring.h>
+
+/* xen/io/usbif.h references PAGE_SIZE without defining it */
+#ifndef PAGE_SIZE
+#define PAGE_SIZE XC_PAGE_SIZE
+#endif
#include <xen/io/usbif.h>
/*
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH] xen-usb.c/usbif.h: fix ARM64 build issue 2016-10-18 19:09 [Qemu-devel] [PATCH] xen-usb.c/usbif.h: fix ARM64 build issue Stefano Stabellini @ 2016-10-19 8:50 ` Wei Liu 2016-10-19 9:09 ` Juergen Gross 0 siblings, 1 reply; 7+ messages in thread From: Wei Liu @ 2016-10-19 8:50 UTC (permalink / raw) To: Stefano Stabellini Cc: xen-devel, wei.liu2, jgross, qemu-devel, anthony.perard, julien.grall On Tue, Oct 18, 2016 at 12:09:55PM -0700, Stefano Stabellini wrote: > Hi all, > > While I was investigating the recent libxl ARM64 build issue, I found > another ARM64 build problem. This one is in QEMU: > > > CC hw/usb/xen-usb.o > hw/usb/xen-usb.c: In function ‘usbback_gnttab_map’: > hw/usb/xen-usb.c:163:56: error: ‘PAGE_SIZE’ undeclared (first use in this function) > (unsigned)usbback_req->req.seg[i].length > PAGE_SIZE) { > ^ > hw/usb/xen-usb.c:163:56: note: each undeclared identifier is reported only once for each function it appears in > In file included from hw/usb/xen-usb.c:36:0: > hw/usb/xen-usb.c: In function ‘usbback_alloc’: > /users/stefanos/xen/tools/../tools/include/xen/io/usbif.h:229:56: error: ‘PAGE_SIZE’ undeclared (first use in this function) > #define USB_URB_RING_SIZE __CONST_RING_SIZE(usbif_urb, PAGE_SIZE) > ^ > /users/stefanos/xen/tools/../tools/include/xen/io/ring.h:45:23: note: in definition of macro ‘__RD32’ > #define __RD32(_x) (((_x) & 0xffff0000) ? __RD16((_x)>>16)<<16 : __RD16(_x)) > ^ > /users/stefanos/xen/tools/../tools/include/xen/io/usbif.h:229:27: note: in expansion of macro ‘__CONST_RING_SIZE’ > #define USB_URB_RING_SIZE __CONST_RING_SIZE(usbif_urb, PAGE_SIZE) > ^ > hw/usb/xen-usb.c:1029:51: note: in expansion of macro ‘USB_URB_RING_SIZE’ > max_grants = USBIF_MAX_SEGMENTS_PER_REQUEST * USB_URB_RING_SIZE + 2; > ^ > make: *** [hw/usb/xen-usb.o] Error 1 > > > It is caused by PAGE_SIZE being utilized in > xen/include/public/io/usbif.h, but undefined on ARM64 (in userspace). > QEMU includes xen/include/public/io/usbif.h directly, therefore > hw/usb/xen-usb.c doesn't build. > > I don't think we should be using "PAGE_SIZE" in public/io/usbif.h, but > that file is present in too many Xen releases to ignore now. So I am > going to work around the issue in QEMU. > Actually it was introduced in 4.7. > --- > > xen-usb.c does not build on ARM64 because it includes xen/io/usbif.h > which uses PAGE_SIZE without defining it. The header file has been > shipped with too many Xen releases to be able to solve the problem at > the source. > > This patch define PAGE_SIZE as XC_PAGE_SIZE when undefined. > > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org> > > diff --git a/hw/usb/xen-usb.c b/hw/usb/xen-usb.c > index 174d715..ca9df87 100644 > --- a/hw/usb/xen-usb.c > +++ b/hw/usb/xen-usb.c > @@ -34,6 +34,11 @@ > #include "qapi/qmp/qstring.h" > > #include <xen/io/ring.h> > + > +/* xen/io/usbif.h references PAGE_SIZE without defining it */ > +#ifndef PAGE_SIZE > +#define PAGE_SIZE XC_PAGE_SIZE > +#endif > #include <xen/io/usbif.h> Can we just change the macro to #define USB_CONN_RING_SIZE(pg_size) __CONST_RING_SIZE(usbif_conn, (pg_size) ? I also found a similar issue in vscsiif.h. Wei. > > /* ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] xen-usb.c/usbif.h: fix ARM64 build issue 2016-10-19 8:50 ` Wei Liu @ 2016-10-19 9:09 ` Juergen Gross 2016-10-19 9:22 ` Wei Liu 0 siblings, 1 reply; 7+ messages in thread From: Juergen Gross @ 2016-10-19 9:09 UTC (permalink / raw) To: Wei Liu, Stefano Stabellini Cc: xen-devel, qemu-devel, anthony.perard, julien.grall On 19/10/16 10:50, Wei Liu wrote: > On Tue, Oct 18, 2016 at 12:09:55PM -0700, Stefano Stabellini wrote: >> Hi all, >> >> While I was investigating the recent libxl ARM64 build issue, I found >> another ARM64 build problem. This one is in QEMU: >> >> >> CC hw/usb/xen-usb.o >> hw/usb/xen-usb.c: In function ‘usbback_gnttab_map’: >> hw/usb/xen-usb.c:163:56: error: ‘PAGE_SIZE’ undeclared (first use in this function) >> (unsigned)usbback_req->req.seg[i].length > PAGE_SIZE) { >> ^ >> hw/usb/xen-usb.c:163:56: note: each undeclared identifier is reported only once for each function it appears in >> In file included from hw/usb/xen-usb.c:36:0: >> hw/usb/xen-usb.c: In function ‘usbback_alloc’: >> /users/stefanos/xen/tools/../tools/include/xen/io/usbif.h:229:56: error: ‘PAGE_SIZE’ undeclared (first use in this function) >> #define USB_URB_RING_SIZE __CONST_RING_SIZE(usbif_urb, PAGE_SIZE) >> ^ >> /users/stefanos/xen/tools/../tools/include/xen/io/ring.h:45:23: note: in definition of macro ‘__RD32’ >> #define __RD32(_x) (((_x) & 0xffff0000) ? __RD16((_x)>>16)<<16 : __RD16(_x)) >> ^ >> /users/stefanos/xen/tools/../tools/include/xen/io/usbif.h:229:27: note: in expansion of macro ‘__CONST_RING_SIZE’ >> #define USB_URB_RING_SIZE __CONST_RING_SIZE(usbif_urb, PAGE_SIZE) >> ^ >> hw/usb/xen-usb.c:1029:51: note: in expansion of macro ‘USB_URB_RING_SIZE’ >> max_grants = USBIF_MAX_SEGMENTS_PER_REQUEST * USB_URB_RING_SIZE + 2; >> ^ >> make: *** [hw/usb/xen-usb.o] Error 1 >> >> >> It is caused by PAGE_SIZE being utilized in >> xen/include/public/io/usbif.h, but undefined on ARM64 (in userspace). >> QEMU includes xen/include/public/io/usbif.h directly, therefore >> hw/usb/xen-usb.c doesn't build. >> >> I don't think we should be using "PAGE_SIZE" in public/io/usbif.h, but >> that file is present in too many Xen releases to ignore now. So I am >> going to work around the issue in QEMU. >> > > Actually it was introduced in 4.7. > >> --- >> >> xen-usb.c does not build on ARM64 because it includes xen/io/usbif.h >> which uses PAGE_SIZE without defining it. The header file has been >> shipped with too many Xen releases to be able to solve the problem at >> the source. >> >> This patch define PAGE_SIZE as XC_PAGE_SIZE when undefined. >> >> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org> >> >> diff --git a/hw/usb/xen-usb.c b/hw/usb/xen-usb.c >> index 174d715..ca9df87 100644 >> --- a/hw/usb/xen-usb.c >> +++ b/hw/usb/xen-usb.c >> @@ -34,6 +34,11 @@ >> #include "qapi/qmp/qstring.h" >> >> #include <xen/io/ring.h> >> + >> +/* xen/io/usbif.h references PAGE_SIZE without defining it */ >> +#ifndef PAGE_SIZE >> +#define PAGE_SIZE XC_PAGE_SIZE >> +#endif >> #include <xen/io/usbif.h> > > > Can we just change the macro to > > #define USB_CONN_RING_SIZE(pg_size) __CONST_RING_SIZE(usbif_conn, (pg_size) > > ? Not easily. On x86 qemu builds just fine and modifying the macro to take a parameter would break the build. We could do something like: #define USB_CONN_RING_SZ(pgsz) __CONST_RING_SIZE(usbif_conn, (pgsz) #ifdef PAGE_SIZE #define USB_CONN_RING_SIZE USB_CONN_RING_SZ(PAGE_SIZE) #endif And then modify qemu to use USB_CONN_RING_SZ() Juergen ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] xen-usb.c/usbif.h: fix ARM64 build issue 2016-10-19 9:09 ` Juergen Gross @ 2016-10-19 9:22 ` Wei Liu 2016-10-19 9:40 ` Juergen Gross 0 siblings, 1 reply; 7+ messages in thread From: Wei Liu @ 2016-10-19 9:22 UTC (permalink / raw) To: Juergen Gross Cc: Wei Liu, Stefano Stabellini, xen-devel, qemu-devel, anthony.perard, julien.grall On Wed, Oct 19, 2016 at 11:09:21AM +0200, Juergen Gross wrote: > On 19/10/16 10:50, Wei Liu wrote: > > On Tue, Oct 18, 2016 at 12:09:55PM -0700, Stefano Stabellini wrote: > >> Hi all, > >> > >> While I was investigating the recent libxl ARM64 build issue, I found > >> another ARM64 build problem. This one is in QEMU: > >> > >> > >> CC hw/usb/xen-usb.o > >> hw/usb/xen-usb.c: In function ‘usbback_gnttab_map’: > >> hw/usb/xen-usb.c:163:56: error: ‘PAGE_SIZE’ undeclared (first use in this function) > >> (unsigned)usbback_req->req.seg[i].length > PAGE_SIZE) { > >> ^ > >> hw/usb/xen-usb.c:163:56: note: each undeclared identifier is reported only once for each function it appears in > >> In file included from hw/usb/xen-usb.c:36:0: > >> hw/usb/xen-usb.c: In function ‘usbback_alloc’: > >> /users/stefanos/xen/tools/../tools/include/xen/io/usbif.h:229:56: error: ‘PAGE_SIZE’ undeclared (first use in this function) > >> #define USB_URB_RING_SIZE __CONST_RING_SIZE(usbif_urb, PAGE_SIZE) > >> ^ > >> /users/stefanos/xen/tools/../tools/include/xen/io/ring.h:45:23: note: in definition of macro ‘__RD32’ > >> #define __RD32(_x) (((_x) & 0xffff0000) ? __RD16((_x)>>16)<<16 : __RD16(_x)) > >> ^ > >> /users/stefanos/xen/tools/../tools/include/xen/io/usbif.h:229:27: note: in expansion of macro ‘__CONST_RING_SIZE’ > >> #define USB_URB_RING_SIZE __CONST_RING_SIZE(usbif_urb, PAGE_SIZE) > >> ^ > >> hw/usb/xen-usb.c:1029:51: note: in expansion of macro ‘USB_URB_RING_SIZE’ > >> max_grants = USBIF_MAX_SEGMENTS_PER_REQUEST * USB_URB_RING_SIZE + 2; > >> ^ > >> make: *** [hw/usb/xen-usb.o] Error 1 > >> > >> > >> It is caused by PAGE_SIZE being utilized in > >> xen/include/public/io/usbif.h, but undefined on ARM64 (in userspace). > >> QEMU includes xen/include/public/io/usbif.h directly, therefore > >> hw/usb/xen-usb.c doesn't build. > >> > >> I don't think we should be using "PAGE_SIZE" in public/io/usbif.h, but > >> that file is present in too many Xen releases to ignore now. So I am > >> going to work around the issue in QEMU. > >> > > > > Actually it was introduced in 4.7. > > > >> --- > >> > >> xen-usb.c does not build on ARM64 because it includes xen/io/usbif.h > >> which uses PAGE_SIZE without defining it. The header file has been > >> shipped with too many Xen releases to be able to solve the problem at > >> the source. > >> > >> This patch define PAGE_SIZE as XC_PAGE_SIZE when undefined. > >> > >> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org> > >> > >> diff --git a/hw/usb/xen-usb.c b/hw/usb/xen-usb.c > >> index 174d715..ca9df87 100644 > >> --- a/hw/usb/xen-usb.c > >> +++ b/hw/usb/xen-usb.c > >> @@ -34,6 +34,11 @@ > >> #include "qapi/qmp/qstring.h" > >> > >> #include <xen/io/ring.h> > >> + > >> +/* xen/io/usbif.h references PAGE_SIZE without defining it */ > >> +#ifndef PAGE_SIZE > >> +#define PAGE_SIZE XC_PAGE_SIZE > >> +#endif > >> #include <xen/io/usbif.h> > > > > > > Can we just change the macro to > > > > #define USB_CONN_RING_SIZE(pg_size) __CONST_RING_SIZE(usbif_conn, (pg_size) > > > > ? > > Not easily. On x86 qemu builds just fine and modifying the macro to take > a parameter would break the build. > > We could do something like: > > #define USB_CONN_RING_SZ(pgsz) __CONST_RING_SIZE(usbif_conn, (pgsz) > #ifdef PAGE_SIZE > #define USB_CONN_RING_SIZE USB_CONN_RING_SZ(PAGE_SIZE) > #endif > > And then modify qemu to use USB_CONN_RING_SZ() > This would also be problematic I'm afraid. One's build could break randomly depending on the order of header files inclusion. I think it would even be harder to debug such bug than the existing one. Inspired by Andrew on IRC. I think we can do: /* Backward-compatible macro, use _SIZE2 variant in new code instead */ #define USB_CONN_RING_SIZE __CONST_RING_SIZE(usbif_conn, 4096) #define USB_CONN_RING_SIZE2(pgsize) __CONST_RING_SIZE(usbif_conn, (pgsize)) The assumption is that all existing implementations of the said protocol use 4k page size. Juergen, do you think this would work? Any thought? Wei. > > Juergen ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] xen-usb.c/usbif.h: fix ARM64 build issue 2016-10-19 9:22 ` Wei Liu @ 2016-10-19 9:40 ` Juergen Gross 2016-10-19 9:55 ` Wei Liu 0 siblings, 1 reply; 7+ messages in thread From: Juergen Gross @ 2016-10-19 9:40 UTC (permalink / raw) To: Wei Liu Cc: Stefano Stabellini, xen-devel, qemu-devel, anthony.perard, julien.grall On 19/10/16 11:22, Wei Liu wrote: > On Wed, Oct 19, 2016 at 11:09:21AM +0200, Juergen Gross wrote: >> On 19/10/16 10:50, Wei Liu wrote: >>> On Tue, Oct 18, 2016 at 12:09:55PM -0700, Stefano Stabellini wrote: >>>> Hi all, >>>> >>>> While I was investigating the recent libxl ARM64 build issue, I found >>>> another ARM64 build problem. This one is in QEMU: >>>> >>>> >>>> CC hw/usb/xen-usb.o >>>> hw/usb/xen-usb.c: In function ‘usbback_gnttab_map’: >>>> hw/usb/xen-usb.c:163:56: error: ‘PAGE_SIZE’ undeclared (first use in this function) >>>> (unsigned)usbback_req->req.seg[i].length > PAGE_SIZE) { >>>> ^ >>>> hw/usb/xen-usb.c:163:56: note: each undeclared identifier is reported only once for each function it appears in >>>> In file included from hw/usb/xen-usb.c:36:0: >>>> hw/usb/xen-usb.c: In function ‘usbback_alloc’: >>>> /users/stefanos/xen/tools/../tools/include/xen/io/usbif.h:229:56: error: ‘PAGE_SIZE’ undeclared (first use in this function) >>>> #define USB_URB_RING_SIZE __CONST_RING_SIZE(usbif_urb, PAGE_SIZE) >>>> ^ >>>> /users/stefanos/xen/tools/../tools/include/xen/io/ring.h:45:23: note: in definition of macro ‘__RD32’ >>>> #define __RD32(_x) (((_x) & 0xffff0000) ? __RD16((_x)>>16)<<16 : __RD16(_x)) >>>> ^ >>>> /users/stefanos/xen/tools/../tools/include/xen/io/usbif.h:229:27: note: in expansion of macro ‘__CONST_RING_SIZE’ >>>> #define USB_URB_RING_SIZE __CONST_RING_SIZE(usbif_urb, PAGE_SIZE) >>>> ^ >>>> hw/usb/xen-usb.c:1029:51: note: in expansion of macro ‘USB_URB_RING_SIZE’ >>>> max_grants = USBIF_MAX_SEGMENTS_PER_REQUEST * USB_URB_RING_SIZE + 2; >>>> ^ >>>> make: *** [hw/usb/xen-usb.o] Error 1 >>>> >>>> >>>> It is caused by PAGE_SIZE being utilized in >>>> xen/include/public/io/usbif.h, but undefined on ARM64 (in userspace). >>>> QEMU includes xen/include/public/io/usbif.h directly, therefore >>>> hw/usb/xen-usb.c doesn't build. >>>> >>>> I don't think we should be using "PAGE_SIZE" in public/io/usbif.h, but >>>> that file is present in too many Xen releases to ignore now. So I am >>>> going to work around the issue in QEMU. >>>> >>> >>> Actually it was introduced in 4.7. >>> >>>> --- >>>> >>>> xen-usb.c does not build on ARM64 because it includes xen/io/usbif.h >>>> which uses PAGE_SIZE without defining it. The header file has been >>>> shipped with too many Xen releases to be able to solve the problem at >>>> the source. >>>> >>>> This patch define PAGE_SIZE as XC_PAGE_SIZE when undefined. >>>> >>>> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org> >>>> >>>> diff --git a/hw/usb/xen-usb.c b/hw/usb/xen-usb.c >>>> index 174d715..ca9df87 100644 >>>> --- a/hw/usb/xen-usb.c >>>> +++ b/hw/usb/xen-usb.c >>>> @@ -34,6 +34,11 @@ >>>> #include "qapi/qmp/qstring.h" >>>> >>>> #include <xen/io/ring.h> >>>> + >>>> +/* xen/io/usbif.h references PAGE_SIZE without defining it */ >>>> +#ifndef PAGE_SIZE >>>> +#define PAGE_SIZE XC_PAGE_SIZE >>>> +#endif >>>> #include <xen/io/usbif.h> >>> >>> >>> Can we just change the macro to >>> >>> #define USB_CONN_RING_SIZE(pg_size) __CONST_RING_SIZE(usbif_conn, (pg_size) >>> >>> ? >> >> Not easily. On x86 qemu builds just fine and modifying the macro to take >> a parameter would break the build. >> >> We could do something like: >> >> #define USB_CONN_RING_SZ(pgsz) __CONST_RING_SIZE(usbif_conn, (pgsz) >> #ifdef PAGE_SIZE >> #define USB_CONN_RING_SIZE USB_CONN_RING_SZ(PAGE_SIZE) >> #endif >> >> And then modify qemu to use USB_CONN_RING_SZ() >> > > This would also be problematic I'm afraid. One's build could break > randomly depending on the order of header files inclusion. I think it > would even be harder to debug such bug than the existing one. > > Inspired by Andrew on IRC. I think we can do: > > /* Backward-compatible macro, use _SIZE2 variant in new code instead */ > #define USB_CONN_RING_SIZE __CONST_RING_SIZE(usbif_conn, 4096) > > #define USB_CONN_RING_SIZE2(pgsize) __CONST_RING_SIZE(usbif_conn, (pgsize)) > > The assumption is that all existing implementations of the said protocol > use 4k page size. Juergen, do you think this would work? In theory, yes. I'd still prefer using PAGE_SIZE if it is defined. What about: #ifdef PAGE_SIZE #define USB_RING_PAGE_SIZE PAGE_SIZE #else #define USB_RING_PAGE_SIZE 4096 #endif #define USB_CONN_RING_SIZE __CONST_RING_SIZE(usbif_conn, USB_RING_PAGE_SIZE) #define USB_CONN_RING_SIZE2(pgsize) __CONST_RING_SIZE(usbif_conn, (pgsize)) Juergen ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] xen-usb.c/usbif.h: fix ARM64 build issue 2016-10-19 9:40 ` Juergen Gross @ 2016-10-19 9:55 ` Wei Liu 2016-10-19 18:07 ` Stefano Stabellini 0 siblings, 1 reply; 7+ messages in thread From: Wei Liu @ 2016-10-19 9:55 UTC (permalink / raw) To: Juergen Gross Cc: Wei Liu, Stefano Stabellini, xen-devel, qemu-devel, anthony.perard, julien.grall On Wed, Oct 19, 2016 at 11:40:40AM +0200, Juergen Gross wrote: > On 19/10/16 11:22, Wei Liu wrote: > > On Wed, Oct 19, 2016 at 11:09:21AM +0200, Juergen Gross wrote: > >> On 19/10/16 10:50, Wei Liu wrote: > >>> On Tue, Oct 18, 2016 at 12:09:55PM -0700, Stefano Stabellini wrote: > >>>> Hi all, > >>>> > >>>> While I was investigating the recent libxl ARM64 build issue, I found > >>>> another ARM64 build problem. This one is in QEMU: > >>>> > >>>> > >>>> CC hw/usb/xen-usb.o > >>>> hw/usb/xen-usb.c: In function ‘usbback_gnttab_map’: > >>>> hw/usb/xen-usb.c:163:56: error: ‘PAGE_SIZE’ undeclared (first use in this function) > >>>> (unsigned)usbback_req->req.seg[i].length > PAGE_SIZE) { > >>>> ^ > >>>> hw/usb/xen-usb.c:163:56: note: each undeclared identifier is reported only once for each function it appears in > >>>> In file included from hw/usb/xen-usb.c:36:0: > >>>> hw/usb/xen-usb.c: In function ‘usbback_alloc’: > >>>> /users/stefanos/xen/tools/../tools/include/xen/io/usbif.h:229:56: error: ‘PAGE_SIZE’ undeclared (first use in this function) > >>>> #define USB_URB_RING_SIZE __CONST_RING_SIZE(usbif_urb, PAGE_SIZE) > >>>> ^ > >>>> /users/stefanos/xen/tools/../tools/include/xen/io/ring.h:45:23: note: in definition of macro ‘__RD32’ > >>>> #define __RD32(_x) (((_x) & 0xffff0000) ? __RD16((_x)>>16)<<16 : __RD16(_x)) > >>>> ^ > >>>> /users/stefanos/xen/tools/../tools/include/xen/io/usbif.h:229:27: note: in expansion of macro ‘__CONST_RING_SIZE’ > >>>> #define USB_URB_RING_SIZE __CONST_RING_SIZE(usbif_urb, PAGE_SIZE) > >>>> ^ > >>>> hw/usb/xen-usb.c:1029:51: note: in expansion of macro ‘USB_URB_RING_SIZE’ > >>>> max_grants = USBIF_MAX_SEGMENTS_PER_REQUEST * USB_URB_RING_SIZE + 2; > >>>> ^ > >>>> make: *** [hw/usb/xen-usb.o] Error 1 > >>>> > >>>> > >>>> It is caused by PAGE_SIZE being utilized in > >>>> xen/include/public/io/usbif.h, but undefined on ARM64 (in userspace). > >>>> QEMU includes xen/include/public/io/usbif.h directly, therefore > >>>> hw/usb/xen-usb.c doesn't build. > >>>> > >>>> I don't think we should be using "PAGE_SIZE" in public/io/usbif.h, but > >>>> that file is present in too many Xen releases to ignore now. So I am > >>>> going to work around the issue in QEMU. > >>>> > >>> > >>> Actually it was introduced in 4.7. > >>> > >>>> --- > >>>> > >>>> xen-usb.c does not build on ARM64 because it includes xen/io/usbif.h > >>>> which uses PAGE_SIZE without defining it. The header file has been > >>>> shipped with too many Xen releases to be able to solve the problem at > >>>> the source. > >>>> > >>>> This patch define PAGE_SIZE as XC_PAGE_SIZE when undefined. > >>>> > >>>> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org> > >>>> > >>>> diff --git a/hw/usb/xen-usb.c b/hw/usb/xen-usb.c > >>>> index 174d715..ca9df87 100644 > >>>> --- a/hw/usb/xen-usb.c > >>>> +++ b/hw/usb/xen-usb.c > >>>> @@ -34,6 +34,11 @@ > >>>> #include "qapi/qmp/qstring.h" > >>>> > >>>> #include <xen/io/ring.h> > >>>> + > >>>> +/* xen/io/usbif.h references PAGE_SIZE without defining it */ > >>>> +#ifndef PAGE_SIZE > >>>> +#define PAGE_SIZE XC_PAGE_SIZE > >>>> +#endif > >>>> #include <xen/io/usbif.h> > >>> > >>> > >>> Can we just change the macro to > >>> > >>> #define USB_CONN_RING_SIZE(pg_size) __CONST_RING_SIZE(usbif_conn, (pg_size) > >>> > >>> ? > >> > >> Not easily. On x86 qemu builds just fine and modifying the macro to take > >> a parameter would break the build. > >> > >> We could do something like: > >> > >> #define USB_CONN_RING_SZ(pgsz) __CONST_RING_SIZE(usbif_conn, (pgsz) > >> #ifdef PAGE_SIZE > >> #define USB_CONN_RING_SIZE USB_CONN_RING_SZ(PAGE_SIZE) > >> #endif > >> > >> And then modify qemu to use USB_CONN_RING_SZ() > >> > > > > This would also be problematic I'm afraid. One's build could break > > randomly depending on the order of header files inclusion. I think it > > would even be harder to debug such bug than the existing one. > > > > Inspired by Andrew on IRC. I think we can do: > > > > /* Backward-compatible macro, use _SIZE2 variant in new code instead */ > > #define USB_CONN_RING_SIZE __CONST_RING_SIZE(usbif_conn, 4096) > > > > #define USB_CONN_RING_SIZE2(pgsize) __CONST_RING_SIZE(usbif_conn, (pgsize)) > > > > The assumption is that all existing implementations of the said protocol > > use 4k page size. Juergen, do you think this would work? > > In theory, yes. I'd still prefer using PAGE_SIZE if it is defined. > > What about: > > #ifdef PAGE_SIZE > #define USB_RING_PAGE_SIZE PAGE_SIZE > #else > #define USB_RING_PAGE_SIZE 4096 > #endif > #define USB_CONN_RING_SIZE __CONST_RING_SIZE(usbif_conn, USB_RING_PAGE_SIZE) > #define USB_CONN_RING_SIZE2(pgsize) __CONST_RING_SIZE(usbif_conn, (pgsize)) > I wish to eliminate PAGE_SIZE all together in public header files, but I don't feel strongly enough to argue against your proposal. So yes, I'm fine with this. Appropriate comments should be added, though. Wei. > > Juergen ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] xen-usb.c/usbif.h: fix ARM64 build issue 2016-10-19 9:55 ` Wei Liu @ 2016-10-19 18:07 ` Stefano Stabellini 0 siblings, 0 replies; 7+ messages in thread From: Stefano Stabellini @ 2016-10-19 18:07 UTC (permalink / raw) To: Wei Liu Cc: Juergen Gross, Stefano Stabellini, xen-devel, qemu-devel, anthony.perard, julien.grall On Wed, 19 Oct 2016, Wei Liu wrote: > On Wed, Oct 19, 2016 at 11:40:40AM +0200, Juergen Gross wrote: > > On 19/10/16 11:22, Wei Liu wrote: > > > On Wed, Oct 19, 2016 at 11:09:21AM +0200, Juergen Gross wrote: > > >> On 19/10/16 10:50, Wei Liu wrote: > > >>> On Tue, Oct 18, 2016 at 12:09:55PM -0700, Stefano Stabellini wrote: > > >>>> Hi all, > > >>>> > > >>>> While I was investigating the recent libxl ARM64 build issue, I found > > >>>> another ARM64 build problem. This one is in QEMU: > > >>>> > > >>>> > > >>>> CC hw/usb/xen-usb.o > > >>>> hw/usb/xen-usb.c: In function ‘usbback_gnttab_map’: > > >>>> hw/usb/xen-usb.c:163:56: error: ‘PAGE_SIZE’ undeclared (first use in this function) > > >>>> (unsigned)usbback_req->req.seg[i].length > PAGE_SIZE) { > > >>>> ^ > > >>>> hw/usb/xen-usb.c:163:56: note: each undeclared identifier is reported only once for each function it appears in > > >>>> In file included from hw/usb/xen-usb.c:36:0: > > >>>> hw/usb/xen-usb.c: In function ‘usbback_alloc’: > > >>>> /users/stefanos/xen/tools/../tools/include/xen/io/usbif.h:229:56: error: ‘PAGE_SIZE’ undeclared (first use in this function) > > >>>> #define USB_URB_RING_SIZE __CONST_RING_SIZE(usbif_urb, PAGE_SIZE) > > >>>> ^ > > >>>> /users/stefanos/xen/tools/../tools/include/xen/io/ring.h:45:23: note: in definition of macro ‘__RD32’ > > >>>> #define __RD32(_x) (((_x) & 0xffff0000) ? __RD16((_x)>>16)<<16 : __RD16(_x)) > > >>>> ^ > > >>>> /users/stefanos/xen/tools/../tools/include/xen/io/usbif.h:229:27: note: in expansion of macro ‘__CONST_RING_SIZE’ > > >>>> #define USB_URB_RING_SIZE __CONST_RING_SIZE(usbif_urb, PAGE_SIZE) > > >>>> ^ > > >>>> hw/usb/xen-usb.c:1029:51: note: in expansion of macro ‘USB_URB_RING_SIZE’ > > >>>> max_grants = USBIF_MAX_SEGMENTS_PER_REQUEST * USB_URB_RING_SIZE + 2; > > >>>> ^ > > >>>> make: *** [hw/usb/xen-usb.o] Error 1 > > >>>> > > >>>> > > >>>> It is caused by PAGE_SIZE being utilized in > > >>>> xen/include/public/io/usbif.h, but undefined on ARM64 (in userspace). > > >>>> QEMU includes xen/include/public/io/usbif.h directly, therefore > > >>>> hw/usb/xen-usb.c doesn't build. > > >>>> > > >>>> I don't think we should be using "PAGE_SIZE" in public/io/usbif.h, but > > >>>> that file is present in too many Xen releases to ignore now. So I am > > >>>> going to work around the issue in QEMU. > > >>>> > > >>> > > >>> Actually it was introduced in 4.7. > > >>> > > >>>> --- > > >>>> > > >>>> xen-usb.c does not build on ARM64 because it includes xen/io/usbif.h > > >>>> which uses PAGE_SIZE without defining it. The header file has been > > >>>> shipped with too many Xen releases to be able to solve the problem at > > >>>> the source. > > >>>> > > >>>> This patch define PAGE_SIZE as XC_PAGE_SIZE when undefined. > > >>>> > > >>>> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org> > > >>>> > > >>>> diff --git a/hw/usb/xen-usb.c b/hw/usb/xen-usb.c > > >>>> index 174d715..ca9df87 100644 > > >>>> --- a/hw/usb/xen-usb.c > > >>>> +++ b/hw/usb/xen-usb.c > > >>>> @@ -34,6 +34,11 @@ > > >>>> #include "qapi/qmp/qstring.h" > > >>>> > > >>>> #include <xen/io/ring.h> > > >>>> + > > >>>> +/* xen/io/usbif.h references PAGE_SIZE without defining it */ > > >>>> +#ifndef PAGE_SIZE > > >>>> +#define PAGE_SIZE XC_PAGE_SIZE > > >>>> +#endif > > >>>> #include <xen/io/usbif.h> > > >>> > > >>> > > >>> Can we just change the macro to > > >>> > > >>> #define USB_CONN_RING_SIZE(pg_size) __CONST_RING_SIZE(usbif_conn, (pg_size) > > >>> > > >>> ? > > >> > > >> Not easily. On x86 qemu builds just fine and modifying the macro to take > > >> a parameter would break the build. > > >> > > >> We could do something like: > > >> > > >> #define USB_CONN_RING_SZ(pgsz) __CONST_RING_SIZE(usbif_conn, (pgsz) > > >> #ifdef PAGE_SIZE > > >> #define USB_CONN_RING_SIZE USB_CONN_RING_SZ(PAGE_SIZE) > > >> #endif > > >> > > >> And then modify qemu to use USB_CONN_RING_SZ() > > >> > > > > > > This would also be problematic I'm afraid. One's build could break > > > randomly depending on the order of header files inclusion. I think it > > > would even be harder to debug such bug than the existing one. > > > > > > Inspired by Andrew on IRC. I think we can do: > > > > > > /* Backward-compatible macro, use _SIZE2 variant in new code instead */ > > > #define USB_CONN_RING_SIZE __CONST_RING_SIZE(usbif_conn, 4096) > > > > > > #define USB_CONN_RING_SIZE2(pgsize) __CONST_RING_SIZE(usbif_conn, (pgsize)) > > > > > > The assumption is that all existing implementations of the said protocol > > > use 4k page size. Juergen, do you think this would work? > > > > In theory, yes. I'd still prefer using PAGE_SIZE if it is defined. > > > > What about: > > > > #ifdef PAGE_SIZE > > #define USB_RING_PAGE_SIZE PAGE_SIZE > > #else > > #define USB_RING_PAGE_SIZE 4096 > > #endif > > #define USB_CONN_RING_SIZE __CONST_RING_SIZE(usbif_conn, USB_RING_PAGE_SIZE) > > #define USB_CONN_RING_SIZE2(pgsize) __CONST_RING_SIZE(usbif_conn, (pgsize)) > > > > I wish to eliminate PAGE_SIZE all together in public header files, but I > don't feel strongly enough to argue against your proposal. So yes, I'm > fine with this. Appropriate comments should be added, though. PAGE_SIZE needs to be eliminated because on some architectures (like ARM) there can be multiple page granularities. So unfortunately PAGE_SIZE doesn't always mean what we think it means. This is why we use XEN_PAGE_SIZE in Linux. We have two options: either we replace PAGE_SIZE with 4096, or we introduce XEN_PAGE_SIZE in Xen public headers. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-10-19 18:07 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-10-18 19:09 [Qemu-devel] [PATCH] xen-usb.c/usbif.h: fix ARM64 build issue Stefano Stabellini 2016-10-19 8:50 ` Wei Liu 2016-10-19 9:09 ` Juergen Gross 2016-10-19 9:22 ` Wei Liu 2016-10-19 9:40 ` Juergen Gross 2016-10-19 9:55 ` Wei Liu 2016-10-19 18:07 ` Stefano Stabellini
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).