Re: [Xen-devel] [PATCH v5 09/15] argo: implement the sendv op; evtchn: expose send_guest_global_virq

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Sun, Feb 03, 2019 at 09:56:26AM -0800, Christopher Clark wrote:
> On Thu, Jan 31, 2019 at 3:01 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
> >
> > On Thu, Jan 31, 2019 at 03:35:23AM -0700, Jan Beulich wrote:
> > > >>> On 31.01.19 at 11:18, <roger.pau@xxxxxxxxxx> wrote:
> > > > On Wed, Jan 30, 2019 at 08:10:28PM -0800, Christopher Clark wrote:
> > > >> On Tue, Jan 22, 2019 at 4:08 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> 
> > > >> wrote:
> > > >> > On Mon, Jan 21, 2019 at 01:59:49AM -0800, Christopher Clark wrote:
> > > >> > > +        /*
> > > >> > > +         * Check padding is zeroed. Reject niov above limit or 
> > > >> > > message_types
> > > >> > > +         * that are outside 32 bit range.
> > > >> > > +         */
> > > >> > > +        if ( unlikely(send_addr.src.pad || send_addr.dst.pad ||
> > > >> > > +                      (arg3 > XEN_ARGO_MAXIOV) || (arg4 & 
> > > >> > > ~0xffffffffUL)) )
> > > >> >
> > > >> > arg4 & (GB(4) - 1)
> > > >> >
> > > >> > Is clearer IMO, or:
> > > >> >
> > > >> > arg4 > UINT32_MAX
> > > >>
> > > >> I've left the code unchanged, as the mask constant is used multiple
> > > >> places elsewhere in Xen. UINT32_MAX is only used as a threshold value.
> > > >
> > > > The fact that others parts of the code could be improved is not an
> > > > excuse to follow suit. I'm having a hard time believing that you find
> > > > "arg4 & ~0xffffffffUL" easier to read than "arg4 & ~(GB(4) - 1)" or
> > > > even "arg4 >= GB(4)".
> 
> 
> Below, I propose an alternative way of achieving our correctness and
> readability goals.
> 
> On the topic of readability, this self-contained definition
> does stand out: ~0xffffffffUL,
> encouraging caution and careful counting of 'f's. However, no other
> source files are involved, making the code independent of changes in
> (macro) definitions in other files.
> 
> In comparison, to understand GB, I have find the external definition,
> and then parse this:
> 
> #define GB(_gb)     (_AC(_gb, ULL) << 30)
> 
> (which seems to have a different type? ULL vs UL?) and then find and
> understand this, in another file:
> 
> #ifdef __ASSEMBLY__
> #define _AC(X,Y)    X
> #define _AT(T,X)    X
> #else
> #define __AC(X,Y)   (X##Y)
> #define _AC(X,Y)    __AC(X,Y)
> #define _AT(T,X)    ((T)(X))
> #endif
> 
> so I'm saying: it's at least somewhat arguable which is easier to understand.
> Regardless, I think there's a better option than either.
> 
> > > > IMO it's much more likely to miss an 'f' in the first construct, and
> > > > thus get the value wrong and introduce a bug.
> > >
> > > I agree with this last statement, but I'm having trouble to see how
> > > message _type_ is related to a size construct like GB(4) is. I see
> > > only UINT32_MAX as a viable alternative for something that's not
> > > expressing the size of anything.
> >
> > I've suggested the GB construct as an alternative because the comment
> > above mentions the 32bit range. IMO anything that avoids using
> > 0xffffffffUL is fine.
> 
> Jan and Andrew have employed a useful technique in recent changes where such a
> test was required.  This could work:
> 
> (arg4 != (uint32_t)arg4))
> 
> It is self-contained, readable and clearly expresses the intent of the check
> being performed. I have tested a series with this applied, and have it ready
> to post if you approve.

Yes, that's fine. As said in v7 anything that doesn't involve an
open-coded mask is fine with me.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel