Re: [Xen-devel] [PATCH 5/8] x86/vm-event/monitor: don't compromise monitor_write_data on domain cleanup

[Corneliu ZUZU <czuzu@xxxxxxxxxxxxxxx>]

On 7/4/2016 3:47 PM, Jan Beulich wrote:
> > > > On 30.06.16 at 20:45, <czuzu@xxxxxxxxxxxxxxx> wrote:
> > The arch_vm_event structure is dynamically allocated and freed @
> > vm_event_cleanup_domain. This cleanup is triggered e.g. when the toolstack user
> > disables domain monitoring (xc_monitor_disable), which in turn effectively
> > discards any information that was in arch_vm_event.write_data.
> Isn't that rather a toolstack user bug, not warranting a relatively
> extensive (even if mostly mechanical) hypervisor change like this
> one? Sane monitor behavior, after all, is required anyway for the
> monitored guest to survive.
Sorry but could you please rephrase this, I don't quite understand what 
you're saying.
The write_data field in arch_vm_event should _not ever_ be invalidated 
as a direct result of a toolstack user's action.
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -473,24 +473,24 @@ void hvm_do_resume(struct vcpu *v)
> >       if ( !handle_hvm_io_completion(v) )
> >           return;
> >   
> > -    if ( unlikely(v->arch.vm_event) )
> > +    if ( unlikely(v->arch.vm_event.emulate_flags) )
> >       {
> > -        if ( v->arch.vm_event->emulate_flags )
> > -        {
> > -            enum emul_kind kind = EMUL_KIND_NORMAL;
> > +        enum emul_kind kind;
> >   
> > -            if ( v->arch.vm_event->emulate_flags &
> > -                 VM_EVENT_FLAG_SET_EMUL_READ_DATA )
> > -                kind = EMUL_KIND_SET_CONTEXT;
> > -            else if ( v->arch.vm_event->emulate_flags &
> > -                      VM_EVENT_FLAG_EMULATE_NOWRITE )
> > -                kind = EMUL_KIND_NOWRITE;
> > +        ASSERT(v->arch.vm_event.emul_read_data);
> >   
> > -            hvm_mem_access_emulate_one(kind, TRAP_invalid_op,
> > -                                       HVM_DELIVER_NO_ERROR_CODE);
> > +        kind = EMUL_KIND_NORMAL;
> Please keep this being the initializer of the variable.
I put it there because of the ASSERT (to do that before anything else), 
but I will undo if you prefer.
> >   
> > -            v->arch.vm_event->emulate_flags = 0;
> > -        }
> > +        if ( v->arch.vm_event.emulate_flags & VM_EVENT_FLAG_SET_EMUL_READ_DATA 
> > )
> Long line.
Long but under 80 columns, isn't that the rule? :-)

> > --- a/xen/include/asm-x86/domain.h
> > +++ b/xen/include/asm-x86/domain.h
> > @@ -259,21 +259,6 @@ struct pv_domain
> >       struct cpuidmasks *cpuidmasks;
> >   };
> >   
> > -enum monitor_write_status
> > -{
> > -    MWS_NOWRITE = 0,
> > -    MWS_MSR,
> > -    MWS_CR0,
> > -    MWS_CR3,
> > -    MWS_CR4,
> > -};
> > -
> > -struct monitor_write_data {
> > -    enum monitor_write_status status;
> > -    uint32_t msr;
> > -    uint64_t value;
> > -};
> > -
> >   struct arch_domain
> >   {
> >       struct page_info *perdomain_l3_pg;
> > @@ -496,6 +481,31 @@ typedef enum __packed {
> >       SMAP_CHECK_DISABLED,        /* disable the check */
> >   } smap_check_policy_t;
> >   
> > +enum monitor_write_status
> > +{
> > +    MWS_NOWRITE = 0,
> > +    MWS_MSR,
> > +    MWS_CR0,
> > +    MWS_CR3,
> > +    MWS_CR4,
> > +};
> > +
> > +struct monitor_write_data {
> > +    enum monitor_write_status status;
> > +    uint32_t msr;
> > +    uint64_t value;
> > +};
> Instead of moving these around now, may I suggest you put them
> into their final place right away in the previous patch?
>
> Jan
Sounds good, will do.

Corneliu.

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