Re: [PATCH 2/2] altp2m: Implement p2m_get_mem_access for altp2m views

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 01/28/2016 05:58 PM, Lengyel, Tamas wrote:
> 
> 
> On Thu, Jan 28, 2016 at 8:20 AM, Razvan Cojocaru
> <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> wrote:
> 
>     On 01/28/2016 05:12 PM, Lengyel, Tamas wrote:
>     >
>     > On Jan 28, 2016 8:02 AM, "Razvan Cojocaru" <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>
>     > <mailto:rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>>> wrote:
>     >>
>     >> On 01/28/2016 04:42 PM, Lengyel, Tamas wrote:
>     >> >
>     >> > On Jan 28, 2016 6:38 AM, "Jan Beulich" <JBeulich@suse.com <mailto:JBeulich@suse.com>
>     > <mailto:JBeulich@suse.com <mailto:JBeulich@suse.com>>
>     >> > <mailto:JBeulich@suse.com <mailto:JBeulich@suse.com>
>     <mailto:JBeulich@suse.com <mailto:JBeulich@suse.com>>>> wrote:
>     >> >>
>     >> >> >>> On 27.01.16 at 21:06, <tlengyel@novetta.com <mailto:tlengyel@novetta.com>
>     > <mailto:tlengyel@novetta.com <mailto:tlengyel@novetta.com>>
>     >> > <mailto:tlengyel@novetta.com <mailto:tlengyel@novetta.com>
>     <mailto:tlengyel@novetta.com <mailto:tlengyel@novetta.com>>>> wrote:
>     >> >> > --- a/xen/arch/x86/mm/p2m.c
>     >> >> > +++ b/xen/arch/x86/mm/p2m.c
>     >> >> > @@ -1572,7 +1572,9 @@ void p2m_mem_access_emulate_check(struct
>     > vcpu *v,
>     >> >> >          bool_t violation = 1;
>     >> >> >          const struct vm_event_mem_access *data =
>     &rsp->u.mem_access;
>     >> >> >
>     >> >> > -        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn),
>     >> > &access) == 0 )
>     >> >> > +        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn),
>     >> >> > +                                altp2m_active(v->domain) ?
>     >> > vcpu_altp2m(v).p2midx : 0,
>     >> >> > +                                &access) == 0 )
>     >> >>
>     >> >> This looks to be a behavioral change beyond what title and
>     >> >> description say, and it's not clear whether that's actually the
>     >> >> behavior everyone wants.
>     >> >
>     >> > I'm fairly comfident its exactly the expected behavior when one
>     uses
>     >> > mem_access in altp2m tables and emulation. Right now because
>     the lack of
>     >> > this AFAIK emulation would not work correctly with altp2m. But
>     Razvan
>     >> > probably can chime in as he uses this path actively.
>     >>
>     >> I've done an experiment to see how much slower using altp2m would
>     be as
>     >> compared to emulation - so I'm not a big user of the feature, but
>     I did
>     >> find it cumbersome to have to work with two sets of APIs (one for
>     what
>     >> could arguably be called the default altp2m view, i.e. the regular
>     >> xc_set_mem_access(), and one for altp2m, i.e.
>     >> xc_altp2m_set_mem_access()). Furthermore, the APIs do not currently
>     >> offer the same features (most notably, xc_altp2m_get_mem_access() is
>     >> completely missing). I've mentioned this to Tamas while initially
>     trying
>     >> to get it to work.
>     >>
>     >> Now, whether the behaviour I expect is what everyone expects is, of
>     >> course, wide open to debate. But I think we can all agree that the
>     >> altp2m interface can, and probably should, be improved.
>     >>
>     >
>     > There is that, but also, what is the exact logic behind doing this
>     check
>     > before emulation? AFAIU emulation happens in response to a vm_event so
>     > we should be fairly certain that this check succeeds as it just
>     verifies
>     > that indeed the permissions are restricted by mem_access in the
>     p2m (and
>     > with altp2m this should be the active one). But when is this check
>     > normally expected to fail?
> 
>     That check is important, please do not remove it. A vm_event is sent
>     into userspace to our monitoring application, but the monitoring
>     application can actually remove the page restrictions before replying,
>     so in that case emulation is pointless - there will be no more page
>     faults for that instruction.
> 
> 
> I see, but then why would you reply with VM_EVENT_FLAG_EMULATE? You know
> you removed the permission before sending the reply, so this sounds like
> something specific to your application.

It's cheap insurance that things go right. If there's some issue with
page rights, or some external tool somehow does an xc_set_mem_access(),
things won't go wrong. And they will go wrong if Xen thinks it should
emulate the next instruction and the next instruction is not the one
that has caused the original fault. I would think that benefits any
application.


Thanks,
Razvan