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

[Wei Liu]

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 <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))
> 

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