[PATCH 0/2] Fix device removal order for Apple dart iommu

[PATCH 0/2] Fix device removal order for Apple dart iommu

[Janne Grunau]

Starting with v2024.10 dev_iommu_dma_unmap calls during device removal
trigger a NULL pointer dereference since the the iommu device is removed
before its user. The sparsely used DM_FLAG_VITAL flag is intended for
this dependency.
This series adds it to the Apple dart iommu driver and implements the
two phased device removal to the EFI loader.

Signed-off-by: Janne Grunau <j@jannau.net>
---
Janne Grunau (2):
      iommu: apple: Mark device with DM_FLAG_VITAL
      efi_loader: remove non vital devices first

 drivers/iommu/apple_dart.c    | 2 +-
 lib/efi_loader/efi_boottime.c | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)
---
base-commit: 1d147b74f437fb0e85821e8271fe52bc5fd30194
change-id: 20241031-iommu_apple_dart_ordering-558e62671512

Best regards,
-- 
Janne Grunau <j@jannau.net>

[PATCH 1/2] iommu: apple: Mark device with DM_FLAG_VITAL

[Janne Grunau]

Avoids NULL pointer dereferences in apple_dart_unmap when the iommu
device is removed before its user. U-boot's device model does not track
dependencies between devices.
Observed on a M1 Ultra Mac Studio with v2024.10.

Signed-off-by: Janne Grunau <j@jannau.net>
---
 drivers/iommu/apple_dart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/apple_dart.c b/drivers/iommu/apple_dart.c
index 611ac7cd6deb4fccd45ccfe8cbb9b3e36e2af753..9df174f32d721c39079e65736c91e3e27a72ace2 100644
--- a/drivers/iommu/apple_dart.c
+++ b/drivers/iommu/apple_dart.c
@@ -312,5 +312,5 @@ U_BOOT_DRIVER(apple_dart) = {
 	.ops = &apple_dart_ops,
 	.probe = apple_dart_probe,
 	.remove = apple_dart_remove,
-	.flags	= DM_FLAG_OS_PREPARE
+	.flags	= DM_FLAG_OS_PREPARE | DM_FLAG_VITAL
 };

-- 
2.47.0

Re: [PATCH 1/2] iommu: apple: Mark device with DM_FLAG_VITAL

[Heinrich Schuchardt]

On 10/31/24 23:48, Janne Grunau wrote:
> Avoids NULL pointer dereferences in apple_dart_unmap when the iommu
> device is removed before its user. U-boot's device model does not track
> dependencies between devices.
> Observed on a M1 Ultra Mac Studio with v2024.10.
>
> Signed-off-by: Janne Grunau <j@jannau.net>

Hello Simon,

could you, please, review this patch.

Best regards

Heinrich

> ---
>   drivers/iommu/apple_dart.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/apple_dart.c b/drivers/iommu/apple_dart.c
> index 611ac7cd6deb4fccd45ccfe8cbb9b3e36e2af753..9df174f32d721c39079e65736c91e3e27a72ace2 100644
> --- a/drivers/iommu/apple_dart.c
> +++ b/drivers/iommu/apple_dart.c
> @@ -312,5 +312,5 @@ U_BOOT_DRIVER(apple_dart) = {
>   	.ops = &apple_dart_ops,
>   	.probe = apple_dart_probe,
>   	.remove = apple_dart_remove,
> -	.flags	= DM_FLAG_OS_PREPARE
> +	.flags	= DM_FLAG_OS_PREPARE | DM_FLAG_VITAL
>   };
>

Re: [PATCH 1/2] iommu: apple: Mark device with DM_FLAG_VITAL

[Heinrich Schuchardt]

On 10/31/24 23:48, Janne Grunau wrote:
> Avoids NULL pointer dereferences in apple_dart_unmap when the iommu
> device is removed before its user. U-boot's device model does not track
> dependencies between devices.
> Observed on a M1 Ultra Mac Studio with v2024.10.
>
> Signed-off-by: Janne Grunau <j@jannau.net>

Hello Simon,

could you, please, review this patch.

Best regards

Heinrich


> ---
>   drivers/iommu/apple_dart.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/apple_dart.c b/drivers/iommu/apple_dart.c
> index 611ac7cd6deb4fccd45ccfe8cbb9b3e36e2af753..9df174f32d721c39079e65736c91e3e27a72ace2 100644
> --- a/drivers/iommu/apple_dart.c
> +++ b/drivers/iommu/apple_dart.c
> @@ -312,5 +312,5 @@ U_BOOT_DRIVER(apple_dart) = {
>   	.ops = &apple_dart_ops,
>   	.probe = apple_dart_probe,
>   	.remove = apple_dart_remove,
> -	.flags	= DM_FLAG_OS_PREPARE
> +	.flags	= DM_FLAG_OS_PREPARE | DM_FLAG_VITAL
>   };
>

Re: [PATCH 1/2] iommu: apple: Mark device with DM_FLAG_VITAL

[Mark Kettenis]

> From: Janne Grunau <j@jannau.net>
> Date: Thu, 31 Oct 2024 23:48:01 +0100
> 
> Avoids NULL pointer dereferences in apple_dart_unmap when the iommu
> device is removed before its user. U-boot's device model does not track
> dependencies between devices.
> Observed on a M1 Ultra Mac Studio with v2024.10.
> 
> Signed-off-by: Janne Grunau <j@jannau.net>

Acked-by: Mark Kettenis <kettenis@openbsd.org>

> ---
>  drivers/iommu/apple_dart.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/apple_dart.c b/drivers/iommu/apple_dart.c
> index 611ac7cd6deb4fccd45ccfe8cbb9b3e36e2af753..9df174f32d721c39079e65736c91e3e27a72ace2 100644
> --- a/drivers/iommu/apple_dart.c
> +++ b/drivers/iommu/apple_dart.c
> @@ -312,5 +312,5 @@ U_BOOT_DRIVER(apple_dart) = {
>  	.ops = &apple_dart_ops,
>  	.probe = apple_dart_probe,
>  	.remove = apple_dart_remove,
> -	.flags	= DM_FLAG_OS_PREPARE
> +	.flags	= DM_FLAG_OS_PREPARE | DM_FLAG_VITAL
>  };
> 
> -- 
> 2.47.0
> 
> 

[PATCH 2/2] efi_loader: remove non vital devices first

[Janne Grunau]

DM_FLAG_VITAL marks devices which are essential for the operation of
other devices. Removing these devices before their users can result in
hangs or crashes.
This potentially fixes EFI boot of Renesas rcar3 devices. Their clock
devices (and with this series the dart iommu) are the only devices
markes as vital.
The arm boot code already handles devioce removal in this way.

Signed-off-by: Janne Grunau <j@jannau.net>
---
 lib/efi_loader/efi_boottime.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 4f52284b4c653c252b0ed6c0c87da8901448d4b4..7db3c95782970f8c06a970a8ee86b1804cd848b6 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -2234,6 +2234,7 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle,
 		if (IS_ENABLED(CONFIG_USB_DEVICE))
 			udc_disconnect();
 		board_quiesce_devices();
+		dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL | DM_REMOVE_NON_VITAL);
 		dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);
 	}
 

-- 
2.47.0

Re: [PATCH 2/2] efi_loader: remove non vital devices first

[Mark Kettenis]

> From: Janne Grunau <j@jannau.net>
> Date: Thu, 31 Oct 2024 23:48:02 +0100
> 
> DM_FLAG_VITAL marks devices which are essential for the operation of
> other devices. Removing these devices before their users can result in
> hangs or crashes.
> This potentially fixes EFI boot of Renesas rcar3 devices. Their clock
> devices (and with this series the dart iommu) are the only devices
> markes as vital.
> The arm boot code already handles devioce removal in this way.

There is a typo in that last sentence of the commit message (devioce).
Otherwise:

> Signed-off-by: Janne Grunau <j@jannau.net>

Reviewed-by: Mark Kettenis <kettenis@openbsd.org>

> ---
>  lib/efi_loader/efi_boottime.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 4f52284b4c653c252b0ed6c0c87da8901448d4b4..7db3c95782970f8c06a970a8ee86b1804cd848b6 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -2234,6 +2234,7 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle,
>  		if (IS_ENABLED(CONFIG_USB_DEVICE))
>  			udc_disconnect();
>  		board_quiesce_devices();
> +		dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL | DM_REMOVE_NON_VITAL);
>  		dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);
>  	}
>  
> 
> -- 
> 2.47.0
> 
> 

Re: [PATCH 2/2] efi_loader: remove non vital devices first

[Heinrich Schuchardt]

On 11/1/24 21:29, Mark Kettenis wrote:
>> From: Janne Grunau <j@jannau.net>
>> Date: Thu, 31 Oct 2024 23:48:02 +0100
>>
>> DM_FLAG_VITAL marks devices which are essential for the operation of
>> other devices. Removing these devices before their users can result in
>> hangs or crashes.
>> This potentially fixes EFI boot of Renesas rcar3 devices. Their clock
>> devices (and with this series the dart iommu) are the only devices
>> markes as vital.
>> The arm boot code already handles devioce removal in this way.
>
> There is a typo in that last sentence of the commit message (devioce).
> Otherwise:
>
>> Signed-off-by: Janne Grunau <j@jannau.net>
>
> Reviewed-by: Mark Kettenis <kettenis@openbsd.org>
>
>> ---
>>   lib/efi_loader/efi_boottime.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>> index 4f52284b4c653c252b0ed6c0c87da8901448d4b4..7db3c95782970f8c06a970a8ee86b1804cd848b6 100644
>> --- a/lib/efi_loader/efi_boottime.c
>> +++ b/lib/efi_loader/efi_boottime.c
>> @@ -2234,6 +2234,7 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle,
>>   		if (IS_ENABLED(CONFIG_USB_DEVICE))
>>   			udc_disconnect();
>>   		board_quiesce_devices();
>> +		dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL | DM_REMOVE_NON_VITAL);
>>   		dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);

Simon's patch 6224dc9ba428 ("arm: Remove vital devices last") addressed
the same issue for bootm on arm. But what about about other architectures?

This logic should be moved to drivers/core/root.c instead of replicating
code.

Best regards

Heinrich

>>   	}
>>
>>
>> --
>> 2.47.0
>>
>>

Re: [PATCH 2/2] efi_loader: remove non vital devices first

[Simon Glass]

Hi,

On Wed, 13 Nov 2024 at 05:52, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 11/1/24 21:29, Mark Kettenis wrote:
> >> From: Janne Grunau <j@jannau.net>
> >> Date: Thu, 31 Oct 2024 23:48:02 +0100
> >>
> >> DM_FLAG_VITAL marks devices which are essential for the operation of
> >> other devices. Removing these devices before their users can result in
> >> hangs or crashes.
> >> This potentially fixes EFI boot of Renesas rcar3 devices. Their clock
> >> devices (and with this series the dart iommu) are the only devices
> >> markes as vital.
> >> The arm boot code already handles devioce removal in this way.
> >
> > There is a typo in that last sentence of the commit message (devioce).
> > Otherwise:
> >
> >> Signed-off-by: Janne Grunau <j@jannau.net>
> >
> > Reviewed-by: Mark Kettenis <kettenis@openbsd.org>
> >
> >> ---
> >>   lib/efi_loader/efi_boottime.c | 1 +
> >>   1 file changed, 1 insertion(+)
> >>
> >> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> >> index 4f52284b4c653c252b0ed6c0c87da8901448d4b4..7db3c95782970f8c06a970a8ee86b1804cd848b6 100644
> >> --- a/lib/efi_loader/efi_boottime.c
> >> +++ b/lib/efi_loader/efi_boottime.c
> >> @@ -2234,6 +2234,7 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle,
> >>              if (IS_ENABLED(CONFIG_USB_DEVICE))
> >>                      udc_disconnect();
> >>              board_quiesce_devices();
> >> +            dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL | DM_REMOVE_NON_VITAL);
> >>              dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);
>
> Simon's patch 6224dc9ba428 ("arm: Remove vital devices last") addressed
> the same issue for bootm on arm. But what about about other architectures?
>
> This logic should be moved to drivers/core/root.c instead of replicating
> code.

We could have a common helper, but it should not be in driver/core as
this ordering is more of a policy decision. Unless we can add a
parameter telling dm exactly what to do...

BTW, Heinrich, this behaviour is exactly what my bootflow_efi() test
was supposed to check. But since it doesn't have the
exit-boot-services piece at your request...

Regards,
Simon

Re: [PATCH 2/2] efi_loader: remove non vital devices first

[Heinrich Schuchardt]

Am 13. November 2024 15:39:22 MEZ schrieb Simon Glass <sjg@chromium.org>:
>Hi,
>
>On Wed, 13 Nov 2024 at 05:52, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 11/1/24 21:29, Mark Kettenis wrote:
>> >> From: Janne Grunau <j@jannau.net>
>> >> Date: Thu, 31 Oct 2024 23:48:02 +0100
>> >>
>> >> DM_FLAG_VITAL marks devices which are essential for the operation of
>> >> other devices. Removing these devices before their users can result in
>> >> hangs or crashes.
>> >> This potentially fixes EFI boot of Renesas rcar3 devices. Their clock
>> >> devices (and with this series the dart iommu) are the only devices
>> >> markes as vital.
>> >> The arm boot code already handles devioce removal in this way.
>> >
>> > There is a typo in that last sentence of the commit message (devioce).
>> > Otherwise:
>> >
>> >> Signed-off-by: Janne Grunau <j@jannau.net>
>> >
>> > Reviewed-by: Mark Kettenis <kettenis@openbsd.org>
>> >
>> >> ---
>> >>   lib/efi_loader/efi_boottime.c | 1 +
>> >>   1 file changed, 1 insertion(+)
>> >>
>> >> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>> >> index 4f52284b4c653c252b0ed6c0c87da8901448d4b4..7db3c95782970f8c06a970a8ee86b1804cd848b6 100644
>> >> --- a/lib/efi_loader/efi_boottime.c
>> >> +++ b/lib/efi_loader/efi_boottime.c
>> >> @@ -2234,6 +2234,7 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle,
>> >>              if (IS_ENABLED(CONFIG_USB_DEVICE))
>> >>                      udc_disconnect();
>> >>              board_quiesce_devices();
>> >> +            dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL | DM_REMOVE_NON_VITAL);
>> >>              dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);
>>
>> Simon's patch 6224dc9ba428 ("arm: Remove vital devices last") addressed
>> the same issue for bootm on arm. But what about about other architectures?
>>
>> This logic should be moved to drivers/core/root.c instead of replicating
>> code.
>
>We could have a common helper, but it should not be in driver/core as
>this ordering is more of a policy decision. Unless we can add a
>parameter telling dm exactly what to do...
>
>BTW, Heinrich, this behaviour is exactly what my bootflow_efi() test
>was supposed to check. But since it doesn't have the
>exit-boot-services piece at your request...
>
>Regards,
>Simon


Why can't we generally remove non-vital devices first if all are to be removed?

I cannot see anything device specific here.

Best regards

Heinrich

Re: [PATCH 2/2] efi_loader: remove non vital devices first

[Simon Glass]

Hi Heinrich,

On Wed, 13 Nov 2024 at 08:17, Heinrich Schuchardt <xypron.glpk@gmx.de>
wrote:
>
> Am 13. November 2024 15:39:22 MEZ schrieb Simon Glass <sjg@chromium.org>:
> >Hi,
> >
> >On Wed, 13 Nov 2024 at 05:52, Heinrich Schuchardt <xypron.glpk@gmx.de>
wrote:
> >>
> >> On 11/1/24 21:29, Mark Kettenis wrote:
> >> >> From: Janne Grunau <j@jannau.net>
> >> >> Date: Thu, 31 Oct 2024 23:48:02 +0100
> >> >>
> >> >> DM_FLAG_VITAL marks devices which are essential for the operation of
> >> >> other devices. Removing these devices before their users can result
in
> >> >> hangs or crashes.
> >> >> This potentially fixes EFI boot of Renesas rcar3 devices. Their
clock
> >> >> devices (and with this series the dart iommu) are the only devices
> >> >> markes as vital.
> >> >> The arm boot code already handles devioce removal in this way.
> >> >
> >> > There is a typo in that last sentence of the commit message
(devioce).
> >> > Otherwise:
> >> >
> >> >> Signed-off-by: Janne Grunau <j@jannau.net>
> >> >
> >> > Reviewed-by: Mark Kettenis <kettenis@openbsd.org>
> >> >
> >> >> ---
> >> >>   lib/efi_loader/efi_boottime.c | 1 +
> >> >>   1 file changed, 1 insertion(+)
> >> >>
> >> >> diff --git a/lib/efi_loader/efi_boottime.c
b/lib/efi_loader/efi_boottime.c
> >> >> index
4f52284b4c653c252b0ed6c0c87da8901448d4b4..7db3c95782970f8c06a970a8ee86b1804cd848b6
100644
> >> >> --- a/lib/efi_loader/efi_boottime.c
> >> >> +++ b/lib/efi_loader/efi_boottime.c
> >> >> @@ -2234,6 +2234,7 @@ static efi_status_t EFIAPI
efi_exit_boot_services(efi_handle_t image_handle,
> >> >>              if (IS_ENABLED(CONFIG_USB_DEVICE))
> >> >>                      udc_disconnect();
> >> >>              board_quiesce_devices();
> >> >> +            dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL |
DM_REMOVE_NON_VITAL);
> >> >>              dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);
> >>
> >> Simon's patch 6224dc9ba428 ("arm: Remove vital devices last") addressed
> >> the same issue for bootm on arm. But what about about other
architectures?
> >>
> >> This logic should be moved to drivers/core/root.c instead of
replicating
> >> code.
> >
> >We could have a common helper, but it should not be in driver/core as
> >this ordering is more of a policy decision. Unless we can add a
> >parameter telling dm exactly what to do...
> >
> >BTW, Heinrich, this behaviour is exactly what my bootflow_efi() test
> >was supposed to check. But since it doesn't have the
> >exit-boot-services piece at your request...
> >
> >Regards,
> >Simon
>
>
> Why can't we generally remove non-vital devices first if all are to be
removed?
>
> I cannot see anything device specific here.

No objection to that...but it needs to be in a new function, not become the
default behaviour of dm_remove_devices_flags(), which is supposed to do
what it is told and in one pass.

Regards,
Simon

Re: [PATCH 2/2] efi_loader: remove non vital devices first

[Heinrich Schuchardt]

Am 13. November 2024 17:03:03 MEZ schrieb Simon Glass <sjg@chromium.org>:
>Hi Heinrich,
>
>On Wed, 13 Nov 2024 at 08:17, Heinrich Schuchardt <xypron.glpk@gmx.de>
>wrote:
>>
>> Am 13. November 2024 15:39:22 MEZ schrieb Simon Glass <sjg@chromium.org>:
>> >Hi,
>> >
>> >On Wed, 13 Nov 2024 at 05:52, Heinrich Schuchardt <xypron.glpk@gmx.de>
>wrote:
>> >>
>> >> On 11/1/24 21:29, Mark Kettenis wrote:
>> >> >> From: Janne Grunau <j@jannau.net>
>> >> >> Date: Thu, 31 Oct 2024 23:48:02 +0100
>> >> >>
>> >> >> DM_FLAG_VITAL marks devices which are essential for the operation of
>> >> >> other devices. Removing these devices before their users can result
>in
>> >> >> hangs or crashes.
>> >> >> This potentially fixes EFI boot of Renesas rcar3 devices. Their
>clock
>> >> >> devices (and with this series the dart iommu) are the only devices
>> >> >> markes as vital.
>> >> >> The arm boot code already handles devioce removal in this way.
>> >> >
>> >> > There is a typo in that last sentence of the commit message
>(devioce).
>> >> > Otherwise:
>> >> >
>> >> >> Signed-off-by: Janne Grunau <j@jannau.net>
>> >> >
>> >> > Reviewed-by: Mark Kettenis <kettenis@openbsd.org>
>> >> >
>> >> >> ---
>> >> >>   lib/efi_loader/efi_boottime.c | 1 +
>> >> >>   1 file changed, 1 insertion(+)
>> >> >>
>> >> >> diff --git a/lib/efi_loader/efi_boottime.c
>b/lib/efi_loader/efi_boottime.c
>> >> >> index
>4f52284b4c653c252b0ed6c0c87da8901448d4b4..7db3c95782970f8c06a970a8ee86b1804cd848b6
>100644
>> >> >> --- a/lib/efi_loader/efi_boottime.c
>> >> >> +++ b/lib/efi_loader/efi_boottime.c
>> >> >> @@ -2234,6 +2234,7 @@ static efi_status_t EFIAPI
>efi_exit_boot_services(efi_handle_t image_handle,
>> >> >>              if (IS_ENABLED(CONFIG_USB_DEVICE))
>> >> >>                      udc_disconnect();
>> >> >>              board_quiesce_devices();
>> >> >> +            dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL |
>DM_REMOVE_NON_VITAL);
>> >> >>              dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);
>> >>
>> >> Simon's patch 6224dc9ba428 ("arm: Remove vital devices last") addressed
>> >> the same issue for bootm on arm. But what about about other
>architectures?
>> >>
>> >> This logic should be moved to drivers/core/root.c instead of
>replicating
>> >> code.
>> >
>> >We could have a common helper, but it should not be in driver/core as
>> >this ordering is more of a policy decision. Unless we can add a
>> >parameter telling dm exactly what to do...
>> >
>> >BTW, Heinrich, this behaviour is exactly what my bootflow_efi() test
>> >was supposed to check. But since it doesn't have the
>> >exit-boot-services piece at your request...
>> >
>> >Regards,
>> >Simon
>>
>>
>> Why can't we generally remove non-vital devices first if all are to be
>removed?
>>
>> I cannot see anything device specific here.
>
>No objection to that...but it needs to be in a new function, not become the
>default behaviour of dm_remove_devices_flags(), which is supposed to do
>what it is told and in one pass.

dm_remove_device_flags() makes no specific promises concerning the deletion sequence and its internal working. But obviously it will fail if non-vital redources are not deleted first.

Leaving this broken function exported and writing a new one has no user benefit.

We could copy the old function to a new static function that we call as needed from the old function name.

Best regards

Heinruch

>
>Regards,
>Simon

Re: [PATCH 2/2] efi_loader: remove non vital devices first

[Simon Glass]

Hi Heinrich,

On Wed, 13 Nov 2024 at 11:45, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Am 13. November 2024 17:03:03 MEZ schrieb Simon Glass <sjg@chromium.org>:
> >Hi Heinrich,
> >
> >On Wed, 13 Nov 2024 at 08:17, Heinrich Schuchardt <xypron.glpk@gmx.de>
> >wrote:
> >>
> >> Am 13. November 2024 15:39:22 MEZ schrieb Simon Glass <sjg@chromium.org>:
> >> >Hi,
> >> >
> >> >On Wed, 13 Nov 2024 at 05:52, Heinrich Schuchardt <xypron.glpk@gmx.de>
> >wrote:
> >> >>
> >> >> On 11/1/24 21:29, Mark Kettenis wrote:
> >> >> >> From: Janne Grunau <j@jannau.net>
> >> >> >> Date: Thu, 31 Oct 2024 23:48:02 +0100
> >> >> >>
> >> >> >> DM_FLAG_VITAL marks devices which are essential for the operation of
> >> >> >> other devices. Removing these devices before their users can result
> >in
> >> >> >> hangs or crashes.
> >> >> >> This potentially fixes EFI boot of Renesas rcar3 devices. Their
> >clock
> >> >> >> devices (and with this series the dart iommu) are the only devices
> >> >> >> markes as vital.
> >> >> >> The arm boot code already handles devioce removal in this way.
> >> >> >
> >> >> > There is a typo in that last sentence of the commit message
> >(devioce).
> >> >> > Otherwise:
> >> >> >
> >> >> >> Signed-off-by: Janne Grunau <j@jannau.net>
> >> >> >
> >> >> > Reviewed-by: Mark Kettenis <kettenis@openbsd.org>
> >> >> >
> >> >> >> ---
> >> >> >>   lib/efi_loader/efi_boottime.c | 1 +
> >> >> >>   1 file changed, 1 insertion(+)
> >> >> >>
> >> >> >> diff --git a/lib/efi_loader/efi_boottime.c
> >b/lib/efi_loader/efi_boottime.c
> >> >> >> index
> >4f52284b4c653c252b0ed6c0c87da8901448d4b4..7db3c95782970f8c06a970a8ee86b1804cd848b6
> >100644
> >> >> >> --- a/lib/efi_loader/efi_boottime.c
> >> >> >> +++ b/lib/efi_loader/efi_boottime.c
> >> >> >> @@ -2234,6 +2234,7 @@ static efi_status_t EFIAPI
> >efi_exit_boot_services(efi_handle_t image_handle,
> >> >> >>              if (IS_ENABLED(CONFIG_USB_DEVICE))
> >> >> >>                      udc_disconnect();
> >> >> >>              board_quiesce_devices();
> >> >> >> +            dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL |
> >DM_REMOVE_NON_VITAL);
> >> >> >>              dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);
> >> >>
> >> >> Simon's patch 6224dc9ba428 ("arm: Remove vital devices last") addressed
> >> >> the same issue for bootm on arm. But what about about other
> >architectures?
> >> >>
> >> >> This logic should be moved to drivers/core/root.c instead of
> >replicating
> >> >> code.
> >> >
> >> >We could have a common helper, but it should not be in driver/core as
> >> >this ordering is more of a policy decision. Unless we can add a
> >> >parameter telling dm exactly what to do...
> >> >
> >> >BTW, Heinrich, this behaviour is exactly what my bootflow_efi() test
> >> >was supposed to check. But since it doesn't have the
> >> >exit-boot-services piece at your request...
> >> >
> >> >Regards,
> >> >Simon
> >>
> >>
> >> Why can't we generally remove non-vital devices first if all are to be
> >removed?
> >>
> >> I cannot see anything device specific here.
> >
> >No objection to that...but it needs to be in a new function, not become the
> >default behaviour of dm_remove_devices_flags(), which is supposed to do
> >what it is told and in one pass.
>
> dm_remove_device_flags() makes no specific promises concerning the deletion sequence and its internal working.

Do you mean in the function comment in the header file? We can
certainly update that.

> But obviously it will fail if non-vital redources are not deleted first.

Either I don't understand that, or I don't agree. Can you give a
specific example?

>
> Leaving this broken function exported and writing a new one has no user benefit.

This function works fine. It is not broken.

>
> We could copy the old function to a new static function that we call as needed from the old function name.

Yes, that's fine with me. Also, did you see my note about the
bootflow_efi() test?

Regards,
Simon

Re: [PATCH 2/2] efi_loader: remove non vital devices first

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4700 bytes --]

On Wed, Nov 13, 2024 at 08:53:17PM -0700, Simon Glass wrote:
> Hi Heinrich,
> 
> On Wed, 13 Nov 2024 at 11:45, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > Am 13. November 2024 17:03:03 MEZ schrieb Simon Glass <sjg@chromium.org>:
> > >Hi Heinrich,
> > >
> > >On Wed, 13 Nov 2024 at 08:17, Heinrich Schuchardt <xypron.glpk@gmx.de>
> > >wrote:
> > >>
> > >> Am 13. November 2024 15:39:22 MEZ schrieb Simon Glass <sjg@chromium.org>:
> > >> >Hi,
> > >> >
> > >> >On Wed, 13 Nov 2024 at 05:52, Heinrich Schuchardt <xypron.glpk@gmx.de>
> > >wrote:
> > >> >>
> > >> >> On 11/1/24 21:29, Mark Kettenis wrote:
> > >> >> >> From: Janne Grunau <j@jannau.net>
> > >> >> >> Date: Thu, 31 Oct 2024 23:48:02 +0100
> > >> >> >>
> > >> >> >> DM_FLAG_VITAL marks devices which are essential for the operation of
> > >> >> >> other devices. Removing these devices before their users can result
> > >in
> > >> >> >> hangs or crashes.
> > >> >> >> This potentially fixes EFI boot of Renesas rcar3 devices. Their
> > >clock
> > >> >> >> devices (and with this series the dart iommu) are the only devices
> > >> >> >> markes as vital.
> > >> >> >> The arm boot code already handles devioce removal in this way.
> > >> >> >
> > >> >> > There is a typo in that last sentence of the commit message
> > >(devioce).
> > >> >> > Otherwise:
> > >> >> >
> > >> >> >> Signed-off-by: Janne Grunau <j@jannau.net>
> > >> >> >
> > >> >> > Reviewed-by: Mark Kettenis <kettenis@openbsd.org>
> > >> >> >
> > >> >> >> ---
> > >> >> >>   lib/efi_loader/efi_boottime.c | 1 +
> > >> >> >>   1 file changed, 1 insertion(+)
> > >> >> >>
> > >> >> >> diff --git a/lib/efi_loader/efi_boottime.c
> > >b/lib/efi_loader/efi_boottime.c
> > >> >> >> index
> > >4f52284b4c653c252b0ed6c0c87da8901448d4b4..7db3c95782970f8c06a970a8ee86b1804cd848b6
> > >100644
> > >> >> >> --- a/lib/efi_loader/efi_boottime.c
> > >> >> >> +++ b/lib/efi_loader/efi_boottime.c
> > >> >> >> @@ -2234,6 +2234,7 @@ static efi_status_t EFIAPI
> > >efi_exit_boot_services(efi_handle_t image_handle,
> > >> >> >>              if (IS_ENABLED(CONFIG_USB_DEVICE))
> > >> >> >>                      udc_disconnect();
> > >> >> >>              board_quiesce_devices();
> > >> >> >> +            dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL |
> > >DM_REMOVE_NON_VITAL);
> > >> >> >>              dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);
> > >> >>
> > >> >> Simon's patch 6224dc9ba428 ("arm: Remove vital devices last") addressed
> > >> >> the same issue for bootm on arm. But what about about other
> > >architectures?
> > >> >>
> > >> >> This logic should be moved to drivers/core/root.c instead of
> > >replicating
> > >> >> code.
> > >> >
> > >> >We could have a common helper, but it should not be in driver/core as
> > >> >this ordering is more of a policy decision. Unless we can add a
> > >> >parameter telling dm exactly what to do...
> > >> >
> > >> >BTW, Heinrich, this behaviour is exactly what my bootflow_efi() test
> > >> >was supposed to check. But since it doesn't have the
> > >> >exit-boot-services piece at your request...
> > >> >
> > >> >Regards,
> > >> >Simon
> > >>
> > >>
> > >> Why can't we generally remove non-vital devices first if all are to be
> > >removed?
> > >>
> > >> I cannot see anything device specific here.
> > >
> > >No objection to that...but it needs to be in a new function, not become the
> > >default behaviour of dm_remove_devices_flags(), which is supposed to do
> > >what it is told and in one pass.
> >
> > dm_remove_device_flags() makes no specific promises concerning the deletion sequence and its internal working.
> 
> Do you mean in the function comment in the header file? We can
> certainly update that.

That seems like "fixing" the problem by documenting that things should
work in the seemingly broken way.

> > But obviously it will fail if non-vital redources are not deleted first.
> 
> Either I don't understand that, or I don't agree. Can you give a
> specific example?

The Apple IOMMU? That's what got this thread started.

> > Leaving this broken function exported and writing a new one has no user benefit.
> 
> This function works fine. It is not broken.
> 
> >
> > We could copy the old function to a new static function that we call as needed from the old function name.
> 
> Yes, that's fine with me. Also, did you see my note about the
> bootflow_efi() test?

But since your test passed (I assume) and real platforms are failing and
requiring a fix, I'm not sure this is a strong argument? Or did you have
a patch in a later series that fixed the problem here as well?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/2] efi_loader: remove non vital devices first

[Simon Glass]

Hi Tom,

On Thu, 14 Nov 2024 at 07:26, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Nov 13, 2024 at 08:53:17PM -0700, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Wed, 13 Nov 2024 at 11:45, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >
> > > Am 13. November 2024 17:03:03 MEZ schrieb Simon Glass <sjg@chromium.org>:
> > > >Hi Heinrich,
> > > >
> > > >On Wed, 13 Nov 2024 at 08:17, Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > >wrote:
> > > >>
> > > >> Am 13. November 2024 15:39:22 MEZ schrieb Simon Glass <sjg@chromium.org>:
> > > >> >Hi,
> > > >> >
> > > >> >On Wed, 13 Nov 2024 at 05:52, Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > >wrote:
> > > >> >>
> > > >> >> On 11/1/24 21:29, Mark Kettenis wrote:
> > > >> >> >> From: Janne Grunau <j@jannau.net>
> > > >> >> >> Date: Thu, 31 Oct 2024 23:48:02 +0100
> > > >> >> >>
> > > >> >> >> DM_FLAG_VITAL marks devices which are essential for the operation of
> > > >> >> >> other devices. Removing these devices before their users can result
> > > >in
> > > >> >> >> hangs or crashes.
> > > >> >> >> This potentially fixes EFI boot of Renesas rcar3 devices. Their
> > > >clock
> > > >> >> >> devices (and with this series the dart iommu) are the only devices
> > > >> >> >> markes as vital.
> > > >> >> >> The arm boot code already handles devioce removal in this way.
> > > >> >> >
> > > >> >> > There is a typo in that last sentence of the commit message
> > > >(devioce).
> > > >> >> > Otherwise:
> > > >> >> >
> > > >> >> >> Signed-off-by: Janne Grunau <j@jannau.net>
> > > >> >> >
> > > >> >> > Reviewed-by: Mark Kettenis <kettenis@openbsd.org>
> > > >> >> >
> > > >> >> >> ---
> > > >> >> >>   lib/efi_loader/efi_boottime.c | 1 +
> > > >> >> >>   1 file changed, 1 insertion(+)
> > > >> >> >>
> > > >> >> >> diff --git a/lib/efi_loader/efi_boottime.c
> > > >b/lib/efi_loader/efi_boottime.c
> > > >> >> >> index
> > > >4f52284b4c653c252b0ed6c0c87da8901448d4b4..7db3c95782970f8c06a970a8ee86b1804cd848b6
> > > >100644
> > > >> >> >> --- a/lib/efi_loader/efi_boottime.c
> > > >> >> >> +++ b/lib/efi_loader/efi_boottime.c
> > > >> >> >> @@ -2234,6 +2234,7 @@ static efi_status_t EFIAPI
> > > >efi_exit_boot_services(efi_handle_t image_handle,
> > > >> >> >>              if (IS_ENABLED(CONFIG_USB_DEVICE))
> > > >> >> >>                      udc_disconnect();
> > > >> >> >>              board_quiesce_devices();
> > > >> >> >> +            dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL |
> > > >DM_REMOVE_NON_VITAL);
> > > >> >> >>              dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);
> > > >> >>
> > > >> >> Simon's patch 6224dc9ba428 ("arm: Remove vital devices last") addressed
> > > >> >> the same issue for bootm on arm. But what about about other
> > > >architectures?
> > > >> >>
> > > >> >> This logic should be moved to drivers/core/root.c instead of
> > > >replicating
> > > >> >> code.
> > > >> >
> > > >> >We could have a common helper, but it should not be in driver/core as
> > > >> >this ordering is more of a policy decision. Unless we can add a
> > > >> >parameter telling dm exactly what to do...
> > > >> >
> > > >> >BTW, Heinrich, this behaviour is exactly what my bootflow_efi() test
> > > >> >was supposed to check. But since it doesn't have the
> > > >> >exit-boot-services piece at your request...
> > > >> >
> > > >> >Regards,
> > > >> >Simon
> > > >>
> > > >>
> > > >> Why can't we generally remove non-vital devices first if all are to be
> > > >removed?
> > > >>
> > > >> I cannot see anything device specific here.
> > > >
> > > >No objection to that...but it needs to be in a new function, not become the
> > > >default behaviour of dm_remove_devices_flags(), which is supposed to do
> > > >what it is told and in one pass.
> > >
> > > dm_remove_device_flags() makes no specific promises concerning the deletion sequence and its internal working.
> >
> > Do you mean in the function comment in the header file? We can
> > certainly update that.
>
> That seems like "fixing" the problem by documenting that things should
> work in the seemingly broken way.
>
> > > But obviously it will fail if non-vital redources are not deleted first.
> >
> > Either I don't understand that, or I don't agree. Can you give a
> > specific example?
>
> The Apple IOMMU? That's what got this thread started.
>
> > > Leaving this broken function exported and writing a new one has no user benefit.
> >
> > This function works fine. It is not broken.
> >
> > >
> > > We could copy the old function to a new static function that we call as needed from the old function name.
> >
> > Yes, that's fine with me. Also, did you see my note about the
> > bootflow_efi() test?
>
> But since your test passed (I assume) and real platforms are failing and
> requiring a fix, I'm not sure this is a strong argument? Or did you have
> a patch in a later series that fixed the problem here as well?

My comments were to Heinrich, not this patch. This patch is fine.

Heinrich, if you want to clean this up, please create a new function
which does these two steps separately.

Reviewed-by: Simon Glass <sjg@chromium.org>

Regards,
Simon

Re: [PATCH 2/2] efi_loader: remove non vital devices first

[Mark Kettenis]

> From: Simon Glass <sjg@chromium.org>
> Date: Wed, 13 Nov 2024 07:39:22 -0700
> 
> Hi,

Hi Simon,

> On Wed, 13 Nov 2024 at 05:52, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > On 11/1/24 21:29, Mark Kettenis wrote:
> > >> From: Janne Grunau <j@jannau.net>
> > >> Date: Thu, 31 Oct 2024 23:48:02 +0100
> > >>
> > >> DM_FLAG_VITAL marks devices which are essential for the operation of
> > >> other devices. Removing these devices before their users can result in
> > >> hangs or crashes.
> > >> This potentially fixes EFI boot of Renesas rcar3 devices. Their clock
> > >> devices (and with this series the dart iommu) are the only devices
> > >> markes as vital.
> > >> The arm boot code already handles devioce removal in this way.
> > >
> > > There is a typo in that last sentence of the commit message (devioce).
> > > Otherwise:
> > >
> > >> Signed-off-by: Janne Grunau <j@jannau.net>
> > >
> > > Reviewed-by: Mark Kettenis <kettenis@openbsd.org>
> > >
> > >> ---
> > >>   lib/efi_loader/efi_boottime.c | 1 +
> > >>   1 file changed, 1 insertion(+)
> > >>
> > >> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> > >> index 4f52284b4c653c252b0ed6c0c87da8901448d4b4..7db3c95782970f8c06a970a8ee86b1804cd848b6 100644
> > >> --- a/lib/efi_loader/efi_boottime.c
> > >> +++ b/lib/efi_loader/efi_boottime.c
> > >> @@ -2234,6 +2234,7 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle,
> > >>              if (IS_ENABLED(CONFIG_USB_DEVICE))
> > >>                      udc_disconnect();
> > >>              board_quiesce_devices();
> > >> +            dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL | DM_REMOVE_NON_VITAL);
> > >>              dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);
> >
> > Simon's patch 6224dc9ba428 ("arm: Remove vital devices last") addressed
> > the same issue for bootm on arm. But what about about other architectures?
> >
> > This logic should be moved to drivers/core/root.c instead of replicating
> > code.
> 
> We could have a common helper, but it should not be in driver/core as
> this ordering is more of a policy decision. Unless we can add a
> parameter telling dm exactly what to do...

But I don't think it makes sense for this to be a per-architecture
policy (like it is now).

Also, not that outside of the testsuite, we currently either do:

  dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL | DM_REMOVE_NON_VITAL);
  dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);

or just:

  dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);

and I'd argue that all instances of the latter should be converted
into the latter.  So we really would only have a single policy here...

Re: [PATCH 0/2] Fix device removal order for Apple dart iommu

[Sughosh Ganu]

On Fri, 1 Nov 2024 at 04:37, Janne Grunau <j@jannau.net> wrote:
>
> Starting with v2024.10 dev_iommu_dma_unmap calls during device removal
> trigger a NULL pointer dereference since the the iommu device is removed
> before its user. The sparsely used DM_FLAG_VITAL flag is intended for
> this dependency.
> This series adds it to the Apple dart iommu driver and implements the
> two phased device removal to the EFI loader.

Is this also the cause of the crash that you were observing with the
RFC patches that I had posted earlier?

-sughosh

>
> Signed-off-by: Janne Grunau <j@jannau.net>
> ---
> Janne Grunau (2):
>       iommu: apple: Mark device with DM_FLAG_VITAL
>       efi_loader: remove non vital devices first
>
>  drivers/iommu/apple_dart.c    | 2 +-
>  lib/efi_loader/efi_boottime.c | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> ---
> base-commit: 1d147b74f437fb0e85821e8271fe52bc5fd30194
> change-id: 20241031-iommu_apple_dart_ordering-558e62671512
>
> Best regards,
> --
> Janne Grunau <j@jannau.net>
>

Re: [PATCH 0/2] Fix device removal order for Apple dart iommu

[Janne Grunau]

On Fri, Nov 01, 2024 at 11:52:26AM +0530, Sughosh Ganu wrote:
> On Fri, 1 Nov 2024 at 04:37, Janne Grunau <j@jannau.net> wrote:
> >
> > Starting with v2024.10 dev_iommu_dma_unmap calls during device removal
> > trigger a NULL pointer dereference since the the iommu device is removed
> > before its user. The sparsely used DM_FLAG_VITAL flag is intended for
> > this dependency.
> > This series adds it to the Apple dart iommu driver and implements the
> > two phased device removal to the EFI loader.
> 
> Is this also the cause of the crash that you were observing with the
> RFC patches that I had posted earlier?

It shouldn't as I applied those patches on top of this. This is a
separate issue already present in v2024.10. I discovered the LMB changes
while investigating this.

Janne

Re: [PATCH 0/2] Fix device removal order for Apple dart iommu

[Janne Grunau]

On Thu, Oct 31, 2024 at 11:48:00PM +0100, Janne Grunau wrote:
> Starting with v2024.10 dev_iommu_dma_unmap calls during device removal
> trigger a NULL pointer dereference since the the iommu device is removed
> before its user. The sparsely used DM_FLAG_VITAL flag is intended for
> this dependency.
> This series adds it to the Apple dart iommu driver and implements the
> two phased device removal to the EFI loader.

Can we get this two small patches merged? They fix a regression although
it worked previously just accidentally. The only drivers using
DM_FLAG_VITAL are clk-rcar-gen3.c, rzg2l-cpg.c and apple_dart.c all used
on arm SoCs. arch/arm/lib/bootm.c already uses a device_remove call with
DM_REMOVE_NON_VITAL so adding it to efi_loader looks reasonable as
regression fix.

Janne

Re: [PATCH 0/2] Fix device removal order for Apple dart iommu

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 993 bytes --]

On Thu, Nov 21, 2024 at 08:39:26AM +0100, Janne Grunau wrote:
> On Thu, Oct 31, 2024 at 11:48:00PM +0100, Janne Grunau wrote:
> > Starting with v2024.10 dev_iommu_dma_unmap calls during device removal
> > trigger a NULL pointer dereference since the the iommu device is removed
> > before its user. The sparsely used DM_FLAG_VITAL flag is intended for
> > this dependency.
> > This series adds it to the Apple dart iommu driver and implements the
> > two phased device removal to the EFI loader.
> 
> Can we get this two small patches merged? They fix a regression although
> it worked previously just accidentally. The only drivers using
> DM_FLAG_VITAL are clk-rcar-gen3.c, rzg2l-cpg.c and apple_dart.c all used
> on arm SoCs. arch/arm/lib/bootm.c already uses a device_remove call with
> DM_REMOVE_NON_VITAL so adding it to efi_loader looks reasonable as
> regression fix.

Can you please take a look at doing the changes Heinrich suggested I
believe in 2/2 ?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]