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

[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.
>>
>>>  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