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

Re: [PATCH 2/6] x86/mm: p2m_add_foreign() is HVM-only

On 11.01.21 09:41, Jan Beulich wrote:

Hi Jan

On 08.01.2021 18:37, Oleksandr wrote:
On 08.01.21 19:01, Jan Beulich wrote:

Hi Jan

On 08.01.2021 17:38, Oleksandr wrote:
On 05.01.21 10:48, Jan Beulich wrote:
On 04.01.2021 17:57, Oleksandr Tyshchenko wrote:
Hello all.

[Sorry for the possible format issues]

On Tue, Dec 22, 2020 at 12:41 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

On 21/12/2020 08:10, Jan Beulich wrote:
On 17.12.2020 20:18, Andrew Cooper wrote:
On 15/12/2020 16:26, Jan Beulich wrote:
This is together with its only caller, xenmem_add_to_physmap_one().
I can't parse this sentence.  Perhaps "... as is it's only caller," as a
follow-on from the subject sentence.

the latter next to p2m_add_foreign(), allowing this one to become
at the same time.

Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
So I had to ask Andrew to revert this (I was already at home when
noticing the breakage), as it turned out to break the shim build.
The problem is that xenmem_add_to_physmap() is non-static and
hence can't be eliminated altogether by the compiler when !HVM.
We could make the function conditionally static
"#if !defined(CONFIG_X86) && !defined(CONFIG_HVM)", but this
looks uglier to me than this extra hunk:

--- unstable.orig/xen/common/memory.c
+++ unstable/xen/common/memory.c
@@ -788,7 +788,11 @@ int xenmem_add_to_physmap(struct domain
        union add_to_physmap_extra extra = {};
        struct page_info *pages[16];

-    ASSERT(paging_mode_translate(d));
+    if ( !paging_mode_translate(d) )
+    {
+        return -EACCES;
+    }

        if ( xatp->space == XENMAPSPACE_gmfn_foreign )
            extra.foreign_domid = DOMID_INVALID;

Andrew, please let me know whether your ack stands with this (or
said alternative) added, or whether you'd prefer me to re-post.
Yeah, this is probably neater than the ifdefary.  My ack stands.


I might miss something or did incorrect tests, but ...
... trying to build current staging
(7ba2ab495be54f608cb47440e1497b2795bd301a) for x86 (with # CONFIG_HVM is
not set) I got the following:

undefined reference to `xenmem_add_to_physmap_one'
relocation truncated to fit: R_X86_64_PC32 against undefined symbol
hidden symbol `xenmem_add_to_physmap_one' isn't defined
ld: final link failed: Bad value

It is worth mentioning that I do not use pvshim_defconfig (I disable HVM
support via menuconfig manually before building).
The specific .config may matter. The specific compiler version may
also matter. Things work fine for me, both for the shim config and
a custom !HVM one, with gcc10.
ok, after updating my a little bit ancient compiler to the latest
possible (?) on xenial gcc-9 the build issue had gone away. Sorry for
the noise.
There's no reason to be sorry - we want Xen to build with a wide
range of compiler versions. It's just that if a build issue is
version dependent, it is often up to the person running into it
to determine how to address the issue (and submit a patch).
ok, the issue was observed with gcc (Ubuntu 5.4.0-6ubuntu1~16.04.12)
5.4.0 20160609

I think (but not 100% sure), to address the build issue something like
the stub below could help:
I'm sure this would help, but personally I'd prefer if we could limit
the number of such stubs and rely on the compiler DCE-ing the calls.
Hence it would be at least desirable (imo necessary) to understand
what prevents this to happen here, with this gcc version.

Sounds reasonable

If you could also provide your exact .config, I could see whether I
can repro here with some of the gcc5 versions I have laying around.

Please see attached


Oleksandr Tyshchenko

Attachment: .config
Description: Text document



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