From mboxrd@z Thu Jan 1 00:00:00 1970 From: Corneliu ZUZU Subject: Re: [PATCH v3 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side. Date: Mon, 15 Feb 2016 15:29:23 +0200 Message-ID: <56C1D2B3.8050604@bitdefender.com> References: <1455518118-414-1-git-send-email-czuzu@bitdefender.com> <1455518254-507-1-git-send-email-czuzu@bitdefender.com> <56C1C79002000078000D20B8@prv-mh.provo.novell.com> <56C1C135.9070707@bitdefender.com> <56C1D62C02000078000D21B3@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0164693340036464930==" Return-path: In-Reply-To: <56C1D62C02000078000D21B3@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Tamas K Lengyel , Keir Fraser , Ian Campbell , Razvan Cojocaru , Andrew Cooper , xen-devel@lists.xen.org, Stefano Stabellini List-Id: xen-devel@lists.xenproject.org This is a multi-part message in MIME format. --===============0164693340036464930== Content-Type: multipart/alternative; boundary="------------070407060203030100060509" This is a multi-part message in MIME format. --------------070407060203030100060509 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit On 2/15/2016 2:44 PM, Jan Beulich wrote: > >> 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); > 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 > Never looked @ that part before, used it the way it was. I suppose that's because "according to the C specification, the result of a bit shift operation on a signed argument gives implementation-defined results, so in/theory/|1U << i|is more portable than|1 << i|" (taken from a stackoverflow post). After changing 1 to 1U though, I don't understand why we should also range-check mop->event. I'm imagining when (mop->event > 31): * (1U << mop->event) = 0 or >= (0x1 + 0xFFFFFFFF) (?) * in both cases arch_monitor_get_capabilities(d) & (1U << mop->event) would be = 0 * in which case we would return -EOPNOTSUPP , no? Corneliu. --------------070407060203030100060509 Content-Type: text/html; charset=windows-1252 Content-Length: 6050 Content-Transfer-Encoding: quoted-printable
On 2/15/2016 2:44 PM, Jan Beulich wrote:

     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);

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 >=3D 31 - I think you want to left
shift 1U instead of plain 1, and you need to range check
mop->event first.

Jan
Never looked @ that part before, used it the way it was.
I suppose that's because "according to the C specification, the result of a bit shift
operation on a signed argument gives implementation-defined results, so in=A0theory=A01U << i=A0is
more portable than=A01 << i" (taken from a stackoverflow post).

After changing 1 to 1U though, I don't understand why we should also range-check mop->event.
I'm imagining when (mop->event > 31):
* (1U << mop->event) =3D 0 or >=3D (0x1 + 0xFFFFFFFF) (=3F)
* in both cases arch_monitor_get_capabilities(d) & (1U << mop->event) would be =3D 0
* in which case we would return -EOPNOTSUPP
, no=3F

Corneliu.
--------------070407060203030100060509-- --===============0164693340036464930== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============0164693340036464930==--