From: Paul Durrant <Paul.Durrant@citrix.com>
To: Jan Beulich <JBeulich@suse.com>, Yu Zhang <yu.c.zhang@linux.intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>,
Andrew Cooper <Andrew.Cooper3@citrix.com>,
"Tim (Xen.org)" <tim@xen.org>,
George Dunlap <George.Dunlap@citrix.com>,
"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>,
"zhiyuan.lv@intel.com" <zhiyuan.lv@intel.com>,
Jun Nakajima <jun.nakajima@intel.com>
Subject: Re: [PATCH v5 3/4] x86/ioreq server: Add HVMOP to map guest ram with p2m_ioreq_server to an ioreq server.
Date: Wed, 10 Aug 2016 10:43:02 +0000 [thread overview]
Message-ID: <f5439adbb06443cab63e5923c6e3dce4@AMSPEX02CL03.citrite.net> (raw)
In-Reply-To: <57AB1F040200007800104A01@prv-mh.provo.novell.com>
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 10 August 2016 11:33
> To: Paul Durrant; Yu Zhang
> Cc: Andrew Cooper; George Dunlap; Jun Nakajima; Kevin Tian;
> zhiyuan.lv@intel.com; xen-devel@lists.xen.org; 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@linux.intel.com> wrote:
>
> >
> > On 8/8/2016 11:40 PM, Jan Beulich wrote:
> >>>>> On 12.07.16 at 11:02, <yu.c.zhang@linux.intel.com> 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?
Paul
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
next prev parent reply other threads:[~2016-08-10 10:43 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-07-12 9:02 [PATCH v5 0/4] x86/ioreq server: Introduce HVMMEM_ioreq_server mem type Yu Zhang
2016-07-12 9:02 ` [PATCH v5 1/4] x86/ioreq server: Rename p2m_mmio_write_dm to p2m_ioreq_server Yu Zhang
2016-07-12 9:02 ` [PATCH v5 2/4] x86/ioreq server: Add new functions to get/set memory types Yu Zhang
2016-07-12 9:02 ` [PATCH v5 3/4] x86/ioreq server: Add HVMOP to map guest ram with p2m_ioreq_server to an ioreq server Yu Zhang
2016-08-08 15:40 ` Jan Beulich
2016-08-09 7:39 ` Yu Zhang
2016-08-09 8:11 ` Jan Beulich
2016-08-09 8:20 ` Paul Durrant
2016-08-09 8:51 ` Yu Zhang
2016-08-09 9:07 ` Paul Durrant
2016-08-10 8:09 ` Yu Zhang
2016-08-10 10:33 ` Jan Beulich
2016-08-10 10:43 ` Paul Durrant [this message]
2016-08-10 12:32 ` Yu Zhang
2016-08-10 10:43 ` Yu Zhang
2016-08-11 8:47 ` Yu Zhang
2016-08-11 8:58 ` Jan Beulich
2016-08-11 9:19 ` Yu Zhang
2016-07-12 9:02 ` [PATCH v5 4/4] x86/ioreq server: Reset outstanding p2m_ioreq_server entries when an ioreq server unmaps Yu Zhang
2016-08-08 16:29 ` Jan Beulich
2016-08-09 7:39 ` Yu Zhang
2016-08-09 8:13 ` Jan Beulich
2016-08-09 9:25 ` Yu Zhang
2016-08-09 9:45 ` Jan Beulich
2016-08-09 10:21 ` Yu Zhang
2016-08-16 13:35 ` George Dunlap
2016-08-16 13:54 ` Jan Beulich
2016-08-30 12:17 ` Yu Zhang
2016-09-02 11:00 ` Yu Zhang
2016-08-05 2:44 ` [PATCH v5 0/4] x86/ioreq server: Introduce HVMMEM_ioreq_server mem type Yu Zhang
2016-08-05 6:16 ` Jan Beulich
2016-08-05 12:46 ` George Dunlap
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=f5439adbb06443cab63e5923c6e3dce4@AMSPEX02CL03.citrite.net \
--to=paul.durrant@citrix.com \
--cc=Andrew.Cooper3@citrix.com \
--cc=George.Dunlap@citrix.com \
--cc=JBeulich@suse.com \
--cc=jun.nakajima@intel.com \
--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).