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

[Costin Lupu <costin.lupu@xxxxxxxxx>]

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.


Cheers,
Costin