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

["Lengyel, Tamas" <tlengyel@xxxxxxxxxxx>]

On Thu, Jan 28, 2016 at 8:20 AM, Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx> wrote:

On 01/28/2016 05:12 PM, Lengyel, Tamas wrote:
>
> On Jan 28, 2016 8:02 AM, "Razvan Cojocaru" <rcojocaru@xxxxxxxxxxxxxxx
> <mailto:rcojocaru@xxxxxxxxxxxxxxx>> wrote:
>>
>> On 01/28/2016 04:42 PM, Lengyel, Tamas wrote:
>> >
>> > On Jan 28, 2016 6:38 AM, "Jan Beulich" <JBeulich@xxxxxxxx
> <mailto:JBeulich@xxxxxxxx>
>> > <mailto:JBeulich@xxxxxxxx <mailto:JBeulich@xxxxxxxx>>> wrote:
>> >>
>> >> >>> On 27.01.16 at 21:06, <tlengyel@xxxxxxxxxxx
> <mailto:tlengyel@xxxxxxxxxxx>

>> > <mailto:tlengyel@xxxxxxxxxxx <mailto:tlengyel@xxxxxxxxxxx>>> 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 = ""> >> >> >
>> >> > -Â Â Â Â 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.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel