[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:33 PM, Jan Beulich wrote:
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.)
Thanks for your reply, Jan.
For " && p2mt != p2m_ioreq_server" condition, it is just to guarantee
that if a write
operation is trapped, and at the same period, device model changed the
status of
ioreq server, it should be discarded.
A second thought is, I am now more worried about the " && dir ==
IOREQ_WRITE"
condition, which we used previously to set s to NULL if it is not a
write operation.
However, if HVM uses a read-modify-write instruction to operate on a
write-protected
address, it will be treated as both read and write accesses in
ept_handle_violation(). In
such situation, we need to emulate the read access first(by just
returning the value being
fetched either in hypervisor or in device model), instead of discarding
the read access.
Thanks
Yu
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
