Re: [Xen-devel] [PATCH v2 3/3] x86/ioreq server: Add HVMOP to map guest ram with p2m_ioreq_server to an ioreq server

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Thu, Mar 31, 2016 at 11:53 AM, Yu Zhang <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
> A new HVMOP - HVMOP_map_mem_type_to_ioreq_server, is added to
> let one ioreq server claim/disclaim its responsibility for the
> handling of guest pages with p2m type p2m_ioreq_server. Users
> of this HVMOP can specify whether the p2m_ioreq_server is supposed
> to handle write accesses or read ones or both in a parameter named
> flags. For now, we only support one ioreq server for this p2m type,
> so once an ioreq server has claimed its ownership, subsequent calls
> of the HVMOP_map_mem_type_to_ioreq_server will fail. Users can also
> disclaim the ownership of guest ram pages with this p2m type, by
> triggering this new HVMOP, with ioreq server id set to the current
> owner's and flags parameter set to 0.
>
> For now, both HVMOP_map_mem_type_to_ioreq_server and p2m_ioreq_server
> are only supported for HVMs with HAP enabled.
>
> Note that flags parameter(if not 0) of this HVMOP only indicates
> which kind of memory accesses are to be forwarded to an ioreq server,
> it has impact on the access rights of guest ram pages, but are not
> the same. Due to hardware limitations, if only write operations are
> to be forwarded, read ones will be performed at full speed, with
> no hypervisor intervention. But if read ones are to be forwarded to
> an ioreq server, writes will inevitably be trapped into hypervisor,
> which means significant performance impact.
>
> Also note that this HVMOP_map_mem_type_to_ioreq_server will not
> change the p2m type of any guest ram page, until HVMOP_set_mem_type
> is triggered. So normally the steps should be the backend driver
> first claims its ownership of guest ram pages with p2m_ioreq_server
> type, and then sets the memory type to p2m_ioreq_server for specified
> guest ram pages.
>
> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> Signed-off-by: Yu Zhang <yu.c.zhang@xxxxxxxxxxxxxxx>

And again, review of this patch was significantly delayed because you
didn't provide any description of the changes you made between v1 and
v2 or why.

Overall looks good.  Just a few questions...

> +static int hvmop_map_mem_type_to_ioreq_server(
> +    XEN_GUEST_HANDLE_PARAM(xen_hvm_map_mem_type_to_ioreq_server_t) uop)
> +{
> +    xen_hvm_map_mem_type_to_ioreq_server_t op;
> +    struct domain *d;
> +    int rc;
> +
> +    if ( copy_from_guest(&op, uop, 1) )
> +        return -EFAULT;
> +
> +    rc = rcu_lock_remote_domain_by_id(op.domid, &d);
> +    if ( rc != 0 )
> +        return rc;
> +
> +    rc = -EINVAL;
> +    if ( !is_hvm_domain(d) )
> +        goto out;
> +
> +    /* For now, only support for HAP enabled hvm */
> +    if ( !hap_enabled(d) )
> +        goto out;

So before I suggested that this be restricted to HAP because you were
using p2m_memory_type_changed(), which was only implemented on EPT.
But since then you've switched that code to use
p2m_change_entry_type_global() instead, which is implemented by both;
and you implement the type in p2m_type_to_flags().  Is there any
reason to keep this restriction?

> +    /*
> +     * Each time we map/unmap an ioreq server to/from p2m_ioreq_server,
> +     * we mark the p2m table to be recalculated, so that gfns which were
> +     * previously marked with p2m_ioreq_server can be resynced.
> +     */
> +    p2m_change_entry_type_global(d, p2m_ioreq_server, p2m_ram_rw);

This comment doesn't seem to be accurate (or if it is it's a bit
confusing).  Would it be more accurate to say something like the
following:

"Each time we map / unmap in ioreq server to/from p2m_ioreq_server, we
reset all memory currently marked p2m_ioreq_server to p2m_ram_rw."

But of course that raises another question: is there ever any risk
that an ioreq server will change some other ram type (say, p2m_ram_ro)
to p2m_ioreq_server, and then have it changed back to p2m_ram_rw when
it detaches?

>  /* Types that can be subject to bulk transitions. */
>  #define P2M_CHANGEABLE_TYPES (p2m_to_mask(p2m_ram_rw) \
> -                              | p2m_to_mask(p2m_ram_logdirty) )
> +                              | p2m_to_mask(p2m_ram_logdirty) \
> +                              | p2m_to_mask(p2m_ioreq_server) )

It's probably worth a comment here noting that you can do a mass
change *from* p2m_ioreq_server, but you can't do a mass change *to*
p2m_ioreq_server.  (And doing so would need to change a number of
places in the code where it's assumed that the result of such a
recalculation is either p2m_logdirty or p2m_ram_rw -- e.g.,
p2m-ept.c:553, p2m-pt.c:452, &c.

Really getting down to the fine details here -- thanks for all your
work on this.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel