[edk2-devel] [PATCH 1/1] EmbeddedPkg/NonCoherentIoMmuDxe: Make SetAttributes always succeed

[edk2-devel] [PATCH 1/1] EmbeddedPkg/NonCoherentIoMmuDxe: Make SetAttributes always succeed

[Ard Biesheuvel via groups.io]

From: Ard Biesheuvel <ardb@kernel.org>

NonCoherentIoMmuSetAttribute() does nothing except return
EFI_UNSUPPORTED. This was fine when it was introduced, but now, the PCI
bus driver will fail a PCI I/O Map() operation if the SetAttributes
fails.

So return EFI_SUCCESS instead.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
This fixes a regression on Raspberry Pi4, which no longer boots.

 EmbeddedPkg/Drivers/NonCoherentIoMmuDxe/NonCoherentIoMmuDxe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/EmbeddedPkg/Drivers/NonCoherentIoMmuDxe/NonCoherentIoMmuDxe.c b/EmbeddedPkg/Drivers/NonCoherentIoMmuDxe/NonCoherentIoMmuDxe.c
index 4e7a5698c162..f02a76a62ea8 100644
--- a/EmbeddedPkg/Drivers/NonCoherentIoMmuDxe/NonCoherentIoMmuDxe.c
+++ b/EmbeddedPkg/Drivers/NonCoherentIoMmuDxe/NonCoherentIoMmuDxe.c
@@ -70,7 +70,7 @@ NonCoherentIoMmuSetAttribute (
   IN UINT64                IoMmuAccess
   )
 {
-  return EFI_UNSUPPORTED;
+  return EFI_SUCCESS;
 }
 
 /**
-- 
2.44.0.278.ge034bb2e1d-goog



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116688): https://edk2.groups.io/g/devel/message/116688
Mute This Topic: https://groups.io/mt/104886877/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] EmbeddedPkg/NonCoherentIoMmuDxe: Make SetAttributes always succeed

[Leif Lindholm]

On 2024-03-12 08:17, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ardb@kernel.org>
> 
> NonCoherentIoMmuSetAttribute() does nothing except return
> EFI_UNSUPPORTED. This was fine when it was introduced, but now, the PCI
> bus driver will fail a PCI I/O Map() operation if the SetAttributes
> fails.
> 
> So return EFI_SUCCESS instead.

It's unclear to me why this change is safe (looking forward).
Does NonCoherentIoMmuDxe also imply no IoMmu actually exists?

/
     Leif

> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> This fixes a regression on Raspberry Pi4, which no longer boots.
> 
>   EmbeddedPkg/Drivers/NonCoherentIoMmuDxe/NonCoherentIoMmuDxe.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/EmbeddedPkg/Drivers/NonCoherentIoMmuDxe/NonCoherentIoMmuDxe.c b/EmbeddedPkg/Drivers/NonCoherentIoMmuDxe/NonCoherentIoMmuDxe.c
> index 4e7a5698c162..f02a76a62ea8 100644
> --- a/EmbeddedPkg/Drivers/NonCoherentIoMmuDxe/NonCoherentIoMmuDxe.c
> +++ b/EmbeddedPkg/Drivers/NonCoherentIoMmuDxe/NonCoherentIoMmuDxe.c
> @@ -70,7 +70,7 @@ NonCoherentIoMmuSetAttribute (
>     IN UINT64                IoMmuAccess
>     )
>   {
> -  return EFI_UNSUPPORTED;
> +  return EFI_SUCCESS;
>   }
>   
>   /**



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116691): https://edk2.groups.io/g/devel/message/116691
Mute This Topic: https://groups.io/mt/104886877/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] EmbeddedPkg/NonCoherentIoMmuDxe: Make SetAttributes always succeed

[Ard Biesheuvel]

On Tue, 12 Mar 2024 at 17:38, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> On 2024-03-12 08:17, Ard Biesheuvel wrote:
> > From: Ard Biesheuvel <ardb@kernel.org>
> >
> > NonCoherentIoMmuSetAttribute() does nothing except return
> > EFI_UNSUPPORTED. This was fine when it was introduced, but now, the PCI
> > bus driver will fail a PCI I/O Map() operation if the SetAttributes
> > fails.
> >
> > So return EFI_SUCCESS instead.
>
> It's unclear to me why this change is safe (looking forward).
> Does NonCoherentIoMmuDxe also imply no IoMmu actually exists?
>

Basically. NonCoherentIoMmuDxe is just a vehicle to allow
NonCoherentDmaLib to be plugged into the PCI host bridge driver. It is
not intended to ever do anything more than that.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116693): https://edk2.groups.io/g/devel/message/116693
Mute This Topic: https://groups.io/mt/104886877/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] EmbeddedPkg/NonCoherentIoMmuDxe: Make SetAttributes always succeed

[Leif Lindholm]

On 2024-03-12 09:50, Ard Biesheuvel wrote:
> On Tue, 12 Mar 2024 at 17:38, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>>
>> On 2024-03-12 08:17, Ard Biesheuvel wrote:
>>> From: Ard Biesheuvel <ardb@kernel.org>
>>>
>>> NonCoherentIoMmuSetAttribute() does nothing except return
>>> EFI_UNSUPPORTED. This was fine when it was introduced, but now, the PCI
>>> bus driver will fail a PCI I/O Map() operation if the SetAttributes
>>> fails.
>>>
>>> So return EFI_SUCCESS instead.
>>
>> It's unclear to me why this change is safe (looking forward).
>> Does NonCoherentIoMmuDxe also imply no IoMmu actually exists?
>>
> 
> Basically. NonCoherentIoMmuDxe is just a vehicle to allow
> NonCoherentDmaLib to be plugged into the PCI host bridge driver. It is
> not intended to ever do anything more than that.

Not that it needs to happen for this
(Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>)
but maybe we ought to consider renaming it then?
DummyIoMmuDxe?

/
     Leif



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116695): https://edk2.groups.io/g/devel/message/116695
Mute This Topic: https://groups.io/mt/104886877/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] EmbeddedPkg/NonCoherentIoMmuDxe: Make SetAttributes always succeed

[Ard Biesheuvel]

On Tue, 12 Mar 2024 at 17:56, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> On 2024-03-12 09:50, Ard Biesheuvel wrote:
> > On Tue, 12 Mar 2024 at 17:38, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
> >>
> >> On 2024-03-12 08:17, Ard Biesheuvel wrote:
> >>> From: Ard Biesheuvel <ardb@kernel.org>
> >>>
> >>> NonCoherentIoMmuSetAttribute() does nothing except return
> >>> EFI_UNSUPPORTED. This was fine when it was introduced, but now, the PCI
> >>> bus driver will fail a PCI I/O Map() operation if the SetAttributes
> >>> fails.
> >>>
> >>> So return EFI_SUCCESS instead.
> >>
> >> It's unclear to me why this change is safe (looking forward).
> >> Does NonCoherentIoMmuDxe also imply no IoMmu actually exists?
> >>
> >
> > Basically. NonCoherentIoMmuDxe is just a vehicle to allow
> > NonCoherentDmaLib to be plugged into the PCI host bridge driver. It is
> > not intended to ever do anything more than that.
>
> Not that it needs to happen for this
> (Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>)
> but maybe we ought to consider renaming it then?
> DummyIoMmuDxe?
>

Fair point. Or PassThroughIoMmuDxe perhaps?


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116696): https://edk2.groups.io/g/devel/message/116696
Mute This Topic: https://groups.io/mt/104886877/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] EmbeddedPkg/NonCoherentIoMmuDxe: Make SetAttributes always succeed

[Leif Lindholm]

On 2024-03-12 09:58, Ard Biesheuvel wrote:
> On Tue, 12 Mar 2024 at 17:56, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>>
>> On 2024-03-12 09:50, Ard Biesheuvel wrote:
>>> On Tue, 12 Mar 2024 at 17:38, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>>>>
>>>> On 2024-03-12 08:17, Ard Biesheuvel wrote:
>>>>> From: Ard Biesheuvel <ardb@kernel.org>
>>>>>
>>>>> NonCoherentIoMmuSetAttribute() does nothing except return
>>>>> EFI_UNSUPPORTED. This was fine when it was introduced, but now, the PCI
>>>>> bus driver will fail a PCI I/O Map() operation if the SetAttributes
>>>>> fails.
>>>>>
>>>>> So return EFI_SUCCESS instead.
>>>>
>>>> It's unclear to me why this change is safe (looking forward).
>>>> Does NonCoherentIoMmuDxe also imply no IoMmu actually exists?
>>>>
>>>
>>> Basically. NonCoherentIoMmuDxe is just a vehicle to allow
>>> NonCoherentDmaLib to be plugged into the PCI host bridge driver. It is
>>> not intended to ever do anything more than that.
>>
>> Not that it needs to happen for this
>> (Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>)
>> but maybe we ought to consider renaming it then?
>> DummyIoMmuDxe?
> 
> Fair point. Or PassThroughIoMmuDxe perhaps?

Works for me.
Or, hmm...
Is there a risk that sounds a bit like a driver that actively configures 
IoMmus into passthrough mode?

/
     Leif



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116697): https://edk2.groups.io/g/devel/message/116697
Mute This Topic: https://groups.io/mt/104886877/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-