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

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

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 09 July 2015 16:24
> To: Paul Durrant
> Cc: xen-devel@lists.xen.org; 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 15:10, <paul.durrant@citrix.com> 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@citrix.com>
> > Cc: Keir Fraser <keir@xen.org>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > ---
> >
> > 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."

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

  Paul

> 
> Jan