Patchwork [v3,2/7] hpet: Save/restore cached RTC IRQ level

login
register
mail settings
Submitter Jan Kiszka
Date Jan. 31, 2012, 5:41 p.m.
Message ID <1e517ab259c42885b0dbb49ea13db92f2df45815.1328031687.git.jan.kiszka@siemens.com>
Download mbox | patch
Permalink /patch/138818/
State New
Headers show

Comments

Jan Kiszka - Jan. 31, 2012, 5:41 p.m.
In legacy mode, the HPET suppresses the RTC interrupt delivery via IRQ
8 but keeps track of the RTC output level and applies it when legacy
mode is turned off again. This value has to be preserved across save/
restore as it cannot be reconstructed otherwise.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/hpet.c |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)
Anthony Liguori - Jan. 31, 2012, 9:02 p.m.
On 01/31/2012 11:41 AM, Jan Kiszka wrote:
> In legacy mode, the HPET suppresses the RTC interrupt delivery via IRQ
> 8 but keeps track of the RTC output level and applies it when legacy
> mode is turned off again. This value has to be preserved across save/
> restore as it cannot be reconstructed otherwise.
>
> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
> ---
>   hw/hpet.c |   26 ++++++++++++++++++++++++++
>   1 files changed, 26 insertions(+), 0 deletions(-)
>
> diff --git a/hw/hpet.c b/hw/hpet.c
> index aba9ea9..39b291f 100644
> --- a/hw/hpet.c
> +++ b/hw/hpet.c
> @@ -240,6 +240,24 @@ static int hpet_post_load(void *opaque, int version_id)
>       return 0;
>   }
>
> +static bool hpet_rtc_irq_level_needed(void *opaque)
> +{
> +    HPETState *s = opaque;
> +
> +    return s->rtc_irq_level != 0;
> +}
> +
> +static const VMStateDescription vmstate_hpet_rtc_irq_level = {
> +    .name = "hpet/rtc_irq_level",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields      = (VMStateField[]) {
> +        VMSTATE_UINT8(rtc_irq_level, HPETState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +

This won't work.  We don't clear rtc_irq_level on reset so rtc_irq_level may be 
high or low after reset.  As such, we can't use a subsection here.  We need to 
bump the savevm state.

Regards,

Anthony Liguori

>   static const VMStateDescription vmstate_hpet_timer = {
>       .name = "hpet_timer",
>       .version_id = 1,
> @@ -273,6 +291,14 @@ static const VMStateDescription vmstate_hpet = {
>           VMSTATE_STRUCT_VARRAY_UINT8(timer, HPETState, num_timers, 0,
>                                       vmstate_hpet_timer, HPETTimer),
>           VMSTATE_END_OF_LIST()
> +    },
> +    .subsections = (VMStateSubsection[]) {
> +        {
> +            .vmsd =&vmstate_hpet_rtc_irq_level,
> +            .needed = hpet_rtc_irq_level_needed,
> +        }, {
> +            /* empty */
> +        }
>       }
>   };
>
Jan Kiszka - Jan. 31, 2012, 10:05 p.m.
On 2012-01-31 22:02, Anthony Liguori wrote:
> On 01/31/2012 11:41 AM, Jan Kiszka wrote:
>> In legacy mode, the HPET suppresses the RTC interrupt delivery via IRQ
>> 8 but keeps track of the RTC output level and applies it when legacy
>> mode is turned off again. This value has to be preserved across save/
>> restore as it cannot be reconstructed otherwise.
>>
>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>> ---
>>   hw/hpet.c |   26 ++++++++++++++++++++++++++
>>   1 files changed, 26 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/hpet.c b/hw/hpet.c
>> index aba9ea9..39b291f 100644
>> --- a/hw/hpet.c
>> +++ b/hw/hpet.c
>> @@ -240,6 +240,24 @@ static int hpet_post_load(void *opaque, int
>> version_id)
>>       return 0;
>>   }
>>
>> +static bool hpet_rtc_irq_level_needed(void *opaque)
>> +{
>> +    HPETState *s = opaque;
>> +
>> +    return s->rtc_irq_level != 0;
>> +}
>> +
>> +static const VMStateDescription vmstate_hpet_rtc_irq_level = {
>> +    .name = "hpet/rtc_irq_level",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .minimum_version_id_old = 1,
>> +    .fields      = (VMStateField[]) {
>> +        VMSTATE_UINT8(rtc_irq_level, HPETState),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
> 
> This won't work.  We don't clear rtc_irq_level on reset so rtc_irq_level
> may be high or low after reset.  As such, we can't use a subsection
> here.  We need to bump the savevm state.

rtc_irq_level is updated during reset by the connected RTC device. It
clears its IRQ line.

Jan
Anthony Liguori - Jan. 31, 2012, 10:38 p.m.
On 01/31/2012 04:05 PM, Jan Kiszka wrote:
> On 2012-01-31 22:02, Anthony Liguori wrote:
>> On 01/31/2012 11:41 AM, Jan Kiszka wrote:
>>> In legacy mode, the HPET suppresses the RTC interrupt delivery via IRQ
>>> 8 but keeps track of the RTC output level and applies it when legacy
>>> mode is turned off again. This value has to be preserved across save/
>>> restore as it cannot be reconstructed otherwise.
>>>
>>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>>> ---
>>>    hw/hpet.c |   26 ++++++++++++++++++++++++++
>>>    1 files changed, 26 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/hw/hpet.c b/hw/hpet.c
>>> index aba9ea9..39b291f 100644
>>> --- a/hw/hpet.c
>>> +++ b/hw/hpet.c
>>> @@ -240,6 +240,24 @@ static int hpet_post_load(void *opaque, int
>>> version_id)
>>>        return 0;
>>>    }
>>>
>>> +static bool hpet_rtc_irq_level_needed(void *opaque)
>>> +{
>>> +    HPETState *s = opaque;
>>> +
>>> +    return s->rtc_irq_level != 0;
>>> +}
>>> +
>>> +static const VMStateDescription vmstate_hpet_rtc_irq_level = {
>>> +    .name = "hpet/rtc_irq_level",
>>> +    .version_id = 1,
>>> +    .minimum_version_id = 1,
>>> +    .minimum_version_id_old = 1,
>>> +    .fields      = (VMStateField[]) {
>>> +        VMSTATE_UINT8(rtc_irq_level, HPETState),
>>> +        VMSTATE_END_OF_LIST()
>>> +    }
>>> +};
>>> +
>>
>> This won't work.  We don't clear rtc_irq_level on reset so rtc_irq_level
>> may be high or low after reset.  As such, we can't use a subsection
>> here.  We need to bump the savevm state.
>
> rtc_irq_level is updated during reset by the connected RTC device. It
> clears its IRQ line.

Ah, that's subtle and warrants a comment :-)

Probably worth adding an s->rtc_irq_level = 0 in reset just to be extra safe.

Regards,

Anthony Liguori

>
> Jan
Jan Kiszka - Jan. 31, 2012, 10:39 p.m.
On 2012-01-31 23:38, Anthony Liguori wrote:
> On 01/31/2012 04:05 PM, Jan Kiszka wrote:
>> On 2012-01-31 22:02, Anthony Liguori wrote:
>>> On 01/31/2012 11:41 AM, Jan Kiszka wrote:
>>>> In legacy mode, the HPET suppresses the RTC interrupt delivery via IRQ
>>>> 8 but keeps track of the RTC output level and applies it when legacy
>>>> mode is turned off again. This value has to be preserved across save/
>>>> restore as it cannot be reconstructed otherwise.
>>>>
>>>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>>>> ---
>>>>    hw/hpet.c |   26 ++++++++++++++++++++++++++
>>>>    1 files changed, 26 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/hw/hpet.c b/hw/hpet.c
>>>> index aba9ea9..39b291f 100644
>>>> --- a/hw/hpet.c
>>>> +++ b/hw/hpet.c
>>>> @@ -240,6 +240,24 @@ static int hpet_post_load(void *opaque, int
>>>> version_id)
>>>>        return 0;
>>>>    }
>>>>
>>>> +static bool hpet_rtc_irq_level_needed(void *opaque)
>>>> +{
>>>> +    HPETState *s = opaque;
>>>> +
>>>> +    return s->rtc_irq_level != 0;
>>>> +}
>>>> +
>>>> +static const VMStateDescription vmstate_hpet_rtc_irq_level = {
>>>> +    .name = "hpet/rtc_irq_level",
>>>> +    .version_id = 1,
>>>> +    .minimum_version_id = 1,
>>>> +    .minimum_version_id_old = 1,
>>>> +    .fields      = (VMStateField[]) {
>>>> +        VMSTATE_UINT8(rtc_irq_level, HPETState),
>>>> +        VMSTATE_END_OF_LIST()
>>>> +    }
>>>> +};
>>>> +
>>>
>>> This won't work.  We don't clear rtc_irq_level on reset so rtc_irq_level
>>> may be high or low after reset.  As such, we can't use a subsection
>>> here.  We need to bump the savevm state.
>>
>> rtc_irq_level is updated during reset by the connected RTC device. It
>> clears its IRQ line.
> 
> Ah, that's subtle and warrants a comment :-)
> 
> Probably worth adding an s->rtc_irq_level = 0 in reset just to be extra
> safe.

Agreed. Will do.

Thanks,
Jan

Patch

diff --git a/hw/hpet.c b/hw/hpet.c
index aba9ea9..39b291f 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -240,6 +240,24 @@  static int hpet_post_load(void *opaque, int version_id)
     return 0;
 }
 
+static bool hpet_rtc_irq_level_needed(void *opaque)
+{
+    HPETState *s = opaque;
+
+    return s->rtc_irq_level != 0;
+}
+
+static const VMStateDescription vmstate_hpet_rtc_irq_level = {
+    .name = "hpet/rtc_irq_level",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField[]) {
+        VMSTATE_UINT8(rtc_irq_level, HPETState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_hpet_timer = {
     .name = "hpet_timer",
     .version_id = 1,
@@ -273,6 +291,14 @@  static const VMStateDescription vmstate_hpet = {
         VMSTATE_STRUCT_VARRAY_UINT8(timer, HPETState, num_timers, 0,
                                     vmstate_hpet_timer, HPETTimer),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (VMStateSubsection[]) {
+        {
+            .vmsd = &vmstate_hpet_rtc_irq_level,
+            .needed = hpet_rtc_irq_level_needed,
+        }, {
+            /* empty */
+        }
     }
 };