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

[Corneliu ZUZU <czuzu@xxxxxxxxxxxxxxx>]

Hi Jan,

On 7/4/2016 1:22 PM, 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)?
I left that so because of patch 5/8 - specifically, monitor_write_data 
handling shouldn't depend on the vm_event subsystem being initialized.
But you're right, it still does depend on that initialization in this 
patch, so I should leave the call inside the if (and remove the check 
inside the function) as you suggest and only get it out in 5/8.
Will do that in v2.

> > @@ -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.
>
> Jan
Well, a summary of what this function does would sound like: "adjusts 
CR3 load-exiting for cr-write monitor vm-events". IMHO that's (monitor) 
vm-event specific enough to be placed within the vm-event subsystem.
Could you suggest concretely how this separation would look like? (where 
to put this function/parts of it (and what parts), what name should it 
have once moved). Another reason this was done (besides avoiding 
hackishly doing a CR0 update when we actually need a CR3 update 
specifically for a vm-event to happen) is keeping symmetry between 
ARM<->X86 in a future patch that would implement monitor CR vm-events 
for ARM. In that patch write_ctrlreg_adjust_traps is renamed and 
implemented per-architecture, on ARM it would have the same job, i.e. 
updating some hypervisor traps (~ vmx execution controls) for CR 
vm-events to happen.
On a different note, one thing I forgot to do though is to also move the 
following check (instead of completely removing it from 
arch_monitor_domctl_event):
        if ( VM_EVENT_X86_CR3 == mop->u.mov_to_cr.index )

inside write_ctrlreg_adjust_traps. Will remedy that in v2.

Thanks,
Corneliu.

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