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

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

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1143 bytes --]

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

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- 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)(




[-- Attachment #2: x86-HVM-first-last-byte.patch --]
[-- Type: text/plain, Size: 1195 bytes --]

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

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- 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)(

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

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

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> 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.

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

  Paul

> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- 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@lists.xen.org
http://lists.xen.org/xen-devel

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

[Jan Beulich]

>>> On 26.04.16 at 10:41, <Paul.Durrant@citrix.com> wrote:
>>  -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> 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.

Jan

>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> 
>> --- 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@lists.xen.org
http://lists.xen.org/xen-devel

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

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> 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@citrix.com> wrote:
> >>  -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> 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@citrix.com>

> Jan
> 
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>
> >> --- 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@lists.xen.org
http://lists.xen.org/xen-devel

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

[Wei Liu]

On Tue, Apr 26, 2016 at 02:28:24AM -0600, Jan Beulich wrote:
> 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
> 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).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 

Release-acked-by: Wei Liu <wei.liu2@citrix.com>


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