Re: [PATCH V5 06/12] x86/hvm: factor out and rename vm_event related functions

[Tamas K Lengyel <tamas.lengyel@zentific.com>]

On Tue, Feb 17, 2015 at 12:56 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 13.02.15 at 17:33, <tamas.lengyel@zentific.com> wrote:
>> +static void hvm_event_cr(uint32_t reason, unsigned long value,
>> +                                unsigned long old)
>> +{
>> +    vm_event_request_t req = {
>> +        .reason = reason,
>> +        .vcpu_id = current->vcpu_id,
>> +        .u.mov_to_cr.new_value = value,
>> +        .u.mov_to_cr.old_value = old
>> +    };
>> +    uint64_t parameters = 0;
>> +
>> +    switch(reason)
>
> Coding style. Also I continue to think using switch() here rather than
> having the caller pass both VM_EVENT_* and HVM_PARAM_* is ugly/
> inefficient (even if the compiler may be able to sort this out for you).

It's getting retired in the series so there isn't much point in
tweaking it here.

>> +    {
>> +    case VM_EVENT_REASON_MOV_TO_CR0:
>> +        parameters = current->domain->arch.hvm_domain
>> +                      .params[HVM_PARAM_MEMORY_EVENT_CR0];
>> +        break;
>> +    case VM_EVENT_REASON_MOV_TO_CR3:
>> +        parameters = current->domain->arch.hvm_domain
>> +                      .params[HVM_PARAM_MEMORY_EVENT_CR3];
>> +        break;
>> +    case VM_EVENT_REASON_MOV_TO_CR4:
>> +        parameters = current->domain->arch.hvm_domain
>> +                      .params[HVM_PARAM_MEMORY_EVENT_CR4];
>> +        break;
>> +    };
>
> In any event, if you stay with the current model, latch current
> (used four times) into local variable.

Ack.

>
>> +void hvm_event_msr(unsigned int msr, uint64_t value)
>> +{
>> +    struct vcpu *curr = current;
>> +    vm_event_request_t req = {
>> +        .reason = VM_EVENT_REASON_MOV_TO_MSR,
>> +        .vcpu_id = curr->vcpu_id,
>> +        .u.mov_to_msr.msr = msr,
>> +        .u.mov_to_msr.value = value,
>> +    };
>> +    uint64_t params = current->domain->arch.hvm_domain
>
> Why "current" when you have "curr"?

Just forgot to update it.

>
> Jan

Thanks,
Tamas