Re: [Xen-devel] [PATCH for-next] x86/svm: Correct vm_event API for descriptor accesses

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Thu, Nov 28, 2019 at 4:44 AM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
> CC: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
> CC: Alexandru Isaila <aisaila@xxxxxxxxxxxxxxx>
> CC: Petre Pircalabu <ppircalabu@xxxxxxxxxxxxxxx>
> CC: Adrian Pop <apop@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel