[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v9 2/5] x86/ioreq server: Add DMOP to map guest ram with p2m_ioreq_server to an ioreq server.
On 3/23/2017 4:57 PM, Jan Beulich wrote:
On 23.03.17 at 04:23, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
On 3/22/2017 10:21 PM, Jan Beulich wrote:
On 21.03.17 at 03:52, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
---
xen/arch/x86/hvm/dm.c | 37 ++++++++++++++++++--
xen/arch/x86/hvm/emulate.c | 65 ++++++++++++++++++++++++++++++++---
xen/arch/x86/hvm/ioreq.c | 38 +++++++++++++++++++++
xen/arch/x86/mm/hap/nested_hap.c | 2 +-
xen/arch/x86/mm/p2m-ept.c | 8 ++++-
xen/arch/x86/mm/p2m-pt.c | 19 +++++++----
xen/arch/x86/mm/p2m.c | 74
++++++++++++++++++++++++++++++++++++++++
xen/arch/x86/mm/shadow/multi.c | 3 +-
xen/include/asm-x86/hvm/ioreq.h | 2 ++
xen/include/asm-x86/p2m.h | 26 ++++++++++++--
xen/include/public/hvm/dm_op.h | 28 +++++++++++++++
xen/include/public/hvm/hvm_op.h | 8 ++++-
12 files changed, 290 insertions(+), 20 deletions(-)
Btw., isn't there a libdevicemodel wrapper missing here for this new
sub-op?
Yes. I planed to add the wrapper code in another patch after this series
is accepted.
Is this a must in this patchset?
I think so, or else the code you add is effectively dead. We should
avoid encouraging people to bypass libxc.
OK. I'll try to add another patch to do so, along with the existing
ones. Thanks.
@@ -177,8 +178,64 @@ 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 designated
+ * ioreq_server for the domain, but there are some corner
+ * cases:
+ *
+ * - If the domain ioreq_server is NULL, assume there is a
+ * race between the unbinding of ioreq server and guest fault
+ * so re-try the instruction.
And that retry won't come back here because of? (The answer
should not include any behavior added by subsequent patches.)
You got me. :)
In this patch, retry will come back here. It should be after patch 4 or
patch 5 that the retry
will be ignored(p2m type changed back to p2m_ram_rw after the unbinding).
In which case I think we shouldn't insist on you to change things, but
you should spell out very clearly that this patch should not go in
without the others going in at the same time.
So maybe it would be better we leave the retry part to a later patch,
say patch 4/5 or patch 5/5,
and return unhandleable in this patch?
--- a/xen/arch/x86/mm/hap/nested_hap.c
+++ b/xen/arch/x86/mm/hap/nested_hap.c
@@ -172,7 +172,7 @@ nestedhap_walk_L0_p2m(struct p2m_domain *p2m, paddr_t
L1_gpa, paddr_t *L0_gpa,
if ( *p2mt == p2m_mmio_direct )
goto direct_mmio_out;
rc = NESTEDHVM_PAGEFAULT_MMIO;
- if ( *p2mt == p2m_mmio_dm )
+ if ( *p2mt == p2m_mmio_dm || *p2mt == p2m_ioreq_server )
Btw., how does this addition match up with the rc value being
assigned right before the if()?
Well returning a NESTEDHVM_PAGEFAULT_MMIO in such case will trigger
handle_mmio() later in
hvm_hap_nested_page_fault(). Guess that is what we expected.
That's probably what is expected, but it's no MMIO which we're
doing in that case. And note that we've stopped abusing
handle_mmio() for non-MMIO purposes a little while ago (commit
3dd00f7b56 ["x86/HVM: restrict permitted instructions during
special purpose emulation"]).
OK. So what about we just remove this "*p2mt == p2m_ioreq_server"?
--- 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);
Is this effectively open coded p2m_get_ioreq_server() actually
okay? If so, why does the function need to be used elsewhere,
instead of doing direct, lock-free accesses?
Maybe your comments is about whether it is necessary to use the lock in
p2m_get_ioreq_server()?
I still believe so, it does not only protect the value of ioreq server,
but also the flag together with it.
Besides, it is used not only in the emulation process, but also the
hypercall to set the mem type.
So the lock can still provide some kind protection against the
p2m_set_ioreq_server() - even it does
not always do so.
The question, fundamentally, is about consistency: The same
access model should be followed universally, unless there is an
explicit reason for an exception.
Sorry, I do not quite understand. Why the consistency is broken?
I think this lock at least protects the ioreq server and the flag. The
only exception
is the one you mentioned - s could become stale which we agreed to let
the device
model do the check. Without this lock, things would become more complex
- more
race conditions...
Thanks
Yu
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
