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

[Juergen Gross <jgross@xxxxxxxx>]

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_*.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature