All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] x86/altp2m: Allow setting the #VE info page for an arbitrary VCPU
@ 2018-07-25 11:49 Adrian Pop
  2018-07-31 11:53 ` Jan Beulich
  0 siblings, 1 reply; 10+ messages in thread
From: Adrian Pop @ 2018-07-25 11:49 UTC (permalink / raw)
  To: xen-devel
  Cc: Tamas K Lengyel, Wei Liu, Adrian Pop, Sergej Proskurin,
	Ian Jackson, Jan Beulich, Andrew Cooper

The intended use-case of this patch is to allow either Dom0 or a control
domain to activate #VE for an introspected guest, and not having to do
this necessarily from an in-guest agent.

Signed-off-by: Adrian Pop <apop@bitdefender.com>
---
 tools/libxc/xc_altp2m.c |  1 -
 xen/arch/x86/hvm/hvm.c  | 47 +++++++++++++++++++++++++++++++++--------
 2 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
index 1c9b572e2b..be5bfd28ed 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 4e318cede4..0157611384 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4467,6 +4467,30 @@ static int hvmop_get_param(
     return rc;
 }
 
+/*
+ * Find the struct vcpu given a dom_id and vcpu_id.
+ * Return NULL if not found.
+ */
+static struct vcpu *__get_vcpu(domid_t domain_id, uint32_t vcpu_id)
+{
+    struct domain *dom;
+    struct vcpu *v;
+
+    dom = rcu_lock_domain_by_id(domain_id);
+
+    for_each_vcpu( dom, v )
+    {
+        if ( vcpu_id == v->vcpu_id )
+        {
+            rcu_unlock_domain(dom);
+            return v;
+        }
+    }
+
+    rcu_unlock_domain(dom);
+    return NULL;
+}
+
 static int do_altp2m_op(
     XEN_GUEST_HANDLE_PARAM(void) arg)
 {
@@ -4504,8 +4528,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);
 
     if ( d == NULL )
         return -ESRCH;
@@ -4576,26 +4599,32 @@ static int do_altp2m_op(
 
     case HVMOP_altp2m_vcpu_enable_notify:
     {
-        struct vcpu *curr = current;
+        struct vcpu *v;
         p2m_type_t p2mt;
 
-        if ( a.u.enable_notify.pad || a.domain != DOMID_SELF ||
-             a.u.enable_notify.vcpu_id != curr->vcpu_id )
+        if ( a.u.enable_notify.pad )
         {
             rc = -EINVAL;
             break;
         }
 
-        if ( !gfn_eq(vcpu_altp2m(curr).veinfo_gfn, INVALID_GFN) ||
-             mfn_eq(get_gfn_query_unlocked(curr->domain,
+        v = __get_vcpu(a.domain, a.u.enable_notify.vcpu_id);
+        if ( !v )
+        {
+            rc = -EFAULT;
+            break;
+        }
+
+        if ( !gfn_eq(vcpu_altp2m(v).veinfo_gfn, INVALID_GFN) ||
+             mfn_eq(get_gfn_query_unlocked(v->domain,
                     a.u.enable_notify.gfn, &p2mt), INVALID_MFN) )
         {
             rc = -EINVAL;
             break;
         }
 
-        vcpu_altp2m(curr).veinfo_gfn = _gfn(a.u.enable_notify.gfn);
-        altp2m_vcpu_update_vmfunc_ve(curr);
+        vcpu_altp2m(v).veinfo_gfn = _gfn(a.u.enable_notify.gfn);
+        altp2m_vcpu_update_vmfunc_ve(v);
         break;
     }
 
-- 
2.17.0


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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] x86/altp2m: Allow setting the #VE info page for an arbitrary VCPU
  2018-07-25 11:49 [PATCH] x86/altp2m: Allow setting the #VE info page for an arbitrary VCPU Adrian Pop
@ 2018-07-31 11:53 ` Jan Beulich
  2018-07-31 17:19   ` Tamas K Lengyel
  2018-08-27  9:41   ` Adrian Pop
  0 siblings, 2 replies; 10+ messages in thread
From: Jan Beulich @ 2018-07-31 11:53 UTC (permalink / raw)
  To: Adrian Pop
  Cc: Tamas K Lengyel, Wei Liu, Sergej Proskurin, Ian Jackson,
	Andrew Cooper, xen-devel

>>> On 25.07.18 at 13:49, <apop@bitdefender.com> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4467,6 +4467,30 @@ static int hvmop_get_param(
>      return rc;
>  }
>  
> +/*
> + * Find the struct vcpu given a dom_id and vcpu_id.
> + * Return NULL if not found.
> + */
> +static struct vcpu *__get_vcpu(domid_t domain_id, uint32_t vcpu_id)

No double leading underscores please, and avoid the use of fixed
width types when you don't really need it (unsigned int is fine here).

> +{
> +    struct domain *dom;

We commonly call this just d.

> +    struct vcpu *v;
> +
> +    dom = rcu_lock_domain_by_id(domain_id);
> +
> +    for_each_vcpu( dom, v )
> +    {
> +        if ( vcpu_id == v->vcpu_id )
> +        {
> +            rcu_unlock_domain(dom);
> +            return v;
> +        }
> +    }

for_each_vcpu() looks excessive here - all you need is a bounds
check and an access into d->vcpus[]. Together with the fact
that your caller has already identified and locked d I wonder
whether this helper is needed in the first place.

> @@ -4576,26 +4599,32 @@ static int do_altp2m_op(
>  
>      case HVMOP_altp2m_vcpu_enable_notify:
>      {
> -        struct vcpu *curr = current;
> +        struct vcpu *v;
>          p2m_type_t p2mt;
>  
> -        if ( a.u.enable_notify.pad || a.domain != DOMID_SELF ||
> -             a.u.enable_notify.vcpu_id != curr->vcpu_id )
> +        if ( a.u.enable_notify.pad )
>          {
>              rc = -EINVAL;
>              break;
>          }
>  
> -        if ( !gfn_eq(vcpu_altp2m(curr).veinfo_gfn, INVALID_GFN) ||
> -             mfn_eq(get_gfn_query_unlocked(curr->domain,
> +        v = __get_vcpu(a.domain, a.u.enable_notify.vcpu_id);
> +        if ( !v )
> +        {
> +            rc = -EFAULT;

Hardly an appropriate error indicator for the condition.

> +            break;
> +        }
> +
> +        if ( !gfn_eq(vcpu_altp2m(v).veinfo_gfn, INVALID_GFN) ||
> +             mfn_eq(get_gfn_query_unlocked(v->domain,
>                      a.u.enable_notify.gfn, &p2mt), INVALID_MFN) )
>          {
>              rc = -EINVAL;
>              break;
>          }
>  
> -        vcpu_altp2m(curr).veinfo_gfn = _gfn(a.u.enable_notify.gfn);
> -        altp2m_vcpu_update_vmfunc_ve(curr);
> +        vcpu_altp2m(v).veinfo_gfn = _gfn(a.u.enable_notify.gfn);
> +        altp2m_vcpu_update_vmfunc_ve(v);

I'd like you to outline in the description how you mean an external
agent to coordinate the use of this GFN with the guest (and in
particular without in-guest agent).

Jan



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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] x86/altp2m: Allow setting the #VE info page for an arbitrary VCPU
  2018-07-31 11:53 ` Jan Beulich
@ 2018-07-31 17:19   ` Tamas K Lengyel
  2018-07-31 17:56     ` Razvan Cojocaru
  2018-08-01  8:23     ` Jan Beulich
  2018-08-27  9:41   ` Adrian Pop
  1 sibling, 2 replies; 10+ messages in thread
From: Tamas K Lengyel @ 2018-07-31 17:19 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Wei Liu, Adrian Pop, Sergej Proskurin, Ian Jackson,
	Andrew Cooper, Xen-devel

On Tue, Jul 31, 2018 at 5:53 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 25.07.18 at 13:49, <apop@bitdefender.com> wrote:
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -4467,6 +4467,30 @@ static int hvmop_get_param(
> >      return rc;
> >  }
> >
> > +/*
> > + * Find the struct vcpu given a dom_id and vcpu_id.
> > + * Return NULL if not found.
> > + */
> > +static struct vcpu *__get_vcpu(domid_t domain_id, uint32_t vcpu_id)
>
> No double leading underscores please, and avoid the use of fixed
> width types when you don't really need it (unsigned int is fine here).
>
> > +{
> > +    struct domain *dom;
>
> We commonly call this just d.
>
> > +    struct vcpu *v;
> > +
> > +    dom = rcu_lock_domain_by_id(domain_id);
> > +
> > +    for_each_vcpu( dom, v )
> > +    {
> > +        if ( vcpu_id == v->vcpu_id )
> > +        {
> > +            rcu_unlock_domain(dom);
> > +            return v;
> > +        }
> > +    }
>
> for_each_vcpu() looks excessive here - all you need is a bounds
> check and an access into d->vcpus[]. Together with the fact
> that your caller has already identified and locked d I wonder
> whether this helper is needed in the first place.
>
> > @@ -4576,26 +4599,32 @@ static int do_altp2m_op(
> >
> >      case HVMOP_altp2m_vcpu_enable_notify:
> >      {
> > -        struct vcpu *curr = current;
> > +        struct vcpu *v;
> >          p2m_type_t p2mt;
> >
> > -        if ( a.u.enable_notify.pad || a.domain != DOMID_SELF ||
> > -             a.u.enable_notify.vcpu_id != curr->vcpu_id )
> > +        if ( a.u.enable_notify.pad )
> >          {
> >              rc = -EINVAL;
> >              break;
> >          }
> >
> > -        if ( !gfn_eq(vcpu_altp2m(curr).veinfo_gfn, INVALID_GFN) ||
> > -             mfn_eq(get_gfn_query_unlocked(curr->domain,
> > +        v = __get_vcpu(a.domain, a.u.enable_notify.vcpu_id);
> > +        if ( !v )
> > +        {
> > +            rc = -EFAULT;
>
> Hardly an appropriate error indicator for the condition.
>
> > +            break;
> > +        }
> > +
> > +        if ( !gfn_eq(vcpu_altp2m(v).veinfo_gfn, INVALID_GFN) ||
> > +             mfn_eq(get_gfn_query_unlocked(v->domain,
> >                      a.u.enable_notify.gfn, &p2mt), INVALID_MFN) )
> >          {
> >              rc = -EINVAL;
> >              break;
> >          }
> >
> > -        vcpu_altp2m(curr).veinfo_gfn = _gfn(a.u.enable_notify.gfn);
> > -        altp2m_vcpu_update_vmfunc_ve(curr);
> > +        vcpu_altp2m(v).veinfo_gfn = _gfn(a.u.enable_notify.gfn);
> > +        altp2m_vcpu_update_vmfunc_ve(v);
>
> I'd like you to outline in the description how you mean an external
> agent to coordinate the use of this GFN with the guest (and in
> particular without in-guest agent).

Using #VE without an in-guest agent isn't really possible so this is
really just about designating the page from dom0 instead of doing it
with the in-guest agent. Something has to be present in the guest to
handle the new interrupts coming from #VE after all. I could image a
number of ways to coordinate that dom0 agent now setting this page
with the guest, my immediate guess is that it would be done through
the vm_event guest request channel. But it could also be done through
xenstore or even ssh.

Tamas

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] x86/altp2m: Allow setting the #VE info page for an arbitrary VCPU
  2018-07-31 17:19   ` Tamas K Lengyel
@ 2018-07-31 17:56     ` Razvan Cojocaru
  2018-08-01  8:23     ` Jan Beulich
  1 sibling, 0 replies; 10+ messages in thread
From: Razvan Cojocaru @ 2018-07-31 17:56 UTC (permalink / raw)
  To: Tamas K Lengyel, Jan Beulich
  Cc: Wei Liu, Adrian Pop, Sergej Proskurin, Ian Jackson,
	Andrew Cooper, Xen-devel

On 07/31/2018 08:19 PM, Tamas K Lengyel wrote:
> On Tue, Jul 31, 2018 at 5:53 AM Jan Beulich <JBeulich@suse.com> wrote:
>> I'd like you to outline in the description how you mean an external
>> agent to coordinate the use of this GFN with the guest (and in
>> particular without in-guest agent).
> 
> Using #VE without an in-guest agent isn't really possible so this is
> really just about designating the page from dom0 instead of doing it
> with the in-guest agent. Something has to be present in the guest to
> handle the new interrupts coming from #VE after all. I could image a
> number of ways to coordinate that dom0 agent now setting this page
> with the guest, my immediate guess is that it would be done through
> the vm_event guest request channel. But it could also be done through
> xenstore or even ssh.

That's exactly what happens - they coordinate via the guest-request
vm_event. There's a #VE driver in the guest.


Thanks,
Razvan

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] x86/altp2m: Allow setting the #VE info page for an arbitrary VCPU
  2018-07-31 17:19   ` Tamas K Lengyel
  2018-07-31 17:56     ` Razvan Cojocaru
@ 2018-08-01  8:23     ` Jan Beulich
  2018-08-01  8:30       ` Razvan Cojocaru
  2018-08-01  8:38       ` Andrew Cooper
  1 sibling, 2 replies; 10+ messages in thread
From: Jan Beulich @ 2018-08-01  8:23 UTC (permalink / raw)
  To: Tamas K Lengyel
  Cc: Wei Liu, Adrian Pop, Sergej Proskurin, Ian Jackson,
	Andrew Cooper, xen-devel

>>> On 31.07.18 at 19:19, <tamas@tklengyel.com> wrote:
> On Tue, Jul 31, 2018 at 5:53 AM Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 25.07.18 at 13:49, <apop@bitdefender.com> wrote:
>> > -        vcpu_altp2m(curr).veinfo_gfn = _gfn(a.u.enable_notify.gfn);
>> > -        altp2m_vcpu_update_vmfunc_ve(curr);
>> > +        vcpu_altp2m(v).veinfo_gfn = _gfn(a.u.enable_notify.gfn);
>> > +        altp2m_vcpu_update_vmfunc_ve(v);
>>
>> I'd like you to outline in the description how you mean an external
>> agent to coordinate the use of this GFN with the guest (and in
>> particular without in-guest agent).
> 
> Using #VE without an in-guest agent isn't really possible so this is
> really just about designating the page from dom0 instead of doing it
> with the in-guest agent. Something has to be present in the guest to
> handle the new interrupts coming from #VE after all.

Not necessarily - the exception could also be intercepted, and the
external agent be informed, with it taking appropriate action.

Anyway - if such a dependency continues to exist, I think it would
be desirable to mention it in the description.

Jan



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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] x86/altp2m: Allow setting the #VE info page for an arbitrary VCPU
  2018-08-01  8:23     ` Jan Beulich
@ 2018-08-01  8:30       ` Razvan Cojocaru
  2018-08-01  8:38       ` Andrew Cooper
  1 sibling, 0 replies; 10+ messages in thread
From: Razvan Cojocaru @ 2018-08-01  8:30 UTC (permalink / raw)
  To: Jan Beulich, Tamas K Lengyel
  Cc: Wei Liu, Adrian Pop, Sergej Proskurin, Ian Jackson,
	Andrew Cooper, xen-devel

On 08/01/2018 11:23 AM, Jan Beulich wrote:
>>>> On 31.07.18 at 19:19, <tamas@tklengyel.com> wrote:
>> On Tue, Jul 31, 2018 at 5:53 AM Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 25.07.18 at 13:49, <apop@bitdefender.com> wrote:
>>>> -        vcpu_altp2m(curr).veinfo_gfn = _gfn(a.u.enable_notify.gfn);
>>>> -        altp2m_vcpu_update_vmfunc_ve(curr);
>>>> +        vcpu_altp2m(v).veinfo_gfn = _gfn(a.u.enable_notify.gfn);
>>>> +        altp2m_vcpu_update_vmfunc_ve(v);
>>>
>>> I'd like you to outline in the description how you mean an external
>>> agent to coordinate the use of this GFN with the guest (and in
>>> particular without in-guest agent).
>>
>> Using #VE without an in-guest agent isn't really possible so this is
>> really just about designating the page from dom0 instead of doing it
>> with the in-guest agent. Something has to be present in the guest to
>> handle the new interrupts coming from #VE after all.
> 
> Not necessarily - the exception could also be intercepted, and the
> external agent be informed, with it taking appropriate action.

Right, but the main attraction of a hypervisor-level agent is that it
can't be manipulated at all from within the guest, unlike traditional
solutions.

With that in mind, the in-guest agent should be an optimization-only
tool, and not allowed to make its own decisions about the core
introspection logic.


Thanks,
Razvan

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] x86/altp2m: Allow setting the #VE info page for an arbitrary VCPU
  2018-08-01  8:23     ` Jan Beulich
  2018-08-01  8:30       ` Razvan Cojocaru
@ 2018-08-01  8:38       ` Andrew Cooper
  2018-08-01  9:14         ` Jan Beulich
  1 sibling, 1 reply; 10+ messages in thread
From: Andrew Cooper @ 2018-08-01  8:38 UTC (permalink / raw)
  To: Jan Beulich, Tamas K Lengyel
  Cc: Sergej Proskurin, Ian Jackson, Wei Liu, xen-devel, Adrian Pop

On 01/08/2018 09:23, Jan Beulich wrote:
>>>> On 31.07.18 at 19:19, <tamas@tklengyel.com> wrote:
>> On Tue, Jul 31, 2018 at 5:53 AM Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 25.07.18 at 13:49, <apop@bitdefender.com> wrote:
>>>> -        vcpu_altp2m(curr).veinfo_gfn = _gfn(a.u.enable_notify.gfn);
>>>> -        altp2m_vcpu_update_vmfunc_ve(curr);
>>>> +        vcpu_altp2m(v).veinfo_gfn = _gfn(a.u.enable_notify.gfn);
>>>> +        altp2m_vcpu_update_vmfunc_ve(v);
>>> I'd like you to outline in the description how you mean an external
>>> agent to coordinate the use of this GFN with the guest (and in
>>> particular without in-guest agent).
>> Using #VE without an in-guest agent isn't really possible so this is
>> really just about designating the page from dom0 instead of doing it
>> with the in-guest agent. Something has to be present in the guest to
>> handle the new interrupts coming from #VE after all.
> Not necessarily - the exception could also be intercepted, and the
> external agent be informed, with it taking appropriate action.
>
> Anyway - if such a dependency continues to exist, I think it would
> be desirable to mention it in the description.

Any setup which intercepts #VE defeats the entire purpose of using #VE
in the first place.

We'll eventually have to cope with this situation because its not one we
can architecturally rule out (especially with nested virt in the mix),
but intercepting #VE is not something we should make available as a
user-available option.

~Andrew

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] x86/altp2m: Allow setting the #VE info page for an arbitrary VCPU
  2018-08-01  8:38       ` Andrew Cooper
@ 2018-08-01  9:14         ` Jan Beulich
  2018-08-01  9:29           ` Andrew Cooper
  0 siblings, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2018-08-01  9:14 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Tamas K Lengyel, Wei Liu, Adrian Pop, Sergej Proskurin,
	Ian Jackson, xen-devel

>>> On 01.08.18 at 10:38, <andrew.cooper3@citrix.com> wrote:
> On 01/08/2018 09:23, Jan Beulich wrote:
>>>>> On 31.07.18 at 19:19, <tamas@tklengyel.com> wrote:
>>> On Tue, Jul 31, 2018 at 5:53 AM Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> On 25.07.18 at 13:49, <apop@bitdefender.com> wrote:
>>>>> -        vcpu_altp2m(curr).veinfo_gfn = _gfn(a.u.enable_notify.gfn);
>>>>> -        altp2m_vcpu_update_vmfunc_ve(curr);
>>>>> +        vcpu_altp2m(v).veinfo_gfn = _gfn(a.u.enable_notify.gfn);
>>>>> +        altp2m_vcpu_update_vmfunc_ve(v);
>>>> I'd like you to outline in the description how you mean an external
>>>> agent to coordinate the use of this GFN with the guest (and in
>>>> particular without in-guest agent).
>>> Using #VE without an in-guest agent isn't really possible so this is
>>> really just about designating the page from dom0 instead of doing it
>>> with the in-guest agent. Something has to be present in the guest to
>>> handle the new interrupts coming from #VE after all.
>> Not necessarily - the exception could also be intercepted, and the
>> external agent be informed, with it taking appropriate action.
>>
>> Anyway - if such a dependency continues to exist, I think it would
>> be desirable to mention it in the description.
> 
> Any setup which intercepts #VE defeats the entire purpose of using #VE
> in the first place.

Mostly, I agree, but "entire"? You never know how creative people
get, and how seeing #VE might better fit their purpose than other
"notification" mechanisms.

Jan



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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] x86/altp2m: Allow setting the #VE info page for an arbitrary VCPU
  2018-08-01  9:14         ` Jan Beulich
@ 2018-08-01  9:29           ` Andrew Cooper
  0 siblings, 0 replies; 10+ messages in thread
From: Andrew Cooper @ 2018-08-01  9:29 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Tamas K Lengyel, Wei Liu, Adrian Pop, Sergej Proskurin,
	Ian Jackson, xen-devel

On 01/08/2018 10:14, Jan Beulich wrote:
>>>> On 01.08.18 at 10:38, <andrew.cooper3@citrix.com> wrote:
>> On 01/08/2018 09:23, Jan Beulich wrote:
>>>>>> On 31.07.18 at 19:19, <tamas@tklengyel.com> wrote:
>>>> On Tue, Jul 31, 2018 at 5:53 AM Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>> On 25.07.18 at 13:49, <apop@bitdefender.com> wrote:
>>>>>> -        vcpu_altp2m(curr).veinfo_gfn = _gfn(a.u.enable_notify.gfn);
>>>>>> -        altp2m_vcpu_update_vmfunc_ve(curr);
>>>>>> +        vcpu_altp2m(v).veinfo_gfn = _gfn(a.u.enable_notify.gfn);
>>>>>> +        altp2m_vcpu_update_vmfunc_ve(v);
>>>>> I'd like you to outline in the description how you mean an external
>>>>> agent to coordinate the use of this GFN with the guest (and in
>>>>> particular without in-guest agent).
>>>> Using #VE without an in-guest agent isn't really possible so this is
>>>> really just about designating the page from dom0 instead of doing it
>>>> with the in-guest agent. Something has to be present in the guest to
>>>> handle the new interrupts coming from #VE after all.
>>> Not necessarily - the exception could also be intercepted, and the
>>> external agent be informed, with it taking appropriate action.
>>>
>>> Anyway - if such a dependency continues to exist, I think it would
>>> be desirable to mention it in the description.
>> Any setup which intercepts #VE defeats the entire purpose of using #VE
>> in the first place.
> Mostly, I agree, but "entire"? You never know how creative people
> get, and how seeing #VE might better fit their purpose than other
> "notification" mechanisms.

It is either #VE, or EPT_VIOLATION (as that is literally the definition
of how #VE gets raised).

With the latter, you get several VMCS fields filled in for you.  For the
former, you've got to find the guest physical address which the
processor dumped the same information into.

I think I'll safely stick with my "entire" here.

~Andrew

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] x86/altp2m: Allow setting the #VE info page for an arbitrary VCPU
  2018-07-31 11:53 ` Jan Beulich
  2018-07-31 17:19   ` Tamas K Lengyel
@ 2018-08-27  9:41   ` Adrian Pop
  1 sibling, 0 replies; 10+ messages in thread
From: Adrian Pop @ 2018-08-27  9:41 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Tamas K Lengyel, Wei Liu, Sergej Proskurin, Ian Jackson,
	Andrew Cooper, xen-devel

On Tue, Jul 31, 2018 at 05:53:00AM -0600, Jan Beulich wrote:
> > +    struct vcpu *v;
> > +
> > +    dom = rcu_lock_domain_by_id(domain_id);
> > +
> > +    for_each_vcpu( dom, v )
> > +    {
> > +        if ( vcpu_id == v->vcpu_id )
> > +        {
> > +            rcu_unlock_domain(dom);
> > +            return v;
> > +        }
> > +    }
> 
> for_each_vcpu() looks excessive here - all you need is a bounds
> check and an access into d->vcpus[]. Together with the fact
> that your caller has already identified and locked d I wonder
> whether this helper is needed in the first place.
 
All right.  I'll remove it altogether.

> > @@ -4576,26 +4599,32 @@ static int do_altp2m_op(
> >  
> >      case HVMOP_altp2m_vcpu_enable_notify:
> >      {
> > -        struct vcpu *curr = current;
> > +        struct vcpu *v;
> >          p2m_type_t p2mt;
> >  
> > -        if ( a.u.enable_notify.pad || a.domain != DOMID_SELF ||
> > -             a.u.enable_notify.vcpu_id != curr->vcpu_id )
> > +        if ( a.u.enable_notify.pad )
> >          {
> >              rc = -EINVAL;
> >              break;
> >          }
> >  
> > -        if ( !gfn_eq(vcpu_altp2m(curr).veinfo_gfn, INVALID_GFN) ||
> > -             mfn_eq(get_gfn_query_unlocked(curr->domain,
> > +        v = __get_vcpu(a.domain, a.u.enable_notify.vcpu_id);
> > +        if ( !v )
> > +        {
> > +            rc = -EFAULT;
> 
> Hardly an appropriate error indicator for the condition.

I'll change it to -EINVAL.

> > +            break;
> > +        }
> > +
> > +        if ( !gfn_eq(vcpu_altp2m(v).veinfo_gfn, INVALID_GFN) ||
> > +             mfn_eq(get_gfn_query_unlocked(v->domain,
> >                      a.u.enable_notify.gfn, &p2mt), INVALID_MFN) )
> >          {
> >              rc = -EINVAL;
> >              break;
> >          }
> >  
> > -        vcpu_altp2m(curr).veinfo_gfn = _gfn(a.u.enable_notify.gfn);
> > -        altp2m_vcpu_update_vmfunc_ve(curr);
> > +        vcpu_altp2m(v).veinfo_gfn = _gfn(a.u.enable_notify.gfn);
> > +        altp2m_vcpu_update_vmfunc_ve(v);
> 
> I'd like you to outline in the description how you mean an external
> agent to coordinate the use of this GFN with the guest (and in
> particular without in-guest agent).

I'll try to clarify this.

Thank you!

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2018-08-27  9:41 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-25 11:49 [PATCH] x86/altp2m: Allow setting the #VE info page for an arbitrary VCPU Adrian Pop
2018-07-31 11:53 ` Jan Beulich
2018-07-31 17:19   ` Tamas K Lengyel
2018-07-31 17:56     ` Razvan Cojocaru
2018-08-01  8:23     ` Jan Beulich
2018-08-01  8:30       ` Razvan Cojocaru
2018-08-01  8:38       ` Andrew Cooper
2018-08-01  9:14         ` Jan Beulich
2018-08-01  9:29           ` Andrew Cooper
2018-08-27  9:41   ` Adrian Pop

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.