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

[Jan Beulich <jbeulich@xxxxxxxx>]

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>
>>>>> wrote:
>>>>>
>>>>>> 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.
>>>>>>>>
>>>>>>>>>    Move
>>>>>>>>> the latter next to p2m_add_foreign(), allowing this one to become
>>>>>> static
>>>>>>>>> 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) )
>>>>>>> +    {
>>>>>>> +        ASSERT_UNREACHABLE();
>>>>>>> +        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.
>>>>>>
>>>>>> ~Andrew
>>>>>>
>>>>> 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:
>>>>>
>>>>> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/common/memory.c:941:
>>>>> undefined reference to `xenmem_add_to_physmap_one'
>>>>> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/common/memory.c:941:(.text+0x1e391):
>>>>> relocation truncated to fit: R_X86_64_PC32 against undefined symbol
>>>>> `xenmem_add_to_physmap_one'
>>>>> ld:
>>>>> /media/b/build/build/tmp/work/x86_64-xt-linux/domd-image-weston/1.0-r0/repo/build/tmp/work/aarch64-poky-linux/xen/4.14.0+gitAUTOINC+2c6e5a8ceb-r0/git/xen/.xen-syms.0:
>>>>> 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.

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.

Jan