From: George Dunlap <george.dunlap@citrix.com>
To: Paul Durrant <Paul.Durrant@citrix.com>,
Yu Zhang <yu.c.zhang@linux.intel.com>,
"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>
Cc: Kevin Tian <kevin.tian@intel.com>,
"Keir (Xen.org)" <keir@xen.org>,
"jbeulich@suse.com" <jbeulich@suse.com>,
Andrew Cooper <Andrew.Cooper3@citrix.com>,
"Tim (Xen.org)" <tim@xen.org>,
"zhiyuan.lv@intel.com" <zhiyuan.lv@intel.com>,
"jun.nakajima@intel.com" <jun.nakajima@intel.com>
Subject: Re: [PATCH 3/3] Add HVMOP to map guest ram with p2m_ioreq_server to an ioreq server
Date: Wed, 23 Mar 2016 11:43:47 +0000 [thread overview]
Message-ID: <56F28173.1080708@citrix.com> (raw)
In-Reply-To: <a52cfdf42b1945868ae157844abaf060@AMSPEX02CL03.citrite.net>
On 22/03/16 17:51, Paul Durrant wrote:
>> -----Original Message-----
>> From: George Dunlap [mailto:george.dunlap@citrix.com]
>> Sent: 22 March 2016 17:27
>> To: Yu Zhang; xen-devel@lists.xen.org
>> Cc: Keir (Xen.org); jbeulich@suse.com; Andrew Cooper; George Dunlap;
>> jun.nakajima@intel.com; Kevin Tian; Tim (Xen.org); Paul Durrant;
>> zhiyuan.lv@intel.com
>> Subject: Re: [PATCH 3/3] Add HVMOP to map guest ram with
>> p2m_ioreq_server to an ioreq server
>>
>> On 16/03/16 12:22, Yu Zhang wrote:
>>> A new HVMOP - HVMOP_map_mem_type_to_ioreq_server, is added to
>>> let one ioreq server claim 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 in a flag. By now, we only
>>> support one ioreq server for this p2m type, so once an ioreq
>>> server has claimed its ownership, following calls of the
>>> HVMOP_map_mem_type_to_ioreq_server will fail.
>>>
>>> Note: this HVMOP does not change the p2m type of any guest ram
>>> page, until the HVMOP_set_mem_type is triggered. So normally
>>> the steps would be the backend driver first claims its ownership
>>> of guest ram pages with p2m_ioreq_server type. At then sets the
>>> memory type to p2m_ioreq_server for specified guest ram pages.
>>
>> Yu, thanks for this work. I think it's heading in the right direction.
>>
>> A couple of comments:
>>
>> There's not much documentation in the code about how this is expected to
>> be used.
>>
>> For instance, having separate flags seems to imply that you can
>> independently select either read intercept, write intercept, or both;
>> but [ept_]p2m_type_to_flags() seems to assume that READ_ACCESS implies
>> WRITE_ACCESS. Do you plan to implement them separately in the future?
>> If not, would it be better to make the interface an enum instead?
>>
>> At very least it should be documented that READ_ACCESS implies
>> WRITE_ACCESS.
>
> That's not true. If WRITE_ACCESS has not been requested then writes are handled directly in Xen rather than being forwarded to the ioreq server. If h/w were to allow pages to be marked write-only then we wouldn't need to do that.
Right -- well this at least needs to be in the commit message, and it
would be good if it were in a comment somewhere as well.
It's probably also useful to note in the interface that if you're only
intercepting writes, reads will happen at full speed; but that if you're
only intercepting reads, writes must be emulated (and so there will have
a significant performance impact).
>>> @@ -168,13 +226,65 @@ static int hvmemul_do_io(
>>> break;
>>> case X86EMUL_UNHANDLEABLE:
>>> {
>>> - struct hvm_ioreq_server *s =
>>> - hvm_select_ioreq_server(curr->domain, &p);
>>> + struct hvm_ioreq_server *s;
>>> + p2m_type_t p2mt;
>>> +
>>> + if ( is_mmio )
>>> + {
>>> + unsigned long gmfn = paddr_to_pfn(addr);
>>> +
>>> + (void) get_gfn_query_unlocked(currd, gmfn, &p2mt);
>>> +
>>> + switch ( p2mt )
>>> + {
>>> + case p2m_ioreq_server:
>>> + {
>>> + unsigned long flags;
>>> +
>>> + p2m_get_ioreq_server(currd, p2mt, &flags, &s);
>>> +
>>> + if ( !s )
>>> + break;
>>> +
>>> + if ( (dir == IOREQ_READ &&
>>> + !(flags & P2M_IOREQ_HANDLE_READ_ACCESS)) ||
>>> + (dir == IOREQ_WRITE &&
>>> + !(flags & P2M_IOREQ_HANDLE_WRITE_ACCESS)) )
>>> + s = NULL;
>>> +
>>> + break;
>>> + }
>>> + case p2m_ram_rw:
>>> + s = NULL;
>>> + break;
>>> +
>>> + default:
>>> + s = hvm_select_ioreq_server(currd, &p);
>>> + break;
>>> + }
>>> + }
>>> + else
>>> + {
>>> + p2mt = p2m_invalid;
>>> +
>>> + s = hvm_select_ioreq_server(currd, &p);
>>> + }
>>>
>>> /* If there is no suitable backing DM, just ignore accesses */
>>> if ( !s )
>>> {
>>> - rc = hvm_process_io_intercept(&null_handler, &p);
>>> + switch ( p2mt )
>>> + {
>>> + case p2m_ioreq_server:
>>> + case p2m_ram_rw:
>>> + rc = hvm_process_io_intercept(&mem_handler, &p);
>>> + break;
>>> +
>>> + default:
>>> + rc = hvm_process_io_intercept(&null_handler, &p);
>>> + break;
>>> + }
>>
>> Is it actually possible to get here with "is_mmio" true and p2mt ==
>> p2m_ram_rw?
>
> I think that's possible if the type change races with an access.
OK -- a brief comment about that would be helpful.
Thanks,
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
prev parent reply other threads:[~2016-03-23 11:43 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-16 12:22 [PATCH 3/3] Add HVMOP to map guest ram with p2m_ioreq_server to an ioreq server Yu Zhang
2016-03-22 17:26 ` George Dunlap
2016-03-22 17:51 ` Paul Durrant
2016-03-23 9:22 ` Jan Beulich
2016-03-23 9:44 ` Yu, Zhang
2016-03-23 10:10 ` Jan Beulich
2016-03-23 10:37 ` Andrew Cooper
2016-03-23 11:43 ` George Dunlap [this message]
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=56F28173.1080708@citrix.com \
--to=george.dunlap@citrix.com \
--cc=Andrew.Cooper3@citrix.com \
--cc=Paul.Durrant@citrix.com \
--cc=jbeulich@suse.com \
--cc=jun.nakajima@intel.com \
--cc=keir@xen.org \
--cc=kevin.tian@intel.com \
--cc=tim@xen.org \
--cc=xen-devel@lists.xen.org \
--cc=yu.c.zhang@linux.intel.com \
--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).