ichwd: introduce i6300esb watch dog driver

[aokblast]

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

[lwhsu]

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]

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?

[aokblast]

fix errors

[aokblast]

Fix incorret reboot flag

[aokblast]

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

[aokblast]

Use >= instead of >

[aokblast]

Add to parent Makefile

[lwhsu]

BTW we can mention PR 259673 in the commit message.

[markj]

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)

[aokblast]

Minor fixes

[aokblast]

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]

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?

[aokblast]

Use sysctl_add_proc and remove useless define

[markj]

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]

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.

[markj]

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.

[aokblast]

Remove useless sysctl

[markj]

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]

rename i6300esb -> i6300esbwd

[aokblast]

Add missing x86 file

[aokblast]

Add manual

[markj]

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 ↗

(On Diff #160927)

I think this constant is unused now?

[aokblast]

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]

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]

Merge driver to ichwd

[aokblast]

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]

Remove useless constant

[lwhsu]

Consider a MFC ?