Re: [Xen-devel] [PATCH v2 3/3] x86/ioreq server: Add HVMOP to map guest ram with p2m_ioreq_server to an ioreq server

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Yu, Zhang [mailto:yu.c.zhang@xxxxxxxxxxxxxxx]
> Sent: 19 April 2016 10:49
> To: Paul Durrant; Kevin Tian; Jan Beulich; Nakajima, Jun
> Cc: Keir (Xen.org); Andrew Cooper; Tim (Xen.org); George Dunlap; xen-
> devel@xxxxxxxxxxxxx; Lv, Zhiyuan
> Subject: Re: [Xen-devel] [PATCH v2 3/3] x86/ioreq server: Add HVMOP to
> map guest ram with p2m_ioreq_server to an ioreq server
> 
> 
> 
> On 4/19/2016 5:40 PM, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Yu, Zhang [mailto:yu.c.zhang@xxxxxxxxxxxxxxx]
> >> Sent: 19 April 2016 10:27
> >> To: Paul Durrant; Kevin Tian; Jan Beulich; Nakajima, Jun
> >> Cc: Keir (Xen.org); Andrew Cooper; Tim (Xen.org); George Dunlap; xen-
> >> devel@xxxxxxxxxxxxx; Lv, Zhiyuan
> >> Subject: Re: [Xen-devel] [PATCH v2 3/3] x86/ioreq server: Add HVMOP to
> >> map guest ram with p2m_ioreq_server to an ioreq server
> >>
> >>
> >>
> >> On 4/19/2016 4:46 PM, Paul Durrant wrote:
> >>>> -----Original Message-----
> >>>> From: Tian, Kevin [mailto:kevin.tian@xxxxxxxxx]
> >>>> Sent: 19 April 2016 05:51
> >>>> To: Yu, Zhang; Jan Beulich; Paul Durrant; Nakajima, Jun
> >>>> Cc: Andrew Cooper; George Dunlap; Lv, Zhiyuan; xen-
> >> devel@xxxxxxxxxxxxx;
> >>>> Keir (Xen.org); Tim (Xen.org)
> >>>> Subject: RE: [Xen-devel] [PATCH v2 3/3] x86/ioreq server: Add HVMOP
> to
> >>>> map guest ram with p2m_ioreq_server to an ioreq server
> >>>>
> >>>>> From: Yu, Zhang [mailto:yu.c.zhang@xxxxxxxxxxxxxxx]
> >>>>> Sent: Thursday, April 14, 2016 5:57 PM
> >>>>>>>>>> And with all three bits now possibly being clear, aren't we
> risking
> >> the
> >>>>>>>>>> entries to be mis-treated as not-present ones?
> >>>>>>>>>
> >>>>>>>>> Hah. You got me. Thanks! :)
> >>>>>>>>> Now I realized it would be difficult if we wanna to emulate the
> read
> >>>>>>>>> operations for HVM. According to Intel mannual, entry->r is to
> be
> >>>>>>>>> cleared, so should entry->w if we do not want ept misconfig.
> And
> >>>>>>>>> with both read and write permissions being forbidden, entry->x
> >> can
> >>>> be
> >>>>>>>>> set only on processors with EXECUTE_ONLY capability.
> >>>>>>>>> To avoid any entry to be mis-treated as not-present. We have
> >> several
> >>>>>>>>> solutions:
> >>>>>>>>> a> do not support the read emulation for now - we have no such
> >>>> usage
> >>>>>>>>> case;
> >>>>>>>>> b> add the check of p2m_t against p2m_ioreq_server in
> >>>> is_epte_present -
> >>>>>>>>> a bit weird to me.
> >>>>>>>>> Which one do you prefer? or any other suggestions?
> >>>>>>>>
> >>>>>>>> That question would also need to be asked to others who had
> >>>>>>>> suggested supporting both. I'd be fine with a, but I also don't view
> >>>>>>>> b as too awkward.
> >>>>>>>
> >>>>>>> According to Intel mannual, an entry is regarded as not present, if
> >>>>>>> bit0:2 is 0. So adding a p2m type check in is_epte_present() means
> we
> >>>>>>> will change its semantics, if this is acceptable(with no hurt to
> >>>>>>> hypervisor). I'd prefer option b>
> >>>>>>
> >>>>>> Perhaps time for the VMX maintainers to chime in - such a change is
> >>>> acceptable
> >>>>>> only if it doesn't result in changed hardware behavior. I can't think 
> >>>>>> of
> >> any
> >>>> such off
> >>>>>> the top of my head, but this really should be confirmed by the
> >>>> maintainers before
> >>>>>> deciding to go such a route.
> >>>>>>
> >>>>>
> >>>>> Thanks, Jan. :)
> >>>>> Jun & Kevin, any suggestions?
> >>>>>
> >>>>
> >>>> I'm a bit worried about this change, since it's a fundamental EPT
> >>>> interface. Can we avoid supporting read emulation now?
> >>>>
> >>>
> >>> I'm happy to drop the implementation of read emulation for the
> moment
> >> to keep things simple, as long as it is kept in the interface.
> >>>
> >>
> >> Thanks, Paul.
> >> So what's the benefit to keep the read in the interface, if we can
> >> not support its emulation?
> >>
> >
> > Well, if it's in the interface then support can be added in future. If it's 
> > not in
> the interface then it needs to be added later and that makes things more
> awkward compatibility-wise.
> >
> >    Paul
> >
> 
> But if we need to support read emulation in the future, we'll have to
> clear bit0:2 in ept pte, which is the same situation we are facing
> now. So why do we not support it now?
> 
> Sorry for seeming so stubborn, I just want to have some more convincing
> explanations. :)
> 

That's an argument you need to have with Kevin. If he thinks the risk of making 
the change now is small then go ahead. All I'm saying is that 4.7 is about to 
go out of the door so whatever happens now needs to be as low risk and as 
minimally disruptive as possible.

  Paul

> Yu
> 
> >> It could be easier to change the is_epte_present definition than
> >> to remove the read emulation code, but either way would not be
> >> difficult.
> >>
> >> Yu
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@xxxxxxxxxxxxx
> > http://lists.xen.org/xen-devel
> >
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel