Page MenuHomeFreeBSD
Differential D52049

ichwd: introduce i6300esb watch dog driver
ClosedPublic
Actions

Authored by aokblast on Wed, Aug 20, 4:56 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Sep 9, 12:05 AM
Unknown Object (File)
Sun, Sep 7, 5:36 PM
Unknown Object (File)
Sat, Sep 6, 8:20 PM
Unknown Object (File)
Wed, Sep 3, 5:11 AM
Unknown Object (File)
Wed, Sep 3, 4:15 AM
Unknown Object (File)
Wed, Sep 3, 3:56 AM
Unknown Object (File)
Wed, Sep 3, 2:49 AM
Unknown Object (File)
Wed, Sep 3, 2:47 AM
View All 52 Files
Subscribers
imp
ziaee

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.

Sponsored by: The FreeBSD Foundation

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 66395
Build 63278: arc lint + arc unit

Event Timeline

aokblast created this revision.Wed, Aug 20, 4:56 PM
Herald added a subscriber: imp. · View Herald TranscriptWed, Aug 20, 4:56 PM
aokblast requested review of this revision.Wed, Aug 20, 4:56 PM
Harbormaster completed remote builds in B66381: Diff 160679.Wed, Aug 20, 4:56 PM
aokblast added a reviewer: gonzo.Wed, Aug 20, 4:58 PM
lwhsu added reviewers: avg, des, markj, lwhsu.Thu, Aug 21, 2:35 AM
lwhsu added inline comments.Thu, Aug 21, 3:26 AM
sys/dev/ichwd/i6300esb.h
17

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.Thu, Aug 21, 3:36 AM
sys/dev/ichwd/i6300esb.c
25

Does this device really need ISA?

58
165
sys/dev/ichwd/i6300esb.h
8

Perhaps it's better to align with the filename, like _I6300ESB_H_ ?

sys/modules/i6300esb/Makefile
5

Does this device really need ISA?

aokblast updated this revision to Diff 160706.Thu, Aug 21, 7:14 AM

fix errors

Harbormaster completed remote builds in B66395: Diff 160706.Thu, Aug 21, 7:14 AM
aokblast updated this revision to Diff 160708.Thu, Aug 21, 7:39 AM

Fix incorret reboot flag

Harbormaster completed remote builds in B66396: Diff 160708.Thu, Aug 21, 7:39 AM
aokblast added a comment.EditedThu, Aug 21, 7:40 AM

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

aokblast updated this revision to Diff 160709.Thu, Aug 21, 7:43 AM

Use >= instead of >

Harbormaster completed remote builds in B66397: Diff 160709.Thu, Aug 21, 7:43 AM
aokblast updated this revision to Diff 160710.Thu, Aug 21, 7:50 AM

Add to parent Makefile

Harbormaster completed remote builds in B66398: Diff 160710.Thu, Aug 21, 7:50 AM
lwhsu added a comment.Thu, Aug 21, 9:09 AM

BTW we can mention PR 259673 in the commit message.

markj added inline comments.Thu, Aug 21, 1:12 PM
sys/dev/ichwd/i6300esb.c
34

Just use VENDORID_INTEL from ichwd.h?

55
88
149
169
194

What happens if the watchdog times out and i6300esb_reboot == 0?

228
sys/dev/ichwd/i6300esb.h
47
aokblast updated this revision to Diff 160738.Thu, Aug 21, 1:31 PM

Minor fixes

Harbormaster completed remote builds in B66421: Diff 160738.Thu, Aug 21, 1:31 PM
aokblast added inline comments.Thu, Aug 21, 1:31 PM
sys/dev/ichwd/i6300esb.c
194

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.Thu, Aug 21, 1:41 PM
sys/dev/ichwd/i6300esb.c
43

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.)

128
sys/dev/ichwd/ichwd.h
159

The new driver handles only _5, and the main driver handles only _1. What about the others?

aokblast updated this revision to Diff 160743.Thu, Aug 21, 3:26 PM

Use sysctl_add_proc and remove useless define

Harbormaster completed remote builds in B66422: Diff 160743.Thu, Aug 21, 3:26 PM
markj added inline comments.Fri, Aug 22, 1:23 PM
sys/dev/ichwd/i6300esb.c
114

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.

138

Did you mean to write !! there?

158

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?

268

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.Fri, Aug 22, 1:39 PM
sys/dev/ichwd/i6300esb.c
138

The WDT_OUTPUT_EN is 0 means enable. 1 means disable. So it make sense to only use !

158

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.

268

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.

markj added a comment.Fri, Aug 22, 1:48 PM

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

sys/dev/ichwd/i6300esb.c
158

Right, I guess my question is, why is there a sysctl for it? Is there a scenario where the user may care about this?

268

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.

aokblast updated this revision to Diff 160817.Fri, Aug 22, 2:57 PM

Remove useless sysctl

Harbormaster completed remote builds in B66453: Diff 160817.Fri, Aug 22, 2:57 PM
markj added a comment.Mon, Aug 25, 12:40 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.

aokblast updated this revision to Diff 160926.Mon, Aug 25, 1:31 PM

rename i6300esb -> i6300esbwd

Harbormaster completed remote builds in B66502: Diff 160926.Mon, Aug 25, 1:31 PM
aokblast updated this revision to Diff 160927.Mon, Aug 25, 1:33 PM

Add missing x86 file

Harbormaster completed remote builds in B66503: Diff 160927.Mon, Aug 25, 1:33 PM
aokblast updated this revision to Diff 160928.Mon, Aug 25, 1:42 PM

Add manual

Herald added a subscriber: ziaee. · View Herald TranscriptMon, Aug 25, 1:42 PM
Harbormaster completed remote builds in B66504: Diff 160928.Mon, Aug 25, 1:42 PM
markj accepted this revision.Mon, Aug 25, 1:43 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.Mon, Aug 25, 1:43 PM
aokblast added a comment.Mon, Aug 25, 1:46 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.

markj added a comment.Mon, Aug 25, 1:55 PM
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.

aokblast updated this revision to Diff 160929.Mon, Aug 25, 2:13 PM

Merge driver to ichwd

This revision now requires review to proceed.Mon, Aug 25, 2:13 PM
Harbormaster completed remote builds in B66505: Diff 160929.Mon, Aug 25, 2:13 PM
aokblast added a comment.Mon, Aug 25, 2:14 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:).

aokblast updated this revision to Diff 160930.Mon, Aug 25, 2:17 PM

Remove useless constant

Harbormaster completed remote builds in B66506: Diff 160930.Mon, Aug 25, 2:17 PM
markj accepted this revision.Mon, Aug 25, 2:23 PM
This revision is now accepted and ready to land.Mon, Aug 25, 2:23 PM
lwhsu accepted this revision.Mon, Aug 25, 3:18 PM

Consider a MFC ?

Closed by commit rG2b74ff5fceb6: ichwd: introduce i6300esbwd watch dog driver (authored by aokblast). · Explain WhyMon, Aug 25, 3:46 PM
This revision was automatically updated to reflect the committed changes.
aokblast added a commit: rG2b74ff5fceb6: ichwd: introduce i6300esbwd watch dog driver.

Revision Contents
Changeset List

PathSize
sys/
dev/
ichwd/
46 lines
236 lines
7 lines
2 lines
modules/
i6300esb/
6 lines
sys/
1 line

Diff 160706

View Options

sys/dev/ichwd/i6300esb.h

Loading...
View Options

sys/dev/ichwd/i6300esb.c

Loading...
View Options

sys/dev/ichwd/ichwd.h

Loading...
View Options

sys/dev/ichwd/ichwd.c

Loading...
View Options

sys/modules/i6300esb/Makefile

Loading...
View Options

sys/sys/watchdog.h

Loading...