[Qemu-devel] [PATCH] Fix comparison which always returned false

[Qemu-devel] [PATCH] Fix comparison which always returned false

[Stefan Weil]

Comparing an 8 bit value with ~0 does not work as expected.
Replace ~0 by UINT8_MAX in comparison and also in assignment
(and fix coding style, too).

Cc: Gleb Natapov <gleb@redhat.com>
Cc: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 hw/hpet.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/hpet.c b/hw/hpet.c
index 0c80ee5..d5c406c 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -74,7 +74,7 @@ typedef struct HPETState {
     uint8_t  hpet_id;           /* instance id */
 } HPETState;
 
-struct hpet_fw_config hpet_cfg = {.count = ~0};
+struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
 
 static uint32_t hpet_in_legacy_mode(HPETState *s)
 {
@@ -682,8 +682,10 @@ static int hpet_init(SysBusDevice *dev)
     int i, iomemtype;
     HPETTimer *timer;
 
-    if (hpet_cfg.count == ~0) /* first instance */
+    if (hpet_cfg.count == UINT8_MAX) {
+        /* first instance */
         hpet_cfg.count = 0;
+    }
 
     if (hpet_cfg.count == 8) {
         fprintf(stderr, "Only 8 instances of HPET is allowed\n");
-- 
1.5.6.5

Re: [Qemu-devel] [PATCH] Fix comparison which always returned false

[Anthony Liguori]

On 06/15/2010 04:03 PM, Stefan Weil wrote:
> Comparing an 8 bit value with ~0 does not work as expected.
> Replace ~0 by UINT8_MAX in comparison and also in assignment
> (and fix coding style, too).
>    

Because when the uint8_t gets promoted, it doesn't get zero filled.  I'd 
rather something a bit more obvious like HPET_INVALID_COUNT.

Regards,

Anthony Liguori

> Cc: Gleb Natapov<gleb@redhat.com>
> Cc: Anthony Liguori<aliguori@us.ibm.com>
> Signed-off-by: Stefan Weil<weil@mail.berlios.de>
> ---
>   hw/hpet.c |    6 ++++--
>   1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/hpet.c b/hw/hpet.c
> index 0c80ee5..d5c406c 100644
> --- a/hw/hpet.c
> +++ b/hw/hpet.c
> @@ -74,7 +74,7 @@ typedef struct HPETState {
>       uint8_t  hpet_id;           /* instance id */
>   } HPETState;
>
> -struct hpet_fw_config hpet_cfg = {.count = ~0};
> +struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
>
>   static uint32_t hpet_in_legacy_mode(HPETState *s)
>   {
> @@ -682,8 +682,10 @@ static int hpet_init(SysBusDevice *dev)
>       int i, iomemtype;
>       HPETTimer *timer;
>
> -    if (hpet_cfg.count == ~0) /* first instance */
> +    if (hpet_cfg.count == UINT8_MAX) {
> +        /* first instance */
>           hpet_cfg.count = 0;
> +    }
>
>       if (hpet_cfg.count == 8) {
>           fprintf(stderr, "Only 8 instances of HPET is allowed\n");
>    

Re: [Qemu-devel] [PATCH] Fix comparison which always returned false

[Richard Henderson]

On 06/15/2010 02:28 PM, Anthony Liguori wrote:
> On 06/15/2010 04:03 PM, Stefan Weil wrote:
>> Comparing an 8 bit value with ~0 does not work as expected.
>> Replace ~0 by UINT8_MAX in comparison and also in assignment
>> (and fix coding style, too).
>>    
> 
> Because when the uint8_t gets promoted, it doesn't get zero filled.  I'd
> rather something a bit more obvious like HPET_INVALID_COUNT.

Er, yes it does.  The problem is that it *did* get zero-extended,
but ~0 is 0xffffffff, so the comparison fails.

But I really agree with Jan Kiszka down-thread -- why do we need
to signal this as a special case at all?


r~

Re: [Qemu-devel] [PATCH] Fix comparison which always returned false

[Anthony Liguori]

On 06/15/2010 07:27 PM, Richard Henderson wrote:
> On 06/15/2010 02:28 PM, Anthony Liguori wrote:
>    
>> On 06/15/2010 04:03 PM, Stefan Weil wrote:
>>      
>>> Comparing an 8 bit value with ~0 does not work as expected.
>>> Replace ~0 by UINT8_MAX in comparison and also in assignment
>>> (and fix coding style, too).
>>>
>>>        
>> Because when the uint8_t gets promoted, it doesn't get zero filled.  I'd
>> rather something a bit more obvious like HPET_INVALID_COUNT.
>>      
> Er, yes it does.  The problem is that it *did* get zero-extended,
> but ~0 is 0xffffffff, so the comparison fails.
>    

Typo on my part.  I meant one filled obviously.

Regards,

Anthony Liguori

> But I really agree with Jan Kiszka down-thread -- why do we need
> to signal this as a special case at all?
>
>
> r~
>    

[Qemu-devel] Re: [PATCH] Fix comparison which always returned false

[Jan Kiszka]

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

Stefan Weil wrote:
> Comparing an 8 bit value with ~0 does not work as expected.
> Replace ~0 by UINT8_MAX in comparison and also in assignment
> (and fix coding style, too).
> 
> Cc: Gleb Natapov <gleb@redhat.com>
> Cc: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> ---
>  hw/hpet.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/hpet.c b/hw/hpet.c
> index 0c80ee5..d5c406c 100644
> --- a/hw/hpet.c
> +++ b/hw/hpet.c
> @@ -74,7 +74,7 @@ typedef struct HPETState {
>      uint8_t  hpet_id;           /* instance id */
>  } HPETState;
>  
> -struct hpet_fw_config hpet_cfg = {.count = ~0};
> +struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
>  
>  static uint32_t hpet_in_legacy_mode(HPETState *s)
>  {
> @@ -682,8 +682,10 @@ static int hpet_init(SysBusDevice *dev)
>      int i, iomemtype;
>      HPETTimer *timer;
>  
> -    if (hpet_cfg.count == ~0) /* first instance */
> +    if (hpet_cfg.count == UINT8_MAX) {
> +        /* first instance */
>          hpet_cfg.count = 0;
> +    }
>  
>      if (hpet_cfg.count == 8) {
>          fprintf(stderr, "Only 8 instances of HPET is allowed\n");

That makes me wonder why we need to signal this special value of
hpet_cfg.count to seabios at all. Why isn't plain 0 for no hpets
sufficient, Gleb?

Jan


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

[Qemu-devel] Re: [PATCH] Fix comparison which always returned false

[Gleb Natapov]

On Tue, Jun 15, 2010 at 11:28:42PM +0200, Jan Kiszka wrote:
> Stefan Weil wrote:
> > Comparing an 8 bit value with ~0 does not work as expected.
> > Replace ~0 by UINT8_MAX in comparison and also in assignment
> > (and fix coding style, too).
> > 
> > Cc: Gleb Natapov <gleb@redhat.com>
> > Cc: Anthony Liguori <aliguori@us.ibm.com>
> > Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> > ---
> >  hw/hpet.c |    6 ++++--
> >  1 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/hpet.c b/hw/hpet.c
> > index 0c80ee5..d5c406c 100644
> > --- a/hw/hpet.c
> > +++ b/hw/hpet.c
> > @@ -74,7 +74,7 @@ typedef struct HPETState {
> >      uint8_t  hpet_id;           /* instance id */
> >  } HPETState;
> >  
> > -struct hpet_fw_config hpet_cfg = {.count = ~0};
> > +struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
> >  
> >  static uint32_t hpet_in_legacy_mode(HPETState *s)
> >  {
> > @@ -682,8 +682,10 @@ static int hpet_init(SysBusDevice *dev)
> >      int i, iomemtype;
> >      HPETTimer *timer;
> >  
> > -    if (hpet_cfg.count == ~0) /* first instance */
> > +    if (hpet_cfg.count == UINT8_MAX) {
> > +        /* first instance */
> >          hpet_cfg.count = 0;
> > +    }
> >  
> >      if (hpet_cfg.count == 8) {
> >          fprintf(stderr, "Only 8 instances of HPET is allowed\n");
> 
> That makes me wonder why we need to signal this special value of
> hpet_cfg.count to seabios at all. Why isn't plain 0 for no hpets
> sufficient, Gleb?
> 
> Jan
> 
I want bios to be able to distinguish between qemu that does not support
HPET cfg_fw and qemu that does. On the former bios should always create
HPET table for backwards compatibility.

--
			Gleb.

Re: [Qemu-devel] [PATCH] Fix comparison which always returned false

[malc]

On Tue, 15 Jun 2010, Stefan Weil wrote:

This should go in asap, it breaks PPC in quite visible and ugly way...

> Comparing an 8 bit value with ~0 does not work as expected.
> Replace ~0 by UINT8_MAX in comparison and also in assignment
> (and fix coding style, too).
> 
> Cc: Gleb Natapov <gleb@redhat.com>
> Cc: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> ---
>  hw/hpet.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/hpet.c b/hw/hpet.c
> index 0c80ee5..d5c406c 100644
> --- a/hw/hpet.c
> +++ b/hw/hpet.c
> @@ -74,7 +74,7 @@ typedef struct HPETState {
>      uint8_t  hpet_id;           /* instance id */
>  } HPETState;
>  
> -struct hpet_fw_config hpet_cfg = {.count = ~0};
> +struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
>  
>  static uint32_t hpet_in_legacy_mode(HPETState *s)
>  {
> @@ -682,8 +682,10 @@ static int hpet_init(SysBusDevice *dev)
>      int i, iomemtype;
>      HPETTimer *timer;
>  
> -    if (hpet_cfg.count == ~0) /* first instance */
> +    if (hpet_cfg.count == UINT8_MAX) {
> +        /* first instance */
>          hpet_cfg.count = 0;
> +    }
>  
>      if (hpet_cfg.count == 8) {
>          fprintf(stderr, "Only 8 instances of HPET is allowed\n");
> 

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] Fix comparison which always returned false

[malc]

On Wed, 16 Jun 2010, malc wrote:

> On Tue, 15 Jun 2010, Stefan Weil wrote:
> 
> This should go in asap, it breaks PPC in quite visible and ugly way...

Right... forgot something, DIY...

Thanks, applied.

[..snip..]

-- 
mailto:av1474@comtv.ru