Re: [Xen-devel] [PATCH 3/8] x86/vm-event/monitor: relocate code-motion more appropriately

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 07/04/16 13:22, Jan Beulich wrote:
>>>> On 30.06.16 at 20:43, <czuzu@xxxxxxxxxxxxxxx> wrote:
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -475,8 +475,6 @@ void hvm_do_resume(struct vcpu *v)
>>  
>>      if ( unlikely(v->arch.vm_event) )
>>      {
>> -        struct monitor_write_data *w = &v->arch.vm_event->write_data;
>> -
>>          if ( v->arch.vm_event->emulate_flags )
>>          {
>>              enum emul_kind kind = EMUL_KIND_NORMAL;
>> @@ -493,32 +491,10 @@ void hvm_do_resume(struct vcpu *v)
>>  
>>              v->arch.vm_event->emulate_flags = 0;
>>          }
>> -
>> -        if ( w->do_write.msr )
>> -        {
>> -            hvm_msr_write_intercept(w->msr, w->value, 0);
>> -            w->do_write.msr = 0;
>> -        }
>> -
>> -        if ( w->do_write.cr0 )
>> -        {
>> -            hvm_set_cr0(w->cr0, 0);
>> -            w->do_write.cr0 = 0;
>> -        }
>> -
>> -        if ( w->do_write.cr4 )
>> -        {
>> -            hvm_set_cr4(w->cr4, 0);
>> -            w->do_write.cr4 = 0;
>> -        }
>> -
>> -        if ( w->do_write.cr3 )
>> -        {
>> -            hvm_set_cr3(w->cr3, 0);
>> -            w->do_write.cr3 = 0;
>> -        }
>>      }
>>  
>> +    arch_monitor_write_data(v);
> 
> Why does this get moved outside the if(), with the same condition
> getting added inside the function (inverted for bailing early)?
> 
>> @@ -119,6 +156,55 @@ bool_t monitored_msr(const struct domain *d, u32 msr)
>>      return test_bit(msr, bitmap);
>>  }
>>  
>> +static void write_ctrlreg_adjust_traps(struct domain *d)
>> +{
>> +    struct vcpu *v;
>> +    struct arch_vmx_struct *avmx;
>> +    unsigned int cr3_bitmask;
>> +    bool_t cr3_vmevent, cr3_ldexit;
>> +
>> +    /* Adjust CR3 load-exiting. */
>> +
>> +    /* vmx only */
>> +    ASSERT(cpu_has_vmx);
>> +
>> +    /* non-hap domains trap CR3 writes unconditionally */
>> +    if ( !paging_mode_hap(d) )
>> +    {
>> +        for_each_vcpu ( d, v )
>> +            ASSERT(v->arch.hvm_vmx.exec_control & 
>> CPU_BASED_CR3_LOAD_EXITING);
>> +        return;
>> +    }
>> +
>> +    cr3_bitmask = monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3);
>> +    cr3_vmevent = !!(d->arch.monitor.write_ctrlreg_enabled & cr3_bitmask);
>> +
>> +    for_each_vcpu ( d, v )
>> +    {
>> +        avmx = &v->arch.hvm_vmx;
>> +        cr3_ldexit = !!(avmx->exec_control & CPU_BASED_CR3_LOAD_EXITING);
>> +
>> +        if ( cr3_vmevent == cr3_ldexit )
>> +            continue;
>> +
>> +        /*
>> +         * If CR0.PE=0, CR3 load exiting must remain enabled.
>> +         * See vmx_update_guest_cr code motion for cr = 0.
>> +         */
>> +        if ( cr3_ldexit && !hvm_paging_enabled(v) && 
>> !vmx_unrestricted_guest(v) 
>> )
>> +            continue;
>> +
>> +        if ( cr3_vmevent )
>> +            avmx->exec_control |= CPU_BASED_CR3_LOAD_EXITING;
>> +        else
>> +            avmx->exec_control &= ~CPU_BASED_CR3_LOAD_EXITING;
>> +
>> +        vmx_vmcs_enter(v);
>> +        vmx_update_cpu_exec_control(v);
>> +        vmx_vmcs_exit(v);
>> +    }
>> +}
> 
> While Razvan gave his ack already, I wonder whether it's really a
> good idea to put deeply VMX-specific code outside of a VMX-specific
> file.

Didn't I add "for monitor / vm_event parts Acked-by: ..."? If I didn't,
I meant to. Obviously VMX code maintainers outrank me on these issues.


Thanks,
Razvan

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