Re: [Xen-devel] [PATCH V3] x86/monitor: Disallow setting mem_access_emulate_each_rep when vm_event is NULL

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Tue, May 3, 2016 at 2:18 AM, Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx> wrote:

On 05/03/2016 11:14 AM, Jan Beulich wrote:
>>>> On 29.04.16 at 18:12, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> On 04/09/16 08:54, Razvan Cojocaru wrote:
>>> It is meaningless (and potentially dangerous - see hvmemul_virtual_to_linear())
>>> to set mem_access_emulate_each_rep before xc_monitor_enable() (which allocates
>>> vcpu->arch.vm_event) has been called, so return an error from the
>>> XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP hypercall when that is the case.
>>>
>>> Signed-off-by: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
>>> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>
>>> ---
>>> Changes since V2:
>>>  - Updated the if() condition as recommended by Andrew Cooper.
>>>  - Added Andrew Cooper's Reviewed-by.
>>> ---
>>>  xen/include/asm-x86/monitor.h | 16 +++++++++++++---
>>>  1 file changed, 13 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
>>> index 0954b59..d367099 100644
>>> --- a/xen/include/asm-x86/monitor.h
>>> +++ b/xen/include/asm-x86/monitor.h
>>> @@ -32,19 +32,29 @@
>>>  static inline
>>>  int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
>>>  {
>>> +    int rc = 0;
>>> +
>>>      switch ( mop->op )
>>>      {
>>>      case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP:
>>>          domain_pause(d);
>>> -        d->arch.mem_access_emulate_each_rep = !!mop->event;
>>> +        /*
>>> +         * Enabling mem_access_emulate_each_rep without a vm_event subscriber
>>> +         * is meaningless.
>>> +         */
>>> +        if ( d->max_vcpus && d->vcpu[0] && d->vcpu[0]->arch.vm_event )
>>> +            d->arch.mem_access_emulate_each_rep = !!mop->event;
>>> +        else
>>> +            rc = -EINVAL;
>>> +
>>>          domain_unpause(d);
>>>          break;
>>>
>>>      default:
>>> -        return -EOPNOTSUPP;
>>> +        rc = -EOPNOTSUPP;
>>>      }
>>>
>>> -    return 0;
>>> +    return rc;
>>>  }
>>>
>>>  int arch_monitor_domctl_event(struct domain *d,
>>
>> According to the previous list discussion with Andrew Cooper, this fix
>> might be considered for the 4.7 release, so CC-ing Wei for a release
>> ack, as suggested.
>
> Even if - without the pending ./MAINTAINERS adjustment - not
> formally required, I don't understand why you didn't Cc Tamas on
> this patch. I don't think this should go in without his ack.

Of course, I was under the impression that he was in the recipients list
(I let scripts/maintaners.pl do the work and didn't pay much attention
to its output).

By all means.

The maintainers file wasn't covering this header properly. Fixed in my other patch-set.

Acked-by: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>

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