From: Julien Grall <jgrall@amazon.com>

Xen is heavily relying on the DCE stage to remove unused code so the
linker doesn't throw an error because a function is not implemented
yet we defined a prototype for it.

On some GCC version (such as 9.4 provided by Debian sid), the compiler
will DCE stage will not managed to figure that out for
xenmem_add_to_physmap_batch():

ld: ld: prelink.o: in function `xenmem_add_to_physmap_batch':
/xen/xen/common/memory.c:942: undefined reference to `xenmem_add_to_physmap_one'
/xen/xen/common/memory.c:942:(.text+0x22145): relocation truncated
to fit: R_X86_64_PLT32 against undefined symbol `xenmem_add_to_physmap_one'
prelink-efi.o: in function `xenmem_add_to_physmap_batch':
/xen/xen/common/memory.c:942: undefined reference to `xenmem_add_to_physmap_one'
make[2]: *** [Makefile:215: /root/xen/xen/xen.efi] Error 1
make[2]: *** Waiting for unfinished jobs....
ld: /xen/xen/.xen-syms.0: hidden symbol `xenmem_add_to_physmap_one' isn't defined
ld: final link failed: bad value

It is not entirely clear why the compiler DCE is not detecting the
unused code. However, moving the permission check from do_memory_op()
to xenmem_add_to_physmap_batch() does the trick.

Note that this required to move the implementation of
xapt_permision_check() earlier on so it can be called in
xemem_add_to_physmap_batch().

No functional change intended.

Fixes: d4f699a0df6c ("x86/mm: p2m_add_foreign() is HVM-only")
Reported-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Signed-off-by: Julien Grall <jgrall@amazon.com>

---

This also resolves a randconfig issue on the gitlab CI.

The gitlab CI is used to provide basic testing on a per-series basis. So
I would like to request this patch to be merged in Xen 4.15 in order to
reduce the number of failure not related to the series tested.

Note that there are a few more randconfig issues that needs to be
addressed.
---
 xen/common/memory.c | 44 +++++++++++++++++++++-----------------------
 1 file changed, 21 insertions(+), 23 deletions(-)

diff --git a/xen/common/memory.c b/xen/common/memory.c
index 01cab7e4930e..b047a93a703a 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -898,11 +898,32 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp,
     return rc;
 }
 
+static long xatp_permission_check(struct domain *d, unsigned int space)
+{
+    if ( !paging_mode_translate(d) )
+        return -EACCES;
+
+    /*
+     * XENMAPSPACE_dev_mmio mapping is only supported for hardware Domain
+     * to map this kind of space to itself.
+     */
+    if ( (space == XENMAPSPACE_dev_mmio) &&
+         (!is_hardware_domain(d) || (d != current->domain)) )
+        return -EACCES;
+
+    return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
+}
+
 static int xenmem_add_to_physmap_batch(struct domain *d,
                                        struct xen_add_to_physmap_batch *xatpb,
                                        unsigned int extent)
 {
     union add_to_physmap_extra extra = {};
+    int rc;
+
+    rc = xatp_permission_check(d, xatpb->space);
+    if ( rc )
+        return rc;
 
     if ( unlikely(xatpb->size < extent) )
         return -EILSEQ;
@@ -1038,22 +1059,6 @@ static int get_reserved_device_memory(xen_pfn_t start, xen_ulong_t nr,
 }
 #endif
 
-static long xatp_permission_check(struct domain *d, unsigned int space)
-{
-    if ( !paging_mode_translate(d) )
-        return -EACCES;
-
-    /*
-     * XENMAPSPACE_dev_mmio mapping is only supported for hardware Domain
-     * to map this kind of space to itself.
-     */
-    if ( (space == XENMAPSPACE_dev_mmio) &&
-         (!is_hardware_domain(d) || (d != current->domain)) )
-        return -EACCES;
-
-    return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
-}
-
 unsigned int ioreq_server_max_frames(const struct domain *d)
 {
     unsigned int nr = 0;
@@ -1442,13 +1447,6 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         if ( d == NULL )
             return -ESRCH;
 
-        rc = xatp_permission_check(d, xatpb.space);
-        if ( rc )
-        {
-            rcu_unlock_domain(d);
-            return rc;
-        }
-
         rc = xenmem_add_to_physmap_batch(d, &xatpb, start_extent);
 
         rcu_unlock_domain(d);
-- 
2.17.1

On 30.01.2021 16:22, Julien Grall wrote:
> @@ -1442,13 +1447,6 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>          if ( d == NULL )
>              return -ESRCH;
>  
> -        rc = xatp_permission_check(d, xatpb.space);
> -        if ( rc )
> -        {
> -            rcu_unlock_domain(d);
> -            return rc;
> -        }
> -
>          rc = xenmem_add_to_physmap_batch(d, &xatpb, start_extent);
>  
>          rcu_unlock_domain(d);

I'd be okay with the code movement if you did so consistently,
i.e. also for the other invocation. I realize this would have
an effect on the dm-op call of the function, but I wonder
whether this wouldn't even be a good thing. If not, I think
duplicating xenmem_add_to_physmap()'s early ASSERT() into
xenmem_add_to_physmap_batch() would be the better course of
action.

Jan

Julien Grall writes ("[PATCH for-4.15] xen/mm: Fix build when CONFIG_HVM=n and CONFIG_COVERAGE=y"):
> Xen is heavily relying on the DCE stage to remove unused code so the
> linker doesn't throw an error because a function is not implemented
> yet we defined a prototype for it.

Thanks for the clear explanation.

> It is not entirely clear why the compiler DCE is not detecting the
> unused code. However, moving the permission check from do_memory_op()
> to xenmem_add_to_physmap_batch() does the trick.

How unfortunate.

> Fixes: d4f699a0df6c ("x86/mm: p2m_add_foreign() is HVM-only")
> Reported-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Signed-off-by: Julien Grall <jgrall@amazon.com>

I have reviewed the diff, but not the code in context.

> The gitlab CI is used to provide basic testing on a per-series basis. So
> I would like to request this patch to be merged in Xen 4.15 in order to
> reduce the number of failure not related to the series tested.

Quite so.

Jan Beulich writes ("Re: [PATCH for-4.15] xen/mm: Fix build when CONFIG_HVM=n and CONFIG_COVERAGE=y"):
> On 30.01.2021 16:22, Julien Grall wrote:
> > @@ -1442,13 +1447,6 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> >          if ( d == NULL )
> >              return -ESRCH;
> >  
> > -        rc = xatp_permission_check(d, xatpb.space);
> > -        if ( rc )
> > -        {
> > -            rcu_unlock_domain(d);
> > -            return rc;
> > -        }
> > -
> >          rc = xenmem_add_to_physmap_batch(d, &xatpb, start_extent);
> >  
> >          rcu_unlock_domain(d);
> 
> I'd be okay with the code movement if you did so consistently,
> i.e. also for the other invocation. I realize this would have
> an effect on the dm-op call of the function, but I wonder
> whether this wouldn't even be a good thing. If not, I think
> duplicating xenmem_add_to_physmap()'s early ASSERT() into
> xenmem_add_to_physmap_batch() would be the better course of
> action.

Jan, can you confirm whether in your opinion this patch as originally
posted by Julien is *correct* as is ?  In particular, Julien did not
intend a functional change.  Have you satisfied yourself that there is
no functional change here ?

I understand your objectiion above to relate to style or neatness,
rather than function.  Is that correct ?  And that your proposed
additional change would have some impact whilch would have to be
assessed.

In which case I think it would be better to defer the style
improvement until after the release.

IOW, the original patch

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

assuming a favourable functional code review from a relevant
maintainer.

Thanks,
Ian.

On 01.02.2021 15:54, Ian Jackson wrote:
> Julien Grall writes ("[PATCH for-4.15] xen/mm: Fix build when CONFIG_HVM=n and CONFIG_COVERAGE=y"):
>> Xen is heavily relying on the DCE stage to remove unused code so the
>> linker doesn't throw an error because a function is not implemented
>> yet we defined a prototype for it.
> 
> Thanks for the clear explanation.
> 
>> It is not entirely clear why the compiler DCE is not detecting the
>> unused code. However, moving the permission check from do_memory_op()
>> to xenmem_add_to_physmap_batch() does the trick.
> 
> How unfortunate.
> 
>> Fixes: d4f699a0df6c ("x86/mm: p2m_add_foreign() is HVM-only")
>> Reported-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> I have reviewed the diff, but not the code in context.
> 
>> The gitlab CI is used to provide basic testing on a per-series basis. So
>> I would like to request this patch to be merged in Xen 4.15 in order to
>> reduce the number of failure not related to the series tested.
> 
> Quite so.
> 
> Jan Beulich writes ("Re: [PATCH for-4.15] xen/mm: Fix build when CONFIG_HVM=n and CONFIG_COVERAGE=y"):
>> On 30.01.2021 16:22, Julien Grall wrote:
>>> @@ -1442,13 +1447,6 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>>          if ( d == NULL )
>>>              return -ESRCH;
>>>  
>>> -        rc = xatp_permission_check(d, xatpb.space);
>>> -        if ( rc )
>>> -        {
>>> -            rcu_unlock_domain(d);
>>> -            return rc;
>>> -        }
>>> -
>>>          rc = xenmem_add_to_physmap_batch(d, &xatpb, start_extent);
>>>  
>>>          rcu_unlock_domain(d);
>>
>> I'd be okay with the code movement if you did so consistently,
>> i.e. also for the other invocation. I realize this would have
>> an effect on the dm-op call of the function, but I wonder
>> whether this wouldn't even be a good thing. If not, I think
>> duplicating xenmem_add_to_physmap()'s early ASSERT() into
>> xenmem_add_to_physmap_batch() would be the better course of
>> action.
> 
> Jan, can you confirm whether in your opinion this patch as originally
> posted by Julien is *correct* as is ?  In particular, Julien did not
> intend a functional change.  Have you satisfied yourself that there is
> no functional change here ?

Yes and yes.

> I understand your objectiion above to relate to style or neatness,
> rather than function.  Is that correct ?

Yes.

>  And that your proposed
> additional change would have some impact whilch would have to be
> assessed.

The first of the proposed alternatives may need further
investigation, yes. The second of the alternatives would
shrink this patch to a 2-line one, i.e. far less code
churn, and is not in need of any assessment afaia. In
fact I believe this latter alternative was discussed as
the approach to take here, before the patch was submitted.

Jan

> In which case I think it would be better to defer the style
> improvement until after the release.
> 
> IOW, the original patch
> 
> Release-Acked-by: Ian Jackson <iwj@xenproject.org>
> 
> assuming a favourable functional code review from a relevant
> maintainer.
> 
> Thanks,
> Ian.
> 

On 01/02/2021 15:02, Jan Beulich wrote:
> On 01.02.2021 15:54, Ian Jackson wrote:
>> Julien Grall writes ("[PATCH for-4.15] xen/mm: Fix build when CONFIG_HVM=n and CONFIG_COVERAGE=y"):
>>> Xen is heavily relying on the DCE stage to remove unused code so the
>>> linker doesn't throw an error because a function is not implemented
>>> yet we defined a prototype for it.
>>
>> Thanks for the clear explanation.
>>
>>> It is not entirely clear why the compiler DCE is not detecting the
>>> unused code. However, moving the permission check from do_memory_op()
>>> to xenmem_add_to_physmap_batch() does the trick.
>>
>> How unfortunate.
>>
>>> Fixes: d4f699a0df6c ("x86/mm: p2m_add_foreign() is HVM-only")
>>> Reported-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>
>> I have reviewed the diff, but not the code in context.
>>
>>> The gitlab CI is used to provide basic testing on a per-series basis. So
>>> I would like to request this patch to be merged in Xen 4.15 in order to
>>> reduce the number of failure not related to the series tested.
>>
>> Quite so.
>>
>> Jan Beulich writes ("Re: [PATCH for-4.15] xen/mm: Fix build when CONFIG_HVM=n and CONFIG_COVERAGE=y"):
>>> On 30.01.2021 16:22, Julien Grall wrote:
>>>> @@ -1442,13 +1447,6 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>           if ( d == NULL )
>>>>               return -ESRCH;
>>>>   
>>>> -        rc = xatp_permission_check(d, xatpb.space);
>>>> -        if ( rc )
>>>> -        {
>>>> -            rcu_unlock_domain(d);
>>>> -            return rc;
>>>> -        }
>>>> -
>>>>           rc = xenmem_add_to_physmap_batch(d, &xatpb, start_extent);
>>>>   
>>>>           rcu_unlock_domain(d);
>>>
>>> I'd be okay with the code movement if you did so consistently,
>>> i.e. also for the other invocation. I realize this would have
>>> an effect on the dm-op call of the function, but I wonder
>>> whether this wouldn't even be a good thing. If not, I think
>>> duplicating xenmem_add_to_physmap()'s early ASSERT() into
>>> xenmem_add_to_physmap_batch() would be the better course of
>>> action.
>>
>> Jan, can you confirm whether in your opinion this patch as originally
>> posted by Julien is *correct* as is ?  In particular, Julien did not
>> intend a functional change.  Have you satisfied yourself that there is
>> no functional change here ?
> 
> Yes and yes.
> 
>> I understand your objectiion above to relate to style or neatness,
>> rather than function.  Is that correct ?
> 
> Yes.
> 
>>   And that your proposed
>> additional change would have some impact whilch would have to be
>> assessed.
> 
> The first of the proposed alternatives may need further
> investigation, yes. The second of the alternatives would
> shrink this patch to a 2-line one, i.e. far less code
> churn, and is not in need of any assessment afaia. In
> fact I believe this latter alternative was discussed as
> the approach to take here, before the patch was submitted.

Right, I chose this approach over the one discussed previously because 
it doesn't duplicate the check for auto-translated domain and I couldn't 
really find a good way to justify it in the code (you requested one).

Regarding calling the xatp_permission_check() from 
xenmem_add_to_physmap(). It would means that the DM code will have two 
XSM check (one XSM_DM_PRIV, one XSM_TARGET). It is not clear to me 
whether this is going to be introducing more headache for the XSM folks.

Therefore, for Xen 4.15, I would prefer to stick with my patch.

Cheers,

-- 
Julien Grall

Jan Beulich writes ("Re: [PATCH for-4.15] xen/mm: Fix build when CONFIG_HVM=n and CONFIG_COVERAGE=y [and 1 more messages]"):
> On 01.02.2021 15:54, Ian Jackson wrote:
> > Julien Grall writes ("[PATCH for-4.15] xen/mm: Fix build when CONFIG_HVM=n and CONFIG_COVERAGE=y"):
...
> > Jan, can you confirm whether in your opinion this patch as originally
> > posted by Julien is *correct* as is ?  In particular, Julien did not
> > intend a functional change.  Have you satisfied yourself that there is
> > no functional change here ?
> 
> Yes and yes.
> 
> > I understand your objectiion above to relate to style or neatness,
> > rather than function.  Is that correct ?
> 
> Yes.

Right, thanks.

> >  And that your proposed
> > additional change would have some impact whilch would have to be
> > assessed.
> 
> The first of the proposed alternatives may need further
> investigation, yes. The second of the alternatives would
> shrink this patch to a 2-line one, i.e. far less code
> churn, and is not in need of any assessment afaia. In
> fact I believe this latter alternative was discussed as
> the approach to take here, before the patch was submitted.

Sorry I missed that part.  I would be happy with that other approach
too, so for that approach (adding a duplicated ASSERT) is also

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

I'm not a huge fan of code duplication, in general.  I suggest that if
the ASSERT is duplicated it might be worth leaving comment(s) by each
one pointing to the other.

Ian.