Re: [Qemu-devel] [PATCH] xen-usb.c/usbif.h: fix ARM64 build issue

[Juergen Gross]

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 <address@hidden>
>>>>
>>>> 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