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

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 05/20/2015 06:48 PM, Jan Beulich wrote:
>>>> On 20.05.15 at 17:24, <rcojocaru@bitdefender.com> wrote:
>> On 05/20/2015 05:53 PM, Jan Beulich wrote:
>>>>>> On 19.05.15 at 10:31, <rcojocaru@bitdefender.com> 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".

Ack, will change it 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.

Understood, will withhold the reordering of control register vm_events
in V4.

>>>> +/* 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.

I see. They're bitmasks because it makes it easy to use them with the
X-bit wide (where X is the number of control register event indices)
flags write_ctrlreg_enabled, write_ctrlreg_sync and
write_ctrlreg_onchangeonly. It makes no difference for the .index field
of the actual event, you are right indeed that it would not make sense
for .index to consist of OR-ed bit masks.

I can change them to 0, 1, 2, and so on, and use individual single-bit
bitfields for write_ctrlreg_enabled & friends, but this way makes it
trivial to add a new control register vm_event: just use 1 << next
available position and widen the proper domain bitfields by one, and the
new vm_event is ready to use.


Thanks,
Razvan