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

[Corneliu ZUZU <czuzu@bitdefender.com>]

On 3/3/2016 8:51 PM, Razvan Cojocaru wrote:
> 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
>
>

I was thinking of the option of giving this function the significance of 
it being called by the
scheduler tail, i.e. just before "entering" a vcpu, in which case we 
could use a name
like vm_event_schedtail.
To me this sounds pretty good since it generalizes the function and it 
makes sense to have
a vm_event_* function that is called just before a vcpu is scheduled, 
i.e. a vm-events function
that would add-in a "final touch" before the "final stage" of actually 
entering the vcpu.

Let me know what you think.

Thanks,
Corneliu.

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