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

From: "Jan Beulich" <JBeulich@suse.com>
To: Corneliu ZUZU <czuzu@bitdefender.com>
Cc: Tamas K Lengyel <tamas@tklengyel.com>, Keir Fraser <keir@xen.org>,
	Ian Campbell <ian.campbell@citrix.com>,
	Razvan Cojocaru <rcojocaru@bitdefender.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	xen-devel@lists.xen.org,
	Stefano Stabellini <stefano.stabellini@citrix.com>
Subject: Re: [PATCH v3 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.
Date: Mon, 15 Feb 2016 05:44:12 -0700	[thread overview]
Message-ID: <56C1D62C02000078000D21B3@prv-mh.provo.novell.com> (raw)
In-Reply-To: <56C1C135.9070707@bitdefender.com>

>>> On 15.02.16 at 13:14, <czuzu@bitdefender.com> wrote:
> On 2/15/2016 1:41 PM, Jan Beulich wrote:
>>>>> On 15.02.16 at 07:37, <czuzu@bitdefender.com> 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