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

[Wei Liu <wei.liu2@citrix.com>]

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