Re: [Xen-devel] [PATCH v2] x86/altp2m: Allow setting the #VE info page for an arbitrary VCPU

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.09.18 at 10:11, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> On 9/6/18 1:27 AM, Tamas K Lengyel wrote:
>> On Wed, Sep 5, 2018 at 12:45 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 
> wrote:
>>>
>>> On 05/09/18 19:40, Tamas K Lengyel wrote:
>>>> On Wed, Sep 5, 2018 at 10:40 AM Razvan Cojocaru
>>>> <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>>> On 9/5/18 7:28 PM, Tamas K Lengyel wrote:
>>>>>> On Tue, Sep 4, 2018 at 2:58 PM Razvan Cojocaru
>>>>>> <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>>>>> On 9/4/18 11:40 PM, Tamas K Lengyel wrote:
>>>>>>>> On Mon, Sep 3, 2018 at 10:59 PM Adrian Pop <apop@xxxxxxxxxxxxxxx> 
>>>>>>>> wrote:
>>>>>>>>> In a classic HVI + Xen setup, the introspection engine would monitor
>>>>>>>>> legacy guest page-tables by marking them read-only inside the EPT; 
>>>>>>>>> this
>>>>>>>>> way any modification explicitly made by the guest or implicitly made 
>>>>>>>>> by
>>>>>>>>> the CPU page walker would trigger an EPT violation, which would be
>>>>>>>>> forwarded by Xen to the SVA and thus the HVI agent.  The HVI agent 
>>>>>>>>> would
>>>>>>>>> analyse the modification, and act upon it - for example, a virtual 
>>>>>>>>> page
>>>>>>>>> may be remapped (its guest physical address changed inside the
>>>>>>>>> page-table), in which case the introspection logic would update the
>>>>>>>>> protection accordingly (remove EPT hook on the old gpa, and place a 
>>>>>>>>> new
>>>>>>>>> EPT hook on the new gpa).  In other cases, the modification may be of 
>>>>>>>>> no
>>>>>>>>> interest to the introspection engine - for example, the accessed/dirty
>>>>>>>>> bits may be cleared by the operating system or the accessed/dirty bits
>>>>>>>>> may be set by the CPU page walker.
>>>>>>>>>
>>>>>>>>> In our tests we discovered that the vast majority of guest page-table
>>>>>>>>> modifications fall in the second category (especially on Windows 10 
>>>>>>>>> RS4
>>>>>>>>> x64 - more than 95% of ALL the page-table modifications are 
>>>>>>>>> irrelevant to
>>>>>>>>> us) - they are of no interest to the introspection logic, but they
>>>>>>>>> trigger a very costly EPT violation nonetheless.  Therefore, we 
>>>>>>>>> decided
>>>>>>>>> to make use of the new #VE & VMFUNC features in recent Intel CPUs to
>>>>>>>>> accelerate the guest page-tables monitoring in the following way:
>>>>>>>>>
>>>>>>>>> 1. Each monitored page-table would be flagged as being convertible
>>>>>>>>>    inside the EPT, thus enabling the CPU to deliver a virtualization
>>>>>>>>>    exception to he guest instead of generating a traditional EPT
>>>>>>>>>    violation.
>>>>>>>>> 2. We inject a small filtering driver inside the protected guest VM,
>>>>>>>>>    which would intercept the virtualization exception in order to 
>>>>>>>>> handle
>>>>>>>>>    guest page-table modifications.
>>>>>>>>> 3. We create a dedicated EPT view (altp2m) for the in-guest agent, 
>>>>>>>>> which
>>>>>>>>>    would isolate the agent from the rest of the operating system; the
>>>>>>>>>    agent will switch in and out of the protected EPT view via the 
>>>>>>>>> VMFUNC
>>>>>>>>>    instruction placed inside a trampoline page, thus making the agent
>>>>>>>>>    immune to malicious code inside the guest.
>>>>>>>>>
>>>>>>>>> This way, all the page-table accesses would generate a
>>>>>>>>> virtualization-exception inside the guest instead of a costly EPT
>>>>>>>>> violation; the #VE agent would emulate and analyse the modification, 
>>>>>>>>> and
>>>>>>>>> decide whether it is relevant for the main introspection logic; if it 
>>>>>>>>> is
>>>>>>>>> relevant, it would do a VMCALL and notify the introspection engine
>>>>>>>>> about the modification; otherwise, it would resume normal instruction
>>>>>>>>> execution, thus avoiding a very costly VM exit.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Adrian Pop <apop@xxxxxxxxxxxxxxx>
>>>>>>>>> ---
>>>>>>>>> Changes in v2:
>>>>>>>>> - remove the "__get_vcpu()" helper
>>>>>>>>> ---
>>>>>>>>>  tools/libxc/xc_altp2m.c |  1 -
>>>>>>>>>  xen/arch/x86/hvm/hvm.c  | 19 ++++++++++---------
>>>>>>>>>  2 files changed, 10 insertions(+), 10 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
>>>>>>>>> index ce4a1e4d60..528e929d7a 100644
>>>>>>>>> --- a/tools/libxc/xc_altp2m.c
>>>>>>>>> +++ b/tools/libxc/xc_altp2m.c
>>>>>>>>> @@ -68,7 +68,6 @@ int xc_altp2m_set_domain_state(xc_interface 
>>>>>>>>> *handle, 
> uint32_t dom, bool state)
>>>>>>>>>      return rc;
>>>>>>>>>  }
>>>>>>>>>
>>>>>>>>> -/* This is a bit odd to me that it acts on current.. */
>>>>>>>>>  int xc_altp2m_set_vcpu_enable_notify(xc_interface *handle, uint32_t 
>>>>>>>>> domid,
>>>>>>>>>                                       uint32_t vcpuid, xen_pfn_t gfn)
>>>>>>>>>  {
>>>>>>>>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>>>>>>>>> index 72c51faecb..49c3bbee94 100644
>>>>>>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>>>>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>>>>>>> @@ -4533,8 +4533,7 @@ static int do_altp2m_op(
>>>>>>>>>          return -EOPNOTSUPP;
>>>>>>>>>      }
>>>>>>>>>
>>>>>>>>> -    d = ( a.cmd != HVMOP_altp2m_vcpu_enable_notify ) ?
>>>>>>>>> -        rcu_lock_domain_by_any_id(a.domain) : 
>>>>>>>>> rcu_lock_current_domain();
>>>>>>>>> +    d = rcu_lock_domain_by_any_id(a.domain);
>>>>>>>> Does rcu_lock_domain_by_any_id work if its from the current domain? If
>>>>>>>> not, doesn't that change this function's accessibility to be from
>>>>>>>> exclusively usable only by the outside agent?
>>>>>>> The code says it should be safe:
>>>>>>>
>>>>>>>  633 struct domain *rcu_lock_domain_by_any_id(domid_t dom)
>>>>>>>  634 {
>>>>>>>  635     if ( dom == DOMID_SELF )
>>>>>>>  636         return rcu_lock_current_domain();
>>>>>>>  637     return rcu_lock_domain_by_id(dom);
>>>>>>>  638 }
>>>>>>>
>>>>>>> as long as dom == DOMID_SELF. I think the old behaviour assumed that
>>>>>>> HVMOP_altp2m_vcpu_enable_notify alone would only ever be used from the
>>>>>>> current domain, and this change expands its usability (Adrian should
>>>>>>> correct me if I'm wrong here).
>>>>>> Sounds good, thanks!
>>>>> May we take that as an Acked-by, or are there are other things you think
>>>>> we should address?
>>>> A Reviewed-by would be appropriate, I don't think the files touched in
>>>> this patch fall under our umbrella.
>>>
>>> That doesn't prohibit you providing a Reviewed-by: tag :)
>>>
>>> The statement itself is useful and hold weight, even if it isn't in code
>>> you are a maintainer of.
>> 
>> Indeed :)
>> 
>> Reviewed-by: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
> 
> Are there any issues preventing this patch to go in? Possibly missing acks?

Well, afaics the patch has no x86 maintainer ack, nor - considering it's
an mm function sitting in the "wrong" file, at least one from the mm
maintainer. As mentioned a number of times before, it is the submitter's
responsibility to chase acks, not the committers' or maintainers'.

Jan


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