* [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).