Re: [Xen-devel] [PATCH V3] xen/vm_event: Clean up control-register-write vm_events

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.05.15 at 10:31, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> The CR0, CR3 and CR4 events are
> now pre-write vm_events.

I didn't get the impression you and Tamas had already settled on
whether this is the way to go forward.

> --- a/xen/arch/x86/hvm/event.c
> +++ b/xen/arch/x86/hvm/event.c
> @@ -88,55 +88,29 @@ static int hvm_event_traps(uint8_t sync, 
> vm_event_request_t *req)
>      return 1;
>  }
>  
> -static inline
> -void hvm_event_cr(uint32_t reason, unsigned long value,
> -                         unsigned long old, bool_t onchangeonly, bool_t sync)
> +void hvm_event_cr(unsigned short index, unsigned long value, unsigned long 
> old)

What's the point of using "unsigned short" here?

>  {
> -    if ( onchangeonly && value == old )
> +    struct arch_domain *currad = &current->domain->arch;
> +
> +    if ( !(currad->monitor.write_ctrlreg_enabled & index) )
> +        return;
> +
> +    if ( (currad->monitor.write_ctrlreg_onchangeonly & index) && value == 
> old )
>          return;
>      else
>      {

Considering that nothing follows this else block, please invert the
condition and avoid both return and else.

> @@ -3201,6 +3204,8 @@ int hvm_set_cr0(unsigned long value)
>          goto gpf;
>      }
>  
> +    hvm_event_cr(VM_EVENT_X86_CR0, value, old_value);
> +
>      if ( (value & X86_CR0_PG) && !(old_value & X86_CR0_PG) )
>      {
>          if ( v->arch.hvm_vcpu.guest_efer & EFER_LME )

If the monitor's response isn't being checked (i.e. "deny" not [yet]
being honored), what's the point of moving the generation of the
event in this patch (which would be involved enough without these
extra adjustments)?

> @@ -3287,7 +3291,9 @@ int hvm_set_cr3(unsigned long value)
>  {
>      struct vcpu *v = current;
>      struct page_info *page;
> -    unsigned long old;
> +    unsigned long old = v->arch.hvm_vcpu.guest_cr[3];
> +
> +    hvm_event_cr(VM_EVENT_X86_CR3, value, old);

I don't think the local variable is warranted anymore with the moved
event generation point.

> @@ -2010,7 +2010,7 @@ static int vmx_cr_access(unsigned long 
> exit_qualification)
>          unsigned long old = curr->arch.hvm_vcpu.guest_cr[0];
>          curr->arch.hvm_vcpu.guest_cr[0] &= ~X86_CR0_TS;
>          vmx_update_guest_cr(curr, 0);
> -        hvm_event_cr0(curr->arch.hvm_vcpu.guest_cr[0], old);
> +        hvm_event_cr(VM_EVENT_X86_CR0, curr->arch.hvm_vcpu.guest_cr[0], old);

Why is this not becoming a pre event?

> +/* Supported values for the vm_event_write_ctrlreg index. */
> +#define VM_EVENT_X86_CR0     (1 << 0)
> +#define VM_EVENT_X86_CR3     (1 << 1)
> +#define VM_EVENT_X86_CR4     (1 << 2)
> +#define VM_EVENT_X86_XCR0    (1 << 3)

Why bit masks rather than an enumeration like thing?

> @@ -156,7 +158,8 @@ struct vm_event_mem_access {
>      uint32_t _pad;
>  };
>  
> -struct vm_event_mov_to_cr {
> +struct vm_event_write_ctrlreg {
> +    uint64_t index;

In particular here - what meaning would it have if there was more
than one bit set?

Jan


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