Skip to content

boards/qemu_x86: Fix emulated boards and their RTCs #59967

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

bjarki-andreasen
Copy link
Collaborator

@bjarki-andreasen bjarki-andreasen commented Jul 3, 2023

This PR has been created in response to this issue #59448 which identified that the RTC was not behaving as expected on the qemu_x86 board. This PR contains the patch for issue along with an improvement to the test suite to catch similar issues in the future.

The QEMU emulated MC146818 seems to have a bug where it is not able to invoke an interrupt, which has not been caught by the developer who wrote the driver for it, since he was building the driver for a real board with real hardware. The unit tests where not running for the QEMU boards since it's YAML files did not indicate they supported the RTC, which is my mistake, since I didn't realize the QEMU boards had RTC support at all.

The fix suggested by this commit is to replace the QEMU emulated MC146818 it with the emulated RTC, which is currently used for the native posix boards as well.

Furthermore, the yaml files for the emulated boards have been updated to include support for the rtc. The test suite has been updated to only test qemu_x86 and and native_posix during integration testing as the qemu_x86_64 runs in real time, causing the test suite to time out...

This addition revealed that the RTC test suite has a bug where it cant run one of the tests on 64-bit architectures. This has been fixed.

The MC146818, which is a real piece of hardware, has been
placed within the ia32.dtsi file by mistake. This board
is the base for the qemu and acrn boards, which do not
have this device present. This commit removes it,
replacing it with the emulated RTC, which is currently
used for the native posix boards as well.

Furthermore, the yaml files for the emulated boards
have been updated to include support for the rtc.

Signed-off-by: Bjarki Arge Andreasen <[email protected]>
@bjarki-andreasen bjarki-andreasen force-pushed the qemu_fix_incorrect_rtc_in_devicetree branch from c5a5d70 to 86a2e88 Compare July 3, 2023 17:52
The addition of the qemu_x86_64 board releaved a bug in
one of the tests, a pointer size issue. This has been fixed.
The qemu_x86_64 board also runs in real time, causing the
test suite to time out, and has therefore been excluded from
integration testing.

Lastly, the test suite has been updated to only test the
emulated boards, and since they all use the emulated RTC
which emulates all features, all features have been added
to the execusion for the emulated boards.

Signed-off-by: Bjarki Arge Andreasen <[email protected]>
@bjarki-andreasen bjarki-andreasen force-pushed the qemu_fix_incorrect_rtc_in_devicetree branch from 86a2e88 to 482c377 Compare July 3, 2023 17:54
@bjarki-andreasen bjarki-andreasen self-assigned this Jul 3, 2023
@henrikbrixandersen
Copy link
Member

Have you considered fixing the bug in QEMU instead?

@bjarki-andreasen
Copy link
Collaborator Author

bjarki-andreasen commented Jul 4, 2023

I have considered it, its quite a large task compared to this fix, and considering I don't see the value in fixing an emulated driver in QEMU (if thats even the issue, I have no clue why the interrupt routine is not working, could be ioapic as well), its not exactly my preferred solution :)

@cfriedt
Copy link
Member

cfriedt commented Jul 5, 2023

On the topic of fixing things in Qemu, the source for the emulated hardware is here. On the + side, if you fix it, you get to correct a piece of code written by the infamous Fabrice Bellard 😏 (assuming that the Qemu side has a bug).

https://github.com/arrikto/qemu-qemu/blob/master/hw/timer/mc146818rtc.c

Technically, it should be possible to write a "qtest" for verification. Qemu has a kind of interesting way to do integration tests. I did one recently for an off-by-one error. The fix is still not yet merged, in spite of multiple follow-ups on the mailing list.
https://gitlab.com/qemu-project/qemu/-/issues/1254

I'm sort of less inclined to approve a workaround for this issue, given that qemu_x86 is a tier0 platform. We could fix this in-tree though in zephyrproject-rtos/sdk-ng by keeping the patch in our SDK build.

Eventually, it should get upstreamed though. Perhaps there is someone in the Zephyr community who is a more regular contributor to Qemu who they might actually pay attention to on the mailing list.

@bjarki-andreasen
Copy link
Collaborator Author

After looking into QEMU, the issue seems to be the use of HPET and IOAPIC. According to the documentation (see first figure on page 7) The IRQ is routed to HPET using IOAPIC. Looking into the source code on QEMU, the IRQ is indeed routed to the HPET if enabled source (line 2008)

Does anyone have any experience regarding HPET and IOAPIC vs 8259 legacy interrupt controllers? I do not know if HPET is somehow controlling and using the RTC as a timer as described in the documentation, or if we can still use the RTC directly without causing issues.

@cfriedt
Copy link
Member

cfriedt commented Jul 12, 2023

Does anyone have any experience regarding HPET and IOAPIC vs 8259 legacy interrupt controllers? I do not know if HPET is somehow controlling and using the RTC as a timer as described in the documentation, or if we can still use the RTC directly without causing issues.

Maybe @dcpleung can help here?

@dcpleung
Copy link
Member

Hm... We enable Legacy Replacement for interrupt routing (LEG_RT_CNF, GCONF_LF in hpet.c). According to the documentation, section 2.4.2.1, enabling this bit would disable RTC. Maybe try adding no-legacy-irq to the HPET devicetree node and see if the RTC interrupt works?

@bjarki-andreasen
Copy link
Collaborator Author

bjarki-andreasen commented Jul 12, 2023

@dcpleung That made the IRQ work! There are still issues with our mc14 driver, but that's a step in the right direction maybe? Is there any downside to using no-legacy-irq for the QEMU boards?

@bjarki-andreasen
Copy link
Collaborator Author

update callback now working! lets go

@dcpleung
Copy link
Member

IIRC, GCONF_LR is used to disable the interrupt from PIT (8254). @andyross might know more about this.

@bjarki-andreasen
Copy link
Collaborator Author

PR with working driver here #60321 :) Closing this PR as we will probably not be replacing it with the zephyr emul one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants