| Message ID | 1276635808-7315-1-git-send-email-weil@mail.berlios.de |
|---|---|
| State | New |
| Headers | show |
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"); >
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
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"); >
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..]
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~
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.
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~ >
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");
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(-)