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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jennifer Herbert [mailto:Jennifer.Herbert@xxxxxxxxxx]
> Sent: 10 April 2017 11:06
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Xen-devel <xen-devel@xxxxxxxxxxxxx>
> Cc: 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: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.

Well that's where I disagree. I don't think it is any more confusing than 
having multiple args to any function.

> 
> If its left separate, its possible someone will feed in a different, but
> similar variable for this parameter, and its purpose defeated.
> 

Well, that would indication fairly serious incompetence in the 'someone'. 
do_dm_op() only exists to be the common entry point to the hypercall after the 
compat and guest handle nastiness has been sorted out. It's static and clearly 
not some sort of general purpose utility function. Keeping the prototype 
aligned with that of the hypercall seems illustrative and entirely reasonable 
to me.
As I replied to Andrew though, I'm ok with a general args structure packaging 
the domid as well (since this is no less critical to security than the nr_bufs 
argument).

  Paul

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