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

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


  • To: Juergen Gross <jgross@xxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Tue, 13 Jul 2021 16:14:17 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=fny9C6Ied+8Sr90b5zajdsmH/hQyiGydJvaeJVjhdBk=; b=Gy8058mciUwqSUnx5CQ9IAezJFs4ku1mSDkNmsGQvVKqV15lZl1VyRBKe79TWSPwVZWUoh87f7yQaON6IIyS7kKnmoIVSG8eHMTuUu3aQbRRyuEc3SmOapW61H2wd/qNqv2M+eW3PhYM/BG67TP+OZV7pmiGRx/+byHaF66Z7VeY5gk8q2dURSZUBHCtPQedvFR+0Rmay0N0/dObpskmApoXKSbeduYW3wsnRZD9DcLkeUEcW7R69gS51cwtBqMg5wu9lxlfudoINfxqJd981sGMabPT2+JUJzj96p/BZ6IrPj34R+0iGn1Oy0hmwmA2aq2wvaPRc9gt3/2yKzp0pQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=ZatsdZ1ltjf2JJdc/nZMOr/LLwi1ueW71QJF60tjWks8/1ecnKO698R3FP6O47XRPmhB5Qu133dBuQxdeiLsyLjHb8A0KGeOlRucMxj16Zaeh+DV+vrKrMpZLUUdyjOGSSWpDFQVXXPOT64vZ2UbQldvfh9f4tEdNR0wazO3P3iCkkwNXAOndc5s1WqtylzfXijtRrbwOaYjtEP2/UwhgFac/hp4+0vT+jzcLNzprkAXZ/w47w+6P4yqpZKk9ZIYxL+wtYD6zrTFPxO5OHFEphFWAzIbBLJ9RBIMmkvgjFlvhk4wPNHc9tlUy0De9+EKNvTuH3Lm4c1PS4yYI9bO6A==
  • Authentication-results: xen.org; dkim=none (message not signed) header.d=none;xen.org; dmarc=none action=none header.from=suse.com;
  • Cc: Julien Grall <jgrall@xxxxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Costin Lupu <costin.lupu@xxxxxxxxx>, Julien Grall <julien@xxxxxxx>
  • Delivery-date: Tue, 13 Jul 2021 14:14:33 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 13.07.2021 16:00, Juergen Gross wrote:
> 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 <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?
> 
> 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_*.

Even that doesn't seem right to me, at least in principle. There shouldn't
be a build time setting when it may vary at runtime. IOW on Arm I think a
runtime query to the hypervisor would be needed instead. And thinking
even more generally, perhaps there could also be mixed (base) page sizes
in use at run time, so it may need to be a bit mask which gets returned.

Jan




 


Rackspace

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