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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.05.15 at 17:24, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> On 05/20/2015 05:53 PM, Jan Beulich wrote:
>>>>> On 19.05.15 at 10:31, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>> --- 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?
> 
> As opposed to an uint8_t? This would allow the hvm_event_cr() signature
> to remain unmodified longer if more than 4 control register events are
> added in the future, and it is the least wasteful of the
> bigger-than-char-sized integers.

No, opposed to "unsigned int".

>>> @@ -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)?
> 
> Consistency. Since the patch concerns itself with cleaning up the
> control register events a bit, it didn't seem too far-fetched to assume
> that having the control register write vm_events sent at the same place
> as the MSR events fits the concept.
> 
> But I am happy to do this in the DENY patch in the next iteration of the
> series if so requested.

In small patches I don't mind multiple things to be done together.
But large patches should focus on one thing at a time.

>>> +/* 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?
> 
> Ack, will change it to an enum. That would have been my first preference
> too, but the header just seemed to be more #define-oriented and I tried
> to follow suit.

And I didn't say use an enum, I intentionally said enum like.

Jan


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