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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 03/03/2016 08:04 PM, Corneliu ZUZU wrote:
> 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?

Yes, I think that's the path of least resistance.

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

Yes, that's the goal.

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

Sounds good, except perhaps for the function name but I'm not sure what
a better one might be unfortunately, maybe someone else with chime in
with a suggestion.


Thanks,
Razvan


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