* [PATCH for-4.15] xen/mm: Fix build when CONFIG_HVM=n and CONFIG_COVERAGE=y
@ 2021-01-30 15:22 Julien Grall
2021-02-01 9:46 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Julien Grall @ 2021-01-30 15:22 UTC (permalink / raw)
To: xen-devel
Cc: julien, Julien Grall, Andrew Cooper, George Dunlap, Ian Jackson,
Jan Beulich, Stefano Stabellini, Wei Liu, Oleksandr Tyshchenko
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH for-4.15] xen/mm: Fix build when CONFIG_HVM=n and CONFIG_COVERAGE=y
2021-01-30 15:22 [PATCH for-4.15] xen/mm: Fix build when CONFIG_HVM=n and CONFIG_COVERAGE=y Julien Grall
@ 2021-02-01 9:46 ` Jan Beulich
2021-02-01 14:54 ` [PATCH for-4.15] xen/mm: Fix build when CONFIG_HVM=n and CONFIG_COVERAGE=y [and 1 more messages] Ian Jackson
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2021-02-01 9:46 UTC (permalink / raw)
To: Julien Grall
Cc: Julien Grall, Andrew Cooper, George Dunlap, Ian Jackson,
Stefano Stabellini, Wei Liu, Oleksandr Tyshchenko, xen-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH for-4.15] xen/mm: Fix build when CONFIG_HVM=n and CONFIG_COVERAGE=y [and 1 more messages]
2021-02-01 9:46 ` Jan Beulich
@ 2021-02-01 14:54 ` Ian Jackson
2021-02-01 15:02 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Ian Jackson @ 2021-02-01 14:54 UTC (permalink / raw)
To: Jan Beulich, Julien Grall
Cc: xen-devel, Julien Grall, Andrew Cooper, George Dunlap,
Stefano Stabellini, Wei Liu, Oleksandr Tyshchenko
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH for-4.15] xen/mm: Fix build when CONFIG_HVM=n and CONFIG_COVERAGE=y [and 1 more messages]
2021-02-01 14:54 ` [PATCH for-4.15] xen/mm: Fix build when CONFIG_HVM=n and CONFIG_COVERAGE=y [and 1 more messages] Ian Jackson
@ 2021-02-01 15:02 ` Jan Beulich
2021-02-01 15:15 ` Ian Jackson
2021-02-01 15:39 ` Julien Grall
0 siblings, 2 replies; 6+ messages in thread
From: Jan Beulich @ 2021-02-01 15:02 UTC (permalink / raw)
To: Ian Jackson, Julien Grall
Cc: xen-devel, Julien Grall, Andrew Cooper, George Dunlap,
Stefano Stabellini, Wei Liu, Oleksandr Tyshchenko
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.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH for-4.15] xen/mm: Fix build when CONFIG_HVM=n and CONFIG_COVERAGE=y [and 1 more messages]
2021-02-01 15:02 ` Jan Beulich
@ 2021-02-01 15:15 ` Ian Jackson
2021-02-01 15:39 ` Julien Grall
1 sibling, 0 replies; 6+ messages in thread
From: Ian Jackson @ 2021-02-01 15:15 UTC (permalink / raw)
To: Jan Beulich
Cc: Julien Grall, xen-devel, Julien Grall, Andrew Cooper,
George Dunlap, Stefano Stabellini, Wei Liu, Oleksandr Tyshchenko
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH for-4.15] xen/mm: Fix build when CONFIG_HVM=n and CONFIG_COVERAGE=y [and 1 more messages]
2021-02-01 15:02 ` Jan Beulich
2021-02-01 15:15 ` Ian Jackson
@ 2021-02-01 15:39 ` Julien Grall
1 sibling, 0 replies; 6+ messages in thread
From: Julien Grall @ 2021-02-01 15:39 UTC (permalink / raw)
To: Jan Beulich, Ian Jackson
Cc: xen-devel, Julien Grall, Andrew Cooper, George Dunlap,
Stefano Stabellini, Wei Liu, Oleksandr Tyshchenko
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-02-01 15:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-30 15:22 [PATCH for-4.15] xen/mm: Fix build when CONFIG_HVM=n and CONFIG_COVERAGE=y Julien Grall
2021-02-01 9:46 ` Jan Beulich
2021-02-01 14:54 ` [PATCH for-4.15] xen/mm: Fix build when CONFIG_HVM=n and CONFIG_COVERAGE=y [and 1 more messages] Ian Jackson
2021-02-01 15:02 ` Jan Beulich
2021-02-01 15:15 ` Ian Jackson
2021-02-01 15:39 ` Julien Grall
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.