Re: [PATCH 2/2] platform/x86: Add Intel Galileo platform specific setup

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Mon, Dec 29, 2014 at 7:23 PM, Bryan O'Donoghue
<pure.logic@nexus-software.ie> wrote:
> Intel Galileo Gen1 and Gen2 boot to Linux from EFI and grub with IMR
> registers enabled around the compressed kernel image and boot params data
> structure.
>
> The purpose of the IMRs around the compressed kernel and boot params data
> structure is to ensure that no spurious data writes from any agent within
> the Quark system can corrupt the kernel/boot-params data during boot.
>
> The kernel needs to tear-down the IMRs placed around the compressed kernel
> image and boot-params data structure since the EFI memory map marks the two
> regions of memory as usable memory and the kernel will happily reuse these
> memory regions. Without tearing down the boot-time IMRs drivers run the
> significant risk of violating one of the stale IMRs since dma_alloc_coherent
> can hand addresses to DMA capable south cluster peripherals such as the SD,
> Ethernet, USB host/device, which will then cause an IMR access violation
> when a DMA read/write occurs to the address ranges in question.
>
> Since the Quark EFI bringup code configures the system to reset on an IMR
> violation, this means that common operations such as mouting an SD based
> root filesystem, communicating with a USB device or sending Ethernet traffic
> can cause an immediate system reset.
>
> IMR usage during system boot on Galileo is detailed in
> Quark_SecureBoot_PRM_330234_001.pdf. This document details each IMR used
> during the boot process and the data being protected by that IMR. The kernel
> needs tidy-up IMRs used during the boot process to ensure an IMR violation
> doesn't cause a system reset.
>
> This platform code does two things.
>
> Firstly it tears down the boot-time IMRs used to protect the compressed
> kernel image and boot params data structure.
>
> Secondly it sets up an IMR around the kernel's text section from &_sinittext
> - &_text ensuring that on the CPU in non-SMM mode can read/write this
> address range. A spurious DMA write to the kernel's .text section will
> then cause an IMR violation and system reset, consistent with the current
> Galileo BSP behaviour.
>
> Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
> ---
>  drivers/platform/x86/Kconfig         |  15 +++
>  drivers/platform/x86/Makefile        |   1 +
>  drivers/platform/x86/intel_galileo.c | 175 +++++++++++++++++++++++++++++++++++
>  3 files changed, 191 insertions(+)
>  create mode 100644 drivers/platform/x86/intel_galileo.c
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 638e7970..e384dcd 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -804,6 +804,21 @@ config INTEL_OAKTRAIL
>           enable/disable the Camera, WiFi, BT etc. devices. If in doubt, say Y
>           here; it will only load on supported platforms.
>
> +config INTEL_GALILEO
> +       bool "Intel Galileo Platform Support"
> +       depends on X86_32 && PCI
> +       select IOSF_MBI
> +       select IMR
> +       ---help---
> +         Intel Galileo platform support. This code is used to tear-down
> +         BIOS and Grub Isolated Memory Regions used during bootup.
> +         This sanitises the IMR memory map to agree with the EFI/e820
> +         memory map, without this code your IMR memory map will conflict
> +         with the EFI memory map and it's highly likely DMA accesses initiated
> +         by Ethernet, SD and/or USB will result in a system reset.
> +
> +         If in doubt, say Y here, the code will only run on a Galileo Gen1/Gen2
> +
>  config SAMSUNG_Q10
>         tristate "Samsung Q10 Extras"
>         depends on ACPI
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index f82232b..a0c013d 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -51,6 +51,7 @@ obj-$(CONFIG_SAMSUNG_LAPTOP)  += samsung-laptop.o
>  obj-$(CONFIG_MXM_WMI)          += mxm-wmi.o
>  obj-$(CONFIG_INTEL_MID_POWER_BUTTON)   += intel_mid_powerbtn.o
>  obj-$(CONFIG_INTEL_OAKTRAIL)   += intel_oaktrail.o
> +obj-$(CONFIG_INTEL_GALILEO)    += intel_galileo.o
>  obj-$(CONFIG_SAMSUNG_Q10)      += samsung-q10.o
>  obj-$(CONFIG_APPLE_GMUX)       += apple-gmux.o
>  obj-$(CONFIG_INTEL_RST)                += intel-rst.o
> diff --git a/drivers/platform/x86/intel_galileo.c b/drivers/platform/x86/intel_galileo.c
> new file mode 100644
> index 0000000..2a555aa
> --- /dev/null
> +++ b/drivers/platform/x86/intel_galileo.c
> @@ -0,0 +1,175 @@
> +/*
> + * intel_galileo.c - Intel Galileo platform support.
> + *
> + * Copyright(c) 2013 Intel Corporation.
> + * Copyright(c) 2014 Bryan O'Donoghue <pure.logic@nexus-software.ie>
> + *
> + * This platform code provides an entry point to do Galileo specific
> + * setup. Critically IMRs are policed to ensure the EFI provided memory
> + * map informing the kernel of it's available memory is consistent with
> + * the IMR lock-down

Missed dot at the end.

> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */

Define pr_fmt().

> +#include <asm-generic/sections.h>
> +#include <asm/imr.h>
> +#include <asm/intel-quark.h>
> +#include <linux/dmi.h>
> +#include <linux/module.h>
> +#include <linux/mm.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +
> +enum {
> +       GALILEO_UNKNOWN = 0,
> +       GALILEO_QRK_GEN1,
> +       GALILEO_QRK_GEN2,
> +};
> +
> +static struct dmi_system_id galileo_baseboards[] = {
> +       {
> +               .ident = "Galileo",
> +               .matches = {
> +                       DMI_MATCH(DMI_BOARD_VENDOR, "Intel"),
> +                       DMI_MATCH(DMI_BOARD_NAME, "Galileo"),
> +               },
> +               .driver_data = (void *)GALILEO_QRK_GEN1,
> +       },
> +       {
> +               .ident = "GalileoGen2",
> +               .matches = {
> +                       DMI_MATCH(DMI_BOARD_VENDOR, "Intel"),
> +                       DMI_MATCH(DMI_BOARD_NAME, "GalileoGen2"),
> +               },
> +               .driver_data = (void *)GALILEO_QRK_GEN2,
> +       },
> +       {}
> +};
> +
> +#ifdef DEBUG

Move this inside function.

> +#define SANITY "IMR : sanity error "

You may define this before function and undefine later. (Actually you
missed undef)

> +
> +/**
> + * intel_galileo_imr_sanity
> + * Verify IMR sanity with some simple tests to verify
> + * overlap, zero sized allocations
> + *
> + * @base: Physical base address of the kernel text section
> + * @size: Extent of kernel memory to be covered from &_text to &_sinittext

> + * @return: none

Redundant.

> + */
> +static void __init
> +intel_galileo_imr_sanity(unsigned long base, unsigned long size)
> +{
> +       /* Test zero zero */
> +       if (imr_add(0, 0, 0, 0, true) == 0)
> +               pr_err(SANITY "zero sized IMR @ 0x00000000\n");
> +
> +       /* Test overlap */
> +       if (imr_add(base, size, IMR_NON_SMM, IMR_NON_SMM, true) == 0)
> +               pr_err(SANITY "overlapped IMR @ (0x%08lx - 0x%08lx)\n",
> +                      base, base + size);
> +
> +       /* Try overlap - IMR_ALIGN */
> +       base = base + size - IMR_ALIGN;

base += size ...

> +       if (imr_add(base, size, IMR_NON_SMM, IMR_NON_SMM, true) == 0)
> +               pr_err(SANITY "overlapped IMR @ (0x%08lx - 0x%08lx)\n",
> +                      base, base + size);
> +}
> +#endif
> +
> +/**
> + * intel_galileo_imr_init
> + *
> + * Tear down IMRs used during bootup. BIOS and Grub
> + * both setup IMRs around compressed kernel, initrd memory
> + * that need to be removed before the kernel hands out one
> + * of the IMR encased addresses to a downstream DMA agent
> + * such as the SD or Ethernet. IMRs on Galileo are setup to
> + * immediately reset the system on violation - so if you're
> + * running a root filesystem from SD - you'll need the IMRs
> + * torn down or you'll find seemingly random resets when using
> + * your filesystem.

Split this to Summary and Description.

> + */
> +static void __init intel_galileo_imr_init(void)
> +{
> +       unsigned long base  = virt_to_phys(&_text);
> +       unsigned long size = virt_to_phys(&_sinittext) - base - IMR_ALIGN;
> +       int i, ret;
> +
> +       /* Tear down all existing unlocked IMRs */
> +       for (i = 0; i <= QUARK_X1000_IMR_NUM; i++)

NUM or last one? Again confusing.

> +               imr_del(i, 0, 0);
> +
> +       /*
> +        * Setup an IMR around the physical extent of the kernel
> +        * Non-SMM mode core read/write from/to kernel physical region.
> +        * IMR locked.
> +        */
> +       ret = imr_add(base, size, IMR_NON_SMM, IMR_NON_SMM, true);
> +       if (ret)
> +               pr_err("Unable to setup IMR for kernel: (%p - %p)\n",
> +                       &_text, &__init_begin);
> +       else
> +               pr_info("IMR protect kernel memory: %ldk (%p - %p)\n",
> +                       size >> 10, &_text, &__init_begin);

10 is a magic number. What is for?

> +#ifdef DEBUG

Move this inside the function.

> +       intel_galileo_imr_sanity(base, size);
> +#endif
> +}
> +
> +/**
> + * intel_galileo_init
> + *
> + * Identify a Galileo Gen1 or Gen2. If found run code to sanitise the
> + * kernel memory space of IMRs that are inconsistent with the EFI memory map.

Split this to parts.

> + */
> +static int __init intel_galileo_init(void)
> +{
> +       int ret = 0, type = GALILEO_UNKNOWN;
> +       struct cpuinfo_x86 *c = &cpu_data(cpu);
> +       const struct dmi_system_id *system_id;
> +
> +       if (!cpu_is_quark(c))

x86_match_cpu() ?

> +               return -ENODEV;
> +
> +       system_id = dmi_first_match(galileo_baseboards);
> +
> +       /* BIOS releases 0.7.5 and 0.8.0 do not provide DMI strings */
> +       if (system_id != NULL) {
> +               type = (int)system_id->driver_data;
> +       } else {
> +               pr_info("Galileo Gen1 BIOS version <= 0.8.0\n");
> +               type = GALILEO_QRK_GEN1;
> +       }
> +
> +       switch (type) {
> +       case GALILEO_QRK_GEN1:
> +       case GALILEO_QRK_GEN2:
> +               intel_galileo_imr_init();
> +               break;
> +       default:
> +               ret = -ENODEV;

return -ENODEV; and remove ret variable.

> +       }
> +
> +       return ret;

return 0;

> +}
> +
> +static void __exit intel_galileo_exit(void)
> +{
> +}

Do we need empty exit function at all?

> +
> +module_init(intel_galileo_init);
> +module_exit(intel_galileo_exit);
> +
> +MODULE_AUTHOR("Bryan O'Donoghue <pure.logic@nexus-software.ie>");
> +MODULE_DESCRIPTION("Intel Galileo platform driver");
> +MODULE_LICENSE("GPL v2");
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
With Best Regards,
Andy Shevchenko