All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [Bug 1080086] [NEW] MC146818 RTC breaks when SET bit in Register B is on.
@ 2012-11-17 14:41 Alex Horn
  2012-11-17 18:47 ` [Qemu-devel] [Bug 1080086] " Alex Horn
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Alex Horn @ 2012-11-17 14:41 UTC (permalink / raw)
  To: qemu-devel

Public bug reported:

This bug occurs when the SET flag of Register B is enabled. When an RTC
data register (i.e. any of the 10 bytes of time/calender data in CMOS) is set,
the data is (as expected) correctly stored in the cmos_data array. However,
since the SET flag is enabled, the function rtc_set_time is not invoked.
As a result, the field base_rtc in RTCState remains uninitialized. This appears to
cause a problem on subsequent writes which can end up overwriting data.

To see this, consider writing data to Register A after having written
data to any of the RTC data registers; the following figure illustrates
the call stack for the Register A write operation:

 +- cmos_io_port_write
 +-- check_update_timer
 +---- get_next_alarm
 +------ rtc_update_time

In rtc_update_time, get_guest_rtc calculates the wrong time and
overwrites the previously written RTC data register values.

I have created a standalone test case which exposes this bug:

https://github.com/ahorn/benchmarks/commit/fff1ca40694bbef6f7f9de323bb0bed63419ef99

** Affects: qemu
     Importance: Undecided
         Status: New


** Tags: rtc

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1080086

Title:
  MC146818 RTC breaks when SET bit in Register B is on.

Status in QEMU:
  New

Bug description:
  This bug occurs when the SET flag of Register B is enabled. When an RTC
  data register (i.e. any of the 10 bytes of time/calender data in CMOS) is set,
  the data is (as expected) correctly stored in the cmos_data array. However,
  since the SET flag is enabled, the function rtc_set_time is not invoked.
  As a result, the field base_rtc in RTCState remains uninitialized. This appears to
  cause a problem on subsequent writes which can end up overwriting data.

  To see this, consider writing data to Register A after having written
  data to any of the RTC data registers; the following figure illustrates
  the call stack for the Register A write operation:

   +- cmos_io_port_write
   +-- check_update_timer
   +---- get_next_alarm
   +------ rtc_update_time

  In rtc_update_time, get_guest_rtc calculates the wrong time and
  overwrites the previously written RTC data register values.

  I have created a standalone test case which exposes this bug:

  https://github.com/ahorn/benchmarks/commit/fff1ca40694bbef6f7f9de323bb0bed63419ef99

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1080086/+subscriptions

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Qemu-devel] [Bug 1080086] Re: MC146818 RTC breaks when SET bit in Register B is on.
  2012-11-17 14:41 [Qemu-devel] [Bug 1080086] [NEW] MC146818 RTC breaks when SET bit in Register B is on Alex Horn
@ 2012-11-17 18:47 ` Alex Horn
  2012-11-18  8:52   ` Paolo Bonzini
  2012-11-19 12:12 ` Alex Horn
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Alex Horn @ 2012-11-17 18:47 UTC (permalink / raw)
  To: qemu-devel

I have attached a patch for the most recent version of the file
hw/mc146818rtc.c [1]. The patch also features a functional test which
executes through the QTest framework.

I would appreciate your thoughts on this.

[1]
http://git.qemu.org/?p=qemu.git;a=blob;f=hw/mc146818rtc.c;h=98839f278d93452d071054e2a017b3d909b45ab2;hb=9cb535fe4ef08b01e583ec955767a0899ff79afe#l563

** Patch added: "register_b_set_flag.patch"
   https://bugs.launchpad.net/qemu/+bug/1080086/+attachment/3436808/+files/register_b_set_flag.patch

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1080086

Title:
  MC146818 RTC breaks when SET bit in Register B is on.

Status in QEMU:
  New

Bug description:
  This bug occurs when the SET flag of Register B is enabled. When an RTC
  data register (i.e. any of the 10 bytes of time/calender data in CMOS) is set,
  the data is (as expected) correctly stored in the cmos_data array. However,
  since the SET flag is enabled, the function rtc_set_time is not invoked.
  As a result, the field base_rtc in RTCState remains uninitialized. This appears to
  cause a problem on subsequent writes which can end up overwriting data.

  To see this, consider writing data to Register A after having written
  data to any of the RTC data registers; the following figure illustrates
  the call stack for the Register A write operation:

   +- cmos_io_port_write
   +-- check_update_timer
   +---- get_next_alarm
   +------ rtc_update_time

  In rtc_update_time, get_guest_rtc calculates the wrong time and
  overwrites the previously written RTC data register values.

  I have created a standalone test case which exposes this bug:

  https://github.com/ahorn/benchmarks/commit/fff1ca40694bbef6f7f9de323bb0bed63419ef99

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1080086/+subscriptions

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [Bug 1080086] Re: MC146818 RTC breaks when SET bit in Register B is on.
  2012-11-17 18:47 ` [Qemu-devel] [Bug 1080086] " Alex Horn
@ 2012-11-18  8:52   ` Paolo Bonzini
  2012-11-19 11:34     ` Alex Horn
  0 siblings, 1 reply; 10+ messages in thread
From: Paolo Bonzini @ 2012-11-18  8:52 UTC (permalink / raw)
  To: Bug 1080086; +Cc: Alex Horn, qemu-devel

Il 17/11/2012 19:47, Alex Horn ha scritto:
> I have attached a patch for the most recent version of the file
> hw/mc146818rtc.c [1]. The patch also features a functional test which
> executes through the QTest framework.
> 
> I would appreciate your thoughts on this.
> 
> [1]
> http://git.qemu.org/?p=qemu.git;a=blob;f=hw/mc146818rtc.c;h=98839f278d93452d071054e2a017b3d909b45ab2;hb=9cb535fe4ef08b01e583ec955767a0899ff79afe#l563
> 
> ** Patch added: "register_b_set_flag.patch"
>    https://bugs.launchpad.net/qemu/+bug/1080086/+attachment/3436808/+files/register_b_set_flag.patch
> 

Hi Alex, the patch is almost good for inclusion.  I'd ask for two
changes: 1) please test == 0, not != REG_B_SET; 2) please leave the
fuzzing test last, because it may leave some registers in an undefined
state.

Paolo

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [Bug 1080086] Re: MC146818 RTC breaks when SET bit in Register B is on.
  2012-11-18  8:52   ` Paolo Bonzini
@ 2012-11-19 11:34     ` Alex Horn
  2012-11-19 11:42       ` Paolo Bonzini
  2012-11-19 11:44       ` Paolo Bonzini
  0 siblings, 2 replies; 10+ messages in thread
From: Alex Horn @ 2012-11-19 11:34 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Thomas Melham, qemu-devel, Daniel Kroening, Bug 1080086

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

> [...] the patch is almost good for inclusion. I'd ask for two changes:
> 1) please test == 0, not != REG_B_SET;
> 2) please leave the fuzzicsng test last

I have attached a new patch with the requested changes.

This patch also improves the quality of the functional test by
checking that RTC_SECONDS is equal (==) to the previously written data
provided the SET flag in Register B is still enabled. This is
justified by the data sheet which states that an enabled SET bit
"stops an existing update" and prevents "a new one from occurring" [1,
p. 15]. In contrast, once the SET flag is disabled, the RTC_SECONDS
check uses an inequality (>=) as in the original test case.

Out of curiosity, does anyone know how long this particular bug has
been undetected or how/when it was introduced? This could help me
explain to others my research interest in symbolic execution of
hardware models and its application in form of automated test
generation.

Finally, if there is interest to improve the robustness of the RTC
model, I could send a patch with several verification conditions (i.e.
assertions) which can help to expose these kind of bugs in the RTC
hardware model. Recall that most compiler can usually optimize these
assertions away unless a developer explicitly enables them. They also
serve as unambiguous code documentation.

With best regards,
Alex

[1] http://www.freescale.com/files/microcontrollers/doc/data_sheet/MC146818.pdf

On 18 November 2012 08:52, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 17/11/2012 19:47, Alex Horn ha scritto:
>> I have attached a patch for the most recent version of the file
>> hw/mc146818rtc.c [1]. The patch also features a functional test which
>> executes through the QTest framework.
>>
>> I would appreciate your thoughts on this.
>>
>> [1]
>> http://git.qemu.org/?p=qemu.git;a=blob;f=hw/mc146818rtc.c;h=98839f278d93452d071054e2a017b3d909b45ab2;hb=9cb535fe4ef08b01e583ec955767a0899ff79afe#l563
>>
>> ** Patch added: "register_b_set_flag.patch"
>>    https://bugs.launchpad.net/qemu/+bug/1080086/+attachment/3436808/+files/register_b_set_flag.patch
>>
>
> Hi Alex, the patch is almost good for inclusion.  I'd ask for two
> changes: 1) please test == 0, not != REG_B_SET; 2) please leave the
> fuzzing test last, because it may leave some registers in an undefined
> state.
>
> Paolo

[-- Attachment #2: register_b_set_flag_v2.patch --]
[-- Type: application/octet-stream, Size: 2775 bytes --]

diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index 98839f2..79c8aa6 100644
--- a/hw/mc146818rtc.c
+++ b/hw/mc146818rtc.c
@@ -569,7 +569,11 @@ static void rtc_update_time(RTCState *s)
     guest_nsec = get_guest_rtc_ns(s);
     guest_sec = guest_nsec / NSEC_PER_SEC;
     gmtime_r(&guest_sec, &ret);
-    rtc_set_cmos(s, &ret);
+
+    /* Is SET flag of Register B disabled? */
+    if ((s->cmos_data[RTC_REG_B] & REG_B_SET) == 0) {
+        rtc_set_cmos(s, &ret);
+    }
 }
 
 static int update_in_progress(RTCState *s)
diff --git a/tests/rtc-test.c b/tests/rtc-test.c
index 7fdc94a..02edbf5 100644
--- a/tests/rtc-test.c
+++ b/tests/rtc-test.c
@@ -327,6 +327,45 @@ static void fuzz_registers(void)
     }
 }
 
+static void register_b_set_flag(void)
+{
+    /* Enable binary-coded decimal (BCD) mode and SET flag in Register B*/
+    cmos_write(RTC_REG_B, (cmos_read(RTC_REG_B) & ~REG_B_DM) | REG_B_SET);
+
+    cmos_write(RTC_REG_A, 0x76);
+    cmos_write(RTC_YEAR, 0x11);
+    cmos_write(RTC_CENTURY, 0x20);
+    cmos_write(RTC_MONTH, 0x02);
+    cmos_write(RTC_DAY_OF_MONTH, 0x02);
+    cmos_write(RTC_HOURS, 0x02);
+    cmos_write(RTC_MINUTES, 0x04);
+    cmos_write(RTC_SECONDS, 0x58);
+    cmos_write(RTC_REG_A, 0x26);
+
+    /* Since SET flag is still enabled, these are equality checks. */
+    g_assert_cmpint(cmos_read(RTC_HOURS), ==, 0x02);
+    g_assert_cmpint(cmos_read(RTC_MINUTES), ==, 0x04);
+    g_assert_cmpint(cmos_read(RTC_SECONDS), ==, 0x58);
+    g_assert_cmpint(cmos_read(RTC_DAY_OF_MONTH), ==, 0x02);
+    g_assert_cmpint(cmos_read(RTC_MONTH), ==, 0x02);
+    g_assert_cmpint(cmos_read(RTC_YEAR), ==, 0x11);
+    g_assert_cmpint(cmos_read(RTC_CENTURY), ==, 0x20);
+
+    /* Disable SET flag in Register B */
+    cmos_write(RTC_REG_B, cmos_read(RTC_REG_B) & ~REG_B_SET);
+
+    g_assert_cmpint(cmos_read(RTC_HOURS), ==, 0x02);
+    g_assert_cmpint(cmos_read(RTC_MINUTES), ==, 0x04);
+
+    /* Since SET flag is disabled, this is an inequality check.
+     * We (reasonably) assume that no (sexagesimal) overflow occurs. */
+    g_assert_cmpint(cmos_read(RTC_SECONDS), >=, 0x58);
+    g_assert_cmpint(cmos_read(RTC_DAY_OF_MONTH), ==, 0x02);
+    g_assert_cmpint(cmos_read(RTC_MONTH), ==, 0x02);
+    g_assert_cmpint(cmos_read(RTC_YEAR), ==, 0x11);
+    g_assert_cmpint(cmos_read(RTC_CENTURY), ==, 0x20);
+}
+
 int main(int argc, char **argv)
 {
     QTestState *s = NULL;
@@ -342,6 +381,7 @@ int main(int argc, char **argv)
     qtest_add_func("/rtc/alarm-time", alarm_time);
     qtest_add_func("/rtc/set-year/20xx", set_year_20xx);
     qtest_add_func("/rtc/set-year/1980", set_year_1980);
+    qtest_add_func("/rtc/register_b_set_flag", register_b_set_flag);
     qtest_add_func("/rtc/fuzz-registers", fuzz_registers);
     ret = g_test_run();
 

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [Bug 1080086] Re: MC146818 RTC breaks when SET bit in Register B is on.
  2012-11-19 11:34     ` Alex Horn
@ 2012-11-19 11:42       ` Paolo Bonzini
  2012-11-19 14:14         ` Alex Horn
  2012-11-19 11:44       ` Paolo Bonzini
  1 sibling, 1 reply; 10+ messages in thread
From: Paolo Bonzini @ 2012-11-19 11:42 UTC (permalink / raw)
  To: Alex Horn; +Cc: Thomas Melham, qemu-devel, Daniel Kroening, Bug 1080086

Il 19/11/2012 12:34, Alex Horn ha scritto:
>> [...] the patch is almost good for inclusion. I'd ask for two changes:
>> 1) please test == 0, not != REG_B_SET;
>> 2) please leave the fuzzicsng test last
> 
> I have attached a new patch with the requested changes.
> 
> This patch also improves the quality of the functional test by
> checking that RTC_SECONDS is equal (==) to the previously written data
> provided the SET flag in Register B is still enabled. This is
> justified by the data sheet which states that an enabled SET bit
> "stops an existing update" and prevents "a new one from occurring" [1,
> p. 15]. In contrast, once the SET flag is disabled, the RTC_SECONDS
> check uses an inequality (>=) as in the original test case.

Right.

> Out of curiosity, does anyone know how long this particular bug has
> been undetected or how/when it was introduced?

Probably it was introduced last September when the model was rewritten.
 But it's really unlikely that the bug would have been detected.

On the other hand, the bug in commit b6db4ac (which also includes a
qtest) was detected by autotest.  Could your tool find it, too?

> This could help me explain to others my research interest in symbolic
> execution of hardware models and its application in form of automated
> test generation.

Very interesting (at least to me :)).

> Finally, if there is interest to improve the robustness of the RTC
> model, I could send a patch with several verification conditions (i.e.
> assertions) which can help to expose these kind of bugs in the RTC
> hardware model.

Sure, that's welcome.

In particular, I assume you verified the "next alarm" code to be correct? :)

Paolo

> Recall that most compiler can usually optimize these
> assertions away unless a developer explicitly enables them. They also
> serve as unambiguous code documentation.
> 
> With best regards,
> Alex
> 
> [1] http://www.freescale.com/files/microcontrollers/doc/data_sheet/MC146818.pdf
> 
> On 18 November 2012 08:52, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 17/11/2012 19:47, Alex Horn ha scritto:
>>> I have attached a patch for the most recent version of the file
>>> hw/mc146818rtc.c [1]. The patch also features a functional test which
>>> executes through the QTest framework.
>>>
>>> I would appreciate your thoughts on this.
>>>
>>> [1]
>>> http://git.qemu.org/?p=qemu.git;a=blob;f=hw/mc146818rtc.c;h=98839f278d93452d071054e2a017b3d909b45ab2;hb=9cb535fe4ef08b01e583ec955767a0899ff79afe#l563
>>>
>>> ** Patch added: "register_b_set_flag.patch"
>>>    https://bugs.launchpad.net/qemu/+bug/1080086/+attachment/3436808/+files/register_b_set_flag.patch
>>>
>>
>> Hi Alex, the patch is almost good for inclusion.  I'd ask for two
>> changes: 1) please test == 0, not != REG_B_SET; 2) please leave the
>> fuzzing test last, because it may leave some registers in an undefined
>> state.
>>
>> Paolo

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [Bug 1080086] Re: MC146818 RTC breaks when SET bit in Register B is on.
  2012-11-19 11:34     ` Alex Horn
  2012-11-19 11:42       ` Paolo Bonzini
@ 2012-11-19 11:44       ` Paolo Bonzini
  1 sibling, 0 replies; 10+ messages in thread
From: Paolo Bonzini @ 2012-11-19 11:44 UTC (permalink / raw)
  To: Alex Horn; +Cc: qemu-devel

Il 19/11/2012 12:34, Alex Horn ha scritto:
> I have attached a new patch with the requested changes.

Could you please resend it using "git send-email", so that it has a
commit message and a "Signed-off-by: Alex Horn <...@...>" line?

Thanks!

Paolo

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Qemu-devel] [Bug 1080086] Re: MC146818 RTC breaks when SET bit in Register B is on.
  2012-11-17 14:41 [Qemu-devel] [Bug 1080086] [NEW] MC146818 RTC breaks when SET bit in Register B is on Alex Horn
  2012-11-17 18:47 ` [Qemu-devel] [Bug 1080086] " Alex Horn
@ 2012-11-19 12:12 ` Alex Horn
  2012-11-19 12:42 ` Alex Horn
  2017-11-07 18:42 ` Peter Maydell
  3 siblings, 0 replies; 10+ messages in thread
From: Alex Horn @ 2012-11-19 12:12 UTC (permalink / raw)
  To: qemu-devel

Cross reference:

  https://lists.gnu.org/archive/html/qemu-devel/2012-11/msg01759.html

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1080086

Title:
  MC146818 RTC breaks when SET bit in Register B is on.

Status in QEMU:
  New

Bug description:
  This bug occurs when the SET flag of Register B is enabled. When an RTC
  data register (i.e. any of the 10 bytes of time/calender data in CMOS) is set,
  the data is (as expected) correctly stored in the cmos_data array. However,
  since the SET flag is enabled, the function rtc_set_time is not invoked.
  As a result, the field base_rtc in RTCState remains uninitialized. This appears to
  cause a problem on subsequent writes which can end up overwriting data.

  To see this, consider writing data to Register A after having written
  data to any of the RTC data registers; the following figure illustrates
  the call stack for the Register A write operation:

   +- cmos_io_port_write
   +-- check_update_timer
   +---- get_next_alarm
   +------ rtc_update_time

  In rtc_update_time, get_guest_rtc calculates the wrong time and
  overwrites the previously written RTC data register values.

  I have created a standalone test case which exposes this bug:

  https://github.com/ahorn/benchmarks/commit/fff1ca40694bbef6f7f9de323bb0bed63419ef99

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1080086/+subscriptions

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Qemu-devel] [Bug 1080086] Re: MC146818 RTC breaks when SET bit in Register B is on.
  2012-11-17 14:41 [Qemu-devel] [Bug 1080086] [NEW] MC146818 RTC breaks when SET bit in Register B is on Alex Horn
  2012-11-17 18:47 ` [Qemu-devel] [Bug 1080086] " Alex Horn
  2012-11-19 12:12 ` Alex Horn
@ 2012-11-19 12:42 ` Alex Horn
  2017-11-07 18:42 ` Peter Maydell
  3 siblings, 0 replies; 10+ messages in thread
From: Alex Horn @ 2012-11-19 12:42 UTC (permalink / raw)
  To: qemu-devel

** Attachment added: "register_b_set_flag_v2.patch"
   https://bugs.launchpad.net/bugs/1080086/+attachment/3438594/+files/register_b_set_flag_v2.patch

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1080086

Title:
  MC146818 RTC breaks when SET bit in Register B is on.

Status in QEMU:
  New

Bug description:
  This bug occurs when the SET flag of Register B is enabled. When an RTC
  data register (i.e. any of the 10 bytes of time/calender data in CMOS) is set,
  the data is (as expected) correctly stored in the cmos_data array. However,
  since the SET flag is enabled, the function rtc_set_time is not invoked.
  As a result, the field base_rtc in RTCState remains uninitialized. This appears to
  cause a problem on subsequent writes which can end up overwriting data.

  To see this, consider writing data to Register A after having written
  data to any of the RTC data registers; the following figure illustrates
  the call stack for the Register A write operation:

   +- cmos_io_port_write
   +-- check_update_timer
   +---- get_next_alarm
   +------ rtc_update_time

  In rtc_update_time, get_guest_rtc calculates the wrong time and
  overwrites the previously written RTC data register values.

  I have created a standalone test case which exposes this bug:

  https://github.com/ahorn/benchmarks/commit/fff1ca40694bbef6f7f9de323bb0bed63419ef99

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1080086/+subscriptions

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [Bug 1080086] Re: MC146818 RTC breaks when SET bit in Register B is on.
  2012-11-19 11:42       ` Paolo Bonzini
@ 2012-11-19 14:14         ` Alex Horn
  0 siblings, 0 replies; 10+ messages in thread
From: Alex Horn @ 2012-11-19 14:14 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Thomas Melham, qemu-devel, Daniel Kroening, Bug 1080086

>> Out of curiosity, does anyone know how long this particular bug has
>> been undetected or how/when it was introduced?
>
> Probably it was introduced last September when the model was rewritten.
>  But it's really unlikely that the bug would have been detected.

Thanks for that quick assessment.

> On the other hand, the bug in commit b6db4ac (which also includes a
> qtest) was detected by autotest.  Could your tool find it, too?

That's a very respectable achievement. I understand that Autotest runs
predefined (i.e. hand-written) tests on a large scale. In contrast, we
seek a higher degree of automation but on a smaller scale. These
differences could make a comparison difficult. However, your example
is much appreciated because it helps us to set our work in
perspective.

>> This could help me explain to others my research interest in symbolic
>> execution of hardware models and its application in form of automated
>> test generation.
>
> Very interesting (at least to me :)).

Perhaps you find the following background helpful. To kick start our
work, we extracted from QEMU a standalone RTC hardware model [2]
because QOM, QMP, TCG or QTest would render the semi-automatic
analysis infeasible. As a next step, we would like to combine this
standalone hardware model with a Linux x86 driver [3]. The reported
bug is a good exemplar of the type of firmware/hardware interface
properties we are interested in. Note that this is only the initial
phase of our research and there is much work yet to be done.

Of course, any comments or collaboration are always welcome.

With kind regards,
Alex

[2] https://github.com/ahorn/benchmarks/tree/master/qemu-hw
[3] https://github.com/ahorn/benchmarks/tree/master/sw-hw/linux/rtc_x86

On 19 November 2012 11:42, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 19/11/2012 12:34, Alex Horn ha scritto:
>>> [...] the patch is almost good for inclusion. I'd ask for two changes:
>>> 1) please test == 0, not != REG_B_SET;
>>> 2) please leave the fuzzicsng test last
>>
>> I have attached a new patch with the requested changes.
>>
>> This patch also improves the quality of the functional test by
>> checking that RTC_SECONDS is equal (==) to the previously written data
>> provided the SET flag in Register B is still enabled. This is
>> justified by the data sheet which states that an enabled SET bit
>> "stops an existing update" and prevents "a new one from occurring" [1,
>> p. 15]. In contrast, once the SET flag is disabled, the RTC_SECONDS
>> check uses an inequality (>=) as in the original test case.
>
> Right.
>
>> Out of curiosity, does anyone know how long this particular bug has
>> been undetected or how/when it was introduced?
>
> Probably it was introduced last September when the model was rewritten.
>  But it's really unlikely that the bug would have been detected.
>
> On the other hand, the bug in commit b6db4ac (which also includes a
> qtest) was detected by autotest.  Could your tool find it, too?
>
>> This could help me explain to others my research interest in symbolic
>> execution of hardware models and its application in form of automated
>> test generation.
>
> Very interesting (at least to me :)).
>
>> Finally, if there is interest to improve the robustness of the RTC
>> model, I could send a patch with several verification conditions (i.e.
>> assertions) which can help to expose these kind of bugs in the RTC
>> hardware model.
>
> Sure, that's welcome.
>
> In particular, I assume you verified the "next alarm" code to be correct? :)
>
> Paolo
>
>> Recall that most compiler can usually optimize these
>> assertions away unless a developer explicitly enables them. They also
>> serve as unambiguous code documentation.
>>
>> With best regards,
>> Alex
>>
>> [1] http://www.freescale.com/files/microcontrollers/doc/data_sheet/MC146818.pdf
>>
>> On 18 November 2012 08:52, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> Il 17/11/2012 19:47, Alex Horn ha scritto:
>>>> I have attached a patch for the most recent version of the file
>>>> hw/mc146818rtc.c [1]. The patch also features a functional test which
>>>> executes through the QTest framework.
>>>>
>>>> I would appreciate your thoughts on this.
>>>>
>>>> [1]
>>>> http://git.qemu.org/?p=qemu.git;a=blob;f=hw/mc146818rtc.c;h=98839f278d93452d071054e2a017b3d909b45ab2;hb=9cb535fe4ef08b01e583ec955767a0899ff79afe#l563
>>>>
>>>> ** Patch added: "register_b_set_flag.patch"
>>>>    https://bugs.launchpad.net/qemu/+bug/1080086/+attachment/3436808/+files/register_b_set_flag.patch
>>>>
>>>
>>> Hi Alex, the patch is almost good for inclusion.  I'd ask for two
>>> changes: 1) please test == 0, not != REG_B_SET; 2) please leave the
>>> fuzzing test last, because it may leave some registers in an undefined
>>> state.
>>>
>>> Paolo
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Qemu-devel] [Bug 1080086] Re: MC146818 RTC breaks when SET bit in Register B is on.
  2012-11-17 14:41 [Qemu-devel] [Bug 1080086] [NEW] MC146818 RTC breaks when SET bit in Register B is on Alex Horn
                   ` (2 preceding siblings ...)
  2012-11-19 12:42 ` Alex Horn
@ 2017-11-07 18:42 ` Peter Maydell
  3 siblings, 0 replies; 10+ messages in thread
From: Peter Maydell @ 2017-11-07 18:42 UTC (permalink / raw)
  To: qemu-devel

We fixed this bug long ago in 2012, in commit 02c6ccc6dde90d (thanks for
your patch!) but forgot to close the bug...so I'm doing it now.


** Changed in: qemu
       Status: New => Fix Released

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1080086

Title:
  MC146818 RTC breaks when SET bit in Register B is on.

Status in QEMU:
  Fix Released

Bug description:
  This bug occurs when the SET flag of Register B is enabled. When an RTC
  data register (i.e. any of the 10 bytes of time/calender data in CMOS) is set,
  the data is (as expected) correctly stored in the cmos_data array. However,
  since the SET flag is enabled, the function rtc_set_time is not invoked.
  As a result, the field base_rtc in RTCState remains uninitialized. This appears to
  cause a problem on subsequent writes which can end up overwriting data.

  To see this, consider writing data to Register A after having written
  data to any of the RTC data registers; the following figure illustrates
  the call stack for the Register A write operation:

   +- cmos_io_port_write
   +-- check_update_timer
   +---- get_next_alarm
   +------ rtc_update_time

  In rtc_update_time, get_guest_rtc calculates the wrong time and
  overwrites the previously written RTC data register values.

  I have created a standalone test case which exposes this bug:

  https://github.com/ahorn/benchmarks/commit/fff1ca40694bbef6f7f9de323bb0bed63419ef99

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1080086/+subscriptions

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2017-11-07 18:55 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-11-17 14:41 [Qemu-devel] [Bug 1080086] [NEW] MC146818 RTC breaks when SET bit in Register B is on Alex Horn
2012-11-17 18:47 ` [Qemu-devel] [Bug 1080086] " Alex Horn
2012-11-18  8:52   ` Paolo Bonzini
2012-11-19 11:34     ` Alex Horn
2012-11-19 11:42       ` Paolo Bonzini
2012-11-19 14:14         ` Alex Horn
2012-11-19 11:44       ` Paolo Bonzini
2012-11-19 12:12 ` Alex Horn
2012-11-19 12:42 ` Alex Horn
2017-11-07 18:42 ` Peter Maydell

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.