tboot-devel — Developer support

Re: [tboot-devel] [PATCH] 20_linux_tboot: efi logic was inverted

[Tony C. <tc...@re...>]

On 6/11/2022 1:41 PM, Timo Lindfors wrote:
> On Sat, 11 Jun 2022, Tony Camuso wrote:
>> OK, so something is going wrong with the information that tboot is
>> forwarding to the kernel launch.
>>
>> On the efi system, with "noefi" removed from the grub command line,
>> the system boots.
>>
>> With "noefi" in the grub command line, Device Mapper cannot find
>> the root and swap devices and drops to the dracut prompt.
>>
>> How can I determine what info efi is providing that tboot is not?
>>
>> Where can I instrument the code to gain that visibility?
> 
> I'm not too familiar with dracut but I'd imagine you can unpack the initramfs and investigate how it works. It might be easiest to spawn a shell at some point.
> 
> Can you reproduce the problem if you do a fresh install? What distribution is this? If you have easy steps to reproduce this I can try to take a look. I'm normally on Debian so will need good instructions for other distros :)
> 
> -Timo
> 

I'm using a RHEL-8.5 based on 5.14 kernel. CentOS 8.5 is the same thing.

You can probably reproduce it on a debian system with storage configured
as VROC RAID. You will need a motherboard and intel cpu that can implement
VROC RAID.

lenovo-st250v2 is the platform I'm using to reproduce the problem.

See...
https://www.intel.com/content/www/us/en/support/articles/000026106/memory-and-storage/ssd-software.html
https://www.intel.com/content/www/us/en/support/articles/000030445/memory-and-storage/ssd-management-tools.html

RAID info
====================================================================

# cat /proc/mdstat
Personalities : [raid0]
md126 : active raid0 sda[1] sdb[0]
       1855842304 blocks super external:/md127/0 128k chunks
       
md127 : inactive sda[1](S) sdb[0](S)
       10402 blocks super external:imsm
        
unused devices: <none>

# cat /etc/mdadm.conf
# mdadm.conf written out by anaconda
MAILADDR root
AUTO +imsm +1.x -all
ARRAY /dev/md/Volume0_0 UUID=8061c4cf:06de8a59:a9eefb7e:3edb011a
ARRAY /dev/md/imsm UUID=549c2ba4:1e03463b:d429e75b:398c67a3
---------------------------------------------------------------------

Physical Volume, Volume Group, and Logical Volume info
=====================================================================
# pvs
   PV           VG                     Fmt  Attr PSize  PFree
   /dev/md126p3 rhel_lenovo-st250v2-02 lvm2 a--  <1.73t    0

# vgs
   VG                     #PV #LV #SN Attr   VSize  VFree
   rhel_lenovo-st250v2-02   1   4   0 wz--n- <1.73t    0

# lvs
   LV   VG                     Attr       LSize
   home rhel_lenovo-st250v2-02 -wi-ao---- 500.00g
   root rhel_lenovo-st250v2-02 -wi-ao----  70.00g
   swap rhel_lenovo-st250v2-02 -wi-ao----  31.47g
   work rhel_lenovo-st250v2-02 -wi-ao----  <1.14t
-----------------------------------------------------------------------

By the time initramfs is running, it's too late, since the Device Mapper
cannot find the logical volumes.

*************
*   NOTE    *
*************
There are differences in the memmap that tboot hands off to the initramfs
with and without the "noefi" directive.

Failed with "noefi"
===================
TBOOT: get_highest_sized_ram: size 14e99600 -> base 62585000, size 24949000
   :                                                 ^^^^^^^^       ^^^^^^^^
TBOOT:  /swap console=ttyS0,115200N81 intel_iommu=on noefi
TBOOT: EFI memmap: memmap base: 0x69808, memmap size: 0x14a0
                                 ^^^^^^^               ^^^^^^

Successful boot with efi
========================
TBOOT: get_highest_sized_ram: size 14e99600 -> base 62045000 size 24789000
   :                                                 ^^^^^^^^      ^^^^^^^^
TBOOT:  /swap console=ttyS0,115200N81 intel_iommu=on
TBOOT: EFI memmap: memmap base: 0x69808, memmap size: 0x1b00
                                 ^^^^^^^               ^^^^^^

Re: [tboot-devel] [PATCH] 20_linux_tboot: efi logic was inverted

[Timo L. <tim...@ik...>]

On Sat, 11 Jun 2022, Tony Camuso wrote:
> OK, so something is going wrong with the information that tboot is
> forwarding to the kernel launch.
>
> On the efi system, with "noefi" removed from the grub command line,
> the system boots.
>
> With "noefi" in the grub command line, Device Mapper cannot find
> the root and swap devices and drops to the dracut prompt.
>
> How can I determine what info efi is providing that tboot is not?
>
> Where can I instrument the code to gain that visibility?

I'm not too familiar with dracut but I'd imagine you can unpack the 
initramfs and investigate how it works. It might be easiest to spawn a 
shell at some point.

Can you reproduce the problem if you do a fresh install? What distribution 
is this? If you have easy steps to reproduce this I can try to take a 
look. I'm normally on Debian so will need good instructions for other 
distros :)

-Timo

Re: [tboot-devel] [PATCH] 20_linux_tboot: efi logic was inverted

[Tony C. <tc...@re...>]

On 6/11/2022 3:24 AM, Timo Lindfors wrote:
> 
> On Fri, 10 Jun 2022, Tony Camuso wrote:
>> If your system is booting in efi mode, then it needs efi.
>> If it's booting in legacy bios mode, then it doesn't need efi
> 
> Commit https://sourceforge.net/p/tboot/code/ci/aad782103a6e
> 
> says that
> 
> "Note that booting *without* noefi is a security risk since the EFI runtime is not part of the trust base after a dynamic launch."
> 
> 
> This suggests to me that you need to use "noefi" on an EFI system to minimize risks.

OK, so something is going wrong with the information that tboot is
forwarding to the kernel launch.

On the efi system, with "noefi" removed from the grub command line,
the system boots.

With "noefi" in the grub command line, Device Mapper cannot find
the root and swap devices and drops to the dracut prompt.

How can I determine what info efi is providing that tboot is not?

Where can I instrument the code to gain that visibility?


> 
> -Timo
> 

Re: [tboot-devel] [PATCH] 20_linux_tboot: efi logic was inverted

[Timo L. <tim...@ik...>]

On Fri, 10 Jun 2022, Tony Camuso wrote:
> If your system is booting in efi mode, then it needs efi.
> If it's booting in legacy bios mode, then it doesn't need efi

Commit https://sourceforge.net/p/tboot/code/ci/aad782103a6e

says that

"Note that booting *without* noefi is a security risk since the EFI 
runtime is not part of the trust base after a dynamic launch."


This suggests to me that you need to use "noefi" on an EFI system to 
minimize risks.

-Timo

Re: [tboot-devel] [PATCH] 20_linux_tboot: efi logic was inverted

[Tony C. <tc...@re...>]

> Date: Fri, 10 Jun 2022 02:13:16 +0000
> From: Derek Dolney <z2...@po...>
> To: tbo...@li...
> Subject: Re: [tboot-devel] [PATCH] 20_linux_tboot: efi logic was
> 	inverted
> Message-ID: <18a...@po...>
> Content-Type: text/plain; charset=windows-1252
> 
> On a Thinkpad T430 this would break my boot. I have /sys/firmware/efi
> and I need the noefi kernel param, otherwise it just reboots itself
> about 10-20 sec after SENTER

If your system is booting in efi mode, then it needs efi.
If it's booting in legacy bios mode, then it doesn't need efi

That's how the logic in this grub script is supposed to work.
The current logic is inverted and needs to be corrected so it
works that way.

Have you tried grub2-mkconfig with this corrected grub script?
You can also try editing the module2 stanza in the grub menu with
and without 'noefi' at the end of it.

If your system is booting legacy mode and needs efi to launch
tboot, or if it's booting efi and needs 'noefi' stanza, then
something else is broken.

> On 6/9/22 3:04 PM, Tony Camuso wrote:
>> # HG changeset patch
>> # User Tony Camuso <tc...@re...>
>> # Date 1654800659 14400
>> #????? Thu Jun 09 14:50:59 2022 -0400
>> # Node ID be868f53407d4460491a0e77e5165025153b0329
>> # Parent? 206a47f3e9d2c18c8a3db082216ee6fc3c5d475c
>> 20_linux_tboot: efi logic was inverted
>>
>> There was a RAID0 system that could boot normally, but not through
>> a tboot launch. The problem was that the logic in this script
>> incorrectly appended 'noefi' to the grub.cfg module2 kernel stanza.
>>
>> When 'noefi' was removed from that stanza, the system could boot
>> through a tboot launch. This patch corrects the logic in the script.
>>
>> diff -r 206a47f3e9d2 -r be868f53407d tboot/20_linux_tboot
>> --- a/tboot/20_linux_tboot??? Thu Mar 17 23:58:50 2022 +0200
>> +++ b/tboot/20_linux_tboot??? Thu Jun 09 14:50:59 2022 -0400
>> @@ -105,11 +105,11 @@
>> ?? if [ -d /sys/firmware/efi ] ; then
>> ?????? mb_directive="multiboot2"
>> ?????? mb_mod_directive="module2"
>> -????? mb_extra_kernel="noefi"
>> +????? mb_extra_kernel=""
>> ?? else
>> ?????? mb_directive="multiboot2"
>> ?????? mb_mod_directive="module2"
>> -????? mb_extra_kernel=""
>> +????? mb_extra_kernel="noefi"
>> ?? fi
>> ?
>> ?? printf "menuentry '${title}' ${CLASS} {\n" "${os}" "${tboot_version}"
>> "${version}"
>>
>>
>>
>> _______________________________________________
>> tboot-devel mailing list
>> tbo...@li...
>> https://lists.sourceforge.net/lists/listinfo/tboot-devel
> 
> 
> 
> ------------------------------
> 
> 
> 
> ------------------------------
> 
> Subject: Digest Footer
> 
> _______________________________________________
> tboot-devel mailing list
> tbo...@li...
> https://lists.sourceforge.net/lists/listinfo/tboot-devel
> 
> 
> ------------------------------
> 
> End of tboot-devel Digest, Vol 147, Issue 2
> *******************************************
> 

Re: [tboot-devel] [PATCH] 20_linux_tboot: efi logic was inverted

[Derek D. <z2...@po...>]

On a Thinkpad T430 this would break my boot. I have /sys/firmware/efi
and I need the noefi kernel param, otherwise it just reboots itself
about 10-20 sec after SENTER

On 6/9/22 3:04 PM, Tony Camuso wrote:
> # HG changeset patch
> # User Tony Camuso <tc...@re...>
> # Date 1654800659 14400
> #      Thu Jun 09 14:50:59 2022 -0400
> # Node ID be868f53407d4460491a0e77e5165025153b0329
> # Parent  206a47f3e9d2c18c8a3db082216ee6fc3c5d475c
> 20_linux_tboot: efi logic was inverted
> 
> There was a RAID0 system that could boot normally, but not through
> a tboot launch. The problem was that the logic in this script
> incorrectly appended 'noefi' to the grub.cfg module2 kernel stanza.
> 
> When 'noefi' was removed from that stanza, the system could boot
> through a tboot launch. This patch corrects the logic in the script.
> 
> diff -r 206a47f3e9d2 -r be868f53407d tboot/20_linux_tboot
> --- a/tboot/20_linux_tboot    Thu Mar 17 23:58:50 2022 +0200
> +++ b/tboot/20_linux_tboot    Thu Jun 09 14:50:59 2022 -0400
> @@ -105,11 +105,11 @@
>    if [ -d /sys/firmware/efi ] ; then
>        mb_directive="multiboot2"
>        mb_mod_directive="module2"
> -      mb_extra_kernel="noefi"
> +      mb_extra_kernel=""
>    else
>        mb_directive="multiboot2"
>        mb_mod_directive="module2"
> -      mb_extra_kernel=""
> +      mb_extra_kernel="noefi"
>    fi
>  
>    printf "menuentry '${title}' ${CLASS} {\n" "${os}" "${tboot_version}"
> "${version}"
> 
> 
> 
> _______________________________________________
> tboot-devel mailing list
> tbo...@li...
> https://lists.sourceforge.net/lists/listinfo/tboot-devel

[tboot-devel] [PATCH] 20_linux_tboot: efi logic was inverted

[Tony C. <tc...@re...>]

# HG changeset patch
# User Tony Camuso <tc...@re...>
# Date 1654800659 14400
#      Thu Jun 09 14:50:59 2022 -0400
# Node ID be868f53407d4460491a0e77e5165025153b0329
# Parent  206a47f3e9d2c18c8a3db082216ee6fc3c5d475c
20_linux_tboot: efi logic was inverted

There was a RAID0 system that could boot normally, but not through
a tboot launch. The problem was that the logic in this script
incorrectly appended 'noefi' to the grub.cfg module2 kernel stanza.

When 'noefi' was removed from that stanza, the system could boot
through a tboot launch. This patch corrects the logic in the script.

diff -r 206a47f3e9d2 -r be868f53407d tboot/20_linux_tboot
--- a/tboot/20_linux_tboot	Thu Mar 17 23:58:50 2022 +0200
+++ b/tboot/20_linux_tboot	Thu Jun 09 14:50:59 2022 -0400
@@ -105,11 +105,11 @@
    if [ -d /sys/firmware/efi ] ; then
        mb_directive="multiboot2"
        mb_mod_directive="module2"
-      mb_extra_kernel="noefi"
+      mb_extra_kernel=""
    else
        mb_directive="multiboot2"
        mb_mod_directive="module2"
-      mb_extra_kernel=""
+      mb_extra_kernel="noefi"
    fi
  
    printf "menuentry '${title}' ${CLASS} {\n" "${os}" "${tboot_version}" "${version}"

Re: [tboot-devel] suspend problem since kernel 5.15

[Randzio, P. <paw...@in...>]

Hi Timo,

No, unfortunately, haha. I've been too early to call the victory.
I've sent it for testing and it came out that our client platforms, like AlderLake and upcoming CPUs work fine with that patch, but server platforms may fail with it, I got ambivalent results on that.

I'm awaiting further testing results with some renditions of that patch to check whether this one particular part needs some tweaking or maybe the fix needs to be a bit more complex.

Nevertheless, I started talks to take over maintenance of TBOOT's kernel module and hopefully when the patch is fully functional and is included in the kernel I'll become the maintainer (and reviewer) for it.

-Paweł


-----Original Message-----
From: Timo Lindfors <tim...@ik...> 
Sent: Wednesday, June 8, 2022 8:13 AM
To: Randzio, Pawel <paw...@in...>
Cc: tbo...@li...
Subject: Re: [tboot-devel] suspend problem since kernel 5.15


On Fri, 27 May 2022, Randzio, Pawel wrote:
> I wish to gladly inform you that I've fixed the bug preventing suspend with Tboot.

Great news! Has this patch already been sent upstream?

-Timo

---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.

Re: [tboot-devel] suspend problem since kernel 5.15

[Timo L. <tim...@ik...>]

On Fri, 27 May 2022, Randzio, Pawel wrote:
> I wish to gladly inform you that I've fixed the bug preventing suspend with Tboot.

Great news! Has this patch already been sent upstream?

-Timo

Re: [tboot-devel] suspend problem since kernel 5.15

[Derek D. <z2...@po...>]

Hi, Paweł,

Thanks for your help. I was just starting to think the same thing, the
code seemed to be trying to put all cpus into WFS. I tested your patch
on the 5.12 commit that broke and also the latest 5.18 git kernel and
indeed it works fine. On a quad core I can see 3 cores go into WFS and
then tboot continues take bfs to s3 state. Very good!

Derek

On 5/27/22 9:50 AM, Randzio, Pawel wrote:
> Hi Derek,
> 
> I wish to gladly inform you that I've fixed the bug preventing suspend with Tboot.
> 
> It was in the piece of code that you've highlighted - the atomic increment on ap_wfs_count variable need have been moved into the else clause of the following if statement, something like this:
> 
> static int tboot_dying_cpu(unsigned int cpu) {
> 	if (num_online_cpus() == 1) {
> 		if (tboot_wait_for_aps(atomic_read(&ap_wfs_count)))
> 			return -EBUSY;
> 	} else
> 	 	atomic_inc(&ap_wfs_count);
> 	return 0;
> }
> 
> Otherwise, when the procedure shutdown all APs, BSP was also counted in as one of them, thus creating a mismatch between what kernel had counted and what TBOOT reported back to it.
> This caused a kernel panic, which froze the machine while putting it into suspend.
> 
> I've sent a version of TBOOT Live Image equipped with this patch of the kernel to our validation teams and I'm awaiting their response after the weekend.
> If I get positive results from their tests I will try and have this patch attached to the official kernel repository and featured in versions 5.19 and up.
> 
> Kind regards,
> Paweł
> 
> 
> 
> 
> 
> -----Original Message-----
> From: Randzio, Pawel 
> Sent: Friday, May 13, 2022 12:12 PM
> To: 'Derek Dolney' <z2...@po...>; Łukasz Hawryłko <lu...@ha...>
> Cc: tbo...@li...
> Subject: RE: [tboot-devel] suspend problem since kernel 5.15
> 
> Hi Derek,
> 
> I've also been picking at that issue for the past few months and reached the same wall as you have with the -EBUSY callback return, although your message kind of gives me a new idea for where to look for the root cause as it seems I have not tracked all possible callbacks.
> I'm not a kernel developer either and honestly debugging that S3 issue is troublesome to me too, to say the least. If anyone on this mailing list has any idea how to further the debugging or even better - solve this issue, please feel free to share ideas.
> 
> On a side note, please add Vincent into the communication, that might speed up the process.
> Vincent may add others that could know what might be going on with that issue.
> 
> Thanks,
> Paweł
> 
> -----Original Message-----
> From: Derek Dolney <z2...@po...> 
> Sent: Thursday, May 12, 2022 6:20 PM
> To: Łukasz Hawryłko <lu...@ha...>; tbo...@li...
> Subject: Re: [tboot-devel] suspend problem since kernel 5.15
> 
> I have been working on this as best I can. However, I confess that I am not a kernel developer and have really no understanding of these tboot internals. Nevertheless here is a brief update. Please anyone feel free to share any ideas how to move forward to some resolution.
> 
> I got a desktop machine with rs232 serial output running tboot and reproduced the suspend problem that way and with this setup I can collect kernel printk and also cpu hotplug (cpuhp) tracing output. I have also thankfully got quite a bit of help from Vincent Donnefort who wrote the cpuhp changes (the commit I posted) that have exposed the issue. He has been very helpful, let me try to tell you what we have figured out.
> 
> On suspend, I get into the tboot callback:
> 
> static int tboot_dying_cpu(unsigned int cpu) {
> 	atomic_inc(&ap_wfs_count);
> 	if (num_online_cpus() == 1) {
> 		if (tboot_wait_for_aps(atomic_read(&ap_wfs_count)))
> 			return -EBUSY;
> 	}
> 	return 0;
> }
> 
> but the tboot_wait_for_aps times out for me so the callback returns EBUSY. The problem with that happening is that there is not a rollback mechanism in place at this point in the cpuhp sequence. So I mean from cpuhp point of view, there is not even a mechanism to handle the tboot callback failure. Besides that, we don't know what could be a sensible thing to do in the case of EBUSY. What does it mean tboot is busy and what should be done about it? Please help us to understand.
> 
> 
> _______________________________________________
> tboot-devel mailing list
> tbo...@li...
> https://lists.sourceforge.net/lists/listinfo/tboot-devel
> ---------------------------------------------------------------------
> Intel Technology Poland sp. z o.o.
> ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
> Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
> This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.
> 

Re: [tboot-devel] suspend problem since kernel 5.15

[Randzio, P. <paw...@in...>]

Hi Derek,

I wish to gladly inform you that I've fixed the bug preventing suspend with Tboot.

It was in the piece of code that you've highlighted - the atomic increment on ap_wfs_count variable need have been moved into the else clause of the following if statement, something like this:

static int tboot_dying_cpu(unsigned int cpu) {
	if (num_online_cpus() == 1) {
		if (tboot_wait_for_aps(atomic_read(&ap_wfs_count)))
			return -EBUSY;
	} else
	 	atomic_inc(&ap_wfs_count);
	return 0;
}

Otherwise, when the procedure shutdown all APs, BSP was also counted in as one of them, thus creating a mismatch between what kernel had counted and what TBOOT reported back to it.
This caused a kernel panic, which froze the machine while putting it into suspend.

I've sent a version of TBOOT Live Image equipped with this patch of the kernel to our validation teams and I'm awaiting their response after the weekend.
If I get positive results from their tests I will try and have this patch attached to the official kernel repository and featured in versions 5.19 and up.

Kind regards,
Paweł





-----Original Message-----
From: Randzio, Pawel 
Sent: Friday, May 13, 2022 12:12 PM
To: 'Derek Dolney' <z2...@po...>; Łukasz Hawryłko <lu...@ha...>
Cc: tbo...@li...
Subject: RE: [tboot-devel] suspend problem since kernel 5.15

Hi Derek,

I've also been picking at that issue for the past few months and reached the same wall as you have with the -EBUSY callback return, although your message kind of gives me a new idea for where to look for the root cause as it seems I have not tracked all possible callbacks.
I'm not a kernel developer either and honestly debugging that S3 issue is troublesome to me too, to say the least. If anyone on this mailing list has any idea how to further the debugging or even better - solve this issue, please feel free to share ideas.

On a side note, please add Vincent into the communication, that might speed up the process.
Vincent may add others that could know what might be going on with that issue.

Thanks,
Paweł

-----Original Message-----
From: Derek Dolney <z2...@po...> 
Sent: Thursday, May 12, 2022 6:20 PM
To: Łukasz Hawryłko <lu...@ha...>; tbo...@li...
Subject: Re: [tboot-devel] suspend problem since kernel 5.15

I have been working on this as best I can. However, I confess that I am not a kernel developer and have really no understanding of these tboot internals. Nevertheless here is a brief update. Please anyone feel free to share any ideas how to move forward to some resolution.

I got a desktop machine with rs232 serial output running tboot and reproduced the suspend problem that way and with this setup I can collect kernel printk and also cpu hotplug (cpuhp) tracing output. I have also thankfully got quite a bit of help from Vincent Donnefort who wrote the cpuhp changes (the commit I posted) that have exposed the issue. He has been very helpful, let me try to tell you what we have figured out.

On suspend, I get into the tboot callback:

static int tboot_dying_cpu(unsigned int cpu) {
	atomic_inc(&ap_wfs_count);
	if (num_online_cpus() == 1) {
		if (tboot_wait_for_aps(atomic_read(&ap_wfs_count)))
			return -EBUSY;
	}
	return 0;
}

but the tboot_wait_for_aps times out for me so the callback returns EBUSY. The problem with that happening is that there is not a rollback mechanism in place at this point in the cpuhp sequence. So I mean from cpuhp point of view, there is not even a mechanism to handle the tboot callback failure. Besides that, we don't know what could be a sensible thing to do in the case of EBUSY. What does it mean tboot is busy and what should be done about it? Please help us to understand.


_______________________________________________
tboot-devel mailing list
tbo...@li...
https://lists.sourceforge.net/lists/listinfo/tboot-devel
---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.

Re: [tboot-devel] suspend problem since kernel 5.15

[Randzio, P. <paw...@in...>]

Hi Derek,

I've also been picking at that issue for the past few months and reached the same wall as you have with the -EBUSY callback return, although your message kind of gives me a new idea for where to look for the root cause as it seems I have not tracked all possible callbacks.
I'm not a kernel developer either and honestly debugging that S3 issue is troublesome to me too, to say the least. If anyone on this mailing list has any idea how to further the debugging or even better - solve this issue, please feel free to share ideas.

On a side note, please add Vincent into the communication, that might speed up the process.
Vincent may add others that could know what might be going on with that issue.

Thanks,
Paweł

-----Original Message-----
From: Derek Dolney <z2...@po...> 
Sent: Thursday, May 12, 2022 6:20 PM
To: Łukasz Hawryłko <lu...@ha...>; tbo...@li...
Subject: Re: [tboot-devel] suspend problem since kernel 5.15

I have been working on this as best I can. However, I confess that I am not a kernel developer and have really no understanding of these tboot internals. Nevertheless here is a brief update. Please anyone feel free to share any ideas how to move forward to some resolution.

I got a desktop machine with rs232 serial output running tboot and reproduced the suspend problem that way and with this setup I can collect kernel printk and also cpu hotplug (cpuhp) tracing output. I have also thankfully got quite a bit of help from Vincent Donnefort who wrote the cpuhp changes (the commit I posted) that have exposed the issue. He has been very helpful, let me try to tell you what we have figured out.

On suspend, I get into the tboot callback:

static int tboot_dying_cpu(unsigned int cpu) {
	atomic_inc(&ap_wfs_count);
	if (num_online_cpus() == 1) {
		if (tboot_wait_for_aps(atomic_read(&ap_wfs_count)))
			return -EBUSY;
	}
	return 0;
}

but the tboot_wait_for_aps times out for me so the callback returns EBUSY. The problem with that happening is that there is not a rollback mechanism in place at this point in the cpuhp sequence. So I mean from cpuhp point of view, there is not even a mechanism to handle the tboot callback failure. Besides that, we don't know what could be a sensible thing to do in the case of EBUSY. What does it mean tboot is busy and what should be done about it? Please help us to understand.


_______________________________________________
tboot-devel mailing list
tbo...@li...
https://lists.sourceforge.net/lists/listinfo/tboot-devel
---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.

Re: [tboot-devel] suspend problem since kernel 5.15

[Derek D. <z2...@po...>]

I have been working on this as best I can. However, I confess that I am
not a kernel developer and have really no understanding of these tboot
internals. Nevertheless here is a brief update. Please anyone feel free
to share any ideas how to move forward to some resolution.

I got a desktop machine with rs232 serial output running tboot and
reproduced the suspend problem that way and with this setup I can
collect kernel printk and also cpu hotplug (cpuhp) tracing output. I
have also thankfully got quite a bit of help from Vincent Donnefort who
wrote the cpuhp changes (the commit I posted) that have exposed the
issue. He has been very helpful, let me try to tell you what we have
figured out.

On suspend, I get into the tboot callback:

static int tboot_dying_cpu(unsigned int cpu)
{
	atomic_inc(&ap_wfs_count);
	if (num_online_cpus() == 1) {
		if (tboot_wait_for_aps(atomic_read(&ap_wfs_count)))
			return -EBUSY;
	}
	return 0;
}

but the tboot_wait_for_aps times out for me so the callback returns
EBUSY. The problem with that happening is that there is not a rollback
mechanism in place at this point in the cpuhp sequence. So I mean from
cpuhp point of view, there is not even a mechanism to handle the tboot
callback failure. Besides that, we don't know what could be a sensible
thing to do in the case of EBUSY. What does it mean tboot is busy and
what should be done about it? Please help us to understand.

Re: [tboot-devel] 11th Gen SINIT ACM

[Timo L. <tim...@ik...>]

Hi,

On Mon, 25 Apr 2022, Jason Andryuk wrote:
>>
>> https://cdrdv2.intel.com/v1/dl/getContent/630744?explicitVersion=true
>>
>> Jeevan Bhungal
>
> Great!  Yeah, the internal link does not work, but the direct link
> does.  Thank you, Jeevan.

Is the plan still to have these listed on

https://www.intel.com/content/www/us/en/developer/articles/tool/intel-trusted-execution-technology.html

along with other ACMs? The reason why I am asking is that I am packaging 
these in Debian non-free intel-acm package and have some automation for 
tracking changes on that page to notice new ACMs.

-Timo

Re: [tboot-devel] status of the grub patch to support multiple SINIT modules?

[Timo L. <tim...@ik...>]

Hi,

On Tue, 22 Mar 2022, Łukasz Hawryłko wrote:
> On Sat, 2022-03-12 at 09:34 +0200, Timo Lindfors wrote:
>> On Fri, 11 Mar 2022, Łukasz Hawryłko wrote:
>>> I see that you have quite complex environment for testing tboot, if I
>>> find my old GRUB patch and prepare patch for tboot that combined should
>>> fix the issue, will you be able to run tests?
>>
>> Yes, I'm happy to run tests :)
>
> I am attaching two patches:
>
> GRUB:
> multiboot2__Implement_the_new_module_load_and_preferences_tag.patch
>
> tboot:
> Use_multiboot2_module_load_preference_tag.patch
>
> On my test platform they fix the issue. Please check how they work on
> your environment.

Sorry for the long delay. I am also able to load all ACM modules with 
these patches.

TPM 2.0 with UEFI:

https://lindi.iki.fi/lindi/tboot/smoketest/test_results/c99c68f2-9651-42dd-b822-9936868bc12a/output/serial.log

TPM 1.2 with BIOS:

https://lindi.iki.fi/lindi/tboot/smoketest/test_results/f6ba18b4-b259-4b68-8eb4-05c0b758996b/output/serial.log

Test script:

https://lindi.iki.fi/lindi/tboot/smoketest/test_results/c99c68f2-9651-42dd-b822-9936868bc12a/input/main


-Timo

Re: [tboot-devel] 11th Gen SINIT ACM

[Jason A. <jan...@gm...>]

On Mon, Apr 25, 2022 at 12:48 PM Bhungal, Jeevan S
<jee...@in...> wrote:
>
> Hi Jason,
>
> All Public ACMs are posted on the Intel RDC Public-Facing site here(Kit #630744),
>
> https://www.intel.com/content/www/us/en/secure/design/internal/content-details.html?DocID=630744
>
> Since I am signed in and access from Intel Account, this is the internal link, but this download link below should work for outside intel folks as well,
>
> https://cdrdv2.intel.com/v1/dl/getContent/630744?explicitVersion=true
>
> Jeevan Bhungal

Great!  Yeah, the internal link does not work, but the direct link
does.  Thank you, Jeevan.

Regards,
Jason

Re: [tboot-devel] 11th Gen SINIT ACM

[Jason A. <jan...@gm...>]

On Fri, Apr 22, 2022 at 5:41 PM Sehra, Supreeti
<sup...@in...> wrote:
>
> Hi Jason, Pawel,
>
> I am not sure if I have access to update the page.  Jeevan will be back on Monday and he can help with adding ACM's on the page.

Great!  Thank you, Supreeti.

Regards,
Jason

Re: [tboot-devel] 11th Gen SINIT ACM

[Randzio, P. <paw...@in...>]

Hi Jason,

Let me redirect your request to the right people.

@Bhungal, Jeevan S, @Crain, Keegan, 
@Gupta, Rahul Kumar, @Sehra, Supreeti

What's the status of the update for SINIT ACM page Jason mentioned?
(10-12th Gen versions missing)

Kind regards,
Paweł

-----Original Message-----
From: Jason Andryuk <jan...@gm...> 
Sent: Thursday, April 21, 2022 5:22 PM
To: Randzio, Pawel <paw...@in...>
Cc: tbo...@li...
Subject: Re: [tboot-devel] 11th Gen SINIT ACM

On Fri, Mar 25, 2022 at 12:11 PM Randzio, Pawel <paw...@in...> wrote:
>
> Hi Jason,
>
> I forwarded your request to the people responsible with keeping the site up to date.
> I was informed that the contents should be updated soon. I'll keep an eye on that.

Hi, Paweł

Are there any updates on the ACMs?  I don't see them on https://software.intel.com/content/www/us/en/develop/articles/intel-trusted-execution-technology.html.

Thanks,
Jason
---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.

Re: [tboot-devel] 11th Gen SINIT ACM

[Jason A. <jan...@gm...>]

On Fri, Mar 25, 2022 at 12:11 PM Randzio, Pawel <paw...@in...> wrote:
>
> Hi Jason,
>
> I forwarded your request to the people responsible with keeping the site up to date.
> I was informed that the contents should be updated soon. I'll keep an eye on that.

Hi, Paweł

Are there any updates on the ACMs?  I don't see them on
https://software.intel.com/content/www/us/en/develop/articles/intel-trusted-execution-technology.html.

Thanks,
Jason

[tboot-devel] Trenchboot is looking for reviewers

[Randzio, P. <paw...@in...>]

Hi everyone,

Sorry for maybe an off-topic announcement, but on behalf of Daniel P. Smith here I would like to say:

Trenchboot project is looking for reviewers with knowledge of Intel TXT Hardware interfaces. They've been working on enabling Operating System Kernels and Hypervisors to be directly launched by DRTM across all architectures. Their first major upstreaming has been to Linux in the form of Secure Launch capability with the initial implementation supporting Intel's TXT. They've encountered a challenge in reluctance of kernel maintainers to review a series of patches as they do not have knowledge of the TXT interface. That is why they wanted to reach out to you and seek individuals willing to take up the task of reviewing and replying to the latest series posted on LKML.

Daniel is open to discuss this issue here on the mailing list, on Trenchboot's mailing list or through direct contact.

Helpful links:
Trenchboot project - https://trenchboot.org
LKML - https://lkml.org/lkml/2022/2/18/699
Trenchboot mailing list - https://groups.google.com/g/trenchboot-devel
Daniel's mail address - dp...@ap...<mailto:dp...@ap...>

Thanks,
-Paweł
---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.

Re: [tboot-devel] 11th Gen SINIT ACM

[Jason A. <jan...@gm...>]

On Fri, Mar 25, 2022 at 12:11 PM Randzio, Pawel <paw...@in...> wrote:
>
> Hi Jason,
>
> I forwarded your request to the people responsible with keeping the site up to date.
> I was informed that the contents should be updated soon. I'll keep an eye on that.
>
> Paweł

Thanks, Paweł!

-Jason

Re: [tboot-devel] 11th Gen SINIT ACM

[Randzio, P. <paw...@in...>]

Hi Jason,

I forwarded your request to the people responsible with keeping the site up to date.
I was informed that the contents should be updated soon. I'll keep an eye on that.

Paweł

-----Original Message-----
From: Jason Andryuk <jan...@gm...> 
Sent: Wednesday, March 23, 2022 5:58 PM
To: tbo...@li...
Subject: [tboot-devel] 11th Gen SINIT ACM

Hi,

The intel page https://software.intel.com/content/www/us/en/develop/articles/intel-trusted-execution-technology.html
doesn't have anything newer than 9th gen SINIT ACM.

Lukasz previously provided a direct link to 10th gen ones.  Can someone from Intel provide a link to 11th gen SINIT ACMs?  And since 12th gen is right around the corner, maybe those too please?

Thanks,
Jason


_______________________________________________
tboot-devel mailing list
tbo...@li...
https://lists.sourceforge.net/lists/listinfo/tboot-devel
---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.

[tboot-devel] 11th Gen SINIT ACM

[Jason A. <jan...@gm...>]

Hi,

The intel page https://software.intel.com/content/www/us/en/develop/articles/intel-trusted-execution-technology.html
doesn't have anything newer than 9th gen SINIT ACM.

Lukasz previously provided a direct link to 10th gen ones.  Can
someone from Intel provide a link to 11th gen SINIT ACMs?  And since
12th gen is right around the corner, maybe those too please?

Thanks,
Jason

Re: [tboot-devel] status of the grub patch to support multiple SINIT modules?

[Łukasz H. <lu...@ha...>]

On Sat, 2022-03-12 at 09:34 +0200, Timo Lindfors wrote:
> On Fri, 11 Mar 2022, Łukasz Hawryłko wrote:
> > I see that you have quite complex environment for testing tboot, if I
> > find my old GRUB patch and prepare patch for tboot that combined should
> > fix the issue, will you be able to run tests?
> 
> Yes, I'm happy to run tests :)

I am attaching two patches:

GRUB:
multiboot2__Implement_the_new_module_load_and_preferences_tag.patch

tboot:
Use_multiboot2_module_load_preference_tag.patch

On my test platform they fix the issue. Please check how they work on
your environment.

Thanks,
Lukasz

Re: [tboot-devel] suspend problem since kernel 5.15

[Derek D. <z2...@po...>]

I did a git bisect on the Linux kernel and found that the commit below
is the one that breaks tboot+suspend to ram. It is part of a series of
some cpu hotplug commits. I don't know how this would affect tboot
suspend but I paste the patch below hoping that maybe you knowledgeable
devs would get some ideas just by seeing it.

Just to be clear: if I build a kernel from the commit just before this
one I can suspend and resume, but if I build with this commit I can not
suspend, laptop gets stuck on blinking power LED. (Note I did this
bisect using tboot 1.10.5).

------------------------------------------------
commit 453e41085183980087f8a80dada523caf1131c3c (HEAD, refs/bisect/bad)
Author: Vincent Donnefort <vin...@ar...>
Date:   Tue Feb 16 10:35:06 2021 +0000

    cpu/hotplug: Add cpuhp_invoke_callback_range()

    Factorizing and unifying cpuhp callback range invocations,
especially for
    the hotunplug path, where two different ways of decrementing were
used. The
    first one, decrements before the callback is called:

     cpuhp_thread_fun()
         state = st->state;
         st->state--;
         cpuhp_invoke_callback(state);

    The second one, after:

     take_down_cpu()|cpuhp_down_callbacks()
         cpuhp_invoke_callback(st->state);
         st->state--;

    This is problematic for rolling back the steps in case of error, as
    depending on the decrement, the rollback will start from N or N-1.
It also
    makes tracing inconsistent, between steps run in the cpuhp thread and
    the others.

    Additionally, avoid useless cpuhp_thread_fun() loops by skipping empty
    steps.

    Signed-off-by: Vincent Donnefort <vin...@ar...>
    Signed-off-by: Peter Zijlstra (Intel) <pe...@in...>
    Signed-off-by: Ingo Molnar <mi...@ke...>
    Link:
https://lkml.kernel.org/r/202...@ar...
--------------------------------------------------------------

***
*** This is what the problem commit has done:
***

diff --git b/kernel/cpu.c a/kernel/cpu.c
index 680ed8f427c0..23505d6abd45 100644
--- b/kernel/cpu.c
+++ a/kernel/cpu.c
@@ -135,6 +135,11 @@ static struct cpuhp_step *cpuhp_get_step(enum
cpuhp_state state)
 	return cpuhp_hp_states + state;
 }

+static bool cpuhp_step_empty(bool bringup, struct cpuhp_step *step)
+{
+	return bringup ? !step->startup.single : !step->teardown.single;
+}
+
 /**
  * cpuhp_invoke_callback _ Invoke the callbacks for a given state
  * @cpu:	The cpu for which the callback should be invoked
@@ -157,26 +162,24 @@ static int cpuhp_invoke_callback(unsigned int cpu,
enum cpuhp_state state,

 	if (st->fail == state) {
 		st->fail = CPUHP_INVALID;
-
-		if (!(bringup ? step->startup.single : step->teardown.single))
-			return 0;
-
 		return -EAGAIN;
 	}

+	if (cpuhp_step_empty(bringup, step)) {
+		WARN_ON_ONCE(1);
+		return 0;
+	}
+
 	if (!step->multi_instance) {
 		WARN_ON_ONCE(lastp && *lastp);
 		cb = bringup ? step->startup.single : step->teardown.single;
-		if (!cb)
-			return 0;
+
 		trace_cpuhp_enter(cpu, st->target, state, cb);
 		ret = cb(cpu);
 		trace_cpuhp_exit(cpu, st->state, state, ret);
 		return ret;
 	}
 	cbm = bringup ? step->startup.multi : step->teardown.multi;
-	if (!cbm)
-		return 0;

 	/* Single invocation for instance add/remove */
 	if (node) {
@@ -475,6 +478,15 @@ cpuhp_set_state(struct cpuhp_cpu_state *st, enum
cpuhp_state target)
 static inline void
 cpuhp_reset_state(struct cpuhp_cpu_state *st, enum cpuhp_state prev_state)
 {
+	st->target = prev_state;
+
+	/*
+	 * Already rolling back. No need invert the bringup value or to change
+	 * the current state.
+	 */
+	if (st->rollback)
+		return;
+
 	st->rollback = true;

 	/*
@@ -488,7 +500,6 @@ cpuhp_reset_state(struct cpuhp_cpu_state *st, enum
cpuhp_state prev_state)
 			st->state++;
 	}

-	st->target = prev_state;
 	st->bringup = !st->bringup;
 }

@@ -591,10 +602,53 @@ static int finish_cpu(unsigned int cpu)
  * Hotplug state machine related functions
  */

-static void undo_cpu_up(unsigned int cpu, struct cpuhp_cpu_state *st)
+/*
+ * Get the next state to run. Empty ones will be skipped. Returns true if a
+ * state must be run.
+ *
+ * st->state will be modified ahead of time, to match state_to_run, as
if it
+ * has already ran.
+ */
+static bool cpuhp_next_state(bool bringup,
+			     enum cpuhp_state *state_to_run,
+			     struct cpuhp_cpu_state *st,
+			     enum cpuhp_state target)
 {
-	for (st->state--; st->state > st->target; st->state--)
-		cpuhp_invoke_callback(cpu, st->state, false, NULL, NULL);
+	do {
+		if (bringup) {
+			if (st->state >= target)
+				return false;
+
+			*state_to_run = ++st->state;
+		} else {
+			if (st->state <= target)
+				return false;
+
+			*state_to_run = st->state--;
+		}
+
+		if (!cpuhp_step_empty(bringup, cpuhp_get_step(*state_to_run)))
+			break;
+	} while (true);
+
+	return true;
+}
+
+static int cpuhp_invoke_callback_range(bool bringup,
+				       unsigned int cpu,
+				       struct cpuhp_cpu_state *st,
+				       enum cpuhp_state target)
+{
+	enum cpuhp_state state;
+	int err = 0;
+
+	while (cpuhp_next_state(bringup, &state, st, target)) {
+		err = cpuhp_invoke_callback(cpu, state, bringup, NULL, NULL);
+		if (err)
+			break;
+	}
+
+	return err;
 }

 static inline bool can_rollback_cpu(struct cpuhp_cpu_state *st)
@@ -617,16 +671,12 @@ static int cpuhp_up_callbacks(unsigned int cpu,
struct cpuhp_cpu_state *st,
 	enum cpuhp_state prev_state = st->state;
 	int ret = 0;

-	while (st->state < target) {
-		st->state++;
-		ret = cpuhp_invoke_callback(cpu, st->state, true, NULL, NULL);
-		if (ret) {
-			if (can_rollback_cpu(st)) {
-				st->target = prev_state;
-				undo_cpu_up(cpu, st);
-			}
-			break;
-		}
+	ret = cpuhp_invoke_callback_range(true, cpu, st, target);
+	if (ret) {
+		cpuhp_reset_state(st, prev_state);
+		if (can_rollback_cpu(st))
+			WARN_ON(cpuhp_invoke_callback_range(false, cpu, st,
+							    prev_state));
 	}
 	return ret;
 }
@@ -690,17 +740,9 @@ static void cpuhp_thread_fun(unsigned int cpu)
 		state = st->cb_state;
 		st->should_run = false;
 	} else {
-		if (bringup) {
-			st->state++;
-			state = st->state;
-			st->should_run = (st->state < st->target);
-			WARN_ON_ONCE(st->state > st->target);
-		} else {
-			state = st->state;
-			st->state--;
-			st->should_run = (st->state > st->target);
-			WARN_ON_ONCE(st->state < st->target);
-		}
+		st->should_run = cpuhp_next_state(bringup, &state, st, st->target);
+		if (!st->should_run)
+			goto end;
 	}

 	WARN_ON_ONCE(!cpuhp_is_ap_state(state));
@@ -728,6 +770,7 @@ static void cpuhp_thread_fun(unsigned int cpu)
 		st->should_run = false;
 	}

+end:
 	cpuhp_lock_release(bringup);
 	lockdep_release_cpus_lock();

@@ -881,19 +924,18 @@ static int take_cpu_down(void *_param)
 		return err;

 	/*
-	 * We get here while we are in CPUHP_TEARDOWN_CPU state and we must not
-	 * do this step again.
+	 * Must be called from CPUHP_TEARDOWN_CPU, which means, as we are going
+	 * down, that the current state is CPUHP_TEARDOWN_CPU - 1.
 	 */
-	WARN_ON(st->state != CPUHP_TEARDOWN_CPU);
-	st->state--;
+	WARN_ON(st->state != (CPUHP_TEARDOWN_CPU - 1));
+
 	/* Invoke the former CPU_DYING callbacks */
-	for (; st->state > target; st->state--) {
-		ret = cpuhp_invoke_callback(cpu, st->state, false, NULL, NULL);
-		/*
-		 * DYING must not fail!
-		 */
-		WARN_ON_ONCE(ret);
-	}
+	ret = cpuhp_invoke_callback_range(false, cpu, st, target);
+
+	/*
+	 * DYING must not fail!
+	 */
+	WARN_ON_ONCE(ret);

 	/* Give up timekeeping duties */
 	tick_handover_do_timer();
@@ -975,27 +1017,22 @@ void cpuhp_report_idle_dead(void)
 				 cpuhp_complete_idle_dead, st, 0);
 }

-static void undo_cpu_down(unsigned int cpu, struct cpuhp_cpu_state *st)
-{
-	for (st->state++; st->state < st->target; st->state++)
-		cpuhp_invoke_callback(cpu, st->state, true, NULL, NULL);
-}
-
 static int cpuhp_down_callbacks(unsigned int cpu, struct
cpuhp_cpu_state *st,
 				enum cpuhp_state target)
 {
 	enum cpuhp_state prev_state = st->state;
 	int ret = 0;

-	for (; st->state > target; st->state--) {
-		ret = cpuhp_invoke_callback(cpu, st->state, false, NULL, NULL);
-		if (ret) {
-			st->target = prev_state;
-			if (st->state < prev_state)
-				undo_cpu_down(cpu, st);
-			break;
-		}
+	ret = cpuhp_invoke_callback_range(false, cpu, st, target);
+	if (ret) {
+
+		cpuhp_reset_state(st, prev_state);
+
+		if (st->state < prev_state)
+			WARN_ON(cpuhp_invoke_callback_range(true, cpu, st,
+							    prev_state));
 	}
+
 	return ret;
 }

@@ -1168,14 +1205,12 @@ void notify_cpu_starting(unsigned int cpu)

 	rcu_cpu_starting(cpu);	/* Enables RCU usage on this CPU. */
 	cpumask_set_cpu(cpu, &cpus_booted_once_mask);
-	while (st->state < target) {
-		st->state++;
-		ret = cpuhp_invoke_callback(cpu, st->state, true, NULL, NULL);
-		/*
-		 * STARTING must not fail!
-		 */
-		WARN_ON_ONCE(ret);
-	}
+	ret = cpuhp_invoke_callback_range(true, cpu, st, target);
+
+	/*
+	 * STARTING must not fail!
+	 */
+	WARN_ON_ONCE(ret);
 }

 /*
@@ -1781,8 +1816,7 @@ static int cpuhp_issue_call(int cpu, enum
cpuhp_state state, bool bringup,
 	 * If there's nothing to do, we done.
 	 * Relies on the union for multi_instance.
 	 */
-	if ((bringup && !sp->startup.single) ||
-	    (!bringup && !sp->teardown.single))
+	if (cpuhp_step_empty(bringup, sp))
 		return 0;
 	/*
 	 * The non AP bound callbacks can fail on bringup. On teardown