Re: [Xen-devel] [PATCH v10] x86/altp2m: support for setting restrictions for an array of pages

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Wed, Dec 13, 2017 at 2:22 PM, Petre Pircalabu
<ppircalabu@xxxxxxxxxxxxxxx> wrote:
> From: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
>
> For the default EPT view we have xc_set_mem_access_multi(), which
> is able to set an array of pages to an array of access rights with
> a single hypercall. However, this functionality was lacking for the
> altp2m subsystem, which could only set page restrictions for one
> page at a time. This patch addresses the gap.
>
> HVMOP_altp2m_set_mem_access_multi has been added as a HVMOP (as opposed to a
> DOMCTL) for consistency with its HVMOP_altp2m_set_mem_access counterpart (and
> hence with the original altp2m design, where domains are allowed - with the
> proper altp2m access rights - to alter these settings), in the absence of an
> official position on the issue from the original altp2m designers.

This mostly looks good to me, with a couple of nitpicks...

> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 666db0b..f171668 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1974,6 +1974,9 @@ int xc_altp2m_set_mem_access(xc_interface *handle, 
> uint32_t domid,
>  int xc_altp2m_change_gfn(xc_interface *handle, uint32_t domid,
>                           uint16_t view_id, xen_pfn_t old_gfn,
>                           xen_pfn_t new_gfn);
> +int xc_altp2m_set_mem_access_multi(xc_interface *handle, uint32_t domid,
> +                                   uint16_t view_id, uint8_t *access,
> +                                   uint64_t *pages, uint32_t nr);

Two minor things:

* It seems like it would make sense to put this directly under the
non-multi version of this call (even though that does put it out of
order with the command number)

* 'Pages' is ambiguous here, as it could be interpreted to mean Linux
virtual pages rather than gfn.  Is there a reason not to call this
argument 'gfns' (as in the other xc call) or 'pfn_list' (as in the
hypercall)?

(And sorry if this has been covered before; I did do a quick look over
the history and didn't notice anything.)

> @@ -4619,6 +4623,37 @@ static int do_altp2m_op(
>                                      a.u.set_mem_access.view);
>          break;
>
> +    case HVMOP_altp2m_set_mem_access_multi:
> +        if ( a.u.set_mem_access_multi.pad ||
> +             a.u.set_mem_access_multi.opaque > a.u.set_mem_access_multi.nr )
> +        {
> +            rc = -EINVAL;
> +            break;
> +        }
> +
> +        /*
> +         * The mask was set (arbitrary) to 0x3F to match the value used for
> +         * MEMOP, despite the fact there are no encoding limitations for the
> +         * start parameter.
> +         */

This comment isn't actually very enlightening if you're not already
intimately familiar with the code; it took me at least 10 minutes of
grepping around to figure out what this was about.

What about this:

"Unlike XENMEM_access_op_set_access_multi, we don't need any bits of
the 'continuation' counter to be zero (to stash a command in).
However, 0x40 is a good 'stride' to make sure
that we make a reasonable amount of forward progress before yielding,
so use a mask of 0x3F here."

Everything else looks good to me.

 -George

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