Re: [PATCH 0/1] ARM: Implement support for write-ctrlreg vm-events

[Corneliu ZUZU <czuzu@bitdefender.com>]

On 3/3/2016 6:15 PM, Razvan Cojocaru wrote:
> On 03/03/2016 04:10 PM, Corneliu ZUZU wrote:
>> Q2) About VM_EVENT_FLAG_DENY
>>
>>    Q2.1)
>>      Doesn't it require sync = 1 (i.e. the vcpu to be paused) to work?
>>      If so, shouldn't we call vm_event_register_write_resume only after checking
>>      that VM_EVENT_FLAG_VCPU_PAUSED flag is set (vm_event_resume). Moreover, if
>>      we do that, wouldn't it be 'cleaner' to rename
>>      vm_event_register_write_resume->vm_event_deny, check for the
>>      VM_EVENT_FLAG_DENY flag in vm_event_resume instead and call vm_event_deny
>>      from there after this check?
> Yes, it does require the VCPU to be paused to work, and yes, it's a good
> idea to check that that flag is set in the response.
>
> Beyond that, I'd prefer we keep vm_event_register_write_resume()
> because, while today all we do there is check that VM_EVENT_FLAG_DENY is
> set, we might want do to other things as well there as well (for
> example, maybe validate the content of some register). That was really
> the point of the "bigger-named" function.
>
> But that's just my opinion, if Tamas prefers your rename suggestion I'll
> consider myself outnumbered.

Oh, ok, then the check for VM_EVENT_FLAG_VCPU_PAUSED would be done in 
vm_event_register_write_resume instead of vm_event_resume,
since I suppose vm_event_register_write_resume shouldn't be called only 
when the vcpu is paused, we only apply that to the DENY flag, correct?

>>    Q2.2)
>>      VM_EVENT_FLAG_DENY functionality is not implemented w/ this change-set.
>>      What is done instead is that the control-register write is done *before*
>>      sending the vm-event (vm_event_put_request). This way, the user can
>>      override the written register value after receiving the vm-event, which
>>      in effect provides the same flexibility as VM_EVENT_FLAG_DENY does.
>>      Using this strategy instead would simplify both Xen's code and the libxc
>>      user's code.
>>      Thoughts?
> That's how I initially did it with CR events, but with an application
> dealing with huge numbers of events, an extra hypercall (to re-set the
> register) can be quite expensive, so I had to rework it to the present
> state. On these grounds I'm opposed to it - and for consistency I would
> prefer that all register write events are pre-write events, and deniable
> with a single vm_event reply.
>

Ah, I understand. I figured the utility of the DENY flag was only for 
cases where you'd want to actively
override the value written to the register (set register to overridden 
value + resume w/ DENY), instead
of just forbidding the write and leaving the register untouched (i.e. 
set on the old value).
If the latter is a  desirable functionality then it indeed makes sense 
to have a DENY flag.

With that said, another thing crossed my mind. Since the DENY flag will 
be implemented for ARM w/ the next
revision and the actual write will be done on the scheduler tail, 
similarly to X86 ((hvm_do_resume), wouldn't
it be good if we separated the code that checks monitor_write_data from 
there into an arch-dependent function,
e.g. vm_event_monitor_write_data? That way the scheduler tail function 
won't be 'polluted' w/ that code and IMHO
it will make the vm-events design more clear (since that functionality 
will also be in vm_event.c along w/ the other
vm_event_* functions).

> HTH,
> Razvan
>

Thanks,
Corneliu.

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