On 13.07.21 15:46, Costin Lupu wrote: > Hi guys, > > On 7/13/21 4:00 PM, Julien Grall wrote: >> >> >> On 13/07/2021 13:39, Andrew Cooper wrote: >>> On 13/07/2021 12:53, Julien Grall wrote: >>>> Hi Andrew, >>>> >>>> On 13/07/2021 12:23, Andrew Cooper wrote: >>>>> On 13/07/2021 12:21, Julien Grall wrote: >>>>>> Hi Andrew, >>>>>> >>>>>> On 13/07/2021 10:35, Andrew Cooper wrote: >>>>>>> On 13/07/2021 10:27, Juergen Gross wrote: >>>>>>>> On 13.07.21 11:20, Julien Grall wrote: >>>>>>>>> From: Julien Grall >>>>>>>>> >>>>>>>>> Commit 0dbb4be739c5 add the inclusion of xenctrl.h from private.h >>>>>>>>> and >>>>>>>>> wreck the build in an interesting way: >>>>>>>>> >>>>>>>>> In file included from xen/stubdom/include/xen/domctl.h:39:0, >>>>>>>>>                      from xen/tools/include/xenctrl.h:36, >>>>>>>>>                      from private.h:4, >>>>>>>>>                      from minios.c:29: >>>>>>>>> xen/include/public/memory.h:407:5: error: expected >>>>>>>>> specifier-qualifier-list before ‘XEN_GUEST_HANDLE_64’ >>>>>>>>>          XEN_GUEST_HANDLE_64(const_uint8) buffer; >>>>>>>>>          ^~~~~~~~~~~~~~~~~~~ >>>>>>>>> >>>>>>>>> This is happening because xenctrl.h defines __XEN_TOOLS__ and >>>>>>>>> therefore >>>>>>>>> the public headers will start to expose the non-stable ABI. >>>>>>>>> However, >>>>>>>>> xen.h has already been included by a mini-OS header before hand. So >>>>>>>>> there is a mismatch in the way the headers are included. >>>>>>>>> >>>>>>>>> For now solve it in a very simple (and gross) way by including >>>>>>>>> xenctrl.h before the mini-os headers. >>>>>>>>> >>>>>>>>> Fixes: 0dbb4be739c5 ("tools/libs/foreignmemory: Fix PAGE_SIZE >>>>>>>>> redefinition error") >>>>>>>>> Signed-off-by: Julien Grall >>>>>>>>> >>>>>>>>> --- >>>>>>>>> >>>>>>>>> Cc: Andrew Cooper >>>>>>>>> >>>>>>>>> I couldn't find a better way with would not result to revert the >>>>>>>>> patch >>>>>>>>> (and break build on some system) or involve a longer rework of the >>>>>>>>> headers. >>>>>>>> >>>>>>>> Just adding a "#define __XEN_TOOLS__" before the #include statements >>>>>>>> doesn't work? >>>>>>> >>>>>>> Not really, no. >>>>>>> >>>>>>> libxenforeignmem has nothing at all to do with any Xen unstable >>>>>>> interfaces.  Including xenctrl.h in the first place was wrong, >>>>>>> because >>>>>>> it is an unstable library.  By extension, the use of XC_PAGE_SIZE is >>>>>>> also wrong. >>>>>> >>>>>> Well... Previously we were using PAGE_SIZE which is just plain wrong >>>>>> on Arm. >>>>>> >>>>>> At the moment, we don't have a way to query the page granularity of >>>>>> the hypervisor. But we know it can't change because of the way the >>>>>> current ABI was designed. Hence why using XC_PAGE_SIZE is the best of >>>>>> option we had until we go to ABIv2. >>>>> >>>>> Still doesn't mean that XC_PAGE_SIZE was ok to use. >>>> >>>> Note that I wrote "best of the option". The series has been sitting >>>> for ages with no-one answering... You could have provided your option >>>> back then if you thought it wasn't a good use... >>> >>> On a series I wasn't even CC'd on? >> >> You had the link on IRC because we discussed it. >> >>> >>> And noone had even bothered to compile test? >> >> Well, that was a mistake. At the same time, if it compiled the "issue" >> you describe would have gone unnoticed. ;) >> >>>> >>>>> >>>>> Sounds like the constant needs moving into the Xen public headers, and >>>>> the inclusions of xenctrl.h into stable libraries needs reverting. >>>> >>>> This could work. Are you planning to work on it? >>> >>> No.  I don't have enough time to do my own work thanks to all the CI >>> breakage and regressions being committed. >>> This needs fixing, or the original series reverting for 4.16 because the >>> current form (with or without this emergency build fix) isn't acceptable >>> to release with. >> I disagree with this caracterization. Yes, this is including a >> non-stable header but it doesn't link with non-stable library. >> >> In fact, reverting the series will bring back two issues: >>   1) Xen tools will not build on all the distros >>   2) Using PAGE_{SIZE, SHIFT} break arm tools because the userspace is >> not meant to rely on a given kernel page granularity. >> >> So this doesn't look like a priority for 4.16. Although, it would be a >> nice clean-up to have so the libraries are more compliant. > > First of all, sorry for breaking the build. > > As Jan already suggested on a different thread, we can fix this by > isolating the XC_PAGE_* definitions of the toolstack in a header of > their own. I'm open to suggestions regarding the name of the header (my > suggestion would be xenctrl_page.h) and path (I guess it should be in > tools/include, right?). Also, should we change the names of the macros > from XC_PAGE_* to something else in order to reflect that they are > toolstack related instead of xenctrl specific? I would rather have that definition in xen/include/public/arch-*.h as this is a hypervisor attribute. And I don't think it should be named XC_PAGE_*, but rather XEN_PAGE_*. Juergen