[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

Thanks for your advices and good questions. :)

On 4/7/2016 1:13 AM, George Dunlap wrote:
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.

Sorry about the inconvenience, will change in next version.

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?

Yes. And this is a change which was not explained clearly. Sorry.

Reason I've chosen p2m_change_entry_type_global() instead:
p2m_memory_type_changed() will only trigger the resynchronization for
the ept memory types in resolve_misconfig(). Yet it is the p2m type we
wanna to be recalculated, so here comes p2m_change_entry_type_global().

Reasons I restricting the code in HAP mode:
Well, I guess p2m_change_entry_type_global() was only called by HAP code
like hap_[en|dis]able_log_dirty() etc, which were registered during
hap_domain_init(). As to shadow mode, it is sh_[en|dis]able_log_dirty(),
which do not use p2m_change_entry_type_global().

Since my intention is to resync the outdated p2m_ioreq_server pages
back to p2m_ram_rw, calling p2m_change_entry_global() directly should
be much more convenient(and correct) for me than inventing another
wrapper to cover both the HAP and shadow mode(which xengt does not use
by now).

+    /*
+     * 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

"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."

Well, I agree this comment is not quite accurate. Like you said in your
comment, the purpose here, calling p2m_change_entry_type_global() is to
"reset all memory currently marked p2m_ioreq_server to p2m_ram_rw". But
the recalculation is asynchronous. So how about:

"Each time we map/unmap an ioreq server to/from p2m_ioreq_server, we
mark the p2m table to be recalculated, so all memory currently marked
p2m_ioreq_server can be reset to p2m_ram_rw later."?

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?

Good question. And unfortunately, yes there is. :)
Maybe we should insist only p2m_ram_rw pages can be changed to
p2m_ioreq_server, and vice versa.

  /* 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.

I agree with adding a note here.
But adding extra code in resolve_miconfig()/do_recalc()? Is this
necessary? IIUC, current code already guarantees there will be no mass
change *to* the p2m_ioreq_server.

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



Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.