All of lore.kernel.org
 help / color / mirror / Atom feed
* [Xen-devel] [PATCH for-next] x86/svm: Correct vm_event API for descriptor accesses
@ 2019-11-28 11:44 Andrew Cooper
  2019-11-28 13:47 ` Tamas K Lengyel
                   ` (5 more replies)
  0 siblings, 6 replies; 7+ messages in thread
From: Andrew Cooper @ 2019-11-28 11:44 UTC (permalink / raw)
  To: Xen-devel
  Cc: Petre Pircalabu, Tamas K Lengyel, Wei Liu, Razvan Cojocaru,
	Adrian Pop, Andrew Cooper, Jan Beulich, Alexandru Isaila,
	Roger Pau Monné

c/s d0a699a389f1 "x86/monitor: add support for descriptor access events"
introduced logic looking for what appeared to be exitinfo (not that this
exists in SVM - exitinfo1 or 2 do), but actually passed the exit IDT vectoring
information.  There is never any IDT vectoring involved in these intercepts so
the value passed is always zero.

In fact, SVM doesn't provide any information, even in exitinfo1 and 2.  Note
the error in the public API and state that this field is always 0, and drop
the SVM logic in hvm_monitor_descriptor_access().

In the SVM vmexit handler itself, optimise the switch statement by observing
that there is a linear transformation between the SVM exit_reason and
VM_EVENT_DESC_* values.  (Bloat-o-meter reports 6028 => 5877 for a saving of
151 bytes).

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Razvan Cojocaru <rcojocaru@bitdefender.com>
CC: Tamas K Lengyel <tamas@tklengyel.com>
CC: Alexandru Isaila <aisaila@bitdefender.com>
CC: Petre Pircalabu <ppircalabu@bitdefender.com>
CC: Adrian Pop <apop@bitdefender.com>

Adrian: Do you recall what information you were attempting to forward from the
VMCB?  I can't locate anything which would plausibly be interesting.

This is part of a longer cleanup series I gathered in the wake of the task
switch issues, but it is pulled out ahead due to its interaction with the
public interface.
---
 xen/arch/x86/hvm/monitor.c    |  4 ----
 xen/arch/x86/hvm/svm/svm.c    | 37 +++++++++++++++++--------------------
 xen/include/public/vm_event.h |  4 ++--
 3 files changed, 19 insertions(+), 26 deletions(-)

diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c
index 7fb1e2c04e..1f23fe25e8 100644
--- a/xen/arch/x86/hvm/monitor.c
+++ b/xen/arch/x86/hvm/monitor.c
@@ -113,10 +113,6 @@ void hvm_monitor_descriptor_access(uint64_t exit_info,
         req.u.desc_access.arch.vmx.instr_info = exit_info;
         req.u.desc_access.arch.vmx.exit_qualification = vmx_exit_qualification;
     }
-    else
-    {
-        req.u.desc_access.arch.svm.exitinfo = exit_info;
-    }
 
     monitor_traps(current, true, &req);
 }
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 0fb1908c18..776cf11459 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2980,29 +2980,26 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
         svm_vmexit_do_pause(regs);
         break;
 
-    case VMEXIT_IDTR_READ:
-    case VMEXIT_IDTR_WRITE:
-        hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0,
-            VM_EVENT_DESC_IDTR, exit_reason == VMEXIT_IDTR_WRITE);
-        break;
-
-    case VMEXIT_GDTR_READ:
-    case VMEXIT_GDTR_WRITE:
-        hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0,
-            VM_EVENT_DESC_GDTR, exit_reason == VMEXIT_GDTR_WRITE);
-        break;
+    case VMEXIT_IDTR_READ ... VMEXIT_TR_WRITE:
+    {
+        /*
+         * Consecutive block of 8 exit codes (sadly not aligned).  Top bit
+         * indicates write (vs read), bottom 2 bits map linearly to
+         * VM_EVENT_DESC_* values.
+         */
+#define E2D(e)      ((((e)         - VMEXIT_IDTR_READ) & 3) + 1)
+        bool write = ((exit_reason - VMEXIT_IDTR_READ) & 4);
+        unsigned int desc = E2D(exit_reason);
 
-    case VMEXIT_LDTR_READ:
-    case VMEXIT_LDTR_WRITE:
-        hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0,
-            VM_EVENT_DESC_LDTR, exit_reason == VMEXIT_LDTR_WRITE);
-        break;
+        BUILD_BUG_ON(E2D(VMEXIT_IDTR_READ) != VM_EVENT_DESC_IDTR);
+        BUILD_BUG_ON(E2D(VMEXIT_GDTR_READ) != VM_EVENT_DESC_GDTR);
+        BUILD_BUG_ON(E2D(VMEXIT_LDTR_READ) != VM_EVENT_DESC_LDTR);
+        BUILD_BUG_ON(E2D(VMEXIT_TR_READ)   != VM_EVENT_DESC_TR);
+#undef E2D
 
-    case VMEXIT_TR_READ:
-    case VMEXIT_TR_WRITE:
-        hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0,
-            VM_EVENT_DESC_TR, exit_reason == VMEXIT_TR_WRITE);
+        hvm_descriptor_access_intercept(0, 0, desc, write);
         break;
+    }
 
     default:
     unexpected_exit_type:
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index 959083d8c4..d1b5c95f72 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -302,8 +302,8 @@ struct vm_event_desc_access {
             uint64_t exit_qualification; /* VMX: VMCS Exit Qualification */
         } vmx;
         struct {
-            uint64_t exitinfo;           /* SVM: VMCB EXITINFO */
-            uint64_t _pad2;
+            uint64_t exitinfo;           /* SVM: Always 0.  This field made */
+            uint64_t _pad2;              /* its way into the API by error.  */
         } svm;
     } arch;
     uint8_t descriptor;                  /* VM_EVENT_DESC_* */
-- 
2.11.0


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

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

* Re: [Xen-devel] [PATCH for-next] x86/svm: Correct vm_event API for descriptor accesses
  2019-11-28 11:44 [Xen-devel] [PATCH for-next] x86/svm: Correct vm_event API for descriptor accesses Andrew Cooper
@ 2019-11-28 13:47 ` Tamas K Lengyel
  2019-11-28 14:06 ` Razvan COJOCARU
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Tamas K Lengyel @ 2019-11-28 13:47 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Petre Pircalabu, Wei Liu, Razvan Cojocaru, Adrian Pop,
	Jan Beulich, Alexandru Isaila, Xen-devel, Roger Pau Monné

On Thu, Nov 28, 2019 at 4:44 AM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> c/s d0a699a389f1 "x86/monitor: add support for descriptor access events"
> introduced logic looking for what appeared to be exitinfo (not that this
> exists in SVM - exitinfo1 or 2 do), but actually passed the exit IDT vectoring
> information.  There is never any IDT vectoring involved in these intercepts so
> the value passed is always zero.
>
> In fact, SVM doesn't provide any information, even in exitinfo1 and 2.  Note
> the error in the public API and state that this field is always 0, and drop
> the SVM logic in hvm_monitor_descriptor_access().
>
> In the SVM vmexit handler itself, optimise the switch statement by observing
> that there is a linear transformation between the SVM exit_reason and
> VM_EVENT_DESC_* values.  (Bloat-o-meter reports 6028 => 5877 for a saving of
> 151 bytes).
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Razvan Cojocaru <rcojocaru@bitdefender.com>
> CC: Tamas K Lengyel <tamas@tklengyel.com>
> CC: Alexandru Isaila <aisaila@bitdefender.com>
> CC: Petre Pircalabu <ppircalabu@bitdefender.com>
> CC: Adrian Pop <apop@bitdefender.com>
>
> Adrian: Do you recall what information you were attempting to forward from the
> VMCB?  I can't locate anything which would plausibly be interesting.
>
> This is part of a longer cleanup series I gathered in the wake of the task
> switch issues, but it is pulled out ahead due to its interaction with the
> public interface.
> ---
>  xen/arch/x86/hvm/monitor.c    |  4 ----
>  xen/arch/x86/hvm/svm/svm.c    | 37 +++++++++++++++++--------------------
>  xen/include/public/vm_event.h |  4 ++--
>  3 files changed, 19 insertions(+), 26 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c
> index 7fb1e2c04e..1f23fe25e8 100644
> --- a/xen/arch/x86/hvm/monitor.c
> +++ b/xen/arch/x86/hvm/monitor.c
> @@ -113,10 +113,6 @@ void hvm_monitor_descriptor_access(uint64_t exit_info,
>          req.u.desc_access.arch.vmx.instr_info = exit_info;
>          req.u.desc_access.arch.vmx.exit_qualification = vmx_exit_qualification;
>      }
> -    else
> -    {
> -        req.u.desc_access.arch.svm.exitinfo = exit_info;
> -    }
>
>      monitor_traps(current, true, &req);
>  }
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 0fb1908c18..776cf11459 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2980,29 +2980,26 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>          svm_vmexit_do_pause(regs);
>          break;
>
> -    case VMEXIT_IDTR_READ:
> -    case VMEXIT_IDTR_WRITE:
> -        hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0,
> -            VM_EVENT_DESC_IDTR, exit_reason == VMEXIT_IDTR_WRITE);
> -        break;
> -
> -    case VMEXIT_GDTR_READ:
> -    case VMEXIT_GDTR_WRITE:
> -        hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0,
> -            VM_EVENT_DESC_GDTR, exit_reason == VMEXIT_GDTR_WRITE);
> -        break;
> +    case VMEXIT_IDTR_READ ... VMEXIT_TR_WRITE:
> +    {
> +        /*
> +         * Consecutive block of 8 exit codes (sadly not aligned).  Top bit
> +         * indicates write (vs read), bottom 2 bits map linearly to
> +         * VM_EVENT_DESC_* values.
> +         */
> +#define E2D(e)      ((((e)         - VMEXIT_IDTR_READ) & 3) + 1)
> +        bool write = ((exit_reason - VMEXIT_IDTR_READ) & 4);
> +        unsigned int desc = E2D(exit_reason);
>
> -    case VMEXIT_LDTR_READ:
> -    case VMEXIT_LDTR_WRITE:
> -        hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0,
> -            VM_EVENT_DESC_LDTR, exit_reason == VMEXIT_LDTR_WRITE);
> -        break;
> +        BUILD_BUG_ON(E2D(VMEXIT_IDTR_READ) != VM_EVENT_DESC_IDTR);
> +        BUILD_BUG_ON(E2D(VMEXIT_GDTR_READ) != VM_EVENT_DESC_GDTR);
> +        BUILD_BUG_ON(E2D(VMEXIT_LDTR_READ) != VM_EVENT_DESC_LDTR);
> +        BUILD_BUG_ON(E2D(VMEXIT_TR_READ)   != VM_EVENT_DESC_TR);
> +#undef E2D
>
> -    case VMEXIT_TR_READ:
> -    case VMEXIT_TR_WRITE:
> -        hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0,
> -            VM_EVENT_DESC_TR, exit_reason == VMEXIT_TR_WRITE);
> +        hvm_descriptor_access_intercept(0, 0, desc, write);
>          break;
> +    }
>
>      default:
>      unexpected_exit_type:
> diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
> index 959083d8c4..d1b5c95f72 100644
> --- a/xen/include/public/vm_event.h
> +++ b/xen/include/public/vm_event.h
> @@ -302,8 +302,8 @@ struct vm_event_desc_access {
>              uint64_t exit_qualification; /* VMX: VMCS Exit Qualification */
>          } vmx;
>          struct {
> -            uint64_t exitinfo;           /* SVM: VMCB EXITINFO */
> -            uint64_t _pad2;
> +            uint64_t exitinfo;           /* SVM: Always 0.  This field made */

There really is no point in retaining a useless field. Just remove it
and bump the event interface version. That's what it's for.

> +            uint64_t _pad2;              /* its way into the API by error.  */

Also not sure what this field is for, we just want to pad things to be
64-bit aligned. So having a 64-bit pad field makes no sense, please
also just remove it.

Thanks,
Tamas

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

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

* Re: [Xen-devel] [PATCH for-next] x86/svm: Correct vm_event API for descriptor accesses
  2019-11-28 11:44 [Xen-devel] [PATCH for-next] x86/svm: Correct vm_event API for descriptor accesses Andrew Cooper
  2019-11-28 13:47 ` Tamas K Lengyel
@ 2019-11-28 14:06 ` Razvan COJOCARU
  2019-11-28 14:46 ` Alexandru Stefan ISAILA
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Razvan COJOCARU @ 2019-11-28 14:06 UTC (permalink / raw)
  To: Andrew Cooper, Xen-devel
  Cc: Petre Ovidiu PIRCALABU, Tamas K Lengyel, Wei Liu, Adrian Pop,
	Jan Beulich, Alexandru Stefan ISAILA, Roger Pau Monné

On 11/28/19 1:44 PM, Andrew Cooper wrote:
> c/s d0a699a389f1 "x86/monitor: add support for descriptor access events"
> introduced logic looking for what appeared to be exitinfo (not that this
> exists in SVM - exitinfo1 or 2 do), but actually passed the exit IDT vectoring
> information.  There is never any IDT vectoring involved in these intercepts so
> the value passed is always zero.
> 
> In fact, SVM doesn't provide any information, even in exitinfo1 and 2.  Note
> the error in the public API and state that this field is always 0, and drop
> the SVM logic in hvm_monitor_descriptor_access().
> 
> In the SVM vmexit handler itself, optimise the switch statement by observing
> that there is a linear transformation between the SVM exit_reason and
> VM_EVENT_DESC_* values.  (Bloat-o-meter reports 6028 => 5877 for a saving of
> 151 bytes).
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Razvan Cojocaru <rcojocaru@bitdefender.com>
> CC: Tamas K Lengyel <tamas@tklengyel.com>
> CC: Alexandru Isaila <aisaila@bitdefender.com>
> CC: Petre Pircalabu <ppircalabu@bitdefender.com>
> CC: Adrian Pop <apop@bitdefender.com>
> 
> Adrian: Do you recall what information you were attempting to forward from the
> VMCB?  I can't locate anything which would plausibly be interesting.

I think it's safe to go the route you're going (you shouldn't break 
anything).

Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

(with or without addressing Tamas' comments).


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

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

* Re: [Xen-devel] [PATCH for-next] x86/svm: Correct vm_event API for descriptor accesses
  2019-11-28 11:44 [Xen-devel] [PATCH for-next] x86/svm: Correct vm_event API for descriptor accesses Andrew Cooper
  2019-11-28 13:47 ` Tamas K Lengyel
  2019-11-28 14:06 ` Razvan COJOCARU
@ 2019-11-28 14:46 ` Alexandru Stefan ISAILA
  2019-11-28 15:06 ` Adrian Pop
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-11-28 14:46 UTC (permalink / raw)
  To: Andrew Cooper, Xen-devel
  Cc: Petre Ovidiu PIRCALABU, Tamas K Lengyel, Wei Liu,
	Razvan COJOCARU, Adrian Pop, Jan Beulich, Roger Pau Monné



On 28.11.2019 13:44, Andrew Cooper wrote:
> c/s d0a699a389f1 "x86/monitor: add support for descriptor access events"
> introduced logic looking for what appeared to be exitinfo (not that this
> exists in SVM - exitinfo1 or 2 do), but actually passed the exit IDT vectoring
> information.  There is never any IDT vectoring involved in these intercepts so
> the value passed is always zero.
> 
> In fact, SVM doesn't provide any information, even in exitinfo1 and 2.  Note
> the error in the public API and state that this field is always 0, and drop
> the SVM logic in hvm_monitor_descriptor_access().
> 
> In the SVM vmexit handler itself, optimise the switch statement by observing
> that there is a linear transformation between the SVM exit_reason and
> VM_EVENT_DESC_* values.  (Bloat-o-meter reports 6028 => 5877 for a saving of
> 151 bytes).
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Alexandru Isaila <aisaila@bitdefender.com>

I agree with Tamas, good thing to have that field removed.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

* Re: [Xen-devel] [PATCH for-next] x86/svm: Correct vm_event API for descriptor accesses
  2019-11-28 11:44 [Xen-devel] [PATCH for-next] x86/svm: Correct vm_event API for descriptor accesses Andrew Cooper
                   ` (2 preceding siblings ...)
  2019-11-28 14:46 ` Alexandru Stefan ISAILA
@ 2019-11-28 15:06 ` Adrian Pop
  2019-11-28 16:39 ` Jan Beulich
  2019-11-29 17:20 ` [Xen-devel] [PATCH for-next v2] " Andrew Cooper
  5 siblings, 0 replies; 7+ messages in thread
From: Adrian Pop @ 2019-11-28 15:06 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Petre Pircalabu, Tamas K Lengyel, Wei Liu, Razvan Cojocaru,
	Jan Beulich, Alexandru Isaila, Xen-devel, Roger Pau Monné


Hello,

Andrew Cooper <andrew.cooper3@citrix.com> writes:

> c/s d0a699a389f1 "x86/monitor: add support for descriptor access events"
> introduced logic looking for what appeared to be exitinfo (not that this
> exists in SVM - exitinfo1 or 2 do), but actually passed the exit IDT vectoring
> information.  There is never any IDT vectoring involved in these intercepts so
> the value passed is always zero.
>
> In fact, SVM doesn't provide any information, even in exitinfo1 and 2.  Note
> the error in the public API and state that this field is always 0, and drop
> the SVM logic in hvm_monitor_descriptor_access().
>
> In the SVM vmexit handler itself, optimise the switch statement by observing
> that there is a linear transformation between the SVM exit_reason and
> VM_EVENT_DESC_* values.  (Bloat-o-meter reports 6028 => 5877 for a saving of
> 151 bytes).
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Razvan Cojocaru <rcojocaru@bitdefender.com>
> CC: Tamas K Lengyel <tamas@tklengyel.com>
> CC: Alexandru Isaila <aisaila@bitdefender.com>
> CC: Petre Pircalabu <ppircalabu@bitdefender.com>
> CC: Adrian Pop <apop@bitdefender.com>
>
> Adrian: Do you recall what information you were attempting to forward from the
> VMCB?  I can't locate anything which would plausibly be interesting.

The SVM part was most likely meant to mirror the logic from VMX.  But,
as I recall, monitoring hadn't been implemented with SVM so this
couldn't really be used on AMD.  Unfortunately I'm not sure what
information was supposed to be forwarded.

The cleanup looks good to me.

Acked-by: Adrian Pop <apop@bitdefender.com>

> This is part of a longer cleanup series I gathered in the wake of the task
> switch issues, but it is pulled out ahead due to its interaction with the
> public interface.

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

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

* Re: [Xen-devel] [PATCH for-next] x86/svm: Correct vm_event API for descriptor accesses
  2019-11-28 11:44 [Xen-devel] [PATCH for-next] x86/svm: Correct vm_event API for descriptor accesses Andrew Cooper
                   ` (3 preceding siblings ...)
  2019-11-28 15:06 ` Adrian Pop
@ 2019-11-28 16:39 ` Jan Beulich
  2019-11-29 17:20 ` [Xen-devel] [PATCH for-next v2] " Andrew Cooper
  5 siblings, 0 replies; 7+ messages in thread
From: Jan Beulich @ 2019-11-28 16:39 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Petre Pircalabu, Tamas K Lengyel, Wei Liu, Razvan Cojocaru,
	Adrian Pop, Alexandru Isaila, Xen-devel, Roger Pau Monné

On 28.11.2019 12:44, Andrew Cooper wrote:
> c/s d0a699a389f1 "x86/monitor: add support for descriptor access events"
> introduced logic looking for what appeared to be exitinfo (not that this
> exists in SVM - exitinfo1 or 2 do), but actually passed the exit IDT vectoring
> information.  There is never any IDT vectoring involved in these intercepts so
> the value passed is always zero.
> 
> In fact, SVM doesn't provide any information, even in exitinfo1 and 2.  Note
> the error in the public API and state that this field is always 0, and drop
> the SVM logic in hvm_monitor_descriptor_access().
> 
> In the SVM vmexit handler itself, optimise the switch statement by observing
> that there is a linear transformation between the SVM exit_reason and
> VM_EVENT_DESC_* values.  (Bloat-o-meter reports 6028 => 5877 for a saving of
> 151 bytes).
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

SVM part of the change
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

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

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

* [Xen-devel] [PATCH for-next v2] x86/svm: Correct vm_event API for descriptor accesses
  2019-11-28 11:44 [Xen-devel] [PATCH for-next] x86/svm: Correct vm_event API for descriptor accesses Andrew Cooper
                   ` (4 preceding siblings ...)
  2019-11-28 16:39 ` Jan Beulich
@ 2019-11-29 17:20 ` Andrew Cooper
  5 siblings, 0 replies; 7+ messages in thread
From: Andrew Cooper @ 2019-11-29 17:20 UTC (permalink / raw)
  To: Xen-devel
  Cc: Petre Pircalabu, Tamas K Lengyel, Wei Liu, Razvan Cojocaru,
	Adrian Pop, Andrew Cooper, Jan Beulich, Alexandru Isaila,
	Roger Pau Monné

c/s d0a699a389f1 "x86/monitor: add support for descriptor access events"
introduced logic looking for what appeared to be exitinfo (not that this
exists in SVM - exitinfo1 or 2 do), but actually passed the exit IDT vectoring
information.  There is never any IDT vectoring involved in these intercepts so
the value passed is always zero.

In fact, SVM doesn't provide any information, even in exitinfo1 and 2.  Drop
the svm struct entirely, and bump the interface version.

In the SVM vmexit handler itself, optimise the switch statement by observing
that there is a linear transformation between the SVM exit_reason and
VM_EVENT_DESC_* values.  (Bloat-o-meter reports 6028 => 5877 for a saving of
151 bytes).

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Reviewed-by: Alexandru Isaila <aisaila@bitdefender.com>
Acked-by: Adrian Pop <apop@bitdefender.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Razvan Cojocaru <rcojocaru@bitdefender.com>
CC: Tamas K Lengyel <tamas@tklengyel.com>
CC: Alexandru Isaila <aisaila@bitdefender.com>
CC: Petre Pircalabu <ppircalabu@bitdefender.com>
CC: Adrian Pop <apop@bitdefender.com>

v2:
 * Drop the svm struct and bump the interface version.
---
 xen/arch/x86/hvm/monitor.c    |  4 ----
 xen/arch/x86/hvm/svm/svm.c    | 37 +++++++++++++++++--------------------
 xen/include/public/vm_event.h |  6 +-----
 3 files changed, 18 insertions(+), 29 deletions(-)

diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c
index 7fb1e2c04e..1f23fe25e8 100644
--- a/xen/arch/x86/hvm/monitor.c
+++ b/xen/arch/x86/hvm/monitor.c
@@ -113,10 +113,6 @@ void hvm_monitor_descriptor_access(uint64_t exit_info,
         req.u.desc_access.arch.vmx.instr_info = exit_info;
         req.u.desc_access.arch.vmx.exit_qualification = vmx_exit_qualification;
     }
-    else
-    {
-        req.u.desc_access.arch.svm.exitinfo = exit_info;
-    }
 
     monitor_traps(current, true, &req);
 }
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 0fb1908c18..776cf11459 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2980,29 +2980,26 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
         svm_vmexit_do_pause(regs);
         break;
 
-    case VMEXIT_IDTR_READ:
-    case VMEXIT_IDTR_WRITE:
-        hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0,
-            VM_EVENT_DESC_IDTR, exit_reason == VMEXIT_IDTR_WRITE);
-        break;
-
-    case VMEXIT_GDTR_READ:
-    case VMEXIT_GDTR_WRITE:
-        hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0,
-            VM_EVENT_DESC_GDTR, exit_reason == VMEXIT_GDTR_WRITE);
-        break;
+    case VMEXIT_IDTR_READ ... VMEXIT_TR_WRITE:
+    {
+        /*
+         * Consecutive block of 8 exit codes (sadly not aligned).  Top bit
+         * indicates write (vs read), bottom 2 bits map linearly to
+         * VM_EVENT_DESC_* values.
+         */
+#define E2D(e)      ((((e)         - VMEXIT_IDTR_READ) & 3) + 1)
+        bool write = ((exit_reason - VMEXIT_IDTR_READ) & 4);
+        unsigned int desc = E2D(exit_reason);
 
-    case VMEXIT_LDTR_READ:
-    case VMEXIT_LDTR_WRITE:
-        hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0,
-            VM_EVENT_DESC_LDTR, exit_reason == VMEXIT_LDTR_WRITE);
-        break;
+        BUILD_BUG_ON(E2D(VMEXIT_IDTR_READ) != VM_EVENT_DESC_IDTR);
+        BUILD_BUG_ON(E2D(VMEXIT_GDTR_READ) != VM_EVENT_DESC_GDTR);
+        BUILD_BUG_ON(E2D(VMEXIT_LDTR_READ) != VM_EVENT_DESC_LDTR);
+        BUILD_BUG_ON(E2D(VMEXIT_TR_READ)   != VM_EVENT_DESC_TR);
+#undef E2D
 
-    case VMEXIT_TR_READ:
-    case VMEXIT_TR_WRITE:
-        hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0,
-            VM_EVENT_DESC_TR, exit_reason == VMEXIT_TR_WRITE);
+        hvm_descriptor_access_intercept(0, 0, desc, write);
         break;
+    }
 
     default:
     unexpected_exit_type:
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index 959083d8c4..aa54c86325 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -29,7 +29,7 @@
 
 #include "xen.h"
 
-#define VM_EVENT_INTERFACE_VERSION 0x00000005
+#define VM_EVENT_INTERFACE_VERSION 0x00000006
 
 #if defined(__XEN__) || defined(__XEN_TOOLS__)
 
@@ -301,10 +301,6 @@ struct vm_event_desc_access {
             uint32_t _pad1;
             uint64_t exit_qualification; /* VMX: VMCS Exit Qualification */
         } vmx;
-        struct {
-            uint64_t exitinfo;           /* SVM: VMCB EXITINFO */
-            uint64_t _pad2;
-        } svm;
     } arch;
     uint8_t descriptor;                  /* VM_EVENT_DESC_* */
     uint8_t is_write;
-- 
2.11.0


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

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

end of thread, other threads:[~2019-11-29 17:20 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-28 11:44 [Xen-devel] [PATCH for-next] x86/svm: Correct vm_event API for descriptor accesses Andrew Cooper
2019-11-28 13:47 ` Tamas K Lengyel
2019-11-28 14:06 ` Razvan COJOCARU
2019-11-28 14:46 ` Alexandru Stefan ISAILA
2019-11-28 15:06 ` Adrian Pop
2019-11-28 16:39 ` Jan Beulich
2019-11-29 17:20 ` [Xen-devel] [PATCH for-next v2] " Andrew Cooper

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.