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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

Sounds like the constant needs moving into the Xen public headers, and
the inclusions of xenctrl.h into stable libraries needs reverting.

~Andrew