Re: [Xen-devel] [PATCH] xen/x86: Clean up vm_event-related code in asm-x86/domain.h

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.08.15 at 11:54, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> @@ -571,7 +571,7 @@ void hvm_do_resume(struct vcpu *v)
>      }
>  
>      /* Inject pending hw/sw trap */
> -    if ( v->arch.hvm_vcpu.inject_trap.vector != -1 ) 
> +    if ( v->arch.hvm_vcpu.inject_trap.vector != -1 )

Stray whitespace change to unrelated code.

> @@ -3371,13 +3371,13 @@ int hvm_set_cr0(unsigned long value, bool_t 
> may_defer)
>                                 monitor_ctrlreg_bitmask(VM_EVENT_X86_CR0)) &&
>           value != old_value )
>      {
> -        ASSERT(currad->event_write_data != NULL);
> +        ASSERT(v->arch.vm_event != NULL);

May I recommend dropping the redundant != NULL namely in ASSERT()
expressions (the only effect they have is produce larger string literals
in debug builds).

>          if ( hvm_event_crX(CR0, value, old_value) )
>          {
>              /* The actual write will occur in hvm_do_resume(), if permitted. 
> */
> -            currad->event_write_data[v->vcpu_id].do_write.cr0 = 1;
> -            currad->event_write_data[v->vcpu_id].cr0 = value;
> +            v->arch.vm_event->write_data.do_write.cr0 = 1;
> +            v->arch.vm_event->write_data.cr0 = value;

Looks like this leaves just a single use of currad in the function, in
which case I'd like to see the variable go away.

> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -8,6 +8,7 @@
>  #include <asm/hvm/domain.h>
>  #include <asm/e820.h>
>  #include <asm/mce.h>
> +#include <public/vm_event.h>

It looks like both this and ...

> @@ -460,6 +459,18 @@ typedef enum __packed {
>      SMAP_CHECK_DISABLED,        /* disable the check */
>  } smap_check_policy_t;
>  
> +/*
> + * Should we emulate the next matching instruction on VCPU resume
> + * after a vm_event?
> + */
> +struct vm_event {
> +    uint32_t emulate_flags;
> +    unsigned long gpa;
> +    unsigned long eip;
> +    struct vm_event_emul_read_data emul_read_data;
> +    struct monitor_write_data write_data;
> +};

... this would better go into asm-x86/vm_event.h (despite it meaning
that the file will then be included by basically everything).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel