Re: [Xen-devel] [PATCH 5/8] x86/vm-event/monitor: don't compromise monitor_write_data on domain cleanup

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.06.16 at 20:45, <czuzu@xxxxxxxxxxxxxxx> wrote:
> The arch_vm_event structure is dynamically allocated and freed @
> vm_event_cleanup_domain. This cleanup is triggered e.g. when the toolstack 
> user
> disables domain monitoring (xc_monitor_disable), which in turn effectively
> discards any information that was in arch_vm_event.write_data.

Isn't that rather a toolstack user bug, not warranting a relatively
extensive (even if mostly mechanical) hypervisor change like this
one? Sane monitor behavior, after all, is required anyway for the
monitored guest to survive.

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -473,24 +473,24 @@ void hvm_do_resume(struct vcpu *v)
>      if ( !handle_hvm_io_completion(v) )
>          return;
>  
> -    if ( unlikely(v->arch.vm_event) )
> +    if ( unlikely(v->arch.vm_event.emulate_flags) )
>      {
> -        if ( v->arch.vm_event->emulate_flags )
> -        {
> -            enum emul_kind kind = EMUL_KIND_NORMAL;
> +        enum emul_kind kind;
>  
> -            if ( v->arch.vm_event->emulate_flags &
> -                 VM_EVENT_FLAG_SET_EMUL_READ_DATA )
> -                kind = EMUL_KIND_SET_CONTEXT;
> -            else if ( v->arch.vm_event->emulate_flags &
> -                      VM_EVENT_FLAG_EMULATE_NOWRITE )
> -                kind = EMUL_KIND_NOWRITE;
> +        ASSERT(v->arch.vm_event.emul_read_data);
>  
> -            hvm_mem_access_emulate_one(kind, TRAP_invalid_op,
> -                                       HVM_DELIVER_NO_ERROR_CODE);
> +        kind = EMUL_KIND_NORMAL;

Please keep this being the initializer of the variable.

>  
> -            v->arch.vm_event->emulate_flags = 0;
> -        }
> +        if ( v->arch.vm_event.emulate_flags & 
> VM_EVENT_FLAG_SET_EMUL_READ_DATA )

Long line.

> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -259,21 +259,6 @@ struct pv_domain
>      struct cpuidmasks *cpuidmasks;
>  };
>  
> -enum monitor_write_status
> -{
> -    MWS_NOWRITE = 0,
> -    MWS_MSR,
> -    MWS_CR0,
> -    MWS_CR3,
> -    MWS_CR4,
> -};
> -
> -struct monitor_write_data {
> -    enum monitor_write_status status;
> -    uint32_t msr;
> -    uint64_t value;
> -};
> -
>  struct arch_domain
>  {
>      struct page_info *perdomain_l3_pg;
> @@ -496,6 +481,31 @@ typedef enum __packed {
>      SMAP_CHECK_DISABLED,        /* disable the check */
>  } smap_check_policy_t;
>  
> +enum monitor_write_status
> +{
> +    MWS_NOWRITE = 0,
> +    MWS_MSR,
> +    MWS_CR0,
> +    MWS_CR3,
> +    MWS_CR4,
> +};
> +
> +struct monitor_write_data {
> +    enum monitor_write_status status;
> +    uint32_t msr;
> +    uint64_t value;
> +};

Instead of moving these around now, may I suggest you put them
into their final place right away in the previous patch?

Jan


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