Re: [Xen-devel] [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than passing two parameters around

[Jennifer Herbert <Jennifer.Herbert@xxxxxxxxxx>]

On 10/04/17 10:40, Paul Durrant wrote:
> > -----Original Message-----
> > From: Andrew Cooper
> > Sent: 10 April 2017 10:29
> > To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; Xen-devel <xen-
> > devel@xxxxxxxxxxxxx>
> > Cc: Jennifer Herbert <jennifer.herbert@xxxxxxxxxx>; Jan Beulich
> > <JBeulich@xxxxxxxx>; Julien Grall <julien.grall@xxxxxxx>
> > Subject: Re: [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than
> > passing two parameters around
> >
> > On 10/04/17 10:04, Paul Durrant wrote:
> > > > -----Original Message-----
> > > > From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> > > > Sent: 07 April 2017 20:36
> > > > To: Xen-devel <xen-devel@xxxxxxxxxxxxx>
> > > > Cc: Jennifer Herbert <jennifer.herbert@xxxxxxxxxx>; Andrew Cooper
> > > > <Andrew.Cooper3@xxxxxxxxxx>; Jan Beulich <JBeulich@xxxxxxxx>; Paul
> > > > Durrant <Paul.Durrant@xxxxxxxxxx>; Julien Grall <julien.grall@xxxxxxx>
> > > > Subject: [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than
> > > > passing two parameters around
> > > >
> > > > From: Jennifer Herbert <Jennifer.Herbert@xxxxxxxxxx>
> > > >
> > > > No functional change.
> > > Why is this a good thing? Passing two parameters around allowed for them
> > to be in registers. I preferred the code as it was before.
> >
> > a) It will always be inlined, so registers aren't relevant.
> Why? I see nothing forcing the compiler to make it so.
>
> >   Even if
> > they were, all values are available directly with the pointer as a base,
> > so there is no reduction in expressiveness.  (i.e. the previous code
> > only increases register pressure).
> > b) passing multiple parameters like that is a recipe for mistakes, and
> > in this case, mistakes mean security vulnerabilities.
> Given the locality of the code I don't buy that as an argument unless you're 
> going to assert that passing more than one parameter is always wrong.
To two parameters should go everywhere together - they work together, 
and should never be separated.  But by having them separate, you end up 
forcing people to think about why do I need this nr_buffs, even though 
in many situations it looks superfluous. Its mostly there as a check, 
and not part of the core flow of how the code works.
It just confuses and clutters things to have it separate.

If its left separate, its possible someone will feed in a different, but 
similar variable for this parameter, and its purpose defeated.
> > c) It is necessary to make legible code for the later changes in this
> > series.
>  From my reading I don't think it would have an overly negative effect on the 
> subsequent patches if this patch were dropped.
No, but since we're having this debate, we clearly care about small 
negative effects too.
-jenny

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