Re: [Xen-devel] [PATCH v7 03/15] x86/hvm: restrict port numbers and uint16_t and sizes to unsigned int

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 09 July 2015 17:20
> To: Paul Durrant
> Cc: xen-devel@xxxxxxxxxxxxx; Keir (Xen.org)
> Subject: RE: [PATCH v7 03/15] x86/hvm: restrict port numbers and uint16_t
> and sizes to unsigned int
> 
> >>> On 09.07.15 at 18:10, <Paul.Durrant@xxxxxxxxxx> wrote:
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: 09 July 2015 16:24
> >> >>> On 09.07.15 at 15:10, <paul.durrant@xxxxxxxxxx> wrote:
> >> > Building on the previous patch, this patch restricts portio port numbers
> >> > to uint16_t in registration/relocate calls. In portio_action_t the port
> >> > number is change to unsigned int though to avoid the compiler
> generating
> >> > 16-bit operations unnecessarily. The patch also changes I/O sizes to
> >> > unsigned int which then allows the io_handler size field to reduce to
> >> > an unsigned int.
> >> >
> >> > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> >> > Cc: Keir Fraser <keir@xxxxxxx>
> >> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> >> > Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >> > ---
> >> >
> >> > v7:
> >> > - Change port type in portio_action_t to unsigned int as requested
> >> >   by Jan
> >>
> >> Yet title and description were left in places, and ...
> >
> > The title remains. The description was modified:
> >
> > " In portio_action_t the port number is change to unsigned int though to
> > avoid the compiler generating 16-bit operations unnecessarily."
> 
> Maybe I'm just being confused by the two "and"-s in the title,
> which I assumed can't both be meant to be "and", or else you'd
> have written "port numbers, uint16_t, and sizes". Plus it seems
> unclear what "restrict a uint16_t to unsigned int" actually means.
> 
> >> > @@ -96,17 +96,17 @@ int hvm_mmio_intercept(ioreq_t *p);
> >> >  int hvm_buffered_io_send(ioreq_t *p);
> >> >
> >> >  static inline void register_portio_handler(
> >> > -    struct domain *d, unsigned long addr,
> >> > -    unsigned long size, portio_action_t action)
> >> > +    struct domain *d, uint16_t port, unsigned int size,
> >> > +    portio_action_t action)
> >> >  {
> >> > -    register_io_handler(d, addr, size, action, HVM_PORTIO);
> >> > +    register_io_handler(d, port, size, action, HVM_PORTIO);
> >> >  }
> >> >
> >> >  static inline void relocate_portio_handler(
> >> > -    struct domain *d, unsigned long old_addr, unsigned long new_addr,
> >> > -    unsigned long size)
> >> > +    struct domain *d, uint16_t old_port, uint16_t new_port,
> >> > +    unsigned int size)
> >> >  {
> >> > -    relocate_io_handler(d, old_addr, new_addr, size, HVM_PORTIO);
> >> > +    relocate_io_handler(d, old_port, new_port, size, HVM_PORTIO);
> >> >  }
> >>
> >> ... these still use uint16_t. I'm pretty sure I gave my comment in a
> >> way indicating that this should generally change, perhaps just at
> >> the example of portio_action_t.
> >
> > Why? Do we not want to restrict to uint16_t at the interface level?
> 
> Quite the inverse - why would we want to? This just makes the
> calling sequence less efficient (due to the needed operand size
> overrides), and hides mistakes in callers properly truncating
> when reading guest registers.
> 

I would have thought that the compiler would point out the overflow if a caller 
tried to pass >64k in the port number, but I'll get rid of the remaining 
uint16_ts.

  Paul

> Jan


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