[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v5 3/4] x86/ioreq server: Add HVMOP to map guest ram with p2m_ioreq_server to an ioreq server.
On 8/10/2016 6:43 PM, Paul Durrant wrote:
-----Original Message-----
From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
Sent: 10 August 2016 11:33
To: Paul Durrant; Yu Zhang
Cc: Andrew Cooper; George Dunlap; Jun Nakajima; Kevin Tian;
zhiyuan.lv@xxxxxxxxx; xen-devel@xxxxxxxxxxxxx; Tim (Xen.org)
Subject: Re: [PATCH v5 3/4] x86/ioreq server: Add HVMOP to map guest ram
with p2m_ioreq_server to an ioreq server.
On 10.08.16 at 10:09, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
On 8/8/2016 11:40 PM, Jan Beulich wrote:
On 12.07.16 at 11:02, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
@@ -178,8 +179,34 @@ 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;
+
+ if ( is_mmio )
+ {
+ unsigned long gmfn = paddr_to_pfn(addr);
+ p2m_type_t p2mt;
+
+ (void) get_gfn_query_unlocked(currd, gmfn, &p2mt);
+
+ if ( p2mt == p2m_ioreq_server )
+ {
+ unsigned int flags;
+
+ if ( dir != IOREQ_WRITE )
+ s = NULL;
+ else
+ {
+ s = p2m_get_ioreq_server(currd, &flags);
+
+ if ( !(flags & P2M_IOREQ_HANDLE_WRITE_ACCESS) )
+ s = NULL;
+ }
+ }
+ else
+ s = hvm_select_ioreq_server(currd, &p);
+ }
+ else
+ s = hvm_select_ioreq_server(currd, &p);
Wouldn't it both be more natural and make the logic even easier
to follow if s got set to NULL up front, all the "else"-s dropped,
and a simple
if ( !s )
s = hvm_select_ioreq_server(currd, &p);
be done in the end?
Sorry, Jan. I tried to simplify above code, but found the new code is
still not very
clean, because in some cases the s is supposed to return NULL instead
of to be
set from the hvm_select_ioreq_server().
To keep the same logic, the simplified code looks like this:
case X86EMUL_UNHANDLEABLE:
{
- struct hvm_ioreq_server *s =
- hvm_select_ioreq_server(curr->domain, &p);
+ struct hvm_ioreq_server *s = NULL;
+ p2m_type_t p2mt = p2m_invalid;
+
+ if ( is_mmio && dir == IOREQ_WRITE )
+ {
+ unsigned long gmfn = paddr_to_pfn(addr);
+
+ (void) get_gfn_query_unlocked(currd, gmfn, &p2mt);
+
+ if ( p2mt == p2m_ioreq_server )
+ {
+ unsigned int flags;
+
+ s = p2m_get_ioreq_server(currd, &flags);
+ if ( !(flags & XEN_HVMOP_IOREQ_MEM_ACCESS_WRITE) )
+ s = NULL;
+ }
+ }
+
+ if ( !s && p2mt != p2m_ioreq_server )
+ s = hvm_select_ioreq_server(currd, &p);
/* If there is no suitable backing DM, just ignore accesses */
if ( !s )
As you can see, definition of p2mt is moved outside the if ( is_mmio )
judgement,
and is checked against p2m_ioreq_server before we search the ioreq
server's rangeset
in hvm_select_ioreq_server(). So I am not quite satisfied with this
simplification.
Any suggestions?
I think it's better than the code was before, but an implicit part of
my suggestion was that I'm not really convinced the
" && p2mt != p2m_ioreq_server" part of your new conditional is
really needed: Would it indeed be wrong to hand such a request
to the "normal" ioreq server, instead of terminating it right away?
(I guess that's a question to you as much as to Paul.)
Well, I thought the correct semantics for p2m_ioreq_server without any
interception are the same as p2m_ram_rw. In which case if we find an ioreq
server, but it does not want to emulate writes, then we should complete the I/O
by writing to guest RAM. But, how are we ever going to hit this code-path
without some form of race with EPT configuration?
Thanks Paul. I think the p2m type change race condition can be avoided in
hvm_hap_nested_page_fault(), by delaying the call of put_gfn() after the
ioreq is delivered. :)
Otherwise we will need to keep the previous mem_handler, and use
get_gfn_type()
instead of get_gfn_query_unlocked() in hvmemul_do_io() - because,
without a lock,
another race condition can happen when
1> a p2m_ioreq_server gfn is changed to p2m_ram_rw;
2> we got this p2mt with value p2m_ram_rw by get_gfn_query_unlocked();
3> p2m type of this gfn is changed back to p2m_ioreq_server;
4> mem_handler is called instead of deliver the ioreq to device model;
However, the mem_handler may really be helpful for the read-modify-write
case
I mentioned in another mail. So hypervisor do not need to forward the
read ioreq
to the device model.
B.R.
Yu
Yu
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
