* [PATCH edk2-platforms 1/2] Silicon/SynQuacer/PlatformDxe: set NETSEC DMA as coherent @ 2019-11-28 14:48 Ard Biesheuvel 2019-11-28 14:48 ` [PATCH edk2-platforms 2/2] Platform/DeveloperBox: disable variable runtime cache Ard Biesheuvel 2019-11-28 14:56 ` [PATCH edk2-platforms 1/2] Silicon/SynQuacer/PlatformDxe: set NETSEC DMA as coherent Leif Lindholm 0 siblings, 2 replies; 9+ messages in thread From: Ard Biesheuvel @ 2019-11-28 14:48 UTC (permalink / raw) To: devel; +Cc: leif.lindholm, 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH edk2-platforms 2/2] Platform/DeveloperBox: disable variable runtime cache 2019-11-28 14:48 [PATCH edk2-platforms 1/2] Silicon/SynQuacer/PlatformDxe: set NETSEC DMA as coherent Ard Biesheuvel @ 2019-11-28 14:48 ` Ard Biesheuvel 2019-11-28 15:01 ` Leif Lindholm 2019-11-28 14:56 ` [PATCH edk2-platforms 1/2] Silicon/SynQuacer/PlatformDxe: set NETSEC DMA as coherent Leif Lindholm 1 sibling, 1 reply; 9+ messages in thread From: Ard Biesheuvel @ 2019-11-28 14:48 UTC (permalink / raw) To: devel; +Cc: leif.lindholm, 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH edk2-platforms 2/2] Platform/DeveloperBox: disable variable runtime cache 2019-11-28 14:48 ` [PATCH edk2-platforms 2/2] Platform/DeveloperBox: disable variable runtime cache Ard Biesheuvel @ 2019-11-28 15:01 ` Leif Lindholm 2019-11-28 15:04 ` Ard Biesheuvel 0 siblings, 1 reply; 9+ messages in thread From: Leif Lindholm @ 2019-11-28 15:01 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: devel 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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH edk2-platforms 2/2] Platform/DeveloperBox: disable variable runtime cache 2019-11-28 15:01 ` Leif Lindholm @ 2019-11-28 15:04 ` Ard Biesheuvel 2019-11-28 15:20 ` [edk2-devel] " Leif Lindholm 0 siblings, 1 reply; 9+ messages in thread From: Ard Biesheuvel @ 2019-11-28 15:04 UTC (permalink / raw) To: Leif Lindholm; +Cc: edk2-devel-groups-io 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 > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH edk2-platforms 2/2] Platform/DeveloperBox: disable variable runtime cache 2019-11-28 15:04 ` Ard Biesheuvel @ 2019-11-28 15:20 ` Leif Lindholm 0 siblings, 0 replies; 9+ messages in thread From: Leif Lindholm @ 2019-11-28 15:20 UTC (permalink / raw) To: devel, ard.biesheuvel 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 > > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH edk2-platforms 1/2] Silicon/SynQuacer/PlatformDxe: set NETSEC DMA as coherent 2019-11-28 14:48 [PATCH edk2-platforms 1/2] Silicon/SynQuacer/PlatformDxe: set NETSEC DMA as coherent Ard Biesheuvel 2019-11-28 14:48 ` [PATCH edk2-platforms 2/2] Platform/DeveloperBox: disable variable runtime cache Ard Biesheuvel @ 2019-11-28 14:56 ` Leif Lindholm 2019-11-28 14:59 ` Ard Biesheuvel 1 sibling, 1 reply; 9+ messages in thread From: Leif Lindholm @ 2019-11-28 14:56 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: devel 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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH edk2-platforms 1/2] Silicon/SynQuacer/PlatformDxe: set NETSEC DMA as coherent 2019-11-28 14:56 ` [PATCH edk2-platforms 1/2] Silicon/SynQuacer/PlatformDxe: set NETSEC DMA as coherent Leif Lindholm @ 2019-11-28 14:59 ` Ard Biesheuvel 2019-11-28 15:19 ` Leif Lindholm 0 siblings, 1 reply; 9+ messages in thread From: Ard Biesheuvel @ 2019-11-28 14:59 UTC (permalink / raw) To: Leif Lindholm; +Cc: edk2-devel-groups-io 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 > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH edk2-platforms 1/2] Silicon/SynQuacer/PlatformDxe: set NETSEC DMA as coherent 2019-11-28 14:59 ` Ard Biesheuvel @ 2019-11-28 15:19 ` Leif Lindholm 2019-11-28 15:58 ` Ard Biesheuvel 0 siblings, 1 reply; 9+ messages in thread From: Leif Lindholm @ 2019-11-28 15:19 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: edk2-devel-groups-io 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 > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH edk2-platforms 1/2] Silicon/SynQuacer/PlatformDxe: set NETSEC DMA as coherent 2019-11-28 15:19 ` Leif Lindholm @ 2019-11-28 15:58 ` Ard Biesheuvel 0 siblings, 0 replies; 9+ messages in thread From: Ard Biesheuvel @ 2019-11-28 15:58 UTC (permalink / raw) To: Leif Lindholm; +Cc: edk2-devel-groups-io 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, ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-11-28 15:58 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-11-28 14:48 [PATCH edk2-platforms 1/2] Silicon/SynQuacer/PlatformDxe: set NETSEC DMA as coherent Ard Biesheuvel 2019-11-28 14:48 ` [PATCH edk2-platforms 2/2] Platform/DeveloperBox: disable variable runtime cache Ard Biesheuvel 2019-11-28 15:01 ` Leif Lindholm 2019-11-28 15:04 ` Ard Biesheuvel 2019-11-28 15:20 ` [edk2-devel] " Leif Lindholm 2019-11-28 14:56 ` [PATCH edk2-platforms 1/2] Silicon/SynQuacer/PlatformDxe: set NETSEC DMA as coherent Leif Lindholm 2019-11-28 14:59 ` Ard Biesheuvel 2019-11-28 15:19 ` Leif Lindholm 2019-11-28 15:58 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox