From: Costin Lupu <costin.lupu@cs.pub.ro>
To: Julien Grall <julien@xen.org>,
Andrew Cooper <andrew.cooper3@citrix.com>,
Juergen Gross <jgross@suse.com>,
xen-devel@lists.xenproject.org
Cc: Julien Grall <jgrall@amazon.com>,
Ian Jackson <iwj@xenproject.org>, Wei Liu <wl@xen.org>,
Jan Beulich <jbeulich@suse.com>
Subject: Re: [PATCH] stubdom: foreignmemory: Fix build after 0dbb4be739c5
Date: Tue, 13 Jul 2021 16:46:23 +0300 [thread overview]
Message-ID: <c299355b-c031-3efd-ef97-671bd102809b@cs.pub.ro> (raw)
In-Reply-To: <3c819563-b354-5527-050d-f698324d6021@xen.org>
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@amazon.com>
>>>>>>>>
>>>>>>>> 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@amazon.com>
>>>>>>>>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>>>>
>>>>>>>> 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
next prev parent reply other threads:[~2021-07-13 13:46 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-13 9:20 [PATCH] stubdom: foreignmemory: Fix build after 0dbb4be739c5 Julien Grall
2021-07-13 9:25 ` Andrew Cooper
2021-07-13 9:27 ` Juergen Gross
2021-07-13 9:31 ` Julien Grall
2021-07-13 10:35 ` Juergen Gross
2021-07-13 9:35 ` Andrew Cooper
2021-07-13 11:21 ` Julien Grall
2021-07-13 11:23 ` Andrew Cooper
2021-07-13 11:53 ` Julien Grall
2021-07-13 12:39 ` Andrew Cooper
2021-07-13 13:00 ` Julien Grall
2021-07-13 13:46 ` Costin Lupu [this message]
2021-07-13 14:00 ` Juergen Gross
2021-07-13 14:14 ` Jan Beulich
2021-07-13 14:19 ` Julien Grall
2021-07-13 14:23 ` Jan Beulich
2021-07-13 14:33 ` Julien Grall
2021-07-13 15:52 ` Jan Beulich
2021-07-13 16:15 ` Julien Grall
2021-07-13 16:27 ` Jan Beulich
2021-07-13 16:33 ` Julien Grall
2021-07-14 6:11 ` Jan Beulich
2021-07-14 8:51 ` Julien Grall
2021-07-13 14:23 ` Juergen Gross
2021-07-13 14:28 ` Jan Beulich
2021-07-13 14:33 ` Juergen Gross
2021-07-13 14:38 ` Julien Grall
2021-07-13 15:09 ` Juergen Gross
2021-07-13 15:15 ` Julien Grall
2021-07-13 15:20 ` Juergen Gross
2021-07-16 18:28 ` Costin Lupu
2021-07-27 13:36 ` Andrew Cooper
2021-07-30 9:18 ` Costin Lupu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c299355b-c031-3efd-ef97-671bd102809b@cs.pub.ro \
--to=costin.lupu@cs.pub.ro \
--cc=andrew.cooper3@citrix.com \
--cc=iwj@xenproject.org \
--cc=jbeulich@suse.com \
--cc=jgrall@amazon.com \
--cc=jgross@suse.com \
--cc=julien@xen.org \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).