[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v7 2/5] x86/ioreq server: Add DMOP to map guest ram with p2m_ioreq_server to an ioreq server.
On 3/13/2017 7:20 PM, Jan Beulich wrote:
On 11.03.17 at 09:42, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
On 3/10/2017 11:29 PM, Jan Beulich wrote:
On 08.03.17 at 16:33, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
changes in v7:
- Use new ioreq server interface - XEN_DMOP_map_mem_type_to_ioreq_server.
- According to comments from George: removed domain_pause/unpause() in
hvm_map_mem_type_to_ioreq_server(), because it's too expensive,
and we can avoid the:
a> deadlock between p2m lock and ioreq server lock by using these locks
in the same order - solved in patch 4;
That is, until patch 4 there is deadlock potential? I think you want to
re-order the patches if so. Or was it that the type can't really be used
until the last patch of the series? (I'm sorry, it's been quite a while
since the previous version.)
Oh. There's no deadlock potential in this version patch set. But in v6, there
was, and I used
domain_pause/unpause() to avoid this. Later on, I realized that if I use
different locks in the
same order, the deadlock potential can be avoid and we do not need
domain_pause/unpause
in this version.
Well, okay, but in the future please don't add misleading change
info then.
Got it. Thanks. :-)
@@ -365,6 +383,24 @@ static int dm_op(domid_t domid,
break;
}
+ case XEN_DMOP_map_mem_type_to_ioreq_server:
+ {
+ const struct xen_dm_op_map_mem_type_to_ioreq_server *data =
+ &op.u.map_mem_type_to_ioreq_server;
+
+ rc = -EINVAL;
+ if ( data->pad )
+ break;
+
+ /* Only support for HAP enabled hvm. */
+ if ( !hap_enabled(d) )
+ break;
Perhaps better to give an error other than -EINVAL in this case?
If so, then the same error should likely also be used in your
set_mem_type() addition.
How about -ENOTSUP?
If you mean -EOPNOTSUPP, then yes.
Yes, I mean -EOPNOTSUPP
@@ -177,8 +178,65 @@ static int hvmemul_do_io(
break;
case X86EMUL_UNHANDLEABLE:
{
- struct hvm_ioreq_server *s =
- hvm_select_ioreq_server(curr->domain, &p);
+ /*
+ * Xen isn't emulating the instruction internally, so see if
+ * there's an ioreq server that can handle it. Rules:
+ *
+ * - PIO and "normal" mmio run through hvm_select_ioreq_server()
+ * to choose the ioreq server by range. If no server is found,
+ * the access is ignored.
+ *
+ * - p2m_ioreq_server accesses are handled by the current
+ * ioreq_server for the domain, but there are some corner
+ * cases:
Who or what is "the current ioreq_server for the domain"?
It means "the current ioreq_server which maps the p2m_ioreq_server type
for this domain"...
I'd like to use a succinct phrase, but now seems not accurate enough.
Any preference?
Just add "designated" after "current", or replacing "current"?
Replacing "current" with "designated" sounds good to me. :-)
+ if ( p2mt == p2m_ioreq_server )
+ {
+ unsigned int flags;
+
+ s = p2m_get_ioreq_server(currd, &flags);
+
+ /*
+ * If p2mt is ioreq_server but ioreq_server is NULL,
+ * we probably lost a race with unbinding of ioreq
+ * server, just retry the access.
+ */
+ if ( s == NULL )
+ {
+ rc = X86EMUL_RETRY;
+ vio->io_req.state = STATE_IOREQ_NONE;
+ break;
+ }
+
+ /*
+ * If the IOREQ_MEM_ACCESS_WRITE flag is not set,
+ * we should set s to NULL, and just ignore such
+ * access.
+ */
+ if ( !(flags & XEN_DMOP_IOREQ_MEM_ACCESS_WRITE) )
+ s = NULL;
What is this about? You only allow WRITE registrations, so this looks
to be dead code. Yet if it is meant to guard against future enabling
of READ, then this clearly should not be done for reads.
It's to guard against future emulation of READ. We can remove it for now.
I guess first of all you need to settle on what you want the code to
look like generally wrt reads: Do you want it to support the option
as much as possible, reducing code changes to a minimum when
someone wants to actually add support, or do you want to reject
such attempts? Whichever variant you choose, you should carry it
out consistently rather than mixing both.
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -131,6 +131,13 @@ static void ept_p2m_type_to_flags(struct p2m_domain *p2m,
ept_entry_t *entry,
entry->r = entry->w = entry->x = 1;
entry->a = entry->d = !!cpu_has_vmx_ept_ad;
break;
+ case p2m_ioreq_server:
+ entry->r = 1;
+ entry->w = !(p2m->ioreq.flags & XEN_DMOP_IOREQ_MEM_ACCESS_WRITE);
Along the lines of the previous comment - if you mean to have the
code cope with READ, please also set ->r accordingly, or add a
comment why this won't have the intended effect (yielding a not
present EPTE).
How about we keep this code and do not support READ? I'll remove above
dead code in hvmemul_do_io().
Sure, as said above: All I'd like to push for is that the result is
consistent across the code base.
Thank you, Jan. I should have keep a consistent principle in this code.
My preference is to remove the possible read emulation logic. But could
we still keep the
definition of XEN_DMOP_IOREQ_MEM_ACCESS_READ, so that people can still
know this
interface can be extended in the future?
+struct hvm_ioreq_server *p2m_get_ioreq_server(struct domain *d,
+ unsigned int *flags)
+{
+ struct p2m_domain *p2m = p2m_get_hostp2m(d);
+ struct hvm_ioreq_server *s;
+
+ spin_lock(&p2m->ioreq.lock);
+
+ s = p2m->ioreq.server;
+ *flags = p2m->ioreq.flags;
+
+ spin_unlock(&p2m->ioreq.lock);
+ return s;
+}
I'm afraid this question was asked before, but since there's no
comment here or anywhere, I can't recall if there was a reason why
s potentially being stale by the time the caller looks at it is not a
problem.
Well, it is possibe that s is stale. I did not take it as a problem
because the device model
will later discard such io request. And I believe current
hvm_select_ioreq_server() also
has the same issue - the returned s should be considered to be stale, if
the MMIO/PIO
address is removed from the ioreq server's rangeset.
Another thought is, if you think it is inappropriate for device model to
do the check,
we can use spin_lock_recursive on ioreq_server.lock to protect all the
ioreq server select
and release the lock after the ioreq server is sent out.
Well, let's first ask Paul as to what his perspective here is, both
specifically for this change and more generally regarding what
you say above.
Paul, any suggestions on this and the above one? :)
Thanks
Yu
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
