[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



>>> On 04.07.16 at 15:03, <czuzu@xxxxxxxxxxxxxxx> wrote:
> 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.

The monitoring app can cause all kinds of problems to the guest it
monitors. Why would this specific one need taking care of in the
hypervisor, instead of demanding that the app not disable monitoring
at the wrong time?

>>> --- 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.

Since the initializer is (very obviously) independent of the
condition the ASSERT() checks, I indeed would prefer it to remain
the way it is before this change.

>>> -            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? :-)

I've counted 81 here.

Jan


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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.