Re: [PATCH v2] x86/mm: Suppresses vm_events caused by page-walks

[Tamas K Lengyel <tamas@tklengyel.com>]

On Fri, Feb 23, 2018 at 3:25 PM, Razvan Cojocaru
<rcojocaru@bitdefender.com> wrote:
> On 02/24/2018 12:06 AM, Tamas K Lengyel wrote:
>> On Mon, Jan 8, 2018 at 5:49 AM, Alexandru Isaila
>> <aisaila@bitdefender.com> wrote:
>>> This patch is adding a way to enable/disable nested pagefault
>>> events. It introduces the xc_monitor_nested_pagefault function
>>> and adds the nested_pagefault_disabled in the monitor structure.
>>> This is needed by the introspection so it will only get gla
>>> faults and not get spammed with other faults.
>>> In p2m_set_ad_bits the v->arch.sse_pg_dirty.eip and
>>> v->arch.sse_pg_dirty.gla are used to mark that this is the
>>> second time a fault occurs and the dirty bit is set.
>>>
>>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>>>
>>> ---
>>> Changes since V1:
>>>         - Rb V1
>>>         - Add comment in domctl.h
>>> ---
>>>  tools/libxc/include/xenctrl.h |  2 ++
>>>  tools/libxc/xc_monitor.c      | 14 ++++++++++++++
>>>  xen/arch/x86/mm/mem_access.c  | 27 +++++++++++++++++++++++++++
>>>  xen/arch/x86/monitor.c        | 13 +++++++++++++
>>>  xen/include/asm-x86/domain.h  |  6 ++++++
>>>  xen/include/asm-x86/monitor.h |  3 ++-
>>>  xen/include/public/domctl.h   |  2 ++
>>>  7 files changed, 66 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>>> index 09e1363..112c974 100644
>>> --- a/tools/libxc/include/xenctrl.h
>>> +++ b/tools/libxc/include/xenctrl.h
>>> @@ -2056,6 +2056,8 @@ int xc_monitor_descriptor_access(xc_interface *xch, uint32_t domain_id,
>>>                                   bool enable);
>>>  int xc_monitor_guest_request(xc_interface *xch, uint32_t domain_id,
>>>                               bool enable, bool sync, bool allow_userspace);
>>> +int xc_monitor_nested_pagefault(xc_interface *xch, uint32_t domain_id,
>>> +                                bool disable);
>>>  int xc_monitor_debug_exceptions(xc_interface *xch, uint32_t domain_id,
>>>                                  bool enable, bool sync);
>>>  int xc_monitor_cpuid(xc_interface *xch, uint32_t domain_id, bool enable);
>>> diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
>>> index 0233b87..e96c56d 100644
>>> --- a/tools/libxc/xc_monitor.c
>>> +++ b/tools/libxc/xc_monitor.c
>>> @@ -163,6 +163,20 @@ int xc_monitor_guest_request(xc_interface *xch, uint32_t domain_id, bool enable,
>>>      return do_domctl(xch, &domctl);
>>>  }
>>>
>>> +int xc_monitor_nested_pagefault(xc_interface *xch, uint32_t domain_id,
>>> +                                bool disable)
>>> +{
>>> +    DECLARE_DOMCTL;
>>> +
>>> +    domctl.cmd = XEN_DOMCTL_monitor_op;
>>> +    domctl.domain = domain_id;
>>> +    domctl.u.monitor_op.op = disable ? XEN_DOMCTL_MONITOR_OP_ENABLE
>>> +                                    : XEN_DOMCTL_MONITOR_OP_DISABLE;
>>> +    domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_NESTED_PAGEFAULT;
>>> +
>>> +    return do_domctl(xch, &domctl);
>>> +}
>>> +
>>>  int xc_monitor_emulate_each_rep(xc_interface *xch, uint32_t domain_id,
>>>                                  bool enable)
>>>  {
>>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>>> index c0cd017..07a334b 100644
>>> --- a/xen/arch/x86/mm/mem_access.c
>>> +++ b/xen/arch/x86/mm/mem_access.c
>>> @@ -137,6 +137,23 @@ bool p2m_mem_access_emulate_check(struct vcpu *v,
>>>      return violation;
>>>  }
>>>
>>> +static void p2m_set_ad_bits(struct vcpu *v, paddr_t ga)
>>> +{
>>> +    struct hvm_hw_cpu ctxt;
>>> +    uint32_t pfec = 0;
>>> +
>>> +    hvm_funcs.save_cpu_ctxt(v, &ctxt);
>>> +
>>> +    if ( guest_cpu_user_regs()->eip == v->arch.pg_dirty.eip
>>> +         && ga == v->arch.pg_dirty.gla )
>>> +        pfec = PFEC_write_access;
>>> +
>>> +    paging_ga_to_gfn_cr3(v, ctxt.cr3, ga, &pfec, NULL);
>>> +
>>> +    v->arch.pg_dirty.eip = guest_cpu_user_regs()->eip;
>>> +    v->arch.pg_dirty.gla = ga;
>>> +}
>>> +
>>>  bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>>>                            struct npfec npfec,
>>>                            vm_event_request_t **req_ptr)
>>> @@ -208,6 +225,16 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>>>          }
>>>      }
>>>
>>> +    if ( vm_event_check_ring(d->vm_event_monitor) &&
>>> +         d->arch.monitor.nested_pagefault_disabled &&
>>> +         npfec.kind != npfec_kind_with_gla ) /* don't send a mem_event */
>>> +    {
>>> +        v->arch.vm_event->emulate_flags = 0;
>>> +        p2m_set_ad_bits(v, gla);
>>> +
>>> +        return true;
>>> +    }
>>> +
>>>      *req_ptr = NULL;
>>>      req = xzalloc(vm_event_request_t);
>>>      if ( req )
>>> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
>>> index f229e69..e35b619 100644
>>> --- a/xen/arch/x86/monitor.c
>>> +++ b/xen/arch/x86/monitor.c
>>> @@ -241,6 +241,19 @@ int arch_monitor_domctl_event(struct domain *d,
>>>          break;
>>>      }
>>>
>>> +    case XEN_DOMCTL_MONITOR_EVENT_NESTED_PAGEFAULT:
>>> +    {
>>> +        bool old_status = ad->monitor.nested_pagefault_disabled;
>>> +
>>> +        if ( unlikely(old_status == requested_status) )
>>> +            return -EEXIST;
>>> +
>>> +        domain_pause(d);
>>> +        ad->monitor.nested_pagefault_disabled = requested_status;
>>> +        domain_unpause(d);
>>> +        break;
>>> +    }
>>> +
>>>      case XEN_DOMCTL_MONITOR_EVENT_DESC_ACCESS:
>>>      {
>>>          bool old_status = ad->monitor.descriptor_access_enabled;
>>> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
>>> index 4679d54..099af7c 100644
>>> --- a/xen/include/asm-x86/domain.h
>>> +++ b/xen/include/asm-x86/domain.h
>>> @@ -412,6 +412,7 @@ struct arch_domain
>>>          unsigned int descriptor_access_enabled                             : 1;
>>>          unsigned int guest_request_userspace_enabled                       : 1;
>>>          unsigned int emul_unimplemented_enabled                            : 1;
>>> +        unsigned int nested_pagefault_disabled                             : 1;
>>
>> All other options are "_enabled" here, so adding one that's flipped
>> just looks out of place. Any objections to making this match the rest?
>> Also, naming it "nested" just makes me think this is somehow would be
>> related to nested virtualization, but that's not the case. These would
>> be just regular pagefaults in the guest, so naming the monitor option
>> simply "pagefault" would look better to me in general.
> Hello Tamas,
>
> Here's the thinking behind preferring "disabled" to "enabled": we want
> to keep the default behaviour as it is currently, and the current
> behaviour is to send out _all_ EPT fault vm_events (caused by page walks
> or not).
>
> Now, struct arch_domain is being zeroed out on init, so if we name this
> "enabled", then that's the behaviour we're starting out with. We have no
> problem with that, but it changes the current default behaviour.

We can keep the "disabled" naming but then please add a comment to the field
saying that by default all events are sent, this is used to filter
pagefaults out.

>
> So either we name this new field "disabled", or we rename it to
> "enabled" (if we rename it, we either need to set it as a special case
> on init, or modify the default behaviour to be _not_ sending out
> page-walk-caused EPT events).
>
> If you feel strongly about options 2.A or 2.C we don't have a problem
> changing the code.
>
> About "pagefault", it reads more confusing to me, since all EPT-related
> vm_events are basically page faults. But maybe that's just me.

True. It's just confusing with "nested" also having multiple meanings.
Perhaps "inguest_pagefaults"?

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel