Re: [Xen-devel] [PATCH v3 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.02.16 at 13:14, <czuzu@xxxxxxxxxxxxxxx> wrote:
> On 2/15/2016 1:41 PM, Jan Beulich wrote:
>>>>> On 15.02.16 at 07:37, <czuzu@xxxxxxxxxxxxxxx> wrote:
>>>       default:
>>> -        return -EOPNOTSUPP;
>>> +        /*
>>> +         * Should not be reached unless arch_monitor_get_capabilities() is 
>>> not
>>> +         * properly implemented. In that case, since reaching this point 
>>> does
>>> +         * not really break anything, don't crash the hypervisor, issue a
>>> +         * warning instead of BUG().
>>> +         */
>>> +        printk(XENLOG_WARNING
>>> +                "WARNING, BUG: arch_monitor_get_capabilities() not 
>>> implemented"
>>> +                "properly.\n");
>>>   
>>> -    };
>>> +        return -EOPNOTSUPP;
>>> +    }
>> I disagree with the issuing of a message here. At the very least this
>> should be a dprintk(). Perhaps an ASSERT_UNREACHABLE() would be
>> the way to go?
> 
> IDK, but I agree that it doesn't look so elegant like that.
> Won't ASSERT_UNREACHABLE crash the hypervisor?

In a debug build yes. In a release build no.

>> What's worse though is that I can't see the checking
>> which would make true the "should not be reached" statement above
>> (not that you must not rely on the caller of the hypercall to be well
>> behaved).
> 
> The reasoning is as follows.
>  From this part in monitor_domctl:
> 
>      switch ( mop->op )
>      {
>      case XEN_DOMCTL_MONITOR_OP_ENABLE:
>      case XEN_DOMCTL_MONITOR_OP_DISABLE:
>          /* Check if event type is available. */
>          if ( unlikely(!(arch_monitor_get_capabilities(d) & (1 << 
> mop->event))) )
>              return -EOPNOTSUPP;
>          /* Arch-side handles enable/disable ops. */
>          return arch_monitor_domctl_event(d, mop);
> 
> we can see that arch_monitor_domctl_event gets to be called only when 
> arch_monitor_get_capabilities reports
> an 'available' mop->event.
> But if then in arch_monitor_domctl_event the default case is reached, it 
> would mean
> that arch_monitor_get_capabilities reported a mop->event as being 
> available/supported
> when is *not actually handled* anywhere.

Ah, I see now that I've mis-read the default: code further below,
which actually calls arch_monitor_domctl_op(), not ..._event().
However, there's an "undefined behavior" issue with the code
above then when mop->event >= 31 - I think you want to left
shift 1U instead of plain 1, and you need to range check
mop->event first.

Jan


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