Re: [Xen-devel] [PATCH] x86/HVM: slightly improve hvm_mmio_{first, last}_byte()

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 26 April 2016 09:47
> To: Paul Durrant
> Cc: Wei Liu; xen-devel
> Subject: RE: [PATCH] x86/HVM: slightly improve
> hvm_mmio_{first,last}_byte()
> 
> >>> On 26.04.16 at 10:41, <Paul.Durrant@xxxxxxxxxx> wrote:
> >>  -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: 26 April 2016 09:28
> >> To: xen-devel
> >> Cc: Paul Durrant; Wei Liu
> >> Subject: [PATCH] x86/HVM: slightly improve
> hvm_mmio_{first,last}_byte()
> >>
> >> EFLAGS.DF can be assumed to be usually clear, so unlikely()-annotate
> >> the conditionals accordingly.
> >>
> >> Also prefer latching p->size (used twice) into a local variable, at
> >
> > Well, it should only be used once since only one of the expressions should
> > be evaluated.
> 
> But there would still be two references in code, and the trivial line
> of thought here is that (leaving optimization aside) accessing some
> structure field twice generate code no smaller (and possibly larger)
> than accessing some other structure field just once.
> 
> >> once making it unnecessary for the reader to be sure expressions get
> >> evaluated left to right (operand promotion would yield a different
> >> result if p->addr + p->size - 1 was evaluted right to left).
> >
> > Would that not be cured by replacing 1 with 1ul?
> 
> That's another possibility, but (being a matter of taste) I prefer to
> avoid type suffixes.
> 

Fair enough, I have no particular preference either way. I guess it would be 
nice if hvm_mmio_first_byte() and hvm_mmio_last_byte() were consistent in their 
use of type suffixes though. However...

Reviewed-by: Paul Durrant <paul.durrant@xxxxxxxxxx>

> Jan
> 
> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >>
> >> --- a/xen/include/asm-x86/hvm/io.h
> >> +++ b/xen/include/asm-x86/hvm/io.h
> >> @@ -44,18 +44,18 @@ struct hvm_mmio_ops {
> >>
> >>  static inline paddr_t hvm_mmio_first_byte(const ioreq_t *p)
> >>  {
> >> -    return p->df ?
> >> +    return unlikely(p->df) ?
> >>             p->addr - (p->count - 1ul) * p->size :
> >>             p->addr;
> >>  }
> >>
> >>  static inline paddr_t hvm_mmio_last_byte(const ioreq_t *p)
> >>  {
> >> -    unsigned long count = p->count;
> >> +    unsigned long size = p->size;
> >>
> >> -    return p->df ?
> >> -           p->addr + p->size - 1:
> >> -           p->addr + (count * p->size) - 1;
> >> +    return unlikely(p->df) ?
> >> +           p->addr + size - 1:
> >> +           p->addr + (p->count * size) - 1;
> >>  }
> >>
> >>  typedef int (*portio_action_t)(
> >>
> >>
> 
> 


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