Skip to content
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

thinkpads: refactorings + modularization + some new options #101

Closed
wants to merge 2 commits into from

Conversation

@danbst
Copy link
Contributor

danbst commented Feb 19, 2019

  • convert common/cpu/intel and common/pc/laptop/acpi_call to proper
    modules. This allows a) descriptions and b) enable/disable instead of import
  • enable both for thinkpad by default. Disable explicitly in specific profiles
    (for example, AMD ThinkPads don't need those). This is mostly to remove
    repetitions and make a "generic" ThinkPad config (I have E470 but none of
    profiles matches my needs).
  • add hardware.cpu.intel.max-frequency option, which allows reducing
    CPU performance (a bit duplicates powerManagement.cpufreq.max but it's
    a separate mechanism). Perhaps should eventually move to nixpkgs?
  • add hardware.battery.powersave, which enables some non-controversial
    power saving services. Disabled by default.
  • add hardware.battery.optimize, which documents nuances of battery
    lifetime. Basically, ressurects deleted in #100 code
    but disabled by default.
  • convert a comment about fprintd into an option hardware.fingerprint.enable,
    with description

What you think about this? I'm now using those options as:

  imports = [
    <nixos-hardware/lenovo/thinkpad>
  ];
  hardware.battery.powersave = true;
  hardware.battery.optimize = "mostly-pluggedin";
  hardware.fingerprint.enable = true;
  # increase nixos-rebuild time from 4.3s to 4.7s
  hardware.cpu.intel.max-frequency = 90;
- convert `common/cpu/intel` and `common/pc/laptop/acpi_call` to proper
  modules. This allows a) descriptions and b) enable/disable instead of `import`
- enable both for thinkpad by default. Disable explicitly in specific profiles
  (for example, AMD ThinkPads don't need those). This is mostly to remove
  repetitions and make a "generic" ThinkPad config (I have E470 but none of
  profiles matches my needs).
- add `hardware.cpu.intel.max-frequency` option, which allows reducing
  CPU performance (a bit duplicates powerManagement.cpufreq.max but it's
  a separate mechanism). Perhaps should eventually move to nixpkgs?
- add `hardware.battery.powersave`, which enables some non-controversial
  power saving services. Disabled by default.
- add `hardware.battery.optimize`, which documents nuances of battery
  lifetime. Basically, ressurects deleted in #100 code
  but disabled by default.
- convert a comment about fprintd into an option `hardware.fingerprint.enable`,
  with description

What you think about this? I'm now using those options as:
```
  imports = [
    <nixos-hardware/lenovo/thinkpad>
  ];
  hardware.battery.powersave = true;
  hardware.battery.optimize = "mostly-pluggedin";
  hardware.fingerprint.enable = true;
  # increase nixos-rebuild time from 4.3s to 4.7s
  hardware.cpu.intel.max-frequency = 90;
```
@danbst danbst requested review from Mic92 and yegortimoshenko as code owners Feb 19, 2019
@Moredread

This comment has been minimized.

Copy link
Contributor

Moredread commented Feb 19, 2019

I +1 the idea to use modules!

Copy link
Member

yegortimoshenko left a comment

I don't think this repo is appropriate place for modules :-( It's oriented towards set-and-forget whole-system hardware profiles, with no opinionated config options, which differentiates it from nixpkgs. I think a lot of this work better belongs to NixOS and/or upstream (in case of tlp).

enable both for thinkpad by default.

I think acpi_call availability is hardware-specific, not really a meaningful option (perhaps with exception of Libreboot?).

Disable explicitly in specific profiles (for example, AMD ThinkPads don't need those).
This is mostly to remove repetitions and make a "generic" ThinkPad config (I have
E470 but none of profiles matches my needs).

We can create <nixos-hardware/lenovo/thinkpad/e470> config in that case. Having people use generic profiles is problematic, because we can't meaningfully fixup/workaround model-specific problems in the future.

Also, semantically, CPU choice is a tristate, not a boolean: there's no reason why generic ThinkPad would be Intel-specific, with AMD being an override (there are both Intel and AMD ThinkPads).

Overall, I think this is out of scope.

@danbst

This comment has been minimized.

Copy link
Contributor Author

danbst commented Feb 19, 2019

@yegortimoshenko ok, let's discuss some points. I want a to provide constructive arguments in my defense.

add E470 profile

there are many devices in thinkpad family. Let's add profiles only for buggy ones. Lenovo Z510 and Thinkpad T430s are good examples of profiles, which fix real problems with hardware.

OTOH, T440s and T440p are bad examples, in that they enable tpm-rng kernel module. Why? What is that? Shouldn't that be called tpm_rng instead? Which kernel package provides it? Also, it has some security controversies. There is no way to disable that module in end-configuration (this is because module system allows add values to list option, but doesn't allow remove them). It also has a TODO which implies this should be refactored eventually...

But if we add tpm_rng as an option to generic ThinkPad, we can define more details: add description, URLs on topic, specify which models this should work with, disable by default, additional settings (it is probably not wise to enable this option without setting rngd daemon).

set-and-forget whole-system hardware profiles

Having people use generic profiles is problematic, because we can't meaningfully fixup/workaround model-specific problems in the future.

I agree with latter, we want workaround model-specific problems later. But I don't see how making separate profile helps here. Instead we can do some runtime checks in "generic" model to apply fixes. In theory, we can also add an option:

options.hardware.laptop.model

and require user to set correct model ("E470" in my case), and then apply changes based on this.

I think a lot of this work better belongs to NixOS

Dunno. I'm not very fond of intel/Thinkpad specific workarounds in NixOS. Also, it's easier to define options here, in-place, than define them in nixpkgs and later declare them in nixos-hardware. It also adds dependency on specific nixpkgs revision/release - not a great thing.

Maybe later?

I think acpi_call availability is hardware-specific

It indeed is, only newer thinkpads support it. Though it is a must-have for TLP to work properly. 6 profiles had imported it explicitly. T430, T440p, T460s, X1, X250, X270 supports it, but didn't enable. Was that a bug or a feature? Nobody knows. I can only assume it was a bug. In that case 12 profiles need acpi_call. This now makes it a good candidate to be "default"!

If we never merge this, then potential users of NixOS on X1 can never find out that their TLP settings don't work properly. Or, worse, they can fix stuff themselves and not upstream here.

with no opinionated config options

the only opinionated settings are acpi_call and cpu.intel, but those are more "default", rather than "opinions". If we pick some random ThinkPad, it is 95% Intel based, and 80% new enough for acpi_call (very rough numbers). So generic module will work in 80% cases at least.

There is room for improvements here.

Overall, I think this is out of scope.

I wanted battery powersaving recommendations since this repo was introduced. So I'd like such "recommendations" to be part of this repo.

@danbst

This comment has been minimized.

Copy link
Contributor Author

danbst commented Feb 19, 2019

btw, cc @eyJhb and @azazel75 on X1 bug assumption. What is your nix-shell -p tlp --run 'sudo tlp stat | grep tpacpi-bat'?

EDIT: ah, X1 and X1 6th gen are different profiles, sorry. X1 6th gen does have acpi_call module

@azazel75

This comment has been minimized.

Copy link
Contributor

azazel75 commented Feb 19, 2019

@danbst Thanks for doing this, I think this repo can be the right place to condense "sane/good defaults". The answer to your question is:

tpacpi-bat = active
tpacpi-bat.BAT0.startThreshold                              =     75 [%]
tpacpi-bat.BAT0.stopThreshold                               =     80 [%]
tpacpi-bat.BAT0.forceDischarge                              =      0
@eyJhb

This comment has been minimized.

Copy link
Member

eyJhb commented Feb 20, 2019

@danbst running with the below, but it is of course changed, and I run a Lenovo Thinkpad x230 ;)

tpacpi-bat = active
tpacpi-bat.BAT0.startThreshold                              =     60 [%]
tpacpi-bat.BAT0.stopThreshold                               =     85 [%]
tpacpi-bat.BAT0.forceDischarge                              =      0
@yegortimoshenko

This comment has been minimized.

Copy link
Member

yegortimoshenko commented Feb 20, 2019

OTOH, T440s and T440p are bad examples, in that they enable tpm-rng kernel module. Also, it has some security controversies.

Linux kernel policy is to use inputs from all available devices, including black box TPM instructions/modules like those on Intel chips (source, sorry it's inflammatory but it's the only one I could find).

Meaning, it's not really possible to control /dev/{u,}random sources and whether user trusts them, because of the kernel policy, not because of this repo. We just enable hardware support.

But if we add tpm_rng as an option to generic ThinkPad, we can define more details: add description, URLs on topic, specify which models this should work with, disable by default, additional settings (it is probably not wise to enable this option without setting rngd daemon).

Again, this is better suited for Nixpkgs. Fundamentally, the only reason people use this repo at all is so that they don't have to set up hardware themselves (we set up everything there is on the motherboard).

There is no way to disable that module in end-configuration (this is because module system allows add values to list option, but doesn't allow remove them).

boot.blacklistedKernelModules.

By the way, this is a solvable problem, NixOS module system can support exclusions without a major rewrite. As a temporary workaround, there are various excludePackages options.

options.hardware.laptop.model

User-defined enums are bad idea: there are quite a few ways to write down the same model. And no one will set it post-hoc, while we should be able to push quirk fixes to devices that might have previously not required any. We enforce leaf profiles so that we can dispatch workarounds based on specific model of the device.

Shouldn't that be called tpm_rng instead?

torvalds/linux@6e592a0#diff-7002202c012eaed630466c5a9d898562L318

I agree with latter, we want workaround model-specific problems later. But I don't see how making separate profile helps here. Instead we can do some runtime checks in "generic" model to apply fixes.

Literal runtime checks are the goal: much of what is not specific to a certain model can and should migrate to nixos-generate-config.

Let's add profiles only for buggy ones.

It's fundamentally unknowable which ones are, or will be buggy. It depends on kernel version: devices do break over time. Case in point, <nixos-hardware/toshiba/swanky> sound setup has changed a few times over recent kernel versions.

It indeed is, only newer thinkpads support it. Though it is a must-have for TLP to work properly. 6 profiles had imported it explicitly. T430, T440p, T460s, X1, X250, X270 supports it, but didn't enable. Was that a bug or a feature? Nobody knows. I can only assume it was a bug. In that case 12 profiles need acpi_call. This now makes it a good candidate to be "default"!

If we never merge this, then potential users of NixOS on X1 can never find out that their TLP settings don't work properly. Or, worse, they can fix stuff themselves and not upstream here.

It's debatable whether this is a good idea (there is a huge lot of ThinkPads in the wild that do not support acpi_call), that said it's just a question of profile inheritance, which is feasible with profile system.

I wanted battery powersaving recommendations since this repo was introduced. So I'd like such "recommendations" to be part of this repo.

I really think that Lenovo default (96-100%) could go into services.tlp, but 75/80% is extremely opinionated and should be set up explicitly with users knowing what they are doing (they are forfeiting 20% of their battery capacity).

services.tlp could accept an attrset with variables instead of types.lines so that overriding is easier. I am willing to help with this work.

@Moredread

This comment has been minimized.

Copy link
Contributor

Moredread commented Feb 20, 2019

Malicious inputs shouldn't be a problem as long as the PRNG is seeded with enough entropy from a secure source. As tpm is not the only source this isn't an issue.

Bad inputs can't hurt the security as long as the PRNG isn't broken. And users can input data to /dev/urandom anyways, so disabling the tpm wouldn't prevent an attack anyways.

A good summary of this is https://www.2uo.de/myths-about-urandom/

@danbst

This comment has been minimized.

Copy link
Contributor Author

danbst commented Feb 20, 2019

(source, sorry it's inflammatory but it's the only one I could find)

👍

boot.blacklistedKernelModules.

👍

By the way, this is a solvable problem, NixOS module system can support exclusions without a major rewrite.

I'd like to know details here.

torvalds/linux@6e592a0#diff-7002202c012eaed630466c5a9d898562L318

👍

It's debatable whether this is a good idea (there is a huge lot of ThinkPads in the wild that do not support acpi_call), that said it's just a question of profile inheritance, which is feasible with profile system.

Also, it is an open question if 6 profiles I've listed above are incomplete. Hardware supports acpi_call but profiles never specify that. Same should go about tpm-rng - what other profiles should include tpm-rng? Why would some include, other don't if hardware supports it? (https://pcsupport.lenovo.com/ua/en/downloads/ds501051 lists some TPM-enabled models, including T460s). Ideally we'd like to distinguish: "is supported", "is not supported" and "is unknown if is supported".

but 75/80% is extremely opinionated and should be set up explicitly with users knowing what they are doing

Ok.

Literal runtime checks are the goal: much of what is not specific to a certain model can and should migrate to nixos-generate-config.

BTW, it may be a good idea to teach nixos-generate-config --show-hardware-config to put device model automatically under hardware.model namespace. A quick check shows that

$ sudo dmidecode -H 0x000E | grep -e "Product Name" -e "Version" -e "Manufacturer"
        Manufacturer: LENOVO
        Product Name: 20H1S00A00
        Version: ThinkPad E470

returns enough information to use then in this repo

{ config, lib, ...}: let
  thinkpadModel = let
    matches = builtins.match "ThinkPad (.*)" config.hardware.model.Version;
  in if matches != null && matches != [] then head matches else null;

  modelHacks = {
    E470 = {
      ...
    };
    ...
  };
  
in {
  config = lib.mkMerge ...convert modelHacks into proper configs...;
}

We can go even further and detect CPU model in nixos-generate-config. Then we don't have to guess whether to enable Intel specifics or not.

@danbst

This comment has been minimized.

Copy link
Contributor Author

danbst commented Feb 28, 2019

maybe another time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.