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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > >>>>
> > >>>> 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.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel