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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan


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