From: "Yu, Zhang" <yu.c.zhang@linux.intel.com>
To: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Kevin Tian <kevin.tian@intel.com>, Keir Fraser <keir@xen.org>,
Jan Beulich <jbeulich@suse.com>,
Andrew Cooper <andrew.cooper3@citrix.com>,
Tim Deegan <tim@xen.org>,
"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>,
Paul Durrant <paul.durrant@citrix.com>,
"Lv, Zhiyuan" <zhiyuan.lv@intel.com>,
Jun Nakajima <jun.nakajima@intel.com>
Subject: Re: [PATCH v2 3/3] x86/ioreq server: Add HVMOP to map guest ram with p2m_ioreq_server to an ioreq server
Date: Thu, 7 Apr 2016 15:01:13 +0800 [thread overview]
Message-ID: <570605B9.10702@linux.intel.com> (raw)
In-Reply-To: <CAFLBxZZriQ5Xfy1wfaS6SG+xKLy6UhcGQ62yXnhkWWdWk9Oy1Q@mail.gmail.com>
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@linux.intel.com> 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@citrix.com>
>> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
>
> 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
> 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."
>
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.
>
> -George
>
B.R.
Yu
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
next prev parent reply other threads:[~2016-04-07 7:01 UTC|newest]
Thread overview: 82+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-31 10:53 [PATCH v2 0/3] x86/ioreq server: introduce HVMMEM_ioreq_server mem type Yu Zhang
2016-03-31 10:53 ` [PATCH v2 1/3] x86/ioreq server: Add new functions to get/set memory types Yu Zhang
2016-04-05 13:57 ` George Dunlap
2016-04-05 14:08 ` George Dunlap
2016-04-08 13:25 ` Andrew Cooper
2016-03-31 10:53 ` [PATCH v2 2/3] x86/ioreq server: Rename p2m_mmio_write_dm to p2m_ioreq_server Yu Zhang
2016-04-05 14:38 ` George Dunlap
2016-04-08 13:26 ` Andrew Cooper
2016-04-08 21:48 ` Jan Beulich
2016-04-18 8:41 ` Paul Durrant
2016-04-18 9:10 ` George Dunlap
2016-04-18 9:14 ` Wei Liu
2016-04-18 9:45 ` Paul Durrant
2016-04-18 16:40 ` Jan Beulich
2016-04-18 16:45 ` Paul Durrant
2016-04-18 16:47 ` Jan Beulich
2016-04-18 16:58 ` Paul Durrant
2016-04-19 11:02 ` Yu, Zhang
2016-04-19 11:15 ` Paul Durrant
2016-04-19 11:38 ` Yu, Zhang
2016-04-19 11:50 ` Paul Durrant
2016-04-19 16:51 ` Jan Beulich
2016-04-20 14:59 ` Wei Liu
2016-04-20 15:02 ` George Dunlap
2016-04-20 16:30 ` George Dunlap
2016-04-20 16:52 ` Jan Beulich
2016-04-20 16:58 ` Paul Durrant
2016-04-20 17:06 ` George Dunlap
2016-04-20 17:09 ` Paul Durrant
2016-04-21 12:24 ` Yu, Zhang
2016-04-21 13:31 ` Paul Durrant
2016-04-21 13:48 ` Yu, Zhang
2016-04-21 13:56 ` Paul Durrant
2016-04-21 14:09 ` George Dunlap
2016-04-20 17:08 ` George Dunlap
2016-04-21 12:04 ` Yu, Zhang
2016-03-31 10:53 ` [PATCH v2 3/3] x86/ioreq server: Add HVMOP to map guest ram with p2m_ioreq_server to an ioreq server Yu Zhang
[not found] ` <20160404082556.GC28633@deinos.phlegethon.org>
2016-04-05 6:01 ` Yu, Zhang
2016-04-06 17:13 ` George Dunlap
2016-04-07 7:01 ` Yu, Zhang [this message]
[not found] ` <CAFLBxZbLp2zWzCzQTaJNWbanQSmTJ57ZyTh0qaD-+YUn8o8pyQ@mail.gmail.com>
2016-04-08 10:39 ` George Dunlap
[not found] ` <5707839F.9060803@linux.intel.com>
2016-04-08 11:01 ` George Dunlap
2016-04-11 11:15 ` Yu, Zhang
2016-04-14 10:45 ` Yu, Zhang
2016-04-18 15:57 ` Paul Durrant
2016-04-19 9:11 ` Yu, Zhang
2016-04-19 9:21 ` Paul Durrant
2016-04-19 9:44 ` Yu, Zhang
2016-04-19 10:05 ` Paul Durrant
2016-04-19 11:17 ` Yu, Zhang
2016-04-19 11:47 ` Paul Durrant
2016-04-19 11:59 ` Yu, Zhang
2016-04-20 14:50 ` George Dunlap
2016-04-20 14:57 ` Paul Durrant
2016-04-20 15:37 ` George Dunlap
2016-04-20 16:30 ` Paul Durrant
2016-04-20 16:58 ` George Dunlap
2016-04-21 13:28 ` Yu, Zhang
2016-04-21 13:21 ` Yu, Zhang
2016-04-22 11:27 ` Wei Liu
2016-04-22 11:30 ` George Dunlap
2016-04-19 4:37 ` Tian, Kevin
2016-04-19 9:21 ` Yu, Zhang
2016-04-08 13:33 ` Andrew Cooper
2016-04-11 11:14 ` Yu, Zhang
2016-04-11 12:20 ` Andrew Cooper
2016-04-11 16:25 ` Jan Beulich
2016-04-08 22:28 ` Jan Beulich
2016-04-11 11:14 ` Yu, Zhang
2016-04-11 16:31 ` Jan Beulich
2016-04-12 9:37 ` Yu, Zhang
2016-04-12 15:08 ` Jan Beulich
2016-04-14 9:56 ` Yu, Zhang
2016-04-19 4:50 ` Tian, Kevin
2016-04-19 8:46 ` Paul Durrant
2016-04-19 9:27 ` Yu, Zhang
2016-04-19 9:40 ` Paul Durrant
2016-04-19 9:49 ` Yu, Zhang
2016-04-19 10:01 ` Paul Durrant
2016-04-19 9:54 ` Yu, Zhang
2016-04-19 9:15 ` Yu, Zhang
2016-04-19 9:23 ` Paul Durrant
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=570605B9.10702@linux.intel.com \
--to=yu.c.zhang@linux.intel.com \
--cc=George.Dunlap@eu.citrix.com \
--cc=andrew.cooper3@citrix.com \
--cc=jbeulich@suse.com \
--cc=jun.nakajima@intel.com \
--cc=keir@xen.org \
--cc=kevin.tian@intel.com \
--cc=paul.durrant@citrix.com \
--cc=tim@xen.org \
--cc=xen-devel@lists.xen.org \
--cc=zhiyuan.lv@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).