[Qemu-devel] [PATCH] Use ffs in favor of ffsll

[Qemu-devel] [PATCH] Use ffs in favor of ffsll

[Jan Kiszka]

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

Not all host platforms support the ll variant. This is not a critical
path, so go the easy way.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 target-i386/machine.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/target-i386/machine.c b/target-i386/machine.c
index 259302c..4ab154c 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -147,10 +147,10 @@ void cpu_save(QEMUFile *f, void *opaque)
     /* There can only be one pending IRQ set in the bitmap at a time, so try
        to find it and save its number instead (-1 for none). */
     pending_irq = -1;
-    for (i = 0; i < ARRAY_SIZE(env->interrupt_bitmap); i++) {
-        bit = ffsll(env->interrupt_bitmap[i]);
+    for (i = 0; i < sizeof(env->interrupt_bitmap) / sizeof(int); i++) {
+        bit = ffs(((int *)env->interrupt_bitmap)[i]);
         if (bit) {
-            pending_irq = i * 64 + bit - 1;
+            pending_irq = i * 8 * sizeof(int) + bit - 1;
             break;
         }
     }


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH] Use ffs in favor of ffsll

[Blue Swirl]

On 7/1/09, Jan Kiszka <jan.kiszka@web.de> wrote:
> Not all host platforms support the ll variant. This is not a critical
>  path, so go the easy way.

>  -    for (i = 0; i < ARRAY_SIZE(env->interrupt_bitmap); i++) {
>  -        bit = ffsll(env->interrupt_bitmap[i]);
>  +    for (i = 0; i < sizeof(env->interrupt_bitmap) / sizeof(int); i++) {
>  +        bit = ffs(((int *)env->interrupt_bitmap)[i]);
>          if (bit) {
>  -            pending_irq = i * 64 + bit - 1;
>  +            pending_irq = i * 8 * sizeof(int) + bit - 1;

I think this will not work on a big endian host.

[Qemu-devel] Re: [PATCH] Use ffs in favor of ffsll

[Jan Kiszka]

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

Blue Swirl wrote:
> On 7/1/09, Jan Kiszka <jan.kiszka@web.de> wrote:
>> Not all host platforms support the ll variant. This is not a critical
>>  path, so go the easy way.
> 
>>  -    for (i = 0; i < ARRAY_SIZE(env->interrupt_bitmap); i++) {
>>  -        bit = ffsll(env->interrupt_bitmap[i]);
>>  +    for (i = 0; i < sizeof(env->interrupt_bitmap) / sizeof(int); i++) {
>>  +        bit = ffs(((int *)env->interrupt_bitmap)[i]);
>>          if (bit) {
>>  -            pending_irq = i * 64 + bit - 1;
>>  +            pending_irq = i * 8 * sizeof(int) + bit - 1;
> 
> I think this will not work on a big endian host.

Right, may theoretically bite us once we are able to migrate between kvm
and tcg. Will send a better version nevertheless.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] [PATCH v2] Use ffs in favor of ffsll

[Jan Kiszka]

Jan Kiszka wrote:
> Blue Swirl wrote:
>> On 7/1/09, Jan Kiszka <jan.kiszka@web.de> wrote:
>>> Not all host platforms support the ll variant. This is not a critical
>>>  path, so go the easy way.
>>>  -    for (i = 0; i < ARRAY_SIZE(env->interrupt_bitmap); i++) {
>>>  -        bit = ffsll(env->interrupt_bitmap[i]);
>>>  +    for (i = 0; i < sizeof(env->interrupt_bitmap) / sizeof(int); i++) {
>>>  +        bit = ffs(((int *)env->interrupt_bitmap)[i]);
>>>          if (bit) {
>>>  -            pending_irq = i * 64 + bit - 1;
>>>  +            pending_irq = i * 8 * sizeof(int) + bit - 1;
>> I think this will not work on a big endian host.
> 
> Right, may theoretically bite us once we are able to migrate between kvm
> and tcg. Will send a better version nevertheless.
> 

I decided that a comment should suffice.

-----

Not all host platforms support the ll variant. This is not a critical
path, so go the easy way.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 target-i386/machine.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/target-i386/machine.c b/target-i386/machine.c
index 259302c..1a56b3d 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -147,10 +147,12 @@ void cpu_save(QEMUFile *f, void *opaque)
     /* There can only be one pending IRQ set in the bitmap at a time, so try
        to find it and save its number instead (-1 for none). */
     pending_irq = -1;
-    for (i = 0; i < ARRAY_SIZE(env->interrupt_bitmap); i++) {
-        bit = ffsll(env->interrupt_bitmap[i]);
+    for (i = 0; i < sizeof(env->interrupt_bitmap) / sizeof(int); i++) {
+        /* Note: This assumes little endian host, which is true in KVM mode.
+           In TCG mode it must be zero anyway. */
+        bit = ffs(((int *)env->interrupt_bitmap)[i]);
         if (bit) {
-            pending_irq = i * 64 + bit - 1;
+            pending_irq = i * 8 * sizeof(int) + bit - 1;
             break;
         }
     }

Re: [Qemu-devel] [PATCH v2] Use ffs in favor of ffsll

[Nathan Froyd]

On Wed, Jul 01, 2009 at 10:55:02PM +0200, Jan Kiszka wrote:
> Jan Kiszka wrote:
> > Blue Swirl wrote:
> >> I think this will not work on a big endian host.
> > 
> > Right, may theoretically bite us once we are able to migrate between kvm
> > and tcg. Will send a better version nevertheless.
> > 
> 
> -    for (i = 0; i < ARRAY_SIZE(env->interrupt_bitmap); i++) {
> -        bit = ffsll(env->interrupt_bitmap[i]);
> +    for (i = 0; i < sizeof(env->interrupt_bitmap) / sizeof(int); i++) {
> +        /* Note: This assumes little endian host, which is true in KVM mode.
> +           In TCG mode it must be zero anyway. */
> +        bit = ffs(((int *)env->interrupt_bitmap)[i]);

ISTR that some PPC hosts support KVM...

-Nathan

[Qemu-devel] Re: [PATCH v2] Use ffs in favor of ffsll

[Jan Kiszka]

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

Nathan Froyd wrote:
> On Wed, Jul 01, 2009 at 10:55:02PM +0200, Jan Kiszka wrote:
>> Jan Kiszka wrote:
>>> Blue Swirl wrote:
>>>> I think this will not work on a big endian host.
>>> Right, may theoretically bite us once we are able to migrate between kvm
>>> and tcg. Will send a better version nevertheless.
>>>
>> -    for (i = 0; i < ARRAY_SIZE(env->interrupt_bitmap); i++) {
>> -        bit = ffsll(env->interrupt_bitmap[i]);
>> +    for (i = 0; i < sizeof(env->interrupt_bitmap) / sizeof(int); i++) {
>> +        /* Note: This assumes little endian host, which is true in KVM mode.
>> +           In TCG mode it must be zero anyway. */
>> +        bit = ffs(((int *)env->interrupt_bitmap)[i]);
> 
> ISTR that some PPC hosts support KVM...
> 

...but not for x86 guests. :)

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Qemu-devel] [PATCH v2] Use ffs in favor of ffsll

[Stuart Brady]

On Wed, Jul 01, 2009 at 01:58:02PM -0700, Nathan Froyd wrote:
> On Wed, Jul 01, 2009 at 10:55:02PM +0200, Jan Kiszka wrote:
> > -    for (i = 0; i < ARRAY_SIZE(env->interrupt_bitmap); i++) {
> > -        bit = ffsll(env->interrupt_bitmap[i]);
> > +    for (i = 0; i < sizeof(env->interrupt_bitmap) / sizeof(int); i++) {
> > +        /* Note: This assumes little endian host, which is true in KVM mode.
> > +           In TCG mode it must be zero anyway. */
> > +        bit = ffs(((int *)env->interrupt_bitmap)[i]);
> 
> ISTR that some PPC hosts support KVM...

FWIW, I notice we currently have a qemu_fls() defined in cutils.c, which 
uses the clz32() defined in host-utils.h.

qemu_ffsll() could be implemented fairly easily in terms of ctz64(),
although note that ffsll(n) == (ctz64(n) + 1) % 65.

Cheers,
-- 
Stuart Brady

[Qemu-devel] [PATCH] Use ctz64 in favor of ffsll

[Jan Kiszka]

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

Stuart Brady wrote:
> On Wed, Jul 01, 2009 at 01:58:02PM -0700, Nathan Froyd wrote:
>> On Wed, Jul 01, 2009 at 10:55:02PM +0200, Jan Kiszka wrote:
>>> -    for (i = 0; i < ARRAY_SIZE(env->interrupt_bitmap); i++) {
>>> -        bit = ffsll(env->interrupt_bitmap[i]);
>>> +    for (i = 0; i < sizeof(env->interrupt_bitmap) / sizeof(int); i++) {
>>> +        /* Note: This assumes little endian host, which is true in KVM mode.
>>> +           In TCG mode it must be zero anyway. */
>>> +        bit = ffs(((int *)env->interrupt_bitmap)[i]);
>> ISTR that some PPC hosts support KVM...
> 
> FWIW, I notice we currently have a qemu_fls() defined in cutils.c, which 
> uses the clz32() defined in host-utils.h.
> 
> qemu_ffsll() could be implemented fairly easily in terms of ctz64(),
> although note that ffsll(n) == (ctz64(n) + 1) % 65.

ctz64 - that's what I wanted! (No need to introduce qemu_ffsll, at least
not for this use case.)

Jan

--------->

Not all host platforms support ffsll.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 target-i386/machine.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/target-i386/machine.c b/target-i386/machine.c
index 259302c..e4099ca 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -148,9 +148,9 @@ void cpu_save(QEMUFile *f, void *opaque)
        to find it and save its number instead (-1 for none). */
     pending_irq = -1;
     for (i = 0; i < ARRAY_SIZE(env->interrupt_bitmap); i++) {
-        bit = ffsll(env->interrupt_bitmap[i]);
-        if (bit) {
-            pending_irq = i * 64 + bit - 1;
+        if (env->interrupt_bitmap[i]) {
+            bit = ctz64(env->interrupt_bitmap[i]);
+            pending_irq = i * 64 + bit;
             break;
         }
     }


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] [PATCH v2] Use ctz64 in favor of ffsll

[Jan Kiszka]

Not all host platforms support ffsll.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 target-i386/machine.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/target-i386/machine.c b/target-i386/machine.c
index 259302c..2a72b01 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -2,6 +2,7 @@
 #include "hw/boards.h"
 #include "hw/pc.h"
 #include "hw/isa.h"
+#include "host-utils.h"
 
 #include "exec-all.h"
 #include "kvm.h"
@@ -148,9 +149,9 @@ void cpu_save(QEMUFile *f, void *opaque)
        to find it and save its number instead (-1 for none). */
     pending_irq = -1;
     for (i = 0; i < ARRAY_SIZE(env->interrupt_bitmap); i++) {
-        bit = ffsll(env->interrupt_bitmap[i]);
-        if (bit) {
-            pending_irq = i * 64 + bit - 1;
+        if (env->interrupt_bitmap[i]) {
+            bit = ctz64(env->interrupt_bitmap[i]);
+            pending_irq = i * 64 + bit;
             break;
         }
     }

Re: [Qemu-devel] [PATCH v2] Use ctz64 in favor of ffsll

[Paul Brook]

On Thursday 02 July 2009, Jan Kiszka wrote:
> Not all host platforms support ffsll.

>-            pending_irq = i * 64 + bit - 1;
>+            bit = ctz64(env->interrupt_bitmap[i]);
>+            pending_irq = i * 64 + bit;

Are we sure there is only ever one bit set in interrupt_bitmap?
If not does it mater that we're processing them in reverse order?

Paul

Re: [Qemu-devel] [PATCH v2] Use ctz64 in favor of ffsll

[Jan Kiszka]

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

Paul Brook wrote:
> On Thursday 02 July 2009, Jan Kiszka wrote:
>> Not all host platforms support ffsll.
> 
>> -            pending_irq = i * 64 + bit - 1;
>> +            bit = ctz64(env->interrupt_bitmap[i]);
>> +            pending_irq = i * 64 + bit;
> 
> Are we sure there is only ever one bit set in interrupt_bitmap?

Yes.

> If not does it mater that we're processing them in reverse order?
> 
> Paul

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Qemu-devel] Re: [PATCH] Use ffs in favor of ffsll

[Christoph Egger]

On Wednesday 01 July 2009 22:27:51 Blue Swirl wrote:
> On 7/1/09, Jan Kiszka <jan.kiszka@web.de> wrote:
> > Not all host platforms support the ll variant. This is not a critical
> >  path, so go the easy way.
> >
> >  -    for (i = 0; i < ARRAY_SIZE(env->interrupt_bitmap); i++) {
> >  -        bit = ffsll(env->interrupt_bitmap[i]);
> >  +    for (i = 0; i < sizeof(env->interrupt_bitmap) / sizeof(int); i++) {
> >  +        bit = ffs(((int *)env->interrupt_bitmap)[i]);
> >          if (bit) {
> >  -            pending_irq = i * 64 + bit - 1;
> >  +            pending_irq = i * 8 * sizeof(int) + bit - 1;
>
> I think this will not work on a big endian host.

A qemu_ffsll() implementation can be something like this:

int qemu_ffsll(long long mask)
{
      int bit;

      bit = ffs((int)mask);
      if (bit == 0) {
           mask >>= 32;
           bit = ffs((int)mask);
           if (bit) bit += 32;
      }
       return bit;
}

Christoph



-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632