[Qemu-devel] [PATCH] hpet: Clean up initial hpet counter

[Qemu-devel] [PATCH] hpet: Clean up initial hpet counter

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

There is no need starting with the special value for hpet_cfg.count.
Either Seabios is aware of the new firmware interface and properly
interprets the counter or it simply ignores it anyway.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/hpet.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/hw/hpet.c b/hw/hpet.c
index d5c406c..ed4e995 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 = UINT8_MAX};
+struct hpet_fw_config hpet_cfg;
 
 static uint32_t hpet_in_legacy_mode(HPETState *s)
 {
@@ -682,11 +682,6 @@ static int hpet_init(SysBusDevice *dev)
     int i, iomemtype;
     HPETTimer *timer;
 
-    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");
         return -1;
-- 
1.6.0.2

[Qemu-devel] Re: [PATCH] hpet: Clean up initial hpet counter

[Gleb Natapov]

On Wed, Jun 16, 2010 at 12:40:28AM +0200, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> There is no need starting with the special value for hpet_cfg.count.
> Either Seabios is aware of the new firmware interface and properly
> interprets the counter or it simply ignores it anyway.
> 
I want seabios to be able to distinguish between old qemu and new one.
Hence special value. I used it incorrectly in may v2 seabios patch. Will
resend asap. Will teach me to not change logic at the last minute :( I
removed "valid" field between v1 and v2 of the patches and introduces
special value for count instead. As a result I made one bug in qemu and
one is seabios. Heh.

> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  hw/hpet.c |    7 +------
>  1 files changed, 1 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/hpet.c b/hw/hpet.c
> index d5c406c..ed4e995 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 = UINT8_MAX};
> +struct hpet_fw_config hpet_cfg;
>  
>  static uint32_t hpet_in_legacy_mode(HPETState *s)
>  {
> @@ -682,11 +682,6 @@ static int hpet_init(SysBusDevice *dev)
>      int i, iomemtype;
>      HPETTimer *timer;
>  
> -    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");
>          return -1;
> -- 
> 1.6.0.2

--
			Gleb.

[Qemu-devel] Re: [PATCH] hpet: Clean up initial hpet counter

[Jan Kiszka]

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

Gleb Natapov wrote:
> On Wed, Jun 16, 2010 at 12:40:28AM +0200, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> There is no need starting with the special value for hpet_cfg.count.
>> Either Seabios is aware of the new firmware interface and properly
>> interprets the counter or it simply ignores it anyway.
>>
> I want seabios to be able to distinguish between old qemu and new one.

I see now. But isn't it a good chance to introduce a proper generic
interface for exploring supported fw-cfg keys?

Jan

> Hence special value. I used it incorrectly in may v2 seabios patch. Will
> resend asap. Will teach me to not change logic at the last minute :( I
> removed "valid" field between v1 and v2 of the patches and introduces
> special value for count instead. As a result I made one bug in qemu and
> one is seabios. Heh.
> 
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  hw/hpet.c |    7 +------
>>  1 files changed, 1 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/hpet.c b/hw/hpet.c
>> index d5c406c..ed4e995 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 = UINT8_MAX};
>> +struct hpet_fw_config hpet_cfg;
>>  
>>  static uint32_t hpet_in_legacy_mode(HPETState *s)
>>  {
>> @@ -682,11 +682,6 @@ static int hpet_init(SysBusDevice *dev)
>>      int i, iomemtype;
>>      HPETTimer *timer;
>>  
>> -    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");
>>          return -1;
>> -- 
>> 1.6.0.2
> 
> --
> 			Gleb.



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

[Qemu-devel] Re: [PATCH] hpet: Clean up initial hpet counter

[Gleb Natapov]

On Wed, Jun 16, 2010 at 09:03:01AM +0200, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Wed, Jun 16, 2010 at 12:40:28AM +0200, Jan Kiszka wrote:
> >> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>
> >> There is no need starting with the special value for hpet_cfg.count.
> >> Either Seabios is aware of the new firmware interface and properly
> >> interprets the counter or it simply ignores it anyway.
> >>
> > I want seabios to be able to distinguish between old qemu and new one.
> 
> I see now. But isn't it a good chance to introduce a proper generic
> interface for exploring supported fw-cfg keys?
> 
Having such interface would be nice. Pity we haven't introduced it from
the start. If we do it now seabios will have to find out somehow that
qemu support such interface. Chicken and egg ;)

> Jan
> 
> > Hence special value. I used it incorrectly in may v2 seabios patch. Will
> > resend asap. Will teach me to not change logic at the last minute :( I
> > removed "valid" field between v1 and v2 of the patches and introduces
> > special value for count instead. As a result I made one bug in qemu and
> > one is seabios. Heh.
> > 
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >> ---
> >>  hw/hpet.c |    7 +------
> >>  1 files changed, 1 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/hw/hpet.c b/hw/hpet.c
> >> index d5c406c..ed4e995 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 = UINT8_MAX};
> >> +struct hpet_fw_config hpet_cfg;
> >>  
> >>  static uint32_t hpet_in_legacy_mode(HPETState *s)
> >>  {
> >> @@ -682,11 +682,6 @@ static int hpet_init(SysBusDevice *dev)
> >>      int i, iomemtype;
> >>      HPETTimer *timer;
> >>  
> >> -    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");
> >>          return -1;
> >> -- 
> >> 1.6.0.2
> > 
> > --
> > 			Gleb.
> 
> 



--
			Gleb.

[Qemu-devel] Re: [PATCH] hpet: Clean up initial hpet counter

[Jan Kiszka]

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

Gleb Natapov wrote:
> On Wed, Jun 16, 2010 at 09:03:01AM +0200, Jan Kiszka wrote:
>> Gleb Natapov wrote:
>>> On Wed, Jun 16, 2010 at 12:40:28AM +0200, Jan Kiszka wrote:
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> There is no need starting with the special value for hpet_cfg.count.
>>>> Either Seabios is aware of the new firmware interface and properly
>>>> interprets the counter or it simply ignores it anyway.
>>>>
>>> I want seabios to be able to distinguish between old qemu and new one.
>> I see now. But isn't it a good chance to introduce a proper generic
>> interface for exploring supported fw-cfg keys?
>>
> Having such interface would be nice. Pity we haven't introduced it from
> the start. If we do it now seabios will have to find out somehow that
> qemu support such interface. Chicken and egg ;)

That is easy: Add a key the describes the highest supported key value
(looks like this is monotonously increasing). Older qemu versions will
return 0.

Jan


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

[Qemu-devel] Re: [PATCH] hpet: Clean up initial hpet counter

[Gleb Natapov]

On Wed, Jun 16, 2010 at 09:51:14AM +0200, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Wed, Jun 16, 2010 at 09:03:01AM +0200, Jan Kiszka wrote:
> >> Gleb Natapov wrote:
> >>> On Wed, Jun 16, 2010 at 12:40:28AM +0200, Jan Kiszka wrote:
> >>>> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>
> >>>> There is no need starting with the special value for hpet_cfg.count.
> >>>> Either Seabios is aware of the new firmware interface and properly
> >>>> interprets the counter or it simply ignores it anyway.
> >>>>
> >>> I want seabios to be able to distinguish between old qemu and new one.
> >> I see now. But isn't it a good chance to introduce a proper generic
> >> interface for exploring supported fw-cfg keys?
> >>
> > Having such interface would be nice. Pity we haven't introduced it from
> > the start. If we do it now seabios will have to find out somehow that
> > qemu support such interface. Chicken and egg ;)
> 
> That is easy: Add a key the describes the highest supported key value
> (looks like this is monotonously increasing). Older qemu versions will
> return 0.
> 
That will not support holes in key space, and our key space is already
sparse.

--
			Gleb.

[Qemu-devel] Re: [PATCH] hpet: Clean up initial hpet counter

[Jan Kiszka]

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

Gleb Natapov wrote:
> On Wed, Jun 16, 2010 at 09:51:14AM +0200, Jan Kiszka wrote:
>> Gleb Natapov wrote:
>>> On Wed, Jun 16, 2010 at 09:03:01AM +0200, Jan Kiszka wrote:
>>>> Gleb Natapov wrote:
>>>>> On Wed, Jun 16, 2010 at 12:40:28AM +0200, Jan Kiszka wrote:
>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>
>>>>>> There is no need starting with the special value for hpet_cfg.count.
>>>>>> Either Seabios is aware of the new firmware interface and properly
>>>>>> interprets the counter or it simply ignores it anyway.
>>>>>>
>>>>> I want seabios to be able to distinguish between old qemu and new one.
>>>> I see now. But isn't it a good chance to introduce a proper generic
>>>> interface for exploring supported fw-cfg keys?
>>>>
>>> Having such interface would be nice. Pity we haven't introduced it from
>>> the start. If we do it now seabios will have to find out somehow that
>>> qemu support such interface. Chicken and egg ;)
>> That is easy: Add a key the describes the highest supported key value
>> (looks like this is monotonously increasing). Older qemu versions will
>> return 0.
>>
> That will not support holes in key space, and our key space is already
> sparse.

Then add a service to obtain a bitmap of supported keys. If that bitmap
is empty...

Jan


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

[Qemu-devel] Re: [PATCH] hpet: Clean up initial hpet counter

[Gleb Natapov]

On Wed, Jun 16, 2010 at 09:57:35AM +0200, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Wed, Jun 16, 2010 at 09:51:14AM +0200, Jan Kiszka wrote:
> >> Gleb Natapov wrote:
> >>> On Wed, Jun 16, 2010 at 09:03:01AM +0200, Jan Kiszka wrote:
> >>>> Gleb Natapov wrote:
> >>>>> On Wed, Jun 16, 2010 at 12:40:28AM +0200, Jan Kiszka wrote:
> >>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>>>
> >>>>>> There is no need starting with the special value for hpet_cfg.count.
> >>>>>> Either Seabios is aware of the new firmware interface and properly
> >>>>>> interprets the counter or it simply ignores it anyway.
> >>>>>>
> >>>>> I want seabios to be able to distinguish between old qemu and new one.
> >>>> I see now. But isn't it a good chance to introduce a proper generic
> >>>> interface for exploring supported fw-cfg keys?
> >>>>
> >>> Having such interface would be nice. Pity we haven't introduced it from
> >>> the start. If we do it now seabios will have to find out somehow that
> >>> qemu support such interface. Chicken and egg ;)
> >> That is easy: Add a key the describes the highest supported key value
> >> (looks like this is monotonously increasing). Older qemu versions will
> >> return 0.
> >>
> > That will not support holes in key space, and our key space is already
> > sparse.
> 
> Then add a service to obtain a bitmap of supported keys. If that bitmap
> is empty...
> 
Bitmap will be 2k long. We can add read capability to control port. To
check if key is present you select it (write its value to control port)
and then read control port back. If values is non-zero the key is valid.
But how to detect qemu that does not support that?

--
			Gleb.

[Qemu-devel] Re: [PATCH] hpet: Clean up initial hpet counter

[Jan Kiszka]

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

Gleb Natapov wrote:
> On Wed, Jun 16, 2010 at 09:57:35AM +0200, Jan Kiszka wrote:
>> Gleb Natapov wrote:
>>> On Wed, Jun 16, 2010 at 09:51:14AM +0200, Jan Kiszka wrote:
>>>> Gleb Natapov wrote:
>>>>> On Wed, Jun 16, 2010 at 09:03:01AM +0200, Jan Kiszka wrote:
>>>>>> Gleb Natapov wrote:
>>>>>>> On Wed, Jun 16, 2010 at 12:40:28AM +0200, Jan Kiszka wrote:
>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>>
>>>>>>>> There is no need starting with the special value for hpet_cfg.count.
>>>>>>>> Either Seabios is aware of the new firmware interface and properly
>>>>>>>> interprets the counter or it simply ignores it anyway.
>>>>>>>>
>>>>>>> I want seabios to be able to distinguish between old qemu and new one.
>>>>>> I see now. But isn't it a good chance to introduce a proper generic
>>>>>> interface for exploring supported fw-cfg keys?
>>>>>>
>>>>> Having such interface would be nice. Pity we haven't introduced it from
>>>>> the start. If we do it now seabios will have to find out somehow that
>>>>> qemu support such interface. Chicken and egg ;)
>>>> That is easy: Add a key the describes the highest supported key value
>>>> (looks like this is monotonously increasing). Older qemu versions will
>>>> return 0.
>>>>
>>> That will not support holes in key space, and our key space is already
>>> sparse.
>> Then add a service to obtain a bitmap of supported keys. If that bitmap
>> is empty...
>>
> Bitmap will be 2k long. We can add read capability to control port. To
> check if key is present you select it (write its value to control port)
> and then read control port back. If values is non-zero the key is valid.
> But how to detect qemu that does not support that?

Isn't there some key that was always there and will always be?

Jan



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

[Qemu-devel] Re: [PATCH] hpet: Clean up initial hpet counter

[Gleb Natapov]

On Wed, Jun 16, 2010 at 11:33:13AM +0200, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Wed, Jun 16, 2010 at 09:57:35AM +0200, Jan Kiszka wrote:
> >> Gleb Natapov wrote:
> >>> On Wed, Jun 16, 2010 at 09:51:14AM +0200, Jan Kiszka wrote:
> >>>> Gleb Natapov wrote:
> >>>>> On Wed, Jun 16, 2010 at 09:03:01AM +0200, Jan Kiszka wrote:
> >>>>>> Gleb Natapov wrote:
> >>>>>>> On Wed, Jun 16, 2010 at 12:40:28AM +0200, Jan Kiszka wrote:
> >>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>>>>>
> >>>>>>>> There is no need starting with the special value for hpet_cfg.count.
> >>>>>>>> Either Seabios is aware of the new firmware interface and properly
> >>>>>>>> interprets the counter or it simply ignores it anyway.
> >>>>>>>>
> >>>>>>> I want seabios to be able to distinguish between old qemu and new one.
> >>>>>> I see now. But isn't it a good chance to introduce a proper generic
> >>>>>> interface for exploring supported fw-cfg keys?
> >>>>>>
> >>>>> Having such interface would be nice. Pity we haven't introduced it from
> >>>>> the start. If we do it now seabios will have to find out somehow that
> >>>>> qemu support such interface. Chicken and egg ;)
> >>>> That is easy: Add a key the describes the highest supported key value
> >>>> (looks like this is monotonously increasing). Older qemu versions will
> >>>> return 0.
> >>>>
> >>> That will not support holes in key space, and our key space is already
> >>> sparse.
> >> Then add a service to obtain a bitmap of supported keys. If that bitmap
> >> is empty...
> >>
> > Bitmap will be 2k long. We can add read capability to control port. To
> > check if key is present you select it (write its value to control port)
> > and then read control port back. If values is non-zero the key is valid.
> > But how to detect qemu that does not support that?
> 
> Isn't there some key that was always there and will always be?
> 
FW_CFG_SIGNATURE

--
			Gleb.

Re: [Qemu-devel] Re: [PATCH] hpet: Clean up initial hpet counter

[Gleb Natapov]

On Wed, Jun 16, 2010 at 12:35:16PM +0300, Gleb Natapov wrote:
> On Wed, Jun 16, 2010 at 11:33:13AM +0200, Jan Kiszka wrote:
> > Gleb Natapov wrote:
> > > On Wed, Jun 16, 2010 at 09:57:35AM +0200, Jan Kiszka wrote:
> > >> Gleb Natapov wrote:
> > >>> On Wed, Jun 16, 2010 at 09:51:14AM +0200, Jan Kiszka wrote:
> > >>>> Gleb Natapov wrote:
> > >>>>> On Wed, Jun 16, 2010 at 09:03:01AM +0200, Jan Kiszka wrote:
> > >>>>>> Gleb Natapov wrote:
> > >>>>>>> On Wed, Jun 16, 2010 at 12:40:28AM +0200, Jan Kiszka wrote:
> > >>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
> > >>>>>>>>
> > >>>>>>>> There is no need starting with the special value for hpet_cfg.count.
> > >>>>>>>> Either Seabios is aware of the new firmware interface and properly
> > >>>>>>>> interprets the counter or it simply ignores it anyway.
> > >>>>>>>>
> > >>>>>>> I want seabios to be able to distinguish between old qemu and new one.
> > >>>>>> I see now. But isn't it a good chance to introduce a proper generic
> > >>>>>> interface for exploring supported fw-cfg keys?
> > >>>>>>
> > >>>>> Having such interface would be nice. Pity we haven't introduced it from
> > >>>>> the start. If we do it now seabios will have to find out somehow that
> > >>>>> qemu support such interface. Chicken and egg ;)
> > >>>> That is easy: Add a key the describes the highest supported key value
> > >>>> (looks like this is monotonously increasing). Older qemu versions will
> > >>>> return 0.
> > >>>>
> > >>> That will not support holes in key space, and our key space is already
> > >>> sparse.
> > >> Then add a service to obtain a bitmap of supported keys. If that bitmap
> > >> is empty...
> > >>
> > > Bitmap will be 2k long. We can add read capability to control port. To
> > > check if key is present you select it (write its value to control port)
> > > and then read control port back. If values is non-zero the key is valid.
> > > But how to detect qemu that does not support that?
> > 
> > Isn't there some key that was always there and will always be?
> > 
> FW_CFG_SIGNATURE
> 
So any ideas? Or did I misunderstood your hint? ;)

--
			Gleb.

Re: [Qemu-devel] Re: [PATCH] hpet: Clean up initial hpet counter

[Jan Kiszka]

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

Gleb Natapov wrote:
> On Wed, Jun 16, 2010 at 12:35:16PM +0300, Gleb Natapov wrote:
>> On Wed, Jun 16, 2010 at 11:33:13AM +0200, Jan Kiszka wrote:
>>> Gleb Natapov wrote:
>>>> On Wed, Jun 16, 2010 at 09:57:35AM +0200, Jan Kiszka wrote:
>>>>> Gleb Natapov wrote:
>>>>>> On Wed, Jun 16, 2010 at 09:51:14AM +0200, Jan Kiszka wrote:
>>>>>>> Gleb Natapov wrote:
>>>>>>>> On Wed, Jun 16, 2010 at 09:03:01AM +0200, Jan Kiszka wrote:
>>>>>>>>> Gleb Natapov wrote:
>>>>>>>>>> On Wed, Jun 16, 2010 at 12:40:28AM +0200, Jan Kiszka wrote:
>>>>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>>>>>
>>>>>>>>>>> There is no need starting with the special value for hpet_cfg.count.
>>>>>>>>>>> Either Seabios is aware of the new firmware interface and properly
>>>>>>>>>>> interprets the counter or it simply ignores it anyway.
>>>>>>>>>>>
>>>>>>>>>> I want seabios to be able to distinguish between old qemu and new one.
>>>>>>>>> I see now. But isn't it a good chance to introduce a proper generic
>>>>>>>>> interface for exploring supported fw-cfg keys?
>>>>>>>>>
>>>>>>>> Having such interface would be nice. Pity we haven't introduced it from
>>>>>>>> the start. If we do it now seabios will have to find out somehow that
>>>>>>>> qemu support such interface. Chicken and egg ;)
>>>>>>> That is easy: Add a key the describes the highest supported key value
>>>>>>> (looks like this is monotonously increasing). Older qemu versions will
>>>>>>> return 0.
>>>>>>>
>>>>>> That will not support holes in key space, and our key space is already
>>>>>> sparse.
>>>>> Then add a service to obtain a bitmap of supported keys. If that bitmap
>>>>> is empty...
>>>>>
>>>> Bitmap will be 2k long. We can add read capability to control port. To
>>>> check if key is present you select it (write its value to control port)
>>>> and then read control port back. If values is non-zero the key is valid.
>>>> But how to detect qemu that does not support that?
>>> Isn't there some key that was always there and will always be?
>>>
>> FW_CFG_SIGNATURE
>>
> So any ideas? Or did I misunderstood your hint? ;)

I thought you found the answer yourself:

Seabios could select FW_CFG_SIGNATURE and then perform a read-back on
the control register. Older QEMUs will return -1, versions that support
the read-back 0. Problem solved, no?

Jan


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

Re: [Qemu-devel] Re: [PATCH] hpet: Clean up initial hpet counter

[Gleb Natapov]

On Wed, Jun 16, 2010 at 06:00:56PM +0200, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Wed, Jun 16, 2010 at 12:35:16PM +0300, Gleb Natapov wrote:
> >> On Wed, Jun 16, 2010 at 11:33:13AM +0200, Jan Kiszka wrote:
> >>> Gleb Natapov wrote:
> >>>> On Wed, Jun 16, 2010 at 09:57:35AM +0200, Jan Kiszka wrote:
> >>>>> Gleb Natapov wrote:
> >>>>>> On Wed, Jun 16, 2010 at 09:51:14AM +0200, Jan Kiszka wrote:
> >>>>>>> Gleb Natapov wrote:
> >>>>>>>> On Wed, Jun 16, 2010 at 09:03:01AM +0200, Jan Kiszka wrote:
> >>>>>>>>> Gleb Natapov wrote:
> >>>>>>>>>> On Wed, Jun 16, 2010 at 12:40:28AM +0200, Jan Kiszka wrote:
> >>>>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>>>>>>>>
> >>>>>>>>>>> There is no need starting with the special value for hpet_cfg.count.
> >>>>>>>>>>> Either Seabios is aware of the new firmware interface and properly
> >>>>>>>>>>> interprets the counter or it simply ignores it anyway.
> >>>>>>>>>>>
> >>>>>>>>>> I want seabios to be able to distinguish between old qemu and new one.
> >>>>>>>>> I see now. But isn't it a good chance to introduce a proper generic
> >>>>>>>>> interface for exploring supported fw-cfg keys?
> >>>>>>>>>
> >>>>>>>> Having such interface would be nice. Pity we haven't introduced it from
> >>>>>>>> the start. If we do it now seabios will have to find out somehow that
> >>>>>>>> qemu support such interface. Chicken and egg ;)
> >>>>>>> That is easy: Add a key the describes the highest supported key value
> >>>>>>> (looks like this is monotonously increasing). Older qemu versions will
> >>>>>>> return 0.
> >>>>>>>
> >>>>>> That will not support holes in key space, and our key space is already
> >>>>>> sparse.
> >>>>> Then add a service to obtain a bitmap of supported keys. If that bitmap
> >>>>> is empty...
> >>>>>
> >>>> Bitmap will be 2k long. We can add read capability to control port. To
> >>>> check if key is present you select it (write its value to control port)
> >>>> and then read control port back. If values is non-zero the key is valid.
> >>>> But how to detect qemu that does not support that?
> >>> Isn't there some key that was always there and will always be?
> >>>
> >> FW_CFG_SIGNATURE
> >>
> > So any ideas? Or did I misunderstood your hint? ;)
> 
> I thought you found the answer yourself:
> 
> Seabios could select FW_CFG_SIGNATURE and then perform a read-back on
> the control register. Older QEMUs will return -1, versions that support
> the read-back 0. Problem solved, no?
> 
AFAIK QEMU returns 0 if io read was done from non-used port or mmio
address, but can we rely on this? If we can then problem solved, if
we can't then no.

--
			Gleb.

Re: [Qemu-devel] Re: [PATCH] hpet: Clean up initial hpet counter

[Jan Kiszka]

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

Gleb Natapov wrote:
> On Wed, Jun 16, 2010 at 06:00:56PM +0200, Jan Kiszka wrote:
>> Gleb Natapov wrote:
>>> On Wed, Jun 16, 2010 at 12:35:16PM +0300, Gleb Natapov wrote:
>>>> On Wed, Jun 16, 2010 at 11:33:13AM +0200, Jan Kiszka wrote:
>>>>> Gleb Natapov wrote:
>>>>>> On Wed, Jun 16, 2010 at 09:57:35AM +0200, Jan Kiszka wrote:
>>>>>>> Gleb Natapov wrote:
>>>>>>>> On Wed, Jun 16, 2010 at 09:51:14AM +0200, Jan Kiszka wrote:
>>>>>>>>> Gleb Natapov wrote:
>>>>>>>>>> On Wed, Jun 16, 2010 at 09:03:01AM +0200, Jan Kiszka wrote:
>>>>>>>>>>> Gleb Natapov wrote:
>>>>>>>>>>>> On Wed, Jun 16, 2010 at 12:40:28AM +0200, Jan Kiszka wrote:
>>>>>>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>>>>>>>
>>>>>>>>>>>>> There is no need starting with the special value for hpet_cfg.count.
>>>>>>>>>>>>> Either Seabios is aware of the new firmware interface and properly
>>>>>>>>>>>>> interprets the counter or it simply ignores it anyway.
>>>>>>>>>>>>>
>>>>>>>>>>>> I want seabios to be able to distinguish between old qemu and new one.
>>>>>>>>>>> I see now. But isn't it a good chance to introduce a proper generic
>>>>>>>>>>> interface for exploring supported fw-cfg keys?
>>>>>>>>>>>
>>>>>>>>>> Having such interface would be nice. Pity we haven't introduced it from
>>>>>>>>>> the start. If we do it now seabios will have to find out somehow that
>>>>>>>>>> qemu support such interface. Chicken and egg ;)
>>>>>>>>> That is easy: Add a key the describes the highest supported key value
>>>>>>>>> (looks like this is monotonously increasing). Older qemu versions will
>>>>>>>>> return 0.
>>>>>>>>>
>>>>>>>> That will not support holes in key space, and our key space is already
>>>>>>>> sparse.
>>>>>>> Then add a service to obtain a bitmap of supported keys. If that bitmap
>>>>>>> is empty...
>>>>>>>
>>>>>> Bitmap will be 2k long. We can add read capability to control port. To
>>>>>> check if key is present you select it (write its value to control port)
>>>>>> and then read control port back. If values is non-zero the key is valid.
>>>>>> But how to detect qemu that does not support that?
>>>>> Isn't there some key that was always there and will always be?
>>>>>
>>>> FW_CFG_SIGNATURE
>>>>
>>> So any ideas? Or did I misunderstood your hint? ;)
>> I thought you found the answer yourself:
>>
>> Seabios could select FW_CFG_SIGNATURE and then perform a read-back on
>> the control register. Older QEMUs will return -1, versions that support
>> the read-back 0. Problem solved, no?
>>
> AFAIK QEMU returns 0 if io read was done from non-used port or mmio
> address, but can we rely on this? If we can then problem solved, if
> we can't then no.

It works for IO-based fw-cfg, but not for MMIO-based. So the firmware
should probably pick a non-zero key for this check, e.g. FW_CFG_ID.

Jan


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

Re: [Qemu-devel] Re: [PATCH] hpet: Clean up initial hpet counter

[Gleb Natapov]

On Thu, Jun 17, 2010 at 09:17:51AM +0200, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Wed, Jun 16, 2010 at 06:00:56PM +0200, Jan Kiszka wrote:
> >> Gleb Natapov wrote:
> >>> On Wed, Jun 16, 2010 at 12:35:16PM +0300, Gleb Natapov wrote:
> >>>> On Wed, Jun 16, 2010 at 11:33:13AM +0200, Jan Kiszka wrote:
> >>>>> Gleb Natapov wrote:
> >>>>>> On Wed, Jun 16, 2010 at 09:57:35AM +0200, Jan Kiszka wrote:
> >>>>>>> Gleb Natapov wrote:
> >>>>>>>> On Wed, Jun 16, 2010 at 09:51:14AM +0200, Jan Kiszka wrote:
> >>>>>>>>> Gleb Natapov wrote:
> >>>>>>>>>> On Wed, Jun 16, 2010 at 09:03:01AM +0200, Jan Kiszka wrote:
> >>>>>>>>>>> Gleb Natapov wrote:
> >>>>>>>>>>>> On Wed, Jun 16, 2010 at 12:40:28AM +0200, Jan Kiszka wrote:
> >>>>>>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> There is no need starting with the special value for hpet_cfg.count.
> >>>>>>>>>>>>> Either Seabios is aware of the new firmware interface and properly
> >>>>>>>>>>>>> interprets the counter or it simply ignores it anyway.
> >>>>>>>>>>>>>
> >>>>>>>>>>>> I want seabios to be able to distinguish between old qemu and new one.
> >>>>>>>>>>> I see now. But isn't it a good chance to introduce a proper generic
> >>>>>>>>>>> interface for exploring supported fw-cfg keys?
> >>>>>>>>>>>
> >>>>>>>>>> Having such interface would be nice. Pity we haven't introduced it from
> >>>>>>>>>> the start. If we do it now seabios will have to find out somehow that
> >>>>>>>>>> qemu support such interface. Chicken and egg ;)
> >>>>>>>>> That is easy: Add a key the describes the highest supported key value
> >>>>>>>>> (looks like this is monotonously increasing). Older qemu versions will
> >>>>>>>>> return 0.
> >>>>>>>>>
> >>>>>>>> That will not support holes in key space, and our key space is already
> >>>>>>>> sparse.
> >>>>>>> Then add a service to obtain a bitmap of supported keys. If that bitmap
> >>>>>>> is empty...
> >>>>>>>
> >>>>>> Bitmap will be 2k long. We can add read capability to control port. To
> >>>>>> check if key is present you select it (write its value to control port)
> >>>>>> and then read control port back. If values is non-zero the key is valid.
> >>>>>> But how to detect qemu that does not support that?
> >>>>> Isn't there some key that was always there and will always be?
> >>>>>
> >>>> FW_CFG_SIGNATURE
> >>>>
> >>> So any ideas? Or did I misunderstood your hint? ;)
> >> I thought you found the answer yourself:
> >>
> >> Seabios could select FW_CFG_SIGNATURE and then perform a read-back on
> >> the control register. Older QEMUs will return -1, versions that support
> >> the read-back 0. Problem solved, no?
> >>
> > AFAIK QEMU returns 0 if io read was done from non-used port or mmio
> > address, but can we rely on this? If we can then problem solved, if
> > we can't then no.
> 
> It works for IO-based fw-cfg, but not for MMIO-based. So the firmware
> should probably pick a non-zero key for this check, e.g. FW_CFG_ID.
> 
Sorry, I lost you here. What "works for IO-based fw-cfg, but not for
MMIO-based". Can you write pseudo logic of how you think it
all should work?

--
			Gleb.

Re: [Qemu-devel] Re: [PATCH] hpet: Clean up initial hpet counter

[Jan Kiszka]

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

Gleb Natapov wrote:
> On Thu, Jun 17, 2010 at 09:17:51AM +0200, Jan Kiszka wrote:
>> Gleb Natapov wrote:
>>> On Wed, Jun 16, 2010 at 06:00:56PM +0200, Jan Kiszka wrote:
>>>> Gleb Natapov wrote:
>>>>> On Wed, Jun 16, 2010 at 12:35:16PM +0300, Gleb Natapov wrote:
>>>>>> On Wed, Jun 16, 2010 at 11:33:13AM +0200, Jan Kiszka wrote:
>>>>>>> Gleb Natapov wrote:
>>>>>>>> On Wed, Jun 16, 2010 at 09:57:35AM +0200, Jan Kiszka wrote:
>>>>>>>>> Gleb Natapov wrote:
>>>>>>>>>> On Wed, Jun 16, 2010 at 09:51:14AM +0200, Jan Kiszka wrote:
>>>>>>>>>>> Gleb Natapov wrote:
>>>>>>>>>>>> On Wed, Jun 16, 2010 at 09:03:01AM +0200, Jan Kiszka wrote:
>>>>>>>>>>>>> Gleb Natapov wrote:
>>>>>>>>>>>>>> On Wed, Jun 16, 2010 at 12:40:28AM +0200, Jan Kiszka wrote:
>>>>>>>>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> There is no need starting with the special value for hpet_cfg.count.
>>>>>>>>>>>>>>> Either Seabios is aware of the new firmware interface and properly
>>>>>>>>>>>>>>> interprets the counter or it simply ignores it anyway.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I want seabios to be able to distinguish between old qemu and new one.
>>>>>>>>>>>>> I see now. But isn't it a good chance to introduce a proper generic
>>>>>>>>>>>>> interface for exploring supported fw-cfg keys?
>>>>>>>>>>>>>
>>>>>>>>>>>> Having such interface would be nice. Pity we haven't introduced it from
>>>>>>>>>>>> the start. If we do it now seabios will have to find out somehow that
>>>>>>>>>>>> qemu support such interface. Chicken and egg ;)
>>>>>>>>>>> That is easy: Add a key the describes the highest supported key value
>>>>>>>>>>> (looks like this is monotonously increasing). Older qemu versions will
>>>>>>>>>>> return 0.
>>>>>>>>>>>
>>>>>>>>>> That will not support holes in key space, and our key space is already
>>>>>>>>>> sparse.
>>>>>>>>> Then add a service to obtain a bitmap of supported keys. If that bitmap
>>>>>>>>> is empty...
>>>>>>>>>
>>>>>>>> Bitmap will be 2k long. We can add read capability to control port. To
>>>>>>>> check if key is present you select it (write its value to control port)
>>>>>>>> and then read control port back. If values is non-zero the key is valid.
>>>>>>>> But how to detect qemu that does not support that?
>>>>>>> Isn't there some key that was always there and will always be?
>>>>>>>
>>>>>> FW_CFG_SIGNATURE
>>>>>>
>>>>> So any ideas? Or did I misunderstood your hint? ;)
>>>> I thought you found the answer yourself:
>>>>
>>>> Seabios could select FW_CFG_SIGNATURE and then perform a read-back on
>>>> the control register. Older QEMUs will return -1, versions that support
>>>> the read-back 0. Problem solved, no?
>>>>
>>> AFAIK QEMU returns 0 if io read was done from non-used port or mmio
>>> address, but can we rely on this? If we can then problem solved, if
>>> we can't then no.
>> It works for IO-based fw-cfg, but not for MMIO-based. So the firmware
>> should probably pick a non-zero key for this check, e.g. FW_CFG_ID.
>>
> Sorry, I lost you here. What "works for IO-based fw-cfg, but not for
> MMIO-based".

Undefined IO ports return -1, undefined (/wrt read access) MMIO 0. So
you need to select a key that is different from both.

> Can you write pseudo logic of how you think it
> all should work?

The firmware should do this:

write(CTL_BASE, FW_CFG_ID);
if (read(CTL_BASE) != FW_CFG_ID)
	deal_with_old_qemu();
else
	check_for_supported_keys();

Jan


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

Re: [Qemu-devel] Re: [PATCH] hpet: Clean up initial hpet counter

[Gleb Natapov]

On Thu, Jun 17, 2010 at 10:30:15AM +0200, Jan Kiszka wrote:
> > Sorry, I lost you here. What "works for IO-based fw-cfg, but not for
> > MMIO-based".
> 
> Undefined IO ports return -1, undefined (/wrt read access) MMIO 0. So
> you need to select a key that is different from both.
> 
But can we rely on it? Is this defined somewhere or if it happens to be
the case in current qemu for x86 arch.

> > Can you write pseudo logic of how you think it
> > all should work?
> 
> The firmware should do this:
> 
> write(CTL_BASE, FW_CFG_ID);
> if (read(CTL_BASE) != FW_CFG_ID)
> 	deal_with_old_qemu();
> else
> 	check_for_supported_keys();
> 
Ah, I thought about read() returning 0/1, not key itself, so any key that
always existed would do.

--
			Gleb.

Re: [Qemu-devel] Re: [PATCH] hpet: Clean up initial hpet counter

[Jan Kiszka]

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

Gleb Natapov wrote:
> On Thu, Jun 17, 2010 at 10:30:15AM +0200, Jan Kiszka wrote:
>>> Sorry, I lost you here. What "works for IO-based fw-cfg, but not for
>>> MMIO-based".
>> Undefined IO ports return -1, undefined (/wrt read access) MMIO 0. So
>> you need to select a key that is different from both.
>>
> But can we rely on it? Is this defined somewhere or if it happens to be
> the case in current qemu for x86 arch.

For x86 with its port-based access, we are on the safe side as (pre-pnp)
device probing used to work this way. Can't tell for the other archs
that support fw-cfg.

> 
>>> Can you write pseudo logic of how you think it
>>> all should work?
>> The firmware should do this:
>>
>> write(CTL_BASE, FW_CFG_ID);
>> if (read(CTL_BASE) != FW_CFG_ID)
>> 	deal_with_old_qemu();
>> else
>> 	check_for_supported_keys();
>>
> Ah, I thought about read() returning 0/1, not key itself, so any key that
> always existed would do.

Yes, read-back would mean returning FWCfgState::cur_entry. And that will
be -1 when selected an invalid one.

Jan


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

Re: [Qemu-devel] Re: [PATCH] hpet: Clean up initial hpet counter

[Gleb Natapov]

On Thu, Jun 17, 2010 at 10:42:34AM +0200, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Thu, Jun 17, 2010 at 10:30:15AM +0200, Jan Kiszka wrote:
> >>> Sorry, I lost you here. What "works for IO-based fw-cfg, but not for
> >>> MMIO-based".
> >> Undefined IO ports return -1, undefined (/wrt read access) MMIO 0. So
> >> you need to select a key that is different from both.
> >>
> > But can we rely on it? Is this defined somewhere or if it happens to be
> > the case in current qemu for x86 arch.
> 
> For x86 with its port-based access, we are on the safe side as (pre-pnp)
> device probing used to work this way. Can't tell for the other archs
> that support fw-cfg.
> 
> > 
> >>> Can you write pseudo logic of how you think it
> >>> all should work?
> >> The firmware should do this:
> >>
> >> write(CTL_BASE, FW_CFG_ID);
> >> if (read(CTL_BASE) != FW_CFG_ID)
> >> 	deal_with_old_qemu();
> >> else
> >> 	check_for_supported_keys();
> >>
> > Ah, I thought about read() returning 0/1, not key itself, so any key that
> > always existed would do.
> 
> Yes, read-back would mean returning FWCfgState::cur_entry. And that will
> be -1 when selected an invalid one.
> 
Heh, actually I have better idea. Why not advance FW_CFG_ID to version 2.

--
			Gleb.

Re: [Qemu-devel] Re: [PATCH] hpet: Clean up initial hpet counter

[Jan Kiszka]

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

Gleb Natapov wrote:
> On Thu, Jun 17, 2010 at 10:42:34AM +0200, Jan Kiszka wrote:
>> Gleb Natapov wrote:
>>> On Thu, Jun 17, 2010 at 10:30:15AM +0200, Jan Kiszka wrote:
>>>>> Sorry, I lost you here. What "works for IO-based fw-cfg, but not for
>>>>> MMIO-based".
>>>> Undefined IO ports return -1, undefined (/wrt read access) MMIO 0. So
>>>> you need to select a key that is different from both.
>>>>
>>> But can we rely on it? Is this defined somewhere or if it happens to be
>>> the case in current qemu for x86 arch.
>> For x86 with its port-based access, we are on the safe side as (pre-pnp)
>> device probing used to work this way. Can't tell for the other archs
>> that support fw-cfg.
>>
>>>>> Can you write pseudo logic of how you think it
>>>>> all should work?
>>>> The firmware should do this:
>>>>
>>>> write(CTL_BASE, FW_CFG_ID);
>>>> if (read(CTL_BASE) != FW_CFG_ID)
>>>> 	deal_with_old_qemu();
>>>> else
>>>> 	check_for_supported_keys();
>>>>
>>> Ah, I thought about read() returning 0/1, not key itself, so any key that
>>> always existed would do.
>> Yes, read-back would mean returning FWCfgState::cur_entry. And that will
>> be -1 when selected an invalid one.
>>
> Heh, actually I have better idea. Why not advance FW_CFG_ID to version 2.

If that is supposed to be a version number - yeah, good idea.

Jan


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

Re: [Qemu-devel] Re: [PATCH] hpet: Clean up initial hpet counter

[Gleb Natapov]

On Thu, Jun 17, 2010 at 10:59:01AM +0200, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Thu, Jun 17, 2010 at 10:42:34AM +0200, Jan Kiszka wrote:
> >> Gleb Natapov wrote:
> >>> On Thu, Jun 17, 2010 at 10:30:15AM +0200, Jan Kiszka wrote:
> >>>>> Sorry, I lost you here. What "works for IO-based fw-cfg, but not for
> >>>>> MMIO-based".
> >>>> Undefined IO ports return -1, undefined (/wrt read access) MMIO 0. So
> >>>> you need to select a key that is different from both.
> >>>>
> >>> But can we rely on it? Is this defined somewhere or if it happens to be
> >>> the case in current qemu for x86 arch.
> >> For x86 with its port-based access, we are on the safe side as (pre-pnp)
> >> device probing used to work this way. Can't tell for the other archs
> >> that support fw-cfg.
> >>
> >>>>> Can you write pseudo logic of how you think it
> >>>>> all should work?
> >>>> The firmware should do this:
> >>>>
> >>>> write(CTL_BASE, FW_CFG_ID);
> >>>> if (read(CTL_BASE) != FW_CFG_ID)
> >>>> 	deal_with_old_qemu();
> >>>> else
> >>>> 	check_for_supported_keys();
> >>>>
> >>> Ah, I thought about read() returning 0/1, not key itself, so any key that
> >>> always existed would do.
> >> Yes, read-back would mean returning FWCfgState::cur_entry. And that will
> >> be -1 when selected an invalid one.
> >>
> > Heh, actually I have better idea. Why not advance FW_CFG_ID to version 2.
> 
> If that is supposed to be a version number - yeah, good idea.
> 
That was the idea behind it. I just forgot it exists.

--
			Gleb.