[v6] drivers/misc: Add Aspeed LPC control driver

In order to manage server systems, there is typically another processor
known as a BMC (Baseboard Management Controller) which is responsible
for powering the server and other various elements, sometimes fans,
often the system flash.

The Aspeed BMC family which is what is used on OpenPOWER machines and a
number of x86 as well is typically connected to the host via an LPC
(Low Pin Count) bus (among others).

The LPC bus is an ISA bus on steroids. It's generally used by the
BMC chip to provide the host with access to the system flash (via MEM/FW
cycles) that contains the BIOS or other host firmware along with a
number of SuperIO-style IOs (via IO space) such as UARTs, IPMI
controllers.

On the BMC chip side, this is all configured via a bunch of registers
whose content is related to a given policy of what devices are exposed
at a per system level, which is system/vendor specific, so we don't want
to bolt that into the BMC kernel. This started with a need to provide
something nicer than /dev/mem for user space to configure these things.

One important aspect of the configuration is how the MEM/FW space is
exposed to the host (ie, the x86 or POWER). Some registers in that
bridge can define a window remapping all or portion of the LPC MEM/FW
space to a portion of the BMC internal bus, with no specific limits
imposed in HW.

I think it makes sense to ensure that this window is configured by a
kernel driver that can apply some serious sanity checks on what it is
configured to map.

In practice, user space wants to control this by flipping the mapping
between essentially two types of portions of the BMC address space:

   - The flash space. This is a region of the BMC MMIO space that
more/less directly maps the system flash (at least for reads, writes
are somewhat more complicated).

   - One (or more) reserved area(s) of the BMC physical memory.

The latter is needed for a number of things, such as avoiding letting
the host manipulate the innards of the BMC flash controller via some
evil backdoor, we want to do flash updates by routing the window to a
portion of memory (under control of a mailbox protocol via some
separate set of registers) which the host can use to write new data in
bulk and then request the BMC to flash it. There are other uses, such
as allowing the host to boot from an in-memory flash image rather than
the one in flash (very handy for continuous integration and test, the
BMC can just download new images).

It is important to note that due to the way the Aspeed chip lets the
kernel configure the mapping between host LPC addresses and BMC ram
addresses the offset within the window must be a multiple of size.
Not doing so will fragment the accessible space rather than simply
moving 'zero' upwards. This is caused by the nature of HICR8 being a
mask and the way host LPC addresses are translated.

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
v2:
   Removed unused functions
   Removed use of access_ok()
   All input is evil
   Reworked the interface as per Benjamin Herrenschmidts vision
v3:
   Removed 'default y' from Kconfig
   Reordered ioctl() struct fields
   Reworeded some comments
v4:
   Reorder ioctl() struct fields (again)
v5:
   Style cleanups
   Use of_address_to_resource()
   Document ioctl structure fields
v6:
   Check and fail ioctl() if flags field non-zero

 drivers/misc/Kconfig                 |   8 ++
 drivers/misc/Makefile                |   1 +
 drivers/misc/aspeed-lpc-ctrl.c       | 267 +++++++++++++++++++++++++++++++++++
 include/uapi/linux/aspeed-lpc-ctrl.h |  60 ++++++++
 4 files changed, 336 insertions(+)
 create mode 100644 drivers/misc/aspeed-lpc-ctrl.c
 create mode 100644 include/uapi/linux/aspeed-lpc-ctrl.h

On Fri, Feb 17, 2017 at 1:58 PM, Cyril Bur <cyrilbur@gmail.com> wrote:
> In order to manage server systems, there is typically another processor
> known as a BMC (Baseboard Management Controller) which is responsible
> for powering the server and other various elements, sometimes fans,
> often the system flash.
>
> The Aspeed BMC family which is what is used on OpenPOWER machines and a
> number of x86 as well is typically connected to the host via an LPC
> (Low Pin Count) bus (among others).
>
> The LPC bus is an ISA bus on steroids. It's generally used by the
> BMC chip to provide the host with access to the system flash (via MEM/FW
> cycles) that contains the BIOS or other host firmware along with a
> number of SuperIO-style IOs (via IO space) such as UARTs, IPMI
> controllers.
>
> On the BMC chip side, this is all configured via a bunch of registers
> whose content is related to a given policy of what devices are exposed
> at a per system level, which is system/vendor specific, so we don't want
> to bolt that into the BMC kernel. This started with a need to provide
> something nicer than /dev/mem for user space to configure these things.
>
> One important aspect of the configuration is how the MEM/FW space is
> exposed to the host (ie, the x86 or POWER). Some registers in that
> bridge can define a window remapping all or portion of the LPC MEM/FW
> space to a portion of the BMC internal bus, with no specific limits
> imposed in HW.
>
> I think it makes sense to ensure that this window is configured by a
> kernel driver that can apply some serious sanity checks on what it is
> configured to map.
>
> In practice, user space wants to control this by flipping the mapping
> between essentially two types of portions of the BMC address space:
>
>    - The flash space. This is a region of the BMC MMIO space that
> more/less directly maps the system flash (at least for reads, writes
> are somewhat more complicated).
>
>    - One (or more) reserved area(s) of the BMC physical memory.
>
> The latter is needed for a number of things, such as avoiding letting
> the host manipulate the innards of the BMC flash controller via some
> evil backdoor, we want to do flash updates by routing the window to a
> portion of memory (under control of a mailbox protocol via some
> separate set of registers) which the host can use to write new data in
> bulk and then request the BMC to flash it. There are other uses, such
> as allowing the host to boot from an in-memory flash image rather than
> the one in flash (very handy for continuous integration and test, the
> BMC can just download new images).
>
> It is important to note that due to the way the Aspeed chip lets the
> kernel configure the mapping between host LPC addresses and BMC ram
> addresses the offset within the window must be a multiple of size.
> Not doing so will fragment the accessible space rather than simply
> moving 'zero' upwards. This is caused by the nature of HICR8 being a
> mask and the way host LPC addresses are translated.
>
> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> ---
> v2:
>    Removed unused functions
>    Removed use of access_ok()
>    All input is evil
>    Reworked the interface as per Benjamin Herrenschmidts vision
> v3:
>    Removed 'default y' from Kconfig
>    Reordered ioctl() struct fields
>    Reworeded some comments
> v4:
>    Reorder ioctl() struct fields (again)
> v5:
>    Style cleanups
>    Use of_address_to_resource()
>    Document ioctl structure fields
> v6:
>    Check and fail ioctl() if flags field non-zero

Woot!

Reviewed-by: Joel Stanley <joel@jms.id.au>

>
>  drivers/misc/Kconfig                 |   8 ++
>  drivers/misc/Makefile                |   1 +
>  drivers/misc/aspeed-lpc-ctrl.c       | 267 +++++++++++++++++++++++++++++++++++
>  include/uapi/linux/aspeed-lpc-ctrl.h |  60 ++++++++
>  4 files changed, 336 insertions(+)
>  create mode 100644 drivers/misc/aspeed-lpc-ctrl.c
>  create mode 100644 include/uapi/linux/aspeed-lpc-ctrl.h
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 64971baf11fa..7dc4c369012f 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -766,6 +766,14 @@ config PANEL_BOOT_MESSAGE
>           An empty message will only clear the display at driver init time. Any other
>           printf()-formatted message is valid with newline and escape codes.
>
> +config ASPEED_LPC_CTRL
> +       depends on (ARCH_ASPEED || COMPILE_TEST) && REGMAP && MFD_SYSCON
> +       tristate "Aspeed ast2400/2500 HOST LPC to BMC bridge control"
> +       ---help---
> +         Control Aspeed ast2400/2500 HOST LPC to BMC mappings through
> +         ioctl()s, the driver also provides a read/write interface to a BMC ram
> +         region where the host LPC read/write region can be buffered.
> +
>  source "drivers/misc/c2port/Kconfig"
>  source "drivers/misc/eeprom/Kconfig"
>  source "drivers/misc/cb710/Kconfig"
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 31983366090a..de1925a9c80b 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -53,6 +53,7 @@ obj-$(CONFIG_ECHO)            += echo/
>  obj-$(CONFIG_VEXPRESS_SYSCFG)  += vexpress-syscfg.o
>  obj-$(CONFIG_CXL_BASE)         += cxl/
>  obj-$(CONFIG_PANEL)             += panel.o
> +obj-$(CONFIG_ASPEED_LPC_CTRL)  += aspeed-lpc-ctrl.o
>
>  lkdtm-$(CONFIG_LKDTM)          += lkdtm_core.o
>  lkdtm-$(CONFIG_LKDTM)          += lkdtm_bugs.o
> diff --git a/drivers/misc/aspeed-lpc-ctrl.c b/drivers/misc/aspeed-lpc-ctrl.c
> new file mode 100644
> index 000000000000..f6acbe1d9378
> --- /dev/null
> +++ b/drivers/misc/aspeed-lpc-ctrl.c
> @@ -0,0 +1,267 @@
> +/*
> + * Copyright 2017 IBM Corporation
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#include <linux/mfd/syscon.h>
> +#include <linux/miscdevice.h>
> +#include <linux/mm.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/poll.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/aspeed-lpc-ctrl.h>
> +
> +#define DEVICE_NAME    "aspeed-lpc-ctrl"
> +
> +#define HICR7 0x8
> +#define HICR8 0xc
> +
> +struct aspeed_lpc_ctrl {
> +       struct miscdevice       miscdev;
> +       struct regmap           *regmap;
> +       phys_addr_t             mem_base;
> +       resource_size_t         mem_size;
> +       u32             pnor_size;
> +       u32             pnor_base;
> +};
> +
> +static struct aspeed_lpc_ctrl *file_aspeed_lpc_ctrl(struct file *file)
> +{
> +       return container_of(file->private_data, struct aspeed_lpc_ctrl,
> +                       miscdev);
> +}
> +
> +static int aspeed_lpc_ctrl_mmap(struct file *file, struct vm_area_struct *vma)
> +{
> +       struct aspeed_lpc_ctrl *lpc_ctrl = file_aspeed_lpc_ctrl(file);
> +       unsigned long vsize = vma->vm_end - vma->vm_start;
> +       pgprot_t prot = vma->vm_page_prot;
> +
> +       if (vma->vm_pgoff + vsize > lpc_ctrl->mem_base + lpc_ctrl->mem_size)
> +               return -EINVAL;
> +
> +       /* ast2400/2500 AHB accesses are not cache coherent */
> +       prot = pgprot_dmacoherent(prot);
> +
> +       if (remap_pfn_range(vma, vma->vm_start,
> +               (lpc_ctrl->mem_base >> PAGE_SHIFT) + vma->vm_pgoff,
> +               vsize, prot))
> +               return -EAGAIN;
> +
> +       return 0;
> +}
> +
> +static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned int cmd,
> +               unsigned long param)
> +{
> +       struct aspeed_lpc_ctrl *lpc_ctrl = file_aspeed_lpc_ctrl(file);
> +       void __user *p = (void __user *)param;
> +       struct aspeed_lpc_ctrl_mapping map;
> +       u32 addr;
> +       u32 size;
> +       long rc;
> +
> +       if (copy_from_user(&map, p, sizeof(map)))
> +               return -EFAULT;
> +
> +       if (map.flags != 0)
> +               return -EINVAL;
> +
> +       switch (cmd) {
> +       case ASPEED_LPC_CTRL_IOCTL_GET_SIZE:
> +               /* The flash windows don't report their size */
> +               if (map.window_type != ASPEED_LPC_CTRL_WINDOW_MEMORY)
> +                       return -EINVAL;
> +
> +               /* Support more than one window id in the future */
> +               if (map.window_id != 0)
> +                       return -EINVAL;
> +
> +               map.size = lpc_ctrl->mem_size;
> +
> +               return copy_to_user(p, &map, sizeof(map)) ? -EFAULT : 0;
> +       case ASPEED_LPC_CTRL_IOCTL_MAP:
> +
> +               /*
> +                * The top half of HICR7 is the MSB of the BMC address of the
> +                * mapping.
> +                * The bottom half of HICR7 is the MSB of the HOST LPC
> +                * firmware space address of the mapping.
> +                *
> +                * The 1 bits in the top of half of HICR8 represent the bits
> +                * (in the requested address) that should be ignored and
> +                * replaced with those from the top half of HICR7.
> +                * The 1 bits in the bottom half of HICR8 represent the bits
> +                * (in the requested address) that should be kept and pass
> +                * into the BMC address space.
> +                */
> +
> +               /*
> +                * It doesn't make sense to talk about a size or offset with
> +                * low 16 bits set. Both HICR7 and HICR8 talk about the top 16
> +                * bits of addresses and sizes.
> +                */
> +
> +               if ((map.size & 0x0000ffff) || (map.offset & 0x0000ffff))
> +                       return -EINVAL;
> +
> +               /*
> +                * Because of the way the masks work in HICR8 offset has to
> +                * be a multiple of size.
> +                */
> +               if (map.offset & (map.size - 1))
> +                       return -EINVAL;
> +
> +               if (map.window_type == ASPEED_LPC_CTRL_WINDOW_FLASH) {
> +                       addr = lpc_ctrl->pnor_base;
> +                       size = lpc_ctrl->pnor_size;
> +               } else if (map.window_type == ASPEED_LPC_CTRL_WINDOW_MEMORY) {
> +                       addr = lpc_ctrl->mem_base;
> +                       size = lpc_ctrl->mem_size;
> +               } else {
> +                       return -EINVAL;
> +               }
> +
> +               /* Check overflow first! */
> +               if (map.offset + map.size < map.offset ||
> +                       map.offset + map.size > size)
> +                       return -EINVAL;
> +
> +               if (map.size == 0 || map.size > size)
> +                       return -EINVAL;
> +
> +               addr += map.offset;
> +
> +               /*
> +                * addr (host lpc address) is safe regardless of values. This
> +                * simply changes the address the host has to request on its
> +                * side of the LPC bus. This cannot impact the hosts own
> +                * memory space by surprise as LPC specific accessors are
> +                * required. The only strange thing that could be done is
> +                * setting the lower 16 bits but the shift takes care of that.
> +                */
> +
> +               rc = regmap_write(lpc_ctrl->regmap, HICR7,
> +                               (addr | (map.addr >> 16)));
> +               if (rc)
> +                       return rc;
> +
> +               return regmap_write(lpc_ctrl->regmap, HICR8,
> +                       (~(map.size - 1)) | ((map.size >> 16) - 1));
> +       }
> +
> +       return -EINVAL;
> +}
> +
> +static const struct file_operations aspeed_lpc_ctrl_fops = {
> +       .owner          = THIS_MODULE,
> +       .mmap           = aspeed_lpc_ctrl_mmap,
> +       .unlocked_ioctl = aspeed_lpc_ctrl_ioctl,
> +};
> +
> +static int aspeed_lpc_ctrl_probe(struct platform_device *pdev)
> +{
> +       struct aspeed_lpc_ctrl *lpc_ctrl;
> +       struct device_node *node;
> +       struct resource resm;
> +       struct device *dev;
> +       int rc;
> +
> +       dev = &pdev->dev;
> +
> +       lpc_ctrl = devm_kzalloc(dev, sizeof(*lpc_ctrl), GFP_KERNEL);
> +       if (!lpc_ctrl)
> +               return -ENOMEM;
> +
> +       node = of_parse_phandle(dev->of_node, "flash", 0);
> +       if (!node) {
> +               dev_err(dev, "Didn't find host pnor flash node\n");
> +               return -ENODEV;
> +       }
> +
> +       rc = of_address_to_resource(node, 1, &resm);
> +       of_node_put(node);
> +       if (rc) {
> +               dev_err(dev, "Couldn't address to resource for flash\n");
> +               return rc;
> +       }
> +
> +       lpc_ctrl->pnor_size = resource_size(&resm);
> +       lpc_ctrl->pnor_base = resm.start;
> +
> +       dev_set_drvdata(&pdev->dev, lpc_ctrl);
> +
> +       node = of_parse_phandle(dev->of_node, "memory-region", 0);
> +       if (!node) {
> +               dev_err(dev, "Didn't find reserved memory\n");
> +               return -EINVAL;
> +       }
> +
> +       rc = of_address_to_resource(node, 0, &resm);
> +       of_node_put(node);
> +       if (rc) {
> +               dev_err(dev, "Couldn't address to resource for reserved memory\n");
> +               return -ENOMEM;
> +       }
> +
> +       lpc_ctrl->mem_size = resource_size(&resm);
> +       lpc_ctrl->mem_base = resm.start;
> +
> +       lpc_ctrl->regmap = syscon_node_to_regmap(
> +                       pdev->dev.parent->of_node);
> +       if (IS_ERR(lpc_ctrl->regmap)) {
> +               dev_err(dev, "Couldn't get regmap\n");
> +               return -ENODEV;
> +       }
> +
> +       lpc_ctrl->miscdev.minor = MISC_DYNAMIC_MINOR;
> +       lpc_ctrl->miscdev.name = DEVICE_NAME;
> +       lpc_ctrl->miscdev.fops = &aspeed_lpc_ctrl_fops;
> +       lpc_ctrl->miscdev.parent = dev;
> +       rc = misc_register(&lpc_ctrl->miscdev);
> +       if (rc)
> +               dev_err(dev, "Unable to register device\n");
> +       else
> +               dev_info(dev, "Loaded at 0x%08x (0x%08x)\n",
> +                       lpc_ctrl->mem_base, lpc_ctrl->mem_size);
> +
> +       return rc;
> +}
> +
> +static int aspeed_lpc_ctrl_remove(struct platform_device *pdev)
> +{
> +       struct aspeed_lpc_ctrl *lpc_ctrl = dev_get_drvdata(&pdev->dev);
> +
> +       misc_deregister(&lpc_ctrl->miscdev);
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id aspeed_lpc_ctrl_match[] = {
> +       { .compatible = "aspeed,ast2400-lpc-ctrl" },
> +       { .compatible = "aspeed,ast2500-lpc-ctrl" },
> +       { },
> +};
> +
> +static struct platform_driver aspeed_lpc_ctrl_driver = {
> +       .driver = {
> +               .name           = DEVICE_NAME,
> +               .of_match_table = aspeed_lpc_ctrl_match,
> +       },
> +       .probe = aspeed_lpc_ctrl_probe,
> +       .remove = aspeed_lpc_ctrl_remove,
> +};
> +
> +module_platform_driver(aspeed_lpc_ctrl_driver);
> +
> +MODULE_DEVICE_TABLE(of, aspeed_lpc_ctrl_match);
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Cyril Bur <cyrilbur@gmail.com>");
> +MODULE_DESCRIPTION("Control for aspeed 2400/2500 LPC HOST to BMC mappings");
> diff --git a/include/uapi/linux/aspeed-lpc-ctrl.h b/include/uapi/linux/aspeed-lpc-ctrl.h
> new file mode 100644
> index 000000000000..f96fa995a3f0
> --- /dev/null
> +++ b/include/uapi/linux/aspeed-lpc-ctrl.h
> @@ -0,0 +1,60 @@
> +/*
> + * Copyright 2017 IBM Corp.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#ifndef _UAPI_LINUX_ASPEED_LPC_CTRL_H
> +#define _UAPI_LINUX_ASPEED_LPC_CTRL_H
> +
> +#include <linux/ioctl.h>
> +
> +/* Window types */
> +#define ASPEED_LPC_CTRL_WINDOW_FLASH   1
> +#define ASPEED_LPC_CTRL_WINDOW_MEMORY  2
> +
> +/*
> + * This driver provides a window for the host to access a BMC resource
> + * across the BMC <-> Host LPC bus.
> + *
> + * window_type: The BMC resource that the host will access through the
> + * window. BMC flash and BMC RAM.
> + *
> + * window_id: For each window type there may be multiple windows,
> + * these are referenced by ID.
> + *
> + * flags: Reserved for future use, this field is expected to be
> + * zeroed.
> + *
> + * addr: Address on the host LPC bus that the specified window should
> + * be mapped. This address must be power of two aligned.
> + *
> + * offset: Offset into the BMC window that should be mapped to the
> + * host (at addr). This must be a multiple of size.
> + *
> + * size: The size of the mapping. The smallest possible size is 64K.
> + * This must be power of two aligned.
> + *
> + */
> +
> +struct aspeed_lpc_ctrl_mapping {
> +       __u8    window_type;
> +       __u8    window_id;
> +       __u16   flags;
> +       __u32   addr;
> +       __u32   offset;
> +       __u32   size;
> +};
> +
> +#define __ASPEED_LPC_CTRL_IOCTL_MAGIC  0xb2
> +
> +#define ASPEED_LPC_CTRL_IOCTL_GET_SIZE _IOWR(__ASPEED_LPC_CTRL_IOCTL_MAGIC, \
> +               0x00, struct aspeed_lpc_ctrl_mapping)
> +
> +#define ASPEED_LPC_CTRL_IOCTL_MAP      _IOW(__ASPEED_LPC_CTRL_IOCTL_MAGIC, \
> +               0x01, struct aspeed_lpc_ctrl_mapping)
> +
> +#endif /* _UAPI_LINUX_ASPEED_LPC_CTRL_H */
> --
> 2.11.1
>

On Fri, 2017-02-17 at 14:28 +1100, Cyril Bur wrote:
> In order to manage server systems, there is typically another
> processor
> known as a BMC (Baseboard Management Controller) which is responsible
> for powering the server and other various elements, sometimes fans,
> often the system flash.

 .../...

> It is important to note that due to the way the Aspeed chip lets the
> kernel configure the mapping between host LPC addresses and BMC ram
> addresses the offset within the window must be a multiple of size.
> Not doing so will fragment the accessible space rather than simply
> moving 'zero' upwards. This is caused by the nature of HICR8 being a
> mask and the way host LPC addresses are translated.
> 
> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>

Reviewed-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Greg, quick Q for you:

We will need to also add a mechanism to poke at a few registers
in the same LPC controllers from userspace.

Mostly those are "scratch" registers that the BMC is supposed to set to
specific values to indicates features it supports etc... (or to enable 
the host BIOS to run in some kind of verbose debug mode).

Think of it as a communication channel between the BMC and the BIOS
running on the host.

The BMC userspace needs to set them to various platform specific
values as appropriate for a given vendor/system (userspace policy
from the BMC perspective) during boot and/or change them as the result
of some user actions (enable debug mode) etc...

The question here is read/write or sysfs files attached to the
sysfs node ?

The former means userspace needs to "seek" to the right magic
offset to write to one of them which is non-trivial to do from
simple shell scripts but is a more natural interface.

It also means the owner/permissions of the /dev entry as set
by uboot apply which can be nice.

The latter can expose each register by its name which provides
a nice way to echo in/out of them from a shell script.

The drawback is that pretty much restricts access to root.

What do you recommend ?

Cheers,
Ben.

On Fri, 2017-02-17 at 14:28 +1100, Cyril Bur wrote:

I may be too late, but see below...
> In order to manage server systems, there is typically another
> processor
> known as a BMC (Baseboard Management Controller) which is responsible
> for powering the server and other various elements, sometimes fans,
> often the system flash.
> 
> The Aspeed BMC family which is what is used on OpenPOWER machines and
> a
> number of x86 as well is typically connected to the host via an LPC
> (Low Pin Count) bus (among others).
> 
> The LPC bus is an ISA bus on steroids. It's generally used by the
> BMC chip to provide the host with access to the system flash (via
> MEM/FW
> cycles) that contains the BIOS or other host firmware along with a
> number of SuperIO-style IOs (via IO space) such as UARTs, IPMI
> controllers.
> 
> On the BMC chip side, this is all configured via a bunch of registers
> whose content is related to a given policy of what devices are
> exposed
> at a per system level, which is system/vendor specific, so we don't
> want
> to bolt that into the BMC kernel. This started with a need to provide
> something nicer than /dev/mem for user space to configure these
> things.
> 
> One important aspect of the configuration is how the MEM/FW space is
> exposed to the host (ie, the x86 or POWER). Some registers in that
> bridge can define a window remapping all or portion of the LPC MEM/FW
> space to a portion of the BMC internal bus, with no specific limits
> imposed in HW.
> 
> I think it makes sense to ensure that this window is configured by a
> kernel driver that can apply some serious sanity checks on what it is
> configured to map.
> 
> In practice, user space wants to control this by flipping the mapping
> between essentially two types of portions of the BMC address space:
> 
>    - The flash space. This is a region of the BMC MMIO space that
> more/less directly maps the system flash (at least for reads, writes
> are somewhat more complicated).
> 
>    - One (or more) reserved area(s) of the BMC physical memory.
> 
> The latter is needed for a number of things, such as avoiding letting
> the host manipulate the innards of the BMC flash controller via some
> evil backdoor, we want to do flash updates by routing the window to a
> portion of memory (under control of a mailbox protocol via some
> separate set of registers) which the host can use to write new data
> in
> bulk and then request the BMC to flash it. There are other uses, such
> as allowing the host to boot from an in-memory flash image rather
> than
> the one in flash (very handy for continuous integration and test, the
> BMC can just download new images).
> 
> It is important to note that due to the way the Aspeed chip lets the
> kernel configure the mapping between host LPC addresses and BMC ram
> addresses the offset within the window must be a multiple of size.
> Not doing so will fragment the accessible space rather than simply
> moving 'zero' upwards. This is caused by the nature of HICR8 being a
> mask and the way host LPC addresses are translated.
> 
> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> ---
> v2:
>    Removed unused functions
>    Removed use of access_ok()
>    All input is evil
>    Reworked the interface as per Benjamin Herrenschmidts vision
> v3:
>    Removed 'default y' from Kconfig
>    Reordered ioctl() struct fields
>    Reworeded some comments
> v4:
>    Reorder ioctl() struct fields (again)
> v5:
>    Style cleanups
>    Use of_address_to_resource()
>    Document ioctl structure fields
> v6:
>    Check and fail ioctl() if flags field non-zero
> 
>  drivers/misc/Kconfig                 |   8 ++
>  drivers/misc/Makefile                |   1 +
>  drivers/misc/aspeed-lpc-ctrl.c       | 267
> +++++++++++++++++++++++++++++++++++
>  include/uapi/linux/aspeed-lpc-ctrl.h |  60 ++++++++
>  4 files changed, 336 insertions(+)
>  create mode 100644 drivers/misc/aspeed-lpc-ctrl.c
>  create mode 100644 include/uapi/linux/aspeed-lpc-ctrl.h
> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 64971baf11fa..7dc4c369012f 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -766,6 +766,14 @@ config PANEL_BOOT_MESSAGE
>  	  An empty message will only clear the display at driver
> init time. Any other
>  	  printf()-formatted message is valid with newline and
> escape codes.
>  
> +config ASPEED_LPC_CTRL
> +	depends on (ARCH_ASPEED || COMPILE_TEST) && REGMAP &&
> MFD_SYSCON
> +	tristate "Aspeed ast2400/2500 HOST LPC to BMC bridge
> control"
> +	---help---
> +	  Control Aspeed ast2400/2500 HOST LPC to BMC mappings
> through
> +	  ioctl()s, the driver also provides a read/write interface
> to a BMC ram
> +	  region where the host LPC read/write region can be
> buffered.
> +
>  source "drivers/misc/c2port/Kconfig"
>  source "drivers/misc/eeprom/Kconfig"
>  source "drivers/misc/cb710/Kconfig"
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 31983366090a..de1925a9c80b 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -53,6 +53,7 @@ obj-$(CONFIG_ECHO)		+= echo/
>  obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
>  obj-$(CONFIG_CXL_BASE)		+= cxl/
>  obj-$(CONFIG_PANEL)             += panel.o
> +obj-$(CONFIG_ASPEED_LPC_CTRL)	+= aspeed-lpc-ctrl.o
>  
>  lkdtm-$(CONFIG_LKDTM)		+= lkdtm_core.o
>  lkdtm-$(CONFIG_LKDTM)		+= lkdtm_bugs.o
> diff --git a/drivers/misc/aspeed-lpc-ctrl.c b/drivers/misc/aspeed-
> lpc-ctrl.c
> new file mode 100644
> index 000000000000..f6acbe1d9378
> --- /dev/null
> +++ b/drivers/misc/aspeed-lpc-ctrl.c
> @@ -0,0 +1,267 @@
> +/*
> + * Copyright 2017 IBM Corporation
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#include <linux/mfd/syscon.h>
> +#include <linux/miscdevice.h>
> +#include <linux/mm.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/poll.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/aspeed-lpc-ctrl.h>
> +
> +#define DEVICE_NAME	"aspeed-lpc-ctrl"
> +
> +#define HICR7 0x8
> +#define HICR8 0xc
> +
> +struct aspeed_lpc_ctrl {
> +	struct miscdevice	miscdev;
> +	struct regmap		*regmap;
> +	phys_addr_t		mem_base;
> +	resource_size_t		mem_size;
> +	u32		pnor_size;
> +	u32		pnor_base;
> +};
> +
> +static struct aspeed_lpc_ctrl *file_aspeed_lpc_ctrl(struct file
> *file)
> +{
> +	return container_of(file->private_data, struct
> aspeed_lpc_ctrl,
> +			miscdev);
> +}
> +
> +static int aspeed_lpc_ctrl_mmap(struct file *file, struct
> vm_area_struct *vma)
> +{
> +	struct aspeed_lpc_ctrl *lpc_ctrl =
> file_aspeed_lpc_ctrl(file);
> +	unsigned long vsize = vma->vm_end - vma->vm_start;
> +	pgprot_t prot = vma->vm_page_prot;
> +
> +	if (vma->vm_pgoff + vsize > lpc_ctrl->mem_base + lpc_ctrl-
> >mem_size)
> +		return -EINVAL;
> +
> +	/* ast2400/2500 AHB accesses are not cache coherent */
> +	prot = pgprot_dmacoherent(prot);
> +
> +	if (remap_pfn_range(vma, vma->vm_start,
> +		(lpc_ctrl->mem_base >> PAGE_SHIFT) + vma->vm_pgoff,
> +		vsize, prot))
> +		return -EAGAIN;
> +
> +	return 0;
> +}
> +
> +static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned int
> cmd,
> +		unsigned long param)
> +{
> +	struct aspeed_lpc_ctrl *lpc_ctrl =
> file_aspeed_lpc_ctrl(file);
> +	void __user *p = (void __user *)param;
> +	struct aspeed_lpc_ctrl_mapping map;
> +	u32 addr;
> +	u32 size;
> +	long rc;
> +
> +	if (copy_from_user(&map, p, sizeof(map)))
> +		return -EFAULT;
> +
> +	if (map.flags != 0)
> +		return -EINVAL;
> +
> +	switch (cmd) {
> +	case ASPEED_LPC_CTRL_IOCTL_GET_SIZE:
> +		/* The flash windows don't report their size */
> +		if (map.window_type !=
> ASPEED_LPC_CTRL_WINDOW_MEMORY)
> +			return -EINVAL;
> +
> +		/* Support more than one window id in the future */
> +		if (map.window_id != 0)
> +			return -EINVAL;
> +
> +		map.size = lpc_ctrl->mem_size;
> +
> +		return copy_to_user(p, &map, sizeof(map)) ? -EFAULT
> : 0;
> +	case ASPEED_LPC_CTRL_IOCTL_MAP:
> +
> +		/*
> +		 * The top half of HICR7 is the MSB of the BMC
> address of the
> +		 * mapping.
How about: HICR7[31:16] is the [offset into BMC memory/most significant
two bytes fo the BMC address] of the mapping.
> +		 * The bottom half of HICR7 is
> the MSB of the HOST LPC
HICR7[15:0] is the [offset on the HOST LPC firmware address space/most
fisnificant two bytes of the HOST LPC firmware address space] of the
mapping
> +		 * firmware space address of the mapping.
> +		 *
> +		 * The 1 bits in the top of half of HICR8 represent
> the bits
> +		 * (in the requested address) that should be ignored
> and
> +		 * replaced with those from the top half of HICR7.
HICR8[31:16] represents a mask of the bits which should be taken from
HICR7[31:16] and placed in the BMC address.
> +		 * The 1 bits in the bottom half of HICR8 represent
> the bits
> +		 * (in the requested address) that should be kept
> and pass
> +		 * into the BMC address space.
HICR8[15:0] represents a mask of the bits which should be taken from
LPC_ADDR[31:16] and placed in the BMC address.

If I understand correctly, we basically end up with:

BMC_ADDR[31:0] = (BMC_OFFSET + LPC_OFFSET + INDEX) << 16 + OFFSET
where:
BMC_OFFSET = HICR7[31:16] & HICR8[31:16] - The offset into bmc memory
of where the window is.
LPC_OFFSET = LPC_ADDR[31:16] - HICR7[15:0] - The offset on the lpc bus
of where we created the window.
INDEX = LPC_ADDR[31:16] & HICR8[15:0] - High bits of the lpc address
which we use in the bmc address.
OFFSET = LPC_ADDR[15:0] - Low bits of the lpc address which we use in
the bmc address.
> +		 */
> +
> +		/*
> +		 * It doesn't make sense to talk about a size or
> offset with
> +		 * low 16 bits set. Both HICR7 and HICR8 talk about
> the top 16
> +		 * bits of addresses and sizes.
> +		 */
> +
> +		if ((map.size & 0x0000ffff) || (map.offset &
> 0x0000ffff))
> +			return -EINVAL;
Am I missing something? Are you checking anywhere that map.size is
power-of-2 aligned?
> +
> +		/*
> +		 * Because of the way the masks work in HICR8 offset
> has to
> +		 * be a multiple of size.
> +		 */
> +		if (map.offset & (map.size - 1))
> +			return -EINVAL;
> +
> +		if (map.window_type == ASPEED_LPC_CTRL_WINDOW_FLASH)
> {
> +			addr = lpc_ctrl->pnor_base;
> +			size = lpc_ctrl->pnor_size;
> +		} else if (map.window_type ==
> ASPEED_LPC_CTRL_WINDOW_MEMORY) {
> +			addr = lpc_ctrl->mem_base;
> +			size = lpc_ctrl->mem_size;
> +		} else {
> +			return -EINVAL;
> +		}
> +
> +		/* Check overflow first! */
> +		if (map.offset + map.size < map.offset ||
> +			map.offset + map.size > size)
> +			return -EINVAL;
> +
> +		if (map.size == 0 || map.size > size)
> +			return -EINVAL;
> +
> +		addr += map.offset;
> +
> +		/*
> +		 * addr (host lpc address) is safe regardless of
> values. This
> +		 * simply changes the address the host has to
> request on its
> +		 * side of the LPC bus. This cannot impact the hosts
> own
> +		 * memory space by surprise as LPC specific
> accessors are
> +		 * required. The only strange thing that could be
> done is
> +		 * setting the lower 16 bits but the shift takes
> care of that.
> +		 */
Don't you have to explicitly check that the bits 15:0 of map.addr
aren't set? Otherwise something could ask to map LPC address 0x1234
(map.addr = 0x1234), you will set HICR7[15:0] = 0 (map.addr >> 16). So
when address 0x1234 is accessed on the lpc bus, this will map to bmc
address 0x1234 instead of the expected 0x0 (since this is the base lpc
address)?

Or do we preclude the low 16 bits of map.addr from being set somewhere
else?
> +
> +		rc = regmap_write(lpc_ctrl->regmap, HICR7,
> +				(addr | (map.addr >> 16)));
> +		if (rc)
> +			return rc;
> +
> +		return regmap_write(lpc_ctrl->regmap, HICR8,
> +			(~(map.size - 1)) | ((map.size >> 16) - 1));
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static const struct file_operations aspeed_lpc_ctrl_fops = {
> +	.owner		= THIS_MODULE,
> +	.mmap		= aspeed_lpc_ctrl_mmap,
> +	.unlocked_ioctl	= aspeed_lpc_ctrl_ioctl,
> +};
> +
> +static int aspeed_lpc_ctrl_probe(struct platform_device *pdev)
> +{
> +	struct aspeed_lpc_ctrl *lpc_ctrl;
> +	struct device_node *node;
> +	struct resource resm;
> +	struct device *dev;
> +	int rc;
> +
> +	dev = &pdev->dev;
> +
> +	lpc_ctrl = devm_kzalloc(dev, sizeof(*lpc_ctrl), GFP_KERNEL);
> +	if (!lpc_ctrl)
> +		return -ENOMEM;
> +
> +	node = of_parse_phandle(dev->of_node, "flash", 0);
> +	if (!node) {
> +		dev_err(dev, "Didn't find host pnor flash node\n");
> +		return -ENODEV;
> +	}
> +
> +	rc = of_address_to_resource(node, 1, &resm);
> +	of_node_put(node);
> +	if (rc) {
> +		dev_err(dev, "Couldn't address to resource for
> flash\n");
> +		return rc;
> +	}
> +
> +	lpc_ctrl->pnor_size = resource_size(&resm);
> +	lpc_ctrl->pnor_base = resm.start;
> +
> +	dev_set_drvdata(&pdev->dev, lpc_ctrl);
> +
> +	node = of_parse_phandle(dev->of_node, "memory-region", 0);
> +	if (!node) {
> +		dev_err(dev, "Didn't find reserved memory\n");
> +		return -EINVAL;
> +	}
> +
> +	rc = of_address_to_resource(node, 0, &resm);
> +	of_node_put(node);
> +	if (rc) {
> +		dev_err(dev, "Couldn't address to resource for
> reserved memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	lpc_ctrl->mem_size = resource_size(&resm);
> +	lpc_ctrl->mem_base = resm.start;
> +
> +	lpc_ctrl->regmap = syscon_node_to_regmap(
> +			pdev->dev.parent->of_node);
> +	if (IS_ERR(lpc_ctrl->regmap)) {
> +		dev_err(dev, "Couldn't get regmap\n");
> +		return -ENODEV;
> +	}
> +
> +	lpc_ctrl->miscdev.minor = MISC_DYNAMIC_MINOR;
> +	lpc_ctrl->miscdev.name = DEVICE_NAME;
> +	lpc_ctrl->miscdev.fops = &aspeed_lpc_ctrl_fops;
> +	lpc_ctrl->miscdev.parent = dev;
> +	rc = misc_register(&lpc_ctrl->miscdev);
> +	if (rc)
> +		dev_err(dev, "Unable to register device\n");
> +	else
> +		dev_info(dev, "Loaded at 0x%08x (0x%08x)\n",
> +			lpc_ctrl->mem_base, lpc_ctrl->mem_size);
> +
> +	return rc;
> +}
> +
> +static int aspeed_lpc_ctrl_remove(struct platform_device *pdev)
> +{
> +	struct aspeed_lpc_ctrl *lpc_ctrl = dev_get_drvdata(&pdev-
> >dev);
> +
> +	misc_deregister(&lpc_ctrl->miscdev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id aspeed_lpc_ctrl_match[] = {
> +	{ .compatible = "aspeed,ast2400-lpc-ctrl" },
> +	{ .compatible = "aspeed,ast2500-lpc-ctrl" },
> +	{ },
> +};
> +
> +static struct platform_driver aspeed_lpc_ctrl_driver = {
> +	.driver = {
> +		.name		= DEVICE_NAME,
> +		.of_match_table = aspeed_lpc_ctrl_match,
> +	},
> +	.probe = aspeed_lpc_ctrl_probe,
> +	.remove = aspeed_lpc_ctrl_remove,
> +};
> +
> +module_platform_driver(aspeed_lpc_ctrl_driver);
> +
> +MODULE_DEVICE_TABLE(of, aspeed_lpc_ctrl_match);
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Cyril Bur <cyrilbur@gmail.com>");
> +MODULE_DESCRIPTION("Control for aspeed 2400/2500 LPC HOST to BMC
> mappings");
> diff --git a/include/uapi/linux/aspeed-lpc-ctrl.h
> b/include/uapi/linux/aspeed-lpc-ctrl.h
> new file mode 100644
> index 000000000000..f96fa995a3f0
> --- /dev/null
> +++ b/include/uapi/linux/aspeed-lpc-ctrl.h
> @@ -0,0 +1,60 @@
> +/*
> + * Copyright 2017 IBM Corp.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#ifndef _UAPI_LINUX_ASPEED_LPC_CTRL_H
> +#define _UAPI_LINUX_ASPEED_LPC_CTRL_H
> +
> +#include <linux/ioctl.h>
> +
> +/* Window types */
> +#define ASPEED_LPC_CTRL_WINDOW_FLASH	1
> +#define ASPEED_LPC_CTRL_WINDOW_MEMORY	2
> +
> +/*
> + * This driver provides a window for the host to access a BMC
> resource
> + * across the BMC <-> Host LPC bus.
> + *
> + * window_type: The BMC resource that the host will access through
> the
> + * window. BMC flash and BMC RAM.
> + *
> + * window_id: For each window type there may be multiple windows,
> + * these are referenced by ID.
> + *
> + * flags: Reserved for future use, this field is expected to be
> + * zeroed.
> + *
> + * addr: Address on the host LPC bus that the specified window
> should
> + * be mapped. This address must be power of two aligned.
If I understand correctly -> AND a multiple of size?
> + *
> + * offset: Offset into the BMC window that should be mapped to the
> + * host (at addr). This must be a multiple of size.
> + *
> + * size: The size of the mapping. The smallest possible size is 64K.
> + * This must be power of two aligned.
> + *
> + */
> +
> +struct aspeed_lpc_ctrl_mapping {
> +	__u8	window_type;
> +	__u8	window_id;
> +	__u16	flags;
> +	__u32	addr;
> +	__u32	offset;
> +	__u32	size;
> +};
> +
> +#define __ASPEED_LPC_CTRL_IOCTL_MAGIC	0xb2
> +
> +#define ASPEED_LPC_CTRL_IOCTL_GET_SIZE	_IOWR(__ASPEED_LPC_CTR
> L_IOCTL_MAGIC, \
> +		0x00, struct aspeed_lpc_ctrl_mapping)
> +
> +#define ASPEED_LPC_CTRL_IOCTL_MAP	_IOW(__ASPEED_LPC_CTRL_IOCT
> L_MAGIC, \
> +		0x01, struct aspeed_lpc_ctrl_mapping)
> +
> +#endif /* _UAPI_LINUX_ASPEED_LPC_CTRL_H */

On Wed, 2017-02-22 at 16:11 +1100, Suraj Jitindar Singh wrote:
> On Fri, 2017-02-17 at 14:28 +1100, Cyril Bur wrote:
> 
> I may be too late, but see below...
> > In order to manage server systems, there is typically another
> > processor
> > known as a BMC (Baseboard Management Controller) which is responsible
> > for powering the server and other various elements, sometimes fans,
> > often the system flash.
> > 
> > The Aspeed BMC family which is what is used on OpenPOWER machines and
> > a
> > number of x86 as well is typically connected to the host via an LPC
> > (Low Pin Count) bus (among others).
> > 
> > The LPC bus is an ISA bus on steroids. It's generally used by the
> > BMC chip to provide the host with access to the system flash (via
> > MEM/FW
> > cycles) that contains the BIOS or other host firmware along with a
> > number of SuperIO-style IOs (via IO space) such as UARTs, IPMI
> > controllers.
> > 
> > On the BMC chip side, this is all configured via a bunch of registers
> > whose content is related to a given policy of what devices are
> > exposed
> > at a per system level, which is system/vendor specific, so we don't
> > want
> > to bolt that into the BMC kernel. This started with a need to provide
> > something nicer than /dev/mem for user space to configure these
> > things.
> > 
> > One important aspect of the configuration is how the MEM/FW space is
> > exposed to the host (ie, the x86 or POWER). Some registers in that
> > bridge can define a window remapping all or portion of the LPC MEM/FW
> > space to a portion of the BMC internal bus, with no specific limits
> > imposed in HW.
> > 
> > I think it makes sense to ensure that this window is configured by a
> > kernel driver that can apply some serious sanity checks on what it is
> > configured to map.
> > 
> > In practice, user space wants to control this by flipping the mapping
> > between essentially two types of portions of the BMC address space:
> > 
> >    - The flash space. This is a region of the BMC MMIO space that
> > more/less directly maps the system flash (at least for reads, writes
> > are somewhat more complicated).
> > 
> >    - One (or more) reserved area(s) of the BMC physical memory.
> > 
> > The latter is needed for a number of things, such as avoiding letting
> > the host manipulate the innards of the BMC flash controller via some
> > evil backdoor, we want to do flash updates by routing the window to a
> > portion of memory (under control of a mailbox protocol via some
> > separate set of registers) which the host can use to write new data
> > in
> > bulk and then request the BMC to flash it. There are other uses, such
> > as allowing the host to boot from an in-memory flash image rather
> > than
> > the one in flash (very handy for continuous integration and test, the
> > BMC can just download new images).
> > 
> > It is important to note that due to the way the Aspeed chip lets the
> > kernel configure the mapping between host LPC addresses and BMC ram
> > addresses the offset within the window must be a multiple of size.
> > Not doing so will fragment the accessible space rather than simply
> > moving 'zero' upwards. This is caused by the nature of HICR8 being a
> > mask and the way host LPC addresses are translated.
> > 
> > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> > ---
> > v2:
> >    Removed unused functions
> >    Removed use of access_ok()
> >    All input is evil
> >    Reworked the interface as per Benjamin Herrenschmidts vision
> > v3:
> >    Removed 'default y' from Kconfig
> >    Reordered ioctl() struct fields
> >    Reworeded some comments
> > v4:
> >    Reorder ioctl() struct fields (again)
> > v5:
> >    Style cleanups
> >    Use of_address_to_resource()
> >    Document ioctl structure fields
> > v6:
> >    Check and fail ioctl() if flags field non-zero
> > 
> >  drivers/misc/Kconfig                 |   8 ++
> >  drivers/misc/Makefile                |   1 +
> >  drivers/misc/aspeed-lpc-ctrl.c       | 267
> > +++++++++++++++++++++++++++++++++++
> >  include/uapi/linux/aspeed-lpc-ctrl.h |  60 ++++++++
> >  4 files changed, 336 insertions(+)
> >  create mode 100644 drivers/misc/aspeed-lpc-ctrl.c
> >  create mode 100644 include/uapi/linux/aspeed-lpc-ctrl.h
> > 
> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> > index 64971baf11fa..7dc4c369012f 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -766,6 +766,14 @@ config PANEL_BOOT_MESSAGE
> >  	  An empty message will only clear the display at driver
> > init time. Any other
> >  	  printf()-formatted message is valid with newline and
> > escape codes.
> >  
> > +config ASPEED_LPC_CTRL
> > +	depends on (ARCH_ASPEED || COMPILE_TEST) && REGMAP &&
> > MFD_SYSCON
> > +	tristate "Aspeed ast2400/2500 HOST LPC to BMC bridge
> > control"
> > +	---help---
> > +	  Control Aspeed ast2400/2500 HOST LPC to BMC mappings
> > through
> > +	  ioctl()s, the driver also provides a read/write interface
> > to a BMC ram
> > +	  region where the host LPC read/write region can be
> > buffered.
> > +
> >  source "drivers/misc/c2port/Kconfig"
> >  source "drivers/misc/eeprom/Kconfig"
> >  source "drivers/misc/cb710/Kconfig"
> > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> > index 31983366090a..de1925a9c80b 100644
> > --- a/drivers/misc/Makefile
> > +++ b/drivers/misc/Makefile
> > @@ -53,6 +53,7 @@ obj-$(CONFIG_ECHO)		+= echo/
> >  obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
> >  obj-$(CONFIG_CXL_BASE)		+= cxl/
> >  obj-$(CONFIG_PANEL)             += panel.o
> > +obj-$(CONFIG_ASPEED_LPC_CTRL)	+= aspeed-lpc-ctrl.o
> >  
> >  lkdtm-$(CONFIG_LKDTM)		+= lkdtm_core.o
> >  lkdtm-$(CONFIG_LKDTM)		+= lkdtm_bugs.o
> > diff --git a/drivers/misc/aspeed-lpc-ctrl.c b/drivers/misc/aspeed-
> > lpc-ctrl.c
> > new file mode 100644
> > index 000000000000..f6acbe1d9378
> > --- /dev/null
> > +++ b/drivers/misc/aspeed-lpc-ctrl.c
> > @@ -0,0 +1,267 @@
> > +/*
> > + * Copyright 2017 IBM Corporation
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version
> > + * 2 of the License, or (at your option) any later version.
> > + */
> > +
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/miscdevice.h>
> > +#include <linux/mm.h>
> > +#include <linux/module.h>
> > +#include <linux/of_address.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/poll.h>
> > +#include <linux/regmap.h>
> > +
> > +#include <linux/aspeed-lpc-ctrl.h>
> > +
> > +#define DEVICE_NAME	"aspeed-lpc-ctrl"
> > +
> > +#define HICR7 0x8
> > +#define HICR8 0xc
> > +
> > +struct aspeed_lpc_ctrl {
> > +	struct miscdevice	miscdev;
> > +	struct regmap		*regmap;
> > +	phys_addr_t		mem_base;
> > +	resource_size_t		mem_size;
> > +	u32		pnor_size;
> > +	u32		pnor_base;
> > +};
> > +
> > +static struct aspeed_lpc_ctrl *file_aspeed_lpc_ctrl(struct file
> > *file)
> > +{
> > +	return container_of(file->private_data, struct
> > aspeed_lpc_ctrl,
> > +			miscdev);
> > +}
> > +
> > +static int aspeed_lpc_ctrl_mmap(struct file *file, struct
> > vm_area_struct *vma)
> > +{
> > +	struct aspeed_lpc_ctrl *lpc_ctrl =
> > file_aspeed_lpc_ctrl(file);
> > +	unsigned long vsize = vma->vm_end - vma->vm_start;
> > +	pgprot_t prot = vma->vm_page_prot;
> > +
> > +	if (vma->vm_pgoff + vsize > lpc_ctrl->mem_base + lpc_ctrl-
> > > mem_size)
> > 
> > +		return -EINVAL;
> > +
> > +	/* ast2400/2500 AHB accesses are not cache coherent */
> > +	prot = pgprot_dmacoherent(prot);
> > +
> > +	if (remap_pfn_range(vma, vma->vm_start,
> > +		(lpc_ctrl->mem_base >> PAGE_SHIFT) + vma->vm_pgoff,
> > +		vsize, prot))
> > +		return -EAGAIN;
> > +
> > +	return 0;
> > +}
> > +
> > +static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned int
> > cmd,
> > +		unsigned long param)
> > +{
> > +	struct aspeed_lpc_ctrl *lpc_ctrl =
> > file_aspeed_lpc_ctrl(file);
> > +	void __user *p = (void __user *)param;
> > +	struct aspeed_lpc_ctrl_mapping map;
> > +	u32 addr;
> > +	u32 size;
> > +	long rc;
> > +
> > +	if (copy_from_user(&map, p, sizeof(map)))
> > +		return -EFAULT;
> > +
> > +	if (map.flags != 0)
> > +		return -EINVAL;
> > +
> > +	switch (cmd) {
> > +	case ASPEED_LPC_CTRL_IOCTL_GET_SIZE:
> > +		/* The flash windows don't report their size */
> > +		if (map.window_type !=
> > ASPEED_LPC_CTRL_WINDOW_MEMORY)
> > +			return -EINVAL;
> > +
> > +		/* Support more than one window id in the future */
> > +		if (map.window_id != 0)
> > +			return -EINVAL;
> > +
> > +		map.size = lpc_ctrl->mem_size;
> > +
> > +		return copy_to_user(p, &map, sizeof(map)) ? -EFAULT
> > : 0;
> > +	case ASPEED_LPC_CTRL_IOCTL_MAP:
> > +
> > +		/*
> > +		 * The top half of HICR7 is the MSB of the BMC
> > address of the
> > +		 * mapping.
> 

Hi Suraj,

I'm all for writing this out as many times as possible to be sure
everyone understands it. I think a visual diagram would be fantastic,
I'd love to do one in my infinite spare time. It may be useful to link
to Benhs comments in skiboot for another wording of how these two
registers work skiboot/hw/ast-bmc/ast-io.c.

> How about: HICR7[31:16] is the [offset into BMC memory/most significant
> two bytes fo the BMC address] of the mapping.
> > +		 * The bottom half of HICR7 is
> > the MSB of the HOST LPC
> 
> HICR7[15:0] is the [offset on the HOST LPC firmware address space/most
> fisnificant two bytes of the HOST LPC firmware address space] of the
> mapping

Yes, this appears correct.

> > +		 * firmware space address of the mapping.
> > +		 *
> > +		 * The 1 bits in the top of half of HICR8 represent
> > the bits
> > +		 * (in the requested address) that should be ignored
> > and
> > +		 * replaced with those from the top half of HICR7.
> 
> HICR8[31:16] represents a mask of the bits which should be taken from
> HICR7[31:16] and placed in the BMC address.

Yes.

> > +		 * The 1 bits in the bottom half of HICR8 represent
> > the bits
> > +		 * (in the requested address) that should be kept
> > and pass
> > +		 * into the BMC address space.
> 
> HICR8[15:0] represents a mask of the bits which should be taken from
> LPC_ADDR[31:16] and placed in the BMC address.
> 
> If I understand correctly, we basically end up with:
> 
> BMC_ADDR[31:0] = (BMC_OFFSET + LPC_OFFSET + INDEX) << 16 + OFFSET
> where:
> BMC_OFFSET = HICR7[31:16] & HICR8[31:16] - The offset into bmc memory
> of where the window is.
> LPC_OFFSET = LPC_ADDR[31:16] - HICR7[15:0] - The offset on the lpc bus
> of where we created the window.
> INDEX = LPC_ADDR[31:16] & HICR8[15:0] - High bits of the lpc address
> which we use in the bmc address.
> OFFSET = LPC_ADDR[15:0] - Low bits of the lpc address which we use in
> the bmc address.
> > +		 */

This sounds right to me.

> > +
> > +		/*
> > +		 * It doesn't make sense to talk about a size or
> > offset with
> > +		 * low 16 bits set. Both HICR7 and HICR8 talk about
> > the top 16
> > +		 * bits of addresses and sizes.
> > +		 */
> > +
> > +		if ((map.size & 0x0000ffff) || (map.offset &
> > 0x0000ffff))
> > +			return -EINVAL;
> 
> Am I missing something? Are you checking anywhere that map.size is
> power-of-2 aligned?
> > +

Not explicitly - I didn't want to be too restrictive. The conditions
only protect against exploits. Any other values should *work* but
produce interesting results. For example, it would be possible to map
the same 64K BMC region to multiple 64K regions on the host - why one
would do that I have no idea but you can.

> > +		/*
> > +		 * Because of the way the masks work in HICR8 offset
> > has to
> > +		 * be a multiple of size.
> > +		 */
> > +		if (map.offset & (map.size - 1))
> > +			return -EINVAL;
> > +
> > +		if (map.window_type == ASPEED_LPC_CTRL_WINDOW_FLASH)
> > {
> > +			addr = lpc_ctrl->pnor_base;
> > +			size = lpc_ctrl->pnor_size;
> > +		} else if (map.window_type ==
> > ASPEED_LPC_CTRL_WINDOW_MEMORY) {
> > +			addr = lpc_ctrl->mem_base;
> > +			size = lpc_ctrl->mem_size;
> > +		} else {
> > +			return -EINVAL;
> > +		}
> > +
> > +		/* Check overflow first! */
> > +		if (map.offset + map.size < map.offset ||
> > +			map.offset + map.size > size)
> > +			return -EINVAL;
> > +
> > +		if (map.size == 0 || map.size > size)
> > +			return -EINVAL;
> > +
> > +		addr += map.offset;
> > +
> > +		/*
> > +		 * addr (host lpc address) is safe regardless of
> > values. This
> > +		 * simply changes the address the host has to
> > request on its
> > +		 * side of the LPC bus. This cannot impact the hosts
> > own
> > +		 * memory space by surprise as LPC specific
> > accessors are
> > +		 * required. The only strange thing that could be
> > done is
> > +		 * setting the lower 16 bits but the shift takes
> > care of that.
> > +		 */
> 
> Don't you have to explicitly check that the bits 15:0 of map.addr
> aren't set? Otherwise something could ask to map LPC address 0x1234
> (map.addr = 0x1234), you will set HICR7[15:0] = 0 (map.addr >> 16). So
> when address 0x1234 is accessed on the lpc bus, this will map to bmc
> address 0x1234 instead of the expected 0x0 (since this is the base lpc
> address)?
> 

Yeah I didn't check, as you say the 0x1234 get thrown away by the
shift. It appears I forgot to explicitly say that that the smallest
valid value is 64K, I did say that about the size. I'd be surprised if
a user didn't know this already but if you feel strongly about it I can
resend with an error condition for it.

Thanks,

Cyril

> Or do we preclude the low 16 bits of map.addr from being set somewhere
> else?
> > +
> > +		rc = regmap_write(lpc_ctrl->regmap, HICR7,
> > +				(addr | (map.addr >> 16)));
> > +		if (rc)
> > +			return rc;
> > +
> > +		return regmap_write(lpc_ctrl->regmap, HICR8,
> > +			(~(map.size - 1)) | ((map.size >> 16) - 1));
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static const struct file_operations aspeed_lpc_ctrl_fops = {
> > +	.owner		= THIS_MODULE,
> > +	.mmap		= aspeed_lpc_ctrl_mmap,
> > +	.unlocked_ioctl	= aspeed_lpc_ctrl_ioctl,
> > +};
> > +
> > +static int aspeed_lpc_ctrl_probe(struct platform_device *pdev)
> > +{
> > +	struct aspeed_lpc_ctrl *lpc_ctrl;
> > +	struct device_node *node;
> > +	struct resource resm;
> > +	struct device *dev;
> > +	int rc;
> > +
> > +	dev = &pdev->dev;
> > +
> > +	lpc_ctrl = devm_kzalloc(dev, sizeof(*lpc_ctrl), GFP_KERNEL);
> > +	if (!lpc_ctrl)
> > +		return -ENOMEM;
> > +
> > +	node = of_parse_phandle(dev->of_node, "flash", 0);
> > +	if (!node) {
> > +		dev_err(dev, "Didn't find host pnor flash node\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	rc = of_address_to_resource(node, 1, &resm);
> > +	of_node_put(node);
> > +	if (rc) {
> > +		dev_err(dev, "Couldn't address to resource for
> > flash\n");
> > +		return rc;
> > +	}
> > +
> > +	lpc_ctrl->pnor_size = resource_size(&resm);
> > +	lpc_ctrl->pnor_base = resm.start;
> > +
> > +	dev_set_drvdata(&pdev->dev, lpc_ctrl);
> > +
> > +	node = of_parse_phandle(dev->of_node, "memory-region", 0);
> > +	if (!node) {
> > +		dev_err(dev, "Didn't find reserved memory\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	rc = of_address_to_resource(node, 0, &resm);
> > +	of_node_put(node);
> > +	if (rc) {
> > +		dev_err(dev, "Couldn't address to resource for
> > reserved memory\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	lpc_ctrl->mem_size = resource_size(&resm);
> > +	lpc_ctrl->mem_base = resm.start;
> > +
> > +	lpc_ctrl->regmap = syscon_node_to_regmap(
> > +			pdev->dev.parent->of_node);
> > +	if (IS_ERR(lpc_ctrl->regmap)) {
> > +		dev_err(dev, "Couldn't get regmap\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	lpc_ctrl->miscdev.minor = MISC_DYNAMIC_MINOR;
> > +	lpc_ctrl->miscdev.name = DEVICE_NAME;
> > +	lpc_ctrl->miscdev.fops = &aspeed_lpc_ctrl_fops;
> > +	lpc_ctrl->miscdev.parent = dev;
> > +	rc = misc_register(&lpc_ctrl->miscdev);
> > +	if (rc)
> > +		dev_err(dev, "Unable to register device\n");
> > +	else
> > +		dev_info(dev, "Loaded at 0x%08x (0x%08x)\n",
> > +			lpc_ctrl->mem_base, lpc_ctrl->mem_size);
> > +
> > +	return rc;
> > +}
> > +
> > +static int aspeed_lpc_ctrl_remove(struct platform_device *pdev)
> > +{
> > +	struct aspeed_lpc_ctrl *lpc_ctrl = dev_get_drvdata(&pdev-
> > > dev);
> > 
> > +
> > +	misc_deregister(&lpc_ctrl->miscdev);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id aspeed_lpc_ctrl_match[] = {
> > +	{ .compatible = "aspeed,ast2400-lpc-ctrl" },
> > +	{ .compatible = "aspeed,ast2500-lpc-ctrl" },
> > +	{ },
> > +};
> > +
> > +static struct platform_driver aspeed_lpc_ctrl_driver = {
> > +	.driver = {
> > +		.name		= DEVICE_NAME,
> > +		.of_match_table = aspeed_lpc_ctrl_match,
> > +	},
> > +	.probe = aspeed_lpc_ctrl_probe,
> > +	.remove = aspeed_lpc_ctrl_remove,
> > +};
> > +
> > +module_platform_driver(aspeed_lpc_ctrl_driver);
> > +
> > +MODULE_DEVICE_TABLE(of, aspeed_lpc_ctrl_match);
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Cyril Bur <cyrilbur@gmail.com>");
> > +MODULE_DESCRIPTION("Control for aspeed 2400/2500 LPC HOST to BMC
> > mappings");
> > diff --git a/include/uapi/linux/aspeed-lpc-ctrl.h
> > b/include/uapi/linux/aspeed-lpc-ctrl.h
> > new file mode 100644
> > index 000000000000..f96fa995a3f0
> > --- /dev/null
> > +++ b/include/uapi/linux/aspeed-lpc-ctrl.h
> > @@ -0,0 +1,60 @@
> > +/*
> > + * Copyright 2017 IBM Corp.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version
> > + * 2 of the License, or (at your option) any later version.
> > + */
> > +
> > +#ifndef _UAPI_LINUX_ASPEED_LPC_CTRL_H
> > +#define _UAPI_LINUX_ASPEED_LPC_CTRL_H
> > +
> > +#include <linux/ioctl.h>
> > +
> > +/* Window types */
> > +#define ASPEED_LPC_CTRL_WINDOW_FLASH	1
> > +#define ASPEED_LPC_CTRL_WINDOW_MEMORY	2
> > +
> > +/*
> > + * This driver provides a window for the host to access a BMC
> > resource
> > + * across the BMC <-> Host LPC bus.
> > + *
> > + * window_type: The BMC resource that the host will access through
> > the
> > + * window. BMC flash and BMC RAM.
> > + *
> > + * window_id: For each window type there may be multiple windows,
> > + * these are referenced by ID.
> > + *
> > + * flags: Reserved for future use, this field is expected to be
> > + * zeroed.
> > + *
> > + * addr: Address on the host LPC bus that the specified window
> > should
> > + * be mapped. This address must be power of two aligned.
> 
> If I understand correctly -> AND a multiple of size?
> > + *
> > + * offset: Offset into the BMC window that should be mapped to the
> > + * host (at addr). This must be a multiple of size.
> > + *
> > + * size: The size of the mapping. The smallest possible size is 64K.
> > + * This must be power of two aligned.
> > + *
> > + */
> > +
> > +struct aspeed_lpc_ctrl_mapping {
> > +	__u8	window_type;
> > +	__u8	window_id;
> > +	__u16	flags;
> > +	__u32	addr;
> > +	__u32	offset;
> > +	__u32	size;
> > +};
> > +
> > +#define __ASPEED_LPC_CTRL_IOCTL_MAGIC	0xb2
> > +
> > +#define ASPEED_LPC_CTRL_IOCTL_GET_SIZE	_IOWR(__ASPEED_LPC_CTR
> > L_IOCTL_MAGIC, \
> > +		0x00, struct aspeed_lpc_ctrl_mapping)
> > +
> > +#define ASPEED_LPC_CTRL_IOCTL_MAP	_IOW(__ASPEED_LPC_CTRL_IOCT
> > L_MAGIC, \
> > +		0x01, struct aspeed_lpc_ctrl_mapping)
> > +
> > +#endif /* _UAPI_LINUX_ASPEED_LPC_CTRL_H */

On Thu, 2017-03-02 at 12:01 +1100, Cyril Bur wrote:
> On Wed, 2017-02-22 at 16:11 +1100, Suraj Jitindar Singh wrote:
> > 
> > On Fri, 2017-02-17 at 14:28 +1100, Cyril Bur wrote:
> > 
> > I may be too late, but see below...
> > > 
> > > In order to manage server systems, there is typically another
> > > processor
> > > known as a BMC (Baseboard Management Controller) which is
> > > responsible
> > > for powering the server and other various elements, sometimes
> > > fans,
> > > often the system flash.
> > > 
> > > The Aspeed BMC family which is what is used on OpenPOWER machines
> > > and
> > > a
> > > number of x86 as well is typically connected to the host via an
> > > LPC
> > > (Low Pin Count) bus (among others).
> > > 
> > > The LPC bus is an ISA bus on steroids. It's generally used by the
> > > BMC chip to provide the host with access to the system flash (via
> > > MEM/FW
> > > cycles) that contains the BIOS or other host firmware along with
> > > a
> > > number of SuperIO-style IOs (via IO space) such as UARTs, IPMI
> > > controllers.
> > > 
> > > On the BMC chip side, this is all configured via a bunch of
> > > registers
> > > whose content is related to a given policy of what devices are
> > > exposed
> > > at a per system level, which is system/vendor specific, so we
> > > don't
> > > want
> > > to bolt that into the BMC kernel. This started with a need to
> > > provide
> > > something nicer than /dev/mem for user space to configure these
> > > things.
> > > 
> > > One important aspect of the configuration is how the MEM/FW space
> > > is
> > > exposed to the host (ie, the x86 or POWER). Some registers in
> > > that
> > > bridge can define a window remapping all or portion of the LPC
> > > MEM/FW
> > > space to a portion of the BMC internal bus, with no specific
> > > limits
> > > imposed in HW.
> > > 
> > > I think it makes sense to ensure that this window is configured
> > > by a
> > > kernel driver that can apply some serious sanity checks on what
> > > it is
> > > configured to map.
> > > 
> > > In practice, user space wants to control this by flipping the
> > > mapping
> > > between essentially two types of portions of the BMC address
> > > space:
> > > 
> > >    - The flash space. This is a region of the BMC MMIO space that
> > > more/less directly maps the system flash (at least for reads,
> > > writes
> > > are somewhat more complicated).
> > > 
> > >    - One (or more) reserved area(s) of the BMC physical memory.
> > > 
> > > The latter is needed for a number of things, such as avoiding
> > > letting
> > > the host manipulate the innards of the BMC flash controller via
> > > some
> > > evil backdoor, we want to do flash updates by routing the window
> > > to a
> > > portion of memory (under control of a mailbox protocol via some
> > > separate set of registers) which the host can use to write new
> > > data
> > > in
> > > bulk and then request the BMC to flash it. There are other uses,
> > > such
> > > as allowing the host to boot from an in-memory flash image rather
> > > than
> > > the one in flash (very handy for continuous integration and test,
> > > the
> > > BMC can just download new images).
> > > 
> > > It is important to note that due to the way the Aspeed chip lets
> > > the
> > > kernel configure the mapping between host LPC addresses and BMC
> > > ram
> > > addresses the offset within the window must be a multiple of
> > > size.
> > > Not doing so will fragment the accessible space rather than
> > > simply
> > > moving 'zero' upwards. This is caused by the nature of HICR8
> > > being a
> > > mask and the way host LPC addresses are translated.
> > > 
> > > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> > > ---
> > > v2:
> > >    Removed unused functions
> > >    Removed use of access_ok()
> > >    All input is evil
> > >    Reworked the interface as per Benjamin Herrenschmidts vision
> > > v3:
> > >    Removed 'default y' from Kconfig
> > >    Reordered ioctl() struct fields
> > >    Reworeded some comments
> > > v4:
> > >    Reorder ioctl() struct fields (again)
> > > v5:
> > >    Style cleanups
> > >    Use of_address_to_resource()
> > >    Document ioctl structure fields
> > > v6:
> > >    Check and fail ioctl() if flags field non-zero
> > > 
> > >  drivers/misc/Kconfig                 |   8 ++
> > >  drivers/misc/Makefile                |   1 +
> > >  drivers/misc/aspeed-lpc-ctrl.c       | 267
> > > +++++++++++++++++++++++++++++++++++
> > >  include/uapi/linux/aspeed-lpc-ctrl.h |  60 ++++++++
> > >  4 files changed, 336 insertions(+)
> > >  create mode 100644 drivers/misc/aspeed-lpc-ctrl.c
> > >  create mode 100644 include/uapi/linux/aspeed-lpc-ctrl.h
> > > 
> > > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> > > index 64971baf11fa..7dc4c369012f 100644
> > > --- a/drivers/misc/Kconfig
> > > +++ b/drivers/misc/Kconfig
> > > @@ -766,6 +766,14 @@ config PANEL_BOOT_MESSAGE
> > >  	  An empty message will only clear the display at driver
> > > init time. Any other
> > >  	  printf()-formatted message is valid with newline and
> > > escape codes.
> > >  
> > > +config ASPEED_LPC_CTRL
> > > +	depends on (ARCH_ASPEED || COMPILE_TEST) && REGMAP &&
> > > MFD_SYSCON
> > > +	tristate "Aspeed ast2400/2500 HOST LPC to BMC bridge
> > > control"
> > > +	---help---
> > > +	  Control Aspeed ast2400/2500 HOST LPC to BMC mappings
> > > through
> > > +	  ioctl()s, the driver also provides a read/write
> > > interface
> > > to a BMC ram
> > > +	  region where the host LPC read/write region can be
> > > buffered.
> > > +
> > >  source "drivers/misc/c2port/Kconfig"
> > >  source "drivers/misc/eeprom/Kconfig"
> > >  source "drivers/misc/cb710/Kconfig"
> > > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> > > index 31983366090a..de1925a9c80b 100644
> > > --- a/drivers/misc/Makefile
> > > +++ b/drivers/misc/Makefile
> > > @@ -53,6 +53,7 @@ obj-$(CONFIG_ECHO)		+= echo/
> > >  obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
> > >  obj-$(CONFIG_CXL_BASE)		+= cxl/
> > >  obj-$(CONFIG_PANEL)             += panel.o
> > > +obj-$(CONFIG_ASPEED_LPC_CTRL)	+= aspeed-lpc-ctrl.o
> > >  
> > >  lkdtm-$(CONFIG_LKDTM)		+= lkdtm_core.o
> > >  lkdtm-$(CONFIG_LKDTM)		+= lkdtm_bugs.o
> > > diff --git a/drivers/misc/aspeed-lpc-ctrl.c
> > > b/drivers/misc/aspeed-
> > > lpc-ctrl.c
> > > new file mode 100644
> > > index 000000000000..f6acbe1d9378
> > > --- /dev/null
> > > +++ b/drivers/misc/aspeed-lpc-ctrl.c
> > > @@ -0,0 +1,267 @@
> > > +/*
> > > + * Copyright 2017 IBM Corporation
> > > + *
> > > + * This program is free software; you can redistribute it and/or
> > > + * modify it under the terms of the GNU General Public License
> > > + * as published by the Free Software Foundation; either version
> > > + * 2 of the License, or (at your option) any later version.
> > > + */
> > > +
> > > +#include <linux/mfd/syscon.h>
> > > +#include <linux/miscdevice.h>
> > > +#include <linux/mm.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of_address.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/poll.h>
> > > +#include <linux/regmap.h>
> > > +
> > > +#include <linux/aspeed-lpc-ctrl.h>
> > > +
> > > +#define DEVICE_NAME	"aspeed-lpc-ctrl"
> > > +
> > > +#define HICR7 0x8
> > > +#define HICR8 0xc
> > > +
> > > +struct aspeed_lpc_ctrl {
> > > +	struct miscdevice	miscdev;
> > > +	struct regmap		*regmap;
> > > +	phys_addr_t		mem_base;
> > > +	resource_size_t		mem_size;
> > > +	u32		pnor_size;
> > > +	u32		pnor_base;
> > > +};
> > > +
> > > +static struct aspeed_lpc_ctrl *file_aspeed_lpc_ctrl(struct file
> > > *file)
> > > +{
> > > +	return container_of(file->private_data, struct
> > > aspeed_lpc_ctrl,
> > > +			miscdev);
> > > +}
> > > +
> > > +static int aspeed_lpc_ctrl_mmap(struct file *file, struct
> > > vm_area_struct *vma)
> > > +{
> > > +	struct aspeed_lpc_ctrl *lpc_ctrl =
> > > file_aspeed_lpc_ctrl(file);
> > > +	unsigned long vsize = vma->vm_end - vma->vm_start;
> > > +	pgprot_t prot = vma->vm_page_prot;
> > > +
> > > +	if (vma->vm_pgoff + vsize > lpc_ctrl->mem_base +
> > > lpc_ctrl-
> > > > 
> > > > mem_size)
> > > +		return -EINVAL;
> > > +
> > > +	/* ast2400/2500 AHB accesses are not cache coherent */
> > > +	prot = pgprot_dmacoherent(prot);
> > > +
> > > +	if (remap_pfn_range(vma, vma->vm_start,
> > > +		(lpc_ctrl->mem_base >> PAGE_SHIFT) + vma-
> > > >vm_pgoff,
> > > +		vsize, prot))
> > > +		return -EAGAIN;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned
> > > int
> > > cmd,
> > > +		unsigned long param)
> > > +{
> > > +	struct aspeed_lpc_ctrl *lpc_ctrl =
> > > file_aspeed_lpc_ctrl(file);
> > > +	void __user *p = (void __user *)param;
> > > +	struct aspeed_lpc_ctrl_mapping map;
> > > +	u32 addr;
> > > +	u32 size;
> > > +	long rc;
> > > +
> > > +	if (copy_from_user(&map, p, sizeof(map)))
> > > +		return -EFAULT;
> > > +
> > > +	if (map.flags != 0)
> > > +		return -EINVAL;
> > > +
> > > +	switch (cmd) {
> > > +	case ASPEED_LPC_CTRL_IOCTL_GET_SIZE:
> > > +		/* The flash windows don't report their size */
> > > +		if (map.window_type !=
> > > ASPEED_LPC_CTRL_WINDOW_MEMORY)
> > > +			return -EINVAL;
> > > +
> > > +		/* Support more than one window id in the future
> > > */
> > > +		if (map.window_id != 0)
> > > +			return -EINVAL;
> > > +
> > > +		map.size = lpc_ctrl->mem_size;
> > > +
> > > +		return copy_to_user(p, &map, sizeof(map)) ?
> > > -EFAULT
> > > : 0;
> > > +	case ASPEED_LPC_CTRL_IOCTL_MAP:
> > > +
> > > +		/*
> > > +		 * The top half of HICR7 is the MSB of the BMC
> > > address of the
> > > +		 * mapping.
> Hi Suraj,
> 
> I'm all for writing this out as many times as possible to be sure
> everyone understands it. I think a visual diagram would be fantastic,
> I'd love to do one in my infinite spare time. It may be useful to
> link
> to Benhs comments in skiboot for another wording of how these two
> registers work skiboot/hw/ast-bmc/ast-io.c.
> 
> > 
> > How about: HICR7[31:16] is the [offset into BMC memory/most
> > significant
> > two bytes fo the BMC address] of the mapping.
> > > 
> > > +		 * The bottom half of HICR7 is
> > > the MSB of the HOST LPC
> > HICR7[15:0] is the [offset on the HOST LPC firmware address
> > space/most
> > fisnificant two bytes of the HOST LPC firmware address space] of
> > the
> > mapping
> Yes, this appears correct.
> 
> > 
> > > 
> > > +		 * firmware space address of the mapping.
> > > +		 *
> > > +		 * The 1 bits in the top of half of HICR8
> > > represent
> > > the bits
> > > +		 * (in the requested address) that should be
> > > ignored
> > > and
> > > +		 * replaced with those from the top half of
> > > HICR7.
> > HICR8[31:16] represents a mask of the bits which should be taken
> > from
> > HICR7[31:16] and placed in the BMC address.
> Yes.
> 
> > 
> > > 
> > > +		 * The 1 bits in the bottom half of HICR8
> > > represent
> > > the bits
> > > +		 * (in the requested address) that should be
> > > kept
> > > and pass
> > > +		 * into the BMC address space.
> > HICR8[15:0] represents a mask of the bits which should be taken
> > from
> > LPC_ADDR[31:16] and placed in the BMC address.
> > 
> > If I understand correctly, we basically end up with:
> > 
> > BMC_ADDR[31:0] = (BMC_OFFSET + LPC_OFFSET + INDEX) << 16 + OFFSET
> > where:
> > BMC_OFFSET = HICR7[31:16] & HICR8[31:16] - The offset into bmc
> > memory
> > of where the window is.
> > LPC_OFFSET = LPC_ADDR[31:16] - HICR7[15:0] - The offset on the lpc
> > bus
> > of where we created the window.
> > INDEX = LPC_ADDR[31:16] & HICR8[15:0] - High bits of the lpc
> > address
> > which we use in the bmc address.
> > OFFSET = LPC_ADDR[15:0] - Low bits of the lpc address which we use
> > in
> > the bmc address.
> > > 
> > > +		 */
> This sounds right to me.
> 
> > 
> > > 
> > > +
> > > +		/*
> > > +		 * It doesn't make sense to talk about a size or
> > > offset with
> > > +		 * low 16 bits set. Both HICR7 and HICR8 talk
> > > about
> > > the top 16
> > > +		 * bits of addresses and sizes.
> > > +		 */
> > > +
> > > +		if ((map.size & 0x0000ffff) || (map.offset &
> > > 0x0000ffff))
> > > +			return -EINVAL;
> > Am I missing something? Are you checking anywhere that map.size is
> > power-of-2 aligned?
> > > 
> > > +
> Not explicitly - I didn't want to be too restrictive. The conditions
> only protect against exploits. Any other values should *work* but
> produce interesting results. For example, it would be possible to map
> the same 64K BMC region to multiple 64K regions on the host - why one
> would do that I have no idea but you can.
> 
> > 
> > > 
> > > +		/*
> > > +		 * Because of the way the masks work in HICR8
> > > offset
> > > has to
> > > +		 * be a multiple of size.
> > > +		 */
> > > +		if (map.offset & (map.size - 1))
> > > +			return -EINVAL;
> > > +
> > > +		if (map.window_type ==
> > > ASPEED_LPC_CTRL_WINDOW_FLASH)
> > > {
> > > +			addr = lpc_ctrl->pnor_base;
> > > +			size = lpc_ctrl->pnor_size;
> > > +		} else if (map.window_type ==
> > > ASPEED_LPC_CTRL_WINDOW_MEMORY) {
> > > +			addr = lpc_ctrl->mem_base;
> > > +			size = lpc_ctrl->mem_size;
> > > +		} else {
> > > +			return -EINVAL;
> > > +		}
> > > +
> > > +		/* Check overflow first! */
> > > +		if (map.offset + map.size < map.offset ||
> > > +			map.offset + map.size > size)
> > > +			return -EINVAL;
> > > +
> > > +		if (map.size == 0 || map.size > size)
> > > +			return -EINVAL;
> > > +
> > > +		addr += map.offset;
> > > +
> > > +		/*
> > > +		 * addr (host lpc address) is safe regardless of
> > > values. This
> > > +		 * simply changes the address the host has to
> > > request on its
> > > +		 * side of the LPC bus. This cannot impact the
> > > hosts
> > > own
> > > +		 * memory space by surprise as LPC specific
> > > accessors are
> > > +		 * required. The only strange thing that could
> > > be
> > > done is
> > > +		 * setting the lower 16 bits but the shift takes
> > > care of that.
> > > +		 */
> > Don't you have to explicitly check that the bits 15:0 of map.addr
> > aren't set? Otherwise something could ask to map LPC address 0x1234
> > (map.addr = 0x1234), you will set HICR7[15:0] = 0 (map.addr >> 16).
> > So
> > when address 0x1234 is accessed on the lpc bus, this will map to
> > bmc
> > address 0x1234 instead of the expected 0x0 (since this is the base
> > lpc
> > address)?
> > 
> Yeah I didn't check, as you say the 0x1234 get thrown away by the
> shift. It appears I forgot to explicitly say that that the smallest
> valid value is 64K, I did say that about the size. I'd be surprised
> if
> a user didn't know this already but if you feel strongly about it I 
I don't feel that strongly... :)
> can
> resend with an error condition for it.
> 
> Thanks,
> 
> Cyril
> 
> > 
> > Or do we preclude the low 16 bits of map.addr from being set
> > somewhere
> > else?
> > > 
> > > +
> > > +		rc = regmap_write(lpc_ctrl->regmap, HICR7,
> > > +				(addr | (map.addr >> 16)));
> > > +		if (rc)
> > > +			return rc;
> > > +
> > > +		return regmap_write(lpc_ctrl->regmap, HICR8,
> > > +			(~(map.size - 1)) | ((map.size >> 16) -
> > > 1));
> > > +	}
> > > +
> > > +	return -EINVAL;
> > > +}
> > > +
> > > +static const struct file_operations aspeed_lpc_ctrl_fops = {
> > > +	.owner		= THIS_MODULE,
> > > +	.mmap		= aspeed_lpc_ctrl_mmap,
> > > +	.unlocked_ioctl	= aspeed_lpc_ctrl_ioctl,
> > > +};
> > > +
> > > +static int aspeed_lpc_ctrl_probe(struct platform_device *pdev)
> > > +{
> > > +	struct aspeed_lpc_ctrl *lpc_ctrl;
> > > +	struct device_node *node;
> > > +	struct resource resm;
> > > +	struct device *dev;
> > > +	int rc;
> > > +
> > > +	dev = &pdev->dev;
> > > +
> > > +	lpc_ctrl = devm_kzalloc(dev, sizeof(*lpc_ctrl),
> > > GFP_KERNEL);
> > > +	if (!lpc_ctrl)
> > > +		return -ENOMEM;
> > > +
> > > +	node = of_parse_phandle(dev->of_node, "flash", 0);
> > > +	if (!node) {
> > > +		dev_err(dev, "Didn't find host pnor flash
> > > node\n");
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	rc = of_address_to_resource(node, 1, &resm);
> > > +	of_node_put(node);
> > > +	if (rc) {
> > > +		dev_err(dev, "Couldn't address to resource for
> > > flash\n");
> > > +		return rc;
> > > +	}
> > > +
> > > +	lpc_ctrl->pnor_size = resource_size(&resm);
> > > +	lpc_ctrl->pnor_base = resm.start;
> > > +
> > > +	dev_set_drvdata(&pdev->dev, lpc_ctrl);
> > > +
> > > +	node = of_parse_phandle(dev->of_node, "memory-region",
> > > 0);
> > > +	if (!node) {
> > > +		dev_err(dev, "Didn't find reserved memory\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	rc = of_address_to_resource(node, 0, &resm);
> > > +	of_node_put(node);
> > > +	if (rc) {
> > > +		dev_err(dev, "Couldn't address to resource for
> > > reserved memory\n");
> > > +		return -ENOMEM;
> > > +	}
> > > +
> > > +	lpc_ctrl->mem_size = resource_size(&resm);
> > > +	lpc_ctrl->mem_base = resm.start;
> > > +
> > > +	lpc_ctrl->regmap = syscon_node_to_regmap(
> > > +			pdev->dev.parent->of_node);
> > > +	if (IS_ERR(lpc_ctrl->regmap)) {
> > > +		dev_err(dev, "Couldn't get regmap\n");
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	lpc_ctrl->miscdev.minor = MISC_DYNAMIC_MINOR;
> > > +	lpc_ctrl->miscdev.name = DEVICE_NAME;
> > > +	lpc_ctrl->miscdev.fops = &aspeed_lpc_ctrl_fops;
> > > +	lpc_ctrl->miscdev.parent = dev;
> > > +	rc = misc_register(&lpc_ctrl->miscdev);
> > > +	if (rc)
> > > +		dev_err(dev, "Unable to register device\n");
> > > +	else
> > > +		dev_info(dev, "Loaded at 0x%08x (0x%08x)\n",
> > > +			lpc_ctrl->mem_base, lpc_ctrl->mem_size);
> > > +
> > > +	return rc;
> > > +}
> > > +
> > > +static int aspeed_lpc_ctrl_remove(struct platform_device *pdev)
> > > +{
> > > +	struct aspeed_lpc_ctrl *lpc_ctrl =
> > > dev_get_drvdata(&pdev-
> > > > 
> > > > dev);
> > > +
> > > +	misc_deregister(&lpc_ctrl->miscdev);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct of_device_id aspeed_lpc_ctrl_match[] = {
> > > +	{ .compatible = "aspeed,ast2400-lpc-ctrl" },
> > > +	{ .compatible = "aspeed,ast2500-lpc-ctrl" },
> > > +	{ },
> > > +};
> > > +
> > > +static struct platform_driver aspeed_lpc_ctrl_driver = {
> > > +	.driver = {
> > > +		.name		= DEVICE_NAME,
> > > +		.of_match_table = aspeed_lpc_ctrl_match,
> > > +	},
> > > +	.probe = aspeed_lpc_ctrl_probe,
> > > +	.remove = aspeed_lpc_ctrl_remove,
> > > +};
> > > +
> > > +module_platform_driver(aspeed_lpc_ctrl_driver);
> > > +
> > > +MODULE_DEVICE_TABLE(of, aspeed_lpc_ctrl_match);
> > > +MODULE_LICENSE("GPL");
> > > +MODULE_AUTHOR("Cyril Bur <cyrilbur@gmail.com>");
> > > +MODULE_DESCRIPTION("Control for aspeed 2400/2500 LPC HOST to BMC
> > > mappings");
> > > diff --git a/include/uapi/linux/aspeed-lpc-ctrl.h
> > > b/include/uapi/linux/aspeed-lpc-ctrl.h
> > > new file mode 100644
> > > index 000000000000..f96fa995a3f0
> > > --- /dev/null
> > > +++ b/include/uapi/linux/aspeed-lpc-ctrl.h
> > > @@ -0,0 +1,60 @@
> > > +/*
> > > + * Copyright 2017 IBM Corp.
> > > + *
> > > + * This program is free software; you can redistribute it and/or
> > > + * modify it under the terms of the GNU General Public License
> > > + * as published by the Free Software Foundation; either version
> > > + * 2 of the License, or (at your option) any later version.
> > > + */
> > > +
> > > +#ifndef _UAPI_LINUX_ASPEED_LPC_CTRL_H
> > > +#define _UAPI_LINUX_ASPEED_LPC_CTRL_H
> > > +
> > > +#include <linux/ioctl.h>
> > > +
> > > +/* Window types */
> > > +#define ASPEED_LPC_CTRL_WINDOW_FLASH	1
> > > +#define ASPEED_LPC_CTRL_WINDOW_MEMORY	2
> > > +
> > > +/*
> > > + * This driver provides a window for the host to access a BMC
> > > resource
> > > + * across the BMC <-> Host LPC bus.
> > > + *
> > > + * window_type: The BMC resource that the host will access
> > > through
> > > the
> > > + * window. BMC flash and BMC RAM.
> > > + *
> > > + * window_id: For each window type there may be multiple
> > > windows,
> > > + * these are referenced by ID.
> > > + *
> > > + * flags: Reserved for future use, this field is expected to be
> > > + * zeroed.
> > > + *
> > > + * addr: Address on the host LPC bus that the specified window
> > > should
> > > + * be mapped. This address must be power of two aligned.
> > If I understand correctly -> AND a multiple of size?
> > > 
> > > + *
> > > + * offset: Offset into the BMC window that should be mapped to
> > > the
> > > + * host (at addr). This must be a multiple of size.
> > > + *
> > > + * size: The size of the mapping. The smallest possible size is
> > > 64K.
> > > + * This must be power of two aligned.
> > > + *
> > > + */
> > > +
> > > +struct aspeed_lpc_ctrl_mapping {
> > > +	__u8	window_type;
> > > +	__u8	window_id;
> > > +	__u16	flags;
> > > +	__u32	addr;
> > > +	__u32	offset;
> > > +	__u32	size;
> > > +};
> > > +
> > > +#define __ASPEED_LPC_CTRL_IOCTL_MAGIC	0xb2
> > > +
> > > +#define ASPEED_LPC_CTRL_IOCTL_GET_SIZE	_IOWR(__ASPEED_LPC
> > > _CTR
> > > L_IOCTL_MAGIC, \
> > > +		0x00, struct aspeed_lpc_ctrl_mapping)
> > > +
> > > +#define ASPEED_LPC_CTRL_IOCTL_MAP	_IOW(__ASPEED_LPC_CTRL_
> > > IOCT
> > > L_MAGIC, \
> > > +		0x01, struct aspeed_lpc_ctrl_mapping)
> > > +
> > > +#endif /* _UAPI_LINUX_ASPEED_LPC_CTRL_H */

On Fri, Feb 17, 2017 at 05:44:57PM +1100, Benjamin Herrenschmidt wrote:
> On Fri, 2017-02-17 at 14:28 +1100, Cyril Bur wrote:
> > In order to manage server systems, there is typically another
> > processor
> > known as a BMC (Baseboard Management Controller) which is responsible
> > for powering the server and other various elements, sometimes fans,
> > often the system flash.
> 
>  .../...
> 
> > It is important to note that due to the way the Aspeed chip lets the
> > kernel configure the mapping between host LPC addresses and BMC ram
> > addresses the offset within the window must be a multiple of size.
> > Not doing so will fragment the accessible space rather than simply
> > moving 'zero' upwards. This is caused by the nature of HICR8 being a
> > mask and the way host LPC addresses are translated.
> > 
> > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> 
> Reviewed-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> Greg, quick Q for you:
> 
> We will need to also add a mechanism to poke at a few registers
> in the same LPC controllers from userspace.
> 
> Mostly those are "scratch" registers that the BMC is supposed to set to
> specific values to indicates features it supports etc... (or to enable 
> the host BIOS to run in some kind of verbose debug mode).
> 
> Think of it as a communication channel between the BMC and the BIOS
> running on the host.
> 
> The BMC userspace needs to set them to various platform specific
> values as appropriate for a given vendor/system (userspace policy
> from the BMC perspective) during boot and/or change them as the result
> of some user actions (enable debug mode) etc...
> 
> The question here is read/write or sysfs files attached to the
> sysfs node ?
> 
> The former means userspace needs to "seek" to the right magic
> offset to write to one of them which is non-trivial to do from
> simple shell scripts but is a more natural interface.
> 
> It also means the owner/permissions of the /dev entry as set
> by uboot apply which can be nice.
> 
> The latter can expose each register by its name which provides
> a nice way to echo in/out of them from a shell script.
> 
> The drawback is that pretty much restricts access to root.
> 
> What do you recommend ?

That wasn't a quick question :)

I really have no idea, sorry, try some things out and see what seems to
work.

greg k-h