[PATCH edk2-platforms 1/2] Silicon/SynQuacer/PlatformDxe: set NETSEC DMA as coherent

[PATCH edk2-platforms 1/2] Silicon/SynQuacer/PlatformDxe: set NETSEC DMA as coherent

[Ard Biesheuvel]

We switched to cache coherent DMA for the NETSEC network controller ages
ago, but the platform driver that registers the non-discoverable device
currently does not reflect this change, which we haven't noticed since
the driver doesn't look at this flag.

Let's fix this nonetheless, in case it ever matters.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c
index 73cc560fa8d8..57f8fa90343a 100644
--- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c
+++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c
@@ -130,7 +130,7 @@ RegisterDevice (
   }
 
   Device->Type = TypeGuid;
-  Device->DmaType = NonDiscoverableDeviceDmaTypeNonCoherent;
+  Device->DmaType = NonDiscoverableDeviceDmaTypeCoherent;
   Device->Resources = Desc;
 
   Status = gBS->InstallMultipleProtocolInterfaces (Handle,
-- 
2.20.1

[PATCH edk2-platforms 2/2] Platform/DeveloperBox: disable variable runtime cache

[Ard Biesheuvel]

The variable runtime cache for SMM enabled implementations of the
variable runtime DXE driver was introduced after the standalone
MM based implementation was merged for the DeveloperBox platform.

This means the combined binary image of ARM Trusted Firmware and
the standalone MM runtime we carry in edk2-non-osi predates this
feature, so we need to disable it when building the non-secure side
when incorporating the prebuilt binary.

Whether it makes sense to enable the runtime cache for this platform
is a different question, so let's keep it enabled entirely until
someone identifies a need for it.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
index a10e48ca07ea..084b4c994b97 100644
--- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
+++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
@@ -130,6 +130,8 @@ [PcdsFeatureFlag]
   # needed for NFIT tables installed by RamDiskDxe
   gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol|TRUE
 
+  gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache|FALSE
+
 [PcdsFixedAtBuild.common]
 !ifdef $(FIRMWARE_VENDOR)
   gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor|L"$(FIRMWARE_VENDOR)"
-- 
2.20.1

Re: [PATCH edk2-platforms 2/2] Platform/DeveloperBox: disable variable runtime cache

[Leif Lindholm]

On Thu, Nov 28, 2019 at 15:48:40 +0100, Ard Biesheuvel wrote:
> The variable runtime cache for SMM enabled implementations of the
> variable runtime DXE driver was introduced after the standalone
> MM based implementation was merged for the DeveloperBox platform.
> 
> This means the combined binary image of ARM Trusted Firmware and
> the standalone MM runtime we carry in edk2-non-osi predates this
> feature, so we need to disable it when building the non-secure side
> when incorporating the prebuilt binary.

Is this something that needs to be explicitly disabled in current
upstream TF? (And if so, could we add a comment to edk2-non-osi
README?) Or does this only affect the requests made from Non-secure
side?

> Whether it makes sense to enable the runtime cache for this platform
> is a different question, so let's keep it enabled entirely until

enabled -> disabled?

/
    Leif

> someone identifies a need for it.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
> index a10e48ca07ea..084b4c994b97 100644
> --- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
> +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
> @@ -130,6 +130,8 @@ [PcdsFeatureFlag]
>    # needed for NFIT tables installed by RamDiskDxe
>    gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol|TRUE
>  
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache|FALSE
> +
>  [PcdsFixedAtBuild.common]
>  !ifdef $(FIRMWARE_VENDOR)
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor|L"$(FIRMWARE_VENDOR)"
> -- 
> 2.20.1
> 

Re: [PATCH edk2-platforms 2/2] Platform/DeveloperBox: disable variable runtime cache

[Ard Biesheuvel]

On Thu, 28 Nov 2019 at 16:02, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Thu, Nov 28, 2019 at 15:48:40 +0100, Ard Biesheuvel wrote:
> > The variable runtime cache for SMM enabled implementations of the
> > variable runtime DXE driver was introduced after the standalone
> > MM based implementation was merged for the DeveloperBox platform.
> >
> > This means the combined binary image of ARM Trusted Firmware and
> > the standalone MM runtime we carry in edk2-non-osi predates this
> > feature, so we need to disable it when building the non-secure side
> > when incorporating the prebuilt binary.
>
> Is this something that needs to be explicitly disabled in current
> upstream TF? (And if so, could we add a comment to edk2-non-osi
> README?) Or does this only affect the requests made from Non-secure
> side?
>

No, it has to be disabled on both sides. But this .dsc.inc gets
included by both sides when you build them from scratch.

> > Whether it makes sense to enable the runtime cache for this platform
> > is a different question, so let's keep it enabled entirely until
>
> enabled -> disabled?
>

Yep

>
> > someone identifies a need for it.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
> > index a10e48ca07ea..084b4c994b97 100644
> > --- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
> > +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
> > @@ -130,6 +130,8 @@ [PcdsFeatureFlag]
> >    # needed for NFIT tables installed by RamDiskDxe
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol|TRUE
> >
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache|FALSE
> > +
> >  [PcdsFixedAtBuild.common]
> >  !ifdef $(FIRMWARE_VENDOR)
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor|L"$(FIRMWARE_VENDOR)"
> > --
> > 2.20.1
> >

Re: [edk2-devel] [PATCH edk2-platforms 2/2] Platform/DeveloperBox: disable variable runtime cache

[Leif Lindholm]

On Thu, Nov 28, 2019 at 16:04:29 +0100, Ard Biesheuvel wrote:
> On Thu, 28 Nov 2019 at 16:02, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >
> > On Thu, Nov 28, 2019 at 15:48:40 +0100, Ard Biesheuvel wrote:
> > > The variable runtime cache for SMM enabled implementations of the
> > > variable runtime DXE driver was introduced after the standalone
> > > MM based implementation was merged for the DeveloperBox platform.
> > >
> > > This means the combined binary image of ARM Trusted Firmware and
> > > the standalone MM runtime we carry in edk2-non-osi predates this
> > > feature, so we need to disable it when building the non-secure side
> > > when incorporating the prebuilt binary.
> >
> > Is this something that needs to be explicitly disabled in current
> > upstream TF? (And if so, could we add a comment to edk2-non-osi
> > README?) Or does this only affect the requests made from Non-secure
> > side?
> 
> No, it has to be disabled on both sides. But this .dsc.inc gets
> included by both sides when you build them from scratch.

Oh, right. That's fine then.

> > > Whether it makes sense to enable the runtime cache for this platform
> > > is a different question, so let's keep it enabled entirely until
> >
> > enabled -> disabled?
> 
> Yep

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


> >
> > > someone identifies a need for it.
> > >
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > ---
> > >  Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
> > > index a10e48ca07ea..084b4c994b97 100644
> > > --- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
> > > +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
> > > @@ -130,6 +130,8 @@ [PcdsFeatureFlag]
> > >    # needed for NFIT tables installed by RamDiskDxe
> > >    gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol|TRUE
> > >
> > > +  gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache|FALSE
> > > +
> > >  [PcdsFixedAtBuild.common]
> > >  !ifdef $(FIRMWARE_VENDOR)
> > >    gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor|L"$(FIRMWARE_VENDOR)"
> > > --
> > > 2.20.1
> > >
> 
> 
> 

Re: [PATCH edk2-platforms 1/2] Silicon/SynQuacer/PlatformDxe: set NETSEC DMA as coherent

[Leif Lindholm]

On Thu, Nov 28, 2019 at 15:48:39 +0100, Ard Biesheuvel wrote:
> We switched to cache coherent DMA for the NETSEC network controller ages
> ago, but the platform driver that registers the non-discoverable device
> currently does not reflect this change, which we haven't noticed since
> the driver doesn't look at this flag.

Should the drive look at this flag?

> Let's fix this nonetheless, in case it ever matters.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

This patch clearly makes sense anyway, so:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>


> ---
>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c
> index 73cc560fa8d8..57f8fa90343a 100644
> --- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c
> +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c
> @@ -130,7 +130,7 @@ RegisterDevice (
>    }
>  
>    Device->Type = TypeGuid;
> -  Device->DmaType = NonDiscoverableDeviceDmaTypeNonCoherent;
> +  Device->DmaType = NonDiscoverableDeviceDmaTypeCoherent;
>    Device->Resources = Desc;
>  
>    Status = gBS->InstallMultipleProtocolInterfaces (Handle,
> -- 
> 2.20.1
> 

Re: [PATCH edk2-platforms 1/2] Silicon/SynQuacer/PlatformDxe: set NETSEC DMA as coherent

[Ard Biesheuvel]

On Thu, 28 Nov 2019 at 15:56, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Thu, Nov 28, 2019 at 15:48:39 +0100, Ard Biesheuvel wrote:
> > We switched to cache coherent DMA for the NETSEC network controller ages
> > ago, but the platform driver that registers the non-discoverable device
> > currently does not reflect this change, which we haven't noticed since
> > the driver doesn't look at this flag.
>
> Should the drive look at this flag?
>

The driver links against DmaLib, so it is either built for coherent
DMA only, or for non-coherent DMA only. This is different from the
eMMC on this platform, since it uses NonDiscoverablePciDeviceDxe,
which implements support for both coherent and non-coherent DMA in the
same driver, so there it has to be accurate or things break. For
NETSEC, we could add a check I suppose, but it is certainly not a high
prio thing imo.

> > Let's fix this nonetheless, in case it ever matters.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> This patch clearly makes sense anyway, so:
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>

Thanks

>
> > ---
> >  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c
> > index 73cc560fa8d8..57f8fa90343a 100644
> > --- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c
> > +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c
> > @@ -130,7 +130,7 @@ RegisterDevice (
> >    }
> >
> >    Device->Type = TypeGuid;
> > -  Device->DmaType = NonDiscoverableDeviceDmaTypeNonCoherent;
> > +  Device->DmaType = NonDiscoverableDeviceDmaTypeCoherent;
> >    Device->Resources = Desc;
> >
> >    Status = gBS->InstallMultipleProtocolInterfaces (Handle,
> > --
> > 2.20.1
> >

Re: [PATCH edk2-platforms 1/2] Silicon/SynQuacer/PlatformDxe: set NETSEC DMA as coherent

[Leif Lindholm]

On Thu, Nov 28, 2019 at 15:59:14 +0100, Ard Biesheuvel wrote:
> On Thu, 28 Nov 2019 at 15:56, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >
> > On Thu, Nov 28, 2019 at 15:48:39 +0100, Ard Biesheuvel wrote:
> > > We switched to cache coherent DMA for the NETSEC network controller ages
> > > ago, but the platform driver that registers the non-discoverable device
> > > currently does not reflect this change, which we haven't noticed since
> > > the driver doesn't look at this flag.
> >
> > Should the drive look at this flag?
> >
> 
> The driver links against DmaLib, so it is either built for coherent
> DMA only, or for non-coherent DMA only. This is different from the
> eMMC on this platform, since it uses NonDiscoverablePciDeviceDxe,
> which implements support for both coherent and non-coherent DMA in the
> same driver, so there it has to be accurate or things break. For
> NETSEC, we could add a check I suppose, but it is certainly not a high
> prio thing imo.

Ah, fair enough.
Thanks for the explanation.

/
    Leif

> > > Let's fix this nonetheless, in case it ever matters.
> > >
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >
> > This patch clearly makes sense anyway, so:
> > Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> >
> 
> Thanks
> 
> >
> > > ---
> > >  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c
> > > index 73cc560fa8d8..57f8fa90343a 100644
> > > --- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c
> > > +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c
> > > @@ -130,7 +130,7 @@ RegisterDevice (
> > >    }
> > >
> > >    Device->Type = TypeGuid;
> > > -  Device->DmaType = NonDiscoverableDeviceDmaTypeNonCoherent;
> > > +  Device->DmaType = NonDiscoverableDeviceDmaTypeCoherent;
> > >    Device->Resources = Desc;
> > >
> > >    Status = gBS->InstallMultipleProtocolInterfaces (Handle,
> > > --
> > > 2.20.1
> > >

Re: [PATCH edk2-platforms 1/2] Silicon/SynQuacer/PlatformDxe: set NETSEC DMA as coherent

[Ard Biesheuvel]

On Thu, 28 Nov 2019 at 16:19, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Thu, Nov 28, 2019 at 15:59:14 +0100, Ard Biesheuvel wrote:
> > On Thu, 28 Nov 2019 at 15:56, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > >
> > > On Thu, Nov 28, 2019 at 15:48:39 +0100, Ard Biesheuvel wrote:
> > > > We switched to cache coherent DMA for the NETSEC network controller ages
> > > > ago, but the platform driver that registers the non-discoverable device
> > > > currently does not reflect this change, which we haven't noticed since
> > > > the driver doesn't look at this flag.
> > >
> > > Should the drive look at this flag?
> > >
> >
> > The driver links against DmaLib, so it is either built for coherent
> > DMA only, or for non-coherent DMA only. This is different from the
> > eMMC on this platform, since it uses NonDiscoverablePciDeviceDxe,
> > which implements support for both coherent and non-coherent DMA in the
> > same driver, so there it has to be accurate or things break. For
> > NETSEC, we could add a check I suppose, but it is certainly not a high
> > prio thing imo.
>
> Ah, fair enough.
> Thanks for the explanation.
>

Series pushed as 6bde2876c3aa..8e75ee0c9653

Thanks,