| Message ID | 20210130152210.17503-1-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [for-4.15] xen/mm: Fix build when CONFIG_HVM=n and CONFIG_COVERAGE=y | expand |
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. >
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.
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,
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);