[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] stubdom: foreignmemory: Fix build after 0dbb4be739c5

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 <jgrall@xxxxxxxxxx>
>>>>>>>> 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 <jgrall@xxxxxxxxxx>
>>>>>>>> ---
>>>>>>>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>>>>>> 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?

@Andrew: Can you please tell me why XC_PAGE_SIZE wasn't ok to use? I'm
asking this in order to fully understand the issue.




Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.