Re: [Xen-devel] [PATCH v1] Fix p2m_set_suppress_ve

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.04.19 at 16:29, <aisaila@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -3011,8 +3011,16 @@ int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, 
> bool suppress_ve,
>      mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL, NULL);
>      if ( !mfn_valid(mfn) )
>      {
> -        rc = -ESRCH;
> -        goto out;
> +        unsigned int page_order;
> +
> +        mfn = __get_gfn_type_access(host_p2m, gfn_x(gfn), &t, &a,
> +                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 0);

I'm not entirely certain about P2M_ALLOC, but I'm pretty sure that
at least P2M_UNSHARE is too heavy: Why would you want to force
un-sharing of a page when all you want to alter is #VE behavior?

A recent patch[1] of mine actually tries to move the other direction.

Additionally, when you add such a lookup as error handling attempt,
I think it is important to leave a code comment. But I wonder
whether this shouldn't be done before the call to ->get_entry(), or
whether in fact there's a bug here in how get_entry() behaves in
this case.

The description also doesn't clarify at all why you (need to?) use
host_p2m here, when get_entry() and set_entry() both use p2m.
I suppose that's because there's some sync-ing happening
between the p2m-s, but at least to me this isn't an obviously
necessary (side) effect of that call.

Also note that if you already need to call this lowest level function
directly here, the last argument should be "false".

Jan

[1] https://lists.xenproject.org/archives/html/xen-devel/2019-03/msg00847.html



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel