[Linaro-uefi,v2,04/10] Platforms/AMD/StyxDtbLoaderLib: disable SMMUs for absent hardware

Message ID 20170627132145.28159-5-ard.biesheuvel@linaro.org
State New
Headers show
Series
  • Platforms/AMD/Styx: various Cello related fixes
Related show

Commit Message

Ard Biesheuvel June 27, 2017, 1:21 p.m.
The logic regarding how the various SMMUs are exposed in the device tree
is inverted, in the sense that they are present in the static DTB image,
and are removed if no SMMU support is requested.

However, the logic is flawed in the sense that it did not remove SMMUs
for hardware that is not there to begin with, i.e., the XGBE network
ports on Cello/Softiron 1000 or the second SATA controller on B1 silicon.

So fix that.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Leif Lindholm June 27, 2017, 4:08 p.m. | #1
On Tue, Jun 27, 2017 at 01:21:39PM +0000, Ard Biesheuvel wrote:
> The logic regarding how the various SMMUs are exposed in the device tree
> is inverted, in the sense that they are present in the static DTB image,
> and are removed if no SMMU support is requested.
> 
> However, the logic is flawed in the sense that it did not remove SMMUs
> for hardware that is not there to begin with, i.e., the XGBE network
> ports on Cello/Softiron 1000 or the second SATA controller on B1 silicon.
> 
> So fix that.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c b/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
> index 093db6517c1a..0da00655396e 100644
> --- a/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
> +++ b/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
> @@ -261,11 +261,18 @@ SetSocIdStatus (
>    if (!PcdGetBool (PcdEnableSmmus)) {
>      DisableSmmu (Fdt, "iommu-map", "/smb/smmu@e0a00000", "/smb/pcie@f0000000");
>      DisableSmmu (Fdt, "iommus", "/smb/smmu@e0200000", "/smb/sata@e0300000");
> +  }
> +
> +  if (!PcdGetBool (PcdEnableSmmus) || !IsRevB1 || FixedPcdGet8 (PcdSata1PortCount) == 0) {
>      DisableSmmu (Fdt, "iommus", "/smb/smmu@e0c00000", "/smb/sata@e0d00000");
> +  }
> +
>  #if DO_XGBE
> +  if (!PcdGetBool (PcdEnableSmmus))
> +#endif
> +  {
>      DisableSmmu (Fdt, "iommus", "/smb/smmu@e0600000", "/smb/xgmac@e0700000");
>      DisableSmmu (Fdt, "iommus", "/smb/smmu@e0800000", "/smb/xgmac@e0900000");
> -#endif
>    }

That's a little bit filthy.
Could you do

#if !DO_XBGE

Around the whole new if-statement instead?

/
    Leif

>  }
>  
> -- 
> 2.9.3
>
Ard Biesheuvel June 27, 2017, 4:11 p.m. | #2
On 27 June 2017 at 16:08, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Tue, Jun 27, 2017 at 01:21:39PM +0000, Ard Biesheuvel wrote:
>> The logic regarding how the various SMMUs are exposed in the device tree
>> is inverted, in the sense that they are present in the static DTB image,
>> and are removed if no SMMU support is requested.
>>
>> However, the logic is flawed in the sense that it did not remove SMMUs
>> for hardware that is not there to begin with, i.e., the XGBE network
>> ports on Cello/Softiron 1000 or the second SATA controller on B1 silicon.
>>
>> So fix that.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c b/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
>> index 093db6517c1a..0da00655396e 100644
>> --- a/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
>> +++ b/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
>> @@ -261,11 +261,18 @@ SetSocIdStatus (
>>    if (!PcdGetBool (PcdEnableSmmus)) {
>>      DisableSmmu (Fdt, "iommu-map", "/smb/smmu@e0a00000", "/smb/pcie@f0000000");
>>      DisableSmmu (Fdt, "iommus", "/smb/smmu@e0200000", "/smb/sata@e0300000");
>> +  }
>> +
>> +  if (!PcdGetBool (PcdEnableSmmus) || !IsRevB1 || FixedPcdGet8 (PcdSata1PortCount) == 0) {
>>      DisableSmmu (Fdt, "iommus", "/smb/smmu@e0c00000", "/smb/sata@e0d00000");
>> +  }
>> +
>>  #if DO_XGBE
>> +  if (!PcdGetBool (PcdEnableSmmus))
>> +#endif
>> +  {
>>      DisableSmmu (Fdt, "iommus", "/smb/smmu@e0600000", "/smb/xgmac@e0700000");
>>      DisableSmmu (Fdt, "iommus", "/smb/smmu@e0800000", "/smb/xgmac@e0900000");
>> -#endif
>>    }
>
> That's a little bit filthy.
> Could you do
>
> #if !DO_XBGE
>
> Around the whole new if-statement instead?
>

Not really. What I actually need is

if (!PcdGetBool (PcdEnableSmmus) || !DO_XGBE)

which is not equivalent. However, this breaks due to the fact the
DO_XGBE is not defined to 0 but simply not defined at all.
Ard Biesheuvel June 27, 2017, 5 p.m. | #3
On 27 June 2017 at 16:11, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 27 June 2017 at 16:08, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> On Tue, Jun 27, 2017 at 01:21:39PM +0000, Ard Biesheuvel wrote:
>>> The logic regarding how the various SMMUs are exposed in the device tree
>>> is inverted, in the sense that they are present in the static DTB image,
>>> and are removed if no SMMU support is requested.
>>>
>>> However, the logic is flawed in the sense that it did not remove SMMUs
>>> for hardware that is not there to begin with, i.e., the XGBE network
>>> ports on Cello/Softiron 1000 or the second SATA controller on B1 silicon.
>>>
>>> So fix that.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>  Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c | 9 ++++++++-
>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c b/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
>>> index 093db6517c1a..0da00655396e 100644
>>> --- a/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
>>> +++ b/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
>>> @@ -261,11 +261,18 @@ SetSocIdStatus (
>>>    if (!PcdGetBool (PcdEnableSmmus)) {
>>>      DisableSmmu (Fdt, "iommu-map", "/smb/smmu@e0a00000", "/smb/pcie@f0000000");
>>>      DisableSmmu (Fdt, "iommus", "/smb/smmu@e0200000", "/smb/sata@e0300000");
>>> +  }
>>> +
>>> +  if (!PcdGetBool (PcdEnableSmmus) || !IsRevB1 || FixedPcdGet8 (PcdSata1PortCount) == 0) {
>>>      DisableSmmu (Fdt, "iommus", "/smb/smmu@e0c00000", "/smb/sata@e0d00000");
>>> +  }
>>> +
>>>  #if DO_XGBE
>>> +  if (!PcdGetBool (PcdEnableSmmus))
>>> +#endif
>>> +  {
>>>      DisableSmmu (Fdt, "iommus", "/smb/smmu@e0600000", "/smb/xgmac@e0700000");
>>>      DisableSmmu (Fdt, "iommus", "/smb/smmu@e0800000", "/smb/xgmac@e0900000");
>>> -#endif
>>>    }
>>
>> That's a little bit filthy.
>> Could you do
>>
>> #if !DO_XBGE
>>
>> Around the whole new if-statement instead?
>>
>
> Not really. What I actually need is
>
> if (!PcdGetBool (PcdEnableSmmus) || !DO_XGBE)
>
> which is not equivalent. However, this breaks due to the fact the
> DO_XGBE is not defined to 0 but simply not defined at all.

How about

#if DO_XGBE
  DisableXgbeSmmus = !PcdGetBool (PcdEnableSmmus);
#else
  DisableXgbeSmmus = TRUE;
#endif

  if (DisableXgbeSmmus) {
    DisableSmmu (Fdt, "iommus", "/smb/smmu@e0600000", "/smb/xgmac@e0700000");
    DisableSmmu (Fdt, "iommus", "/smb/smmu@e0800000", "/smb/xgmac@e0900000");
  }

?
Leif Lindholm June 27, 2017, 5:21 p.m. | #4
On Tue, Jun 27, 2017 at 05:00:12PM +0000, Ard Biesheuvel wrote:
> On 27 June 2017 at 16:11, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > On 27 June 2017 at 16:08, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >> On Tue, Jun 27, 2017 at 01:21:39PM +0000, Ard Biesheuvel wrote:
> >>> The logic regarding how the various SMMUs are exposed in the device tree
> >>> is inverted, in the sense that they are present in the static DTB image,
> >>> and are removed if no SMMU support is requested.
> >>>
> >>> However, the logic is flawed in the sense that it did not remove SMMUs
> >>> for hardware that is not there to begin with, i.e., the XGBE network
> >>> ports on Cello/Softiron 1000 or the second SATA controller on B1 silicon.
> >>>
> >>> So fix that.
> >>>
> >>> Contributed-under: TianoCore Contribution Agreement 1.0
> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>> ---
> >>>  Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c | 9 ++++++++-
> >>>  1 file changed, 8 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c b/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
> >>> index 093db6517c1a..0da00655396e 100644
> >>> --- a/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
> >>> +++ b/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
> >>> @@ -261,11 +261,18 @@ SetSocIdStatus (
> >>>    if (!PcdGetBool (PcdEnableSmmus)) {
> >>>      DisableSmmu (Fdt, "iommu-map", "/smb/smmu@e0a00000", "/smb/pcie@f0000000");
> >>>      DisableSmmu (Fdt, "iommus", "/smb/smmu@e0200000", "/smb/sata@e0300000");
> >>> +  }
> >>> +
> >>> +  if (!PcdGetBool (PcdEnableSmmus) || !IsRevB1 || FixedPcdGet8 (PcdSata1PortCount) == 0) {
> >>>      DisableSmmu (Fdt, "iommus", "/smb/smmu@e0c00000", "/smb/sata@e0d00000");
> >>> +  }
> >>> +
> >>>  #if DO_XGBE
> >>> +  if (!PcdGetBool (PcdEnableSmmus))
> >>> +#endif
> >>> +  {
> >>>      DisableSmmu (Fdt, "iommus", "/smb/smmu@e0600000", "/smb/xgmac@e0700000");
> >>>      DisableSmmu (Fdt, "iommus", "/smb/smmu@e0800000", "/smb/xgmac@e0900000");
> >>> -#endif
> >>>    }
> >>
> >> That's a little bit filthy.
> >> Could you do
> >>
> >> #if !DO_XBGE
> >>
> >> Around the whole new if-statement instead?
> >>
> >
> > Not really. What I actually need is
> >
> > if (!PcdGetBool (PcdEnableSmmus) || !DO_XGBE)
> >
> > which is not equivalent. However, this breaks due to the fact the
> > DO_XGBE is not defined to 0 but simply not defined at all.
> 
> How about
> 
> #if DO_XGBE
>   DisableXgbeSmmus = !PcdGetBool (PcdEnableSmmus);
> #else
>   DisableXgbeSmmus = TRUE;
> #endif
> 
>   if (DisableXgbeSmmus) {
>     DisableSmmu (Fdt, "iommus", "/smb/smmu@e0600000", "/smb/xgmac@e0700000");
>     DisableSmmu (Fdt, "iommus", "/smb/smmu@e0800000", "/smb/xgmac@e0900000");
>   }
> 
> ?

That looks optimal.
With that:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

Patch hide | download patch | download mbox

diff --git a/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c b/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
index 093db6517c1a..0da00655396e 100644
--- a/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
+++ b/Platforms/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
@@ -261,11 +261,18 @@  SetSocIdStatus (
   if (!PcdGetBool (PcdEnableSmmus)) {
     DisableSmmu (Fdt, "iommu-map", "/smb/smmu@e0a00000", "/smb/pcie@f0000000");
     DisableSmmu (Fdt, "iommus", "/smb/smmu@e0200000", "/smb/sata@e0300000");
+  }
+
+  if (!PcdGetBool (PcdEnableSmmus) || !IsRevB1 || FixedPcdGet8 (PcdSata1PortCount) == 0) {
     DisableSmmu (Fdt, "iommus", "/smb/smmu@e0c00000", "/smb/sata@e0d00000");
+  }
+
 #if DO_XGBE
+  if (!PcdGetBool (PcdEnableSmmus))
+#endif
+  {
     DisableSmmu (Fdt, "iommus", "/smb/smmu@e0600000", "/smb/xgmac@e0700000");
     DisableSmmu (Fdt, "iommus", "/smb/smmu@e0800000", "/smb/xgmac@e0900000");
-#endif
   }
 }