From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57663) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bwndP-00075k-F2 for qemu-devel@nongnu.org; Wed, 19 Oct 2016 05:58:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bwndM-0008T2-E9 for qemu-devel@nongnu.org; Wed, 19 Oct 2016 05:58:07 -0400 Received: from smtp02.citrix.com ([66.165.176.63]:60492) by eggs.gnu.org with esmtps (TLS1.0:RSA_ARCFOUR_SHA1:16) (Exim 4.71) (envelope-from ) id 1bwndM-0008SN-6N for qemu-devel@nongnu.org; Wed, 19 Oct 2016 05:58:04 -0400 Date: Wed, 19 Oct 2016 10:55:48 +0100 From: Wei Liu Message-ID: <20161019095548.GH2639@citrix.com> References: <20161019085059.GC2639@citrix.com> <89df0df2-7e2d-fe4c-7810-83bd8f6514ac@suse.com> <20161019092251.GD2639@citrix.com> <0ddc3f9a-06ab-1c2b-7b2b-7bca4d0c272b@suse.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <0ddc3f9a-06ab-1c2b-7b2b-7bca4d0c272b@suse.com> Subject: Re: [Qemu-devel] [PATCH] xen-usb.c/usbif.h: fix ARM64 build issue List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Juergen Gross Cc: Wei Liu , Stefano Stabellini , xen-devel@lists.xenproject.org, qemu-devel@nongnu.org, anthony.perard@citrix.com, julien.grall@arm.com 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 > >>>> > >>>> 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/usbif.h references PAGE_SIZE without defining it */ > >>>> +#ifndef PAGE_SIZE > >>>> +#define PAGE_SIZE XC_PAGE_SIZE > >>>> +#endif > >>>> #include > >>> > >>> > >>> 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