Re: [Xen-devel] [PATCH v1] x86/hvm: Add MSR old value

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.10.17 at 11:10, <aisaila@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/monitor.c
> +++ b/xen/arch/x86/hvm/monitor.c
> @@ -74,16 +74,19 @@ bool hvm_monitor_emul_unimplemented(void)
>          monitor_traps(curr, true, &req) == 1;
>  }
>  
> -void hvm_monitor_msr(unsigned int msr, uint64_t value)
> +void hvm_monitor_msr(unsigned int msr, uint64_t new_value, uint64_t 
> old_value)
>  {
>      struct vcpu *curr = current;
>  
> -    if ( monitored_msr(curr->domain, msr) )
> +    if ( monitored_msr(curr->domain, msr) &&
> +         ( !monitored_msr_onchangeonly(curr->domain, msr) ||
> +           new_value != old_value ) )

Stray blanks inside the inner parentheses.

> @@ -84,6 +84,11 @@ static int monitor_enable_msr(struct domain *d, u32 msr)
>  
>      hvm_enable_msr_interception(d, msr);
>  
> +    if( onchangeonly )

Style.

> +        __set_bit(index + sizeof(struct monitor_msr_bitmap), bitmap);

I think you miss "* 8" here - a bit position plus sizeof() doesn't
produce any useful value.

But what's worse - having read till the end of the patch I don't
see you change any allocation, yet you clearly need to double
the space now that you need two bits per MSR.

> --- a/xen/include/asm-x86/monitor.h
> +++ b/xen/include/asm-x86/monitor.h
> @@ -105,4 +105,6 @@ void arch_monitor_cleanup_domain(struct domain *d);
>  
>  bool monitored_msr(const struct domain *d, u32 msr);
>  
> +bool monitored_msr_onchangeonly(const struct domain *d, u32 msr);
> +

Them belonging together, please have them together (without an
intervening blank line).

Jan


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