| Message ID | alpine.DEB.2.10.1610181145470.16441@sstabellini-ThinkPad-X260 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > > /*
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
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
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
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
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.
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> /*
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>