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

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 10 April 2017 11:04
> To: Paul Durrant <Paul.Durrant@citrix.com>; Xen-devel <xen-
> devel@lists.xen.org>
> Cc: Jennifer Herbert <jennifer.herbert@citrix.com>; Jan Beulich
> <JBeulich@suse.com>; Julien Grall <julien.grall@arm.com>
> 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:
> >>
> >>> 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.
> 
> Fine.  Let me rephrase as "GCC does inline it".
> 

That's better :-)

Yes, the fact it's a const pointer does allow for inlining (dereferencing stack pointer usually being a surefire way to stop inlining and burn stack frames) but perhaps there's a case for forcing an inline? (I don't know what clang will do).

> >
> >>  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.
> 
> Passing more than one parameter is of course fine.
> 
> Requiring the caller to pass two parameters which strictly must be in
> sync for security reasons is not fine.
> 

For calling non-local functions, I agree. But the callee is local here and the intention was that the prototype be the same as the hypercall modulo the guest handle nastiness. So, if you want to pack things into a struct then I'd prefer a general 'dm_op_args' struct  that includes the domid as well.

  Paul

> ~Andrew

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