diff mbox series

[2/2,RFC] arm64: Add dependencies to vendor-specific errata

Message ID 20200416115658.20406-3-geert+renesas@glider.be (mailing list archive)
State New, archived
Headers show
Series arm64: Vendor-specific errata improvements | expand

Commit Message

Geert Uytterhoeven April 16, 2020, 11:56 a.m. UTC
Currently the user is asked about enabling support for each and every
vendor-specific erratum, even when support for the specific platform is
not enabled.

Fix this by adding platform dependencies to the config options
controlling support for vendor-specific errata.

Note that FUJITSU_ERRATUM_010001 is left untouched, as no config symbol
exists for the Fujitsu A64FX platform.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 arch/arm64/Kconfig | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Mark Rutland April 16, 2020, 12:56 p.m. UTC | #1
Hi Geert,

On Thu, Apr 16, 2020 at 01:56:58PM +0200, Geert Uytterhoeven wrote:
> Currently the user is asked about enabling support for each and every
> vendor-specific erratum, even when support for the specific platform is
> not enabled.
> 
> Fix this by adding platform dependencies to the config options
> controlling support for vendor-specific errata.
> 
> Note that FUJITSU_ERRATUM_010001 is left untouched, as no config symbol
> exists for the Fujitsu A64FX platform.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

I'm not su1re that it makes sense to do this in general, becaose the
ARCH_* platform symbols are about plactform/SoC support (e.g. pinctrl
drivers), and these are (mostly) CPU-local and/or VM-visible.

I think that it makes sense for those to be independent because:

* future SoCs in the same family might not need the same CPU errata
  workarounds, and it's arguably just as confusing to have the option
  there.

* It prevents building a minimal VM image with all (non-virtualized)
  platform support disabled, but all possible (VM-visible) errata
  options enabled. I do that occassionally for testing/analysis, and I
  can imagine this is useful for those building images that are only
  intended to be used in VMs.

I think the change to SOCIONEXT_SYNQUACER_PREITS makes sense given
that's a platform-level detail. Arguably that should be moved into
drivers/irqchip/Kconfig.

Thanks,
Mark.

> ---
>  arch/arm64/Kconfig | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 8d33d7fed6d8549b..81f52f0b988e6350 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -622,6 +622,8 @@ config ARM64_ERRATUM_1542419
>  
>  	  If unsure, say Y.
>  
> +if ARCH_THUNDER2
> +
>  config CAVIUM_ERRATUM_22375
>  	bool "Cavium erratum 22375, 24313"
>  	default y
> @@ -697,6 +699,8 @@ config CAVIUM_TX2_ERRATUM_219
>  
>  	  If unsure, say Y.
>  
> +endif # ARCH_THUNDER2
> +
>  config FUJITSU_ERRATUM_010001
>  	bool "Fujitsu-A64FX erratum E#010001: Undefined fault may occur wrongly"
>  	default y
> @@ -718,6 +722,7 @@ config FUJITSU_ERRATUM_010001
>  
>  config HISILICON_ERRATUM_161600802
>  	bool "Hip07 161600802: Erroneous redistributor VLPI base"
> +	depends on ARCH_HISI
>  	default y
>  	help
>  	  The HiSilicon Hip07 SoC uses the wrong redistributor base
> @@ -726,6 +731,8 @@ config HISILICON_ERRATUM_161600802
>  
>  	  If unsure, say Y.
>  
> +if ARCH_QCOM
> +
>  config QCOM_FALKOR_ERRATUM_1003
>  	bool "Falkor E1003: Incorrect translation due to ASID change"
>  	default y
> @@ -768,8 +775,11 @@ config QCOM_FALKOR_ERRATUM_E1041
>  
>  	  If unsure, say Y.
>  
> +endif # ARCH_QCOM
> +
>  config SOCIONEXT_SYNQUACER_PREITS
>  	bool "Socionext Synquacer: Workaround for GICv3 pre-ITS"
> +	depends on ARCH_SYNQUACER
>  	default y
>  	help
>  	  Socionext Synquacer SoCs implement a separate h/w block to generate
> -- 
> 2.17.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Arnd Bergmann April 16, 2020, 1:36 p.m. UTC | #2
On Thu, Apr 16, 2020 at 2:56 PM Mark Rutland <mark.rutland@arm.com> wrote:
> On Thu, Apr 16, 2020 at 01:56:58PM +0200, Geert Uytterhoeven wrote:
> > Currently the user is asked about enabling support for each and every
> > vendor-specific erratum, even when support for the specific platform is
> > not enabled.
> >
> > Fix this by adding platform dependencies to the config options
> > controlling support for vendor-specific errata.
> >
> > Note that FUJITSU_ERRATUM_010001 is left untouched, as no config symbol
> > exists for the Fujitsu A64FX platform.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> I'm not su1re that it makes sense to do this in general, becaose the
> ARCH_* platform symbols are about plactform/SoC support (e.g. pinctrl
> drivers), and these are (mostly) CPU-local and/or VM-visible.
>
> I think that it makes sense for those to be independent because:
>
> * future SoCs in the same family might not need the same CPU errata
>   workarounds, and it's arguably just as confusing to have the option
>   there.
>
> * It prevents building a minimal VM image with all (non-virtualized)
>   platform support disabled, but all possible (VM-visible) errata
>   options enabled. I do that occassionally for testing/analysis, and I
>   can imagine this is useful for those building images that are only
>   intended to be used in VMs.

Most architectures over time grow a CPU selection option that is
at least somewhat orthogonal to the platform selection. I think so far
arm64 has intentionally resisted this based on the idea that the CPUs
are mostly equal and differences are better handled at runtime.

If we decide to revisit this in the future that might help both the
errata selection and give a way to e.g. build for an ARMv8.2
baseline.
This does seem pretty far out at the moment of course, given
that most SoCs we work on are still based on Cortex-A53 or A72 ;-)

> I think the change to SOCIONEXT_SYNQUACER_PREITS makes sense given
> that's a platform-level detail. Arguably that should be moved into
> drivers/irqchip/Kconfig.

Agreed

       Arnd
Geert Uytterhoeven April 16, 2020, 3:38 p.m. UTC | #3
Hi Mark,

On Thu, Apr 16, 2020 at 2:56 PM Mark Rutland <mark.rutland@arm.com> wrote:
> On Thu, Apr 16, 2020 at 01:56:58PM +0200, Geert Uytterhoeven wrote:
> > Currently the user is asked about enabling support for each and every
> > vendor-specific erratum, even when support for the specific platform is
> > not enabled.
> >
> > Fix this by adding platform dependencies to the config options
> > controlling support for vendor-specific errata.
> >
> > Note that FUJITSU_ERRATUM_010001 is left untouched, as no config symbol
> > exists for the Fujitsu A64FX platform.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> I'm not su1re that it makes sense to do this in general, becaose the
> ARCH_* platform symbols are about plactform/SoC support (e.g. pinctrl
> drivers), and these are (mostly) CPU-local and/or VM-visible.
>
> I think that it makes sense for those to be independent because:
>
> * future SoCs in the same family might not need the same CPU errata
>   workarounds, and it's arguably just as confusing to have the option
>   there.

True.  But at least the dependency restricts the confusion to a smaller
audience.

> * It prevents building a minimal VM image with all (non-virtualized)
>   platform support disabled, but all possible (VM-visible) errata
>   options enabled. I do that occassionally for testing/analysis, and I
>   can imagine this is useful for those building images that are only
>   intended to be used in VMs.

Oh, you also want to build a "generic" guest kernel, with all ARCH_*
symbols disabled. Let's hope a maleficent user cannot disable errata
mitigations in the guest kernel and break the host ;-)
And perhaps you do want to enable some platform-specific drivers for
VFIO pass-through?  Hence having ARCH_* dependencies on those drivers
means they cannot be enabled :-( Hmm...

> I think the change to SOCIONEXT_SYNQUACER_PREITS makes sense given
> that's a platform-level detail. Arguably that should be moved into
> drivers/irqchip/Kconfig.

OK, makes sense.

Gr{oetje,eeting}s,

                        Geert
Mark Rutland April 16, 2020, 3:56 p.m. UTC | #4
On Thu, Apr 16, 2020 at 05:38:07PM +0200, Geert Uytterhoeven wrote:
> On Thu, Apr 16, 2020 at 2:56 PM Mark Rutland <mark.rutland@arm.com> wrote:
> > On Thu, Apr 16, 2020 at 01:56:58PM +0200, Geert Uytterhoeven wrote:
> > > Currently the user is asked about enabling support for each and every
> > > vendor-specific erratum, even when support for the specific platform is
> > > not enabled.
> > >
> > > Fix this by adding platform dependencies to the config options
> > > controlling support for vendor-specific errata.

> > I'm not su1re that it makes sense to do this in general, becaose the
> > ARCH_* platform symbols are about plactform/SoC support (e.g. pinctrl
> > drivers), and these are (mostly) CPU-local and/or VM-visible.
> >
> > I think that it makes sense for those to be independent because:
 
> > * It prevents building a minimal VM image with all (non-virtualized)
> >   platform support disabled, but all possible (VM-visible) errata
> >   options enabled. I do that occassionally for testing/analysis, and I
> >   can imagine this is useful for those building images that are only
> >   intended to be used in VMs.
> 
> Oh, you also want to build a "generic" guest kernel, with all ARCH_*
> symbols disabled. 

Yup! As above I do this today for building test kernels I run on a
number of different hosts, and I'm aware of other use-cases (e.g. WSL2
or docker for mac) where you may want to do this to minimize the core
kernel either for size or security reasons.

> Let's hope a maleficent user cannot disable errata mitigations in the
> guest kernel and break the host ;-)

Indeed ;)

For cases where a malicious guest could cause harm we've added
workarounds in KVM, so unless we've missed something that shouldn't be
the case.

Otherwise, a guest missing these is just shooting itself in the foot.

> And perhaps you do want to enable some platform-specific drivers for
> VFIO pass-through?  Hence having ARCH_* dependencies on those drivers
> means they cannot be enabled :-( Hmm...

IIRC platform device passthrough requires an corresponding VFIO platform
driver in the host to handle reset and so on, but it does seem a shame
to not allow the user to select a driver if they really want it.

I guess there might be platform-specific PCIe drivers too, which might
work with VFIO regardless.

Thanks,
Mark.
Geert Uytterhoeven April 16, 2020, 4:18 p.m. UTC | #5
Hi Mark,

On Thu, Apr 16, 2020 at 5:57 PM Mark Rutland <mark.rutland@arm.com> wrote:
> On Thu, Apr 16, 2020 at 05:38:07PM +0200, Geert Uytterhoeven wrote:
> > And perhaps you do want to enable some platform-specific drivers for
> > VFIO pass-through?  Hence having ARCH_* dependencies on those drivers
> > means they cannot be enabled :-( Hmm...
>
> IIRC platform device passthrough requires an corresponding VFIO platform
> driver in the host to handle reset and so on, but it does seem a shame

If your SoC has a reset controller, that problem has been solved in a generic
way, cfr. "[PATCH v5] vfio: platform: Add generic reset controller support"
(https://lore.kernel.org/lkml/20181113131508.18246-1-geert+renesas@glider.be/).
Unfortunately not yet upstream.

Combine with "hw/arm/sysbus-fdt: Add support for instantiating generic devices"
(https://github.com/geertu/qemu/commit/180318003c08594e8e852b2285a98184f905bfa9)
and you're set ;-)

> to not allow the user to select a driver if they really want it.

I forgot you can add "|| VIRTIO_MMIO" to the dependencies of drivers for
devices that can be used with VFIO pass-through.

> I guess there might be platform-specific PCIe drivers too, which might
> work with VFIO regardless.

Indeed. PCI is business as usual.

Gr{oetje,eeting}s,

                        Geert
Robert Richter April 17, 2020, 3:57 p.m. UTC | #6
On 16.04.20 13:56:58, Geert Uytterhoeven wrote:
> Currently the user is asked about enabling support for each and every
> vendor-specific erratum, even when support for the specific platform is
> not enabled.
> 
> Fix this by adding platform dependencies to the config options
> controlling support for vendor-specific errata.
> 
> Note that FUJITSU_ERRATUM_010001 is left untouched, as no config symbol
> exists for the Fujitsu A64FX platform.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  arch/arm64/Kconfig | 10 ++++++++++
>  1 file changed, 10 insertions(+)

> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 8d33d7fed6d8549b..81f52f0b988e6350 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -622,6 +622,8 @@ config ARM64_ERRATUM_1542419
>  
>  	  If unsure, say Y.
>  
> +if ARCH_THUNDER2
> +
>  config CAVIUM_ERRATUM_22375
>  	bool "Cavium erratum 22375, 24313"
>  	default y
> @@ -697,6 +699,8 @@ config CAVIUM_TX2_ERRATUM_219
>  
>  	  If unsure, say Y.
>  
> +endif # ARCH_THUNDER2

For Cavium servers these ARCH_* options are only used to enable some
minor (mostly platform) drivers (i2c, spi, gpio, etc.), so the options
are not of much use and I better would like to get rid of them
completely, which makes us independent and more flexible when enabling
or disabling options. In ThunderX* systems there are not many devices
that are soc or board specific, most of them are detected using
generic methods like pci or acpi. So the situation is rather
comparable to x86 systems (there are options based on vendor or core)
than to ARM embedded chips.

Using the ARCH_* options to control also enablement of cpu errata
handling would add a strong dependency here and will make that options
a requirement.

How about having core/vendor specific submenus that make switching off
options easier?

Thanks,

-Robert
diff mbox series

Patch

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 8d33d7fed6d8549b..81f52f0b988e6350 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -622,6 +622,8 @@  config ARM64_ERRATUM_1542419
 
 	  If unsure, say Y.
 
+if ARCH_THUNDER2
+
 config CAVIUM_ERRATUM_22375
 	bool "Cavium erratum 22375, 24313"
 	default y
@@ -697,6 +699,8 @@  config CAVIUM_TX2_ERRATUM_219
 
 	  If unsure, say Y.
 
+endif # ARCH_THUNDER2
+
 config FUJITSU_ERRATUM_010001
 	bool "Fujitsu-A64FX erratum E#010001: Undefined fault may occur wrongly"
 	default y
@@ -718,6 +722,7 @@  config FUJITSU_ERRATUM_010001
 
 config HISILICON_ERRATUM_161600802
 	bool "Hip07 161600802: Erroneous redistributor VLPI base"
+	depends on ARCH_HISI
 	default y
 	help
 	  The HiSilicon Hip07 SoC uses the wrong redistributor base
@@ -726,6 +731,8 @@  config HISILICON_ERRATUM_161600802
 
 	  If unsure, say Y.
 
+if ARCH_QCOM
+
 config QCOM_FALKOR_ERRATUM_1003
 	bool "Falkor E1003: Incorrect translation due to ASID change"
 	default y
@@ -768,8 +775,11 @@  config QCOM_FALKOR_ERRATUM_E1041
 
 	  If unsure, say Y.
 
+endif # ARCH_QCOM
+
 config SOCIONEXT_SYNQUACER_PREITS
 	bool "Socionext Synquacer: Workaround for GICv3 pre-ITS"
+	depends on ARCH_SYNQUACER
 	default y
 	help
 	  Socionext Synquacer SoCs implement a separate h/w block to generate