Re: [PATCH 1/3] xen/monitor: Control register values

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.05.2020 15:42, Tamas K Lengyel wrote:
> On Wed, May 20, 2020 at 7:36 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> On 15.05.2020 18:53, Tamas K Lengyel wrote:
>>> Extend the monitor_op domctl to include option that enables
>>> controlling what values certain registers are permitted to hold
>>> by a monitor subscriber.
>>
>> This needs a bit more explanation, especially for those of us
>> who aren't that introspection savvy. For example, from the text
>> here I didn't expect a simple bool control, but something where
>> actual (register) values get passed back and forth.
>>
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -2263,9 +2263,10 @@ int hvm_set_cr0(unsigned long value, bool may_defer)
>>>      {
>>>          ASSERT(v->arch.vm_event);
>>>
>>> -        if ( hvm_monitor_crX(CR0, value, old_value) )
>>> +        if ( hvm_monitor_crX(CR0, value, old_value) &&
>>> +             v->domain->arch.monitor.control_register_values )
>>>          {
>>> -            /* The actual write will occur in hvm_do_resume(), if 
>>> permitted. */
>>> +            /* The actual write will occur in hvm_do_resume, if permitted. 
>>> */
>>
>> Please can you leave alone this and the similar comments below.
>> And for consistency _add_ parentheses to the one new instance
>> you add?
> 
> I changed to because now it doesn't fit into the 80-line limit below,
> and then changed it everywhere _for_ consistency.

The 80-char limit is easy to deal with - wrap the line.

>>> --- a/xen/arch/x86/monitor.c
>>> +++ b/xen/arch/x86/monitor.c
>>> @@ -144,7 +144,15 @@ int arch_monitor_domctl_event(struct domain *d,
>>>                                struct xen_domctl_monitor_op *mop)
>>>  {
>>>      struct arch_domain *ad = &d->arch;
>>> -    bool requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op);
>>> +    bool requested_status;
>>> +
>>> +    if ( XEN_DOMCTL_MONITOR_OP_CONTROL_REGISTERS == mop->op )
>>> +    {
>>> +        ad->monitor.control_register_values = true;
>>
>> And there's no way to clear this flag again?
> 
> There is. Disable the monitor vm_event interface and reinitialize.

Quite heavy handed, isn't it?

Jan