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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Sep 20, 2018 at 02:22:29AM -0600, Jan Beulich wrote:
> >>> 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'.


Oh, sorry. I have missed that this at least needs an ack from George.
I will wait until EOD for George to give an ack. If there isn't one by
then I will revert the patch in staging.

Wei.

> 
> Jan
> 

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