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

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

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>> 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>>> wrote:
>> >>
>> >> >>> On 27.01.16 at 21:06, <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.


Thanks,
Razvan