Details

Reviewers

gonzo
avg
des
markj
lwhsu

Commits

rG2b74ff5fceb6: ichwd: introduce i6300esbwd watch dog driver

Summary

The i6300ESB watchdog is a special ICH-based watchdog device with a
different interface.
QEMU implements this watchdog for x86 systems.

This change enables watchdog mode (rather than free-running mode) and
introduces two sysctls:

hw.i6300esb.reboot: when enabled, the system will reboot upon watchdog timeout.
hw.i6300esb.locked: locks the watchdog register after the event is triggered, preventing it from being disabled until a hard reset.

This feature has been tested on a Vultr AMD guest machine.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 66503
Build 63386: arc lint + arc unit

Event Timeline

aokblast created this revision.Aug 20 2025, 4:56 PM

Herald added a subscriber: imp. · View Herald TranscriptAug 20 2025, 4:56 PM

aokblast requested review of this revision.Aug 20 2025, 4:56 PM

Harbormaster completed remote builds in B66381: Diff 160679.Aug 20 2025, 4:56 PM

aokblast added a reviewer: gonzo.Aug 20 2025, 4:58 PM

lwhsu added reviewers: avg, des, markj, lwhsu.Aug 21 2025, 2:35 AM

lwhsu added inline comments.Aug 21 2025, 3:26 AM

sys/dev/ichwd/i6300esb.h
16 ↗	(On Diff #160679)	From https://www.intel.com/content/dam/doc/datasheet/6300esb-io-controller-hub-datasheet.pdf 16.4.21, should this be `0xC` ?

lwhsu added inline comments.Aug 21 2025, 3:36 AM

sys/dev/ichwd/i6300esb.c
24 ↗	(On Diff #160679)	Does this device really need ISA?
57 ↗	(On Diff #160679)
164 ↗	(On Diff #160679)
sys/dev/ichwd/i6300esb.h
7 ↗	(On Diff #160679)	Perhaps it's better to align with the filename, like `_I6300ESB_H_` ?
sys/modules/i6300esb/Makefile
4 ↗	(On Diff #160679)	Does this device really need ISA?

fix errors

Harbormaster completed remote builds in B66395: Diff 160706.Aug 21 2025, 7:14 AM

Fix incorret reboot flag

Harbormaster completed remote builds in B66396: Diff 160708.Aug 21 2025, 7:39 AM

Tested by a qemu guest on my machine with -watchdog-action reset

Use >= instead of >

Harbormaster completed remote builds in B66397: Diff 160709.Aug 21 2025, 7:43 AM

Add to parent Makefile

Harbormaster completed remote builds in B66398: Diff 160710.Aug 21 2025, 7:50 AM

BTW we can mention PR 259673 in the commit message.

markj added inline comments.Aug 21 2025, 1:12 PM

sys/dev/ichwd/i6300esb.c
33 ↗	(On Diff #160710)	Just use VENDORID_INTEL from ichwd.h?
54 ↗	(On Diff #160710)
87 ↗	(On Diff #160710)
148 ↗	(On Diff #160710)
168 ↗	(On Diff #160710)
193 ↗	(On Diff #160710)	What happens if the watchdog times out and i6300esb_reboot == 0?
227 ↗	(On Diff #160710)
sys/dev/ichwd/i6300esb.h
46 ↗	(On Diff #160710)

Minor fixes

Harbormaster completed remote builds in B66421: Diff 160738.Aug 21 2025, 1:31 PM

aokblast added inline comments.Aug 21 2025, 1:31 PM

sys/dev/ichwd/i6300esb.c
193 ↗	(On Diff #160710)	In QEMU, it does nothing. According to the documentation, WDT_PRE_SEL indicate if the timeout event will be output to a specific CPU pin. Therefore, when we turn WDT_OUTPUT_EN off, nothing should happens.

markj added inline comments.Aug 21 2025, 1:41 PM

sys/dev/ichwd/i6300esb.c
42 ↗	(On Diff #160738)	If you use SYSCTL_ADD_* you can attach the sysctls to the device_t node instead. That would be better IMO, since then the device sysctls are all in one place. (Though, realistically there will only ever be one instance of this driver in a running system.)
127 ↗	(On Diff #160738)
sys/dev/ichwd/ichwd.h
159	The new driver handles only _5, and the main driver handles only _1. What about the others?

Use sysctl_add_proc and remove useless define

Harbormaster completed remote builds in B66422: Diff 160743.Aug 21 2025, 3:26 PM

markj added inline comments.Aug 22 2025, 1:23 PM

sys/dev/ichwd/i6300esb.c
113 ↗	(On Diff #160743)	We usually avoid using `!!`, more idiomatic would be to write `(i6300esb_cfg_read(sc) & WDT_PRE_SEL) != 0`. That pattern is much more common in freebsd kernel sources.
137 ↗	(On Diff #160743)	Did you mean to write `!!` there?
157 ↗	(On Diff #160743)	I don't really understand this "ticks" value. Here, the watchdog framework is saying, "the watchdog will time out in 2^cmd nanoseconds" and we're converting the timeout to a count of microseconds or milliseconds, right? Why would the user care which one is used?
267 ↗	(On Diff #160743)	Why is this configurable? The whole purpose of the watchdog is to reboot the system when the timer runs out, so it seems a bit strange to have a sysctl. If there is some purpose for the sysctl, then the description should explain what happens if the value is 0 and the timeout elapses.
sys/dev/ichwd/ichwd.h
155	So should we call it 6300ESB_QEMU or something like that?

aokblast added inline comments.Aug 22 2025, 1:39 PM

sys/dev/ichwd/i6300esb.c
137 ↗	(On Diff #160743)	The WDT_OUTPUT_EN is 0 means enable. 1 means disable. So it make sense to only use !
157 ↗	(On Diff #160743)	The precision of i6300esb can be adjusted from 1khz (1ms per tick) to 1mhz (1us per tick). Howevere, the value fill into the preload register should be tick. If user specified I want 2^cmd nanosecond, we should convert the nanosecond to the tick before we fill the register. Maybe I should rename the name to ticks_per_ns.
267 ↗	(On Diff #160743)	Sometime we want to test our unstable system. However, if we reboot frequently, we are not able to test it. Therefore, providing such sysctl is somehow userful for debugging.
sys/dev/ichwd/ichwd.h
155	Emm, it is a real device with intel support. I am not sure naming it QEMU is good or not.

This generally looks ok to me, I am just skeptical about some of the sysctls.

sys/dev/ichwd/i6300esb.c
157 ↗	(On Diff #160743)	Right, I guess my question is, why is there a sysctl for it? Is there a scenario where the user may care about this?
267 ↗	(On Diff #160743)	But if the system is unstable, we should just disable the watchdog using the watchdog framework, rather than a device-specific sysctl. Note that the main ichwd driver has no such sysctl.
sys/dev/ichwd/ichwd.h
155	Ah ok, I thought it was some QEMU-specific variant.

Remove useless sysctl

Harbormaster completed remote builds in B66453: Diff 160817.Aug 22 2025, 2:57 PM

I suspect this driver should be called i6300esbwd, since it only exposes the watchdog functionality of the i6300esb chipset.

Other than that, I think this is ok.

rename i6300esb -> i6300esbwd

Harbormaster completed remote builds in B66502: Diff 160926.Aug 25 2025, 1:31 PM

Add missing x86 file

Harbormaster completed remote builds in B66503: Diff 160927.Aug 25 2025, 1:33 PM

Add manual

Herald added a subscriber: ziaee. · View Herald TranscriptAug 25 2025, 1:42 PM

Harbormaster completed remote builds in B66504: Diff 160928.Aug 25 2025, 1:42 PM

I think it's ok, thank you.

One thing I am not sure about: should we just compile the new driver into ichwd.ko rather than create a new kernel module? I do not think it is a major issue, but personally I would probably just fold this into ichwd.ko to keep the diff smaller. I leave it up to you.

sys/sys/watchdog.h
86	I think this constant is unused now?

This revision is now accepted and ready to land.Aug 25 2025, 1:43 PM

In D52049#1191168, @markj wrote:

I think it's ok, thank you.

One thing I am not sure about: should we just compile the new driver into ichwd.ko rather than create a new kernel module? I do not think it is a major issue, but personally I would probably just fold this into ichwd.ko to keep the diff smaller. I leave it up to you.

I never do that before. Can I define two modules inside a ko? I know that the kernel module is done through special ELF section and will be handled by kernel loader. I don't know if the linker works on combining these two into a single section.

In D52049#1191172, @aokblast wrote:

In D52049#1191168, @markj wrote:

I think it's ok, thank you.

One thing I am not sure about: should we just compile the new driver into ichwd.ko rather than create a new kernel module? I do not think it is a major issue, but personally I would probably just fold this into ichwd.ko to keep the diff smaller. I leave it up to you.

I never do that before. Can I define two modules inside a ko? I know that the kernel module is done through special ELF section and will be handled by kernel loader. I don't know if the linker works on combining these two into a single section.

Yes! It is a bit confusing, but there are two distinct concepts, linker files (i.e., ELF files which the kernel can load, so a .ko file) and modules (typically drivers). A linker file can contain multiple modules. kldstat -v will list the modules in each linker file, e.g.,:

4    1 0xffffffff8227d000   380300 vmm.ko (/boot/kernel.GENERIC-NODEBUG/vmm.ko)
       Contains modules:                                                                                                                                      
                Id Name                                                                                                                                       
                 6 acpi/ivhd                                                                                                                                  
                 5 pci/amdviiommu   
                 4 pci/ppt                                                                                                                                    
                 3 vmm                                                                                                                                        
5    1 0xffffffff825fe000   61fa88 zfs.ko (/boot/kernel.GENERIC-NODEBUG/zfs.ko)                                                                               
       Contains modules:                                                                                                                                      
                Id Name                                                                                                                                       
                 7 zfsctrl                                                                                                                                    
                 9 zfs                                                                                                                                        
                10 zfs_zvol
                 8 zfs_vdev

Confusingly, to most people "kernel module" means "linker file." Usually the distinction is not important, and usually a linker file contains only one module.

Merge driver to ichwd

This revision now requires review to proceed.Aug 25 2025, 2:13 PM

Harbormaster completed remote builds in B66505: Diff 160929.Aug 25 2025, 2:13 PM

In D52049#1191173, @markj wrote:
In D52049#1191172, @aokblast wrote:

In D52049#1191168, @markj wrote:

I think it's ok, thank you.

One thing I am not sure about: should we just compile the new driver into ichwd.ko rather than create a new kernel module? I do not think it is a major issue, but personally I would probably just fold this into ichwd.ko to keep the diff smaller. I leave it up to you.

I never do that before. Can I define two modules inside a ko? I know that the kernel module is done through special ELF section and will be handled by kernel loader. I don't know if the linker works on combining these two into a single section.

Yes! It is a bit confusing, but there are two distinct concepts, linker files (i.e., ELF files which the kernel can load, so a .ko file) and modules (typically drivers). A linker file can contain multiple modules. kldstat -v will list the modules in each linker file, e.g.,:
4    1 0xffffffff8227d000   380300 vmm.ko (/boot/kernel.GENERIC-NODEBUG/vmm.ko)
       Contains modules:                                                                                                                                      
                Id Name                                                                                                                                       
                 6 acpi/ivhd                                                                                                                                  
                 5 pci/amdviiommu   
                 4 pci/ppt                                                                                                                                    
                 3 vmm                                                                                                                                        
5    1 0xffffffff825fe000   61fa88 zfs.ko (/boot/kernel.GENERIC-NODEBUG/zfs.ko)                                                                               
       Contains modules:                                                                                                                                      
                Id Name                                                                                                                                       
                 7 zfsctrl                                                                                                                                    
                 9 zfs                                                                                                                                        
                10 zfs_zvol
                 8 zfs_vdev
Confusingly, to most people "kernel module" means "linker file." Usually the distinction is not important, and usually a linker file contains only one module.

Ok. Got it. I know linker file since it was part of my work in GSoC 2023 lldb:).

Remove useless constant

Harbormaster completed remote builds in B66506: Diff 160930.Aug 25 2025, 2:17 PM

markj accepted this revision.Aug 25 2025, 2:23 PM

This revision is now accepted and ready to land.Aug 25 2025, 2:23 PM

Consider a MFC ?

Closed by commit rG2b74ff5fceb6: ichwd: introduce i6300esbwd watch dog driver (authored by aokblast). · Explain WhyAug 25 2025, 3:46 PM

This revision was automatically updated to reflect the committed changes.

aokblast added a commit: rG2b74ff5fceb6: ichwd: introduce i6300esbwd watch dog driver.

ichwd: introduce i6300esb watch dog driver
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 160927

sys/conf/files.x86

sys/dev/ichwd/i6300esbwd.h

sys/dev/ichwd/i6300esbwd.c

sys/dev/ichwd/ichwd.h

sys/dev/ichwd/ichwd.c

sys/modules/Makefile

sys/modules/i6300esbwd/Makefile

sys/sys/watchdog.h

sys/x86/conf/NOTES

ichwd: introduce i6300esb watch dog driverClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 160927

sys/conf/files.x86

sys/dev/ichwd/i6300esbwd.h

sys/dev/ichwd/i6300esbwd.c

sys/dev/ichwd/ichwd.h

sys/dev/ichwd/ichwd.c

sys/modules/Makefile

sys/modules/i6300esbwd/Makefile

sys/sys/watchdog.h

sys/x86/conf/NOTES

ichwd: introduce i6300esb watch dog driver
ClosedPublic
Actions

Revision Contents
Changeset List